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 6cdde52b04b..5acc8529834 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..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 @@ -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, gceRegionTopologyHandler); 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,67 @@ func pdNameFromVolumeID(id string) (string, error) { return splitID[volIDDiskNameValue], nil } +// 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 { + + // 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...) + } + } + 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}, + }) + + } + + // 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] + } + } + + return nil +} + // 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..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 @@ -19,6 +19,7 @@ package plugins import ( "errors" "fmt" + "sort" "strings" v1 "k8s.io/api/core/v1" @@ -76,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 { @@ -85,29 +95,41 @@ 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 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 || 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 + // remove duplication and sort them in order for better usage + var re []string + for k := range values { + re = append(re, k) + } + sort.Strings(re) + 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{} @@ -124,9 +146,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,31 +164,141 @@ 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. -// In-tree topology has precedence over zone labels. -func translateTopology(pv *v1.PersistentVolume, topologyKey string) error { - // If topology is already set, assume the content is accurate - if len(getTopologyZones(pv, topologyKey)) > 0 { - return nil - } +// translateTopologyFromInTreeToCSI converts existing zone labels or in-tree topology to CSI topology. +// 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 { - zones := getTopologyZones(pv, v1.LabelFailureDomainBetaZone) + zoneLabel, regionLabel := getTopologyLabel(pv) + + // If Zone kubernetes topology exist, replace it to use csiTopologyKey + zones := getTopologyValues(pv, zoneLabel) if len(zones) > 0 { - return replaceTopology(pv, v1.LabelFailureDomainBetaZone, 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 label, ok := pv.Labels[v1.LabelFailureDomainBetaZone]; 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 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) { + + if zoneGA := TopologyKeyExist(v1.LabelTopologyZone, pv.Spec.NodeAffinity); zoneGA { + return v1.LabelTopologyZone, v1.LabelTopologyRegion + } + if zoneBeta := TopologyKeyExist(v1.LabelFailureDomainBetaZone, pv.Spec.NodeAffinity); zoneBeta { + return v1.LabelFailureDomainBetaZone, v1.LabelFailureDomainBetaRegion + } + if _, zoneGA := pv.Labels[v1.LabelTopologyZone]; zoneGA { + return v1.LabelTopologyZone, v1.LabelTopologyRegion + } + if _, zoneBeta := pv.Labels[v1.LabelFailureDomainBetaZone]; zoneBeta { + return v1.LabelFailureDomainBetaZone, v1.LabelFailureDomainBetaRegion + } + // No labels or NodeAffinity exist, default to GA version + return 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 regionTopologyHandler func(pv *v1.PersistentVolume) 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 { + + zoneLabel, _ := getTopologyLabel(pv) + + // 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) + } + + // 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) + } + } + + // 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 { + zoneValStr := strings.Join(zoneVals, labelMultiZoneDelimiter) + pv.Labels[zoneLabel] = zoneValStr } } @@ -177,7 +317,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..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 @@ -21,8 +21,521 @@ 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", + } + useast1aGANodeSelectorTermZoneFirst = []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"}, + }, + }, + }, + } + + 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", + } + + uswest2bBetaNodeSelectorTermZoneFirst = []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"}, + }, + }, + }, + } + + 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) { + testCases := []struct { + name string + key string + expErr bool + 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, + regionTopologyHandler: gceRegionTopologyHandler, + 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.LabelTopologyRegion, + Operator: v1.NodeSelectorOpIn, + Values: []string{"us-east1"}, + }, + { + Key: GCEPDTopologyKey, + Operator: v1.NodeSelectorOpIn, + Values: []string{"us-east1-a"}, + }, + }, + }, + }, + }, + }, + }, + }, + expectedNodeSelectorTerms: useast1aGANodeSelectorTermRegionFirst, + expectedLabels: useast1aGALabels, + }, + { + 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", + Labels: uswest2bBetaLabels, + }, + Spec: v1.PersistentVolumeSpec{ + NodeAffinity: &v1.VolumeNodeAffinity{ + Required: &v1.NodeSelector{ + NodeSelectorTerms: []v1.NodeSelectorTerm{ + { + MatchExpressions: []v1.NodeSelectorRequirement{ + { + Key: v1.LabelFailureDomainBetaRegion, + Operator: v1.NodeSelectorOpIn, + Values: []string{"us-west2"}, + }, + { + Key: GCEPDTopologyKey, + Operator: v1.NodeSelectorOpIn, + Values: []string{"us-west2-b"}, + }, + }, + }, + }, + }, + }, + }, + }, + expectedNodeSelectorTerms: uswest2bBetaNodeSelectorTermRegionFirst, + expectedLabels: uswest2bBetaLabels, + }, + { + 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", + 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: useast1aGANodeSelectorTermZoneFirst, + expectedLabels: map[string]string{ + v1.LabelTopologyRegion: "existingRegion", + v1.LabelTopologyZone: "existingZone", + }, + }, + { + 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", + Labels: map[string]string{ + v1.LabelTopologyZone: "existingZone", + v1.LabelTopologyRegion: "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, + regionTopologyHandler: gceRegionTopologyHandler, + 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: useast1aGANodeSelectorTermZoneFirst, + expectedLabels: useast1aGALabels, + }, + { + 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", + }, + 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"}, + }, + }, + }, + }, + }, + }, + }, + }, + expectedNodeSelectorTerms: uswest2bBetaNodeSelectorTermZoneFirst, + expectedLabels: uswest2bBetaLabels, + }, + { + 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", + }, + 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: "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", + }, + 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"}, + }, + { + 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, + 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.regionTopologyHandler) + 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 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 @@ -212,3 +725,195 @@ func TestAddTopology(t *testing.T) { } } } + +func TestReplaceTopology(t *testing.T) { + testCases := []struct { + name string + oldKey string + newKey string + pv *v1.PersistentVolume + expOk bool + expectedAffinity *v1.VolumeNodeAffinity + }{ + { + name: "Replace single csi topology from PV", + oldKey: GCEPDTopologyKey, + newKey: v1.LabelTopologyZone, + 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{ + { + MatchExpressions: []v1.NodeSelectorRequirement{ + { + Key: v1.LabelTopologyZone, + Operator: v1.NodeSelectorOpIn, + Values: []string{"us-east1-a"}, + }, + }, + }, + }, + }, + }, + }, + { + name: "Not found the topology key so do nothing", + oldKey: GCEPDTopologyKey, + newKey: v1.LabelTopologyZone, + 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: "Replace the topology key from multiple terms", + oldKey: GCEPDTopologyKey, + newKey: v1.LabelTopologyZone, + 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{ + { + 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: "Replace the topology key from single term and not combine topology key", + oldKey: GCEPDTopologyKey, + newKey: v1.LabelTopologyZone, + pv: makePVWithNodeSelectorTerms([]v1.NodeSelectorTerm{ + { + MatchExpressions: []v1.NodeSelectorRequirement{ + { + Key: GCEPDTopologyKey, + Operator: v1.NodeSelectorOpIn, + Values: []string{"us-east1-a"}, + }, + { + Key: v1.LabelTopologyZone, + Operator: v1.NodeSelectorOpIn, + Values: []string{"us-east1-c"}, + }, + }, + }, + }), + expOk: true, + expectedAffinity: &v1.VolumeNodeAffinity{ + Required: &v1.NodeSelector{ + 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"}, + }, + }, + }, + }, + }, + }, + }, + } + + for _, tc := range testCases { + t.Logf("Running test: %v", tc.name) + 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) + } + } +} + +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 3fb3862e568..9f2bfbde737 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 @@ -120,7 +120,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) + } } } }