Return generated instance names from autoscaling GCE client

When there are multiple `createInstances` requests sent in a sequence,
the next request must be aware of the instance names used by the
previous request. Without this change sending a sequence of create
instances requests would result in the instance name duplication error.

Author: pmendelski

cluster-autoscaler/cloudprovider/gce/autoscaling_gce_client.go

@@ -138,7 +138,7 @@ type AutoscalingGceClient interface {
 	// modifying resources
 	ResizeMig(GceRef, int64) error
 	DeleteInstances(migRef GceRef, instances []GceRef) error
-	CreateInstances(GceRef, string, int64, []string) error
+	CreateInstances(GceRef, string, int64, []string) ([]string, error)
 
 	// WaitForOperation can be used to poll GCE operations until completion/timeout using WAIT calls.
 	// Calling this is normally not needed when interacting with the client, other methods should call it internally.
@@ -296,24 +296,26 @@ func (client *autoscalingGceClientV1) ResizeMig(migRef GceRef, size int64) error
 	return client.WaitForOperation(op.Name, op.OperationType, migRef.Project, migRef.Zone)
 }
 
-func (client *autoscalingGceClientV1) CreateInstances(migRef GceRef, baseName string, delta int64, existingInstanceProviderIds []string) error {
+func (client *autoscalingGceClientV1) CreateInstances(migRef GceRef, baseName string, delta int64, existingInstanceProviderIds []string) ([]string, error) {
 	registerRequest("instance_group_managers", "create_instances")
 	ctx, cancel := context.WithTimeout(context.Background(), client.operationPerCallTimeout)
 	defer cancel()
 	req := gce.InstanceGroupManagersCreateInstancesRequest{}
 	instanceNames := instanceIdsToNamesMap(existingInstanceProviderIds)
 	req.Instances = make([]*gce.PerInstanceConfig, 0, delta)
-	for i := int64(0); i < delta; i++ {
+	names := make([]string, delta)
+	for i := range delta {
 		newInstanceName := generateInstanceName(baseName, instanceNames)
 		instanceNames[newInstanceName] = true
+		names[i] = newInstanceName
 		req.Instances = append(req.Instances, &gce.PerInstanceConfig{Name: newInstanceName})
 	}
 
 	op, err := client.gceService.InstanceGroupManagers.CreateInstances(migRef.Project, migRef.Zone, migRef.Name, &req).Context(ctx).Do()
 	if err != nil {
-		return err
+		return nil, err
 	}
-	return client.WaitForOperation(op.Name, op.OperationType, migRef.Project, migRef.Zone)
+	return names, client.WaitForOperation(op.Name, op.OperationType, migRef.Project, migRef.Zone)
 }
 
 func instanceIdsToNamesMap(instanceProviderIds []string) map[string]bool {

cluster-autoscaler/cloudprovider/gce/autoscaling_gce_client_test.go

@@ -23,6 +23,7 @@ import (
 	"net/http"
 	"os"
 	"regexp"
+	"strings"
 	"testing"
 	"time"
 
@@ -34,6 +35,7 @@ import (
 	"github.com/google/go-cmp/cmp/cmpopts"
 	"github.com/stretchr/testify/assert"
 	"github.com/stretchr/testify/mock"
+	"github.com/stretchr/testify/require"
 	gce_api "google.golang.org/api/compute/v1"
 )
 
@@ -693,7 +695,8 @@ func TestAutoscalingClientTimeouts(t *testing.T) {
 	}{
 		"CreateInstances_ContextTimeout": {
 			clientFunc: func(client *autoscalingGceClientV1) error {
-				return client.CreateInstances(GceRef{}, "", 0, nil)
+				_, err := client.CreateInstances(GceRef{}, "", 0, nil)
+				return err
 			},
 			operationPerCallTimeout: &instantTimeout,
 		},
@@ -760,7 +763,8 @@ func TestAutoscalingClientTimeouts(t *testing.T) {
 		},
 		"CreateInstances_HttpClientTimeout": {
 			clientFunc: func(client *autoscalingGceClientV1) error {
-				return client.CreateInstances(GceRef{}, "", 0, nil)
+				_, err := client.CreateInstances(GceRef{}, "", 0, nil)
+				return err
 			},
 			httpTimeout: instantTimeout,
 		},
@@ -892,6 +896,26 @@ func TestAutoscalingClientTimeouts(t *testing.T) {
 	}
 }
 
+func TestCreateInstances(t *testing.T) {
+	server := test_util.NewHttpServerMock()
+	defer server.Close()
+	b, err := json.Marshal(gce_api.Operation{
+		Name: "operation-2505728466148-216f5197",
+	})
+	assert.NoError(t, err)
+	server.On("handle", "/projects/project1/zones/us-central1-b/instanceGroupManagers/igm1/createInstances").Return(string(b)).Times(1)
+	server.On("handle", "/projects/project1/zones/us-central1-b/operations/operation-2505728466148-216f5197/wait").Return(operationDoneResponse).Once()
+	client := newTestAutoscalingGceClientWithTimeout(t, "project", server.URL, "", time.Second)
+	migRef := GceRef{Project: "project1", Zone: "us-central1-b", Name: "igm1"}
+	names, err := client.CreateInstances(migRef, migRef.Name, 10, nil)
+	assert.NoError(t, err)
+	assert.Len(t, names, 10, "Expected 10 instance names in result")
+	for _, name := range names {
+		prefixed := strings.HasPrefix(name, migRef.Name+"-")
+		require.Truef(t, prefixed, "Expected node name \"%v\" to be prefixed with \"%v\"", name, migRef.Name)
+	}
+}
+
 func TestFetchAllInstances(t *testing.T) {
 	igm1 := "projects/893226960234/zones/zones/instanceGroupManagers/test-igm1-grp"
 	igm2 := "projects/893226960234/zones/zones/instanceGroupManagers/test-igm2-grp"

cluster-autoscaler/cloudprovider/gce/gce_manager.go

@@ -333,7 +333,8 @@ func (m *gceManagerImpl) CreateInstances(mig Mig, delta int64) error {
 		return fmt.Errorf("can't upscale %s: failed to collect BaseInstanceName: %w", mig.GceRef(), err)
 	}
 	m.cache.InvalidateMigTargetSize(mig.GceRef())
-	return m.GceService.CreateInstances(mig.GceRef(), baseName, delta, instancesNames)
+	_, err = m.GceService.CreateInstances(mig.GceRef(), baseName, delta, instancesNames)
+	return err
 }
 
 func (m *gceManagerImpl) forceRefresh() error {

cluster-autoscaler/cloudprovider/gce/mig_info_provider_test.go

@@ -192,8 +192,8 @@ func (client *mockAutoscalingGceClient) DeleteInstances(_ GceRef, _ []GceRef) er
 	return nil
 }
 
-func (client *mockAutoscalingGceClient) CreateInstances(_ GceRef, _ string, _ int64, _ []string) error {
-	return nil
+func (client *mockAutoscalingGceClient) CreateInstances(_ GceRef, _ string, _ int64, _ []string) ([]string, error) {
+	return nil, nil
 }
 
 func (client *mockAutoscalingGceClient) WaitForOperation(_, _, _, _ string) error {