Fixes issue where PVCs using `standard` StorageClass create PDs in disks in wrong zone in multi-zone GKE clusters #52322

davidz627 · 2017-09-12T03:23:27Z

Changed GetAllZones to only get zones with nodes that are currently running (renamed to GetAllCurrentZones). Added E2E test to confirm this behavior.

Fix a bug in GCE multizonal clusters where PersistentVolumes were sometimes created in zones without nodes.

xiangpengzhao · 2017-09-12T03:41:05Z

/release-note-none
/ok-to-test

msau42 · 2017-09-12T17:48:27Z

pkg/cloudprovider/providers/gce/gce_instances.go

+
+	for _, node := range nodeList.Items {
+		labels := node.ObjectMeta.Labels
+		zone := labels[kubeletapis.LabelZoneFailureDomain]


Check for label doesn't exist.

msau42 · 2017-09-12T17:59:50Z

test/e2e/ubernetes_lite.go

+	var extraZone string
+	for _, zone := range allZonesInRegion {
+		if !expectedZones.Has(zone.Name) {
+			extraZone = zone.Name


You can break after this

msau42 · 2017-09-12T18:01:14Z

test/e2e/ubernetes_lite.go

+
+	// If no zones left to create an extra instance we screwed and we stop the test right here
+	Expect(extraZone).NotTo(Equal(""), fmt.Sprintf("No extra zones available in region %s", region))
+	framework.Logf("We are going to start a compute instance in unused zone: %v\n", extraZone)


You can use By() here

msau42 · 2017-09-12T18:02:15Z

test/e2e/ubernetes_lite.go

+	gotZones, err := gceCloud.GetAllCurrentZones()
+	Expect(err).NotTo(HaveOccurred())
+
+	Expect(gotZones.Equal(expectedZones)).To(BeTrue(), fmt.Sprintf("Expected zones: %v, Got Zones: %v", expectedZones, gotZones))


Do you need to sort the zones before comparing?

This is set equality. It does a carnality comparison then checks whether one is a superset of the other.

msau42 · 2017-09-12T18:03:06Z

test/e2e/ubernetes_lite.go

+func OnlyAllowNodeZones(f *framework.Framework, zoneCount int, image string) {
+	gceCloud, err := framework.GetGCECloud()
+	Expect(err).NotTo(HaveOccurred())
+	gceCloud.SetClientSet(f.ClientSet)


Is it possible to set this earlier in e2e.go when we initialize the cloud provider?

Not without pretty significant code changes I think. I looked at the way the framework is setting it up for f.ClientSet and its pretty involved and has some pretty complex dependancies. Also f.ClientSet isn't set up until the start of the specific test so we can't use it in e2e.go.

lmk if you want to talk about this.

I think i've actually solved this. Testing now.

msau42 · 2017-09-12T19:58:38Z

/assign

msau42 · 2017-09-12T20:05:13Z

test/e2e/e2e.go

 		if cloudConfig.Zone == "" && framework.TestContext.CloudConfig.MultiZone {
-			zones, err := gceCloud.GetAllZones()
+			zones, err := gceCloud.GetAllCurrentZones()


Is this going to work? This is done early in e2e initialization, where client is not set yet?

msau42 · 2017-09-12T20:05:26Z

pkg/cloudprovider/providers/gce/gce_instances.go

 	zones := sets.NewString()
+	nodeList, err := gce.client.Core().Nodes().List(metav1.ListOptions{})


Check for client nil

msau42 · 2017-09-12T20:19:10Z

test/e2e/ubernetes_lite.go

@@ -61,8 +62,87 @@ var _ = framework.KubeDescribe("Multi-AZ Clusters", func() {
 	It("should schedule pods in the same zones as statically provisioned PVs", func() {
 		PodsUseStaticPVsOrFail(f, (2*zoneCount)+1, image)
 	})
+
+	It("should only be allowed to provision PDs in zones where nodes exist", func() {


We can also control the zones that volumes are provisioned in, if we name them correctly. So maybe a test case we can add is to create PVCs with specific names, and make sure they get spread across the appropriate zones.

Should this test case be added in this same PR or should I create a new PR/issue for it?

I would put it as part of this test, maybe instead of (or in addition to) comparing the GetAllZones call. Because from a user's standpoint, they would see issues in the volume provisioning. The GetAllZones call is just an implementation detail.

I added it in addition to. So the GetAllZones check still exists, the PVC checking follows.

davidz627 · 2017-09-13T18:24:00Z

addressed comments

davidz627 · 2017-09-13T20:41:56Z

/retest

msau42 · 2017-09-13T20:51:32Z

pkg/cloudprovider/providers/gce/gce_instances.go

+	}
+	nodeList, err := client.Core().Nodes().List(metav1.ListOptions{})
+	if err != nil {
+		log.Fatalf("Failed to list nodes: %v", err)


We can let the caller log the error if they want to.

msau42 · 2017-09-13T21:22:09Z

pkg/cloudprovider/providers/gce/gce_instances.go

 			zones.Insert(zone)
+		} else {
+			return nil, fmt.Errorf("Could not find zone for node %v", node.ObjectMeta.Name)


I think we should log and skip. I don't want to fail the whole provisioning operation just because one of the nodes is in a bad state.

msau42 · 2017-09-13T21:26:06Z

pkg/cloudprovider/providers/gce/gce_instances.go

 	return zones, nil
 }

+// GetAllManagedZones gets all the zones managed by k8s
+func (gce *GCECloud) GetAllManagedZones() (sets.String, error) {
+	if gce.managedZones != nil {


When is this field set? Is it during initialization and never changes after that?

Yes, it set from the config and doesn't ever change after. I feel like the zones returned by this method could be different from GetAllCurrentZones

I'm not sure how useful "zones when initialized" is considering you can add/remove zones during runtime.

Yes - that was my point when I suggested removing it. It seems @davidz627 agreed for removing this function completely in another comment I added.

Something that confuses me. There are a few places that iterate through managedZones, but if managedZones never changed after initialization, how does it get new zones that are added later?

It seems like all other uses of managedZones makes sense to me. Just this one seems to be related to the issue this PR is trying to solve. Can anyone confirm?

It's checking that the zone passed in is in the managedZones list, but maybe it makes more sense to compare against GetAllCurrentZones. The actual selection of the zone is done by ChooseZoneForVolumes, which calls GetAllCurrentZones already. But it's also possible for the user to explicitly set an invalid zone as well.

@saad-ali we were wondering if we should restrict both CreateDisk and CreateRegionalDisk to only being able to create disks in zones with nodes in them (a la GetAllCurrentZones). Or if they should be left in the current state (to allow users to create disks in any zone in the region).

In its current state we believe that a user trying to create a Disk in a zone without a node would be most likely a mistake (since the PD would be unusable) and should trigger an error. However, it could be possible that advanced users may want to be able to create PDs in zones without nodes for regionaldisks for manual failover.

What do you think, Saad? It seems to be a tradeoff between failing fast for standard use-cases, or allowing users to do hypothetical funky things manually that we may not want.

FWIW @msau42 and I think surfacing an error for provisioning a PD in a zone without nodes is the way to go here. But I am interested to see if this may have some unintended side effects or if we missed some other use cases.

@saad-ali we were wondering if we should restrict both CreateDisk and CreateRegionalDisk to only being able to create disks in zones with nodes in them (a la GetAllCurrentZones). Or if they should be left in the current state (to allow users to create disks in any zone in the region).

In its current state we believe that a user trying to create a Disk in a zone without a node would be most likely a mistake (since the PD would be unusable) and should trigger an error. However, it could be possible that advanced users may want to be able to create PDs in zones without nodes for regionaldisks for manual failover.

What do you think, Saad? It seems to be a tradeoff between failing fast for standard use-cases, or allowing users to do hypothetical funky things manually that we may not want.

FWIW @msau42 and I think surfacing an error for provisioning a PD in a zone without nodes is the way to go here. But I am interested to see if this may have some unintended side effects or if we missed some other use cases.

I agree, provisioning a volume in a zone that does not have any active nodes should result in an error.

msau42 · 2017-09-13T21:45:56Z

test/e2e/ubernetes_lite.go

+	expectedZones, err := gceCloud.GetAllCurrentZones()
+	Expect(err).NotTo(HaveOccurred())
+	framework.Logf("Expected zones: %v\n", expectedZones)
+	// Get all the zones in this current region


Can you add more newlines between operations for better readability?

msau42 · 2017-09-13T21:48:19Z

test/e2e/e2e.go

+		if err != nil {
+			glog.Fatal("Error loading client: ", err)
+		}
+		gceCloud.SetClientSet(cs)


In a production environment, when is clientset set? Can we add it to CloudConfig instead of a new method?

It seems to be set using a method "Initialize" in gce.go. It's the line "gce.client = clientBuilder.ClientOrDie("cloud-provider")".

Looks like in production code this is done separately from creating the gceCloud object itself because it is an interface method and is called for any of the many cloud objects. It is called here:

kubernetes/cmd/cloud-controller-manager/app/controllermanager.go

Line 176 in 6b6b1e5

func StartControllers(s *options.CloudControllerManagerServer, kubeconfig *restclient.Config, clientBuilder controller.ControllerClientBuilder, stop <-chan struct{}, recorder record.EventRecorder, cloud cloudprovider.Interface) error {

Additionally I'm not sure that the clientset is a "config" item. In production code the cloudconfig is generated from reading in a config file and the clientset doesn't really seem to fit into that paradigm.

Setting the clientset with a method (although not the same method) after it is created is consistent with how it works in the production code. I think adding it to the CloudConfig for the e2e tests would actually make it inconsistent with the production code. I am open to adding it to the CloudConfig if you still think it is the right move, just wanted to provide you with additional info before we make a decision.

Ok that's fine then. Thanks for the detailed explanation!

wojtek-t · 2017-09-14T13:36:12Z

test/e2e/storage/volume_provisioning.go

@@ -422,7 +422,7 @@ var _ = SIGDescribe("Dynamic Provisioning", func() {
 			Expect(err).NotTo(HaveOccurred())

 			// Get all k8s managed zones
-			managedZones, err = gceCloud.GetAllZones()
+			managedZones, err = gceCloud.GetAllManagedZones()


Why we can't have "GetAllCurrentZones() here?

wojtek-t · 2017-09-14T13:54:14Z

pkg/cloudprovider/providers/gce/gce_instances.go

 	return zones, nil
 }

+// GetAllManagedZones gets all the zones managed by k8s
+func (gce *GCECloud) GetAllManagedZones() (sets.String, error) {


Do we really need this method? I would really like to get rid of it.

Currently there is only one place in test using this method and I personally think that we can use GetAllCurrentZones() there too.

Note that currently "managed_zones" can be only set to two values:

one particular zones

all zones from the region

There is no way to set it differently in production, thus we probably shouldn't set it differently in tests too.

I see your point. I have removed this method and replaced the one use with GetAllCurrentZones

wojtek-t · 2017-09-14T14:02:45Z

pkg/cloudprovider/providers/gce/gce_instances.go

-	// TODO: Caching, but this is currently only called when we are creating a volume,
-	// which is a relatively infrequent operation, and this is only # zones API calls
+// GetAllCurrentZones returns all the zones in which nodes are currently running
+func (gce *GCECloud) GetAllCurrentZones() (sets.String, error) {


Can you please add a TODO to add some caching to it?

If there will be too many calls to it, it may overload apiaserver.

Ideally, we would like to make it watch-based.

davidz627 · 2017-09-15T01:16:42Z

All comments addressed.

wojtek-t

One minor comment. Also please squash commits

Other than that LGTM

wojtek-t · 2017-09-15T06:35:13Z

test/e2e/storage/volume_provisioning.go

@@ -421,8 +421,8 @@ var _ = SIGDescribe("Dynamic Provisioning", func() {
 			gceCloud, err := framework.GetGCECloud()
 			Expect(err).NotTo(HaveOccurred())

-			// Get all k8s managed zones
-			managedZones, err = gceCloud.GetAllZones()
+			// Get all k8s managed zones (in this case it is just the single zone)


I don't think that's actually true. If you will be running this test for multizone clusters, these actually can be more than one zone.

davidz627 · 2017-09-15T17:56:28Z

Lots of unit test issues because of change to CreateDisk. Have to modify many of them so that may need to be looked at later.

davidz627 · 2017-09-15T20:04:44Z

comment addressed. Modified unit tests and level of indirection for GetAllCurrentZones. Please review new commit changes.

msau42 · 2017-09-15T20:45:37Z

pkg/cloudprovider/providers/gce/gce_disks_test.go

@@ -37,7 +38,8 @@ func TestCreateDisk_Basic(t *testing.T) {
 	/* Arrange */
 	gceProjectId := "test-project"
 	gceRegion := "fake-region"
-	fakeManager := newFakeManager(gceProjectId, gceRegion)
+	zonesWithNodes := []string{"zone1"}


Can we remove the managedZones setting in these tests?

many of the tests rely on "managed zones", especially the ones that delete instances. I can remove them on the ones that don't require doing a delete if you think that would be useful

nah, I'm just trying to see if there's a way we can avoid duplicate definitions. Maybe can you change the managedZones to use zonesWithNodes?

k8s-github-robot · 2017-11-21T01:02:39Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: davidz627, msau42, saad-ali, wojtek-t

Associated issue: 50115

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

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

~~cmd/kube-controller-manager/OWNERS~~ [wojtek-t]
~~pkg/cloudprovider/OWNERS~~ [wojtek-t]
~~pkg/volume/gce_pd/OWNERS~~ [saad-ali,wojtek-t]
~~test/OWNERS~~ [saad-ali,wojtek-t]

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

k8s-github-robot · 2017-11-21T01:05:41Z

[MILESTONENOTIFIER] Milestone Pull Request Current

@davidz627 @foxish @krousey @msau42 @saad-ali

Note: If this pull request is not resolved or labeled as priority/critical-urgent by Wed, Nov 22 it will be moved out of the v1.9 milestone.

Pull Request Labels

sig/auth sig/storage: Pull Request will be escalated to these SIGs if needed.
priority/important-soon: Escalate to the pull request owners and SIG owner; move out of milestone after several unsuccessful escalation attempts.
kind/bug: Fixes a bug discovered during the current release.

Help

davidz627 · 2017-11-21T01:47:18Z

/retest

fejta-bot · 2017-11-21T05:01:13Z

/retest
This bot automatically retries jobs that failed/flaked on approved PRs (send feedback to @fejta).

Review the full test history for this PR.

k8s-ci-robot · 2017-11-21T05:47:47Z

@davidz627: The following tests failed, say /retest to rerun them all:

Test name	Commit	Details	Rerun command
pull-kubernetes-e2e-gce-bazel	`38fd65b`	link	`/test pull-kubernetes-e2e-gce-bazel`
pull-kubernetes-e2e-gce-etcd3	`38fd65b`	link	`/test pull-kubernetes-e2e-gce-etcd3`

Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR.

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.

fejta-bot · 2017-11-21T07:49:15Z

/retest
This bot automatically retries jobs that failed/flaked on approved PRs (send feedback to @fejta).

Review the full test history for this PR.

k8s-github-robot · 2017-11-21T08:48:26Z

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

k8s-github-robot · 2017-11-21T09:35:01Z

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

wojtek-t · 2017-11-21T09:40:01Z

Added cherrypick-candidate label - this should be cherrypicked to 1.7 and 1.8 branches.

wojtek-t · 2017-11-28T09:27:28Z

@davidz627 - I wanted to do automated cherrypick of it to 1.8 branch but it generated conflicts. Can you please create this cherrypick - I think it's better since you wrote this code.

wojtek-t · 2017-11-29T08:42:14Z

Thanks @davidz627 !

…322-upstream-release-1.8 Automatic merge from submit-queue. Automated cherry pick of #52322 upstream release 1.8 Automated cherry pick of #52322 upstream release 1.8 #52322: Fixes issue where PVCs using `standard` StorageClass create PDs in disks in wrong zone in multi-zone GKE clusters ```release-note Fix a bug in GCE multizonal clusters where PersistentVolumes were sometimes created in zones without nodes. ```

…322-upstream-release-1.7 Automatic merge from submit-queue. Automated cherry pick of #52322 upstream release 1.7 Cherry pick of #52322 on release-1.7. #52322: Fixes issue where PVCs using `standard` StorageClass create PDs in disks in wrong zone in multi-zone GKE clusters ```release-note Fix a bug in GCE multizonal clusters where PersistentVolumes were sometimes created in zones without nodes. ```

k8s-cherrypick-bot · 2017-12-02T01:27:51Z

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.

k8s-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Sep 12, 2017

k8s-github-robot assigned krousey and foxish Sep 12, 2017

k8s-github-robot added the do-not-merge/release-note-label-needed Indicates that a PR should not merge because it's missing one of the release note labels. label Sep 12, 2017

davidz627 force-pushed the multizoneWrongZone branch 2 times, most recently from 2ff6383 to 861ca18 Compare September 12, 2017 16:53

msau42 reviewed Sep 12, 2017

View reviewed changes

davidz627 force-pushed the multizoneWrongZone branch from 861ca18 to 3524496 Compare September 12, 2017 19:55

k8s-ci-robot assigned msau42 Sep 12, 2017

msau42 reviewed Sep 12, 2017

View reviewed changes

davidz627 force-pushed the multizoneWrongZone branch from 3524496 to 547f0ec Compare September 12, 2017 21:23

msau42 reviewed Sep 13, 2017

View reviewed changes

wojtek-t reviewed Sep 14, 2017

View reviewed changes

wojtek-t reviewed Sep 15, 2017

View reviewed changes

davidz627 force-pushed the multizoneWrongZone branch from 62a72e5 to a24264e Compare September 15, 2017 20:08

msau42 reviewed Sep 15, 2017

View reviewed changes

davidz627 force-pushed the multizoneWrongZone branch from a24264e to 38fd65b Compare September 15, 2017 21:06

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Nov 21, 2017

saad-ali added status/in-progress and removed milestone/needs-attention labels Nov 21, 2017

k8s-github-robot merged commit 80e1c79 into kubernetes:master Nov 21, 2017

wojtek-t modified the milestones: v1.9, v1.7 Nov 21, 2017

wojtek-t added the cherrypick-candidate label Nov 21, 2017

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 Nov 28, 2017

This was referenced Nov 28, 2017

Automated cherry pick of #52322 upstream release 1.8 #56517

Merged

Automated cherry pick of #52322 upstream release 1.7 #56526

Merged

k8s-cherrypick-bot removed the cherrypick-candidate label Dec 2, 2017

davidz627 deleted the multizoneWrongZone branch December 2, 2017 01:28

msau42 mentioned this pull request Jan 30, 2018

Change mechanism of zone selection for dynamic provisioning #48504

Closed

This was referenced Mar 21, 2018

[GCP] Can we re-enable GCP multi-zone deployments? gardener/gardener#64

Closed

Reenable multi-zone support for GCP (has been fixed from 1.8.5 and 1.9) gardener/gardener#115

Merged

		zones := sets.NewString()
		nodeList, err := gce.client.Core().Nodes().List(metav1.ListOptions{})

Fixes issue where PVCs using standard StorageClass create PDs in disks in wrong zone in multi-zone GKE clusters #52322

Fixes issue where PVCs using standard StorageClass create PDs in disks in wrong zone in multi-zone GKE clusters #52322

Conversation

davidz627 commented Sep 12, 2017 • edited by wojtek-t

xiangpengzhao commented Sep 12, 2017

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

msau42 commented Sep 12, 2017

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

davidz627 commented Sep 13, 2017

davidz627 commented Sep 13, 2017

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

saad-ali Sep 14, 2017 • edited

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

davidz627 commented Sep 15, 2017

wojtek-t left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

davidz627 commented Sep 15, 2017

davidz627 commented Sep 15, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

k8s-github-robot commented Nov 21, 2017

k8s-github-robot commented Nov 21, 2017

davidz627 commented Nov 21, 2017

fejta-bot commented Nov 21, 2017

k8s-ci-robot commented Nov 21, 2017 • edited

fejta-bot commented Nov 21, 2017

k8s-github-robot commented Nov 21, 2017

k8s-github-robot commented Nov 21, 2017

wojtek-t commented Nov 21, 2017

wojtek-t commented Nov 28, 2017

wojtek-t commented Nov 29, 2017

k8s-cherrypick-bot commented Dec 2, 2017

Fixes issue where PVCs using `standard` StorageClass create PDs in disks in wrong zone in multi-zone GKE clusters #52322

Fixes issue where PVCs using `standard` StorageClass create PDs in disks in wrong zone in multi-zone GKE clusters #52322

davidz627 commented Sep 12, 2017 •

edited by wojtek-t

saad-ali Sep 14, 2017 •

edited

k8s-ci-robot commented Nov 21, 2017 •

edited