diff --git a/staging/src/k8s.io/csi-translation-lib/plugins/gce_pd.go b/staging/src/k8s.io/csi-translation-lib/plugins/gce_pd.go index ca02aac507c..7ca1f67c58f 100644 --- a/staging/src/k8s.io/csi-translation-lib/plugins/gce_pd.go +++ b/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) { - _, zoneLabel, regionLabel := getTopologyLabel(pv) +// gceRegionTopologyHandler will process the PV and add region +// 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) - // if we found the val in topology directly - if len(regionVal) != 0 { - if len(regionVal) > 1 { - return "", fmt.Errorf("multiple regions found: %v", regionVal) + // 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 + } + + 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 - zonesVal := getTopologyValues(pv, zoneLabel) - if len(zonesVal) == 0 { - // No kubernetes NodeAffinity found, generate it from CSI Topology - zonesVal = getTopologyValues(pv, GCEPDTopologyKey) - } - if len(zonesVal) != 0 { - return getRegionFromZones(zonesVal) + // Add region label + regionVals := getTopologyValues(pv, regionLabel) + if len(regionVals) == 1 { + // We should only have exactly 1 region value + if pv.Labels == nil { + pv.Labels = make(map[string]string) + } + _, regionOK := pv.Labels[regionLabel] + if !regionOK { + pv.Labels[regionLabel] = regionVals[0] + } } - // Not found anything in NodeAffinity, check region labels - 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) + return nil } // TODO: Replace this with the imported one from GCE PD CSI Driver when diff --git a/staging/src/k8s.io/csi-translation-lib/plugins/in_tree_volume.go b/staging/src/k8s.io/csi-translation-lib/plugins/in_tree_volume.go index c2a9e8956de..32c0d22190c 100644 --- a/staging/src/k8s.io/csi-translation-lib/plugins/in_tree_volume.go +++ b/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. -func translateTopologyFromInTreeToCSI(pv *v1.PersistentVolume, topologyKey string) error { - // If topology is already set, assume the content is accurate - if len(getTopologyValues(pv, topologyKey)) > 0 { - return nil - } +// In-tree topology has precedence over zone labels. When both in-tree topology and zone labels exist +// for a particular CSI topology, in-tree topology will be used. +// This function will remove the Beta version Kubernetes topology label in case the node upgrade to a +// newer version where it does not have any Beta topology label anymore +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 - label, ok := pv.Labels[zoneLabel] - if ok { - zones = strings.Split(label, labelMultiZoneDelimiter) - if len(zones) > 0 { - return addTopology(pv, topologyKey, zones) + // if the in-tree PV has beta region label, replace it with GA label to ensure + // the scheduler is able to schedule it on new nodes with only GA kubernetes label + // No need to check it for zone label because it has already been replaced if exist + if regionLabel == v1.LabelFailureDomainBetaRegion { + regions := getTopologyValues(pv, regionLabel) + if len(regions) > 0 { + replaceTopology(pv, regionLabel, v1.LabelTopologyRegion) } } return nil } -// getTopologyLabel checks if the kubernetes topology used in this PV is GA -// and return the zone/region label used. -// It first check the NodeAffinity to find topology. If nothing is found, -// It checks the PV labels. If it is empty in both places, it will return -// GA label by default -func getTopologyLabel(pv *v1.PersistentVolume) (bool, string, string) { +// getTopologyLabel checks if the kubernetes topology label used in this +// PV is GA and return the zone/region label used. +// The version checking follows the following orders +// 1. Check NodeAffinity +// 1.1 Check if zoneGA exists, if yes return GA labels +// 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 - zoneGA := TopologyKeyExist(v1.LabelTopologyZone, pv.Spec.NodeAffinity) - regionGA := TopologyKeyExist(v1.LabelTopologyRegion, pv.Spec.NodeAffinity) - if zoneGA || regionGA { - // GA NodeAffinity exists - return true, v1.LabelTopologyZone, v1.LabelTopologyRegion + if zoneGA := TopologyKeyExist(v1.LabelTopologyZone, pv.Spec.NodeAffinity); zoneGA { + return v1.LabelTopologyZone, v1.LabelTopologyRegion } - - // If no GA topology in NodeAffinity, check the beta one - 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 zoneBeta := TopologyKeyExist(v1.LabelFailureDomainBetaZone, pv.Spec.NodeAffinity); zoneBeta { + return v1.LabelFailureDomainBetaZone, v1.LabelFailureDomainBetaRegion } - - // If nothing is in the NodeAfinity, we should check pv labels - _, 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 _, zoneGA := pv.Labels[v1.LabelTopologyZone]; zoneGA { + return v1.LabelTopologyZone, v1.LabelTopologyRegion } - - // If GA label not exists, check beta version - _, 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 + if _, zoneBeta := pv.Labels[v1.LabelFailureDomainBetaZone]; zoneBeta { + return 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 -// Kubernetes topology and add labels to it. Note that this function -// will only work for plugin with single topologyKey. If a plugin has -// more than one topologyKey, it will need to be processed separately -// by the plugin. -// regionParser is a function to generate region val based on PV -// if the function is not set, we will not set region topology. -// 1. Remove any existing CSI topologyKey from NodeAffinity -// 2. Add Kubernetes Topology in the NodeAffinity if it does not exist -// 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 -// 3. Add Kubernetes Topology labels(zone and region) -func translateTopologyFromCSIToInTree(pv *v1.PersistentVolume, topologyKey string, regionParser regionParser) error { +// translateTopologyFromCSIToInTree translate a CSI topology to +// Kubernetes topology and add topology labels to it. Note that this function +// will only work for plugin with a single topologyKey that translates to +// Kubernetes zone(and region if regionTopologyHandler is passed in). +// If a plugin has more than one topologyKey, it will need to be processed +// separately by the plugin. +// regionTopologyHandler is a function to add region topology NodeAffinity +// and labels for the given PV. It assumes the Zone NodeAffinity already exists +// If regionTopologyHandler is nil, no region NodeAffinity will be added +// +// 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 { - csiTopologyZoneValues := getTopologyValues(pv, topologyKey) + zoneLabel, _ := getTopologyLabel(pv) - // 1. Frist remove the CSI topology Key - removeTopology(pv, topologyKey) + // 1. Replace all CSI topology to Kubernetes Zone label + 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) - zoneLabelVal, zoneOK := pv.Labels[zoneLabel] - regionLabelVal, regionOK := pv.Labels[regionLabel] - - // 2. Add Kubernetes Topology in the NodeAffinity if it does not exist - // Check if Kubernetes Zone Topology already exist - 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) - } - } + // 2. Take care of region topology if a regionTopologyHandler is passed + if regionTopologyHandler != nil { + // let's make less strict on this one. Even if there is an error in the region processing, just ignore it + err = regionTopologyHandler(pv) + if err != nil { + return fmt.Errorf("Failed to handle region topology. error: %v", err) } } - // Check if Kubernetes Region Topology already exist - 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 { - 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 { + // 3. Add labels about Kubernetes Topology + zoneVals := getTopologyValues(pv, zoneLabel) + if len(zoneVals) > 0 { if pv.Labels == nil { pv.Labels = make(map[string]string) } + _, zoneOK := pv.Labels[zoneLabel] if !zoneOK { - csiTopologyZoneValStr := strings.Join(csiTopologyZoneValues, labelMultiZoneDelimiter) - pv.Labels[zoneLabel] = csiTopologyZoneValStr - } - 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 + zoneValStr := strings.Join(zoneVals, labelMultiZoneDelimiter) + pv.Labels[zoneLabel] = zoneValStr } } diff --git a/staging/src/k8s.io/csi-translation-lib/plugins/in_tree_volume_test.go b/staging/src/k8s.io/csi-translation-lib/plugins/in_tree_volume_test.go index 5c2efb1c4c9..21448622300 100644 --- a/staging/src/k8s.io/csi-translation-lib/plugins/in_tree_volume_test.go +++ b/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", - key: GCEPDTopologyKey, - expErr: false, - regionParser: gceRegionParser, + name: "Remove CSI Topology Key and do not change existing GA Kubernetes topology", + key: GCEPDTopologyKey, + expErr: false, + 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", - key: GCEPDTopologyKey, - expErr: false, - regionParser: gceRegionParser, + name: "Remove CSI Topology Key and do not change existing Beta Kubernetes topology", + key: GCEPDTopologyKey, + expErr: false, + 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", - key: GCEPDTopologyKey, - expErr: false, - regionParser: gceRegionParser, + name: "Remove CSI Topology Key and add Kubernetes topology from NodeAffinity, ignore labels", + key: GCEPDTopologyKey, + expErr: false, + 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", - key: GCEPDTopologyKey, - expErr: false, - regionParser: gceRegionParser, + name: "No CSI topology label exists and no change to the NodeAffinity", + key: GCEPDTopologyKey, + expErr: false, + regionTopologyHandler: gceRegionTopologyHandler, pv: &v1.PersistentVolume{ ObjectMeta: metav1.ObjectMeta{ Name: "gcepd", Namespace: "myns", @@ -211,33 +235,25 @@ func TestTranslateTopologyFromCSIToInTree(t *testing.T) { v1.LabelTopologyRegion: "existingRegion", }, }, - }, - expectedNodeSelectorTerms: []v1.NodeSelectorTerm{ - { - MatchExpressions: []v1.NodeSelectorRequirement{ - { - Key: v1.LabelTopologyZone, - Operator: v1.NodeSelectorOpIn, - Values: []string{"existingZone"}, - }, - { - Key: v1.LabelTopologyRegion, - Operator: v1.NodeSelectorOpIn, - Values: []string{"existingRegion"}, + Spec: v1.PersistentVolumeSpec{ + NodeAffinity: &v1.VolumeNodeAffinity{ + Required: &v1.NodeSelector{ + NodeSelectorTerms: []v1.NodeSelectorTerm{}, }, }, }, }, + expectedNodeSelectorTerms: []v1.NodeSelectorTerm{}, expectedLabels: map[string]string{ v1.LabelTopologyZone: "existingZone", v1.LabelTopologyRegion: "existingRegion", }, }, { - name: "Generate GA labels and kubernetes topology only from CSI topology", - key: GCEPDTopologyKey, - expErr: false, - regionParser: gceRegionParser, + name: "Generate GA labels and kubernetes topology only from CSI topology", + key: GCEPDTopologyKey, + expErr: false, + 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", - key: GCEPDTopologyKey, - expErr: false, - regionParser: gceRegionParser, + name: "Generate Beta labels and kubernetes topology from Beta NodeAffinity", + key: GCEPDTopologyKey, + expErr: false, + 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", - key: GCEPDTopologyKey, - expErr: false, - regionParser: nil, + name: "regionParser is missing and only zone labels get generated", + key: GCEPDTopologyKey, + expErr: false, + 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", - key: GCEPDTopologyKey, - expErr: false, - regionParser: gceRegionParser, + name: "Replace multi-term CSI Topology Key and add Region Kubernetes topology for both", + key: GCEPDTopologyKey, + expErr: false, + 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", - topologyKey: GCEPDTopologyKey, + name: "Replace single csi topology from PV", + 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", - topologyKey: GCEPDTopologyKey, + name: "Not found the topology key so do nothing", + 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", - topologyKey: GCEPDTopologyKey, + name: "Replace the topology key from multiple terms", + 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", - topologyKey: GCEPDTopologyKey, + name: "Replace the topology key from single term and not combine topology key", + 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) - if tc.expOk != ok { - t.Errorf("Expected ok: %v, but got: %v", tc.expOk, ok) + err := replaceTopology(tc.pv, tc.oldKey, tc.newKey) + if err != nil && tc.expOk { + 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)