Skip to content

Commit 423e6c5

Browse files
authored
Merge pull request #6796 from enxebre/capi-nodegroups
Avoid expesive pointer copy in capi nodegroup
2 parents 44b128f + 8cfe11c commit 423e6c5

File tree

5 files changed

+43
-21
lines changed

5 files changed

+43
-21
lines changed

cluster-autoscaler/cloudprovider/clusterapi/clusterapi_controller.go

Lines changed: 3 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -672,13 +672,13 @@ func (c *machineController) findScalableResourceProviderIDs(scalableResource *un
672672
return providerIDs, nil
673673
}
674674

675-
func (c *machineController) nodeGroups() ([]*nodegroup, error) {
675+
func (c *machineController) nodeGroups() ([]cloudprovider.NodeGroup, error) {
676676
scalableResources, err := c.listScalableResources()
677677
if err != nil {
678678
return nil, err
679679
}
680680

681-
nodegroups := make([]*nodegroup, 0, len(scalableResources))
681+
nodegroups := make([]cloudprovider.NodeGroup, 0, len(scalableResources))
682682

683683
for _, r := range scalableResources {
684684
ng, err := newNodeGroupFromScalableResource(c, r)
@@ -688,6 +688,7 @@ func (c *machineController) nodeGroups() ([]*nodegroup, error) {
688688

689689
if ng != nil {
690690
nodegroups = append(nodegroups, ng)
691+
klog.V(4).Infof("discovered node group: %s", ng.Debug())
691692
}
692693
}
693694
return nodegroups, nil

cluster-autoscaler/cloudprovider/clusterapi/clusterapi_controller_test.go

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -76,7 +76,7 @@ type testSpec struct {
7676
const customCAPIGroup = "custom.x-k8s.io"
7777
const fifteenSecondDuration = time.Second * 15
7878

79-
func mustCreateTestController(t *testing.T, testConfigs ...*testConfig) (*machineController, testControllerShutdownFunc) {
79+
func mustCreateTestController(t testing.TB, testConfigs ...*testConfig) (*machineController, testControllerShutdownFunc) {
8080
t.Helper()
8181

8282
nodeObjects := make([]runtime.Object, 0)
@@ -492,7 +492,7 @@ func makeLinkedNodeAndMachine(i int, namespace, clusterName string, owner metav1
492492
return node, machine
493493
}
494494

495-
func addTestConfigs(t *testing.T, controller *machineController, testConfigs ...*testConfig) error {
495+
func addTestConfigs(t testing.TB, controller *machineController, testConfigs ...*testConfig) error {
496496
t.Helper()
497497

498498
for _, config := range testConfigs {
@@ -2113,15 +2113,15 @@ func Test_machineController_nodeGroups(t *testing.T) {
21132113

21142114
// Sort results as order is not guaranteed.
21152115
sort.Slice(got, func(i, j int) bool {
2116-
return got[i].scalableResource.Name() < got[j].scalableResource.Name()
2116+
return got[i].(*nodegroup).scalableResource.Name() < got[j].(*nodegroup).scalableResource.Name()
21172117
})
21182118
sort.Slice(tc.expectedScalableResources, func(i, j int) bool {
21192119
return tc.expectedScalableResources[i].GetName() < tc.expectedScalableResources[j].GetName()
21202120
})
21212121

21222122
if err == nil {
21232123
for i := range got {
2124-
if !reflect.DeepEqual(got[i].scalableResource.unstructured, tc.expectedScalableResources[i]) {
2124+
if !reflect.DeepEqual(got[i].(*nodegroup).scalableResource.unstructured, tc.expectedScalableResources[i]) {
21252125
t.Errorf("nodeGroups() got = %v, expected to consist of nodegroups for scalable resources: %v", got, tc.expectedScalableResources)
21262126
}
21272127
}

cluster-autoscaler/cloudprovider/clusterapi/clusterapi_nodegroup_test.go

Lines changed: 9 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -261,7 +261,7 @@ func TestNodeGroupIncreaseSizeErrors(t *testing.T) {
261261
t.Fatalf("expected 1 nodegroup, got %d", l)
262262
}
263263

264-
ng := nodegroups[0]
264+
ng := nodegroups[0].(*nodegroup)
265265
currReplicas, err := ng.TargetSize()
266266
if err != nil {
267267
t.Fatalf("unexpected error: %v", err)
@@ -348,7 +348,7 @@ func TestNodeGroupIncreaseSize(t *testing.T) {
348348
t.Fatalf("expected 1 nodegroup, got %d", l)
349349
}
350350

351-
ng := nodegroups[0]
351+
ng := nodegroups[0].(*nodegroup)
352352
currReplicas, err := ng.TargetSize()
353353
if err != nil {
354354
t.Fatalf("unexpected error: %v", err)
@@ -427,7 +427,7 @@ func TestNodeGroupDecreaseTargetSize(t *testing.T) {
427427
t.Fatalf("expected 1 nodegroup, got %d", l)
428428
}
429429

430-
ng := nodegroups[0]
430+
ng := nodegroups[0].(*nodegroup)
431431

432432
gvr, err := ng.scalableResource.GroupVersionResource()
433433
if err != nil {
@@ -596,7 +596,7 @@ func TestNodeGroupDecreaseSizeErrors(t *testing.T) {
596596
t.Fatalf("expected 1 nodegroup, got %d", l)
597597
}
598598

599-
ng := nodegroups[0]
599+
ng := nodegroups[0].(*nodegroup)
600600
currReplicas, err := ng.TargetSize()
601601
if err != nil {
602602
t.Fatalf("unexpected error: %v", err)
@@ -676,7 +676,7 @@ func TestNodeGroupDeleteNodes(t *testing.T) {
676676
t.Fatalf("expected 1 nodegroup, got %d", l)
677677
}
678678

679-
ng := nodegroups[0]
679+
ng := nodegroups[0].(*nodegroup)
680680
nodeNames, err := ng.Nodes()
681681
if err != nil {
682682
t.Fatalf("unexpected error: %v", err)
@@ -889,7 +889,7 @@ func TestNodeGroupDeleteNodesTwice(t *testing.T) {
889889
t.Fatalf("expected 1 nodegroup, got %d", l)
890890
}
891891

892-
ng := nodegroups[0]
892+
ng := nodegroups[0].(*nodegroup)
893893
nodeNames, err := ng.Nodes()
894894
if err != nil {
895895
t.Fatalf("unexpected error: %v", err)
@@ -961,7 +961,7 @@ func TestNodeGroupDeleteNodesTwice(t *testing.T) {
961961
t.Fatalf("unexpected error: %v", err)
962962
}
963963

964-
ng = nodegroups[0]
964+
ng = nodegroups[0].(*nodegroup)
965965

966966
// Check the nodegroup is at the expected size
967967
actualSize, err := ng.TargetSize()
@@ -1066,7 +1066,7 @@ func TestNodeGroupDeleteNodesSequential(t *testing.T) {
10661066
t.Fatalf("expected 1 nodegroup, got %d", l)
10671067
}
10681068

1069-
ng := nodegroups[0]
1069+
ng := nodegroups[0].(*nodegroup)
10701070
nodeNames, err := ng.Nodes()
10711071
if err != nil {
10721072
t.Fatalf("unexpected error: %v", err)
@@ -1132,7 +1132,7 @@ func TestNodeGroupDeleteNodesSequential(t *testing.T) {
11321132
t.Fatalf("unexpected error: %v", err)
11331133
}
11341134

1135-
ng = nodegroups[0]
1135+
ng = nodegroups[0].(*nodegroup)
11361136

11371137
// Check the nodegroup is at the expected size
11381138
actualSize, err := ng.scalableResource.Replicas()

cluster-autoscaler/cloudprovider/clusterapi/clusterapi_provider.go

Lines changed: 1 addition & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -59,17 +59,12 @@ func (p *provider) GetResourceLimiter() (*cloudprovider.ResourceLimiter, error)
5959
}
6060

6161
func (p *provider) NodeGroups() []cloudprovider.NodeGroup {
62-
var result []cloudprovider.NodeGroup
6362
nodegroups, err := p.controller.nodeGroups()
6463
if err != nil {
6564
klog.Errorf("error getting node groups: %v", err)
6665
return nil
6766
}
68-
for _, ng := range nodegroups {
69-
klog.V(4).Infof("discovered node group: %s", ng.Debug())
70-
result = append(result, ng)
71-
}
72-
return result
67+
return nodegroups
7368
}
7469

7570
func (p *provider) NodeGroupForNode(node *corev1.Node) (cloudprovider.NodeGroup, error) {

cluster-autoscaler/cloudprovider/clusterapi/clusterapi_provider_test.go

Lines changed: 26 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -101,3 +101,29 @@ func TestProviderConstructorProperties(t *testing.T) {
101101
t.Fatalf("expected 0 GPU types, got %d", got)
102102
}
103103
}
104+
func BenchmarkNodeGroups(b *testing.B) {
105+
resourceLimits := cloudprovider.ResourceLimiter{}
106+
annotations := map[string]string{
107+
nodeGroupMinSizeAnnotationKey: "1",
108+
nodeGroupMaxSizeAnnotationKey: "2",
109+
}
110+
111+
controller, stop := mustCreateTestController(b)
112+
defer stop()
113+
machineSetConfigs := createMachineSetTestConfigs("namespace", "", RandomString(6), 100, 1, annotations, nil)
114+
if err := addTestConfigs(b, controller, machineSetConfigs...); err != nil {
115+
b.Fatalf("unexpected error: %v", err)
116+
}
117+
118+
provider := newProvider(cloudprovider.ClusterAPIProviderName, &resourceLimits, controller)
119+
if actual := provider.Name(); actual != cloudprovider.ClusterAPIProviderName {
120+
b.Errorf("expected %q, got %q", cloudprovider.ClusterAPIProviderName, actual)
121+
}
122+
123+
b.ResetTimer()
124+
b.Run("NodeGroups", func(b *testing.B) {
125+
for i := 0; i < b.N; i++ {
126+
provider.NodeGroups()
127+
}
128+
})
129+
}

0 commit comments

Comments
 (0)