Use GA topology labels for EBS #99130

ayberk · 2021-02-16T18:45:55Z

What type of PR is this?

/kind bug

What this PR does / why we need it:

Similar to #98700, this PR replaces the deprecated topology labels in favor of the new GA ones for EBS plugin.

Which issue(s) this PR fixes:

Does this PR introduce a user-facing change?

[ACTION REQUIRED] Newly provisioned PVs by EBS plugin will no longer use the deprecated "failure-domain.beta.kubernetes.io/zone" and "failure-domain.beta.kubernetes.io/region" labels. It will use "topology.kubernetes.io/zone" and "topology.kubernetes.io/region" labels instead.

/sig storage
/cc @msau42 @wongma7

wongma7 · 2021-02-16T18:49:46Z

/triage accepted
/lgtm

I guess this PR is a lot smaller than the GCE counterpart because gce nodes can be moved to different zones and has more code to account for that..

wongma7 · 2021-02-16T18:55:30Z

/test pull-kubernetes-e2e-kops-aws

ayberk · 2021-02-16T21:27:36Z

No storage related failures on kops-aws test.

/retest

msau42 · 2021-02-17T00:47:31Z

https://github.com/Jiawei0227/kubernetes/blob/e76f60d2b82529c4ce26be99a424ebe6c7b361a3/test/e2e/storage/drivers/in_tree.go#L1685

probably needs to be updated too.

msau42 · 2021-02-17T00:52:01Z

There may be some work needed in the translation library to recognize region labels (can be looked at separately). Similar to https://github.com/kubernetes/kubernetes/pull/97823/files#diff-91f0ca8be9396a2cd3e57cf95a919896eb6821a64b509dc9f96e6a1d730ab1c3R380

ayberk · 2021-02-18T00:55:19Z

staging/src/k8s.io/csi-translation-lib/plugins/aws_ebs.go

@@ -253,3 +259,90 @@ func KubernetesVolumeIDToEBSVolumeID(kubernetesID string) (string, error) {

 	return awsID, nil
 }
+
+// This code is copied over from gce_pd as is. However, we're passing our own getRegionFromZones implementation.
+// It might be worth moving this to a common place and pass that function as a parameter.


I noted this here from your conversation, but I can definitely move this to a common place.

Yeah if you can move this to a shared library that would be great! cc @Jiawei0227

Yeah I think it would be good if we can combine this into a common place, something like

type regionParserFn([]string) (string, error) func regionTopologyHandler(pv *v1.PersistentVolume, regionParser regionParserFn) error {...}

Will do! Is in_tree_volume.go a good place for it?

Yep. Thanks!

ayberk · 2021-02-19T20:47:38Z

staging/src/k8s.io/csi-translation-lib/plugins/in_tree_volume.go

@@ -270,7 +271,7 @@ type regionTopologyHandler func(pv *v1.PersistentVolume) error
 // 1. Replace all CSI topology to Kubernetes Zone topology label
 // 2. Process and generate region topology if a regionTopologyHandler is passed
 // 3. Add Kubernetes Topology labels(zone) if they do not exist
