Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Reverting deprecation of vCenter port in vSphere Cloud Provider #49689

Merged
merged 1 commit into from
Jul 28, 2017
Merged

Reverting deprecation of vCenter port in vSphere Cloud Provider #49689

merged 1 commit into from
Jul 28, 2017

Conversation

divyenpatel
Copy link
Member

@divyenpatel divyenpatel commented Jul 27, 2017 •
edited by enisoc
Loading

What this PR does / why we need it:
This PR reverts back the deprecation of vCenter port made with PR: #42024
We observed an issue when kubernetes cluster is deployed on the vCenter with non-default Port.
When VC is configured with non default port govmomi requests were going to default port 443. This is not valid, as confirmed in vmware/govmomi#802

Which issue this PR fixes
fixes # vmware-archive#220

Special notes for your reviewer:
Verified with deploying Kubernetes Cluster on the vCenter with port (7443).
Used following vsphere.conf file on master and worker nodes

# cat vsphere.conf
[Global]
        user = "[email protected]"
        password = "Admin!23"
        server = "10.192.87.209"
        port = "7443"
        insecure-flag = "1"
        datacenter = "Datacenter"
        datastore = "storage1"
        working-dir = "kubernetes"
[Disk]
	scsicontrollertype = pvscsi

Created Storage Class, PV, PVC and Pod. Verified Attach, Detach, Create and Delete Volume Operations are working as expected.

Also verified with completely removing port from vsphere.conf file. default 443 is used in this case.

# cat vsphere.conf
[Global]
        user = "[email protected]"
        password = "Admin!23"
        server = "10.192.66.216"
        insecure-flag = "1"
        datacenter = "Datacenter"
        datastore = "storage1"
        working-dir = "kubernetes"
[Disk]
	scsicontrollertype = pvscsi

Release note:

Revert deprecation of vCenter port in vSphere Cloud Provider.

@luomiao @BaluDontu @tusharnt @rohitjogvmw

@k8s-ci-robot k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Jul 27, 2017
@k8s-ci-robot
Copy link
Contributor

Hi @divyenpatel. Thanks for your PR.

I'm waiting for a kubernetes member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

I understand the commands that are listed here.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.

@k8s-ci-robot k8s-ci-robot added the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label Jul 27, 2017
@divyenpatel divyenpatel changed the title Reverting deprecation of vCenter port Reverting deprecation of vCenter port in vSphere Cloud Provider Jul 27, 2017
@k8s-github-robot k8s-github-robot added approved Indicates a PR has been approved by an approver from all required OWNERS files. size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. release-note-none Denotes a PR that doesn't merit a release note. labels Jul 27, 2017
@liggitt
Copy link
Member

liggitt commented Jul 27, 2017

should do an actual revert of https://github.com/kubernetes/kubernetes/pull/42024/files restoring the envvar and tests as well

@divyenpatel divyenpatel force-pushed the revert-deprecation-of-vcenter-port branch 2 times, most recently from 2134159 to 411a1a8 Compare July 27, 2017 13:45
@divyenpatel
Copy link
Member Author

@liggitt thanks for catching this. Reverted changes for tests as well.

Check condition for vCenter port must be 443 in the vsphere/vsphere_test.go is not required. so not reverting that back.

@liggitt
Copy link
Member

liggitt commented Jul 27, 2017

/ok-to-test

@k8s-ci-robot k8s-ci-robot removed the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label Jul 27, 2017
@liggitt
Copy link
Member

liggitt commented Jul 27, 2017

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jul 27, 2017
@liggitt liggitt added this to the v1.7 milestone Jul 27, 2017
@liggitt
Copy link
Member

liggitt commented Jul 27, 2017

cc @luomiao @jingxu97 from original PR

@k8s-github-robot
Copy link

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: divyenpatel, liggitt

Associated issue: 42024

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these OWNERS Files:

You can indicate your approval by writing /approve in a comment
You can cancel your approval by writing /approve cancel in a comment

@luomiao
Copy link

luomiao commented Jul 27, 2017

@divyenpatel
Thank you for exploring this issue!
LGTM

@divyenpatel
Copy link
Member Author

/test pull-kubernetes-kubemark-e2e-gce

@k8s-github-robot
Copy link

Automatic merge from submit-queue (batch tested with PRs 49665, 49689, 49495, 49146, 48934)

@k8s-github-robot k8s-github-robot merged commit 400ce15 into kubernetes:master Jul 28, 2017
@wojtek-t
Copy link
Member

@divyenpatel - to have this cherrypicked, please add a release note to this PR.

This was referenced Jul 28, 2017
Closed
Merged
Merged
@enisoc enisoc added release-note Denotes a PR that will be considered when it comes time to generate release notes. and removed release-note-none Denotes a PR that doesn't merit a release note. labels Jul 28, 2017
k8s-github-robot pushed a commit that referenced this pull request Jul 28, 2017
Merge pull request #49790 from vmware/automated-cherry-pick-of-#49689…
fc4902f 
…-upstream-release-1.6

Automatic merge from submit-queue

Automated cherry pick of #49689 upstream release 1.6

Cherry pick of #49689 on release-1.6

#49689: Reverting deprecation of vCenter port in vSphere Cloud Provider

@luomiao @BaluDontu @tusharnt
@k8s-cherrypick-bot
Copy link

Commit found in the "release-1.7" branch appears to be this PR. Removing the "cherrypick-candidate" label. If this is an error find help to get your PR picked.

@wojtek-t wojtek-t added the cherry-pick-approved Indicates a cherry-pick PR into a release branch has been approved by the release branch manager. label Aug 1, 2017
k8s-github-robot pushed a commit that referenced this pull request Aug 1, 2017
Merge pull request #49791 from vmware/automated-cherry-pick-of-#49689…
e047a5e 
…-upstream-release-1.7

Automatic merge from submit-queue

Automated cherry pick of #49689 upstream release 1.7

Cherry pick of #49689 on release-1.7

#49689: Reverting deprecation of vCenter port in vSphere Cloud Provider

@luomiao @BaluDontu @tusharnt
@k8s-cherrypick-bot
Copy link

Commit found in the "release-1.7" branch appears to be this PR. Removing the "cherrypick-candidate" label. If this is an error find help to get your PR picked.

@divyenpatel divyenpatel deleted the revert-deprecation-of-vcenter-port branch September 5, 2017 23:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees

@liggitt liggitt

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. cherry-pick-approved Indicates a cherry-pick PR into a release branch has been approved by the release branch manager. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. lgtm "Looks good to me", indicates that a PR is ready to be merged. release-note Denotes a PR that will be considered when it comes time to generate release notes. size/XS Denotes a PR that changes 0-9 lines, ignoring generated files.
Projects
None yet
Milestone
v1.7
Development

Successfully merging this pull request may close these issues.
8 participants
@divyenpatel @k8s-ci-robot @liggitt @k8s-github-robot @luomiao @wojtek-t @k8s-cherrypick-bot @enisoc