From 67fed317a1b6ffc58d21afc32730a5084575a0c9 Mon Sep 17 00:00:00 2001 From: Jiawei Wang Date: Thu, 7 Jan 2021 16:39:30 -0800 Subject: [PATCH 1/2] Prepare for Topology migration to GA from CSI migration This also includes a change on CSI migration TranslateCSIToInTree where we remove the CSI topology and add Kubernetes Topology to the NodeAffinity --- .../persistentvolume/label/admission.go | 22 +- .../persistentvolume/label/admission_test.go | 90 ++- .../cloud-provider/volume/helpers/zones.go | 13 +- .../volume/helpers/zones_test.go | 118 ++-- .../csi-translation-lib/plugins/aws_ebs.go | 2 +- .../csi-translation-lib/plugins/gce_pd.go | 45 +- .../plugins/in_tree_volume.go | 263 ++++++++- .../plugins/in_tree_volume_test.go | 538 ++++++++++++++++++ .../plugins/openstack_cinder.go | 2 +- .../csi-translation-lib/translate_test.go | 76 ++- 10 files changed, 1067 insertions(+), 102 deletions(-) diff --git a/plugin/pkg/admission/storage/persistentvolume/label/admission.go b/plugin/pkg/admission/storage/persistentvolume/label/admission.go index 36235e21792..7a7fe94dca7 100644 --- a/plugin/pkg/admission/storage/persistentvolume/label/admission.go +++ b/plugin/pkg/admission/storage/persistentvolume/label/admission.go @@ -130,7 +130,7 @@ func (l *persistentVolumeLabel) Admit(ctx context.Context, a admission.Attribute // Set NodeSelectorRequirements based on the labels var values []string - if k == v1.LabelFailureDomainBetaZone { + if k == v1.LabelTopologyZone || k == v1.LabelFailureDomainBetaZone { zones, err := volumehelpers.LabelZonesToSet(v) if err != nil { return admission.NewForbidden(a, fmt.Errorf("failed to convert label string for Zone: %s to a Set", v)) @@ -172,15 +172,31 @@ func (l *persistentVolumeLabel) findVolumeLabels(volume *api.PersistentVolume) ( existingLabels := volume.Labels // All cloud providers set only these two labels. - domain, domainOK := existingLabels[v1.LabelFailureDomainBetaZone] - region, regionOK := existingLabels[v1.LabelFailureDomainBetaRegion] + topologyLabelGA := true + domain, domainOK := existingLabels[v1.LabelTopologyZone] + region, regionOK := existingLabels[v1.LabelTopologyRegion] + // If they dont have GA labels we should check for failuredomain beta labels + // TODO: remove this once all the cloud provider change to GA topology labels + if !domainOK || !regionOK { + topologyLabelGA = false + domain, domainOK = existingLabels[v1.LabelFailureDomainBetaZone] + region, regionOK = existingLabels[v1.LabelFailureDomainBetaRegion] + } + isDynamicallyProvisioned := metav1.HasAnnotation(volume.ObjectMeta, persistentvolume.AnnDynamicallyProvisioned) if isDynamicallyProvisioned && domainOK && regionOK { // PV already has all the labels and we can trust the dynamic provisioning that it provided correct values. + if topologyLabelGA { + return map[string]string{ + v1.LabelTopologyZone: domain, + v1.LabelTopologyRegion: region, + }, nil + } return map[string]string{ v1.LabelFailureDomainBetaZone: domain, v1.LabelFailureDomainBetaRegion: region, }, nil + } // Either missing labels or we don't trust the user provided correct values. diff --git a/plugin/pkg/admission/storage/persistentvolume/label/admission_test.go b/plugin/pkg/admission/storage/persistentvolume/label/admission_test.go index f46a7b7b312..398a53c5f4c 100644 --- a/plugin/pkg/admission/storage/persistentvolume/label/admission_test.go +++ b/plugin/pkg/admission/storage/persistentvolume/label/admission_test.go @@ -66,9 +66,9 @@ func Test_PVLAdmission(t *testing.T) { name: "non-cloud PV ignored", handler: newPersistentVolumeLabel(), pvlabeler: mockVolumeLabels(map[string]string{ - "a": "1", - "b": "2", - v1.LabelFailureDomainBetaZone: "1__2__3", + "a": "1", + "b": "2", + v1.LabelTopologyZone: "1__2__3", }), preAdmissionPV: &api.PersistentVolume{ ObjectMeta: metav1.ObjectMeta{Name: "noncloud", Namespace: "myns"}, @@ -234,7 +234,7 @@ func Test_PVLAdmission(t *testing.T) { err: nil, }, { - name: "existing labels from dynamic provisioning are not changed", + name: "existing Beta labels from dynamic provisioning are not changed", handler: newPersistentVolumeLabel(), pvlabeler: mockVolumeLabels(map[string]string{ v1.LabelFailureDomainBetaZone: "domain1", @@ -301,6 +301,74 @@ func Test_PVLAdmission(t *testing.T) { }, err: nil, }, + { + name: "existing GA labels from dynamic provisioning are not changed", + handler: newPersistentVolumeLabel(), + pvlabeler: mockVolumeLabels(map[string]string{ + v1.LabelTopologyZone: "domain1", + v1.LabelTopologyRegion: "region1", + }), + preAdmissionPV: &api.PersistentVolume{ + ObjectMeta: metav1.ObjectMeta{ + Name: "awsebs", Namespace: "myns", + Labels: map[string]string{ + v1.LabelTopologyZone: "existingDomain", + v1.LabelTopologyRegion: "existingRegion", + }, + Annotations: map[string]string{ + persistentvolume.AnnDynamicallyProvisioned: "kubernetes.io/aws-ebs", + }, + }, + Spec: api.PersistentVolumeSpec{ + PersistentVolumeSource: api.PersistentVolumeSource{ + AWSElasticBlockStore: &api.AWSElasticBlockStoreVolumeSource{ + VolumeID: "123", + }, + }, + }, + }, + postAdmissionPV: &api.PersistentVolume{ + ObjectMeta: metav1.ObjectMeta{ + Name: "awsebs", + Namespace: "myns", + Labels: map[string]string{ + v1.LabelTopologyZone: "existingDomain", + v1.LabelTopologyRegion: "existingRegion", + }, + Annotations: map[string]string{ + persistentvolume.AnnDynamicallyProvisioned: "kubernetes.io/aws-ebs", + }, + }, + Spec: api.PersistentVolumeSpec{ + PersistentVolumeSource: api.PersistentVolumeSource{ + AWSElasticBlockStore: &api.AWSElasticBlockStoreVolumeSource{ + VolumeID: "123", + }, + }, + NodeAffinity: &api.VolumeNodeAffinity{ + Required: &api.NodeSelector{ + NodeSelectorTerms: []api.NodeSelectorTerm{ + { + MatchExpressions: []api.NodeSelectorRequirement{ + { + Key: v1.LabelTopologyRegion, + Operator: api.NodeSelectorOpIn, + Values: []string{"existingRegion"}, + }, + { + Key: v1.LabelTopologyZone, + Operator: api.NodeSelectorOpIn, + Values: []string{"existingDomain"}, + }, + }, + }, + }, + }, + }, + }, + }, + err: nil, + }, { name: "existing labels from user are changed", handler: newPersistentVolumeLabel(), @@ -367,9 +435,9 @@ func Test_PVLAdmission(t *testing.T) { name: "GCE PD PV labeled correctly", handler: newPersistentVolumeLabel(), pvlabeler: mockVolumeLabels(map[string]string{ - "a": "1", - "b": "2", - v1.LabelFailureDomainBetaZone: "1__2__3", + "a": "1", + "b": "2", + v1.LabelTopologyZone: "1__2__3", }), preAdmissionPV: &api.PersistentVolume{ ObjectMeta: metav1.ObjectMeta{Name: "gcepd", Namespace: "myns"}, @@ -386,9 +454,9 @@ func Test_PVLAdmission(t *testing.T) { Name: "gcepd", Namespace: "myns", Labels: map[string]string{ - "a": "1", - "b": "2", - v1.LabelFailureDomainBetaZone: "1__2__3", + "a": "1", + "b": "2", + v1.LabelTopologyZone: "1__2__3", }, }, Spec: api.PersistentVolumeSpec{ @@ -413,7 +481,7 @@ func Test_PVLAdmission(t *testing.T) { Values: []string{"2"}, }, { - Key: v1.LabelFailureDomainBetaZone, + Key: v1.LabelTopologyZone, Operator: api.NodeSelectorOpIn, Values: []string{"1", "2", "3"}, }, diff --git a/staging/src/k8s.io/cloud-provider/volume/helpers/zones.go b/staging/src/k8s.io/cloud-provider/volume/helpers/zones.go index a3a7375d1b5..ff3a392846f 100644 --- a/staging/src/k8s.io/cloud-provider/volume/helpers/zones.go +++ b/staging/src/k8s.io/cloud-provider/volume/helpers/zones.go @@ -23,7 +23,7 @@ import ( "strconv" "strings" - "k8s.io/api/core/v1" + v1 "k8s.io/api/core/v1" "k8s.io/apimachinery/pkg/util/sets" cloudvolume "k8s.io/cloud-provider/volume" "k8s.io/klog/v2" @@ -118,9 +118,12 @@ func SelectZonesForVolume(zoneParameterPresent, zonesParameterPresent bool, zone // pick node's zone for one of the replicas var ok bool - zoneFromNode, ok = node.ObjectMeta.Labels[v1.LabelFailureDomainBetaZone] + zoneFromNode, ok = node.ObjectMeta.Labels[v1.LabelTopologyZone] if !ok { - return nil, fmt.Errorf("%s Label for node missing", v1.LabelFailureDomainBetaZone) + zoneFromNode, ok = node.ObjectMeta.Labels[v1.LabelFailureDomainBetaZone] + if !ok { + return nil, fmt.Errorf("Either %s or %s Label for node missing", v1.LabelTopologyZone, v1.LabelFailureDomainBetaZone) + } } // if single replica volume and node with zone found, return immediately if numReplicas == 1 { @@ -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) } if allowedZones.Len() > 0 { @@ -185,7 +188,7 @@ func ZonesFromAllowedTopologies(allowedTopologies []v1.TopologySelectorTerm) (se zones := make(sets.String) for _, term := range allowedTopologies { for _, exp := range term.MatchLabelExpressions { - if exp.Key == v1.LabelFailureDomainBetaZone { + if exp.Key == v1.LabelTopologyZone || exp.Key == v1.LabelFailureDomainBetaZone { for _, value := range exp.Values { zones.Insert(value) } diff --git a/staging/src/k8s.io/cloud-provider/volume/helpers/zones_test.go b/staging/src/k8s.io/cloud-provider/volume/helpers/zones_test.go index e07a5af458f..e31a7224b2b 100644 --- a/staging/src/k8s.io/cloud-provider/volume/helpers/zones_test.go +++ b/staging/src/k8s.io/cloud-provider/volume/helpers/zones_test.go @@ -20,7 +20,7 @@ import ( "hash/fnv" "testing" - "k8s.io/api/core/v1" + v1 "k8s.io/api/core/v1" "k8s.io/apimachinery/pkg/util/sets" ) @@ -416,8 +416,11 @@ func TestChooseZonesForVolume(t *testing.T) { func TestSelectZoneForVolume(t *testing.T) { - nodeWithZoneLabels := &v1.Node{} - nodeWithZoneLabels.Labels = map[string]string{v1.LabelFailureDomainBetaZone: "zoneX"} + nodeWithGATopologyLabels := &v1.Node{} + nodeWithGATopologyLabels.Labels = map[string]string{v1.LabelTopologyZone: "zoneX"} + + nodeWithBetaTopologyLabels := &v1.Node{} + nodeWithBetaTopologyLabels.Labels = map[string]string{v1.LabelFailureDomainBetaZone: "zoneY"} nodeWithNoLabels := &v1.Node{} @@ -468,7 +471,7 @@ func TestSelectZoneForVolume(t *testing.T) { // [3] AllowedTopologies irrelevant { Name: "Node_with_Zone_labels_and_Zone_parameter_present", - Node: nodeWithZoneLabels, + Node: nodeWithGATopologyLabels, ZonePresent: true, Zone: "zoneX", Reject: true, @@ -480,7 +483,7 @@ func TestSelectZoneForVolume(t *testing.T) { // [3] AllowedTopologies irrelevant { Name: "Node_with_Zone_labels_and_Zones_parameter_present", - Node: nodeWithZoneLabels, + Node: nodeWithGATopologyLabels, ZonesPresent: true, Zones: "zoneX,zoneY", Reject: true, @@ -499,7 +502,7 @@ func TestSelectZoneForVolume(t *testing.T) { { MatchLabelExpressions: []v1.TopologySelectorLabelRequirement{ { - Key: v1.LabelFailureDomainBetaZone, + Key: v1.LabelTopologyZone, Values: []string{"zoneX"}, }, }, @@ -521,7 +524,7 @@ func TestSelectZoneForVolume(t *testing.T) { { MatchLabelExpressions: []v1.TopologySelectorLabelRequirement{ { - Key: v1.LabelFailureDomainBetaZone, + Key: v1.LabelTopologyZone, Values: []string{"zoneX"}, }, }, @@ -530,7 +533,7 @@ func TestSelectZoneForVolume(t *testing.T) { Reject: true, }, - // Key specified in AllowedTopologies is not LabelFailureDomainBetaZone [Fail] + // Key specified in AllowedTopologies is not LabelTopologyZone [Fail] // [1] nil Node // [2] no Zone/Zones parameter // [3] AllowedTopologies with invalid key specified @@ -545,7 +548,7 @@ func TestSelectZoneForVolume(t *testing.T) { Values: []string{"zoneX"}, }, { - Key: v1.LabelFailureDomainBetaZone, + Key: v1.LabelTopologyZone, Values: []string{"zoneY"}, }, }, @@ -554,7 +557,7 @@ func TestSelectZoneForVolume(t *testing.T) { Reject: true, }, - // AllowedTopologies without keys specifying LabelFailureDomainBetaZone [Fail] + // AllowedTopologies without keys specifying LabelTopologyZone [Fail] // [1] nil Node // [2] no Zone/Zones parameter // [3] Invalid AllowedTopologies @@ -610,30 +613,42 @@ func TestSelectZoneForVolume(t *testing.T) { ExpectedZones: "zoneX,zoneY", }, - // Select zone from node label [Pass] + // Select zone from node ga label [Pass] // [1] Node with zone labels // [2] no zone/zones parameters // [3] no AllowedTopology { - Name: "Node_with_Zone_labels_and_VolumeScheduling_enabled", - Node: nodeWithZoneLabels, + Name: "Node_with_GA_Zone_labels_and_VolumeScheduling_enabled", + Node: nodeWithGATopologyLabels, Reject: false, ExpectSpecificZone: true, ExpectedZone: "zoneX", }, + // Select zone from node beta label [Pass] + // [1] Node with zone labels + // [2] no zone/zones parameters + // [3] no AllowedTopology + { + Name: "Node_with_Beta_Zone_labels_and_VolumeScheduling_enabled", + Node: nodeWithBetaTopologyLabels, + Reject: false, + ExpectSpecificZone: true, + ExpectedZone: "zoneY", + }, + // Select zone from node label [Pass] // [1] Node with zone labels // [2] no Zone/Zones parameters // [3] AllowedTopology with single term with multiple values specified (ignored) { Name: "Node_with_Zone_labels_and_Multiple_Allowed_Topology_values_and_VolumeScheduling_enabled", - Node: nodeWithZoneLabels, + Node: nodeWithGATopologyLabels, AllowedTopologies: []v1.TopologySelectorTerm{ { MatchLabelExpressions: []v1.TopologySelectorLabelRequirement{ { - Key: v1.LabelFailureDomainBetaZone, + Key: v1.LabelTopologyZone, Values: []string{"zoneZ", "zoneY"}, }, }, @@ -651,6 +666,27 @@ func TestSelectZoneForVolume(t *testing.T) { { Name: "Nil_Node_with_Multiple_Allowed_Topology_values_and_VolumeScheduling_enabled", Node: nil, + AllowedTopologies: []v1.TopologySelectorTerm{ + { + MatchLabelExpressions: []v1.TopologySelectorLabelRequirement{ + { + Key: v1.LabelTopologyZone, + Values: []string{"zoneX", "zoneY"}, + }, + }, + }, + }, + Reject: false, + ExpectedZones: "zoneX,zoneY", + }, + + // Select Zone from AllowedTopologies using Beta labels [Pass] + // [1] nil Node + // [2] no Zone/Zones parametes specified + // [3] AllowedTopologies with single term with multiple values specified + { + Name: "Nil_Node_with_Multiple_Allowed_Topology_values_Beta_label_and_VolumeScheduling_enabled", + Node: nil, AllowedTopologies: []v1.TopologySelectorTerm{ { MatchLabelExpressions: []v1.TopologySelectorLabelRequirement{ @@ -676,7 +712,7 @@ func TestSelectZoneForVolume(t *testing.T) { { MatchLabelExpressions: []v1.TopologySelectorLabelRequirement{ { - Key: v1.LabelFailureDomainBetaZone, + Key: v1.LabelTopologyZone, Values: []string{"zoneX"}, }, }, @@ -684,7 +720,7 @@ func TestSelectZoneForVolume(t *testing.T) { { MatchLabelExpressions: []v1.TopologySelectorLabelRequirement{ { - Key: v1.LabelFailureDomainBetaZone, + Key: v1.LabelTopologyZone, Values: []string{"zoneY"}, }, }, @@ -706,7 +742,7 @@ func TestSelectZoneForVolume(t *testing.T) { { MatchLabelExpressions: []v1.TopologySelectorLabelRequirement{ { - Key: v1.LabelFailureDomainBetaZone, + Key: v1.LabelTopologyZone, Values: []string{"zoneX"}, }, }, @@ -771,7 +807,7 @@ func TestSelectZoneForVolume(t *testing.T) { func TestSelectZonesForVolume(t *testing.T) { nodeWithZoneLabels := &v1.Node{} - nodeWithZoneLabels.Labels = map[string]string{v1.LabelFailureDomainBetaZone: "zoneX"} + nodeWithZoneLabels.Labels = map[string]string{v1.LabelTopologyZone: "zoneX"} nodeWithNoLabels := &v1.Node{} @@ -865,7 +901,7 @@ func TestSelectZonesForVolume(t *testing.T) { { MatchLabelExpressions: []v1.TopologySelectorLabelRequirement{ { - Key: v1.LabelFailureDomainBetaZone, + Key: v1.LabelTopologyZone, Values: []string{"zoneX"}, }, }, @@ -889,7 +925,7 @@ func TestSelectZonesForVolume(t *testing.T) { { MatchLabelExpressions: []v1.TopologySelectorLabelRequirement{ { - Key: v1.LabelFailureDomainBetaZone, + Key: v1.LabelTopologyZone, Values: []string{"zoneX"}, }, }, @@ -898,7 +934,7 @@ func TestSelectZonesForVolume(t *testing.T) { Reject: true, }, - // Key specified in AllowedTopologies is not LabelFailureDomainBetaZone [Fail] + // Key specified in AllowedTopologies is not LabelTopologyZone [Fail] // [1] nil Node // [2] no Zone/Zones parameter // [3] AllowedTopologies with invalid key specified @@ -915,7 +951,7 @@ func TestSelectZonesForVolume(t *testing.T) { Values: []string{"zoneX"}, }, { - Key: v1.LabelFailureDomainBetaZone, + Key: v1.LabelTopologyZone, Values: []string{"zoneY"}, }, }, @@ -924,7 +960,7 @@ func TestSelectZonesForVolume(t *testing.T) { Reject: true, }, - // AllowedTopologies without keys specifying LabelFailureDomainBetaZone [Fail] + // AllowedTopologies without keys specifying LabelTopologyZone [Fail] // [1] nil Node // [2] no Zone/Zones parameter // [3] Invalid AllowedTopologies @@ -999,7 +1035,7 @@ func TestSelectZonesForVolume(t *testing.T) { { MatchLabelExpressions: []v1.TopologySelectorLabelRequirement{ { - Key: v1.LabelFailureDomainBetaZone, + Key: v1.LabelTopologyZone, Values: []string{"zoneX"}, }, }, @@ -1123,7 +1159,7 @@ func TestSelectZonesForVolume(t *testing.T) { { MatchLabelExpressions: []v1.TopologySelectorLabelRequirement{ { - Key: v1.LabelFailureDomainBetaZone, + Key: v1.LabelTopologyZone, Values: []string{"zoneV", "zoneW", "zoneX", "zoneY", "zoneZ"}, }, }, @@ -1149,7 +1185,7 @@ func TestSelectZonesForVolume(t *testing.T) { { MatchLabelExpressions: []v1.TopologySelectorLabelRequirement{ { - Key: v1.LabelFailureDomainBetaZone, + Key: v1.LabelTopologyZone, Values: []string{"zoneX", "zoneY"}, }, }, @@ -1176,7 +1212,7 @@ func TestSelectZonesForVolume(t *testing.T) { { MatchLabelExpressions: []v1.TopologySelectorLabelRequirement{ { - Key: v1.LabelFailureDomainBetaZone, + Key: v1.LabelTopologyZone, Values: []string{"zoneX", "zoneY", "zoneZ"}, }, }, @@ -1200,7 +1236,7 @@ func TestSelectZonesForVolume(t *testing.T) { { MatchLabelExpressions: []v1.TopologySelectorLabelRequirement{ { - Key: v1.LabelFailureDomainBetaZone, + Key: v1.LabelTopologyZone, Values: []string{"zoneX", "zoneY"}, }, }, @@ -1228,7 +1264,7 @@ func TestSelectZonesForVolume(t *testing.T) { { MatchLabelExpressions: []v1.TopologySelectorLabelRequirement{ { - Key: v1.LabelFailureDomainBetaZone, + Key: v1.LabelTopologyZone, Values: []string{"zoneV"}, }, }, @@ -1236,7 +1272,7 @@ func TestSelectZonesForVolume(t *testing.T) { { MatchLabelExpressions: []v1.TopologySelectorLabelRequirement{ { - Key: v1.LabelFailureDomainBetaZone, + Key: v1.LabelTopologyZone, Values: []string{"zoneW"}, }, }, @@ -1244,7 +1280,7 @@ func TestSelectZonesForVolume(t *testing.T) { { MatchLabelExpressions: []v1.TopologySelectorLabelRequirement{ { - Key: v1.LabelFailureDomainBetaZone, + Key: v1.LabelTopologyZone, Values: []string{"zoneX"}, }, }, @@ -1252,7 +1288,7 @@ func TestSelectZonesForVolume(t *testing.T) { { MatchLabelExpressions: []v1.TopologySelectorLabelRequirement{ { - Key: v1.LabelFailureDomainBetaZone, + Key: v1.LabelTopologyZone, Values: []string{"zoneY"}, }, }, @@ -1260,7 +1296,7 @@ func TestSelectZonesForVolume(t *testing.T) { { MatchLabelExpressions: []v1.TopologySelectorLabelRequirement{ { - Key: v1.LabelFailureDomainBetaZone, + Key: v1.LabelTopologyZone, Values: []string{"zoneZ"}, }, }, @@ -1286,7 +1322,7 @@ func TestSelectZonesForVolume(t *testing.T) { { MatchLabelExpressions: []v1.TopologySelectorLabelRequirement{ { - Key: v1.LabelFailureDomainBetaZone, + Key: v1.LabelTopologyZone, Values: []string{"zoneX"}, }, }, @@ -1294,7 +1330,7 @@ func TestSelectZonesForVolume(t *testing.T) { { MatchLabelExpressions: []v1.TopologySelectorLabelRequirement{ { - Key: v1.LabelFailureDomainBetaZone, + Key: v1.LabelTopologyZone, Values: []string{"zoneY"}, }, }, @@ -1321,7 +1357,7 @@ func TestSelectZonesForVolume(t *testing.T) { { MatchLabelExpressions: []v1.TopologySelectorLabelRequirement{ { - Key: v1.LabelFailureDomainBetaZone, + Key: v1.LabelTopologyZone, Values: []string{"zoneX"}, }, }, @@ -1329,7 +1365,7 @@ func TestSelectZonesForVolume(t *testing.T) { { MatchLabelExpressions: []v1.TopologySelectorLabelRequirement{ { - Key: v1.LabelFailureDomainBetaZone, + Key: v1.LabelTopologyZone, Values: []string{"zoneY"}, }, }, @@ -1337,7 +1373,7 @@ func TestSelectZonesForVolume(t *testing.T) { { MatchLabelExpressions: []v1.TopologySelectorLabelRequirement{ { - Key: v1.LabelFailureDomainBetaZone, + Key: v1.LabelTopologyZone, Values: []string{"zoneZ"}, }, }, @@ -1361,7 +1397,7 @@ func TestSelectZonesForVolume(t *testing.T) { { MatchLabelExpressions: []v1.TopologySelectorLabelRequirement{ { - Key: v1.LabelFailureDomainBetaZone, + Key: v1.LabelTopologyZone, Values: []string{"zoneX"}, }, }, @@ -1369,7 +1405,7 @@ func TestSelectZonesForVolume(t *testing.T) { { MatchLabelExpressions: []v1.TopologySelectorLabelRequirement{ { - Key: v1.LabelFailureDomainBetaZone, + Key: v1.LabelTopologyZone, Values: []string{"zoneY"}, }, }, diff --git a/staging/src/k8s.io/csi-translation-lib/plugins/aws_ebs.go b/staging/src/k8s.io/csi-translation-lib/plugins/aws_ebs.go index 5db1e921f8d..843da7d867d 100644 --- a/staging/src/k8s.io/csi-translation-lib/plugins/aws_ebs.go +++ b/staging/src/k8s.io/csi-translation-lib/plugins/aws_ebs.go @@ -142,7 +142,7 @@ func (t *awsElasticBlockStoreCSITranslator) TranslateInTreePVToCSI(pv *v1.Persis }, } - if err := translateTopology(pv, AWSEBSTopologyKey); err != nil { + if err := translateTopologyFromInTreeToCSI(pv, AWSEBSTopologyKey); err != nil { return nil, fmt.Errorf("failed to translate topology: %v", err) } 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 1a1541019ce..ca02aac507c 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 @@ -215,7 +215,12 @@ func (g *gcePersistentDiskCSITranslator) TranslateInTreePVToCSI(pv *v1.Persisten return nil, fmt.Errorf("pv is nil or GCE Persistent Disk source not defined on pv") } + // depend on which version it migrates from, the label could be failuredomain beta or topology GA version zonesLabel := pv.Labels[v1.LabelFailureDomainBetaZone] + if zonesLabel == "" { + zonesLabel = pv.Labels[v1.LabelTopologyZone] + } + zones := strings.Split(zonesLabel, labelMultiZoneDelimiter) if len(zones) == 1 && len(zones[0]) != 0 { // Zonal @@ -249,7 +254,7 @@ func (g *gcePersistentDiskCSITranslator) TranslateInTreePVToCSI(pv *v1.Persisten }, } - if err := translateTopology(pv, GCEPDTopologyKey); err != nil { + if err := translateTopologyFromInTreeToCSI(pv, GCEPDTopologyKey); err != nil { return nil, fmt.Errorf("failed to translate topology: %v", err) } @@ -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, gceRegionParser); err != nil { + return nil, fmt.Errorf("failed to translate topology. PV:%+v. Error:%v", *pv, err) + } pv.Spec.CSI = nil pv.Spec.GCEPersistentDisk = gceSource @@ -369,6 +377,39 @@ func pdNameFromVolumeID(id string) (string, error) { return splitID[volIDDiskNameValue], nil } +func gceRegionParser(pv *v1.PersistentVolume) (string, error) { + _, zoneLabel, regionLabel := getTopologyLabel(pv) + + 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) + } + return regionVal[0], nil + } + + // 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) + } + + // 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) +} + // TODO: Replace this with the imported one from GCE PD CSI Driver when // the driver removes all k8s/k8s dependencies func getRegionFromZones(zones []string) (string, error) { 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 9d22b80aad7..c2a9e8956de 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 @@ -19,6 +19,7 @@ package plugins import ( "errors" "fmt" + "sort" "strings" v1 "k8s.io/api/core/v1" @@ -88,23 +89,30 @@ func replaceTopology(pv *v1.PersistentVolume, oldKey, newKey string) error { return nil } -// getTopologyZones returns all topology zones with the given key found in the PV. -func getTopologyZones(pv *v1.PersistentVolume, key string) []string { +// getTopologyValues returns all unique topology values with the given key found in the PV. +func getTopologyValues(pv *v1.PersistentVolume, key string) []string { if pv.Spec.NodeAffinity == nil || pv.Spec.NodeAffinity.Required == nil || len(pv.Spec.NodeAffinity.Required.NodeSelectorTerms) < 1 { return nil } - var values []string + values := make(map[string]bool) 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...) + for _, v := range r.Values { + values[v] = true + } } } } - return values + var re []string + for k := range values { + re = append(re, k) + } + sort.Strings(re) + return re } // addTopology appends the topology to the given PV. @@ -124,9 +132,17 @@ func addTopology(pv *v1.PersistentVolume, topologyKey string, zones []string) er } // Make sure the necessary fields exist - pv.Spec.NodeAffinity = new(v1.VolumeNodeAffinity) - pv.Spec.NodeAffinity.Required = new(v1.NodeSelector) - pv.Spec.NodeAffinity.Required.NodeSelectorTerms = make([]v1.NodeSelectorTerm, 1) + if pv.Spec.NodeAffinity == nil { + pv.Spec.NodeAffinity = new(v1.VolumeNodeAffinity) + } + + if pv.Spec.NodeAffinity.Required == nil { + pv.Spec.NodeAffinity.Required = new(v1.NodeSelector) + } + + if len(pv.Spec.NodeAffinity.Required.NodeSelectorTerms) == 0 { + pv.Spec.NodeAffinity.Required.NodeSelectorTerms = make([]v1.NodeSelectorTerm, 1) + } topology := v1.NodeSelectorRequirement{ Key: topologyKey, @@ -134,28 +150,86 @@ func addTopology(pv *v1.PersistentVolume, topologyKey string, zones []string) er Values: zones, } - pv.Spec.NodeAffinity.Required.NodeSelectorTerms[0].MatchExpressions = append( - pv.Spec.NodeAffinity.Required.NodeSelectorTerms[0].MatchExpressions, - topology, - ) + // add the CSI topology to each term + for i := range pv.Spec.NodeAffinity.Required.NodeSelectorTerms { + pv.Spec.NodeAffinity.Required.NodeSelectorTerms[i].MatchExpressions = append( + pv.Spec.NodeAffinity.Required.NodeSelectorTerms[i].MatchExpressions, + topology, + ) + } return nil } -// translateTopology converts existing zone labels or in-tree topology to CSI topology. +// 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 translateTopology(pv *v1.PersistentVolume, topologyKey string) error { +func translateTopologyFromInTreeToCSI(pv *v1.PersistentVolume, topologyKey string) error { // If topology is already set, assume the content is accurate - if len(getTopologyZones(pv, topologyKey)) > 0 { + if len(getTopologyValues(pv, topologyKey)) > 0 { return nil } - zones := getTopologyZones(pv, v1.LabelFailureDomainBetaZone) + _, zoneLabel, _ := getTopologyLabel(pv) + + // migrate topology node affinity + zones := getTopologyValues(pv, zoneLabel) if len(zones) > 0 { - return replaceTopology(pv, v1.LabelFailureDomainBetaZone, topologyKey) + return replaceTopology(pv, zoneLabel, topologyKey) } - if label, ok := pv.Labels[v1.LabelFailureDomainBetaZone]; ok { + // 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) @@ -165,6 +239,157 @@ func translateTopology(pv *v1.PersistentVolume, topologyKey string) error { 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) { + + // 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 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 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 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 + } + + // No labels or NodeAffinity exist, default to GA version + return true, v1.LabelTopologyZone, v1.LabelTopologyRegion +} + +// TopologyKeyExist checks if a certain key exists in a VolumeNodeAffinity +func TopologyKeyExist(key string, vna *v1.VolumeNodeAffinity) bool { + if vna == nil || vna.Required == nil || vna.Required.NodeSelectorTerms == nil || len(vna.Required.NodeSelectorTerms) == 0 { + return false + } + + for _, nodeSelectorTerms := range vna.Required.NodeSelectorTerms { + nsrequirements := nodeSelectorTerms.MatchExpressions + for _, nodeSelectorRequirement := range nsrequirements { + if nodeSelectorRequirement.Key == key { + return true + } + } + } + return false +} + +type regionParser func(pv *v1.PersistentVolume) (string, 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 { + + csiTopologyZoneValues := getTopologyValues(pv, topologyKey) + + // 1. Frist remove the CSI topology Key + removeTopology(pv, topologyKey) + + _, 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) + } + } + } + } + + // 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 { + if pv.Labels == nil { + pv.Labels = make(map[string]string) + } + 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 + } + } + + return nil +} + // translateAllowedTopologies translates allowed topologies within storage class // from legacy failure domain to given CSI topology key func translateAllowedTopologies(terms []v1.TopologySelectorTerm, key string) ([]v1.TopologySelectorTerm, error) { @@ -177,7 +402,7 @@ func translateAllowedTopologies(terms []v1.TopologySelectorTerm, key string) ([] newTerm := v1.TopologySelectorTerm{} for _, exp := range term.MatchLabelExpressions { var newExp v1.TopologySelectorLabelRequirement - if exp.Key == v1.LabelFailureDomainBetaZone { + if exp.Key == v1.LabelFailureDomainBetaZone || exp.Key == v1.LabelTopologyZone { newExp = v1.TopologySelectorLabelRequirement{ Key: key, Values: exp.Values, 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 db5685c1a5d..5c2efb1c4c9 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 @@ -21,8 +21,403 @@ import ( "testing" v1 "k8s.io/api/core/v1" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" ) +var ( + useast1aGALabels = map[string]string{ + v1.LabelTopologyZone: "us-east1-a", + v1.LabelTopologyRegion: "us-east1", + } + useast1aGANodeSelectorTerm = []v1.NodeSelectorTerm{ + { + MatchExpressions: []v1.NodeSelectorRequirement{ + { + Key: v1.LabelTopologyZone, + Operator: v1.NodeSelectorOpIn, + Values: []string{"us-east1-a"}, + }, + { + Key: v1.LabelTopologyRegion, + Operator: v1.NodeSelectorOpIn, + Values: []string{"us-east1"}, + }, + }, + }, + } + + uswest2bBetaLabels = map[string]string{ + v1.LabelFailureDomainBetaZone: "us-west2-b", + v1.LabelFailureDomainBetaRegion: "us-west2", + } + + uswest2bBetaNodeSelectorTerm = []v1.NodeSelectorTerm{ + { + MatchExpressions: []v1.NodeSelectorRequirement{ + { + Key: v1.LabelFailureDomainBetaZone, + Operator: v1.NodeSelectorOpIn, + Values: []string{"us-west2-b"}, + }, + { + Key: v1.LabelFailureDomainBetaRegion, + Operator: v1.NodeSelectorOpIn, + Values: []string{"us-west2"}, + }, + }, + }, + } +) + +func TestTranslateTopologyFromCSIToInTree(t *testing.T) { + testCases := []struct { + name string + key string + expErr bool + regionParser regionParser + 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, + pv: &v1.PersistentVolume{ + ObjectMeta: metav1.ObjectMeta{ + Name: "gcepd", Namespace: "myns", + Labels: useast1aGALabels, + }, + Spec: v1.PersistentVolumeSpec{ + NodeAffinity: &v1.VolumeNodeAffinity{ + Required: &v1.NodeSelector{ + NodeSelectorTerms: []v1.NodeSelectorTerm{ + { + MatchExpressions: []v1.NodeSelectorRequirement{ + { + Key: v1.LabelTopologyZone, + Operator: v1.NodeSelectorOpIn, + Values: []string{"us-east1-a"}, + }, + { + Key: v1.LabelTopologyRegion, + Operator: v1.NodeSelectorOpIn, + Values: []string{"us-east1"}, + }, + { + Key: GCEPDTopologyKey, + Operator: v1.NodeSelectorOpIn, + Values: []string{"whatever"}, + }, + }, + }, + }, + }, + }, + }, + }, + expectedNodeSelectorTerms: useast1aGANodeSelectorTerm, + expectedLabels: useast1aGALabels, + }, + { + name: "Remove CSI Topology Key and do not change existing Beta Kubernetes topology", + key: GCEPDTopologyKey, + expErr: false, + regionParser: gceRegionParser, + pv: &v1.PersistentVolume{ + ObjectMeta: metav1.ObjectMeta{ + Name: "gcepd", Namespace: "myns", + Labels: uswest2bBetaLabels, + }, + Spec: v1.PersistentVolumeSpec{ + NodeAffinity: &v1.VolumeNodeAffinity{ + Required: &v1.NodeSelector{ + NodeSelectorTerms: []v1.NodeSelectorTerm{ + { + MatchExpressions: []v1.NodeSelectorRequirement{ + { + Key: v1.LabelFailureDomainBetaZone, + Operator: v1.NodeSelectorOpIn, + Values: []string{"us-west2-b"}, + }, + { + Key: v1.LabelFailureDomainBetaRegion, + Operator: v1.NodeSelectorOpIn, + Values: []string{"us-west2"}, + }, + { + Key: GCEPDTopologyKey, + Operator: v1.NodeSelectorOpIn, + Values: []string{"whatever"}, + }, + }, + }, + }, + }, + }, + }, + }, + expectedNodeSelectorTerms: uswest2bBetaNodeSelectorTerm, + expectedLabels: uswest2bBetaLabels, + }, + { + name: "Remove CSI Topology Key and add Kubernetes topology from NodeAffinity, ignore labels", + key: GCEPDTopologyKey, + expErr: false, + regionParser: gceRegionParser, + pv: &v1.PersistentVolume{ + ObjectMeta: metav1.ObjectMeta{ + Name: "gcepd", Namespace: "myns", + Labels: map[string]string{ + v1.LabelTopologyZone: "existingZone", + v1.LabelTopologyRegion: "existingRegion", + }, + }, + Spec: v1.PersistentVolumeSpec{ + NodeAffinity: &v1.VolumeNodeAffinity{ + Required: &v1.NodeSelector{ + NodeSelectorTerms: []v1.NodeSelectorTerm{ + { + MatchExpressions: []v1.NodeSelectorRequirement{ + { + Key: GCEPDTopologyKey, + Operator: v1.NodeSelectorOpIn, + Values: []string{"us-east1-a"}, + }, + }, + }, + }, + }, + }, + }, + }, + expectedNodeSelectorTerms: useast1aGANodeSelectorTerm, + expectedLabels: map[string]string{ + v1.LabelTopologyRegion: "existingRegion", + v1.LabelTopologyZone: "existingZone", + }, + }, + { + name: "Add GA Kubernetes topology from labels", + key: GCEPDTopologyKey, + expErr: false, + regionParser: gceRegionParser, + pv: &v1.PersistentVolume{ + ObjectMeta: metav1.ObjectMeta{ + Name: "gcepd", Namespace: "myns", + Labels: map[string]string{ + v1.LabelTopologyZone: "existingZone", + 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"}, + }, + }, + }, + }, + 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, + pv: &v1.PersistentVolume{ + ObjectMeta: metav1.ObjectMeta{ + Name: "gcepd", Namespace: "myns", + }, + Spec: v1.PersistentVolumeSpec{ + NodeAffinity: &v1.VolumeNodeAffinity{ + Required: &v1.NodeSelector{ + NodeSelectorTerms: []v1.NodeSelectorTerm{ + { + MatchExpressions: []v1.NodeSelectorRequirement{ + { + Key: GCEPDTopologyKey, + Operator: v1.NodeSelectorOpIn, + Values: []string{"us-east1-a"}, + }, + }, + }, + }, + }, + }, + }, + }, + expectedNodeSelectorTerms: useast1aGANodeSelectorTerm, + expectedLabels: useast1aGALabels, + }, + { + name: "Generate Beta labels and kubernetes topology from CSI topology with partial Beta NodeAffinity", + key: GCEPDTopologyKey, + expErr: false, + regionParser: gceRegionParser, + pv: &v1.PersistentVolume{ + ObjectMeta: metav1.ObjectMeta{ + Name: "gcepd", Namespace: "myns", + }, + Spec: v1.PersistentVolumeSpec{ + NodeAffinity: &v1.VolumeNodeAffinity{ + Required: &v1.NodeSelector{ + NodeSelectorTerms: []v1.NodeSelectorTerm{ + { + MatchExpressions: []v1.NodeSelectorRequirement{ + { + Key: GCEPDTopologyKey, + Operator: v1.NodeSelectorOpIn, + Values: []string{"us-west2-b"}, + }, + { + Key: v1.LabelFailureDomainBetaZone, + Operator: v1.NodeSelectorOpIn, + Values: []string{"us-west2-b"}, + }, + }, + }, + }, + }, + }, + }, + }, + expectedNodeSelectorTerms: uswest2bBetaNodeSelectorTerm, + expectedLabels: uswest2bBetaLabels, + }, + { + name: "regionParser is missing and only zone labels get generated", + key: GCEPDTopologyKey, + expErr: false, + regionParser: nil, + pv: &v1.PersistentVolume{ + ObjectMeta: metav1.ObjectMeta{ + Name: "gcepd", Namespace: "myns", + }, + Spec: v1.PersistentVolumeSpec{ + NodeAffinity: &v1.VolumeNodeAffinity{ + Required: &v1.NodeSelector{ + NodeSelectorTerms: []v1.NodeSelectorTerm{ + { + MatchExpressions: []v1.NodeSelectorRequirement{ + { + Key: GCEPDTopologyKey, + Operator: v1.NodeSelectorOpIn, + Values: []string{"us-east1-a"}, + }, + }, + }, + }, + }, + }, + }, + }, + expectedNodeSelectorTerms: []v1.NodeSelectorTerm{ + { + MatchExpressions: []v1.NodeSelectorRequirement{ + { + Key: v1.LabelTopologyZone, + Operator: v1.NodeSelectorOpIn, + Values: []string{"us-east1-a"}, + }, + }, + }, + }, + expectedLabels: map[string]string{ + v1.LabelTopologyZone: "us-east1-a", + }, + }, + { + name: "Remove multi-term CSI Topology Key and add GA Kubernetes topology", + key: GCEPDTopologyKey, + expErr: false, + regionParser: gceRegionParser, + pv: &v1.PersistentVolume{ + ObjectMeta: metav1.ObjectMeta{ + Name: "gcepd", Namespace: "myns", + }, + Spec: v1.PersistentVolumeSpec{ + NodeAffinity: &v1.VolumeNodeAffinity{ + Required: &v1.NodeSelector{ + NodeSelectorTerms: []v1.NodeSelectorTerm{ + { + MatchExpressions: []v1.NodeSelectorRequirement{ + { + Key: GCEPDTopologyKey, + Operator: v1.NodeSelectorOpIn, + Values: []string{"us-east1-a"}, + }, + }, + }, + { + MatchExpressions: []v1.NodeSelectorRequirement{ + { + Key: GCEPDTopologyKey, + Operator: v1.NodeSelectorOpIn, + Values: []string{"us-east1-c"}, + }, + }, + }, + }, + }, + }, + }, + }, + expectedNodeSelectorTerms: []v1.NodeSelectorTerm{ + { + MatchExpressions: []v1.NodeSelectorRequirement{ + { + Key: v1.LabelTopologyZone, + Operator: v1.NodeSelectorOpIn, + Values: []string{"us-east1-a", "us-east1-c"}, + }, + { + Key: v1.LabelTopologyRegion, + Operator: v1.NodeSelectorOpIn, + Values: []string{"us-east1"}, + }, + }, + }, + }, + expectedLabels: map[string]string{ + v1.LabelTopologyZone: "us-east1-a__us-east1-c", + v1.LabelTopologyRegion: "us-east1", + }, + }, + } + + for _, tc := range testCases { + t.Logf("Running test: %v", tc.name) + err := translateTopologyFromCSIToInTree(tc.pv, tc.key, tc.regionParser) + 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) + } + if !reflect.DeepEqual(tc.pv.Labels, tc.expectedLabels) { + t.Errorf("Expected labels: %v, but got: %v", tc.expectedLabels, tc.pv.Labels) + } + } +} + func TestTranslateAllowedTopologies(t *testing.T) { testCases := []struct { name string @@ -212,3 +607,146 @@ func TestAddTopology(t *testing.T) { } } } + +func TestRemoveTopology(t *testing.T) { + testCases := []struct { + name string + topologyKey string + pv *v1.PersistentVolume + expOk bool + expectedAffinity *v1.VolumeNodeAffinity + }{ + { + name: "Remove single csi topology from PV", + topologyKey: GCEPDTopologyKey, + pv: makePVWithNodeSelectorTerms([]v1.NodeSelectorTerm{ + { + MatchExpressions: []v1.NodeSelectorRequirement{ + { + Key: GCEPDTopologyKey, + Operator: v1.NodeSelectorOpIn, + Values: []string{"us-east1-a"}, + }, + }, + }, + }), + expOk: true, + expectedAffinity: &v1.VolumeNodeAffinity{ + Required: &v1.NodeSelector{ + NodeSelectorTerms: []v1.NodeSelectorTerm{}, + }, + }, + }, + { + name: "Not found the topology key so do nothing", + topologyKey: GCEPDTopologyKey, + pv: makePVWithNodeSelectorTerms([]v1.NodeSelectorTerm{ + { + MatchExpressions: []v1.NodeSelectorRequirement{ + { + Key: v1.LabelTopologyZone, + Operator: v1.NodeSelectorOpIn, + Values: []string{"us-east1-a"}, + }, + }, + }, + }), + expOk: false, + expectedAffinity: &v1.VolumeNodeAffinity{ + Required: &v1.NodeSelector{ + NodeSelectorTerms: []v1.NodeSelectorTerm{ + { + MatchExpressions: []v1.NodeSelectorRequirement{ + { + Key: v1.LabelTopologyZone, + Operator: v1.NodeSelectorOpIn, + Values: []string{"us-east1-a"}, + }, + }, + }, + }, + }, + }, + }, + { + name: "Remove the topology key from multiple terms", + topologyKey: GCEPDTopologyKey, + pv: makePVWithNodeSelectorTerms([]v1.NodeSelectorTerm{ + { + MatchExpressions: []v1.NodeSelectorRequirement{ + { + Key: GCEPDTopologyKey, + Operator: v1.NodeSelectorOpIn, + Values: []string{"us-east1-a"}, + }, + }, + }, + { + MatchExpressions: []v1.NodeSelectorRequirement{ + { + Key: GCEPDTopologyKey, + Operator: v1.NodeSelectorOpIn, + Values: []string{"us-east1-c"}, + }, + }, + }, + }), + expOk: true, + expectedAffinity: &v1.VolumeNodeAffinity{ + Required: &v1.NodeSelector{ + NodeSelectorTerms: []v1.NodeSelectorTerm{}, + }, + }, + }, + { + name: "Remove the topology key from single term duplicate expression", + topologyKey: GCEPDTopologyKey, + pv: makePVWithNodeSelectorTerms([]v1.NodeSelectorTerm{ + { + MatchExpressions: []v1.NodeSelectorRequirement{ + { + Key: GCEPDTopologyKey, + Operator: v1.NodeSelectorOpIn, + Values: []string{"us-east1-a"}, + }, + { + Key: GCEPDTopologyKey, + Operator: v1.NodeSelectorOpIn, + Values: []string{"us-east1-a"}, + }, + }, + }, + }), + expOk: true, + expectedAffinity: &v1.VolumeNodeAffinity{ + Required: &v1.NodeSelector{ + NodeSelectorTerms: []v1.NodeSelectorTerm{}, + }, + }, + }, + } + + 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) + } + if !reflect.DeepEqual(tc.pv.Spec.NodeAffinity, tc.expectedAffinity) { + t.Errorf("Expected affinity: %v, but got: %v", tc.expectedAffinity, tc.pv.Spec.NodeAffinity) + } + } +} + +func makePVWithNodeSelectorTerms(nodeSelectorTerms []v1.NodeSelectorTerm) *v1.PersistentVolume { + return &v1.PersistentVolume{ + Spec: v1.PersistentVolumeSpec{ + NodeAffinity: &v1.VolumeNodeAffinity{ + Required: &v1.NodeSelector{ + NodeSelectorTerms: nodeSelectorTerms, + }, + }, + }, + } + +} diff --git a/staging/src/k8s.io/csi-translation-lib/plugins/openstack_cinder.go b/staging/src/k8s.io/csi-translation-lib/plugins/openstack_cinder.go index f3e21bc760a..c506e6acefc 100644 --- a/staging/src/k8s.io/csi-translation-lib/plugins/openstack_cinder.go +++ b/staging/src/k8s.io/csi-translation-lib/plugins/openstack_cinder.go @@ -95,7 +95,7 @@ func (t *osCinderCSITranslator) TranslateInTreePVToCSI(pv *v1.PersistentVolume) VolumeAttributes: map[string]string{}, } - if err := translateTopology(pv, CinderTopologyKey); err != nil { + if err := translateTopologyFromInTreeToCSI(pv, CinderTopologyKey); err != nil { return nil, fmt.Errorf("failed to translate topology: %v", err) } diff --git a/staging/src/k8s.io/csi-translation-lib/translate_test.go b/staging/src/k8s.io/csi-translation-lib/translate_test.go index da0e60416ef..fc32cf250b7 100644 --- a/staging/src/k8s.io/csi-translation-lib/translate_test.go +++ b/staging/src/k8s.io/csi-translation-lib/translate_test.go @@ -28,13 +28,20 @@ import ( ) var ( - defaultZoneLabels = map[string]string{ + kubernetesBetaTopologyLabels = map[string]string{ v1.LabelFailureDomainBetaZone: "us-east-1a", v1.LabelFailureDomainBetaRegion: "us-east-1", } - regionalPDLabels = map[string]string{ + kubernetesGATopologyLabels = map[string]string{ + v1.LabelTopologyZone: "us-east-1a", + v1.LabelTopologyRegion: "us-east-1", + } + regionalBetaPDLabels = map[string]string{ v1.LabelFailureDomainBetaZone: "europe-west1-b__europe-west1-c", } + regionalGAPDLabels = map[string]string{ + v1.LabelTopologyZone: "europe-west1-b__europe-west1-c", + } ) func TestTranslationStability(t *testing.T) { @@ -94,12 +101,20 @@ func TestTranslationStability(t *testing.T) { func TestTopologyTranslation(t *testing.T) { testCases := []struct { name string + key string pv *v1.PersistentVolume expectedNodeAffinity *v1.VolumeNodeAffinity }{ { - name: "GCE PD with zone labels", - pv: makeGCEPDPV(defaultZoneLabels, nil /*topology*/), + name: "GCE PD with beta zone labels", + key: plugins.GCEPDTopologyKey, + pv: makeGCEPDPV(kubernetesBetaTopologyLabels, nil /*topology*/), + expectedNodeAffinity: makeNodeAffinity(false /*multiTerms*/, plugins.GCEPDTopologyKey, "us-east-1a"), + }, + { + name: "GCE PD with GA kubernetes zone labels", + key: plugins.GCEPDTopologyKey, + pv: makeGCEPDPV(kubernetesGATopologyLabels, nil /*topology*/), expectedNodeAffinity: makeNodeAffinity(false /*multiTerms*/, plugins.GCEPDTopologyKey, "us-east-1a"), }, { @@ -109,35 +124,46 @@ func TestTopologyTranslation(t *testing.T) { }, { name: "GCE PD with existing topology (CSI keys)", + key: plugins.GCEPDTopologyKey, pv: makeGCEPDPV(nil /*labels*/, makeTopology(plugins.GCEPDTopologyKey, "us-east-2a")), expectedNodeAffinity: makeNodeAffinity(false /*multiTerms*/, plugins.GCEPDTopologyKey, "us-east-2a"), }, { name: "GCE PD with zone labels and topology", - pv: makeGCEPDPV(defaultZoneLabels, makeTopology(v1.LabelFailureDomainBetaZone, "us-east-2a")), + pv: makeGCEPDPV(kubernetesBetaTopologyLabels, makeTopology(v1.LabelFailureDomainBetaZone, "us-east-2a")), expectedNodeAffinity: makeNodeAffinity(false /*multiTerms*/, plugins.GCEPDTopologyKey, "us-east-2a"), }, { name: "GCE PD with regional zones", - pv: makeGCEPDPV(regionalPDLabels, nil /*topology*/), + key: plugins.GCEPDTopologyKey, + pv: makeGCEPDPV(regionalBetaPDLabels, nil /*topology*/), expectedNodeAffinity: makeNodeAffinity(false /*multiTerms*/, plugins.GCEPDTopologyKey, "europe-west1-b", "europe-west1-c"), }, { name: "GCE PD with regional topology", - pv: makeGCEPDPV(nil /*labels*/, makeTopology(v1.LabelFailureDomainBetaZone, "europe-west1-b", "europe-west1-c")), + key: plugins.GCEPDTopologyKey, + pv: makeGCEPDPV(nil /*labels*/, makeTopology(v1.LabelTopologyZone, "europe-west1-b", "europe-west1-c")), expectedNodeAffinity: makeNodeAffinity(false /*multiTerms*/, plugins.GCEPDTopologyKey, "europe-west1-b", "europe-west1-c"), }, { - name: "GCE PD with regional zone and topology", - pv: makeGCEPDPV(regionalPDLabels, makeTopology(v1.LabelFailureDomainBetaZone, "europe-west1-f", "europe-west1-g")), + name: "GCE PD with Beta regional zone and topology", + key: plugins.GCEPDTopologyKey, + pv: makeGCEPDPV(regionalBetaPDLabels, makeTopology(v1.LabelFailureDomainBetaZone, "europe-west1-f", "europe-west1-g")), + expectedNodeAffinity: makeNodeAffinity(false /*multiTerms*/, plugins.GCEPDTopologyKey, "europe-west1-f", "europe-west1-g"), + }, + { + name: "GCE PD with GA regional zone and topology", + key: plugins.GCEPDTopologyKey, + pv: makeGCEPDPV(regionalGAPDLabels, makeTopology(v1.LabelTopologyZone, "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", + key: plugins.GCEPDTopologyKey, pv: makeGCEPDPVMultTerms( nil, /*labels*/ - makeTopology(v1.LabelFailureDomainBetaZone, "europe-west1-f"), - makeTopology(v1.LabelFailureDomainBetaZone, "europe-west1-g")), + makeTopology(v1.LabelTopologyZone, "europe-west1-f"), + makeTopology(v1.LabelTopologyZone, "europe-west1-g")), expectedNodeAffinity: makeNodeAffinity( true, /*multiTerms*/ plugins.GCEPDTopologyKey, "europe-west1-f", "europe-west1-g"), @@ -145,23 +171,23 @@ func TestTopologyTranslation(t *testing.T) { // 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*/), + pv: makeAWSEBSPV(kubernetesBetaTopologyLabels, nil /*topology*/), expectedNodeAffinity: makeNodeAffinity(false /*multiTerms*/, plugins.AWSEBSTopologyKey, "us-east-1a"), }, { name: "AWS EBS with zone labels and topology", - pv: makeAWSEBSPV(defaultZoneLabels, makeTopology(v1.LabelFailureDomainBetaZone, "us-east-2a")), + pv: makeAWSEBSPV(kubernetesBetaTopologyLabels, makeTopology(v1.LabelFailureDomainBetaZone, "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*/), + pv: makeCinderPV(kubernetesBetaTopologyLabels, nil /*topology*/), expectedNodeAffinity: makeNodeAffinity(false /*multiTerms*/, plugins.CinderTopologyKey, "us-east-1a"), }, { name: "OpenStack Cinder with zone labels and topology", - pv: makeCinderPV(defaultZoneLabels, makeTopology(v1.LabelFailureDomainBetaZone, "us-east-2a")), + pv: makeCinderPV(kubernetesBetaTopologyLabels, makeTopology(v1.LabelFailureDomainBetaZone, "us-east-2a")), expectedNodeAffinity: makeNodeAffinity(false /*multiTerms*/, plugins.CinderTopologyKey, "us-east-2a"), }, } @@ -181,15 +207,27 @@ func TestTopologyTranslation(t *testing.T) { t.Errorf("Expected node affinity %v, got %v", *test.expectedNodeAffinity, *nodeAffinity) } - // Translate back to in-tree and make sure node affinity is still set + // Translate back to in-tree and make sure node affinity has been removed newInTreePV, err := ctl.TranslateCSIPVToInTree(newCSIPV) if err != nil { t.Errorf("Error when translating to in-tree: %v", err) } - nodeAffinity = newInTreePV.Spec.NodeAffinity - if !reflect.DeepEqual(nodeAffinity, test.expectedNodeAffinity) { - t.Errorf("Expected node affinity %v, got %v", *test.expectedNodeAffinity, *nodeAffinity) + // For now, non-pd cloud should stay the old behavior which is still have the CSI topology. + if test.key != "" { + nodeAffinity = newInTreePV.Spec.NodeAffinity + if plugins.TopologyKeyExist(test.key, nodeAffinity) { + t.Errorf("Expected node affinity key %v being removed, got %v", test.key, *nodeAffinity) + } + // verify that either beta or GA kubernetes topology key should exist + if !(plugins.TopologyKeyExist(v1.LabelFailureDomainBetaZone, nodeAffinity) || plugins.TopologyKeyExist(v1.LabelTopologyZone, nodeAffinity)) { + t.Errorf("Expected node affinity kuberenetes topology label exist, got %v", *nodeAffinity) + } + } else { + nodeAffinity := newCSIPV.Spec.NodeAffinity + if !reflect.DeepEqual(nodeAffinity, test.expectedNodeAffinity) { + t.Errorf("Expected node affinity %v, got %v", *test.expectedNodeAffinity, *nodeAffinity) + } } } } From f83df5d1621e967a140e4e3aafbd602de1848638 Mon Sep 17 00:00:00 2001 From: Jiawei Wang Date: Wed, 13 Jan 2021 01:00:38 -0800 Subject: [PATCH 2/2] 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 --- .../csi-translation-lib/plugins/gce_pd.go | 80 ++-- .../plugins/in_tree_volume.go | 269 +++++--------- .../plugins/in_tree_volume_test.go | 347 +++++++++++++----- 3 files changed, 403 insertions(+), 293 deletions(-) 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)