Preparation for Topology migration to GA for CSI migration #97823

Jiawei0227 · 2021-01-08T00:49:57Z

This also includes a change on CSI migration TranslateCSIToInTree
where we remove the CSI topology and add Kubernetes Topology to
the NodeAffinity

What type of PR is this?
/kind bug
/kind cleanup

What this PR does / why we need it:
This PR is a subset work from #97746 with some comments addressed and test.
Specifically, this PR prepares for the migration from failure-domain.beta.kubernetes.io/zone topology to GA version which is topology.kubernetes.io/zone.

Besides, this PR also contains a behavior change in the csi-translation library TranslateCSIToInTree for gce-pd where we remove the CSI topology and inserts the kubernetes topology. This surfaces two purpose: 1. Fix a potential rollback issue. 2. Prepare for the fully deprecation of PV label admission-controller.

Which issue(s) this PR fixes:

Fixes #

Partially fix #92237

Special notes for your reviewer:

Does this PR introduce a user-facing change?:

Remove CSI topology from migrated in-tree gcepd volume.

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

/sig storage
/assign @msau42
/cc @wongma7 @mattcary @verult

The follow-up for this PR includes:

Upgrade csi-translation-lib to the newer version on csi-provisioner
Upgrade in-tree volume plugins to use the GA label

k8s-ci-robot · 2021-01-08T00:50:04Z

@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

Hey @msau42 , thanks for your review! Sorry I have to close the old one and open this new one.

I think to make it cleaner, I am focusing this PR on the csi-translation-lib fix first and I also include some preparation work for topology label GA promotion. I will work on the actual label migration for gcepd as soon as this PR merges.

I have copied your comment and replied below. Please take a look. Thanks!

plugin/pkg/admission/storage/persistentvolume/label/admission_test.go

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

Jiawei0227 · 2021-01-08T00:55:19Z

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

@@ -286,7 +291,10 @@ func (g *gcePersistentDiskCSITranslator) TranslateCSIPVToInTree(pv *v1.Persisten
 		gceSource.Partition = int32(partInt)
 	}

