Skip to content

Commit 8c514c6

Browse files
divyenpateldivyenpatel
authored
Merge pull request #3714 from divyenpatel/fix-isDatastoreAccessibleToAZClusters-for-multi-cluster-per-zone
fix isDatastoreAccessibleToAZClusters for multiple clusters per zone fix getSharedDatastoresInClusters to return union of datastores fully accessible to any of the supplied clusters
2 parents 870b1d4 + 78aa262 commit 8c514c6

File tree

3 files changed

+227
-36
lines changed

3 files changed

+227
-36
lines changed

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

Lines changed: 22 additions & 23 deletions
Original file line numberDiff line numberDiff line change
@@ -1587,42 +1587,41 @@ func (volTopology *wcpControllerVolumeTopology) GetTopologyInfoFromNodes(ctx con
15871587
return topologySegments, nil
15881588
}
15891589

1590-
// getSharedDatastoresInClusters helps find shared datastores accessible to all given clusters
1590+
// getSharedDatastoresInClusters returns the union of datastores accessible to any of the supplied clusters.
15911591
func getSharedDatastoresInClusters(ctx context.Context, clusterMorefs []string,
15921592
vc *cnsvsphere.VirtualCenter) ([]*cnsvsphere.DatastoreInfo, error) {
15931593
log := logger.GetLogger(ctx)
1594-
var sharedDatastoresForclusterMorefs []*cnsvsphere.DatastoreInfo
1595-
for index, clusterMoref := range clusterMorefs {
1594+
dsMap := make(map[string]*cnsvsphere.DatastoreInfo)
1595+
1596+
for _, clusterMoref := range clusterMorefs {
15961597
accessibleDs, _, err := cnsvsphere.GetCandidateDatastoresInCluster(ctx, vc, clusterMoref, false)
15971598
if err != nil {
15981599
return nil, logger.LogNewErrorf(log,
15991600
"failed to find candidate datastores to place volume in cluster %q. Error: %v",
16001601
clusterMoref, err)
16011602
}
16021603
if len(accessibleDs) == 0 {
1603-
return nil, logger.LogNewErrorf(log,
1604-
"no accessibleDs candidate datastores found to place volume for cluster %v", clusterMoref)
1604+
log.Infof("no accessible candidate datastores found for cluster %q", clusterMoref)
1605+
continue
16051606
}
1606-
if index == 0 {
1607-
sharedDatastoresForclusterMorefs = append(sharedDatastoresForclusterMorefs, accessibleDs...)
1608-
} else {
1609-
var sharedAccessibleDatastores []*cnsvsphere.DatastoreInfo
1610-
for _, sharedDatastore := range sharedDatastoresForclusterMorefs {
1611-
for _, accessibleDsInCluster := range accessibleDs {
1612-
if sharedDatastore.Info.Url == accessibleDsInCluster.Info.Url {
1613-
sharedAccessibleDatastores = append(sharedAccessibleDatastores, accessibleDsInCluster)
1614-
break
1615-
}
1616-
}
1617-
}
1618-
if len(sharedAccessibleDatastores) == 0 {
1619-
return nil, logger.LogNewErrorf(log,
1620-
"no shared candidate datastores found to place volume for clusters %v", clusterMorefs)
1621-
}
1622-
sharedDatastoresForclusterMorefs = sharedAccessibleDatastores
1607+
1608+
for _, ds := range accessibleDs {
1609+
dsMap[ds.Info.Url] = ds
16231610
}
16241611
}
1625-
return sharedDatastoresForclusterMorefs, nil
1612+
1613+
if len(dsMap) == 0 {
1614+
return nil, logger.LogNewErrorf(log,
1615+
"no shared candidate datastores found to place volume for clusters %v", clusterMorefs)
1616+
}
1617+
1618+
// Convert map to slice
1619+
var allSharedDatastores []*cnsvsphere.DatastoreInfo
1620+
for _, ds := range dsMap {
1621+
allSharedDatastores = append(allSharedDatastores, ds)
1622+
}
1623+
1624+
return allSharedDatastores, nil
16261625
}
16271626

16281627
// StartZonesInformer watches on changes to Zone instances.

‎pkg/syncer/cnsoperator/controller/cnsregistervolume/util.go‎

Lines changed: 1 addition & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -83,30 +83,18 @@ func isDatastoreAccessibleToAZClusters(ctx context.Context, vc *vsphere.VirtualC
8383
azClustersMap map[string][]string, datastoreURL string) bool {
8484
log := logger.GetLogger(ctx)
8585
for _, clusterIDs := range azClustersMap {
86-
var found bool
8786
for _, clusterID := range clusterIDs {
8887
sharedDatastores, _, err := vsphere.GetCandidateDatastoresInCluster(ctx, vc, clusterID, false)
8988
if err != nil {
9089
log.Warnf("Failed to get candidate datastores for cluster: %s with err: %+v", clusterID, err)
9190
continue
9291
}
93-
found = false
9492
for _, ds := range sharedDatastores {
9593
if ds.Info.Url == datastoreURL {
9694
log.Infof("Found datastoreUrl: %s is accessible to cluster: %s", datastoreURL, clusterID)
97-
found = true
95+
return true
9896
}
9997
}
100-
// If datastoreURL was found in the list of datastores accessible to the
101-
// cluster with clusterID, continue checking for the rest of the clusters
102-
// in AZ. Otherwise, break and check the next AZ in azClustersMap.
103-
if !found {
104-
break
105-
}
106-
}
107-
// datastoreURL was found in all the clusters with clusterIDs.
108-
if found {
109-
return true
11098
}
11199
}
112100
return false

‎pkg/syncer/cnsoperator/controller/cnsregistervolume/util_test.go‎

Lines changed: 204 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,204 @@
1+
/*
2+
Copyright 2025 The Kubernetes Authors.
3+
4+
Licensed under the Apache License, Version 2.0 (the "License");
5+
you may not use this file except in compliance with the License.
6+
You may obtain a copy of the License at
7+
8+
http://www.apache.org/licenses/LICENSE-2.0
9+
10+
Unless required by applicable law or agreed to in writing, software
11+
distributed under the License is distributed on an "AS IS" BASIS,
12+
WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
13+
See the License for the specific language governing permissions and
14+
limitations under the License.
15+
*/
16+
17+
package cnsregistervolume
18+
19+
import (
20+
"context"
21+
"fmt"
22+
"testing"
23+
"time"
24+
25+
"github.com/agiledragon/gomonkey/v2"
26+
"github.com/stretchr/testify/assert"
27+
"github.com/vmware/govmomi/vim25/types"
28+
apitypes "k8s.io/apimachinery/pkg/types"
29+
cnsvsphere "sigs.k8s.io/vsphere-csi-driver/v3/pkg/common/cns-lib/vsphere"
30+
)
31+
32+
// TestIsDatastoreAccessibleToAZClusters tests the isDatastoreAccessibleToAZClusters function
33+
// using standard Go testing framework to avoid conflicts with existing Ginkgo suites
34+
func TestIsDatastoreAccessibleToAZClusters(t *testing.T) {
35+
// Initialize backOffDuration map to prevent nil map assignment panic
36+
backOffDuration = make(map[apitypes.NamespacedName]time.Duration)
37+
38+
ctx := context.Background()
39+
mockVC := &cnsvsphere.VirtualCenter{}
40+
datastoreURL := "ds:///vmfs/volumes/test-datastore"
41+
42+
t.Run("1 cluster per zone - datastore accessible to 1 cluster", func(t *testing.T) {
43+
azClustersMap := map[string][]string{
44+
"zone-a": {"cluster-a1"},
45+
"zone-b": {"cluster-b1"},
46+
}
47+
48+
patches := gomonkey.ApplyFunc(cnsvsphere.GetCandidateDatastoresInCluster,
49+
func(ctx context.Context, vc *cnsvsphere.VirtualCenter, clusterID string,
50+
includevSANDirectDatastores bool) ([]*cnsvsphere.DatastoreInfo, []*cnsvsphere.DatastoreInfo, error) {
51+
if clusterID == "cluster-a1" {
52+
return []*cnsvsphere.DatastoreInfo{
53+
{
54+
Info: &types.DatastoreInfo{
55+
Url: datastoreURL,
56+
},
57+
},
58+
}, []*cnsvsphere.DatastoreInfo{}, nil
59+
}
60+
return []*cnsvsphere.DatastoreInfo{}, []*cnsvsphere.DatastoreInfo{}, nil
61+
})
62+
defer patches.Reset()
63+
64+
result := isDatastoreAccessibleToAZClusters(ctx, mockVC, azClustersMap, datastoreURL)
65+
assert.True(t, result)
66+
})
67+
68+
t.Run("2 clusters per zone - datastore accessible to all clusters", func(t *testing.T) {
69+
azClustersMap := map[string][]string{
70+
"zone-a": {"cluster-a1", "cluster-a2"},
71+
"zone-b": {"cluster-b1", "cluster-b2"},
72+
}
73+
74+
patches := gomonkey.ApplyFunc(cnsvsphere.GetCandidateDatastoresInCluster,
75+
func(ctx context.Context, vc *cnsvsphere.VirtualCenter, clusterID string,
76+
includevSANDirectDatastores bool) ([]*cnsvsphere.DatastoreInfo, []*cnsvsphere.DatastoreInfo, error) {
77+
return []*cnsvsphere.DatastoreInfo{
78+
{
79+
Info: &types.DatastoreInfo{
80+
Url: datastoreURL,
81+
},
82+
},
83+
}, []*cnsvsphere.DatastoreInfo{}, nil
84+
})
85+
defer patches.Reset()
86+
87+
result := isDatastoreAccessibleToAZClusters(ctx, mockVC, azClustersMap, datastoreURL)
88+
assert.True(t, result)
89+
})
90+
91+
t.Run("2 clusters per zone - datastore accessible to only 1 cluster", func(t *testing.T) {
92+
azClustersMap := map[string][]string{
93+
"zone-a": {"cluster-a1", "cluster-a2"},
94+
"zone-b": {"cluster-b1", "cluster-b2"},
95+
}
96+
97+
patches := gomonkey.ApplyFunc(cnsvsphere.GetCandidateDatastoresInCluster,
98+
func(ctx context.Context, vc *cnsvsphere.VirtualCenter, clusterID string,
99+
includevSANDirectDatastores bool) ([]*cnsvsphere.DatastoreInfo, []*cnsvsphere.DatastoreInfo, error) {
100+
if clusterID == "cluster-a1" {
101+
return []*cnsvsphere.DatastoreInfo{
102+
{
103+
Info: &types.DatastoreInfo{
104+
Url: datastoreURL,
105+
},
106+
},
107+
}, []*cnsvsphere.DatastoreInfo{}, nil
108+
}
109+
return []*cnsvsphere.DatastoreInfo{}, []*cnsvsphere.DatastoreInfo{}, nil
110+
})
111+
defer patches.Reset()
112+
113+
result := isDatastoreAccessibleToAZClusters(ctx, mockVC, azClustersMap, datastoreURL)
114+
assert.True(t, result)
115+
})
116+
117+
t.Run("2 clusters per zone - datastore accessible to only 1 cluster in different zone",
118+
func(t *testing.T) {
119+
azClustersMap := map[string][]string{
120+
"zone-a": {"cluster-a1", "cluster-a2"},
121+
"zone-b": {"cluster-b1", "cluster-b2"},
122+
}
123+
124+
patches := gomonkey.ApplyFunc(cnsvsphere.GetCandidateDatastoresInCluster,
125+
func(ctx context.Context, vc *cnsvsphere.VirtualCenter, clusterID string,
126+
includevSANDirectDatastores bool) ([]*cnsvsphere.DatastoreInfo, []*cnsvsphere.DatastoreInfo, error) {
127+
if clusterID == "cluster-b2" {
128+
return []*cnsvsphere.DatastoreInfo{
129+
{
130+
Info: &types.DatastoreInfo{
131+
Url: datastoreURL,
132+
},
133+
},
134+
}, []*cnsvsphere.DatastoreInfo{}, nil
135+
}
136+
return []*cnsvsphere.DatastoreInfo{}, []*cnsvsphere.DatastoreInfo{}, nil
137+
})
138+
defer patches.Reset()
139+
140+
result := isDatastoreAccessibleToAZClusters(ctx, mockVC, azClustersMap, datastoreURL)
141+
assert.True(t, result)
142+
})
143+
144+
t.Run("datastore not accessible to any cluster", func(t *testing.T) {
145+
azClustersMap := map[string][]string{
146+
"zone-a": {"cluster-a1"},
147+
"zone-b": {"cluster-b1"},
148+
}
149+
150+
patches := gomonkey.ApplyFunc(cnsvsphere.GetCandidateDatastoresInCluster,
151+
func(ctx context.Context, vc *cnsvsphere.VirtualCenter, clusterID string,
152+
includevSANDirectDatastores bool) ([]*cnsvsphere.DatastoreInfo, []*cnsvsphere.DatastoreInfo, error) {
153+
return []*cnsvsphere.DatastoreInfo{}, []*cnsvsphere.DatastoreInfo{}, nil
154+
})
155+
defer patches.Reset()
156+
157+
result := isDatastoreAccessibleToAZClusters(ctx, mockVC, azClustersMap, datastoreURL)
158+
assert.False(t, result)
159+
})
160+
161+
t.Run("error handling - should continue processing other clusters", func(t *testing.T) {
162+
azClustersMap := map[string][]string{
163+
"zone-a": {"cluster-a1"},
164+
"zone-b": {"cluster-b1"},
165+
}
166+
167+
patches := gomonkey.ApplyFunc(cnsvsphere.GetCandidateDatastoresInCluster,
168+
func(ctx context.Context, vc *cnsvsphere.VirtualCenter, clusterID string,
169+
includevSANDirectDatastores bool) ([]*cnsvsphere.DatastoreInfo, []*cnsvsphere.DatastoreInfo, error) {
170+
if clusterID == "cluster-a1" {
171+
return nil, nil, fmt.Errorf("failed to get datastores for cluster-a1")
172+
}
173+
if clusterID == "cluster-b1" {
174+
return []*cnsvsphere.DatastoreInfo{
175+
{
176+
Info: &types.DatastoreInfo{
177+
Url: datastoreURL,
178+
},
179+
},
180+
}, []*cnsvsphere.DatastoreInfo{}, nil
181+
}
182+
return []*cnsvsphere.DatastoreInfo{}, []*cnsvsphere.DatastoreInfo{}, nil
183+
})
184+
defer patches.Reset()
185+
186+
result := isDatastoreAccessibleToAZClusters(ctx, mockVC, azClustersMap, datastoreURL)
187+
assert.True(t, result)
188+
})
189+
190+
t.Run("empty azClustersMap", func(t *testing.T) {
191+
azClustersMap := map[string][]string{}
192+
result := isDatastoreAccessibleToAZClusters(ctx, mockVC, azClustersMap, datastoreURL)
193+
assert.False(t, result)
194+
})
195+
196+
t.Run("empty cluster lists in zones", func(t *testing.T) {
197+
azClustersMap := map[string][]string{
198+
"zone-a": {},
199+
"zone-b": {},
200+
}
201+
result := isDatastoreAccessibleToAZClusters(ctx, mockVC, azClustersMap, datastoreURL)
202+
assert.False(t, result)
203+
})
204+
}

0 commit comments

Comments
 (0)