Skip to content

Commit 1ee16ce

Browse files
author
maxime.hubert
committed
fix(ovhcloud): fix default nodegroup lookup by switching to node labels only
1 parent dd0af01 commit 1ee16ce

File tree

5 files changed

+152
-151
lines changed

5 files changed

+152
-151
lines changed

‎cluster-autoscaler/cloudprovider/ovhcloud/ovh_cloud_manager.go‎

Lines changed: 26 additions & 26 deletions
Original file line numberDiff line numberDiff line change
@@ -62,9 +62,9 @@ type OvhCloudManager struct {
6262
ClusterID string
6363
ProjectID string
6464

65-
NodePoolsPerID map[string]*sdk.NodePool
66-
NodeGroupPerProviderID map[string]*NodeGroup
67-
NodeGroupPerProviderIDLock sync.RWMutex
65+
NodePoolsPerName map[string]*sdk.NodePool
66+
NodeGroupPerName map[string]*NodeGroup
67+
NodeGroupPerNameLock sync.RWMutex
6868

6969
FlavorsCache map[string]sdk.Flavor
7070
FlavorsCacheExpirationTime time.Time
@@ -162,9 +162,9 @@ func NewManager(configFile io.Reader) (*OvhCloudManager, error) {
162162
ProjectID: cfg.ProjectID,
163163
ClusterID: cfg.ClusterID,
164164

165-
NodePoolsPerID: make(map[string]*sdk.NodePool),
166-
NodeGroupPerProviderID: make(map[string]*NodeGroup),
167-
NodeGroupPerProviderIDLock: sync.RWMutex{},
165+
NodePoolsPerName: make(map[string]*sdk.NodePool),
166+
NodeGroupPerName: make(map[string]*NodeGroup),
167+
NodeGroupPerNameLock: sync.RWMutex{},
168168

169169
FlavorsCache: make(map[string]sdk.Flavor),
170170
FlavorsCacheExpirationTime: time.Time{},
@@ -209,20 +209,20 @@ func (m *OvhCloudManager) getFlavorByName(flavorName string) (sdk.Flavor, error)
209209
return sdk.Flavor{}, fmt.Errorf("flavor %s not found in available flavors", flavorName)
210210
}
211211

212-
// setNodeGroupPerProviderID stores the association provider ID => node group in cache for future reference
213-
func (m *OvhCloudManager) setNodeGroupPerProviderID(providerID string, nodeGroup *NodeGroup) {
214-
m.NodeGroupPerProviderIDLock.Lock()
215-
defer m.NodeGroupPerProviderIDLock.Unlock()
212+
// setNodeGroupPerName stores the node group in cache for future reference
213+
func (m *OvhCloudManager) setNodeGroupPerName(nodepoolName string, nodeGroup *NodeGroup) {
214+
m.NodeGroupPerNameLock.Lock()
215+
defer m.NodeGroupPerNameLock.Unlock()
216216

217-
m.NodeGroupPerProviderID[providerID] = nodeGroup
217+
m.NodeGroupPerName[nodepoolName] = nodeGroup
218218
}
219219

220-
// getNodeGroupPerProviderID gets from cache the node group associated to the given provider ID
221-
func (m *OvhCloudManager) getNodeGroupPerProviderID(providerID string) *NodeGroup {
222-
m.NodeGroupPerProviderIDLock.RLock()
223-
defer m.NodeGroupPerProviderIDLock.RUnlock()
220+
// GetNodeGroupPerName gets from cache the node group using its name
221+
func (m *OvhCloudManager) GetNodeGroupPerName(nodepoolName string) *NodeGroup {
222+
m.NodeGroupPerNameLock.RLock()
223+
defer m.NodeGroupPerNameLock.RUnlock()
224224

225-
return m.NodeGroupPerProviderID[providerID]
225+
return m.NodeGroupPerName[nodepoolName]
226226
}
227227

228228
// ReAuthenticate allows OpenStack keystone token to be revoked and re-created to call API
@@ -247,35 +247,35 @@ func (m *OvhCloudManager) ReAuthenticate() error {
247247
}
248248

249249
// setNodePoolsState updates nodepool local informations based on given list
250-
// Updates NodePoolsPerID by modifying data so the reference in NodeGroupPerProviderID can access refreshed data
250+
// Updates NodePoolsPerName by modifying data so the reference in NodeGroupPerName can access refreshed data
251251
//
252252
// - Updates fields on already referenced nodepool
253253
// - Adds nodepool if not referenced yet
254254
// - Deletes from map if nodepool is not in the given list (it doesn't exist anymore)
255255
func (m *OvhCloudManager) setNodePoolsState(pools []sdk.NodePool) {
256-
m.NodeGroupPerProviderIDLock.Lock()
257-
defer m.NodeGroupPerProviderIDLock.Unlock()
256+
m.NodeGroupPerNameLock.Lock()
257+
defer m.NodeGroupPerNameLock.Unlock()
258258

259-
poolIDsToKeep := []string{}
259+
poolNamesToKeep := []string{}
260260
for _, pool := range pools {
261-
poolIDsToKeep = append(poolIDsToKeep, pool.ID)
261+
poolNamesToKeep = append(poolNamesToKeep, pool.Name)
262262
}
263263

264264
// Update nodepools state
265265
for _, pool := range pools {
266-
poolRef, ok := m.NodePoolsPerID[pool.ID]
266+
poolRef, ok := m.NodePoolsPerName[pool.Name]
267267
if ok {
268268
*poolRef = pool // Update existing value
269269
} else {
270270
poolCopy := pool
271-
m.NodePoolsPerID[pool.ID] = &poolCopy
271+
m.NodePoolsPerName[pool.Name] = &poolCopy
272272
}
273273
}
274274

275275
// Remove nodepools that doesn't exist anymore
276-
for poolID := range m.NodePoolsPerID {
277-
if !slices.Contains(poolIDsToKeep, poolID) {
278-
delete(m.NodePoolsPerID, poolID)
276+
for poolName := range m.NodePoolsPerName {
277+
if !slices.Contains(poolNamesToKeep, poolName) {
278+
delete(m.NodePoolsPerName, poolName)
279279
}
280280
}
281281
}

‎cluster-autoscaler/cloudprovider/ovhcloud/ovh_cloud_manager_test.go‎

Lines changed: 50 additions & 52 deletions
Original file line numberDiff line numberDiff line change
@@ -192,14 +192,14 @@ func TestOvhCloudManager_getFlavorByName(t *testing.T) {
192192
})
193193
}
194194

195-
func TestOvhCloudManager_setNodeGroupPerProviderID(t *testing.T) {
195+
func TestOvhCloudManager_setNodeGroupPerName(t *testing.T) {
196196
manager := newTestManager(t)
197197
ng1 := NodeGroup{
198198
CurrentSize: 1,
199199
}
200200

201201
type fields struct {
202-
NodeGroupPerProviderID map[string]*NodeGroup
202+
NodeGroupPerName map[string]*NodeGroup
203203
}
204204
type args struct {
205205
providerID string
@@ -214,7 +214,7 @@ func TestOvhCloudManager_setNodeGroupPerProviderID(t *testing.T) {
214214
{
215215
name: "New entry",
216216
fields: fields{
217-
NodeGroupPerProviderID: map[string]*NodeGroup{},
217+
NodeGroupPerName: map[string]*NodeGroup{},
218218
},
219219
args: args{
220220
providerID: "providerID1",
@@ -226,7 +226,7 @@ func TestOvhCloudManager_setNodeGroupPerProviderID(t *testing.T) {
226226
}, {
227227
name: "Replace entry",
228228
fields: fields{
229-
NodeGroupPerProviderID: map[string]*NodeGroup{
229+
NodeGroupPerName: map[string]*NodeGroup{
230230
"providerID1": {},
231231
},
232232
},
@@ -241,23 +241,23 @@ func TestOvhCloudManager_setNodeGroupPerProviderID(t *testing.T) {
241241
}
242242
for _, tt := range tests {
243243
t.Run(tt.name, func(t *testing.T) {
244-
manager.NodeGroupPerProviderID = tt.fields.NodeGroupPerProviderID
244+
manager.NodeGroupPerName = tt.fields.NodeGroupPerName
245245

246-
manager.setNodeGroupPerProviderID(tt.args.providerID, tt.args.nodeGroup)
246+
manager.setNodeGroupPerName(tt.args.providerID, tt.args.nodeGroup)
247247

248-
assert.Equal(t, tt.wantCache, manager.NodeGroupPerProviderID)
248+
assert.Equal(t, tt.wantCache, manager.NodeGroupPerName)
249249
})
250250
}
251251
}
252252

253-
func TestOvhCloudManager_getNodeGroupPerProviderID(t *testing.T) {
253+
func TestOvhCloudManager_GetNodeGroupPerName(t *testing.T) {
254254
manager := newTestManager(t)
255255
ng1 := NodeGroup{
256256
CurrentSize: 1,
257257
}
258258

259259
type fields struct {
260-
NodeGroupPerProviderID map[string]*NodeGroup
260+
NodeGroupPerName map[string]*NodeGroup
261261
}
262262
type args struct {
263263
providerID string
@@ -271,7 +271,7 @@ func TestOvhCloudManager_getNodeGroupPerProviderID(t *testing.T) {
271271
{
272272
name: "Node group found",
273273
fields: fields{
274-
NodeGroupPerProviderID: map[string]*NodeGroup{
274+
NodeGroupPerName: map[string]*NodeGroup{
275275
"providerID1": &ng1,
276276
},
277277
},
@@ -283,7 +283,7 @@ func TestOvhCloudManager_getNodeGroupPerProviderID(t *testing.T) {
283283
{
284284
name: "Node group not found",
285285
fields: fields{
286-
NodeGroupPerProviderID: map[string]*NodeGroup{},
286+
NodeGroupPerName: map[string]*NodeGroup{},
287287
},
288288
args: args{
289289
providerID: "providerID1",
@@ -293,115 +293,113 @@ func TestOvhCloudManager_getNodeGroupPerProviderID(t *testing.T) {
293293
}
294294
for _, tt := range tests {
295295
t.Run(tt.name, func(t *testing.T) {
296-
manager.NodeGroupPerProviderID = tt.fields.NodeGroupPerProviderID
296+
manager.NodeGroupPerName = tt.fields.NodeGroupPerName
297297

298-
assert.Equalf(t, tt.want, manager.getNodeGroupPerProviderID(tt.args.providerID), "getNodeGroupPerProviderID(%v)", tt.args.providerID)
298+
assert.Equalf(t, tt.want, manager.GetNodeGroupPerName(tt.args.providerID), "GetNodeGroupPerName(%v)", tt.args.providerID)
299299
})
300300
}
301301
}
302302

303303
func TestOvhCloudManager_cacheConcurrency(t *testing.T) {
304304
manager := newTestManager(t)
305305

306-
t.Run("Check NodeGroupPerProviderID cache is safe for concurrency (needs to be run with -race)", func(t *testing.T) {
306+
t.Run("Check NodeGroupPerName cache is safe for concurrency (needs to be run with -race)", func(t *testing.T) {
307307
go func() {
308-
manager.setNodeGroupPerProviderID("", &NodeGroup{})
308+
manager.setNodeGroupPerName("", &NodeGroup{})
309309
}()
310-
manager.getNodeGroupPerProviderID("")
310+
manager.GetNodeGroupPerName("")
311311
})
312312
}
313313

314314
func TestOvhCloudManager_setNodePoolsState(t *testing.T) {
315315
manager := newTestManager(t)
316-
np1 := sdk.NodePool{ID: "np1", DesiredNodes: 1}
317-
np2 := sdk.NodePool{ID: "np2", DesiredNodes: 2}
318-
np3 := sdk.NodePool{ID: "np3", DesiredNodes: 3}
316+
np1 := sdk.NodePool{Name: "np1", DesiredNodes: 1}
317+
np2 := sdk.NodePool{Name: "np2", DesiredNodes: 2}
318+
np3 := sdk.NodePool{Name: "np3", DesiredNodes: 3}
319319

320320
type fields struct {
321-
NodePoolsPerID map[string]*sdk.NodePool
322-
NodeGroupPerProviderID map[string]*NodeGroup
321+
NodePoolsPerName map[string]*sdk.NodePool
322+
NodeGroupPerName map[string]*NodeGroup
323323
}
324324
type args struct {
325325
poolsList []sdk.NodePool
326326

327-
nodePoolsPerID map[string]*sdk.NodePool
328-
nodeGroupPerProviderID map[string]*NodeGroup
327+
NodePoolsPerName map[string]*sdk.NodePool
328+
NodeGroupPerName map[string]*NodeGroup
329329
}
330330
tests := []struct {
331-
name string
332-
fields fields
333-
args args
334-
wantNodePoolsPerID map[string]uint32 // ID => desired nodes
335-
wantNodeGroupPerProviderID map[string]uint32 // ID => desired nodes
331+
name string
332+
fields fields
333+
args args
334+
wantNodePoolsPerName map[string]uint32 // ID => desired nodes
335+
wantNodeGroupPerName map[string]uint32 // ID => desired nodes
336336
}{
337337
{
338-
name: "NodePoolsPerID and NodeGroupPerProviderID empty",
338+
name: "NodePoolsPerName and NodeGroupPerName empty",
339339
fields: fields{
340-
NodePoolsPerID: map[string]*sdk.NodePool{},
341-
NodeGroupPerProviderID: map[string]*NodeGroup{},
340+
NodePoolsPerName: map[string]*sdk.NodePool{},
341+
NodeGroupPerName: map[string]*NodeGroup{},
342342
},
343343
args: args{
344344
poolsList: []sdk.NodePool{
345345
np1,
346346
},
347-
nodePoolsPerID: map[string]*sdk.NodePool{},
348-
nodeGroupPerProviderID: map[string]*NodeGroup{},
347+
NodePoolsPerName: map[string]*sdk.NodePool{},
349348
},
350-
wantNodePoolsPerID: map[string]uint32{"np1": 1},
351-
wantNodeGroupPerProviderID: map[string]uint32{},
349+
wantNodePoolsPerName: map[string]uint32{"np1": 1},
350+
wantNodeGroupPerName: map[string]uint32{},
352351
},
353352
{
354-
name: "NodePoolsPerID and NodeGroupPerProviderID empty",
353+
name: "NodePoolsPerName and NodeGroupPerName empty",
355354
fields: fields{
356-
NodePoolsPerID: map[string]*sdk.NodePool{
355+
NodePoolsPerName: map[string]*sdk.NodePool{
357356
"np2": &np2,
358357
"np3": &np3,
359358
},
360-
NodeGroupPerProviderID: map[string]*NodeGroup{
359+
NodeGroupPerName: map[string]*NodeGroup{
361360
"np2-node-id": {NodePool: &np2},
362361
"np3-node-id": {NodePool: &np3},
363362
},
364363
},
365364
args: args{
366365
poolsList: []sdk.NodePool{
367366
{
368-
ID: "np1",
367+
Name: "np1",
369368
DesiredNodes: 1,
370369
},
371370
{
372-
ID: "np2",
371+
Name: "np2",
373372
DesiredNodes: 20,
374373
},
375374
},
376-
nodePoolsPerID: map[string]*sdk.NodePool{},
377-
nodeGroupPerProviderID: map[string]*NodeGroup{},
375+
NodeGroupPerName: map[string]*NodeGroup{},
378376
},
379-
wantNodePoolsPerID: map[string]uint32{
377+
wantNodePoolsPerName: map[string]uint32{
380378
"np1": 1, // np1 added
381379
"np2": 20, // np2 updated
382380
// np3 removed
383381
},
384-
wantNodeGroupPerProviderID: map[string]uint32{
382+
wantNodeGroupPerName: map[string]uint32{
385383
"np2-node-id": 20,
386384
"np3-node-id": 3, // Node reference that eventually stays in cache must not crash
387385
},
388386
},
389387
}
390388
for _, tt := range tests {
391389
t.Run(tt.name, func(t *testing.T) {
392-
manager.NodePoolsPerID = tt.fields.NodePoolsPerID
393-
manager.NodeGroupPerProviderID = tt.fields.NodeGroupPerProviderID
390+
manager.NodePoolsPerName = tt.fields.NodePoolsPerName
391+
manager.NodeGroupPerName = tt.fields.NodeGroupPerName
394392

395393
manager.setNodePoolsState(tt.args.poolsList)
396394

397-
assert.Len(t, manager.NodePoolsPerID, len(tt.wantNodePoolsPerID))
398-
for id, desiredNodes := range tt.wantNodePoolsPerID {
399-
assert.Equal(t, desiredNodes, manager.NodePoolsPerID[id].DesiredNodes)
395+
assert.Len(t, manager.NodePoolsPerName, len(tt.wantNodePoolsPerName))
396+
for name, desiredNodes := range tt.wantNodePoolsPerName {
397+
assert.Equal(t, desiredNodes, manager.NodePoolsPerName[name].DesiredNodes)
400398
}
401399

402-
assert.Len(t, manager.NodeGroupPerProviderID, len(tt.wantNodeGroupPerProviderID))
403-
for nodeID, desiredNodes := range tt.wantNodeGroupPerProviderID {
404-
assert.Equal(t, desiredNodes, manager.NodeGroupPerProviderID[nodeID].DesiredNodes)
400+
assert.Len(t, manager.NodeGroupPerName, len(tt.wantNodeGroupPerName))
401+
for nodeID, desiredNodes := range tt.wantNodeGroupPerName {
402+
assert.Equal(t, desiredNodes, manager.NodeGroupPerName[nodeID].DesiredNodes)
405403
}
406404
})
407405
}

‎cluster-autoscaler/cloudprovider/ovhcloud/ovh_cloud_node_group.go‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -213,7 +213,7 @@ func (ng *NodeGroup) Nodes() ([]cloudprovider.Instance, error) {
213213
instances = append(instances, instance)
214214

215215
// Store the associated node group in cache for future reference
216-
ng.Manager.setNodeGroupPerProviderID(instance.Id, ng)
216+
ng.Manager.setNodeGroupPerName(ng.Name, ng)
217217
}
218218

219219
return instances, nil
@@ -238,7 +238,7 @@ func (ng *NodeGroup) TemplateNodeInfo() (*framework.NodeInfo, error) {
238238
},
239239
}
240240

241-
// Add the nodepool label
241+
// Add the nodepool name label
242242
if node.ObjectMeta.Labels == nil {
243243
node.ObjectMeta.Labels = make(map[string]string)
244244
}

0 commit comments

Comments
 (0)