Add zones support for vSphere cloud provider(in-tree) #66795

jiatongw · 2018-07-30T19:27:03Z

What this PR does / why we need it:
This PR added zones(built-in node labels) support for vSphere cloud provider(in-tree). More details can be found in the issue as below.

Which issue(s) this PR fixes :
Partially fixes phase 1 of issue #64021

Special notes for your reviewer:

Release note:

Add zones support for vSphere cloud provider

jiatongw · 2018-07-30T19:37:37Z

/assign @dougm

jiatongw · 2018-07-30T19:45:07Z

pkg/cloudprovider/providers/vsphere/vsphere.go

+func (vs *VSphere) GetZoneByProviderID(ctx context.Context, providerID string) (cloudprovider.Zone, error) {
+
+	return cloudprovider.Zone{}, nil
+}


GetZoneByNodeName() and GetZoneByProviderID() are called by out-tree cloud provider, which have not been tested yet. I have completed the code for those two funcs but for now I leave it just by returning an empty zone. If this is not a good way, please comment.

neolit123 · 2018-07-30T21:54:21Z

@kubernetes/sig-cloud-provider-pr-reviews
@kubernetes/vmware

neolit123

thanks for the PR @jiatongw
i've added some comments, mostly about whitespace.

re: [2]
i see that the files use glog.Errorf() instead of propagating errors back to the caller which isn't exactly a good practice.
feel free to ignore this for now, but this type of glog binding is undesired and possibly needs a cleanup in a separate PR.

thanks.

neolit123 · 2018-07-30T21:55:20Z

pkg/cloudprovider/providers/vsphere/vclib/datacenter.go

