Merge pull request kubernetes#6796 from enxebre/capi-nodegroups

Avoid expesive pointer copy in capi nodegroup
enxebre · May 9, 2024 · f789a93 · f789a93
1 parent c852679
commit f789a93
Show file tree

Hide file tree

Showing 5 changed files with 43 additions and 21 deletions.
diff --git a/cluster-autoscaler/cloudprovider/clusterapi/clusterapi_controller.go b/cluster-autoscaler/cloudprovider/clusterapi/clusterapi_controller.go
@@ -670,13 +670,13 @@ func (c *machineController) findScalableResourceProviderIDs(scalableResource *un
 	return providerIDs, nil
 }
 
-func (c *machineController) nodeGroups() ([]*nodegroup, error) {
+func (c *machineController) nodeGroups() ([]cloudprovider.NodeGroup, error) {
 	scalableResources, err := c.listScalableResources()
 	if err != nil {
 		return nil, err
 	}
 
-	nodegroups := make([]*nodegroup, 0, len(scalableResources))
+	nodegroups := make([]cloudprovider.NodeGroup, 0, len(scalableResources))
 
 	for _, r := range scalableResources {
 		ng, err := newNodeGroupFromScalableResource(c, r)
@@ -686,6 +686,7 @@ func (c *machineController) nodeGroups() ([]*nodegroup, error) {
 
 		if ng != nil {
 			nodegroups = append(nodegroups, ng)
+			klog.V(4).Infof("discovered node group: %s", ng.Debug())
 		}
 	}
 	return nodegroups, nil

diff --git a/cluster-autoscaler/cloudprovider/clusterapi/clusterapi_controller_test.go b/cluster-autoscaler/cloudprovider/clusterapi/clusterapi_controller_test.go
@@ -76,7 +76,7 @@ type testSpec struct {
 const customCAPIGroup = "custom.x-k8s.io"
 const fifteenSecondDuration = time.Second * 15
 
-func mustCreateTestController(t *testing.T, testConfigs ...*testConfig) (*machineController, testControllerShutdownFunc) {
+func mustCreateTestController(t testing.TB, testConfigs ...*testConfig) (*machineController, testControllerShutdownFunc) {
 	t.Helper()
 
 	nodeObjects := make([]runtime.Object, 0)
@@ -514,7 +514,7 @@ func makeLinkedNodeAndMachine(i int, namespace, clusterName string, owner metav1
 	return node, machine
 }
 
-func addTestConfigs(t *testing.T, controller *machineController, testConfigs ...*testConfig) error {
+func addTestConfigs(t testing.TB, controller *machineController, testConfigs ...*testConfig) error {
 	t.Helper()
 
 	for _, config := range testConfigs {
@@ -2136,15 +2136,15 @@ func Test_machineController_nodeGroups(t *testing.T) {
 
 			// Sort results as order is not guaranteed.
 			sort.Slice(got, func(i, j int) bool {
-				return got[i].scalableResource.Name() < got[j].scalableResource.Name()
+				return got[i].(*nodegroup).scalableResource.Name() < got[j].(*nodegroup).scalableResource.Name()
 			})
 			sort.Slice(tc.expectedScalableResources, func(i, j int) bool {
 				return tc.expectedScalableResources[i].GetName() < tc.expectedScalableResources[j].GetName()
 			})
 
 			if err == nil {
 				for i := range got {
-					if !reflect.DeepEqual(got[i].scalableResource.unstructured, tc.expectedScalableResources[i]) {
+					if !reflect.DeepEqual(got[i].(*nodegroup).scalableResource.unstructured, tc.expectedScalableResources[i]) {
 						t.Errorf("nodeGroups() got = %v, expected to consist of nodegroups for scalable resources: %v", got, tc.expectedScalableResources)
 					}
 				}

diff --git a/cluster-autoscaler/cloudprovider/clusterapi/clusterapi_nodegroup_test.go b/cluster-autoscaler/cloudprovider/clusterapi/clusterapi_nodegroup_test.go
@@ -249,7 +249,7 @@ func TestNodeGroupIncreaseSizeErrors(t *testing.T) {
 			t.Fatalf("expected 1 nodegroup, got %d", l)
 		}
 
-		ng := nodegroups[0]
+		ng := nodegroups[0].(*nodegroup)
 		currReplicas, err := ng.TargetSize()
 		if err != nil {
 			t.Fatalf("unexpected error: %v", err)
@@ -336,7 +336,7 @@ func TestNodeGroupIncreaseSize(t *testing.T) {
 			t.Fatalf("expected 1 nodegroup, got %d", l)
 		}
 
-		ng := nodegroups[0]
+		ng := nodegroups[0].(*nodegroup)
 		currReplicas, err := ng.TargetSize()
 		if err != nil {
 			t.Fatalf("unexpected error: %v", err)
@@ -415,7 +415,7 @@ func TestNodeGroupDecreaseTargetSize(t *testing.T) {
 			t.Fatalf("expected 1 nodegroup, got %d", l)
 		}
 
-		ng := nodegroups[0]
+		ng := nodegroups[0].(*nodegroup)
 
 		gvr, err := ng.scalableResource.GroupVersionResource()
 		if err != nil {
@@ -584,7 +584,7 @@ func TestNodeGroupDecreaseSizeErrors(t *testing.T) {
 			t.Fatalf("expected 1 nodegroup, got %d", l)
 		}
 
-		ng := nodegroups[0]
+		ng := nodegroups[0].(*nodegroup)
 		currReplicas, err := ng.TargetSize()
 		if err != nil {
 			t.Fatalf("unexpected error: %v", err)
@@ -664,7 +664,7 @@ func TestNodeGroupDeleteNodes(t *testing.T) {
 			t.Fatalf("expected 1 nodegroup, got %d", l)
 		}
 
-		ng := nodegroups[0]
+		ng := nodegroups[0].(*nodegroup)
 		nodeNames, err := ng.Nodes()
 		if err != nil {
 			t.Fatalf("unexpected error: %v", err)
@@ -877,7 +877,7 @@ func TestNodeGroupDeleteNodesTwice(t *testing.T) {
 			t.Fatalf("expected 1 nodegroup, got %d", l)
 		}
 
-		ng := nodegroups[0]
+		ng := nodegroups[0].(*nodegroup)
 		nodeNames, err := ng.Nodes()
 		if err != nil {
 			t.Fatalf("unexpected error: %v", err)
@@ -949,7 +949,7 @@ func TestNodeGroupDeleteNodesTwice(t *testing.T) {
 			t.Fatalf("unexpected error: %v", err)
 		}
 
-		ng = nodegroups[0]
+		ng = nodegroups[0].(*nodegroup)
 
 		// Check the nodegroup is at the expected size
 		actualSize, err := ng.TargetSize()
@@ -1054,7 +1054,7 @@ func TestNodeGroupDeleteNodesSequential(t *testing.T) {
 			t.Fatalf("expected 1 nodegroup, got %d", l)
 		}
 
-		ng := nodegroups[0]
+		ng := nodegroups[0].(*nodegroup)
 		nodeNames, err := ng.Nodes()
 		if err != nil {
 			t.Fatalf("unexpected error: %v", err)
@@ -1120,7 +1120,7 @@ func TestNodeGroupDeleteNodesSequential(t *testing.T) {
 			t.Fatalf("unexpected error: %v", err)
 		}
 
-		ng = nodegroups[0]
+		ng = nodegroups[0].(*nodegroup)
 
 		// Check the nodegroup is at the expected size
 		actualSize, err := ng.scalableResource.Replicas()

diff --git a/cluster-autoscaler/cloudprovider/clusterapi/clusterapi_provider.go b/cluster-autoscaler/cloudprovider/clusterapi/clusterapi_provider.go
@@ -58,17 +58,12 @@ func (p *provider) GetResourceLimiter() (*cloudprovider.ResourceLimiter, error)
 }
 
 func (p *provider) NodeGroups() []cloudprovider.NodeGroup {
-	var result []cloudprovider.NodeGroup
 	nodegroups, err := p.controller.nodeGroups()
 	if err != nil {
 		klog.Errorf("error getting node groups: %v", err)
 		return nil
 	}
-	for _, ng := range nodegroups {
-		klog.V(4).Infof("discovered node group: %s", ng.Debug())
-		result = append(result, ng)
-	}
-	return result
+	return nodegroups
 }
 
 func (p *provider) NodeGroupForNode(node *corev1.Node) (cloudprovider.NodeGroup, error) {

diff --git a/cluster-autoscaler/cloudprovider/clusterapi/clusterapi_provider_test.go b/cluster-autoscaler/cloudprovider/clusterapi/clusterapi_provider_test.go
@@ -101,3 +101,29 @@ func TestProviderConstructorProperties(t *testing.T) {
 		t.Fatalf("expected 0 GPU types, got %d", got)
 	}
 }
+func BenchmarkNodeGroups(b *testing.B) {
+	resourceLimits := cloudprovider.ResourceLimiter{}
+	annotations := map[string]string{
+		nodeGroupMinSizeAnnotationKey: "1",
+		nodeGroupMaxSizeAnnotationKey: "2",
+	}
+
+	controller, stop := mustCreateTestController(b)
+	defer stop()
+	machineSetConfigs := createMachineSetTestConfigs("namespace", "", RandomString(6), 100, 1, annotations, nil)
+	if err := addTestConfigs(b, controller, machineSetConfigs...); err != nil {
+		b.Fatalf("unexpected error: %v", err)
+	}
+
+	provider := newProvider(cloudprovider.ClusterAPIProviderName, &resourceLimits, controller)
+	if actual := provider.Name(); actual != cloudprovider.ClusterAPIProviderName {
+		b.Errorf("expected %q, got %q", cloudprovider.ClusterAPIProviderName, actual)
+	}
+
+	b.ResetTimer()
+	b.Run("NodeGroups", func(b *testing.B) {
+		for i := 0; i < b.N; i++ {
+			provider.NodeGroups()
+		}
+	})
+}