From c22f5f36cb4c7b85fdb910288f121bb9f3913d19 Mon Sep 17 00:00:00 2001 From: parth-gr Date: Thu, 2 Mar 2023 20:02:41 +0530 Subject: [PATCH 1/2] osd: validate and remove duplicate topology remove the duplicate topology added to the node, and the priority to keep duplicate would be give to the one who is lowest in the hierarchy Closes: https://github.com/rook/rook/issues/11800 Signed-off-by: parth-gr --- .../ceph/cluster/osd/topology/topology.go | 96 ++++++++++--------- .../cluster/osd/topology/topology_test.go | 12 ++- 2 files changed, 59 insertions(+), 49 deletions(-) diff --git a/pkg/operator/ceph/cluster/osd/topology/topology.go b/pkg/operator/ceph/cluster/osd/topology/topology.go index c1128b951..e82d9b6ed 100644 --- a/pkg/operator/ceph/cluster/osd/topology/topology.go +++ b/pkg/operator/ceph/cluster/osd/topology/topology.go @@ -43,6 +43,7 @@ var ( const ( topologyLabelPrefix = "topology.rook.io/" + labelHostname = "kubernetes.io/hostname" ) // ExtractTopologyFromLabels extracts rook topology from labels and returns a map from topology type to value @@ -56,6 +57,34 @@ func ExtractOSDTopologyFromLabels(labels map[string]string) (map[string]string, return topology, topologyAffinity } +func rookTopologyLabelsOrdered() []string { + topologyLabelsOrdered := []string{} + for i := len(CRUSHTopologyLabels) - 1; i >= 0; i-- { + label := CRUSHTopologyLabels[i] + topologyLabelsOrdered = append(topologyLabelsOrdered, topologyLabelPrefix+label) + } + return topologyLabelsOrdered +} + +func allKubernetesTopologyLabelsOrdered() []string { + return append([]string{corev1.LabelFailureDomainBetaRegion, // deprecated label for backwards compatibility + corev1.LabelTopologyRegion, + corev1.LabelZoneFailureDomain, // deprecated label for backwards compatibility, + corev1.LabelTopologyZone, + labelHostname}, + rookTopologyLabelsOrdered()..., + ) +} + +func kubernetesTopologyLabelToCRUSHLabel(label string) string { + crushLabel := strings.Split(label, "/") + if crushLabel[len(crushLabel)-1] == "hostname" { + // kubernetes uses "kubernetes.io/hostname" whereas CRUSH uses "host" + return "host" + } + return crushLabel[len(crushLabel)-1] +} + // ExtractTopologyFromLabels extracts rook topology from labels and returns a map from topology type to value func extractTopologyFromLabels(labels map[string]string) (map[string]string, string) { topology := make(map[string]string) @@ -63,59 +92,34 @@ func extractTopologyFromLabels(labels map[string]string) (map[string]string, str // The topology affinity for the osd is the lowest topology label found in the hierarchy, // not including the host name var topologyAffinity string - - // check for the region k8s topology label that was deprecated in 1.17 - const regionLabel = "region" - region, ok := labels[corev1.LabelZoneRegion] - if ok { - topology[regionLabel] = region - topologyAffinity = formatTopologyAffinity(corev1.LabelZoneRegion, region) - } - - // check for the region k8s topology label that is GA in 1.17. - region, ok = labels[corev1.LabelZoneRegionStable] - if ok { - topology[regionLabel] = region - topologyAffinity = formatTopologyAffinity(corev1.LabelZoneRegionStable, region) - } - - // check for the zone k8s topology label that was deprecated in 1.17 - const zoneLabel = "zone" - zone, ok := labels[corev1.LabelZoneFailureDomain] - if ok { - topology[zoneLabel] = zone - topologyAffinity = formatTopologyAffinity(corev1.LabelZoneFailureDomain, zone) - } - - // check for the zone k8s topology label that is GA in 1.17. - zone, ok = labels[corev1.LabelZoneFailureDomainStable] - if ok { - topology[zoneLabel] = zone - topologyAffinity = formatTopologyAffinity(corev1.LabelZoneFailureDomainStable, zone) - } - - // ignore the region k8s topology label when it has the same value as the k8s zone label - if topology[regionLabel] == topology[zoneLabel] { - delete(topology, regionLabel) - } - - // get host - host, ok := labels[corev1.LabelHostname] - if ok { - topology["host"] = host - } + allKubernetesTopologyLabels := allKubernetesTopologyLabelsOrdered() // get the labels for the CRUSH map hierarchy - // iterate in reverse order so that the last topology found will be the lowest level in the hierarchy + // iterate in a way so the last topology found will be the lowest level in the hierarchy // for the topology affinity - for i := len(CRUSHTopologyLabels) - 1; i >= 0; i-- { - topologyID := CRUSHTopologyLabels[i] - label := topologyLabelPrefix + topologyID + for _, label := range allKubernetesTopologyLabels { + topologyID := kubernetesTopologyLabelToCRUSHLabel(label) if value, ok := labels[label]; ok { topology[topologyID] = value - topologyAffinity = formatTopologyAffinity(label, value) + if topologyID != "host" { + topologyAffinity = formatTopologyAffinity(label, value) + } } } + // iterate in lowest to highest order as the lowest level should be sustained and higher level duplicate + // should be removed + duplicateTopology := make(map[string]int) + for i := len(allKubernetesTopologyLabels) - 1; i >= 0; i-- { + topologyLabel := allKubernetesTopologyLabels[i] + if value, ok := labels[topologyLabel]; ok { + if _, ok := duplicateTopology[value]; ok { + delete(topology, kubernetesTopologyLabelToCRUSHLabel(topologyLabel)) + } else { + duplicateTopology[value] = 1 + } + } + } + return topology, topologyAffinity } diff --git a/pkg/operator/ceph/cluster/osd/topology/topology_test.go b/pkg/operator/ceph/cluster/osd/topology/topology_test.go index 720414f15..882f04aa1 100644 --- a/pkg/operator/ceph/cluster/osd/topology/topology_test.go +++ b/pkg/operator/ceph/cluster/osd/topology/topology_test.go @@ -48,16 +48,22 @@ func TestCleanTopologyLabels(t *testing.T) { "topology.rook.io/rack": "r.rack", "topology.rook.io/row": "r.row", "topology.rook.io/datacenter": "d.datacenter", - } + "topology.rook.io/room": "test", + "topology.rook.io/chassis": "test", + "topology.rook.io/pod": "test"} topology, affinity := ExtractOSDTopologyFromLabels(nodeLabels) - assert.Equal(t, 6, len(topology)) + assert.Equal(t, 7, len(topology)) assert.Equal(t, "r-region", topology["region"]) assert.Equal(t, "z-zone", topology["zone"]) assert.Equal(t, "host-name", topology["host"]) assert.Equal(t, "r-rack", topology["rack"]) assert.Equal(t, "r-row", topology["row"]) assert.Equal(t, "d-datacenter", topology["datacenter"]) - assert.Equal(t, "topology.rook.io/rack=r.rack", affinity) + assert.Equal(t, "topology.rook.io/chassis=test", affinity) + assert.Equal(t, "test", topology["chassis"]) + assert.Equal(t, "", topology["pod"]) + assert.Equal(t, "", topology["room"]) + } func TestTopologyLabels(t *testing.T) { From 89764f03715d8fa9f383ea25c7407663759b13df Mon Sep 17 00:00:00 2001 From: parth-gr Date: Mon, 13 Mar 2023 19:47:23 +0530 Subject: [PATCH 2/2] osd: remove deprecated k8s topology label Signed-off-by: parth-gr --- pkg/operator/ceph/cluster/osd/osd_test.go | 12 ++-- .../ceph/cluster/osd/topology/topology.go | 11 ++- .../cluster/osd/topology/topology_test.go | 68 ++++--------------- 3 files changed, 26 insertions(+), 65 deletions(-) diff --git a/pkg/operator/ceph/cluster/osd/osd_test.go b/pkg/operator/ceph/cluster/osd/osd_test.go index bcaa024cf..2e8bedf16 100644 --- a/pkg/operator/ceph/cluster/osd/osd_test.go +++ b/pkg/operator/ceph/cluster/osd/osd_test.go @@ -570,18 +570,18 @@ func TestDetectCrushLocation(t *testing.T) { // update the location with valid topology labels nodeLabels = map[string]string{ - "failure-domain.beta.kubernetes.io/region": "region1", - "failure-domain.beta.kubernetes.io/zone": "zone1", - "topology.rook.io/rack": "rack1", - "topology.rook.io/row": "row1", + "topology.kubernetes.io/region": "region1", + "topology.kubernetes.io/zone": "zone", + "topology.rook.io/rack": "rack1", + "topology.rook.io/row": "row1", } - + // sorted in alphabetical order expected := []string{ "host=foo", "rack=rack1", "region=region1", "row=row1", - "zone=zone1", + "zone=zone", } updateLocationWithNodeLabels(&location, nodeLabels) diff --git a/pkg/operator/ceph/cluster/osd/topology/topology.go b/pkg/operator/ceph/cluster/osd/topology/topology.go index e82d9b6ed..ab25bc173 100644 --- a/pkg/operator/ceph/cluster/osd/topology/topology.go +++ b/pkg/operator/ceph/cluster/osd/topology/topology.go @@ -67,12 +67,11 @@ func rookTopologyLabelsOrdered() []string { } func allKubernetesTopologyLabelsOrdered() []string { - return append([]string{corev1.LabelFailureDomainBetaRegion, // deprecated label for backwards compatibility - corev1.LabelTopologyRegion, - corev1.LabelZoneFailureDomain, // deprecated label for backwards compatibility, - corev1.LabelTopologyZone, - labelHostname}, - rookTopologyLabelsOrdered()..., + return append( + append([]string{corev1.LabelTopologyRegion, + corev1.LabelTopologyZone}, + rookTopologyLabelsOrdered()...), + labelHostname, // host is the lowest level in the crush map hierarchy ) } diff --git a/pkg/operator/ceph/cluster/osd/topology/topology_test.go b/pkg/operator/ceph/cluster/osd/topology/topology_test.go index 882f04aa1..33a5c4cdd 100644 --- a/pkg/operator/ceph/cluster/osd/topology/topology_test.go +++ b/pkg/operator/ceph/cluster/osd/topology/topology_test.go @@ -42,19 +42,17 @@ func TestOrderedCRUSHLabels(t *testing.T) { func TestCleanTopologyLabels(t *testing.T) { // load all the expected labels nodeLabels := map[string]string{ - corev1.LabelZoneRegionStable: "r.region", - corev1.LabelZoneFailureDomainStable: "z.zone", - "kubernetes.io/hostname": "host.name", - "topology.rook.io/rack": "r.rack", - "topology.rook.io/row": "r.row", - "topology.rook.io/datacenter": "d.datacenter", - "topology.rook.io/room": "test", - "topology.rook.io/chassis": "test", - "topology.rook.io/pod": "test"} + corev1.LabelZoneRegionStable: "r.region", + "kubernetes.io/hostname": "host.name", + "topology.rook.io/rack": "r.rack", + "topology.rook.io/row": "r.row", + "topology.rook.io/datacenter": "d.datacenter", + "topology.rook.io/room": "test", + "topology.rook.io/chassis": "test", + "topology.rook.io/pod": "test"} topology, affinity := ExtractOSDTopologyFromLabels(nodeLabels) - assert.Equal(t, 7, len(topology)) + assert.Equal(t, 6, len(topology)) assert.Equal(t, "r-region", topology["region"]) - assert.Equal(t, "z-zone", topology["zone"]) assert.Equal(t, "host-name", topology["host"]) assert.Equal(t, "r-rack", topology["rack"]) assert.Equal(t, "r-row", topology["row"]) @@ -92,47 +90,21 @@ func TestTopologyLabels(t *testing.T) { // load all the expected labels nodeLabels = map[string]string{ - corev1.LabelZoneRegionStable: "r1", - corev1.LabelZoneFailureDomainStable: "z1", - "kubernetes.io/hostname": "myhost", - "topology.rook.io/rack": "rack1", - "topology.rook.io/row": "row1", - "topology.rook.io/datacenter": "d1", + corev1.LabelZoneRegionStable: "r1", + "kubernetes.io/hostname": "myhost", + "topology.rook.io/rack": "rack1", + "topology.rook.io/row": "row1", + "topology.rook.io/datacenter": "d1", } topology, affinity = extractTopologyFromLabels(nodeLabels) - assert.Equal(t, 6, len(topology)) + assert.Equal(t, 5, len(topology)) assert.Equal(t, "r1", topology["region"]) - assert.Equal(t, "z1", topology["zone"]) assert.Equal(t, "myhost", topology["host"]) assert.Equal(t, "rack1", topology["rack"]) assert.Equal(t, "row1", topology["row"]) assert.Equal(t, "d1", topology["datacenter"]) assert.Equal(t, "topology.rook.io/rack=rack1", affinity) - // ensure deprecated k8s labels are loaded - nodeLabels = map[string]string{ - corev1.LabelZoneRegion: "r1", - corev1.LabelZoneFailureDomain: "z1", - } - topology, affinity = extractTopologyFromLabels(nodeLabels) - assert.Equal(t, 2, len(topology)) - assert.Equal(t, "r1", topology["region"]) - assert.Equal(t, "z1", topology["zone"]) - assert.Equal(t, "failure-domain.beta.kubernetes.io/zone=z1", affinity) - - // ensure deprecated k8s labels are overridden - nodeLabels = map[string]string{ - corev1.LabelZoneRegionStable: "r1", - corev1.LabelZoneFailureDomainStable: "z1", - corev1.LabelZoneRegion: "oldregion", - corev1.LabelZoneFailureDomain: "oldzone", - } - topology, affinity = extractTopologyFromLabels(nodeLabels) - assert.Equal(t, 2, len(topology)) - assert.Equal(t, "r1", topology["region"]) - assert.Equal(t, "z1", topology["zone"]) - assert.Equal(t, "topology.kubernetes.io/zone=z1", affinity) - // invalid labels under topology.rook.io return an error nodeLabels = map[string]string{ "topology.rook.io/row/bad": "r1", @@ -140,16 +112,6 @@ func TestTopologyLabels(t *testing.T) { topology, affinity = extractTopologyFromLabels(nodeLabels) assert.Equal(t, 0, len(topology)) assert.Equal(t, "", affinity) - - // ignore the region k8s topology label when it has the same value as the k8s zone label - nodeLabels = map[string]string{ - corev1.LabelZoneRegionStable: "zone", - corev1.LabelZoneFailureDomainStable: "zone", - } - topology, affinity = extractTopologyFromLabels(nodeLabels) - assert.Equal(t, 1, len(topology)) - assert.Equal(t, "zone", topology["zone"]) - assert.Equal(t, "topology.kubernetes.io/zone=zone", affinity) } func TestGetDefaultTopologyLabels(t *testing.T) {