Expand unit tests for topology translation in csi-translation-lib

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

@@ -17,7 +17,7 @@ limitations under the License.
 package plugins
 
 import (
-	"fmt"
+	"errors"
 	"strings"
 
 	v1 "k8s.io/api/core/v1"
@@ -65,26 +65,39 @@ type InTreePlugin interface {
 	RepairVolumeHandle(volumeHandle, nodeID string) (string, error)
 }
 
-// getTopology returns the current topology zones with the given key.
+// replaceTopology overwrites an existing topology key by a new one.
-func getTopology(pv *v1.PersistentVolume, key string) []string {
+func replaceTopology(pv *v1.PersistentVolume, oldKey, newKey string) error {
-	if pv.Spec.NodeAffinity == nil ||
-		pv.Spec.NodeAffinity.Required == nil ||
-		len(pv.Spec.NodeAffinity.Required.NodeSelectorTerms) < 1 {
-		return nil
-	}
 	for i := range pv.Spec.NodeAffinity.Required.NodeSelectorTerms {
-		for _, r := range pv.Spec.NodeAffinity.Required.NodeSelectorTerms[i].MatchExpressions {
+		for j, r := range pv.Spec.NodeAffinity.Required.NodeSelectorTerms[i].MatchExpressions {
-			if r.Key == key {
+			if r.Key == oldKey {
-				return r.Values
+				pv.Spec.NodeAffinity.Required.NodeSelectorTerms[i].MatchExpressions[j].Key = newKey
 			}
 		}
 	}
 	return nil
 }
 
-// recordTopology writes the topology to the given PV.
+// getTopologyZones returns all topology zones with the given key found in the PV.
-// If topology already exists with the given key, assume the content is already accurate.
+func getTopologyZones(pv *v1.PersistentVolume, key string) []string {
-func recordTopology(pv *v1.PersistentVolume, key string, zones []string) error {
+	if pv.Spec.NodeAffinity == nil ||
+		pv.Spec.NodeAffinity.Required == nil ||
+		len(pv.Spec.NodeAffinity.Required.NodeSelectorTerms) < 1 {
+		return nil
+	}
+
+	var values []string
+	for i := range pv.Spec.NodeAffinity.Required.NodeSelectorTerms {
+		for _, r := range pv.Spec.NodeAffinity.Required.NodeSelectorTerms[i].MatchExpressions {
+			if r.Key == key {
+				values = append(values, r.Values...)
+			}
+		}
+	}
+	return values
+}
+
+// addTopology appends the topology to the given PV.
+func addTopology(pv *v1.PersistentVolume, topologyKey string, zones []string) error {
 	// Make sure there are no duplicate or empty strings
 	filteredZones := sets.String{}
 	for i := range zones {
@@ -94,34 +107,26 @@ func recordTopology(pv *v1.PersistentVolume, key string, zones []string) error {
 		}
 	}
 
-	if filteredZones.Len() < 1 {
+	zones = filteredZones.UnsortedList()
-		return fmt.Errorf("there are no valid zones for topology")
+	if len(zones) < 1 {
+		return errors.New("there are no valid zones to add to pv")
 	}
 
 	// Make sure the necessary fields exist
-	if pv.Spec.NodeAffinity == nil {
+	pv.Spec.NodeAffinity = new(v1.VolumeNodeAffinity)
-		pv.Spec.NodeAffinity = new(v1.VolumeNodeAffinity)
+	pv.Spec.NodeAffinity.Required = new(v1.NodeSelector)
+	pv.Spec.NodeAffinity.Required.NodeSelectorTerms = make([]v1.NodeSelectorTerm, 1)
+
+	topology := v1.NodeSelectorRequirement{
+		Key:      topologyKey,
+		Operator: v1.NodeSelectorOpIn,
+		Values:   zones,
 	}
 
-	if pv.Spec.NodeAffinity.Required == nil {
+	pv.Spec.NodeAffinity.Required.NodeSelectorTerms[0].MatchExpressions = append(
-		pv.Spec.NodeAffinity.Required = new(v1.NodeSelector)
+		pv.Spec.NodeAffinity.Required.NodeSelectorTerms[0].MatchExpressions,
-	}
+		topology,
-
+	)
-	if len(pv.Spec.NodeAffinity.Required.NodeSelectorTerms) == 0 {
-		pv.Spec.NodeAffinity.Required.NodeSelectorTerms = make([]v1.NodeSelectorTerm, 1)
-	}
-
-	// Don't overwrite if topology is already set
-	if len(getTopology(pv, key)) > 0 {
-		return nil
-	}
-
-	pv.Spec.NodeAffinity.Required.NodeSelectorTerms[0].MatchExpressions = append(pv.Spec.NodeAffinity.Required.NodeSelectorTerms[0].MatchExpressions,
-		v1.NodeSelectorRequirement{
-			Key:      key,
-			Operator: v1.NodeSelectorOpIn,
-			Values:   filteredZones.List(),
-		})
 
 	return nil
 }
@@ -129,15 +134,20 @@ func recordTopology(pv *v1.PersistentVolume, key string, zones []string) error {
 // translateTopology converts existing zone labels or in-tree topology to CSI topology.
 // In-tree topology has precedence over zone labels.
 func translateTopology(pv *v1.PersistentVolume, topologyKey string) error {
-	zones := getTopology(pv, v1.LabelZoneFailureDomain)
+	// If topology is already set, assume the content is accurate
+	if len(getTopologyZones(pv, topologyKey)) > 0 {
+		return nil
+	}
+
+	zones := getTopologyZones(pv, v1.LabelZoneFailureDomain)
 	if len(zones) > 0 {
-		return recordTopology(pv, topologyKey, zones)
+		return replaceTopology(pv, v1.LabelZoneFailureDomain, topologyKey)
 	}
 
 	if label, ok := pv.Labels[v1.LabelZoneFailureDomain]; ok {
 		zones = strings.Split(label, cloudvolume.LabelMultiZoneDelimiter)
 		if len(zones) > 0 {
-			return recordTopology(pv, topologyKey, zones)
+			return addTopology(pv, topologyKey, zones)
 		}
 	}
 

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

@@ -25,6 +25,16 @@ import (
 	"k8s.io/csi-translation-lib/plugins"
 )
 
+var (
+	defaultZoneLabels = map[string]string{
+		v1.LabelZoneFailureDomain: "us-east-1a",
+		v1.LabelZoneRegion:        "us-east-1",
+	}
+	regionalPDLabels = map[string]string{
+		v1.LabelZoneFailureDomain: "europe-west1-b__europe-west1-c",
+	}
+)
+
 func TestTranslationStability(t *testing.T) {
 	testCases := []struct {
 		name string
@@ -79,92 +89,94 @@ func TestTranslationStability(t *testing.T) {
 	}
 }
 
-func TestZoneTranslation(t *testing.T) {
+func TestTopologyTranslation(t *testing.T) {
 	testCases := []struct {
-		name        string
+		name                 string
-		pv          *v1.PersistentVolume
+		pv                   *v1.PersistentVolume
-		topologyKey string
+		expectedNodeAffinity *v1.VolumeNodeAffinity
 	}{
 		{
-			name: "GCE PD PV Source",
+			name:                 "GCE PD with zone labels",
-			pv: &v1.PersistentVolume{
+			pv:                   makeGCEPDPV(defaultZoneLabels, nil /*topology*/),
-				ObjectMeta: metav1.ObjectMeta{
+			expectedNodeAffinity: makeNodeAffinity(false /*multiTerms*/, plugins.GCEPDTopologyKey, "us-east-1a"),
-					Labels: map[string]string{
-						v1.LabelZoneFailureDomain: "us-east-1",
-						v1.LabelZoneRegion:        "us-east-1a",
-					},
-				},
-				Spec: v1.PersistentVolumeSpec{
-					PersistentVolumeSource: v1.PersistentVolumeSource{
-						GCEPersistentDisk: &v1.GCEPersistentDiskVolumeSource{
-							PDName:    "test-disk",
-							FSType:    "ext4",
-							Partition: 0,
-							ReadOnly:  false,
-						},
-					},
-				},
-			},
-			topologyKey: plugins.GCEPDTopologyKey,
 		},
 		{
-			name: "AWS EBS PV Source",
+			name:                 "GCE PD with existing topology (beta keys)",
-			pv: &v1.PersistentVolume{
+			pv:                   makeGCEPDPV(nil /*labels*/, makeTopology(v1.LabelZoneFailureDomain, "us-east-2a")),
-				ObjectMeta: metav1.ObjectMeta{
+			expectedNodeAffinity: makeNodeAffinity(false /*multiTerms*/, plugins.GCEPDTopologyKey, "us-east-2a"),
-					Labels: map[string]string{
-						v1.LabelZoneFailureDomain: "us-east-1",
-						v1.LabelZoneRegion:        "us-east-1a",
-					},
-				},
-				Spec: v1.PersistentVolumeSpec{
-					PersistentVolumeSource: v1.PersistentVolumeSource{
-						AWSElasticBlockStore: &v1.AWSElasticBlockStoreVolumeSource{
-							VolumeID:  "vol01",
-							FSType:    "ext3",
-							Partition: 1,
-							ReadOnly:  true,
-						},
-					},
-				},
-			},
-			topologyKey: plugins.AWSEBSTopologyKey,
 		},
 		{
-			name: "Cinder PV Source",
+			name:                 "GCE PD with existing topology (CSI keys)",
-			pv: &v1.PersistentVolume{
+			pv:                   makeGCEPDPV(nil /*labels*/, makeTopology(plugins.GCEPDTopologyKey, "us-east-2a")),
-				ObjectMeta: metav1.ObjectMeta{
+			expectedNodeAffinity: makeNodeAffinity(false /*multiTerms*/, plugins.GCEPDTopologyKey, "us-east-2a"),
-					Labels: map[string]string{
+		},
-						v1.LabelZoneFailureDomain: "us-east-1",
+		{
-						v1.LabelZoneRegion:        "us-east-1a",
+			name:                 "GCE PD with zone labels and topology",
-					},
+			pv:                   makeGCEPDPV(defaultZoneLabels, makeTopology(v1.LabelZoneFailureDomain, "us-east-2a")),
-				},
+			expectedNodeAffinity: makeNodeAffinity(false /*multiTerms*/, plugins.GCEPDTopologyKey, "us-east-2a"),
-				Spec: v1.PersistentVolumeSpec{
+		},
-					PersistentVolumeSource: v1.PersistentVolumeSource{
+		{
-						Cinder: &v1.CinderPersistentVolumeSource{
+			name:                 "GCE PD with regional zones",
-							VolumeID: "vol1",
+			pv:                   makeGCEPDPV(regionalPDLabels, nil /*topology*/),
-							FSType:   "ext4",
+			expectedNodeAffinity: makeNodeAffinity(false /*multiTerms*/, plugins.GCEPDTopologyKey, "europe-west1-b", "europe-west1-c"),
-							ReadOnly: false,
+		},
-						},
+		{
-					},
+			name:                 "GCE PD with regional topology",
-				},
+			pv:                   makeGCEPDPV(nil /*labels*/, makeTopology(v1.LabelZoneFailureDomain, "europe-west1-b", "europe-west1-c")),
-			},
+			expectedNodeAffinity: makeNodeAffinity(false /*multiTerms*/, plugins.GCEPDTopologyKey, "europe-west1-b", "europe-west1-c"),
-			topologyKey: plugins.CinderTopologyKey,
+		},
+		{
+			name:                 "GCE PD with regional zone and topology",
+			pv:                   makeGCEPDPV(regionalPDLabels, makeTopology(v1.LabelZoneFailureDomain, "europe-west1-f", "europe-west1-g")),
+			expectedNodeAffinity: makeNodeAffinity(false /*multiTerms*/, plugins.GCEPDTopologyKey, "europe-west1-f", "europe-west1-g"),
+		},
+		{
+			name: "GCE PD with multiple node selector terms",
+			pv: makeGCEPDPVMultTerms(
+				nil, /*labels*/
+				makeTopology(v1.LabelZoneFailureDomain, "europe-west1-f"),
+				makeTopology(v1.LabelZoneFailureDomain, "europe-west1-g")),
+			expectedNodeAffinity: makeNodeAffinity(
+				true, /*multiTerms*/
+				plugins.GCEPDTopologyKey, "europe-west1-f", "europe-west1-g"),
+		},
+		// EBS test cases: test mostly topology key, i.e., don't repeat testing done with GCE
+		{
+			name:                 "AWS EBS with zone labels",
+			pv:                   makeAWSEBSPV(defaultZoneLabels, nil /*topology*/),
+			expectedNodeAffinity: makeNodeAffinity(false /*multiTerms*/, plugins.AWSEBSTopologyKey, "us-east-1a"),
+		},
+		{
+			name:                 "AWS EBS with zone labels and topology",
+			pv:                   makeAWSEBSPV(defaultZoneLabels, makeTopology(v1.LabelZoneFailureDomain, "us-east-2a")),
+			expectedNodeAffinity: makeNodeAffinity(false /*multiTerms*/, plugins.AWSEBSTopologyKey, "us-east-2a"),
+		},
+		// Cinder test cases: test mosty topology key, i.e., don't repeat testing done with GCE
+		{
+			name:                 "OpenStack Cinder with zone labels",
+			pv:                   makeCinderPV(defaultZoneLabels, nil /*topology*/),
+			expectedNodeAffinity: makeNodeAffinity(false /*multiTerms*/, plugins.CinderTopologyKey, "us-east-1a"),
+		},
+		{
+			name:                 "OpenStack Cinder with zone labels and topology",
+			pv:                   makeCinderPV(defaultZoneLabels, makeTopology(v1.LabelZoneFailureDomain, "us-east-2a")),
+			expectedNodeAffinity: makeNodeAffinity(false /*multiTerms*/, plugins.CinderTopologyKey, "us-east-2a"),
 		},
 	}
+
 	for _, test := range testCases {
 		ctl := New()
 		t.Logf("Testing %v", test.name)
 
-		zone := test.pv.ObjectMeta.Labels[v1.LabelZoneFailureDomain]
-
 		// Translate to CSI PV and check translated node affinity
 		newCSIPV, err := ctl.TranslateInTreePVToCSI(test.pv)
 		if err != nil {
 			t.Errorf("Error when translating to CSI: %v", err)
 		}
 
-		if !isNodeAffinitySet(newCSIPV, test.topologyKey, zone) {
+		nodeAffinity := newCSIPV.Spec.NodeAffinity
-			t.Errorf("Volume after translation lacks topology: %#v", newCSIPV)
+		if !reflect.DeepEqual(nodeAffinity, test.expectedNodeAffinity) {
+			t.Errorf("Expected node affinity %v, got %v", *test.expectedNodeAffinity, *nodeAffinity)
 		}
 
 		// Translate back to in-tree and make sure node affinity is still set
@@ -173,21 +185,131 @@ func TestZoneTranslation(t *testing.T) {
 			t.Errorf("Error when translating to in-tree: %v", err)
 		}
 
-		if !isNodeAffinitySet(newInTreePV, test.topologyKey, zone) {
+		nodeAffinity = newInTreePV.Spec.NodeAffinity
-			t.Errorf("Volume after translation lacks topology: %#v", newInTreePV)
+		if !reflect.DeepEqual(nodeAffinity, test.expectedNodeAffinity) {
+			t.Errorf("Expected node affinity %v, got %v", *test.expectedNodeAffinity, *nodeAffinity)
 		}
 	}
 }
 
-func isNodeAffinitySet(pv *v1.PersistentVolume, topologyKey, zone string) bool {
+func makePV(labels map[string]string, topology *v1.NodeSelectorRequirement) *v1.PersistentVolume {
-	for i := range pv.Spec.NodeAffinity.Required.NodeSelectorTerms {
+	pv := &v1.PersistentVolume{
-		for _, r := range pv.Spec.NodeAffinity.Required.NodeSelectorTerms[i].MatchExpressions {
+		ObjectMeta: metav1.ObjectMeta{
-			if r.Key == topologyKey && r.Values[0] == zone {
+			Labels: labels,
-				return true
+		},
-			}
+		Spec: v1.PersistentVolumeSpec{},
+	}
+
+	if topology != nil {
+		pv.Spec.NodeAffinity = &v1.VolumeNodeAffinity{
+			Required: &v1.NodeSelector{
+				NodeSelectorTerms: []v1.NodeSelectorTerm{
+					{MatchExpressions: []v1.NodeSelectorRequirement{*topology}},
+				},
+			},
 		}
 	}
-	return false
+
+	return pv
+}
+
+func makeGCEPDPV(labels map[string]string, topology *v1.NodeSelectorRequirement) *v1.PersistentVolume {
+	pv := makePV(labels, topology)
+	pv.Spec.PersistentVolumeSource = v1.PersistentVolumeSource{
+		GCEPersistentDisk: &v1.GCEPersistentDiskVolumeSource{
+			PDName:    "test-disk",
+			FSType:    "ext4",
+			Partition: 0,
+			ReadOnly:  false,
+		},
+	}
+	return pv
+}
+
+func makeGCEPDPVMultTerms(labels map[string]string, topologies ...*v1.NodeSelectorRequirement) *v1.PersistentVolume {
+	pv := makeGCEPDPV(labels, topologies[0])
+	for _, topology := range topologies[1:] {
+		pv.Spec.NodeAffinity.Required.NodeSelectorTerms = append(
+			pv.Spec.NodeAffinity.Required.NodeSelectorTerms,
+			v1.NodeSelectorTerm{
+				MatchExpressions: []v1.NodeSelectorRequirement{*topology},
+			},
+		)
+	}
+	return pv
+}
+
+func makeAWSEBSPV(labels map[string]string, topology *v1.NodeSelectorRequirement) *v1.PersistentVolume {
+	pv := makePV(labels, topology)
+	pv.Spec.PersistentVolumeSource = v1.PersistentVolumeSource{
+		AWSElasticBlockStore: &v1.AWSElasticBlockStoreVolumeSource{
+			VolumeID:  "vol01",
+			FSType:    "ext3",
+			Partition: 1,
+			ReadOnly:  true,
+		},
+	}
+	return pv
+}
+
+func makeCinderPV(labels map[string]string, topology *v1.NodeSelectorRequirement) *v1.PersistentVolume {
+	pv := makePV(labels, topology)
+	pv.Spec.PersistentVolumeSource = v1.PersistentVolumeSource{
+		Cinder: &v1.CinderPersistentVolumeSource{
+			VolumeID: "vol1",
+			FSType:   "ext4",
+			ReadOnly: false,
+		},
+	}
+	return pv
+}
+
+func makeNodeAffinity(multiTerms bool, key string, values ...string) *v1.VolumeNodeAffinity {
+	nodeAffinity := &v1.VolumeNodeAffinity{
+		Required: &v1.NodeSelector{
+			NodeSelectorTerms: []v1.NodeSelectorTerm{
+				{
+					MatchExpressions: []v1.NodeSelectorRequirement{
+						{
+							Key:      key,
+							Operator: v1.NodeSelectorOpIn,
+							Values:   values,
+						},
+					},
+				},
+			},
+		},
+	}
+
+	// If multiple terms is NOT requested, return a single term with all values
+	if !multiTerms {
+		return nodeAffinity
+	}
+
+	// Otherwise return multiple terms, each one with a single value
+	nodeAffinity.Required.NodeSelectorTerms[0].MatchExpressions[0].Values = values[:1] // If values=[1,2,3], overwrite with [1]
+	for _, value := range values[1:] {
+		term := v1.NodeSelectorTerm{
+			MatchExpressions: []v1.NodeSelectorRequirement{
+				{
+					Key:      key,
+					Operator: v1.NodeSelectorOpIn,
+					Values:   []string{value},
+				},
+			},
+		}
+		nodeAffinity.Required.NodeSelectorTerms = append(nodeAffinity.Required.NodeSelectorTerms, term)
+	}
+
+	return nodeAffinity
+}
+
+func makeTopology(key string, values ...string) *v1.NodeSelectorRequirement {
+	return &v1.NodeSelectorRequirement{
+		Key:      key,
+		Operator: v1.NodeSelectorOpIn,
+		Values:   values,
+	}
 }
 
 func TestPluginNameMappings(t *testing.T) {