-	// TODO: Take the zone/regional information and stick it into the label.
+	// translate CSI topology to In-tree topology for rollback compatibility
+	if err := translateTopologyFromCSIToInTree(pv, GCEPDTopologyKey); err != nil {


I think it's not just removing the old key. We need to add in the kubernetes topology keys.
The PV label admission controller is going to be removed in the future, so we cannot depend on the admission controller adding it for us.

Done.

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

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

Jiawei0227 · 2021-01-08T01:00:13Z

staging/src/k8s.io/cloud-provider/volume/helpers/zones.go

@@ -135,7 +138,7 @@ func SelectZonesForVolume(zoneParameterPresent, zonesParameterPresent bool, zone
 	}

 	if (len(allowedTopologies) > 0) && (allowedZones.Len() == 0) {
-		return nil, fmt.Errorf("no matchLabelExpressions with %s key found in allowedTopologies. Please specify matchLabelExpressions with %s key", v1.LabelFailureDomainBetaZone, v1.LabelFailureDomainBetaZone)
+		return nil, fmt.Errorf("no matchLabelExpressions with %s key found in allowedTopologies. Please specify matchLabelExpressions with %s key", v1.LabelTopologyZone, v1.LabelTopologyZone)


Is this logic shared by other clouds too? Will this cause other clouds' Provision() to use GA labels?

This function SelectZoneForVolume is shared by all in-tree plugins and is being called in in-tree code. This function only picks a zone value from the labels. So it does not matter if the value comes from GA or beta label. For example:

If a node has failure-domain.beta.kubernetes.io/zone=us-west-2 and topology.kubernetes.io/zone=us-west-2. This function will just return the us-west-2 so it should not matter here.

staging/src/k8s.io/csi-translation-lib/translate_test.go

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

plugin/pkg/admission/storage/persistentvolume/label/admission.go

staging/src/k8s.io/cloud-provider/volume/helpers/zones_test.go

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

msau42 · 2021-01-08T01:22:43Z

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

@@ -286,7 +291,10 @@ func (g *gcePersistentDiskCSITranslator) TranslateCSIPVToInTree(pv *v1.Persisten
 		gceSource.Partition = int32(partInt)
 	}

-	// TODO: Take the zone/regional information and stick it into the label.
+	// translate CSI topology to In-tree topology for rollback compatibility
+	if err := translateTopologyFromCSIToInTree(pv, GCEPDTopologyKey); err != nil {


we may want to add the labels too in case people have tooling on the kubernetes labels.

The labels of migrated PV already exist in the current case even before it goes to label admission controller.

The PV.Labels are set?

Oops, it is not... Sorry I had a misunderstanding. Will add it.

I add the zone labels in if it does not exist. But I dont know if we can derive region info from Zone values for all the plugins. So I did not add the region label.

An alternative solution is to add a generateRegionFromZone func as a parameter to let each plugin pass a region parser.

We only have 5 plugins to support. I think it's not that critical to have a common util function and each plugin can parse region their own ways.

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

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

wongma7 · 2021-01-11T20:21:27Z

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

 // In-tree topology has precedence over zone labels.
-func translateTopology(pv *v1.PersistentVolume, topologyKey string) error {


btw do you know come this function only translated zone and not region? it looks like azuredd and cinder provisioners do set region, so the info gets lost on translation, potentially it is a bug for them.

OTOH, your new translateTopologyFromCSIToInTree does handle region makes more sense to me.

Hmm, I am not familiar with those two so I could not tell if this is going to be a problem. But I guess their driver has its own CSI migration test and if those tests are working fine. Then it should be good?

Yeah let's leave it as is, I may open a separate issue if necessary but I was just curious since now there is some asymmetry between translateTopologyFromCSIToInTree and translateTopologyFromInTreeToCSI.

cc @andyzhangx @gnufied for region questions on azure and openstack.

At least in aws and gcp cases, you can derive the region from the zone.

msau42 · 2021-01-11T21:38:18Z

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

@@ -286,7 +291,10 @@ func (g *gcePersistentDiskCSITranslator) TranslateCSIPVToInTree(pv *v1.Persisten
 		gceSource.Partition = int32(partInt)
 	}

-	// TODO: Take the zone/regional information and stick it into the label.
+	// translate CSI topology to In-tree topology for rollback compatibility
+	if err := translateTopologyFromCSIToInTree(pv, GCEPDTopologyKey); err != nil {


The PV.Labels are set?

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

msau42 · 2021-01-11T21:52:49Z

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

 // In-tree topology has precedence over zone labels.
-func translateTopology(pv *v1.PersistentVolume, topologyKey string) error {


cc @andyzhangx @gnufied for region questions on azure and openstack.

At least in aws and gcp cases, you can derive the region from the zone.

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

msau42 · 2021-01-12T00:11:12Z

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

+				}
+			}
+		} else {
+			// nothing is in the label, we try our best to add the CSI topology zone in


I think we should prioritize CSI first before the PV label. Someone would have to manually create their own PV or have their own CSI driver to get into the scenario where a CSI PV object would have a intree label on it.

See comments above.

Sorry which comment are you referring to?

Sorry wrong reply, lol. I thought I was replying to another comment. NVM

So here, you mean if there is a CSI topology and also PV labels. We should use the CSI topology to generate Kubernetes topology first?

Yes, we should prefer using what's in CSI topology over PV labels because in the normal case we expect only CSI topology to exist. If PV label exists, something unusual is going on.

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

msau42 · 2021-01-12T01:27:58Z

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

+				}
+			}
+		} else {
+			// nothing is in the label, we try our best to add the CSI topology zone in


Sorry which comment are you referring to?

Jiawei0227 · 2021-01-12T23:48:15Z

/retest

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

msau42 · 2021-01-12T21:50:43Z

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

+		for i, nodeSelectorRequirement := range nsrequirements {
+			if nodeSelectorRequirement.Key == topologyKey {
+				index = i
+				break


This assumes that the key is only in one element. Is always going to be the case?

Especially consider RegionalPD because the topology can be expressed 2 different ways. cc @mattcary @verult

okay, I changed the logic to remove the key from:

Multiple terms(This will be the regional PD case I think)

Single term with duplicate key match expression(although I doubt this will happen)

Also add removeTopology test

msau42 · 2021-01-12T21:59:16Z

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

+	// Check the NodeAffinity first for the topology version
+	zoneGA := TopologyKeyExist(v1.LabelTopologyZone, pv.Spec.NodeAffinity)
+	regionGA := TopologyKeyExist(v1.LabelTopologyRegion, pv.Spec.NodeAffinity)
+	if zoneGA || regionGA {


This would be very unexpected, but what if the zone label was beta and the region label was GA?

Yeah, firstly it will be very unexpected... But it could happen, my criteria here is that if it has a GA version topology, then it should be treated as GA supported.

I guess what if zone label is beta, but region label is GA. Then we will fail to fine the zone because we're looking for GA zone label?

Hmm, so do you suggest we return zone label version and region label version separately? Like we can return GA version region label and beta version zone label in the case you described?

That's a good idea. I think it will simplify the logic too

As we discussed offline, it would be good to not overcomplicate this, so I will keep the old logic which will assume Zone and Region labels are in the same version.

Sorry coming back to this again. Seeing zone || region makes me asks questions like "what if zone was false?". Since we care about zone most importantly, what if we only check zone, and then return the corresponding region version based off of what we found? (ie, if we found beta zone, then return beta region without checking)

msau42 · 2021-01-12T22:01:42Z

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

+	topologyRegionValue := getTopologyValues(pv, regionLabel)
+	if len(topologyRegionValue) == 0 {
+		// 2.1 Let's try to use CSI topology to recover the region topology first
+		if len(csiTopologyZoneValues) > 0 && regionParser != nil {


Do we need csi zone values to be set? Or just regionParser?

Jiawei0227 · 2021-01-13T09:07:11Z

@msau42 I thought about this more offline. And I thought you are right, it would be easier if we use replaceTopology instead of remove/add. I changed all the corresponding part and write completely new function in the new commit. To summarize the behavior in translateTopologyFromCSIToInTree:

We will replace all the CSITopology to Kubernetes Zone Topology(And combine existing Kubernetes Zone Topology if any exists already)
We will add region topology if a regionTopologyHandler function is passed by the plugin. So each plugin can handle their region logic separately. Specifically for gce-pd, I prepared a gceRegionTopologyHandler which basically goes over each NodeAffinity term, and generate region val for each term. So this also handles the case where we have multiple region in different terms.
Add zone and region labels to the PV if it does not exist.

Please let me know if this model is easier to understand and follow.

Jiawei0227 · 2021-01-16T05:08:37Z

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

+			continue
+		}
+		// If no regionLabel found, generate region label from the zoneLabel we collect from this term
+		regionVal, err := getRegionFromZones(zoneVals)


I actually think a lot of other cloud-provider like aws/azure an reuse this function if we allow getRegionFromZones function to be a parameter. The other logic should be the same for other cloud-providers. But I guess it is fine we keep it this way. We can let them to decide if they want to use this or not.

That sounds like a good idea.

msau42 · 2021-01-20T02:04:25Z

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

+			continue
+		}
+		// If no regionLabel found, generate region label from the zoneLabel we collect from this term
+		regionVal, err := getRegionFromZones(zoneVals)


That sounds like a good idea.

msau42 · 2021-01-20T02:10:08Z

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

+	if zoneBeta := TopologyKeyExist(v1.LabelFailureDomainBetaZone, pv.Spec.NodeAffinity); zoneBeta {
+		return v1.LabelFailureDomainBetaZone, v1.LabelFailureDomainBetaRegion
+	}
+	if regionGA := TopologyKeyExist(v1.LabelTopologyRegion, pv.Spec.NodeAffinity); regionGA {


I was thinking we don't need to check for region at all. Because if zone is missing, then we're in trouble.

Sounds good. I can remove the region related checks and only check zone

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

andyzhangx · 2021-01-20T06:38:21Z

/test pull-kubernetes-e2e-azure-disk-vmss
/test pull-kubernetes-e2e-azure-disk

msau42 · 2021-01-20T17:58:30Z

/lgtm
/approve

Thanks!

msau42 · 2021-01-20T18:00:42Z

/assign @andrewsykim

This also includes a change on CSI migration TranslateCSIToInTree where we remove the CSI topology and add Kubernetes Topology to the NodeAffinity

- Instead of using a remove/add way to deal with the topology label, go directly with the replace topology way so that we dont have to care all the corner case. Also, since the translateTopologyFromCSIToInTree only get called from csi-provisioner, there should not be very odd PV object spec. - Change the beheavior translateTopologyFromInTreeToCSI so that it will replace zone label to CSI Topology label and also replace any beta region label to GA label

Jiawei0227 · 2021-01-20T18:54:25Z

I squashed commits to make it cleaner. No new changes are introduced.

andyzhangx · 2021-01-21T02:14:52Z

/test pull-kubernetes-e2e-azure-disk-vmss
/test pull-kubernetes-e2e-azure-disk

msau42 · 2021-01-23T01:19:28Z

/lgtm

Jiawei0227 · 2021-01-28T21:33:58Z

@andrewsykim Gentle ping. Hey Andrew, can you help to take a look? This is blocking the following steps of updating the topology label to GA. Thanks a lot

andrewsykim

/approve

for k8s.io/cloud-provider, thanks!

k8s-ci-robot · 2021-02-02T15:57:35Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: andrewsykim, 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:

~~plugin/pkg/admission/storage/persistentvolume/label/OWNERS~~ [andrewsykim,msau42]
~~staging/src/k8s.io/cloud-provider/OWNERS~~ [andrewsykim]
~~staging/src/k8s.io/csi-translation-lib/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 assigned msau42 Jan 8, 2021

k8s-ci-robot added release-note Denotes a PR that will be considered when it comes time to generate release notes. kind/bug Categorizes issue or PR as related to a bug. size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Jan 8, 2021

k8s-ci-robot requested review from mattcary, verult and wongma7 January 8, 2021 00:50

k8s-ci-robot added kind/cleanup Categorizes issue or PR as related to cleaning up code, process, or technical debt. sig/storage Categorizes an issue or PR as relevant to SIG Storage. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels Jan 8, 2021

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

Jiawei0227 commented Jan 8, 2021

View reviewed changes

Jiawei0227 changed the title ~~Preparation for Topology migration to GA from CSI migration~~ Preparation for Topology migration to GA for CSI migration Jan 8, 2021

msau42 reviewed Jan 8, 2021

View reviewed changes

Jiawei0227 force-pushed the translation-lib branch 2 times, most recently from 555f064 to aed3975 Compare January 8, 2021 19:03

wongma7 reviewed Jan 11, 2021

View reviewed changes

msau42 reviewed Jan 11, 2021

View reviewed changes

msau42 reviewed Jan 12, 2021

View reviewed changes

Jiawei0227 force-pushed the translation-lib branch from b0d313b to b0e5532 Compare January 12, 2021 04:32

msau42 reviewed Jan 13, 2021

View reviewed changes

k8s-ci-robot added size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. and removed size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Jan 13, 2021

Jiawei0227 commented Jan 16, 2021

View reviewed changes

msau42 reviewed Jan 20, 2021

View reviewed changes

Jiawei0227 force-pushed the translation-lib branch from c78bffb to 12e8d47 Compare January 20, 2021 07:10

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

k8s-ci-robot assigned andrewsykim Jan 20, 2021

Jiawei0227 added 2 commits January 20, 2021 10:49

Prepare for Topology migration to GA from CSI migration

67fed31

This also includes a change on CSI migration TranslateCSIToInTree where we remove the CSI topology and add Kubernetes Topology to the NodeAffinity

Jiawei0227 force-pushed the translation-lib branch from 12e8d47 to f83df5d Compare January 20, 2021 18:53

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

Jiawei0227 mentioned this pull request Jan 21, 2021

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

Closed

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

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

k8s-ci-robot merged commit 34f138f into kubernetes:master Feb 2, 2021

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

Jiawei0227 mentioned this pull request Feb 2, 2021

Update gce-pd volume topology label to GA #98700

Merged

github-actions bot mentioned this pull request Feb 3, 2021

Week Ending January 31, 2021 dev-obs/actus#339

Open

This was referenced Feb 11, 2021

Cut release with latest translation-lib kubernetes-csi/external-resizer#137

Closed

Make release with latest translation-lib kubernetes-csi/external-provisioner#563

Closed

Make release with latest translation-lib kubernetes-csi/external-attacher#290

Closed

jsafrane mentioned this pull request Feb 23, 2021

cinder: Translate CSI topology affinity into in-tree labels #99367

Merged

ialidzhikov mentioned this pull request Apr 8, 2022

gce: KCM detaches all in-tree volumes during update from K8s 1.20 to 1.21 #109354

Closed

		// In-tree topology has precedence over zone labels.
		func translateTopology(pv *v1.PersistentVolume, topologyKey string) error {

Preparation for Topology migration to GA for CSI migration #97823

Preparation for Topology migration to GA for CSI migration #97823

Conversation

Jiawei0227 commented Jan 8, 2021 • edited

k8s-ci-robot commented Jan 8, 2021

Jiawei0227 left a comment • 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

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

Jiawei0227 commented Jan 12, 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Jiawei0227 commented Jan 13, 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

andyzhangx commented Jan 20, 2021

msau42 commented Jan 20, 2021

msau42 commented Jan 20, 2021

Jiawei0227 commented Jan 20, 2021 • edited

andyzhangx commented Jan 21, 2021

msau42 commented Jan 23, 2021

Jiawei0227 commented Jan 28, 2021

andrewsykim left a comment

Choose a reason for hiding this comment

k8s-ci-robot commented Feb 2, 2021

Jiawei0227 commented Jan 8, 2021 •

edited

Jiawei0227 left a comment •

edited

Jiawei0227 commented Jan 20, 2021 •

edited