Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Update gce-pd volume topology label to GA #98700

Merged
merged 1 commit into from Feb 4, 2021
Merged

Update gce-pd volume topology label to GA #98700

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
2 changes: 1 addition & 1 deletion pkg/volume/gcepd/attacher_test.go
View file
Open in desktop
Expand Up @@ -458,7 +458,7 @@ func createPVSpec(name string, readOnly bool, zones []string) *volume.Spec {
if zones != nil {
zonesLabel := strings.Join(zones, cloudvolume.LabelMultiZoneDelimiter)
spec.PersistentVolume.ObjectMeta.Labels = map[string]string{
v1.LabelFailureDomainBetaZone: zonesLabel,
v1.LabelTopologyZone: zonesLabel,
}
}

Expand Down
2 changes: 1 addition & 1 deletion pkg/volume/gcepd/gce_pd.go
View file
Open in desktop
Expand Up @@ -545,7 +545,7 @@ func (c *gcePersistentDiskProvisioner) Provision(selectedNode *v1.Node, allowedT
for k, v := range labels {
pv.Labels[k] = v
var values []string
if k == v1.LabelFailureDomainBetaZone {
if k == v1.LabelTopologyZone {
values, err = volumehelpers.LabelZonesToList(v)
if err != nil {
return nil, fmt.Errorf("failed to convert label string for Zone: %s to a List: %v", v, err)
Expand Down
10 changes: 5 additions & 5 deletions pkg/volume/gcepd/gce_pd_test.go
View file
Open in desktop
Expand Up @@ -86,7 +86,7 @@ type fakePDManager struct {
func (fake *fakePDManager) CreateVolume(c *gcePersistentDiskProvisioner, node *v1.Node, allowedTopologies []v1.TopologySelectorTerm) (volumeID string, volumeSizeGB int, labels map[string]string, fstype string, err error) {
labels = make(map[string]string)
labels["fakepdmanager"] = "yes"
labels[v1.LabelFailureDomainBetaZone] = "zone1__zone2"
labels[v1.LabelTopologyZone] = "zone1__zone2"
return "test-gce-volume-name", 100, labels, "", nil
}

Expand Down Expand Up @@ -200,8 +200,8 @@ func TestPlugin(t *testing.T) {
t.Errorf("Provision() returned unexpected value for fakepdmanager: %v", persistentSpec.Labels["fakepdmanager"])
}

if persistentSpec.Labels[v1.LabelFailureDomainBetaZone] != "zone1__zone2" {
t.Errorf("Provision() returned unexpected value for %s: %v", v1.LabelFailureDomainBetaZone, persistentSpec.Labels[v1.LabelFailureDomainBetaZone])
if persistentSpec.Labels[v1.LabelTopologyZone] != "zone1__zone2" {
t.Errorf("Provision() returned unexpected value for %s: %v", v1.LabelTopologyZone, persistentSpec.Labels[v1.LabelTopologyZone])
}

if persistentSpec.Spec.NodeAffinity == nil {
Expand All @@ -219,9 +219,9 @@ func TestPlugin(t *testing.T) {
t.Errorf("NodeSelectorRequirement fakepdmanager-in-yes not found in volume NodeAffinity")
}
zones, _ := volumehelpers.ZonesToSet("zone1,zone2")
r, _ = getNodeSelectorRequirementWithKey(v1.LabelFailureDomainBetaZone, term)
r, _ = getNodeSelectorRequirementWithKey(v1.LabelTopologyZone, term)
if r == nil {
t.Errorf("NodeSelectorRequirement %s-in-%v not found in volume NodeAffinity", v1.LabelFailureDomainBetaZone, zones)
t.Errorf("NodeSelectorRequirement %s-in-%v not found in volume NodeAffinity", v1.LabelTopologyZone, zones)
} else {
sort.Strings(r.Values)
if !reflect.DeepEqual(r.Values, zones.List()) {
Expand Down
5 changes: 4 additions & 1 deletion pkg/volume/gcepd/gce_util.go
View file
Open in desktop
Expand Up @@ -356,7 +356,10 @@ func udevadmChangeToDrive(drivePath string) error {
// Checks whether the given GCE PD volume spec is associated with a regional PD.
func isRegionalPD(spec *volume.Spec) bool {
if spec.PersistentVolume != nil {
zonesLabel := spec.PersistentVolume.Labels[v1.LabelFailureDomainBetaZone]
zonesLabel := spec.PersistentVolume.Labels[v1.LabelTopologyZone]
if zonesLabel == "" {
zonesLabel = spec.PersistentVolume.Labels[v1.LabelFailureDomainBetaZone]
}
zones := strings.Split(zonesLabel, cloudvolume.LabelMultiZoneDelimiter)
return len(zones) > 1
}
Expand Down
15 changes: 9 additions & 6 deletions staging/src/k8s.io/legacy-cloud-providers/gce/gce_disks.go
View file
Open in desktop
Expand Up @@ -515,7 +515,10 @@ func (g *Cloud) GetLabelsForVolume(ctx context.Context, pv *v1.PersistentVolume)

// If the zone is already labeled, honor the hint
name := pv.Spec.GCEPersistentDisk.PDName
zone := pv.Labels[v1.LabelFailureDomainBetaZone]
zone := pv.Labels[v1.LabelTopologyZone]
Jiawei0227 marked this conversation as resolved.
Show resolved Hide resolved
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

FYI, We are no longer accepting enhancements on legacy-cloud-providers. Any such enhancements must be done out of tree. As this is clearly just a bug fix going to let it through.

if zone == "" {
zone = pv.Labels[v1.LabelFailureDomainBetaZone]
}

disk, err := g.getDiskByNameAndOptionalLabelZones(name, zone)
if err != nil {
Expand Down Expand Up @@ -848,7 +851,7 @@ func (g *Cloud) ResizeDisk(diskToResize string, oldSize resource.Quantity, newSi
}

// GetAutoLabelsForPD builds the labels that should be automatically added to a PersistentVolume backed by a GCE PD
// Specifically, this builds FailureDomain (zone) and Region labels.
// Specifically, this builds Topology (zone) and Region labels.
// The PersistentVolumeLabel admission controller calls this and adds the labels when a PV is created.
func (g *Cloud) GetAutoLabelsForPD(disk *Disk) (map[string]string, error) {
labels := make(map[string]string)
Expand All @@ -858,16 +861,16 @@ func (g *Cloud) GetAutoLabelsForPD(disk *Disk) (map[string]string, error) {
// Unexpected, but sanity-check
return nil, fmt.Errorf("PD did not have zone/region information: %v", disk)
}
labels[v1.LabelFailureDomainBetaZone] = zoneInfo.zone
labels[v1.LabelFailureDomainBetaRegion] = disk.Region
labels[v1.LabelTopologyZone] = zoneInfo.zone
labels[v1.LabelTopologyRegion] = disk.Region
case multiZone:
if zoneInfo.replicaZones == nil || zoneInfo.replicaZones.Len() <= 0 {
// Unexpected, but sanity-check
return nil, fmt.Errorf("PD is regional but does not have any replicaZones specified: %v", disk)
}
labels[v1.LabelFailureDomainBetaZone] =
labels[v1.LabelTopologyZone] =
volumehelpers.ZonesSetToLabelValue(zoneInfo.replicaZones)
labels[v1.LabelFailureDomainBetaRegion] = disk.Region
labels[v1.LabelTopologyRegion] = disk.Region
case nil:
// Unexpected, but sanity-check
return nil, fmt.Errorf("PD did not have ZoneInfo: %v", disk)
Expand Down
46 changes: 23 additions & 23 deletions staging/src/k8s.io/legacy-cloud-providers/gce/gce_disks_test.go
View file
Open in desktop
Expand Up @@ -443,7 +443,7 @@ func pv(name, zone string) *v1.PersistentVolume {
return &v1.PersistentVolume{
ObjectMeta: metav1.ObjectMeta{
Labels: map[string]string{
v1.LabelFailureDomainBetaZone: zone,
v1.LabelTopologyZone: zone,
},
},
Spec: v1.PersistentVolumeSpec{
Expand Down Expand Up @@ -484,12 +484,12 @@ func TestGetLabelsForVolume_Basic(t *testing.T) {
if err != nil {
t.Error(err)
}
if labels[v1.LabelFailureDomainBetaZone] != zone {
t.Errorf("Failure domain is '%v', but zone is '%v'",
labels[v1.LabelFailureDomainBetaZone], zone)
if labels[v1.LabelTopologyZone] != zone {
t.Errorf("Topology Zone is '%v', but zone is '%v'",
labels[v1.LabelTopologyZone], zone)
}
if labels[v1.LabelFailureDomainBetaRegion] != gceRegion {
t.Errorf("Region is '%v', but region is 'us-central1'", labels[v1.LabelFailureDomainBetaRegion])
if labels[v1.LabelTopologyRegion] != gceRegion {
t.Errorf("Region is '%v', but region is 'us-central1'", labels[v1.LabelTopologyRegion])
}
}

Expand All @@ -515,7 +515,7 @@ func TestGetLabelsForVolume_NoZone(t *testing.T) {
gce.CreateDisk(diskName, diskType, zone, sizeGb, nil)

pv := pv(diskName, zone)
delete(pv.Labels, v1.LabelFailureDomainBetaZone)
delete(pv.Labels, v1.LabelTopologyZone)

/* Act */
labels, err := gce.GetLabelsForVolume(ctx, pv)
Expand All @@ -524,12 +524,12 @@ func TestGetLabelsForVolume_NoZone(t *testing.T) {
if err != nil {
t.Error(err)
}
if labels[v1.LabelFailureDomainBetaZone] != zone {
t.Errorf("Failure domain is '%v', but zone is '%v'",
labels[v1.LabelFailureDomainBetaZone], zone)
if labels[v1.LabelTopologyZone] != zone {
t.Errorf("Topology Zone is '%v', but zone is '%v'",
labels[v1.LabelTopologyZone], zone)
}
if labels[v1.LabelFailureDomainBetaRegion] != gceRegion {
t.Errorf("Region is '%v', but region is 'europe-west1'", labels[v1.LabelFailureDomainBetaRegion])
if labels[v1.LabelTopologyRegion] != gceRegion {
t.Errorf("Region is '%v', but region is 'europe-west1'", labels[v1.LabelTopologyRegion])
}
}

Expand Down Expand Up @@ -575,7 +575,7 @@ func TestGetLabelsForVolume_DiskNotFoundAndNoZone(t *testing.T) {
}

pv := pv(diskName, zone)
delete(pv.Labels, v1.LabelFailureDomainBetaZone)
delete(pv.Labels, v1.LabelTopologyZone)

/* Act */
_, err := gce.GetLabelsForVolume(ctx, pv)
Expand Down Expand Up @@ -617,12 +617,12 @@ func TestGetLabelsForVolume_DupDisk(t *testing.T) {
if err != nil {
t.Error("Disk name and zone uniquely identifies a disk, yet an error is returned.")
}
if labels[v1.LabelFailureDomainBetaZone] != zone {
t.Errorf("Failure domain is '%v', but zone is '%v'",
labels[v1.LabelFailureDomainBetaZone], zone)
if labels[v1.LabelTopologyZone] != zone {
t.Errorf("Topology Zone is '%v', but zone is '%v'",
labels[v1.LabelTopologyZone], zone)
}
if labels[v1.LabelFailureDomainBetaRegion] != gceRegion {
t.Errorf("Region is '%v', but region is 'us-west1'", labels[v1.LabelFailureDomainBetaRegion])
if labels[v1.LabelTopologyRegion] != gceRegion {
t.Errorf("Region is '%v', but region is 'us-west1'", labels[v1.LabelTopologyRegion])
}
}

Expand Down Expand Up @@ -651,7 +651,7 @@ func TestGetLabelsForVolume_DupDiskNoZone(t *testing.T) {
}

pv := pv(diskName, zone)
delete(pv.Labels, v1.LabelFailureDomainBetaZone)
delete(pv.Labels, v1.LabelTopologyZone)

/* Act */
_, err := gce.GetLabelsForVolume(ctx, pv)
Expand Down Expand Up @@ -746,13 +746,13 @@ func TestGetAutoLabelsForPD(t *testing.T) {
return
}

if got := labels[v1.LabelFailureDomainBetaZone]; !tc.wantZoneLabel.Has(got) {
t.Errorf("labels[v1.LabelFailureDomainBetaZone] = %v; want one of: %v", got, tc.wantZoneLabel.List())
if got := labels[v1.LabelTopologyZone]; !tc.wantZoneLabel.Has(got) {
t.Errorf("labels[v1.LabelTopologyZone] = %v; want one of: %v", got, tc.wantZoneLabel.List())
}

// Validate labels
if got := labels[v1.LabelFailureDomainBetaRegion]; got != gceRegion {
t.Errorf("labels[v1.LabelFailureDomainBetaRegion] = %v; want: %v", got, gceRegion)
if got := labels[v1.LabelTopologyRegion]; got != gceRegion {
t.Errorf("labels[v1.LabelTopologyRegion] = %v; want: %v", got, gceRegion)
}
})
}
Expand Down
2 changes: 1 addition & 1 deletion test/e2e/storage/drivers/in_tree.go
View file
Open in desktop
Expand Up @@ -1243,7 +1243,7 @@ func InitGcePdDriver() storageframework.TestDriver {
},
SupportedFsType: supportedTypes,
SupportedMountOption: sets.NewString("debug", "nouid32"),
TopologyKeys: []string{v1.LabelFailureDomainBetaZone},
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmmm let's double check but I think we have upgrade tests than run N-1 e2e version against a N version cluster. We may need to make some changes to the older e2e to handle both labels.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

so there's a couple of variations of tests, but IIRC, one variation will run N-1 e2e.test on an N-1 cluster, upgrade the cluster, and then run N-1 e2e.test on the version N cluster

I run the e2e binary on new cluster with my change and the topology test pass.

./e2e.test --ginkgo.focus=".*In-tree Volumes.*gcepd.*topology.*" -provider gce -gce-project=jiaweiwang-gke-multi-cloud-dev -gce-zone=us-central1-b

{"msg":"PASSED [sig-storage] In-tree Volumes [Driver: gcepd] [Testpattern: Dynamic PV (delayed binding)] topology should provision a volume and schedule a pod with AllowedTopologies","total":8,"completed":1,"skipped":1030,"failed":0}
{"msg":"PASSED [sig-storage] In-tree Volumes [Driver: gcepd] [Testpattern: Dynamic PV (immediate binding)] topology should provision a volume and schedule a pod with AllowedTopologies","total":8,"completed":2,"skipped":2859,"failed":0}

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you be more specific if you have concerns on which tests?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Did you run e2e.test from 1 version before, ie release 1.20?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, I build the e2e.test from release-1.20 branch.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Did you also run the regional PD tests? (it requires a regional cluster). I see some parts of that test are validating the labels on the PV object, so I would expect a 1.20 test validating beta labels will fail on a 1.21 cluster that uses GA labels.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It seems regional PD is checking PV labels. So we need to fix the 1.20 release branch e2e test to not break the version skew test.
#98733 PR out for review and cherry-pick approval.

TopologyKeys: []string{v1.LabelTopologyZone},
Capabilities: map[storageframework.Capability]bool{
storageframework.CapPersistence: true,
storageframework.CapFsGroup: true,
Expand Down
34 changes: 17 additions & 17 deletions test/e2e/storage/regional_pd.go
View file
Open in desktop
Expand Up @@ -235,10 +235,10 @@ func testZonalFailover(c clientset.Interface, ns string) {
nodeName := pod.Spec.NodeName
node, err := c.CoreV1().Nodes().Get(context.TODO(), nodeName, metav1.GetOptions{})
framework.ExpectNoError(err)
podZone := node.Labels[v1.LabelFailureDomainBetaZone]
podZone := node.Labels[v1.LabelTopologyZone]

ginkgo.By("tainting nodes in the zone the pod is scheduled in")
selector := labels.SelectorFromSet(labels.Set(map[string]string{v1.LabelFailureDomainBetaZone: podZone}))
selector := labels.SelectorFromSet(labels.Set(map[string]string{v1.LabelTopologyZone: podZone}))
nodesInZone, err := c.CoreV1().Nodes().List(context.TODO(), metav1.ListOptions{LabelSelector: selector.String()})
framework.ExpectNoError(err)
removeTaintFunc := addTaint(c, ns, nodesInZone.Items, podZone)
Expand Down Expand Up @@ -266,7 +266,7 @@ func testZonalFailover(c clientset.Interface, ns string) {
if err != nil {
return false, nil
}
newPodZone := node.Labels[v1.LabelFailureDomainBetaZone]
newPodZone := node.Labels[v1.LabelTopologyZone]
return newPodZone == otherZone, nil
})
framework.ExpectNoError(waitErr, "Error waiting for pod to be scheduled in a different zone (%q): %v", otherZone, err)
Expand Down Expand Up @@ -354,9 +354,9 @@ func testRegionalDelayedBinding(c clientset.Interface, ns string, pvcCount int)
if node == nil {
framework.Failf("unexpected nil node found")
}
zone, ok := node.Labels[v1.LabelFailureDomainBetaZone]
zone, ok := node.Labels[v1.LabelTopologyZone]
if !ok {
framework.Failf("label %s not found on Node", v1.LabelFailureDomainBetaZone)
framework.Failf("label %s not found on Node", v1.LabelTopologyZone)
}
for _, pv := range pvs {
checkZoneFromLabelAndAffinity(pv, zone, false)
Expand Down Expand Up @@ -423,9 +423,9 @@ func testRegionalAllowedTopologiesWithDelayedBinding(c clientset.Interface, ns s
if node == nil {
framework.Failf("unexpected nil node found")
}
nodeZone, ok := node.Labels[v1.LabelFailureDomainBetaZone]
nodeZone, ok := node.Labels[v1.LabelTopologyZone]
if !ok {
framework.Failf("label %s not found on Node", v1.LabelFailureDomainBetaZone)
framework.Failf("label %s not found on Node", v1.LabelTopologyZone)
}
zoneFound := false
for _, zone := range topoZones {
Expand Down Expand Up @@ -466,7 +466,7 @@ func addAllowedTopologiesToStorageClass(c clientset.Interface, sc *storagev1.Sto
term := v1.TopologySelectorTerm{
MatchLabelExpressions: []v1.TopologySelectorLabelRequirement{
{
Key: v1.LabelFailureDomainBetaZone,
Key: v1.LabelTopologyZone,
Values: zones,
},
},
Expand Down Expand Up @@ -568,7 +568,7 @@ func getTwoRandomZones(c clientset.Interface) []string {
// If match is true, check if zones in PV exactly match zones given.
// Otherwise, check whether zones in PV is superset of zones given.
func verifyZonesInPV(volume *v1.PersistentVolume, zones sets.String, match bool) error {
pvZones, err := volumehelpers.LabelZonesToSet(volume.Labels[v1.LabelFailureDomainBetaZone])
pvZones, err := volumehelpers.LabelZonesToSet(volume.Labels[v1.LabelTopologyZone])
if err != nil {
return err
}
Expand All @@ -585,28 +585,28 @@ func checkZoneFromLabelAndAffinity(pv *v1.PersistentVolume, zone string, matchZo
checkZonesFromLabelAndAffinity(pv, sets.NewString(zone), matchZone)
}

// checkZoneLabelAndAffinity checks the LabelFailureDomainBetaZone label of PV and terms
// with key LabelFailureDomainBetaZone in PV's node affinity contains zone
// checkZoneLabelAndAffinity checks the LabelTopologyZone label of PV and terms
// with key LabelTopologyZone in PV's node affinity contains zone
// matchZones is used to indicate if zones should match perfectly
func checkZonesFromLabelAndAffinity(pv *v1.PersistentVolume, zones sets.String, matchZones bool) {
ginkgo.By("checking PV's zone label and node affinity terms match expected zone")
if pv == nil {
framework.Failf("nil pv passed")
}
pvLabel, ok := pv.Labels[v1.LabelFailureDomainBetaZone]
pvLabel, ok := pv.Labels[v1.LabelTopologyZone]
if !ok {
framework.Failf("label %s not found on PV", v1.LabelFailureDomainBetaZone)
framework.Failf("label %s not found on PV", v1.LabelTopologyZone)
}

zonesFromLabel, err := volumehelpers.LabelZonesToSet(pvLabel)
if err != nil {
framework.Failf("unable to parse zone labels %s: %v", pvLabel, err)
}
if matchZones && !zonesFromLabel.Equal(zones) {
framework.Failf("value[s] of %s label for PV: %v does not match expected zone[s]: %v", v1.LabelFailureDomainBetaZone, zonesFromLabel, zones)
framework.Failf("value[s] of %s label for PV: %v does not match expected zone[s]: %v", v1.LabelTopologyZone, zonesFromLabel, zones)
}
if !matchZones && !zonesFromLabel.IsSuperset(zones) {
framework.Failf("value[s] of %s label for PV: %v does not contain expected zone[s]: %v", v1.LabelFailureDomainBetaZone, zonesFromLabel, zones)
framework.Failf("value[s] of %s label for PV: %v does not contain expected zone[s]: %v", v1.LabelTopologyZone, zonesFromLabel, zones)
}
if pv.Spec.NodeAffinity == nil {
framework.Failf("node affinity not found in PV spec %v", pv.Spec)
Expand All @@ -618,7 +618,7 @@ func checkZonesFromLabelAndAffinity(pv *v1.PersistentVolume, zones sets.String,
for _, term := range pv.Spec.NodeAffinity.Required.NodeSelectorTerms {
keyFound := false
for _, r := range term.MatchExpressions {
if r.Key != v1.LabelFailureDomainBetaZone {
if r.Key != v1.LabelTopologyZone {
continue
}
keyFound = true
Expand All @@ -632,7 +632,7 @@ func checkZonesFromLabelAndAffinity(pv *v1.PersistentVolume, zones sets.String,
break
}
if !keyFound {
framework.Failf("label %s not found in term %v", v1.LabelFailureDomainBetaZone, term)
framework.Failf("label %s not found in term %v", v1.LabelTopologyZone, term)
}
}
}
Expand Down