A complete new way

- 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

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

@@ -292,7 +292,7 @@ func (g *gcePersistentDiskCSITranslator) TranslateCSIPVToInTree(pv *v1.Persisten
 	}
 
 	// translate CSI topology to In-tree topology for rollback compatibility
-	if err := translateTopologyFromCSIToInTree(pv, GCEPDTopologyKey, gceRegionParser); err != nil {
+	if err := translateTopologyFromCSIToInTree(pv, GCEPDTopologyKey, gceRegionTopologyHandler); err != nil {
 		return nil, fmt.Errorf("failed to translate topology. PV:%+v. Error:%v", *pv, err)
 	}
 
@@ -377,37 +377,65 @@ func pdNameFromVolumeID(id string) (string, error) {
 	return splitID[volIDDiskNameValue], nil
 }
 
-func gceRegionParser(pv *v1.PersistentVolume) (string, error) {
+// gceRegionTopologyHandler will process the PV and add region
-	_, zoneLabel, regionLabel := getTopologyLabel(pv)
+// kubernetes topology label to its NodeAffinity and labels
+// It assumes the Zone NodeAffinity already exists
+func gceRegionTopologyHandler(pv *v1.PersistentVolume) error {
 
-	regionVal := getTopologyValues(pv, regionLabel)
+	// Make sure the necessary fields exist
-	// if we found the val in topology directly
+	if pv == nil || pv.Spec.NodeAffinity == nil || pv.Spec.NodeAffinity.Required == nil ||
-	if len(regionVal) != 0 {
+		pv.Spec.NodeAffinity.Required.NodeSelectorTerms == nil || len(pv.Spec.NodeAffinity.Required.NodeSelectorTerms) == 0 {
-		if len(regionVal) > 1 {
+		return nil
-			return "", fmt.Errorf("multiple regions found: %v", regionVal)
+	}
+
+	zoneLabel, regionLabel := getTopologyLabel(pv)
+
+	// process each term
+	for index, nodeSelectorTerm := range pv.Spec.NodeAffinity.Required.NodeSelectorTerms {
+		// In the first loop, see if regionLabel already exist
+		regionExist := false
+		var zoneVals []string
+		for _, nsRequirement := range nodeSelectorTerm.MatchExpressions {
+			if nsRequirement.Key == regionLabel {
+				regionExist = true
+				break
+			} else if nsRequirement.Key == zoneLabel {
+				zoneVals = append(zoneVals, nsRequirement.Values...)
+			}
 		}
-		return regionVal[0], nil
+		if regionExist {
+			// Regionlabel already exist in this term, skip it
+			continue
+		}
+		// If no regionLabel found, generate region label from the zoneLabel we collect from this term
+		regionVal, err := getRegionFromZones(zoneVals)
+		if err != nil {
+			return err
+		}
+		// Add the regionVal to this term
+		pv.Spec.NodeAffinity.Required.NodeSelectorTerms[index].MatchExpressions =
+			append(pv.Spec.NodeAffinity.Required.NodeSelectorTerms[index].MatchExpressions, v1.NodeSelectorRequirement{
+				Key:      regionLabel,
+				Operator: v1.NodeSelectorOpIn,
+				Values:   []string{regionVal},
+			})
+
 	}
 
-	// No region info found in NodeAffinity, generate it from Zones NodeAffinity
+	// Add region label
-	zonesVal := getTopologyValues(pv, zoneLabel)
+	regionVals := getTopologyValues(pv, regionLabel)
-	if len(zonesVal) == 0 {
+	if len(regionVals) == 1 {
-		// No kubernetes NodeAffinity found, generate it from CSI Topology
+		// We should only have exactly 1 region value
-		zonesVal = getTopologyValues(pv, GCEPDTopologyKey)
+		if pv.Labels == nil {
-	}
+			pv.Labels = make(map[string]string)
-	if len(zonesVal) != 0 {
+		}
-		return getRegionFromZones(zonesVal)
+		_, regionOK := pv.Labels[regionLabel]
+		if !regionOK {
+			pv.Labels[regionLabel] = regionVals[0]
+		}
 	}
 
-	// Not found anything in NodeAffinity, check region labels
+	return nil
-	regionValFromLabel, regionOK := pv.Labels[regionLabel]
-	if regionOK {
-		return regionValFromLabel, nil
-	}
-	// Not found anything in region labels, check zone labels
-	zonesVal = strings.Split(pv.Labels[zoneLabel], labelMultiZoneDelimiter)
-
-	return getRegionFromZones(zonesVal)
 }
 
 // TODO: Replace this with the imported one from GCE PD CSI Driver when

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

@@ -77,8 +77,17 @@ const (
 	zonesKey = "zones"
 )
 
-// replaceTopology overwrites an existing topology key by a new one.
+// replaceTopology overwrites an existing key in NodeAffinity by a new one.
+// If there are any newKey already exist in an expression of a term, we will
+// not combine the replaced key Values with the existing ones.
+// So there might be duplication if there is any newKey expression
+// already in the terms.
 func replaceTopology(pv *v1.PersistentVolume, oldKey, newKey string) error {
+	// Make sure the necessary fields exist
+	if pv == nil || pv.Spec.NodeAffinity == nil || pv.Spec.NodeAffinity.Required == nil ||
+		pv.Spec.NodeAffinity.Required.NodeSelectorTerms == nil || len(pv.Spec.NodeAffinity.Required.NodeSelectorTerms) == 0 {
+		return nil
+	}
 	for i := range pv.Spec.NodeAffinity.Required.NodeSelectorTerms {
 		for j, r := range pv.Spec.NodeAffinity.Required.NodeSelectorTerms[i].MatchExpressions {
 			if r.Key == oldKey {
@@ -86,10 +95,14 @@ func replaceTopology(pv *v1.PersistentVolume, oldKey, newKey string) error {
 			}
 		}
 	}
+
 	return nil
 }
 
-// getTopologyValues returns all unique topology values with the given key found in the PV.
+// getTopologyValues returns all unique topology values with the given key found in
+// the PV NodeAffinity. Sort by alphabetical order.
+// This function collapses multiple zones into a list that is ORed. This assumes that
+// the plugin does not support a constraint like key in "zone1" AND "zone2"
 func getTopologyValues(pv *v1.PersistentVolume, key string) []string {
 	if pv.Spec.NodeAffinity == nil ||
 		pv.Spec.NodeAffinity.Required == nil ||
@@ -107,6 +120,7 @@ func getTopologyValues(pv *v1.PersistentVolume, key string) []string {
 			}
 		}
 	}
+	// remove duplication and sort them in order for better usage
 	var re []string
 	for k := range values {
 		re = append(re, k)
@@ -115,7 +129,7 @@ func getTopologyValues(pv *v1.PersistentVolume, key string) []string {
 	return re
 }
 
-// addTopology appends the topology to the given PV.
+// addTopology appends the topology to the given PV to all Terms.
 func addTopology(pv *v1.PersistentVolume, topologyKey string, zones []string) error {
 	// Make sure there are no duplicate or empty strings
 	filteredZones := sets.String{}
@@ -161,125 +175,67 @@ func addTopology(pv *v1.PersistentVolume, topologyKey string, zones []string) er
 	return nil
 }
 
-// removeTopology removes the topology from the given PV. Return false
-// if the topology key is not found
-func removeTopology(pv *v1.PersistentVolume, topologyKey string) bool {
-	// Make sure the necessary fields exist
-	if pv == nil || pv.Spec.NodeAffinity == nil || pv.Spec.NodeAffinity.Required == nil ||
-		pv.Spec.NodeAffinity.Required.NodeSelectorTerms == nil || len(pv.Spec.NodeAffinity.Required.NodeSelectorTerms) == 0 {
-		return false
-	}
-
-	found := true
-	succeed := false
-	for found == true {
-		found = false
-		var termIndexRemoved []int
-		for termIndex, nodeSelectorTerms := range pv.Spec.NodeAffinity.Required.NodeSelectorTerms {
-			nsrequirements := nodeSelectorTerms.MatchExpressions
-
-			index := -1
-			for i, nodeSelectorRequirement := range nsrequirements {
-				if nodeSelectorRequirement.Key == topologyKey {
-					index = i
-					break
-				}
-			}
-			// We found the key that need to be removed
-			if index != -1 {
-				nsrequirements[len(nsrequirements)-1], nsrequirements[index] = nsrequirements[index], nsrequirements[len(nsrequirements)-1]
-				pv.Spec.NodeAffinity.Required.NodeSelectorTerms[termIndex].MatchExpressions = nsrequirements[:len(nsrequirements)-1]
-				if len(nsrequirements)-1 == 0 {
-					// No other expression left, the whole term should be removed
-					termIndexRemoved = append(termIndexRemoved, termIndex)
-				}
-				succeed = true
-				found = true
-			}
-		}
-
-		if len(termIndexRemoved) > 0 {
-			for i, index := range termIndexRemoved {
-				index = index - i
-				nodeSelectorTerms := pv.Spec.NodeAffinity.Required.NodeSelectorTerms
-				nodeSelectorTerms[len(nodeSelectorTerms)-1], nodeSelectorTerms[index] = nodeSelectorTerms[index], nodeSelectorTerms[len(nodeSelectorTerms)-1]
-				pv.Spec.NodeAffinity.Required.NodeSelectorTerms = nodeSelectorTerms[:len(nodeSelectorTerms)-1]
-			}
-		}
-	}
-
-	return succeed
-}
-
 // translateTopologyFromInTreeToCSI converts existing zone labels or in-tree topology to CSI topology.
-// In-tree topology has precedence over zone labels.
+// In-tree topology has precedence over zone labels. When both in-tree topology and zone labels exist
-func translateTopologyFromInTreeToCSI(pv *v1.PersistentVolume, topologyKey string) error {
+// for a particular CSI topology, in-tree topology will be used.
-	// If topology is already set, assume the content is accurate
+// This function will remove the Beta version Kubernetes topology label in case the node upgrade to a
-	if len(getTopologyValues(pv, topologyKey)) > 0 {
+// newer version where it does not have any Beta topology label anymore
-		return nil
+func translateTopologyFromInTreeToCSI(pv *v1.PersistentVolume, csiTopologyKey string) error {
-	}
 
-	_, zoneLabel, _ := getTopologyLabel(pv)
+	zoneLabel, regionLabel := getTopologyLabel(pv)
 
-	// migrate topology node affinity
+	// If Zone kubernetes topology exist, replace it to use csiTopologyKey
 	zones := getTopologyValues(pv, zoneLabel)
 	if len(zones) > 0 {
-		return replaceTopology(pv, zoneLabel, topologyKey)
+		replaceTopology(pv, zoneLabel, csiTopologyKey)
+	} else {
+		// if nothing is in the NodeAffinity, try to fetch the topology from PV labels
+		if label, ok := pv.Labels[zoneLabel]; ok {
+			zones = strings.Split(label, labelMultiZoneDelimiter)
+			if len(zones) > 0 {
+				addTopology(pv, csiTopologyKey, zones)
+			}
+		}
 	}
 
-	// if nothing is in the NodeAffinity, try to fetch the topology from PV labels
+	// if the in-tree PV has beta region label, replace it with GA label to ensure
-	label, ok := pv.Labels[zoneLabel]
+	// the scheduler is able to schedule it on new nodes with only GA kubernetes label
-	if ok {
+	// No need to check it for zone label because it has already been replaced if exist
-		zones = strings.Split(label, labelMultiZoneDelimiter)
+	if regionLabel == v1.LabelFailureDomainBetaRegion {
-		if len(zones) > 0 {
+		regions := getTopologyValues(pv, regionLabel)
-			return addTopology(pv, topologyKey, zones)
+		if len(regions) > 0 {
+			replaceTopology(pv, regionLabel, v1.LabelTopologyRegion)
 		}
 	}
 
 	return nil
 }
 
-// getTopologyLabel checks if the kubernetes topology used in this PV is GA
+// getTopologyLabel checks if the kubernetes topology label used in this
-// and return the zone/region label used.
+// PV is GA and return the zone/region label used.
-// It first check the NodeAffinity to find topology. If nothing is found,
+// The version checking follows the following orders
-// It checks the PV labels. If it is empty in both places, it will return
+// 1. Check NodeAffinity
-// GA label by default
+//   1.1 Check if zoneGA exists, if yes return GA labels
-func getTopologyLabel(pv *v1.PersistentVolume) (bool, string, string) {
+//   1.2 Check if zoneBeta exists, if yes return Beta labels
+// 2. Check PV labels
+//   2.1 Check if zoneGA exists, if yes return GA labels
+//   2.2 Check if zoneBeta exists, if yes return Beta labels
+func getTopologyLabel(pv *v1.PersistentVolume) (zoneLabel string, regionLabel string) {
 
-	// Check the NodeAffinity first for the topology version
+	if zoneGA := TopologyKeyExist(v1.LabelTopologyZone, pv.Spec.NodeAffinity); zoneGA {
-	zoneGA := TopologyKeyExist(v1.LabelTopologyZone, pv.Spec.NodeAffinity)
+		return v1.LabelTopologyZone, v1.LabelTopologyRegion
-	regionGA := TopologyKeyExist(v1.LabelTopologyRegion, pv.Spec.NodeAffinity)
-	if zoneGA || regionGA {
-		// GA NodeAffinity exists
-		return true, v1.LabelTopologyZone, v1.LabelTopologyRegion
 	}
-
+	if zoneBeta := TopologyKeyExist(v1.LabelFailureDomainBetaZone, pv.Spec.NodeAffinity); zoneBeta {
-	// If no GA topology in NodeAffinity, check the beta one
+		return v1.LabelFailureDomainBetaZone, v1.LabelFailureDomainBetaRegion
-	zoneBeta := TopologyKeyExist(v1.LabelFailureDomainBetaZone, pv.Spec.NodeAffinity)
-	regionBeta := TopologyKeyExist(v1.LabelFailureDomainBetaRegion, pv.Spec.NodeAffinity)
-	if zoneBeta || regionBeta {
-		// Beta NodeAffinity exists, GA NodeAffinity not exist
-		return false, v1.LabelFailureDomainBetaZone, v1.LabelFailureDomainBetaRegion
 	}
-
+	if _, zoneGA := pv.Labels[v1.LabelTopologyZone]; zoneGA {
-	// If nothing is in the NodeAfinity, we should check pv labels
+		return v1.LabelTopologyZone, v1.LabelTopologyRegion
-	_, zoneGA = pv.Labels[v1.LabelTopologyZone]
-	_, regionGA = pv.Labels[v1.LabelTopologyRegion]
-	if zoneGA || regionGA {
-		// NodeAffinity not exist, GA label exists
-		return true, v1.LabelTopologyZone, v1.LabelTopologyRegion
 	}
-
+	if _, zoneBeta := pv.Labels[v1.LabelFailureDomainBetaZone]; zoneBeta {
-	// If GA label not exists, check beta version
+		return v1.LabelFailureDomainBetaZone, v1.LabelFailureDomainBetaRegion
-	_, zoneBeta = pv.Labels[v1.LabelFailureDomainBetaZone]
-	_, regionBeta = pv.Labels[v1.LabelFailureDomainBetaRegion]
-	if zoneBeta || regionBeta {
-		// Beta label exists, NodeAffinity not exist, GA label not exists
-		return false, v1.LabelFailureDomainBetaZone, v1.LabelFailureDomainBetaRegion
 	}
-
 	// No labels or NodeAffinity exist, default to GA version
-	return true, v1.LabelTopologyZone, v1.LabelTopologyRegion
+	return v1.LabelTopologyZone, v1.LabelTopologyRegion
 }
 
 // TopologyKeyExist checks if a certain key exists in a VolumeNodeAffinity
@@ -299,91 +255,50 @@ func TopologyKeyExist(key string, vna *v1.VolumeNodeAffinity) bool {
 	return false
 }
 
-type regionParser func(pv *v1.PersistentVolume) (string, error)
+type regionTopologyHandler func(pv *v1.PersistentVolume) error
 
-// translateTopologyFromCSIToInTree translate a CSI PV topology to
+// translateTopologyFromCSIToInTree translate a CSI topology to
-// Kubernetes topology and add labels to it. Note that this function
+// Kubernetes topology and add topology labels to it. Note that this function
-// will only work for plugin with single topologyKey. If a plugin has
+// will only work for plugin with a single topologyKey that translates to
-// more than one topologyKey, it will need to be processed separately
+// Kubernetes zone(and region if regionTopologyHandler is passed in).
-// by the plugin.
+// If a plugin has more than one topologyKey, it will need to be processed
-// regionParser is a function to generate region val based on PV
+// separately by the plugin.
-// if the function is not set, we will not set region topology.
+// regionTopologyHandler is a function to add region topology NodeAffinity
-// 1. Remove any existing CSI topologyKey from NodeAffinity
+// and labels for the given PV. It assumes the Zone NodeAffinity already exists
-// 2. Add Kubernetes Topology in the NodeAffinity if it does not exist
+// If regionTopologyHandler is nil, no region NodeAffinity will be added
-//    2.1 Try to use CSI topology values to recover the Kubernetes topology first
+//
-//    2.2 If CSI topology values does not exist, try to use PV label
+// 1. Replace all CSI topology to Kubernetes Zone topology label
-// 3. Add Kubernetes Topology labels(zone and region)
+// 2. Process and generate region topology if a regionTopologyHandler is passed
-func translateTopologyFromCSIToInTree(pv *v1.PersistentVolume, topologyKey string, regionParser regionParser) error {
+// 3. Add Kubernetes Topology labels(zone) if they do not exist
+func translateTopologyFromCSIToInTree(pv *v1.PersistentVolume, csiTopologyKey string, regionTopologyHandler regionTopologyHandler) error {
 
-	csiTopologyZoneValues := getTopologyValues(pv, topologyKey)
+	zoneLabel, _ := getTopologyLabel(pv)
 
-	// 1. Frist remove the CSI topology Key
+	// 1. Replace all CSI topology to Kubernetes Zone label
-	removeTopology(pv, topologyKey)
+	err := replaceTopology(pv, csiTopologyKey, zoneLabel)
+	if err != nil {
+		return fmt.Errorf("Failed to replace CSI topology to Kubernetes topology, error: %v", err)
+	}
 
-	_, zoneLabel, regionLabel := getTopologyLabel(pv)
+	// 2. Take care of region topology if a regionTopologyHandler is passed
-	zoneLabelVal, zoneOK := pv.Labels[zoneLabel]
+	if regionTopologyHandler != nil {
-	regionLabelVal, regionOK := pv.Labels[regionLabel]
+		// let's make less strict on this one. Even if there is an error in the region processing, just ignore it
-
+		err = regionTopologyHandler(pv)
-	// 2. Add Kubernetes Topology in the NodeAffinity if it does not exist
+		if err != nil {
-	// Check if Kubernetes Zone Topology already exist
+			return fmt.Errorf("Failed to handle region topology. error: %v", err)
-	topologyZoneValue := getTopologyValues(pv, zoneLabel)
-	if len(topologyZoneValue) == 0 {
-		// No Kubernetes Topology exist in the current PV, we need to add it
-		// 2.1 Let's try to use CSI topology to recover the zone topology first
-		if len(csiTopologyZoneValues) > 0 {
-			err := addTopology(pv, zoneLabel, csiTopologyZoneValues)
-			if err != nil {
-				return fmt.Errorf("Failed to add Kubernetes topology zone to PV NodeAffinity. %v", err)
-			}
-		} else if zoneOK {
-			// 2.2 If no CSI topology values exist, try to search PV labels for zone labels
-			zones := strings.Split(zoneLabelVal, labelMultiZoneDelimiter)
-			if len(zones) > 0 {
-				err := addTopology(pv, zoneLabel, zones)
-				if err != nil {
-					return fmt.Errorf("Failed to add Kubernetes topology zone to PV NodeAffinity. %v", err)
-				}
-			}
 		}
 	}
 
-	// Check if Kubernetes Region Topology already exist
+	// 3. Add labels about Kubernetes Topology
-	topologyRegionValue := getTopologyValues(pv, regionLabel)
+	zoneVals := getTopologyValues(pv, zoneLabel)
-	if len(topologyRegionValue) == 0 {
+	if len(zoneVals) > 0 {
-		// 2.1 Let's try to use CSI topology to recover the region topology first
-		if len(csiTopologyZoneValues) > 0 && regionParser != nil {
-			regionVal, err := regionParser(pv)
-			if err != nil {
-				return fmt.Errorf("Failed to parse zones value(%v) to region label %v", csiTopologyZoneValues, err)
-			}
-			err = addTopology(pv, regionLabel, []string{regionVal})
-			if err != nil {
-				return fmt.Errorf("Failed to add Kubernetes topology region to PV NodeAffinity. %v", err)
-			}
-		} else if regionOK {
-			// 2.2 If no CSI topology values exist, try to search PV labels for region labels
-			err := addTopology(pv, regionLabel, []string{regionLabelVal})
-			if err != nil {
-				return fmt.Errorf("Failed to add Kubernetes topology region to PV NodeAffinity. %v", err)
-			}
-		}
-	}
-
-	// 3. Add Kubernetes Topology labels, if it already exists, we trust it
-	if len(csiTopologyZoneValues) > 0 {
 		if pv.Labels == nil {
 			pv.Labels = make(map[string]string)
 		}
+		_, zoneOK := pv.Labels[zoneLabel]
 		if !zoneOK {
-			csiTopologyZoneValStr := strings.Join(csiTopologyZoneValues, labelMultiZoneDelimiter)
+			zoneValStr := strings.Join(zoneVals, labelMultiZoneDelimiter)
-			pv.Labels[zoneLabel] = csiTopologyZoneValStr
+			pv.Labels[zoneLabel] = zoneValStr
-		}
-		if !regionOK && regionParser != nil {
-			regionVal, err := regionParser(pv)
-			if err != nil {
-				return fmt.Errorf("Failed to parse zones value(%v) to region label %v", csiTopologyZoneValues, err)
-			}
-			pv.Labels[regionLabel] = regionVal
 		}
 	}
 

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

@@ -29,7 +29,7 @@ var (
 		v1.LabelTopologyZone:   "us-east1-a",
 		v1.LabelTopologyRegion: "us-east1",
 	}
-	useast1aGANodeSelectorTerm = []v1.NodeSelectorTerm{
+	useast1aGANodeSelectorTermZoneFirst = []v1.NodeSelectorTerm{
 		{
 			MatchExpressions: []v1.NodeSelectorRequirement{
 				{
@@ -46,12 +46,29 @@ var (
 		},
 	}
 
+	useast1aGANodeSelectorTermRegionFirst = []v1.NodeSelectorTerm{
+		{
+			MatchExpressions: []v1.NodeSelectorRequirement{
+				{
+					Key:      v1.LabelTopologyRegion,
+					Operator: v1.NodeSelectorOpIn,
+					Values:   []string{"us-east1"},
+				},
+				{
+					Key:      v1.LabelTopologyZone,
+					Operator: v1.NodeSelectorOpIn,
+					Values:   []string{"us-east1-a"},
+				},
+			},
+		},
+	}
+
 	uswest2bBetaLabels = map[string]string{
 		v1.LabelFailureDomainBetaZone:   "us-west2-b",
 		v1.LabelFailureDomainBetaRegion: "us-west2",
 	}
 
-	uswest2bBetaNodeSelectorTerm = []v1.NodeSelectorTerm{
+	uswest2bBetaNodeSelectorTermZoneFirst = []v1.NodeSelectorTerm{
 		{
 			MatchExpressions: []v1.NodeSelectorRequirement{
 				{
@@ -67,6 +84,23 @@ var (
 			},
 		},
 	}
+
+	uswest2bBetaNodeSelectorTermRegionFirst = []v1.NodeSelectorTerm{
+		{
+			MatchExpressions: []v1.NodeSelectorRequirement{
+				{
+					Key:      v1.LabelFailureDomainBetaRegion,
+					Operator: v1.NodeSelectorOpIn,
+					Values:   []string{"us-west2"},
+				},
+				{
+					Key:      v1.LabelFailureDomainBetaZone,
+					Operator: v1.NodeSelectorOpIn,
+					Values:   []string{"us-west2-b"},
+				},
+			},
+		},
+	}
 )
 
 func TestTranslateTopologyFromCSIToInTree(t *testing.T) {
@@ -74,16 +108,16 @@ func TestTranslateTopologyFromCSIToInTree(t *testing.T) {
 		name                      string
 		key                       string
 		expErr                    bool
-		regionParser              regionParser
+		regionTopologyHandler     regionTopologyHandler
 		pv                        *v1.PersistentVolume
 		expectedNodeSelectorTerms []v1.NodeSelectorTerm
 		expectedLabels            map[string]string
 	}{
 		{
-			name:         "Remove CSI Topology Key and do not change existing GA Kubernetes topology",
+			name:                  "Remove CSI Topology Key and do not change existing GA Kubernetes topology",
-			key:          GCEPDTopologyKey,
+			key:                   GCEPDTopologyKey,
-			expErr:       false,
+			expErr:                false,
-			regionParser: gceRegionParser,
+			regionTopologyHandler: gceRegionTopologyHandler,
 			pv: &v1.PersistentVolume{
 				ObjectMeta: metav1.ObjectMeta{
 					Name: "gcepd", Namespace: "myns",
@@ -95,11 +129,6 @@ func TestTranslateTopologyFromCSIToInTree(t *testing.T) {
 							NodeSelectorTerms: []v1.NodeSelectorTerm{
 								{
 									MatchExpressions: []v1.NodeSelectorRequirement{
-										{
-											Key:      v1.LabelTopologyZone,
-											Operator: v1.NodeSelectorOpIn,
-											Values:   []string{"us-east1-a"},
-										},
 										{
 											Key:      v1.LabelTopologyRegion,
 											Operator: v1.NodeSelectorOpIn,
@@ -108,7 +137,7 @@ func TestTranslateTopologyFromCSIToInTree(t *testing.T) {
 										{
 											Key:      GCEPDTopologyKey,
 											Operator: v1.NodeSelectorOpIn,
-											Values:   []string{"whatever"},
+											Values:   []string{"us-east1-a"},
 										},
 									},
 								},
@@ -117,14 +146,14 @@ func TestTranslateTopologyFromCSIToInTree(t *testing.T) {
 					},
 				},
 			},
-			expectedNodeSelectorTerms: useast1aGANodeSelectorTerm,
+			expectedNodeSelectorTerms: useast1aGANodeSelectorTermRegionFirst,
 			expectedLabels:            useast1aGALabels,
 		},
 		{
-			name:         "Remove CSI Topology Key and do not change existing Beta Kubernetes topology",
+			name:                  "Remove CSI Topology Key and do not change existing Beta Kubernetes topology",
-			key:          GCEPDTopologyKey,
+			key:                   GCEPDTopologyKey,
-			expErr:       false,
+			expErr:                false,
-			regionParser: gceRegionParser,
+			regionTopologyHandler: gceRegionTopologyHandler,
 			pv: &v1.PersistentVolume{
 				ObjectMeta: metav1.ObjectMeta{
 					Name: "gcepd", Namespace: "myns",
@@ -136,11 +165,6 @@ func TestTranslateTopologyFromCSIToInTree(t *testing.T) {
 							NodeSelectorTerms: []v1.NodeSelectorTerm{
 								{
 									MatchExpressions: []v1.NodeSelectorRequirement{
-										{
-											Key:      v1.LabelFailureDomainBetaZone,
-											Operator: v1.NodeSelectorOpIn,
-											Values:   []string{"us-west2-b"},
-										},
 										{
 											Key:      v1.LabelFailureDomainBetaRegion,
 											Operator: v1.NodeSelectorOpIn,
@@ -149,7 +173,7 @@ func TestTranslateTopologyFromCSIToInTree(t *testing.T) {
 										{
 											Key:      GCEPDTopologyKey,
 											Operator: v1.NodeSelectorOpIn,
-											Values:   []string{"whatever"},
+											Values:   []string{"us-west2-b"},
 										},
 									},
 								},
@@ -158,14 +182,14 @@ func TestTranslateTopologyFromCSIToInTree(t *testing.T) {
 					},
 				},
 			},
-			expectedNodeSelectorTerms: uswest2bBetaNodeSelectorTerm,
+			expectedNodeSelectorTerms: uswest2bBetaNodeSelectorTermRegionFirst,
 			expectedLabels:            uswest2bBetaLabels,
 		},
 		{
-			name:         "Remove CSI Topology Key and add Kubernetes topology from NodeAffinity, ignore labels",
+			name:                  "Remove CSI Topology Key and add Kubernetes topology from NodeAffinity, ignore labels",
-			key:          GCEPDTopologyKey,
+			key:                   GCEPDTopologyKey,
-			expErr:       false,
+			expErr:                false,
-			regionParser: gceRegionParser,
+			regionTopologyHandler: gceRegionTopologyHandler,
 			pv: &v1.PersistentVolume{
 				ObjectMeta: metav1.ObjectMeta{
 					Name: "gcepd", Namespace: "myns",
@@ -192,17 +216,17 @@ func TestTranslateTopologyFromCSIToInTree(t *testing.T) {
 					},
 				},
 			},
-			expectedNodeSelectorTerms: useast1aGANodeSelectorTerm,
+			expectedNodeSelectorTerms: useast1aGANodeSelectorTermZoneFirst,
 			expectedLabels: map[string]string{
 				v1.LabelTopologyRegion: "existingRegion",
 				v1.LabelTopologyZone:   "existingZone",
 			},
 		},
 		{
-			name:         "Add GA Kubernetes topology from labels",
+			name:                  "No CSI topology label exists and no change to the NodeAffinity",
-			key:          GCEPDTopologyKey,
+			key:                   GCEPDTopologyKey,
-			expErr:       false,
+			expErr:                false,
-			regionParser: gceRegionParser,
+			regionTopologyHandler: gceRegionTopologyHandler,
 			pv: &v1.PersistentVolume{
 				ObjectMeta: metav1.ObjectMeta{
 					Name: "gcepd", Namespace: "myns",
@@ -211,33 +235,25 @@ func TestTranslateTopologyFromCSIToInTree(t *testing.T) {
 						v1.LabelTopologyRegion: "existingRegion",
 					},
 				},
-			},
+				Spec: v1.PersistentVolumeSpec{
-			expectedNodeSelectorTerms: []v1.NodeSelectorTerm{
+					NodeAffinity: &v1.VolumeNodeAffinity{
-				{
+						Required: &v1.NodeSelector{
-					MatchExpressions: []v1.NodeSelectorRequirement{
+							NodeSelectorTerms: []v1.NodeSelectorTerm{},
-						{
-							Key:      v1.LabelTopologyZone,
-							Operator: v1.NodeSelectorOpIn,
-							Values:   []string{"existingZone"},
-						},
-						{
-							Key:      v1.LabelTopologyRegion,
-							Operator: v1.NodeSelectorOpIn,
-							Values:   []string{"existingRegion"},
 						},
 					},
 				},
 			},
+			expectedNodeSelectorTerms: []v1.NodeSelectorTerm{},
 			expectedLabels: map[string]string{
 				v1.LabelTopologyZone:   "existingZone",
 				v1.LabelTopologyRegion: "existingRegion",
 			},
 		},
 		{
-			name:         "Generate GA labels and kubernetes topology only from CSI topology",
+			name:                  "Generate GA labels and kubernetes topology only from CSI topology",
-			key:          GCEPDTopologyKey,
+			key:                   GCEPDTopologyKey,
-			expErr:       false,
+			expErr:                false,
-			regionParser: gceRegionParser,
+			regionTopologyHandler: gceRegionTopologyHandler,
 			pv: &v1.PersistentVolume{
 				ObjectMeta: metav1.ObjectMeta{
 					Name: "gcepd", Namespace: "myns",
@@ -260,14 +276,14 @@ func TestTranslateTopologyFromCSIToInTree(t *testing.T) {
 					},
 				},
 			},
-			expectedNodeSelectorTerms: useast1aGANodeSelectorTerm,
+			expectedNodeSelectorTerms: useast1aGANodeSelectorTermZoneFirst,
 			expectedLabels:            useast1aGALabels,
 		},
 		{
-			name:         "Generate Beta labels and kubernetes topology from CSI topology with partial Beta NodeAffinity",
+			name:                  "Generate Beta labels and kubernetes topology from Beta NodeAffinity",
-			key:          GCEPDTopologyKey,
+			key:                   GCEPDTopologyKey,
-			expErr:       false,
+			expErr:                false,
-			regionParser: gceRegionParser,
+			regionTopologyHandler: gceRegionTopologyHandler,
 			pv: &v1.PersistentVolume{
 				ObjectMeta: metav1.ObjectMeta{
 					Name: "gcepd", Namespace: "myns",
@@ -278,11 +294,6 @@ func TestTranslateTopologyFromCSIToInTree(t *testing.T) {
 							NodeSelectorTerms: []v1.NodeSelectorTerm{
 								{
 									MatchExpressions: []v1.NodeSelectorRequirement{
-										{
-											Key:      GCEPDTopologyKey,
-											Operator: v1.NodeSelectorOpIn,
-											Values:   []string{"us-west2-b"},
-										},
 										{
 											Key:      v1.LabelFailureDomainBetaZone,
 											Operator: v1.NodeSelectorOpIn,
@@ -295,14 +306,14 @@ func TestTranslateTopologyFromCSIToInTree(t *testing.T) {
 					},
 				},
 			},
-			expectedNodeSelectorTerms: uswest2bBetaNodeSelectorTerm,
+			expectedNodeSelectorTerms: uswest2bBetaNodeSelectorTermZoneFirst,
 			expectedLabels:            uswest2bBetaLabels,
 		},
 		{
-			name:         "regionParser is missing and only zone labels get generated",
+			name:                  "regionParser is missing and only zone labels get generated",
-			key:          GCEPDTopologyKey,
+			key:                   GCEPDTopologyKey,
-			expErr:       false,
+			expErr:                false,
-			regionParser: nil,
+			regionTopologyHandler: nil,
 			pv: &v1.PersistentVolume{
 				ObjectMeta: metav1.ObjectMeta{
 					Name: "gcepd", Namespace: "myns",
@@ -341,10 +352,10 @@ func TestTranslateTopologyFromCSIToInTree(t *testing.T) {
 			},
 		},
 		{
-			name:         "Remove multi-term CSI Topology Key and add GA Kubernetes topology",
+			name:                  "Replace multi-term CSI Topology Key and add Region Kubernetes topology for both",
-			key:          GCEPDTopologyKey,
+			key:                   GCEPDTopologyKey,
-			expErr:       false,
+			expErr:                false,
-			regionParser: gceRegionParser,
+			regionTopologyHandler: gceRegionTopologyHandler,
 			pv: &v1.PersistentVolume{
 				ObjectMeta: metav1.ObjectMeta{
 					Name: "gcepd", Namespace: "myns",
@@ -382,7 +393,21 @@ func TestTranslateTopologyFromCSIToInTree(t *testing.T) {
 						{
 							Key:      v1.LabelTopologyZone,
 							Operator: v1.NodeSelectorOpIn,
-							Values:   []string{"us-east1-a", "us-east1-c"},
+							Values:   []string{"us-east1-a"},
+						},
+						{
+							Key:      v1.LabelTopologyRegion,
+							Operator: v1.NodeSelectorOpIn,
+							Values:   []string{"us-east1"},
+						},
+					},
+				},
+				{
+					MatchExpressions: []v1.NodeSelectorRequirement{
+						{
+							Key:      v1.LabelTopologyZone,
+							Operator: v1.NodeSelectorOpIn,
+							Values:   []string{"us-east1-c"},
 						},
 						{
 							Key:      v1.LabelTopologyRegion,
@@ -401,7 +426,7 @@ func TestTranslateTopologyFromCSIToInTree(t *testing.T) {
 
 	for _, tc := range testCases {
 		t.Logf("Running test: %v", tc.name)
-		err := translateTopologyFromCSIToInTree(tc.pv, tc.key, tc.regionParser)
+		err := translateTopologyFromCSIToInTree(tc.pv, tc.key, tc.regionTopologyHandler)
 		if err != nil && !tc.expErr {
 			t.Errorf("Did not expect an error, got: %v", err)
 		}
@@ -418,6 +443,99 @@ func TestTranslateTopologyFromCSIToInTree(t *testing.T) {
 	}
 }
 
+func TestTranslateTopologyFromInTreeToCSI(t *testing.T) {
+	testCases := []struct {
+		name                      string
+		key                       string
+		expErr                    bool
+		pv                        *v1.PersistentVolume
+		expectedNodeSelectorTerms []v1.NodeSelectorTerm
+	}{
+		{
+			name:   "Replace GA Kubernetes Zone Topology to GCE CSI Topology",
+			key:    GCEPDTopologyKey,
+			expErr: false,
+			pv: &v1.PersistentVolume{
+				ObjectMeta: metav1.ObjectMeta{
+					Name: "gcepd", Namespace: "myns",
+					Labels: useast1aGALabels,
+				},
+				Spec: v1.PersistentVolumeSpec{
+					NodeAffinity: &v1.VolumeNodeAffinity{
+						Required: &v1.NodeSelector{
+							NodeSelectorTerms: useast1aGANodeSelectorTermZoneFirst,
+						},
+					},
+				},
+			},
+			expectedNodeSelectorTerms: []v1.NodeSelectorTerm{
+				{
+					MatchExpressions: []v1.NodeSelectorRequirement{
+						{
+							Key:      GCEPDTopologyKey,
+							Operator: v1.NodeSelectorOpIn,
+							Values:   []string{"us-east1-a"},
+						},
+						{
+							Key:      v1.LabelTopologyRegion,
+							Operator: v1.NodeSelectorOpIn,
+							Values:   []string{"us-east1"},
+						},
+					},
+				},
+			},
+		},
+		{
+			name:   "Replace Beta Kubernetes Topology to GCE CSI Topology and upgrade region label",
+			key:    GCEPDTopologyKey,
+			expErr: false,
+			pv: &v1.PersistentVolume{
+				ObjectMeta: metav1.ObjectMeta{
+					Name: "gcepd", Namespace: "myns",
+					Labels: useast1aGALabels,
+				},
+				Spec: v1.PersistentVolumeSpec{
+					NodeAffinity: &v1.VolumeNodeAffinity{
+						Required: &v1.NodeSelector{
+							NodeSelectorTerms: uswest2bBetaNodeSelectorTermZoneFirst,
+						},
+					},
+				},
+			},
+			expectedNodeSelectorTerms: []v1.NodeSelectorTerm{
+				{
+					MatchExpressions: []v1.NodeSelectorRequirement{
+						{
+							Key:      GCEPDTopologyKey,
+							Operator: v1.NodeSelectorOpIn,
+							Values:   []string{"us-west2-b"},
+						},
+						{
+							Key:      v1.LabelTopologyRegion,
+							Operator: v1.NodeSelectorOpIn,
+							Values:   []string{"us-west2"},
+						},
+					},
+				},
+			},
+		},
+	}
+	for _, tc := range testCases {
+		t.Logf("Running test: %v", tc.name)
+		err := translateTopologyFromInTreeToCSI(tc.pv, tc.key)
+		if err != nil && !tc.expErr {
+			t.Errorf("Did not expect an error, got: %v", err)
+		}
+		if err == nil && tc.expErr {
+			t.Errorf("Expected an error but did not get one")
+		}
+
+		if !reflect.DeepEqual(tc.pv.Spec.NodeAffinity.Required.NodeSelectorTerms, tc.expectedNodeSelectorTerms) {
+			t.Errorf("Expected topology: %v, but got: %v", tc.expectedNodeSelectorTerms, tc.pv.Spec.NodeAffinity.Required.NodeSelectorTerms)
+		}
+	}
+}
+
 func TestTranslateAllowedTopologies(t *testing.T) {
 	testCases := []struct {
 		name            string
@@ -608,17 +726,19 @@ func TestAddTopology(t *testing.T) {
 	}
 }
 
-func TestRemoveTopology(t *testing.T) {
+func TestReplaceTopology(t *testing.T) {
 	testCases := []struct {
 		name             string
-		topologyKey      string
+		oldKey           string
+		newKey           string
 		pv               *v1.PersistentVolume
 		expOk            bool
 		expectedAffinity *v1.VolumeNodeAffinity
 	}{
 		{
-			name:        "Remove single csi topology from PV",
+			name:   "Replace single csi topology from PV",
-			topologyKey: GCEPDTopologyKey,
+			oldKey: GCEPDTopologyKey,
+			newKey: v1.LabelTopologyZone,
 			pv: makePVWithNodeSelectorTerms([]v1.NodeSelectorTerm{
 				{
 					MatchExpressions: []v1.NodeSelectorRequirement{
@@ -633,13 +753,24 @@ func TestRemoveTopology(t *testing.T) {
 			expOk: true,
 			expectedAffinity: &v1.VolumeNodeAffinity{
 				Required: &v1.NodeSelector{
-					NodeSelectorTerms: []v1.NodeSelectorTerm{},
+					NodeSelectorTerms: []v1.NodeSelectorTerm{
+						{
+							MatchExpressions: []v1.NodeSelectorRequirement{
+								{
+									Key:      v1.LabelTopologyZone,
+									Operator: v1.NodeSelectorOpIn,
+									Values:   []string{"us-east1-a"},
+								},
+							},
+						},
+					},
 				},
 			},
 		},
 		{
-			name:        "Not found the topology key so do nothing",
+			name:   "Not found the topology key so do nothing",
-			topologyKey: GCEPDTopologyKey,
+			oldKey: GCEPDTopologyKey,
+			newKey: v1.LabelTopologyZone,
 			pv: makePVWithNodeSelectorTerms([]v1.NodeSelectorTerm{
 				{
 					MatchExpressions: []v1.NodeSelectorRequirement{
@@ -669,8 +800,9 @@ func TestRemoveTopology(t *testing.T) {
 			},
 		},
 		{
-			name:        "Remove the topology key from multiple terms",
+			name:   "Replace the topology key from multiple terms",
-			topologyKey: GCEPDTopologyKey,
+			oldKey: GCEPDTopologyKey,
+			newKey: v1.LabelTopologyZone,
 			pv: makePVWithNodeSelectorTerms([]v1.NodeSelectorTerm{
 				{
 					MatchExpressions: []v1.NodeSelectorRequirement{
@@ -694,13 +826,33 @@ func TestRemoveTopology(t *testing.T) {
 			expOk: true,
 			expectedAffinity: &v1.VolumeNodeAffinity{
 				Required: &v1.NodeSelector{
-					NodeSelectorTerms: []v1.NodeSelectorTerm{},
+					NodeSelectorTerms: []v1.NodeSelectorTerm{
+						{
+							MatchExpressions: []v1.NodeSelectorRequirement{
+								{
+									Key:      v1.LabelTopologyZone,
+									Operator: v1.NodeSelectorOpIn,
+									Values:   []string{"us-east1-a"},
+								},
+							},
+						},
+						{
+							MatchExpressions: []v1.NodeSelectorRequirement{
+								{
+									Key:      v1.LabelTopologyZone,
+									Operator: v1.NodeSelectorOpIn,
+									Values:   []string{"us-east1-c"},
+								},
+							},
+						},
+					},
 				},
 			},
 		},
 		{
-			name:        "Remove the topology key from single term duplicate expression",
+			name:   "Replace the topology key from single term and not combine topology key",
-			topologyKey: GCEPDTopologyKey,
+			oldKey: GCEPDTopologyKey,
+			newKey: v1.LabelTopologyZone,
 			pv: makePVWithNodeSelectorTerms([]v1.NodeSelectorTerm{
 				{
 					MatchExpressions: []v1.NodeSelectorRequirement{
@@ -710,9 +862,9 @@ func TestRemoveTopology(t *testing.T) {
 							Values:   []string{"us-east1-a"},
 						},
 						{
-							Key:      GCEPDTopologyKey,
+							Key:      v1.LabelTopologyZone,
 							Operator: v1.NodeSelectorOpIn,
-							Values:   []string{"us-east1-a"},
+							Values:   []string{"us-east1-c"},
 						},
 					},
 				},
@@ -720,7 +872,22 @@ func TestRemoveTopology(t *testing.T) {
 			expOk: true,
 			expectedAffinity: &v1.VolumeNodeAffinity{
 				Required: &v1.NodeSelector{
-					NodeSelectorTerms: []v1.NodeSelectorTerm{},
+					NodeSelectorTerms: []v1.NodeSelectorTerm{
+						{
+							MatchExpressions: []v1.NodeSelectorRequirement{
+								{
+									Key:      v1.LabelTopologyZone,
+									Operator: v1.NodeSelectorOpIn,
+									Values:   []string{"us-east1-a"},
+								},
+								{
+									Key:      v1.LabelTopologyZone,
+									Operator: v1.NodeSelectorOpIn,
+									Values:   []string{"us-east1-c"},
+								},
+							},
+						},
+					},
 				},
 			},
 		},
@@ -728,9 +895,9 @@ func TestRemoveTopology(t *testing.T) {
 
 	for _, tc := range testCases {
 		t.Logf("Running test: %v", tc.name)
-		ok := removeTopology(tc.pv, tc.topologyKey)
+		err := replaceTopology(tc.pv, tc.oldKey, tc.newKey)
-		if tc.expOk != ok {
+		if err != nil && tc.expOk {
-			t.Errorf("Expected ok: %v, but got: %v", tc.expOk, ok)
+			t.Errorf("Expected no err: %v, but got err: %v", tc.expOk, err)
 		}
 		if !reflect.DeepEqual(tc.pv.Spec.NodeAffinity, tc.expectedAffinity) {
 			t.Errorf("Expected affinity: %v, but got: %v", tc.expectedAffinity, tc.pv.Spec.NodeAffinity)