Update gce-pd volume topology label to GA #98700

Jiawei0227 · 2021-02-02T19:57:42Z

What type of PR is this?
/kind feature

What this PR does / why we need it:
This is a follow-up PR for #97823

This PR updates the gce-pd volume topology label to use GA version. Previously we were using beta label(FailureDomain) for the in-tree volumes. But as the Beta label has been deprecated for a while and is scheduled to be removed soon. We should update this to use GA label instead.

Which issue(s) this PR fixes:

Fix #92237 by upgrading the label to GA version.

Special notes for your reviewer:

Does this PR introduce a user-facing change?:

[ACTION REQUIRED] Newly provisioned PVs by gce-pd will no longer have the beta FailureDomain label. gce-pd volume plugin will start to have GA topology label instead.

Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.:

/sig storage
/cc @msau42

k8s-ci-robot · 2021-02-02T19:57:49Z

@Jiawei0227: This issue is currently awaiting triage.

If a SIG or subproject determines this is a relevant issue, they will accept it by applying the triage/accepted label and provide further guidance.

The triage/accepted label can be added by org members by writing /triage accepted in a comment.

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.

Jiawei0227 · 2021-02-02T21:36:28Z

/retest

staging/src/k8s.io/legacy-cloud-providers/gce/gce_disks.go

msau42 · 2021-02-02T22:40:35Z

test/e2e/storage/drivers/in_tree.go

@@ -1243,7 +1243,7 @@ func InitGcePdDriver() storageframework.TestDriver {
 			},
 			SupportedFsType:      supportedTypes,
 			SupportedMountOption: sets.NewString("debug", "nouid32"),
-			TopologyKeys:         []string{v1.LabelFailureDomainBetaZone},


Hmmm let's double check but I think we have upgrade tests than run N-1 e2e version against a N version cluster. We may need to make some changes to the older e2e to handle both labels.

so there's a couple of variations of tests, but IIRC, one variation will run N-1 e2e.test on an N-1 cluster, upgrade the cluster, and then run N-1 e2e.test on the version N cluster

I run the e2e binary on new cluster with my change and the topology test pass.

./e2e.test --ginkgo.focus=".*In-tree Volumes.*gcepd.*topology.*" -provider gce -gce-project=jiaweiwang-gke-multi-cloud-dev -gce-zone=us-central1-b

{"msg":"PASSED [sig-storage] In-tree Volumes [Driver: gcepd] [Testpattern: Dynamic PV (delayed binding)] topology should provision a volume and schedule a pod with AllowedTopologies","total":8,"completed":1,"skipped":1030,"failed":0}
{"msg":"PASSED [sig-storage] In-tree Volumes [Driver: gcepd] [Testpattern: Dynamic PV (immediate binding)] topology should provision a volume and schedule a pod with AllowedTopologies","total":8,"completed":2,"skipped":2859,"failed":0}

Can you be more specific if you have concerns on which tests?

Did you run e2e.test from 1 version before, ie release 1.20?

Yes, I build the e2e.test from release-1.20 branch.

Did you also run the regional PD tests? (it requires a regional cluster). I see some parts of that test are validating the labels on the PV object, so I would expect a 1.20 test validating beta labels will fail on a 1.21 cluster that uses GA labels.

It seems regional PD is checking PV labels. So we need to fix the 1.20 release branch e2e test to not break the version skew test.
#98733 PR out for review and cherry-pick approval.

msau42 · 2021-02-02T22:41:47Z

/assign @mattcary
cc @wongma7 @andyzhangx @jsafrane FYI for other clouds

msau42 · 2021-02-02T23:58:56Z

oops forgot one cc @divyenpatel @xing-yang as fyi

Jiawei0227 · 2021-02-03T07:22:25Z

/retest

msau42 · 2021-02-03T20:39:44Z

/lgtm
/approve
/assign @cheftako
for cloud provider updates

msau42 · 2021-02-03T20:42:58Z

Can you also update the release notes saying that newly provisioned PVs will no longer have the beta label, so any outside tools need to be fixed (and add an ACTION REQUIRED)

Jiawei0227 · 2021-02-03T21:59:04Z

Can you also update the release notes saying that newly provisioned PVs will no longer have the beta label, so any outside tools need to be fixed (and add an ACTION REQUIRED)

Done

cheftako · 2021-02-03T22:58:08Z

staging/src/k8s.io/legacy-cloud-providers/gce/gce_disks.go

@@ -515,7 +515,10 @@ func (g *Cloud) GetLabelsForVolume(ctx context.Context, pv *v1.PersistentVolume)

 	// If the zone is already labeled, honor the hint
 	name := pv.Spec.GCEPersistentDisk.PDName
-	zone := pv.Labels[v1.LabelFailureDomainBetaZone]
+	zone := pv.Labels[v1.LabelTopologyZone]


FYI, We are no longer accepting enhancements on legacy-cloud-providers. Any such enhancements must be done out of tree. As this is clearly just a bug fix going to let it through.

