MachinePool: populate providerIDList

In order for nodes to be associated to the MachinePool, we need to populate the
spec.providerIDList field.  This field is known to the MachinePool controller.

Author: justinsb

cloud/services/compute/instancegroupmanagers/instancegroupmanagers_reconcile.go

@@ -19,7 +19,9 @@ package instancegroupmanagers
 import (
 	"context"
 	"fmt"
+	"strings"
 
+	"github.com/GoogleCloudPlatform/k8s-cloud-provider/pkg/cloud/filter"
 	"github.com/GoogleCloudPlatform/k8s-cloud-provider/pkg/cloud/meta"
 	"google.golang.org/api/compute/v1"
 	"sigs.k8s.io/cluster-api-provider-gcp/cloud/gcperrors"
@@ -28,16 +30,16 @@ import (
 )
 
 // Reconcile reconcile machine instance.
-func (s *Service) Reconcile(ctx context.Context, instanceTemplateKey *meta.Key) error {
+func (s *Service) Reconcile(ctx context.Context, instanceTemplateKey *meta.Key) (*compute.InstanceGroupManager, error) {
 	log := log.FromContext(ctx)
 	log.Info("Reconciling instanceGroupManager resources")
 	igm, err := s.createOrGet(ctx, instanceTemplateKey)
 	if err != nil {
-		return err
+		return nil, err
 	}
 	log.V(2).Info("Reconciled instanceGroupManager", "selfLink", igm.SelfLink)
 
-	return nil
+	return igm, nil
 }
 
 // Delete deletes the GCP instanceGroupManager resource.
@@ -131,3 +133,33 @@ func (s *Service) createOrGet(ctx context.Context, instanceTemplateKey *meta.Key
 
 	return actual, nil
 }
+
+// ListInstances lists instances in the the instanceGroup linked to the passed instanceGroupManager.
+func (s *Service) ListInstances(ctx context.Context, instanceGroupManager *compute.InstanceGroupManager) ([]*compute.InstanceWithNamedPorts, error) {
+	log := log.FromContext(ctx)
+
+	var igKey *meta.Key
+	{
+		instanceGroup := instanceGroupManager.InstanceGroup
+		instanceGroup = strings.TrimPrefix(instanceGroup, "https://www.googleapis.com/")
+		instanceGroup = strings.TrimPrefix(instanceGroup, "compute/v1/")
+		tokens := strings.Split(instanceGroup, "/")
+		if len(tokens) == 6 && tokens[0] == "projects" && tokens[2] == "zones" && tokens[4] == "instanceGroups" {
+			igKey = meta.ZonalKey(tokens[5], tokens[3])
+		} else {
+			return nil, fmt.Errorf("unexpected format for instanceGroup: %q", instanceGroup)
+		}
+	}
+
+	log.Info("Listing instances in instanceGroup", "instanceGroup", instanceGroupManager.InstanceGroup)
+	listInstancesRequest := &compute.InstanceGroupsListInstancesRequest{
+		InstanceState: "ALL",
+	}
+	instances, err := s.instanceGroups.ListInstances(ctx, igKey, listInstancesRequest, filter.None)
+	if err != nil {
+		log.Error(err, "Error listing instances in instanceGroup", "instanceGroup", instanceGroupManager.InstanceGroup)
+		return nil, fmt.Errorf("listing instances in instanceGroup %q: %w", instanceGroupManager.InstanceGroup, err)
+	}
+
+	return instances, nil
+}

cloud/services/compute/instancegroupmanagers/service.go

@@ -50,6 +50,7 @@ type Scope interface {
 type Service struct {
 	scope                 Scope
 	instanceGroupManagers instanceGroupManagersClient
+	instanceGroups        k8scloud.InstanceGroups
 }
 
 // var _ cloud.Reconciler = &Service{}
@@ -61,5 +62,6 @@ func New(scope Scope) *Service {
 	return &Service{
 		scope:                 scope,
 		instanceGroupManagers: cloudScope.InstanceGroupManagers(),
+		instanceGroups:        cloudScope.InstanceGroups(),
 	}
 }

config/crd/bases/infrastructure.cluster.x-k8s.io_gcpmachinepools.yaml

@@ -256,6 +256,13 @@ spec:
               preemptible:
                 description: Preemptible defines if instance is preemptible
                 type: boolean
+              providerIDList:
+                description: |-
+                  ProviderIDList are the identification IDs of machine instances provided by the provider.
+                  This field must match the provider IDs as seen on the node objects corresponding to a machine pool's machine instances.
+                items:
+                  type: string
+                type: array
               provisioningModel:
                 description: |-
                   ProvisioningModel defines if instance is spot.

exp/api/v1beta1/gcpmachinepool_types.go

@@ -32,6 +32,11 @@ const (
 
 // GCPMachinePoolSpec defines the desired state of GCPMachinePool.
 type GCPMachinePoolSpec struct {
+	// ProviderIDList are the identification IDs of machine instances provided by the provider.
+	// This field must match the provider IDs as seen on the node objects corresponding to a machine pool's machine instances.
+	// +optional
+	ProviderIDList []string `json:"providerIDList,omitempty"`
+
 	// InstanceType is the type of instance to create. Example: n1.standard-2
 	InstanceType string `json:"instanceType"`
 

exp/api/v1beta1/gcpmachinepool_webhook.go

@@ -19,7 +19,6 @@ package v1beta1
 import (
 	"context"
 	"fmt"
-	"reflect"
 
 	apierrors "k8s.io/apimachinery/pkg/api/errors"
 	"k8s.io/apimachinery/pkg/runtime"
@@ -60,23 +59,14 @@ func (*gcpMachinePoolWebhook) ValidateCreate(_ context.Context, obj runtime.Obje
 }
 
 // ValidateUpdate implements webhook.Validator so a webhook will be registered for the type.
-func (*gcpMachinePoolWebhook) ValidateUpdate(_ context.Context, oldObj, newObj runtime.Object) (admission.Warnings, error) {
-	old, ok := oldObj.(*GCPMachinePool)
-	if !ok {
-		return nil, apierrors.NewBadRequest(fmt.Sprintf("expected a GCPMachinePool but got a %T", oldObj))
-	}
-
+func (*gcpMachinePoolWebhook) ValidateUpdate(_ context.Context, _, newObj runtime.Object) (admission.Warnings, error) {
 	r, ok := newObj.(*GCPMachinePool)
 	if !ok {
-		return nil, apierrors.NewBadRequest(fmt.Sprintf("expected a GCPMachinePool but got a %T", newObj))
+		return nil, fmt.Errorf("expected a GCPMachinePool object but got %T", r)
 	}
 
 	gcpMachinePoolLog.Info("Validating GCPMachinePool update", "name", r.Name)
 
-	if !reflect.DeepEqual(r.Spec, old.Spec) {
-		return nil, apierrors.NewBadRequest("GCPMachinePool.Spec is immutable")
-	}
-
 	return nil, nil
 }
 

exp/api/v1beta1/zz_generated.deepcopy.go

@@ -20,6 +20,8 @@ package controllers
 import (
 	"context"
 	"fmt"
+	"strings"
+	"time"
 
 	"github.com/google/go-cmp/cmp"
 	corev1 "k8s.io/api/core/v1"
@@ -180,7 +182,7 @@ func (r *GCPMachinePoolReconciler) SetupWithManager(ctx context.Context, mgr ctr
 		Complete(r)
 }
 
-func (r *GCPMachinePoolReconciler) reconcile(ctx context.Context, machinePoolScope *scope.MachinePoolScope) (ctrl.Result, error) { //nolint:unparam
+func (r *GCPMachinePoolReconciler) reconcile(ctx context.Context, machinePoolScope *scope.MachinePoolScope) (ctrl.Result, error) {
 	log := logger.FromContext(ctx)
 
 	log.Info("Reconciling GCPMachinePool")
@@ -226,7 +228,8 @@ func (r *GCPMachinePoolReconciler) reconcile(ctx context.Context, machinePoolSco
 	// set the InstanceTemplateReadyCondition condition
 	conditions.MarkTrue(machinePoolScope.GCPMachinePool, expinfrav1.InstanceTemplateReadyCondition)
 
-	if err := instancegroupmanagers.New(machinePoolScope).Reconcile(ctx, instanceTemplateKey); err != nil {
+	igm, err := instancegroupmanagers.New(machinePoolScope).Reconcile(ctx, instanceTemplateKey)
+	if err != nil {
 		log.Error(err, "Error reconciling instanceGroupManager")
 		// record.Warnf(machineScope.GCPMachine, "GCPMachineReconcile", "Reconcile error - %v", err)
 		conditions.MarkUnknown(machinePoolScope.GCPMachinePool, expinfrav1.MIGReadyCondition, expinfrav1.MIGNotFoundReason, "%s", err.Error())
@@ -236,7 +239,39 @@ func (r *GCPMachinePoolReconciler) reconcile(ctx context.Context, machinePoolSco
 	// set the MIGReadyCondition condition
 	conditions.MarkTrue(machinePoolScope.GCPMachinePool, expinfrav1.MIGReadyCondition)
 
-	return ctrl.Result{}, nil
+	igmInstances, err := instancegroupmanagers.New(machinePoolScope).ListInstances(ctx, igm)
+	if err != nil {
+		log.Error(err, "Error listing instances in instanceGroupManager")
+		return ctrl.Result{}, err
+	}
+
+	providerIDList := make([]string, len(igmInstances))
+
+	for i, instance := range igmInstances {
+		var providerID string
+
+		// Convert instance URL to providerID format
+		u := instance.Instance
+		u = strings.TrimPrefix(u, "https://www.googleapis.com/compute/v1/")
+		tokens := strings.Split(u, "/")
+		if len(tokens) == 6 && tokens[0] == "projects" && tokens[2] == "zones" && tokens[4] == "instances" {
+			providerID = fmt.Sprintf("gce://%s/%s/%s", tokens[1], tokens[3], tokens[5])
+		} else {
+			return ctrl.Result{}, fmt.Errorf("unexpected instance URL format: %s", instance.Instance)
+		}
+
+		providerIDList[i] = providerID
+	}
+
+	// FUTURE: do we need to verify that the instances are actually running?
+	machinePoolScope.GCPMachinePool.Spec.ProviderIDList = providerIDList
+	machinePoolScope.GCPMachinePool.Status.Replicas = int32(len(providerIDList))
+	machinePoolScope.GCPMachinePool.Status.Ready = true
+
+	// Requeue so that we can keep the spec.providerIDList and status in sync with the MIG.
+	// This is important for scaling up and down, as the CAPI MachinePool controller relies on
+	// the providerIDList to determine which machines belong to the MachinePool.
+	return ctrl.Result{RequeueAfter: 1 * time.Minute}, nil
 }
 
 func (r *GCPMachinePoolReconciler) reconcileDelete(ctx context.Context, machinePoolScope *scope.MachinePoolScope) error {