-func translateTopologyFromCSIToInTree(pv *v1.PersistentVolume, csiTopologyKey string, regionTopologyHandler regionTopologyHandler) error {
+func translateTopologyFromCSIToInTree(pv *v1.PersistentVolume, csiTopologyKey string, regionTopologyHandler regionTopologyHandlerFn, regionParser regionParserFn) error {


I tried to make regionParser part of regionTopologyHandler, but couldn't find a simple way. Open to suggestions :)

What about remove the type regionTopologyHandlerFn, I think the regionTopologyHandler() should be generic enough to be shared?
And then the code will be like:

func translateTopologyFromCSIToInTree(pv *v1.PersistentVolume, csiTopologyKey string, regionParser regionParserFn) error {

I actually did that change but then reverted it back because of this line specifically. If we're OK with using the regionParser for the nil check, should be a trivial change.

I think it should be okay

ayberk · 2021-02-20T01:23:39Z

/retest

Jiawei0227 · 2021-02-20T06:49:45Z

/lgtm

If there is any CI in AWS or EKS please also sync with @wongma7 to update them.

@wongma7 for approval.

wongma7 · 2021-02-22T18:50:09Z

/lgtm
/approve

wongma7 · 2021-02-22T18:51:34Z

staging/src/k8s.io/csi-translation-lib/plugins/aws_ebs.go

+		return "", fmt.Errorf("no zones specified")
+	}
+
+	// AWS zones can be in four forms:


what about copying/using

kubernetes/staging/src/k8s.io/legacy-cloud-providers/aws/aws.go

Line 1240 in f48972e

// Derives the region from a valid az name.

? seems simpler and less likely to break

u r right it will break on wavelength zone .

msau42 · 2021-02-22T19:23:58Z

/approve

In the release notes, can you reference the exact label key?

ayberk · 2021-02-22T19:55:32Z

@msau42 Done.

Jiawei0227 · 2021-02-23T21:17:20Z

/assign @cheftako
Hey Walter, can you help to approve this? This is similar to what we have done in #98700

cheftako · 2021-02-24T03:33:52Z

/approve

k8s-ci-robot · 2021-02-24T03:34:27Z

[APPROVALNOTIFIER] This PR is APPROVED

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

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/awsebs/OWNERS~~ [msau42]
~~plugin/pkg/admission/storage/persistentvolume/label/OWNERS~~ [msau42]
~~staging/src/k8s.io/csi-translation-lib/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

k8s-ci-robot · 2021-02-24T04:34:02Z

@ayberk: The following test failed, say /retest to rerun all failed tests:

Test name	Commit	Details	Rerun command
pull-kubernetes-e2e-kops-aws	fec7a559ab95c319ecbe98abfd7f2eebcd577224	link	`/test pull-kubernetes-e2e-kops-aws`

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.

Jiawei0227 · 2021-02-24T05:35:47Z

/retest

jastBytes · 2022-02-08T10:33:57Z

A short question on this, what happens to existing STS and PVs? How is the migration path here?

k8s-ci-robot requested review from gnufied and M00nF1sh February 16, 2021 18:46

k8s-ci-robot added area/cloudprovider sig/cloud-provider Categorizes an issue or PR as relevant to SIG Cloud Provider. labels Feb 16, 2021

k8s-ci-robot added triage/accepted Indicates an issue or PR is ready to be actively worked on. and removed needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one. labels Feb 16, 2021

k8s-ci-robot assigned wongma7 Feb 16, 2021

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

ayberk changed the title ~~Use GA topoogy labels for EBS~~ Use GA topology labels for EBS Feb 16, 2021

Use GA topoogy labels for EBS

339b8b4

ayberk force-pushed the ebs_ga_labels branch from fec7a55 to 339b8b4 Compare February 18, 2021 00:50

ayberk commented Feb 18, 2021

View reviewed changes

ayberk commented Feb 19, 2021

View reviewed changes

ayberk force-pushed the ebs_ga_labels branch 2 times, most recently from 0ee8598 to 85203fa Compare February 19, 2021 22:59

Move RegionTopologyHandler to in_tree_volume

e500271

ayberk force-pushed the ebs_ga_labels branch from 85203fa to e500271 Compare February 19, 2021 23:45

k8s-ci-robot assigned Jiawei0227 Feb 20, 2021

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

wongma7 reviewed Feb 22, 2021

View reviewed changes

Jiawei0227 mentioned this pull request Feb 22, 2021

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

Closed

k8s-ci-robot assigned cheftako Feb 23, 2021

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

k8s-ci-robot merged commit 267e47f into kubernetes:master Feb 24, 2021

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

ayberk deleted the ebs_ga_labels branch February 26, 2021 19:07

This was referenced May 1, 2021

Cherry pick PRs from K/K kubernetes/cloud-provider-aws#209

Closed

Cherry-pick: Use GA topoogy labels for EBS (#99130) kubernetes/cloud-provider-aws#211

Merged

ialidzhikov mentioned this pull request May 5, 2021

[release-2.1] Update to k8s.io/csi-translation-lib@v0.21.0 kubernetes-csi/external-provisioner#621

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use GA topology labels for EBS #99130

Use GA topology labels for EBS #99130

ayberk commented Feb 16, 2021 •

edited

wongma7 commented Feb 16, 2021

wongma7 commented Feb 16, 2021

ayberk commented Feb 16, 2021

msau42 commented Feb 17, 2021

msau42 commented Feb 17, 2021

ayberk Feb 18, 2021

msau42 Feb 18, 2021

Jiawei0227 Feb 18, 2021

ayberk Feb 18, 2021

Jiawei0227 Feb 18, 2021

ayberk Feb 19, 2021

Jiawei0227 Feb 19, 2021

ayberk Feb 19, 2021

Jiawei0227 Feb 19, 2021

ayberk commented Feb 20, 2021

Jiawei0227 commented Feb 20, 2021

wongma7 commented Feb 22, 2021

wongma7 Feb 22, 2021

wongma7 Feb 22, 2021

msau42 commented Feb 22, 2021

ayberk commented Feb 22, 2021

Jiawei0227 commented Feb 23, 2021

cheftako commented Feb 24, 2021

k8s-ci-robot commented Feb 24, 2021

k8s-ci-robot commented Feb 24, 2021 •

edited

Jiawei0227 commented Feb 24, 2021

jastBytes commented Feb 8, 2022

Use GA topology labels for EBS #99130

Use GA topology labels for EBS #99130

Conversation

ayberk commented Feb 16, 2021 • edited

What type of PR is this?

What this PR does / why we need it:

Which issue(s) this PR fixes:

Does this PR introduce a user-facing change?

wongma7 commented Feb 16, 2021

wongma7 commented Feb 16, 2021

ayberk commented Feb 16, 2021

msau42 commented Feb 17, 2021

msau42 commented Feb 17, 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ayberk commented Feb 20, 2021

Jiawei0227 commented Feb 20, 2021

wongma7 commented Feb 22, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

msau42 commented Feb 22, 2021

ayberk commented Feb 22, 2021

Jiawei0227 commented Feb 23, 2021

cheftako commented Feb 24, 2021

k8s-ci-robot commented Feb 24, 2021

k8s-ci-robot commented Feb 24, 2021 • edited

Jiawei0227 commented Feb 24, 2021

jastBytes commented Feb 8, 2022

ayberk commented Feb 16, 2021 •

edited

k8s-ci-robot commented Feb 24, 2021 •

edited