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.

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

Fixing vSphere Cloud Provider to use "vsphere-cloud-provider" to create ClientBuilder #57286

Merged

Conversation

rjog
Copy link

@rjog rjog commented Dec 17, 2017

What this PR does / why we need it:
vSphere cloud Provider is not using lower case naming while creating clientBuilder.
With this fix, ClientBuilder is created using lowercase naming.
With mixed upper-lower case name, controller manager is crashing.

Which issue(s) this PR fixes
Fixes # #57279

Special notes for your reviewer:
None

Release note:

Fixes controller manager crash in certain vSphere cloud provider environment.

…ud Provider

is not using lower case naming while creating clientBuilder.
With this fix, ClientBuilder is created using lowercase naming.
@k8s-ci-robot k8s-ci-robot added 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. labels Dec 17, 2017
@k8s-ci-robot k8s-ci-robot added needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels Dec 17, 2017
@rjog
Copy link
Author

rjog commented Dec 17, 2017

/assign @abrarshivani

@dims
Copy link
Member

dims commented Dec 17, 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 Dec 17, 2017
@liggitt
Copy link
Member

liggitt commented Dec 17, 2017

CI isn't going to do much for us here, since this was failing as-is and wasn't caught by CI

@dims
Copy link
Member

dims commented Dec 17, 2017

totally @liggitt we'd need that just to keep the bots happy (and get the thing merged eventually). Not sure if there is a way to do this without kicking off the tests for folks who are not maintainers

@abrarshivani
Copy link
Contributor

@rohitjogvmw Where you able to produce same failure scenario?

@abrarshivani
Copy link
Contributor

@rohitjogvmw I was able to succeed in deploying a K8s cluster with official google container image (gcr.io/google_containers/hyperkube-amd64:v1.9.0) and cloud provider as vsphere.

> kubectl version
Client Version: version.Info{Major:"1", Minor:"9", GitVersion:"v1.9.0", GitCommit:"925c127ec6b946659ad0fd596fa959be43f0cc05", GitTreeState:"clean", BuildDate:"2017-12-15T21:07:38Z", GoVersion:"go1.9.2", Compiler:"gc", Platform:"linux/amd64"}
Server Version: version.Info{Major:"1", Minor:"9", GitVersion:"v1.9.0", GitCommit:"925c127ec6b946659ad0fd596fa959be43f0cc05", GitTreeState:"clean", BuildDate:"2017-12-15T20:55:30Z", GoVersion:"go1.9.2", Compiler:"gc", Platform:"linux/amd64"}

@andremarianiello
Copy link

@abrarshivani I also ran into this issue. kubectl version also works for me, because this issue affects the controller manager, not the api server. If you look, you should see that the controller manager has crashed and that no service accounts, including vsphere's, have been created.

kubectl get serviceaccounts --all-namespaces

@rjog
Copy link
Author

rjog commented Dec 29, 2017

Hi @abrarshivani,
Issue is getting solved with my change.
Can you please approve the PR?

@abrarshivani
Copy link
Contributor

@andremarianiello For me controller manager is up all the time. It seems that @rohitjogvmw has successfully tested and verified the fix.

@rohitjogvmw Sure, I will approve it. BTW, just curious to know how you were able to reproduce it?

@abrarshivani
Copy link
Contributor

/approve

@k8s-ci-robot k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Dec 29, 2017
@andremarianiello
Copy link

@abrarshivani what is the name of the created vsphere service account on your healthy cluster?

@rjog
Copy link
Author

rjog commented Dec 30, 2017

@abrarshivani, my change is confirmed by @andremarianiello.
Can you also add lgtm label?

@abrarshivani
Copy link
Contributor

abrarshivani commented Dec 30, 2017

@andremarianiello Thanks for validating the change. Following are the pods running on Kubernetes cluster configured with vsphere cloud provider.

> kubectl get pods --all-namespaces
NAMESPACE     NAME                                        READY     STATUS    RESTARTS   AGE
kube-system   etcd-server-kubernetes-master               1/1       Running   0          13m
kube-system   heapster-v1.2.0-757f46b777-9nw62            2/2       Running   0          14m
kube-system   kube-apiserver-kubernetes-master            1/1       Running   0          14m
kube-system   kube-controller-manager-kubernetes-master   1/1       Running   0          13m
kube-system   kube-dns-v19-qptlp                          3/3       Running   0          14m
kube-system   kube-dns-v19-wcpb5                          3/3       Running   0          14m
kube-system   kube-proxy-gh527                            1/1       Running   0          14m
kube-system   kube-proxy-gwsxv                            1/1       Running   0          14m
kube-system   kube-proxy-r5k65                            1/1       Running   0          14m
kube-system   kube-scheduler-kubernetes-master            1/1       Running   0          13m
kube-system   kubernetes-dashboard-6ff67cd6-5fcz9         1/1       Running   0          14m

Following are the service accounts,

> kubectl get serviceaccounts --all-namespaces
NAMESPACE     NAME      SECRETS   AGE
default       default   1         12m
kube-public   default   1         12m
kube-system   default   1         12m

K8s cluster version:

> kubectl version
Client Version: version.Info{Major:"1", Minor:"9", GitVersion:"v1.9.0", GitCommit:"925c127ec6b946659ad0fd596fa959be43f0cc05", GitTreeState:"clean", BuildDate:"2017-12-15T21:07:38Z", GoVersion:"go1.9.2", Compiler:"gc", Platform:"linux/amd64"}
Server Version: version.Info{Major:"1", Minor:"9", GitVersion:"v1.9.0", GitCommit:"925c127ec6b946659ad0fd596fa959be43f0cc05", GitTreeState:"clean", BuildDate:"2017-12-15T20:55:30Z", GoVersion:"go1.9.2", Compiler:"gc", Platform:"linux/amd64"}

@abrarshivani
Copy link
Contributor

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Dec 30, 2017
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: abrarshivani, rohitjogvmw

Associated issue: #57279

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

@rjog
Copy link
Author

rjog commented Dec 30, 2017

/test pull-kubernetes-e2e-gce

@k8s-github-robot
Copy link

/test all [submit-queue is verifying that this PR is safe to merge]

@k8s-github-robot
Copy link

Automatic merge from submit-queue. If you want to cherry-pick this change to another branch, please follow the instructions here.

@k8s-github-robot k8s-github-robot merged commit da9a4d5 into kubernetes:master Dec 30, 2017
k8s-github-robot pushed a commit that referenced this pull request Jan 9, 2018
…-upstream-release-1.9

Automatic merge from submit-queue.

Automated cherry pick of #57286 upstream release-1.9 

Cherry pick of #57286 on release-1.9
#57286: Fixing vSphere Cloud Provider to use "vsphere-cloud-provider" to create ClientBuilder

**Release note:**
```
This fixes controller manager crash in certain vSphere cloud provider environment.
```

cc: @rohitjogvmw @abrarshivani
@rjog rjog deleted the controller-mgr-fix branch January 30, 2018 18:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. 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
Development

Successfully merging this pull request may close these issues.

None yet

9 participants