cheftako · 2021-02-03T22:59:04Z

/approve

k8s-ci-robot · 2021-02-03T22:59:34Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: cheftako, Jiawei0227, msau42

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:

~~pkg/volume/gcepd/OWNERS~~ [msau42]
~~staging/src/k8s.io/legacy-cloud-providers/OWNERS~~ [cheftako]
~~test/e2e/storage/OWNERS~~ [msau42]

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

Jiawei0227 · 2021-02-04T00:13:37Z

/retest

AlwaySummit · 2021-07-05T08:59:06Z

Hi @Jiawei0227 , sorry to trouble but I'm hitting an error due to the missing labels of pv. I'm not sure if it is related to this PR. In version 1.18, neither the failure-domain.beta.kubernetes.io/zone nor topology.kubernetes.io/zone is correctly set for pv. I guess it is because the in-tree plugin was deprecated and the logic of label set was gone simultaneously but I don't have any concrete evidence to prove it since I'm not expert on this area. Could you please share more insights of this? Thanks.
BTW, please refer more detail for this issue: #103403

k8s-ci-robot requested a review from msau42 February 2, 2021 19:57

k8s-ci-robot added needs-priority Indicates a PR lacks a `priority/foo` label and requires one. area/cloudprovider area/test sig/cloud-provider Categorizes an issue or PR as relevant to SIG Cloud Provider. sig/testing Categorizes an issue or PR as relevant to SIG Testing. labels Feb 2, 2021

msau42 reviewed Feb 2, 2021

View reviewed changes

k8s-ci-robot assigned mattcary Feb 2, 2021

Update gce-pd volume topology label to GA

e76f60d

Jiawei0227 force-pushed the update_label branch from 977e548 to e76f60d Compare February 3, 2021 03:14

Jiawei0227 mentioned this pull request Feb 3, 2021

Update region_pd e2e test to support PV have GA topology #98733

Merged

k8s-ci-robot assigned cheftako and msau42 Feb 3, 2021

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Feb 3, 2021

k8s-ci-robot added release-note-action-required Denotes a PR that introduces potentially breaking changes that require user action. and removed release-note Denotes a PR that will be considered when it comes time to generate release notes. labels Feb 3, 2021

cheftako reviewed Feb 3, 2021

View reviewed changes

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Feb 3, 2021

k8s-ci-robot merged commit 429eaca into kubernetes:master Feb 4, 2021

k8s-ci-robot added this to the v1.21 milestone Feb 4, 2021

Jiawei0227 mentioned this pull request Feb 4, 2021

Update in-tree PV kubernetes topology label to GA #98281

Closed

ayberk mentioned this pull request Feb 16, 2021

Use GA topology labels for EBS #99130

Merged

kassarl mentioned this pull request Feb 24, 2021

Use GA topology labels for Azuredisk #99424

Closed

kassarl mentioned this pull request Apr 27, 2021

Use GA topology labels for Azuredisk #101534

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update gce-pd volume topology label to GA #98700

Update gce-pd volume topology label to GA #98700

Jiawei0227 commented Feb 2, 2021 •

edited

k8s-ci-robot commented Feb 2, 2021

Jiawei0227 commented Feb 2, 2021

msau42 Feb 2, 2021

Jiawei0227 Feb 3, 2021

Jiawei0227 Feb 3, 2021

msau42 Feb 3, 2021

Jiawei0227 Feb 3, 2021

msau42 Feb 3, 2021

Jiawei0227 Feb 3, 2021

msau42 commented Feb 2, 2021

msau42 commented Feb 2, 2021

Jiawei0227 commented Feb 3, 2021

msau42 commented Feb 3, 2021

msau42 commented Feb 3, 2021 •

edited

Jiawei0227 commented Feb 3, 2021

cheftako Feb 3, 2021

cheftako commented Feb 3, 2021

k8s-ci-robot commented Feb 3, 2021

Jiawei0227 commented Feb 4, 2021

AlwaySummit commented Jul 5, 2021

Update gce-pd volume topology label to GA #98700

Update gce-pd volume topology label to GA #98700

Conversation

Jiawei0227 commented Feb 2, 2021 • edited

k8s-ci-robot commented Feb 2, 2021

Jiawei0227 commented Feb 2, 2021

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 Feb 2, 2021

msau42 commented Feb 2, 2021

Jiawei0227 commented Feb 3, 2021

msau42 commented Feb 3, 2021

msau42 commented Feb 3, 2021 • edited

Jiawei0227 commented Feb 3, 2021

Choose a reason for hiding this comment

cheftako commented Feb 3, 2021

k8s-ci-robot commented Feb 3, 2021

Jiawei0227 commented Feb 4, 2021

AlwaySummit commented Jul 5, 2021

Jiawei0227 commented Feb 2, 2021 •

edited

msau42 commented Feb 3, 2021 •

edited