+	svm, err := s.FindByUuid(ctx, dc.Datacenter, vmUUID, true, nil)
+	if err != nil {
+		glog.Errorf("Failed to find VM by UUID. VM UUID: %s, err: %+v", vmUUID, err)
+		return nil, err


would it be better to return the above error using fmt.Errorf() instead of calling glog.Errorf?
[2]

thanks for catching this! But looking at other code in this file, it seems that glog.Errorf is using more frequently. @dougm there are 44 places that using glog.Errof. What do you think about this or fmt.Errorf?

There are cases in VCP where return fmt.Errorf is used too. I think its ok to leave as glog.Errorf as it can be useful to have the filename:linenumber info that it provides. We should probably have a separate issue to clean up the the logging in VCP. In this particular case, looks like this code should be calling Datacenter.GetVMByUUID instead as I commented below.

neolit123 · 2018-07-30T21:55:42Z

pkg/cloudprovider/providers/vsphere/vclib/tagmanagers/tagmanegers.go

+	defer t.Client.Logout(ctx)
+
+	return f(t.Client)
+


please, remove this empty line.
[1]

neolit123 · 2018-07-30T21:56:19Z

pkg/cloudprovider/providers/vsphere/vclib/tagmanagers/tagmanegers.go

+}
+
+func (t TagManagers) WithClient(ctx context.Context, f func(client *tags.RestClient) error) error {
+	err := t.Client.Login(ctx)


can be changed to:

if err := t.Client.Login(ctx); err != nil { ...

neolit123 · 2018-07-30T21:56:36Z

pkg/cloudprovider/providers/vsphere/vsphere.go

+
+	zone := cloudprovider.Zone{}
+	vsi, err := vs.getVSphereInstanceForServer(vs.cfg.Workspace.VCenterIP, ctx)
+


neolit123 · 2018-07-30T21:57:11Z

pkg/cloudprovider/providers/vsphere/vsphere.go

+			ctx,
+			vmHost,
+		)
+


neolit123 · 2018-07-30T22:00:53Z

pkg/cloudprovider/providers/vsphere/vsphere.go

+	dc, err := vclib.GetDatacenter(ctx, vsi.conn, vs.cfg.Workspace.Datacenter)
+	if err != nil {
+		glog.Errorf("Cannot connect to datacenter. Get zone for node %s error", nodeName)
+		return cloudprovider.Zone{}, err


neolit123 · 2018-07-30T22:00:56Z

pkg/cloudprovider/providers/vsphere/vsphere.go

+
+	if err != nil {
+		glog.Errorf("Cannot connect to vsphere. Get zone for node %s error", nodeName)
+		return cloudprovider.Zone{}, err


neolit123 · 2018-07-30T22:01:01Z

pkg/cloudprovider/providers/vsphere/vsphere.go

+	vmHost, err := dc.GetHostByVMUUID(ctx, vs.vmUUID)
+	if err != nil {
+		glog.Errorf("Cannot find VM runtime host. Get zone for node %s error", nodeName)
+		return cloudprovider.Zone{}, err


neolit123 · 2018-07-30T22:01:07Z

pkg/cloudprovider/providers/vsphere/vsphere.go

+			tag, err := client.GetTag(ctx, value)
+			if err != nil {
+				glog.Errorf("Get tag %s error", value)
+				return err


neolit123 · 2018-07-30T22:01:13Z

pkg/cloudprovider/providers/vsphere/vsphere.go

+			category, err := client.GetCategory(ctx, tag.CategoryID)
+			if err != nil {
+				glog.Errorf("Get category %s error", value)
+				return err


dougm

@jiatongw looking good. You'll need to restore the Godeps/LICENSES files, not sure why there are "103,568 deletions"?

dougm · 2018-07-30T23:48:03Z

pkg/cloudprovider/providers/vsphere/vclib/datacenter.go

+		glog.Errorf("Unable to find VM by UUID. VM UUID: %s", vmUUID)
+		return nil, ErrNoVMFound
+	}
+	virtualMachine := VirtualMachine{object.NewVirtualMachine(dc.Client(), svm.Reference()), dc}


Looks like you can use the existing Datacenter.GetVMByUUID method here instead of copy-n-paste.

dougm · 2018-07-30T23:55:19Z

pkg/cloudprovider/providers/vsphere/vclib/tagmanagers/tagmanegers.go

+	return TagManagers{Client: tags.NewClient(vsURL, connection.Insecure, "")}
+}
+
+func (t TagManagers) WithClient(ctx context.Context, f func(client *tags.RestClient) error) error {


Typo in file name: tagmanegers.go

Looks like this should just be a func in vclib, rather than a new package. And longer term will go away when vmware/govmomi#1179 is fixed.

Ok. So it's good that I move this file into vclib root floder and delete tagmanagers folder?

func withTagsClient in vsphere.go should be fine. No need for the TagManagers type. If you do feel there's a need for the type, let's call it tagsClient instead of TagManagers. And no need for a separate file as this function will go away when vmware/govmomi#1179 is fixed.

jiatongw · 2018-08-02T08:19:42Z

pkg/cloudprovider/providers/vsphere/vclib/datacenter.go

+// GetHostByVMUUID gets the host object from the given vmUUID
+func (dc *Datacenter) GetHostByVMUUID(ctx context.Context, vmUUID string) (*types.ManagedObjectReference, error) {
+	virtualMachine, err := dc.GetVMByUUID(ctx, vmUUID)
+	var vmMo mo.VirtualMachine


Called dc.GetVMByUUID() here

jiatongw · 2018-08-02T08:22:05Z

Ready for review. Added withTagsClient func in vsphere.go and remove tagmanagers/ folder.
@dougm

dougm · 2018-08-02T15:59:08Z

pkg/cloudprovider/providers/vsphere/vsphere.go

@@ -808,8 +816,7 @@ func (vs *VSphere) LoadBalancer() (cloudprovider.LoadBalancer, bool) {

 // Zones returns an implementation of Zones for vSphere.
 func (vs *VSphere) Zones() (cloudprovider.Zones, bool) {
-	glog.V(1).Info("The vSphere cloud provider does not support zones")
-	return nil, false
+	return vs, true


As we are discussing if vcp config should be required on the nodes, maybe we should return false here if there is no vcenter configured?

you mean no vcp.conf file or no [labels] field in vcp.conf file?
If it's the second case, since the kubelet code will detect whether the zone and region is "" or not. If it's empty, the kubelet code will handle this. So we can just return vs, true here.

I was thinking no vcenter == if vs.cfg.Workspace.VCenterIP == "": return false

If there is no vcp.conf file, Zones() maybe even not called since get node return No resources found

I see. one more concern, Should we also apply this check to other functions/interfaces in vsphere.go which they use vcp.conf file?

Ok. I see there are other similar checks like this already applied in vsphere.go . What about
if vs.cfg == "": return false? make it more simple

Yes, checking vs.cfg == nil should work fine.

dougm · 2018-08-02T16:00:56Z

pkg/cloudprovider/providers/vsphere/vsphere.go

+}
+
+func (vs *VSphere) GetZoneByNodeName(ctx context.Context, nodeName k8stypes.NodeName) (cloudprovider.Zone, error) {
+	return cloudprovider.Zone{}, nil


Should probably return cloudprovider.NotImplemented instead of nil here.

dougm · 2018-08-02T16:01:01Z

pkg/cloudprovider/providers/vsphere/vsphere.go

+}
+
+func (vs *VSphere) GetZoneByProviderID(ctx context.Context, providerID string) (cloudprovider.Zone, error) {
+	return cloudprovider.Zone{}, nil


Should probably return cloudprovider.NotImplemented instead of nil here.

dougm · 2018-08-02T16:02:04Z

pkg/cloudprovider/providers/vsphere/vsphere.go

+	}
+	client := vsi.conn
+	err = withTagsClient(ctx, client, func(client *tags.RestClient) error {
+		tags, err := client.ListAttachedTags(


This a short parameter list, no need for multiple lines: client.ListAttachedTags(ctx, vmHost)

jiatongw · 2018-08-03T20:42:05Z

pkg/cloudprovider/providers/vsphere/vsphere.go

+	client := vsi.conn
+	err = withTagsClient(ctx, client, func(client *tags.RestClient) error {
+		tags, err := client.ListAttachedTags(ctx, vmHost)
+		if err != nil {


Changed to one line arguments

And removed all not necessary blank lines, fixed problems in the above conversations.
@dougm

jiatongw · 2018-08-03T20:52:16Z

For those who review this PR, my changes on k8s code are the file vsphere.go and vclib/datacenter.go. The other 40 changes of file are because I updated the vmware/govmomi vendor and used Godep. So you can ignore those file changes when reviewing this PR

dougm

/ok-to-test
/approve

dougm · 2018-08-03T22:55:56Z

pkg/cloudprovider/providers/vsphere/vclib/datacenter.go

+		return nil, err
+	}
+	host := vmMo.Summary.Runtime.Host
+	glog.Infof("%s host is %s", virtualMachine.Reference(), vmMo.Summary.Runtime.Host)


You can make use of the host variable here too.

Zones implementation for vSphere cloud provider needs dependencies which are not included in current vmware/govmomi vendor. So this update added "vapi" package to support zones.

k8s-ci-robot · 2018-08-04T05:18:27Z

@jiatongw: GitHub didn't allow me to assign the following users: dep-approvers.

Note that only kubernetes members and repo collaborators can be assigned.
For more information please see the contributor guide

In response to this:

/assign @dep-approvers

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.

cblecker · 2018-08-06T21:01:40Z

/ok-to-test

jiatongw · 2018-08-06T21:24:26Z

/test pull-kubernetes-bazel-test

cblecker · 2018-08-06T21:44:09Z

@jiatongw This is a legitimate test failure. Please review.

jiatongw · 2018-08-06T21:58:41Z

@cblecker Yes, I have to modify the test func in _test.go file

jiatongw · 2018-08-06T22:45:44Z

@dougm Modified TestZone() in vsphere_test.go. We need test both GetZone() and Zones() interface here. Currently the we don't have simulator support, so the GetZone() will return an empty zone and nil error. Zones() will return true because we implemented this interface.

dougm · 2018-08-06T22:54:38Z

@jiatongw yes, that's good for now. I am currently working on vcsim support for the tags API.

jiatongw · 2018-08-06T23:45:20Z

@cblecker tests passed. Please have a review, thanks!

dougm · 2018-08-07T22:37:38Z

/lgtm

cblecker · 2018-08-07T22:43:09Z

/approve
Approving for godeps.

k8s-ci-robot · 2018-08-07T22:44:01Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: cblecker, dougm, jiatongw

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

The pull request process is described here

Needs approval from an approver in each of these files:

~~Godeps/OWNERS~~ [cblecker]
~~pkg/cloudprovider/providers/vsphere/OWNERS~~ [dougm]
~~vendor/OWNERS~~ [cblecker]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

k8s-github-robot · 2018-08-08T00:16:02Z

Automatic merge from submit-queue (batch tested with PRs 67052, 67094, 66795). If you want to cherry-pick this change to another branch, please follow the instructions here.

- Add tests for GetZones() - Fix bug where a host tag other than region or zone caused an error - Fix bug where GetZones() errored if zone tag was set, but region was not Follow up to PR kubernetes#66795 / towards kubernetes#64021

Automatic merge from submit-queue (batch tested with PRs 66980, 67604, 67741, 67715). If you want to cherry-pick this change to another branch, please follow the instructions <a href="https://github.com/kubernetes/community/blob/master/contributors/devel/cherry-picks.md">here</a>. vsphere: add tests for Cloud Provider Zones implementation **What this PR does / why we need it**: - Add tests for GetZones() - Fix bug where a host tag other than region or zone caused an error - Fix bug where GetZones() errored if zone tag was set, but region was not Follow up to PR #66795 / towards #64021 **Release note**: ```release-note NONE ```

k8s-ci-robot requested review from freehan and lavalamp July 30, 2018 19:27

k8s-ci-robot assigned dougm Jul 30, 2018

jiatongw commented Jul 30, 2018

View reviewed changes

jiatongw force-pushed the zones_vendor branch from 0e6eb01 to 06f433c Compare July 30, 2018 19:58

k8s-ci-robot added the sig/cloud-provider Categorizes an issue or PR as relevant to SIG Cloud Provider. label Jul 30, 2018

neolit123 reviewed Jul 30, 2018

View reviewed changes

dougm reviewed Jul 30, 2018

View reviewed changes

jiatongw force-pushed the zones_vendor branch 2 times, most recently from e16d6e3 to bbd02b7 Compare August 2, 2018 08:15

jiatongw commented Aug 2, 2018

View reviewed changes

dougm reviewed Aug 2, 2018

View reviewed changes

Add zones support for vSphere cloud provider (in-tree)

99abd4b

jiatongw force-pushed the zones_vendor branch from bbd02b7 to d9d412e Compare August 3, 2018 20:28

jiatongw commented Aug 3, 2018

View reviewed changes

dougm approved these changes Aug 3, 2018

View reviewed changes

Update vmware/govmomi vendor: add vapi package

5c44fd8

Zones implementation for vSphere cloud provider needs dependencies which are not included in current vmware/govmomi vendor. So this update added "vapi" package to support zones.

jiatongw force-pushed the zones_vendor branch from d9d412e to 5c44fd8 Compare August 4, 2018 05:13

k8s-ci-robot assigned cblecker Aug 6, 2018

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 Aug 6, 2018

Add TestZone()

af5084e

frapposelli mentioned this pull request Aug 7, 2018

Port Zones support to out-of-tree CCM kubernetes/cloud-provider-vsphere#36

Closed

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Aug 7, 2018

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Aug 7, 2018

k8s-github-robot merged commit 9d260ff into kubernetes:master Aug 8, 2018

embano1 mentioned this pull request Aug 22, 2018

vSphere Cloud Provider does not work when deployed across Zones with zone-local Storage #67703

Closed

4 tasks

dougm mentioned this pull request Aug 22, 2018

godeps: update vmware/govmomi #67722

Merged

dougm mentioned this pull request Aug 23, 2018

vsphere: add tests for Cloud Provider Zones implementation #67741

Merged

k8s-ci-robot 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 Sep 6, 2018

dougm mentioned this pull request Sep 6, 2018

vSphere Cloud Provider should support implement Zones() interface #64021

Closed

celiawa mentioned this pull request Jan 17, 2019

Not getzone for vsphere cloud provider if users don't want to get zone/region #73002

Closed

subramanian-neelakantan mentioned this pull request Jan 25, 2019

vSphere volumes do not have zone labels auto applied #73301

Closed

rhockenbury mentioned this pull request Feb 20, 2019

Add labels to vsphere cloud config kubernetes-sigs/kubespray#4275

Merged


		zone := cloudprovider.Zone{}
		vsi, err := vs.getVSphereInstanceForServer(vs.cfg.Workspace.VCenterIP, ctx)

Add zones support for vSphere cloud provider(in-tree) #66795

Add zones support for vSphere cloud provider(in-tree) #66795

Conversation

jiatongw commented Jul 30, 2018 • edited by dougm Loading

jiatongw commented Jul 30, 2018 • edited Loading

Choose a reason for hiding this comment

neolit123 commented Jul 30, 2018

neolit123 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dougm left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jiatongw commented Aug 2, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jiatongw Aug 3, 2018 • edited Loading

Choose a reason for hiding this comment

jiatongw commented Aug 3, 2018

dougm left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

k8s-ci-robot commented Aug 4, 2018

cblecker commented Aug 6, 2018

jiatongw commented Aug 6, 2018

cblecker commented Aug 6, 2018

jiatongw commented Aug 6, 2018

jiatongw commented Aug 6, 2018

dougm commented Aug 6, 2018

jiatongw commented Aug 6, 2018

dougm commented Aug 7, 2018

cblecker commented Aug 7, 2018

k8s-ci-robot commented Aug 7, 2018

k8s-github-robot commented Aug 8, 2018

jiatongw commented Jul 30, 2018 •

edited by dougm

Loading

jiatongw commented Jul 30, 2018 •

edited

Loading

jiatongw commented Aug 2, 2018 •

edited

Loading

jiatongw Aug 3, 2018 •

edited

Loading