fix GetActiveClustersForNamespaceInRequestedZones to return correct set of active clusters for the namespace (#3580)

Author: divyenpatel

pkg/csi/service/common/commonco/k8sorchestrator/topology.go

@@ -1727,9 +1727,14 @@ func (c *K8sOrchestrator) GetActiveClustersForNamespaceInRequestedZones(ctx cont
 		}
 		// check if zone belong to namespace specified with targetNS and belong to
 		// volume requirement specified with requestedZones
-		if zoneObjUnstructured.GetNamespace() != targetNS && !slices.Contains(requestedZones, zoneObjUnstructured.GetName()) {
-			log.Debugf("skipping zone: %q as it does not match requested targetNS: %q and requestedZones: %v requirement",
-				zoneObjUnstructured.GetName(), targetNS, requestedZones)
+		if zoneObjUnstructured.GetNamespace() != targetNS {
+			log.Debugf("skipping zone:%q as it is not assgiend to namespace: %q",
+				zoneObjUnstructured.GetName(), targetNS)
+			continue
+		}
+		if !slices.Contains(requestedZones, zoneObjUnstructured.GetName()) {
+			log.Debugf("skipping zone:%q as it does not belong to requestedZones: %v",
+				zoneObjUnstructured.GetName(), requestedZones)
 			continue
 		}
 		// capture active cluster on the namespace from zone CR instance
@@ -1743,6 +1748,13 @@ func (c *K8sOrchestrator) GetActiveClustersForNamespaceInRequestedZones(ctx cont
 			return nil, logger.LogNewErrorf(log, "clusterMoIDs not found in zone instance :%q",
 				zoneObj.(*unstructured.Unstructured).GetName())
 		}
+		log.Infof("zone name=%q ns=%q uid=%q rv=%q active clusters: %v",
+			zoneObjUnstructured.GetName(),
+			zoneObjUnstructured.GetNamespace(),
+			zoneObjUnstructured.GetUID(),
+			zoneObjUnstructured.GetResourceVersion(),
+			clusters,
+		)
 		activeClusters = append(activeClusters, clusters...)
 	}
 	if len(activeClusters) == 0 {

pkg/csi/service/common/commonco/k8sorchestrator/topology_test.go

@@ -1,10 +1,17 @@
 package k8sorchestrator
 
 import (
+	"context"
 	"encoding/json"
 	"testing"
+	"time"
 
+	. "github.com/onsi/ginkgo/v2"
+	. "github.com/onsi/gomega"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
+	"k8s.io/apimachinery/pkg/apis/meta/v1/unstructured"
+	"k8s.io/apimachinery/pkg/runtime/schema"
+	"k8s.io/client-go/tools/cache"
 	csinodetopologyv1alpha1 "sigs.k8s.io/vsphere-csi-driver/v3/pkg/internalapis/csinodetopology/v1alpha1"
 )
 
@@ -44,3 +51,124 @@ func TestPatchNodeTopology(t *testing.T) {
 		t.Errorf("ResourceVersion should be %s, got %s", csiNodeTopology.ResourceVersion, resourceVersion)
 	}
 }
+
+// ---------------------- Fake Informer ----------------------
+type fakeInformer struct {
+	cache.SharedIndexInformer
+	store cache.Store
+}
+
+func (f *fakeInformer) GetStore() cache.Store {
+	return f.store
+}
+
+// ---------------------- Test Helpers ----------------------
+func makeZone(name, ns string, deleted bool, clusterMoIDs []string, causeNestedError bool) *unstructured.Unstructured {
+	zone := &unstructured.Unstructured{}
+	zone.SetGroupVersionKind(schema.GroupVersionKind{
+		Group:   "cns.vmware.com",
+		Version: "v1alpha1",
+		Kind:    "Zone",
+	})
+	zone.SetName(name)
+	zone.SetNamespace(ns)
+
+	if deleted {
+		now := metav1.NewTime(time.Now())
+		zone.SetDeletionTimestamp(&now)
+	}
+
+	if causeNestedError {
+		zone.Object["spec"] = "not-a-map"
+	} else if clusterMoIDs != nil {
+		_ = unstructured.SetNestedStringSlice(zone.Object, clusterMoIDs, "spec", "namespace", "clusterMoIDs")
+	}
+
+	return zone
+}
+
+func setupFakeStore(zones ...*unstructured.Unstructured) cache.Store {
+	store := cache.NewStore(cache.DeletionHandlingMetaNamespaceKeyFunc)
+	for _, z := range zones {
+		_ = store.Add(z)
+	}
+	return store
+}
+
+// ---------------------- Tests ----------------------
+var _ = Describe("GetActiveClustersForNamespaceInRequestedZones", func() {
+	var (
+		ctx  context.Context
+		orch *K8sOrchestrator
+	)
+
+	BeforeEach(func() {
+		ctx = context.Background()
+		orch = &K8sOrchestrator{}
+	})
+
+	It("returns clusters when valid zones exist", func() {
+		z1 := makeZone("zone-A", "ns1", false, []string{"cluster-1"}, false)
+		zoneInformer = &fakeInformer{store: setupFakeStore(z1)}
+
+		clusters, err := orch.GetActiveClustersForNamespaceInRequestedZones(ctx, "ns1", []string{"zone-A"})
+		Expect(err).NotTo(HaveOccurred())
+		Expect(clusters).To(ConsistOf("cluster-1"))
+	})
+
+	It("skips zones with deletion timestamp", func() {
+		z := makeZone("zone-A", "ns1", true, []string{"cluster-1"}, false)
+		zoneInformer = &fakeInformer{store: setupFakeStore(z)}
+
+		clusters, err := orch.GetActiveClustersForNamespaceInRequestedZones(ctx, "ns1", []string{"zone-A"})
+		Expect(err).To(HaveOccurred())
+		Expect(err.Error()).To(ContainSubstring("could not find active cluster"))
+		Expect(clusters).To(BeNil())
+	})
+
+	It("skips zones not in requestedZones", func() {
+		z := makeZone("zone-A", "ns1", false, []string{"cluster-1"}, false)
+		zoneInformer = &fakeInformer{store: setupFakeStore(z)}
+
+		clusters, err := orch.GetActiveClustersForNamespaceInRequestedZones(ctx, "ns1", []string{"zone-B"})
+		Expect(err).To(HaveOccurred())
+		Expect(err.Error()).To(ContainSubstring("could not find active cluster"))
+		Expect(clusters).To(BeNil())
+	})
+
+	It("returns error when clusterMoIDs not found", func() {
+		z := makeZone("zone-A", "ns1", false, nil, false)
+		zoneInformer = &fakeInformer{store: setupFakeStore(z)}
+
+		clusters, err := orch.GetActiveClustersForNamespaceInRequestedZones(ctx, "ns1", []string{"zone-A"})
+		Expect(err).To(HaveOccurred())
+		Expect(err.Error()).To(ContainSubstring("clusterMoIDs not found"))
+		Expect(clusters).To(BeNil())
+	})
+
+	It("returns error when NestedStringSlice returns error", func() {
+		z := makeZone("zone-A", "ns1", false, nil, true)
+		zoneInformer = &fakeInformer{store: setupFakeStore(z)}
+
+		clusters, err := orch.GetActiveClustersForNamespaceInRequestedZones(ctx, "ns1", []string{"zone-A"})
+		Expect(err).To(HaveOccurred())
+		Expect(err.Error()).To(ContainSubstring("failed to get clusterMoIDs"))
+		Expect(clusters).To(BeNil())
+	})
+
+	It("aggregates clusters from multiple zones", func() {
+		z1 := makeZone("zone-A", "ns1", false, []string{"cluster-1"}, false)
+		z2 := makeZone("zone-B", "ns1", false, []string{"cluster-2", "cluster-3"}, false)
+		zoneInformer = &fakeInformer{store: setupFakeStore(z1, z2)}
+
+		clusters, err := orch.GetActiveClustersForNamespaceInRequestedZones(ctx, "ns1", []string{"zone-A", "zone-B"})
+		Expect(err).NotTo(HaveOccurred())
+		Expect(clusters).To(ConsistOf("cluster-1", "cluster-2", "cluster-3"))
+	})
+})
+
+// ---------------------- Ginkgo Entry ----------------------
+func TestGetActiveClusters_Ginkgo(t *testing.T) {
+	RegisterFailHandler(Fail)
+	RunSpecs(t, "K8sOrchestrator GetActiveClusters Suite")
+}