From 3589b337c23657621122e5f859f4bab762ff4437 Mon Sep 17 00:00:00 2001 From: Charles Zhang Date: Fri, 22 Sep 2023 13:37:22 -0700 Subject: [PATCH] add more features to gcpmmp lint --- cloud/scope/managedmachinepool.go | 59 ++++-- cloud/scope/managedmachinepool_test.go | 141 +++++++++++++++ cloud/scope/scope_suite_test.go | 13 ++ .../services/container/clusters/reconcile.go | 2 +- .../services/container/nodepools/reconcile.go | 112 ++++++++---- ...uster.x-k8s.io_gcpmanagedmachinepools.yaml | 83 ++++++++- .../v1beta1/gcpmanagedmachinepool_types.go | 79 +++++++- .../v1beta1/gcpmanagedmachinepool_webhook.go | 108 +++++++++-- .../gcpmanagedmachinepool_webhook_test.go | 168 ++++++++++++++++++ exp/api/v1beta1/types.go | 51 +++++- exp/api/v1beta1/zz_generated.deepcopy.go | 70 ++++++++ .../gcpmanagedmachinepool_controller.go | 1 + 12 files changed, 816 insertions(+), 71 deletions(-) create mode 100644 cloud/scope/managedmachinepool_test.go create mode 100644 cloud/scope/scope_suite_test.go create mode 100644 exp/api/v1beta1/gcpmanagedmachinepool_webhook_test.go diff --git a/cloud/scope/managedmachinepool.go b/cloud/scope/managedmachinepool.go index 7f0dea9270..28f0517164 100644 --- a/cloud/scope/managedmachinepool.go +++ b/cloud/scope/managedmachinepool.go @@ -20,6 +20,7 @@ import ( "context" "fmt" + infrav1 "sigs.k8s.io/cluster-api-provider-gcp/api/v1beta1" "sigs.k8s.io/cluster-api-provider-gcp/cloud" "sigs.k8s.io/cluster-api-provider-gcp/util/location" @@ -153,30 +154,68 @@ func (s *ManagedMachinePoolScope) NodePoolVersion() *string { return s.MachinePool.Spec.Template.Spec.Version } +// NodePoolResourceLabels returns the resource labels of the node pool. +func NodePoolResourceLabels(additionalLabels infrav1.Labels, clusterName string) infrav1.Labels { + resourceLabels := additionalLabels.DeepCopy() + if resourceLabels == nil { + resourceLabels = infrav1.Labels{} + } + resourceLabels[infrav1.ClusterTagKey(clusterName)] = string(infrav1.ResourceLifecycleOwned) + return resourceLabels +} + // ConvertToSdkNodePool converts a node pool to format that is used by GCP SDK. -func ConvertToSdkNodePool(nodePool infrav1exp.GCPManagedMachinePool, machinePool clusterv1exp.MachinePool, regional bool) *containerpb.NodePool { +func ConvertToSdkNodePool(nodePool infrav1exp.GCPManagedMachinePool, machinePool clusterv1exp.MachinePool, regional bool, clusterName string) *containerpb.NodePool { replicas := *machinePool.Spec.Replicas if regional { replicas /= cloud.DefaultNumRegionsPerZone } nodePoolName := nodePool.Spec.NodePoolName if len(nodePoolName) == 0 { + // Use the GCPManagedMachinePool CR name if nodePoolName is not specified nodePoolName = nodePool.Name } + // build node pool in GCP SDK format using the GCPManagedMachinePool spec sdkNodePool := containerpb.NodePool{ Name: nodePoolName, InitialNodeCount: replicas, Config: &containerpb.NodeConfig{ - Labels: nodePool.Spec.KubernetesLabels, - Taints: infrav1exp.ConvertToSdkTaint(nodePool.Spec.KubernetesTaints), - Metadata: nodePool.Spec.AdditionalLabels, + Labels: nodePool.Spec.KubernetesLabels, + Taints: infrav1exp.ConvertToSdkTaint(nodePool.Spec.KubernetesTaints), + ResourceLabels: NodePoolResourceLabels(nodePool.Spec.AdditionalLabels, clusterName), + Tags: nodePool.Spec.NetworkTags, }, } + if nodePool.Spec.MachineType != nil { + sdkNodePool.Config.MachineType = *nodePool.Spec.MachineType + } + if nodePool.Spec.DiskSizeGb != nil { + sdkNodePool.Config.DiskSizeGb = *nodePool.Spec.DiskSizeGb + } + if nodePool.Spec.ServiceAccount != nil { + sdkNodePool.Config.ServiceAccount = *nodePool.Spec.ServiceAccount + } + if nodePool.Spec.ImageType != nil { + sdkNodePool.Config.ImageType = *nodePool.Spec.ImageType + } + if nodePool.Spec.LocalSsdCount != nil { + sdkNodePool.Config.LocalSsdCount = *nodePool.Spec.LocalSsdCount + } + if nodePool.Spec.DiskType != nil { + sdkNodePool.Config.DiskType = *nodePool.Spec.DiskType + } if nodePool.Spec.Scaling != nil { - sdkNodePool.Autoscaling = &containerpb.NodePoolAutoscaling{ - Enabled: true, - MinNodeCount: *nodePool.Spec.Scaling.MinCount, - MaxNodeCount: *nodePool.Spec.Scaling.MaxCount, + sdkNodePool.Autoscaling = infrav1exp.ConvertToSdkAutoscaling(nodePool.Spec.Scaling) + } + if nodePool.Spec.Management != nil { + sdkNodePool.Management = &containerpb.NodeManagement{ + AutoRepair: nodePool.Spec.Management.AutoRepair, + AutoUpgrade: nodePool.Spec.Management.AutoUpgrade, + } + } + if nodePool.Spec.MaxPodsConstraint != nil { + sdkNodePool.MaxPodsConstraint = &containerpb.MaxPodsConstraint{ + MaxPodsPerNode: *nodePool.Spec.MaxPodsConstraint, } } if machinePool.Spec.Template.Spec.Version != nil { @@ -186,10 +225,10 @@ func ConvertToSdkNodePool(nodePool infrav1exp.GCPManagedMachinePool, machinePool } // ConvertToSdkNodePools converts node pools to format that is used by GCP SDK. -func ConvertToSdkNodePools(nodePools []infrav1exp.GCPManagedMachinePool, machinePools []clusterv1exp.MachinePool, regional bool) []*containerpb.NodePool { +func ConvertToSdkNodePools(nodePools []infrav1exp.GCPManagedMachinePool, machinePools []clusterv1exp.MachinePool, regional bool, clusterName string) []*containerpb.NodePool { res := []*containerpb.NodePool{} for i := range nodePools { - res = append(res, ConvertToSdkNodePool(nodePools[i], machinePools[i], regional)) + res = append(res, ConvertToSdkNodePool(nodePools[i], machinePools[i], regional, clusterName)) } return res } diff --git a/cloud/scope/managedmachinepool_test.go b/cloud/scope/managedmachinepool_test.go new file mode 100644 index 0000000000..924b1a3b84 --- /dev/null +++ b/cloud/scope/managedmachinepool_test.go @@ -0,0 +1,141 @@ +package scope + +import ( + "cloud.google.com/go/container/apiv1/containerpb" + . "github.com/onsi/ginkgo/v2" + . "github.com/onsi/gomega" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + infrav1 "sigs.k8s.io/cluster-api-provider-gcp/api/v1beta1" + "sigs.k8s.io/cluster-api-provider-gcp/cloud" + "sigs.k8s.io/cluster-api-provider-gcp/exp/api/v1beta1" + clusterv1exp "sigs.k8s.io/cluster-api/exp/api/v1beta1" +) + +var TestGCPMMP *v1beta1.GCPManagedMachinePool +var TestMP *clusterv1exp.MachinePool +var TestClusterName string + +var _ = Describe("GCPManagedMachinePool Scope", func() { + BeforeEach(func() { + TestClusterName = "test-cluster" + gcpmmpName := "test-gcpmmp" + nodePoolName := "test-pool" + namespace := "capg-system" + replicas := int32(1) + + TestGCPMMP = &v1beta1.GCPManagedMachinePool{ + ObjectMeta: metav1.ObjectMeta{ + Name: gcpmmpName, + Namespace: namespace, + }, + Spec: v1beta1.GCPManagedMachinePoolSpec{ + NodePoolName: nodePoolName, + }, + } + TestMP = &clusterv1exp.MachinePool{ + Spec: clusterv1exp.MachinePoolSpec{ + Replicas: &replicas, + }, + } + }) + + Context("Test NodePoolResourceLabels", func() { + It("should append cluster owned label", func() { + labels := infrav1.Labels{"test-key": "test-value"} + + Expect(NodePoolResourceLabels(labels, TestClusterName)).To(Equal(infrav1.Labels{ + "test-key": "test-value", + infrav1.ClusterTagKey(TestClusterName): string(infrav1.ResourceLifecycleOwned), + })) + }) + }) + + Context("Test ConvertToSdkNodePool", func() { + It("should convert to SDK node pool with default values", func() { + sdkNodePool := ConvertToSdkNodePool(*TestGCPMMP, *TestMP, false, TestClusterName) + + Expect(sdkNodePool).To(Equal(&containerpb.NodePool{ + Name: TestGCPMMP.Spec.NodePoolName, + InitialNodeCount: *TestMP.Spec.Replicas, + Config: &containerpb.NodeConfig{ + ResourceLabels: NodePoolResourceLabels(nil, TestClusterName), + }, + })) + }) + + It("should convert to SDK node pool node count in a regional cluster", func() { + replicas := int32(6) + TestMP.Spec.Replicas = &replicas + + sdkNodePool := ConvertToSdkNodePool(*TestGCPMMP, *TestMP, true, TestClusterName) + + Expect(sdkNodePool).To(Equal(&containerpb.NodePool{ + Name: TestGCPMMP.Spec.NodePoolName, + InitialNodeCount: replicas / cloud.DefaultNumRegionsPerZone, + Config: &containerpb.NodeConfig{ + ResourceLabels: NodePoolResourceLabels(nil, TestClusterName), + }, + })) + }) + + It("should convert to SDK node pool using GCPManagedMachinePool", func() { + machineType := "n1-standard-1" + diskSizeGb := int32(128) + serviceAccount := "test@project.iam.gserviceaccount.com" + imageType := "ubuntu_containerd" + localSsdCount := int32(2) + diskType := "pd-ssd" + maxPodsConstraint := int64(20) + enableAutoscaling := false + scaling := v1beta1.NodePoolAutoScaling{ + EnableAutoscaling: &enableAutoscaling, + } + labels := infrav1.Labels{"test-key": "test-value"} + taints := v1beta1.Taints{ + { + Key: "test-key", + Value: "test-value", + Effect: "NoSchedule", + }, + } + tags := []string{"test-tag"} + resourceLabels := infrav1.Labels{"test-key": "test-value"} + + TestGCPMMP.Spec.MachineType = &machineType + TestGCPMMP.Spec.DiskSizeGb = &diskSizeGb + TestGCPMMP.Spec.ServiceAccount = &serviceAccount + TestGCPMMP.Spec.ImageType = &imageType + TestGCPMMP.Spec.LocalSsdCount = &localSsdCount + TestGCPMMP.Spec.DiskType = &diskType + TestGCPMMP.Spec.Scaling = &scaling + TestGCPMMP.Spec.MaxPodsConstraint = &maxPodsConstraint + TestGCPMMP.Spec.KubernetesLabels = labels + TestGCPMMP.Spec.KubernetesTaints = taints + TestGCPMMP.Spec.NetworkTags = tags + TestGCPMMP.Spec.AdditionalLabels = resourceLabels + + sdkNodePool := ConvertToSdkNodePool(*TestGCPMMP, *TestMP, false, TestClusterName) + + Expect(sdkNodePool).To(Equal(&containerpb.NodePool{ + Name: TestGCPMMP.Spec.NodePoolName, + InitialNodeCount: *TestMP.Spec.Replicas, + Config: &containerpb.NodeConfig{ + Labels: labels, + Taints: v1beta1.ConvertToSdkTaint(taints), + ResourceLabels: NodePoolResourceLabels(resourceLabels, TestClusterName), + Tags: tags, + MachineType: machineType, + DiskSizeGb: diskSizeGb, + ServiceAccount: serviceAccount, + ImageType: imageType, + LocalSsdCount: localSsdCount, + DiskType: diskType, + }, + Autoscaling: v1beta1.ConvertToSdkAutoscaling(&scaling), + MaxPodsConstraint: &containerpb.MaxPodsConstraint{ + MaxPodsPerNode: maxPodsConstraint, + }, + })) + }) + }) +}) diff --git a/cloud/scope/scope_suite_test.go b/cloud/scope/scope_suite_test.go new file mode 100644 index 0000000000..8c0eb51228 --- /dev/null +++ b/cloud/scope/scope_suite_test.go @@ -0,0 +1,13 @@ +package scope_test + +import ( + "testing" + + . "github.com/onsi/ginkgo/v2" + . "github.com/onsi/gomega" +) + +func TestScope(t *testing.T) { + RegisterFailHandler(Fail) + RunSpecs(t, "Scope Suite") +} diff --git a/cloud/services/container/clusters/reconcile.go b/cloud/services/container/clusters/reconcile.go index 89894f50f2..9392d7c6e5 100644 --- a/cloud/services/container/clusters/reconcile.go +++ b/cloud/services/container/clusters/reconcile.go @@ -265,7 +265,7 @@ func (s *Service) createCluster(ctx context.Context, log *logr.Logger) error { cluster.InitialClusterVersion = *s.scope.GCPManagedControlPlane.Spec.ControlPlaneVersion } if !s.scope.IsAutopilotCluster() { - cluster.NodePools = scope.ConvertToSdkNodePools(nodePools, machinePools, isRegional) + cluster.NodePools = scope.ConvertToSdkNodePools(nodePools, machinePools, isRegional, cluster.Name) } createClusterRequest := &containerpb.CreateClusterRequest{ diff --git a/cloud/services/container/nodepools/reconcile.go b/cloud/services/container/nodepools/reconcile.go index f315f339c6..3cdeb9c809 100644 --- a/cloud/services/container/nodepools/reconcile.go +++ b/cloud/services/container/nodepools/reconcile.go @@ -20,6 +20,7 @@ import ( "context" "fmt" "reflect" + "strings" "sigs.k8s.io/cluster-api-provider-gcp/cloud" "sigs.k8s.io/cluster-api-provider-gcp/util/resourceurl" @@ -43,20 +44,32 @@ import ( "sigs.k8s.io/controller-runtime/pkg/log" ) +// setReadyStatusFromConditions updates the GCPManagedMachinePool's ready status based on its conditions. +func (s *Service) setReadyStatusFromConditions() { + machinePool := s.scope.GCPManagedMachinePool + if conditions.IsTrue(machinePool, clusterv1.ReadyCondition) || conditions.IsTrue(machinePool, infrav1exp.GKEMachinePoolUpdatingCondition) { + s.scope.GCPManagedMachinePool.Status.Ready = true + return + } + + s.scope.GCPManagedMachinePool.Status.Ready = false +} + // Reconcile reconcile GKE node pool. func (s *Service) Reconcile(ctx context.Context) (ctrl.Result, error) { log := log.FromContext(ctx) log.Info("Reconciling node pool resources") + // Update GCPManagedMachinePool ready status based on conditions + defer s.setReadyStatusFromConditions() + nodePool, err := s.describeNodePool(ctx, &log) if err != nil { - s.scope.GCPManagedMachinePool.Status.Ready = false conditions.MarkFalse(s.scope.ConditionSetter(), clusterv1.ReadyCondition, infrav1exp.GKEMachinePoolReconciliationFailedReason, clusterv1.ConditionSeverityError, err.Error()) return ctrl.Result{}, err } if nodePool == nil { log.Info("Node pool not found, creating", "cluster", s.scope.Cluster.Name) - s.scope.GCPManagedMachinePool.Status.Ready = false if err = s.createNodePool(ctx, &log); err != nil { conditions.MarkFalse(s.scope.ConditionSetter(), clusterv1.ReadyCondition, infrav1exp.GKEMachinePoolReconciliationFailedReason, clusterv1.ConditionSeverityError, err.Error()) conditions.MarkFalse(s.scope.ConditionSetter(), infrav1exp.GKEMachinePoolReadyCondition, infrav1exp.GKEMachinePoolReconciliationFailedReason, clusterv1.ConditionSeverityError, err.Error()) @@ -73,7 +86,6 @@ func (s *Service) Reconcile(ctx context.Context) (ctrl.Result, error) { instances, err := s.getInstances(ctx, nodePool) if err != nil { - s.scope.GCPManagedMachinePool.Status.Ready = false conditions.MarkFalse(s.scope.ConditionSetter(), clusterv1.ReadyCondition, infrav1exp.GKEMachinePoolReconciliationFailedReason, clusterv1.ConditionSeverityError, err.Error()) return ctrl.Result{}, err } @@ -83,59 +95,63 @@ func (s *Service) Reconcile(ctx context.Context) (ctrl.Result, error) { providerID, err := providerid.NewFromResourceURL(*instance.Instance) if err != nil { log.Error(err, "parsing instance url", "url", *instance.Instance) - s.scope.GCPManagedMachinePool.Status.Ready = false conditions.MarkFalse(s.scope.ConditionSetter(), infrav1exp.GKEMachinePoolReadyCondition, infrav1exp.GKEMachinePoolErrorReason, clusterv1.ConditionSeverityError, "") return ctrl.Result{}, err } providerIDList = append(providerIDList, providerID.String()) } s.scope.GCPManagedMachinePool.Spec.ProviderIDList = providerIDList + s.scope.GCPManagedMachinePool.Status.Replicas = int32(len(providerIDList)) + // Update GKEManagedMachinePool conditions based on GKE node pool status switch nodePool.Status { case containerpb.NodePool_PROVISIONING: + // node pool is creating log.Info("Node pool provisioning in progress") - s.scope.GCPManagedMachinePool.Status.Ready = false conditions.MarkFalse(s.scope.ConditionSetter(), clusterv1.ReadyCondition, infrav1exp.GKEMachinePoolCreatingReason, clusterv1.ConditionSeverityInfo, "") conditions.MarkFalse(s.scope.ConditionSetter(), infrav1exp.GKEMachinePoolReadyCondition, infrav1exp.GKEMachinePoolCreatingReason, clusterv1.ConditionSeverityInfo, "") conditions.MarkTrue(s.scope.ConditionSetter(), infrav1exp.GKEMachinePoolCreatingCondition) return ctrl.Result{RequeueAfter: reconciler.DefaultRetryTime}, nil case containerpb.NodePool_RECONCILING: + // node pool is updating/reconciling log.Info("Node pool reconciling in progress") - s.scope.GCPManagedMachinePool.Status.Ready = true conditions.MarkTrue(s.scope.ConditionSetter(), infrav1exp.GKEMachinePoolUpdatingCondition) return ctrl.Result{RequeueAfter: reconciler.DefaultRetryTime}, nil case containerpb.NodePool_STOPPING: + // node pool is deleting log.Info("Node pool stopping in progress") - s.scope.GCPManagedMachinePool.Status.Ready = false conditions.MarkFalse(s.scope.ConditionSetter(), clusterv1.ReadyCondition, infrav1exp.GKEMachinePoolDeletingReason, clusterv1.ConditionSeverityInfo, "") conditions.MarkFalse(s.scope.ConditionSetter(), infrav1exp.GKEMachinePoolReadyCondition, infrav1exp.GKEMachinePoolDeletingReason, clusterv1.ConditionSeverityInfo, "") conditions.MarkTrue(s.scope.ConditionSetter(), infrav1exp.GKEMachinePoolDeletingCondition) return ctrl.Result{}, nil case containerpb.NodePool_ERROR, containerpb.NodePool_RUNNING_WITH_ERROR: + // node pool is in error or degraded state var msg string if len(nodePool.Conditions) > 0 { msg = nodePool.Conditions[0].GetMessage() } log.Error(errors.New("Node pool in error/degraded state"), msg, "name", s.scope.GCPManagedMachinePool.Name) - s.scope.GCPManagedMachinePool.Status.Ready = false conditions.MarkFalse(s.scope.ConditionSetter(), infrav1exp.GKEMachinePoolReadyCondition, infrav1exp.GKEMachinePoolErrorReason, clusterv1.ConditionSeverityError, "") return ctrl.Result{}, nil case containerpb.NodePool_RUNNING: + // node pool is ready and running + conditions.MarkTrue(s.scope.ConditionSetter(), clusterv1.ReadyCondition) + conditions.MarkTrue(s.scope.ConditionSetter(), infrav1exp.GKEMachinePoolReadyCondition) + conditions.MarkFalse(s.scope.ConditionSetter(), infrav1exp.GKEMachinePoolCreatingCondition, infrav1exp.GKEMachinePoolCreatedReason, clusterv1.ConditionSeverityInfo, "") log.Info("Node pool running") default: log.Error(errors.New("Unhandled node pool status"), fmt.Sprintf("Unhandled node pool status %s", nodePool.Status), "name", s.scope.GCPManagedMachinePool.Name) return ctrl.Result{}, nil } - needUpdateVersionOrImage, nodePoolUpdateVersionOrImage := s.checkDiffAndPrepareUpdateVersionOrImage(nodePool) - if needUpdateVersionOrImage { - log.Info("Version/image update required") - err = s.updateNodePoolVersionOrImage(ctx, nodePoolUpdateVersionOrImage) + needUpdateConfig, nodePoolUpdateConfigRequest := s.checkDiffAndPrepareUpdateConfig(nodePool) + if needUpdateConfig { + log.Info("Node pool config update required") + err = s.updateNodePoolConfig(ctx, nodePoolUpdateConfigRequest) if err != nil { return ctrl.Result{}, err } - log.Info("Node pool version/image updating in progress") - s.scope.GCPManagedMachinePool.Status.Ready = true + log.Info("Node pool config updating in progress") conditions.MarkTrue(s.scope.ConditionSetter(), infrav1exp.GKEMachinePoolUpdatingCondition) return ctrl.Result{RequeueAfter: reconciler.DefaultRetryTime}, nil } @@ -148,7 +164,6 @@ func (s *Service) Reconcile(ctx context.Context) (ctrl.Result, error) { return ctrl.Result{}, err } log.Info("Node pool auto scaling updating in progress") - s.scope.GCPManagedMachinePool.Status.Ready = true conditions.MarkTrue(s.scope.ConditionSetter(), infrav1exp.GKEMachinePoolUpdatingCondition) return ctrl.Result{RequeueAfter: reconciler.DefaultRetryTime}, nil } @@ -161,7 +176,6 @@ func (s *Service) Reconcile(ctx context.Context) (ctrl.Result, error) { return ctrl.Result{}, err } log.Info("Node pool size updating in progress") - s.scope.GCPManagedMachinePool.Status.Ready = true conditions.MarkTrue(s.scope.ConditionSetter(), infrav1exp.GKEMachinePoolUpdatingCondition) return ctrl.Result{RequeueAfter: reconciler.DefaultRetryTime}, nil } @@ -183,6 +197,8 @@ func (s *Service) Delete(ctx context.Context) (ctrl.Result, error) { log := log.FromContext(ctx) log.Info("Deleting node pool resources") + defer s.setReadyStatusFromConditions() + nodePool, err := s.describeNodePool(ctx, &log) if err != nil { return ctrl.Result{}, err @@ -214,7 +230,6 @@ func (s *Service) Delete(ctx context.Context) (ctrl.Result, error) { return ctrl.Result{}, err } log.Info("Node pool deleting in progress") - s.scope.GCPManagedMachinePool.Status.Ready = false conditions.MarkFalse(s.scope.ConditionSetter(), clusterv1.ReadyCondition, infrav1exp.GKEMachinePoolDeletingReason, clusterv1.ConditionSeverityInfo, "") conditions.MarkFalse(s.scope.ConditionSetter(), infrav1exp.GKEMachinePoolReadyCondition, infrav1exp.GKEMachinePoolDeletingReason, clusterv1.ConditionSeverityInfo, "") conditions.MarkTrue(s.scope.ConditionSetter(), infrav1exp.GKEMachinePoolDeletingCondition) @@ -279,7 +294,7 @@ func (s *Service) createNodePool(ctx context.Context, log *logr.Logger) error { isRegional := shared.IsRegional(s.scope.Region()) createNodePoolRequest := &containerpb.CreateNodePoolRequest{ - NodePool: scope.ConvertToSdkNodePool(*s.scope.GCPManagedMachinePool, *s.scope.MachinePool, isRegional), + NodePool: scope.ConvertToSdkNodePool(*s.scope.GCPManagedMachinePool, *s.scope.MachinePool, isRegional, s.scope.GCPManagedControlPlane.Spec.ClusterName), Parent: s.scope.NodePoolLocation(), } _, err := s.scope.ManagedMachinePoolClient().CreateNodePool(ctx, createNodePoolRequest) @@ -290,7 +305,7 @@ func (s *Service) createNodePool(ctx context.Context, log *logr.Logger) error { return nil } -func (s *Service) updateNodePoolVersionOrImage(ctx context.Context, updateNodePoolRequest *containerpb.UpdateNodePoolRequest) error { +func (s *Service) updateNodePoolConfig(ctx context.Context, updateNodePoolRequest *containerpb.UpdateNodePoolRequest) error { _, err := s.scope.ManagedMachinePoolClient().UpdateNodePool(ctx, updateNodePoolRequest) if err != nil { return err @@ -329,29 +344,52 @@ func (s *Service) deleteNodePool(ctx context.Context) error { return nil } -func (s *Service) checkDiffAndPrepareUpdateVersionOrImage(existingNodePool *containerpb.NodePool) (bool, *containerpb.UpdateNodePoolRequest) { +func (s *Service) checkDiffAndPrepareUpdateConfig(existingNodePool *containerpb.NodePool) (bool, *containerpb.UpdateNodePoolRequest) { needUpdate := false updateNodePoolRequest := containerpb.UpdateNodePoolRequest{ Name: s.scope.NodePoolFullName(), } + + isRegional := shared.IsRegional(s.scope.Region()) + desiredNodePool := scope.ConvertToSdkNodePool(*s.scope.GCPManagedMachinePool, *s.scope.MachinePool, isRegional, s.scope.GCPManagedControlPlane.Spec.ClusterName) + // Node version - if s.scope.NodePoolVersion() != nil && *s.scope.NodePoolVersion() != existingNodePool.Version { + if s.scope.NodePoolVersion() != nil && *s.scope.NodePoolVersion() != infrav1exp.ConvertFromSdkNodeVersion(existingNodePool.Version) { needUpdate = true updateNodePoolRequest.NodeVersion = *s.scope.NodePoolVersion() } // Kubernetes labels - if !reflect.DeepEqual(map[string]string(s.scope.GCPManagedMachinePool.Spec.KubernetesLabels), existingNodePool.Config.Labels) { + if !reflect.DeepEqual(desiredNodePool.Config.GetLabels(), existingNodePool.Config.Labels) { needUpdate = true updateNodePoolRequest.Labels = &containerpb.NodeLabels{ - Labels: s.scope.GCPManagedMachinePool.Spec.KubernetesLabels, + Labels: desiredNodePool.Config.Labels, } } // Kubernetes taints - desiredKubernetesTaints := infrav1exp.ConvertToSdkTaint(s.scope.GCPManagedMachinePool.Spec.KubernetesTaints) - if !reflect.DeepEqual(desiredKubernetesTaints, existingNodePool.Config.Taints) { + if !reflect.DeepEqual(desiredNodePool.Config.GetTaints(), existingNodePool.Config.Taints) { needUpdate = true updateNodePoolRequest.Taints = &containerpb.NodeTaints{ - Taints: desiredKubernetesTaints, + Taints: desiredNodePool.Config.GetTaints(), + } + } + // Node image type + // GCP API returns image type string in all uppercase, we can do a case-insensitive check here. + if desiredNodePool.Config.ImageType != "" && !strings.EqualFold(desiredNodePool.Config.ImageType, existingNodePool.Config.ImageType) { + needUpdate = true + updateNodePoolRequest.ImageType = desiredNodePool.Config.ImageType + } + // Network tags + if !reflect.DeepEqual(desiredNodePool.Config.Tags, existingNodePool.Config.Tags) { + needUpdate = true + updateNodePoolRequest.Tags = &containerpb.NetworkTags{ + Tags: desiredNodePool.Config.Tags, + } + } + // Additional resource labels + if !reflect.DeepEqual(desiredNodePool.Config.ResourceLabels, existingNodePool.Config.ResourceLabels) { + needUpdate = true + updateNodePoolRequest.ResourceLabels = &containerpb.ResourceLabels{ + Labels: desiredNodePool.Config.ResourceLabels, } } return needUpdate, &updateNodePoolRequest @@ -359,23 +397,12 @@ func (s *Service) checkDiffAndPrepareUpdateVersionOrImage(existingNodePool *cont func (s *Service) checkDiffAndPrepareUpdateAutoscaling(existingNodePool *containerpb.NodePool) (bool, *containerpb.SetNodePoolAutoscalingRequest) { needUpdate := false - - isRegional := shared.IsRegional(s.scope.Region()) - - desiredAutoscaling := scope.ConvertToSdkNodePool(*s.scope.GCPManagedMachinePool, *s.scope.MachinePool, isRegional).Autoscaling - var existingAutoscaling *containerpb.NodePoolAutoscaling - if existingNodePool.Autoscaling != nil && existingNodePool.Autoscaling.Enabled { - existingAutoscaling = &containerpb.NodePoolAutoscaling{ - Enabled: true, - MinNodeCount: existingNodePool.Autoscaling.MinNodeCount, - MaxNodeCount: existingNodePool.Autoscaling.MaxNodeCount, - } - } + desiredAutoscaling := infrav1exp.ConvertToSdkAutoscaling(s.scope.GCPManagedMachinePool.Spec.Scaling) setNodePoolAutoscalingRequest := containerpb.SetNodePoolAutoscalingRequest{ Name: s.scope.NodePoolFullName(), } - if !reflect.DeepEqual(desiredAutoscaling, existingAutoscaling) { + if !reflect.DeepEqual(desiredAutoscaling, existingNodePool.Autoscaling) { needUpdate = true setNodePoolAutoscalingRequest.Autoscaling = desiredAutoscaling } @@ -384,6 +411,13 @@ func (s *Service) checkDiffAndPrepareUpdateAutoscaling(existingNodePool *contain func (s *Service) checkDiffAndPrepareUpdateSize(existingNodePool *containerpb.NodePool) (bool, *containerpb.SetNodePoolSizeRequest) { needUpdate := false + desiredAutoscaling := infrav1exp.ConvertToSdkAutoscaling(s.scope.GCPManagedMachinePool.Spec.Scaling) + + if desiredAutoscaling.Enabled { + // Do not update node pool size if autoscaling is enabled. + return false, nil + } + setNodePoolSizeRequest := containerpb.SetNodePoolSizeRequest{ Name: s.scope.NodePoolFullName(), } diff --git a/config/crd/bases/infrastructure.cluster.x-k8s.io_gcpmanagedmachinepools.yaml b/config/crd/bases/infrastructure.cluster.x-k8s.io_gcpmanagedmachinepools.yaml index 95cffa8a92..1f5734fd6f 100644 --- a/config/crd/bases/infrastructure.cluster.x-k8s.io_gcpmanagedmachinepools.yaml +++ b/config/crd/bases/infrastructure.cluster.x-k8s.io_gcpmanagedmachinepools.yaml @@ -20,8 +20,11 @@ spec: scope: Namespaced versions: - additionalPrinterColumns: - - jsonPath: .spec.mode - name: Mode + - jsonPath: .status.ready + name: Ready + type: string + - jsonPath: .status.replicas + name: Replicas type: string name: v1beta1 schema: @@ -51,6 +54,21 @@ spec: GCP resources managed by the GCP provider, in addition to the ones added by default. type: object + diskSizeGb: + description: DiskSizeGb is the size of the disk attached to each node, + specified in GB. The smallest allowed disk size is 10GB. If unspecified, + the default disk size is 100GB. + format: int32 + type: integer + diskType: + description: DiskType is the of the disk attached to each node + type: string + imageType: + description: ImageType is the type of image to use for this node. + Note that for a given image type, the latest version of it will + be used. Please see https://cloud.google.com/kubernetes-engine/docs/concepts/node-images + for available image types. + type: string kubernetesLabels: additionalProperties: type: string @@ -82,6 +100,44 @@ spec: - value type: object type: array + localSsdCount: + description: LocalSsdCount is the number of local SSD disks to be + attached to the node. + format: int32 + type: integer + machineType: + description: MachineType is the name of a Google Compute Engine [machine + type](https://cloud.google.com/compute/docs/machine-types). If unspecified, + the default machine type is `e2-medium`. + type: string + management: + properties: + autoRepair: + description: AutoRepair specifies whether the node auto-repair + is enabled for the node pool. If enabled, the nodes in this + node pool will be monitored and, if they fail health checks + too many times, an automatic repair action will be triggered. + type: boolean + autoUpgrade: + description: AutoUpgrade specifies whether node auto-upgrade is + enabled for the node pool. If enabled, node auto-upgrade helps + keep the nodes in your node pool up to date with the latest + release version of Kubernetes. + type: boolean + type: object + maxPodsConstraint: + description: MaxPodsConstraint is the maximum number of pods that + can be run simultaneously on a node in the node pool. + format: int64 + type: integer + networkTags: + description: The list of instance tags applied to all nodes. Tags + are used to identify valid sources or targets for network firewalls + and are specified by the client during cluster or node pool creation. + Each tag within the list must comply with RFC1035. + items: + type: string + type: array nodePoolName: description: NodePoolName specifies the name of the GKE node pool corresponding to this MachinePool. If you don't specify a name then @@ -98,13 +154,33 @@ spec: scaling: description: Scaling specifies scaling for the node pool properties: + enableAutoscaling: + description: Is autoscaling enabled for this node pool. If unspecified, + the default value is true. + type: boolean + locationPolicy: + description: Location policy used when scaling up a nodepool. + enum: + - balanced + - any + type: string maxCount: + description: MaxCount specifies the maximum number of nodes in + the node pool format: int32 type: integer minCount: + description: MinCount specifies the minimum number of nodes in + the node pool format: int32 type: integer type: object + serviceAccount: + description: ServiceAccount is the Google Cloud Platform Service Account + to be used by the node VMs. Specify the email address of the Service + Account; otherwise, if no Service Account is specified, the "default" + service account is used. + type: string type: object status: description: GCPManagedMachinePoolStatus defines the observed state of @@ -157,6 +233,9 @@ spec: type: object type: array ready: + default: false + description: Ready denotes that the GCPManagedMachinePool has joined + the cluster type: boolean replicas: description: Replicas is the most recently observed number of replicas. diff --git a/exp/api/v1beta1/gcpmanagedmachinepool_types.go b/exp/api/v1beta1/gcpmanagedmachinepool_types.go index b8a2aea0a1..f493dfd025 100644 --- a/exp/api/v1beta1/gcpmanagedmachinepool_types.go +++ b/exp/api/v1beta1/gcpmanagedmachinepool_types.go @@ -34,6 +34,37 @@ type GCPManagedMachinePoolSpec struct { // then a default name will be created based on the namespace and name of the managed machine pool. // +optional NodePoolName string `json:"nodePoolName,omitempty"` + // MachineType is the name of a Google Compute Engine [machine + // type](https://cloud.google.com/compute/docs/machine-types). + // If unspecified, the default machine type is `e2-medium`. + // +optional + MachineType *string `json:"machineType,omitempty"` + // DiskSizeGb is the size of the disk attached to each node, specified in GB. + // The smallest allowed disk size is 10GB. If unspecified, the default disk size is 100GB. + // +optional + DiskSizeGb *int32 `json:"diskSizeGb,omitempty"` + // ServiceAccount is the Google Cloud Platform Service Account to be used by the node VMs. + // Specify the email address of the Service Account; otherwise, if no Service + // Account is specified, the "default" service account is used. + // +optional + ServiceAccount *string `json:"serviceAccount,omitempty"` + // ImageType is the type of image to use for this node. Note that for a given image type, + // the latest version of it will be used. Please see + // https://cloud.google.com/kubernetes-engine/docs/concepts/node-images for + // available image types. + // +optional + ImageType *string `json:"imageType,omitempty"` + // LocalSsdCount is the number of local SSD disks to be attached to the node. + // +optional + LocalSsdCount *int32 `json:"localSsdCount,omitempty"` + // The list of instance tags applied to all nodes. Tags are used to identify + // valid sources or targets for network firewalls and are specified by + // the client during cluster or node pool creation. Each tag within the list + // must comply with RFC1035. + NetworkTags []string `json:"networkTags,omitempty"` + // DiskType is the of the disk attached to each node + // +optional + DiskType *string `json:"diskType,omitempty"` // Scaling specifies scaling for the node pool // +optional Scaling *NodePoolAutoScaling `json:"scaling,omitempty"` @@ -46,7 +77,12 @@ type GCPManagedMachinePoolSpec struct { // AdditionalLabels is an optional set of tags to add to GCP resources managed by the GCP provider, in addition to the // ones added by default. // +optional - AdditionalLabels infrav1.Labels `json:"additionalLabels,omitempty"` + AdditionalLabels infrav1.Labels `json:"additionalLabels,omitempty"` + Management *NodePoolManagement `json:"management,omitempty"` + // MaxPodsConstraint is the maximum number of pods that can be run + // simultaneously on a node in the node pool. + // +optional + MaxPodsConstraint *int64 `json:"maxPodsConstraint,omitempty"` // ProviderIDList are the provider IDs of instances in the // managed instance group corresponding to the nodegroup represented by this // machine pool @@ -56,6 +92,8 @@ type GCPManagedMachinePoolSpec struct { // GCPManagedMachinePoolStatus defines the observed state of GCPManagedMachinePool. type GCPManagedMachinePoolStatus struct { + // Ready denotes that the GCPManagedMachinePool has joined the cluster + // +kubebuilder:default=false Ready bool `json:"ready"` // Replicas is the most recently observed number of replicas. // +optional @@ -65,10 +103,11 @@ type GCPManagedMachinePoolStatus struct { } // +kubebuilder:object:root=true -// +kubebuilder:printcolumn:name="Mode",type="string",JSONPath=".spec.mode" +// +kubebuilder:subresource:status +// +kubebuilder:printcolumn:name="Ready",type="string",JSONPath=".status.ready" +// +kubebuilder:printcolumn:name="Replicas",type="string",JSONPath=".status.replicas" // +kubebuilder:resource:path=gcpmanagedmachinepools,scope=Namespaced,categories=cluster-api,shortName=gcpmmp // +kubebuilder:storageversion -// +kubebuilder:subresource:status // GCPManagedMachinePool is the Schema for the gcpmanagedmachinepools API. type GCPManagedMachinePool struct { @@ -90,10 +129,44 @@ type GCPManagedMachinePoolList struct { // NodePoolAutoScaling specifies scaling options. type NodePoolAutoScaling struct { + // MinCount specifies the minimum number of nodes in the node pool + // +optional MinCount *int32 `json:"minCount,omitempty"` + // MaxCount specifies the maximum number of nodes in the node pool + // +optional MaxCount *int32 `json:"maxCount,omitempty"` + // Is autoscaling enabled for this node pool. If unspecified, the default value is true. + // +optional + EnableAutoscaling *bool `json:"enableAutoscaling,omitempty"` + // Location policy used when scaling up a nodepool. + // +kubebuilder:validation:Enum=balanced;any + // +optional + LocationPolicy *ManagedNodePoolLocationPolicy `json:"locationPolicy,omitempty"` +} + +// NodePoolManagement specifies auto-upgrade and auto-repair options. +type NodePoolManagement struct { + // AutoUpgrade specifies whether node auto-upgrade is enabled for the node + // pool. If enabled, node auto-upgrade helps keep the nodes in your node pool + // up to date with the latest release version of Kubernetes. + AutoUpgrade bool `json:"autoUpgrade,omitempty"` + // AutoRepair specifies whether the node auto-repair is enabled for the node + // pool. If enabled, the nodes in this node pool will be monitored and, if + // they fail health checks too many times, an automatic repair action will be + // triggered. + AutoRepair bool `json:"autoRepair,omitempty"` } +// ManagedNodePoolLocationPolicy specifies the location policy of the node pool when autoscaling is enabled. +type ManagedNodePoolLocationPolicy string + +const ( + // ManagedNodePoolLocationPolicyBalanced aims to balance the sizes of different zones. + ManagedNodePoolLocationPolicyBalanced ManagedNodePoolLocationPolicy = "balanced" + // ManagedNodePoolLocationPolicyAny picks zones that have the highest capacity available. + ManagedNodePoolLocationPolicyAny ManagedNodePoolLocationPolicy = "any" +) + // GetConditions returns the machine pool conditions. func (r *GCPManagedMachinePool) GetConditions() clusterv1.Conditions { return r.Status.Conditions diff --git a/exp/api/v1beta1/gcpmanagedmachinepool_webhook.go b/exp/api/v1beta1/gcpmanagedmachinepool_webhook.go index e425cbf0a3..3b19c2a91a 100644 --- a/exp/api/v1beta1/gcpmanagedmachinepool_webhook.go +++ b/exp/api/v1beta1/gcpmanagedmachinepool_webhook.go @@ -18,8 +18,8 @@ package v1beta1 import ( "fmt" + "reflect" - "github.com/google/go-cmp/cmp" apierrors "k8s.io/apimachinery/pkg/api/errors" "k8s.io/apimachinery/pkg/runtime" "k8s.io/apimachinery/pkg/util/validation/field" @@ -55,17 +55,63 @@ func (r *GCPManagedMachinePool) Default() { var _ webhook.Validator = &GCPManagedMachinePool{} +// validateSpec validates that the GCPManagedMachinePool spec is valid. +func (r *GCPManagedMachinePool) validateSpec() field.ErrorList { + var allErrs field.ErrorList + + // Validate node pool name + if len(r.Spec.NodePoolName) > maxNodePoolNameLength { + allErrs = append(allErrs, + field.Invalid(field.NewPath("spec", "NodePoolName"), + r.Spec.NodePoolName, fmt.Sprintf("node pool name cannot have more than %d characters", maxNodePoolNameLength)), + ) + } + + if errs := r.validateScaling(); errs != nil || len(errs) == 0 { + allErrs = append(allErrs, errs...) + } + + if errs := r.validateNonNegative(); errs != nil || len(errs) == 0 { + allErrs = append(allErrs, errs...) + } + + if len(allErrs) == 0 { + return nil + } + return allErrs +} + +// validateScaling validates that the GCPManagedMachinePool autoscaling spec is valid. func (r *GCPManagedMachinePool) validateScaling() field.ErrorList { var allErrs field.ErrorList if r.Spec.Scaling != nil { minField := field.NewPath("spec", "scaling", "minCount") maxField := field.NewPath("spec", "scaling", "maxCount") + locationPolicyField := field.NewPath("spec", "scaling", "locationPolicy") + min := r.Spec.Scaling.MinCount max := r.Spec.Scaling.MaxCount + locationPolicy := r.Spec.Scaling.LocationPolicy + + // cannot specify autoscaling config if autoscaling is disabled + if r.Spec.Scaling.EnableAutoscaling != nil && !*r.Spec.Scaling.EnableAutoscaling { + if min != nil { + allErrs = append(allErrs, field.Forbidden(minField, "minCount cannot be specified when autoscaling is disabled")) + } + if max != nil { + allErrs = append(allErrs, field.Forbidden(maxField, "maxCount cannot be specified when autoscaling is disabled")) + } + if locationPolicy != nil { + allErrs = append(allErrs, field.Forbidden(locationPolicyField, "locationPolicy cannot be specified when autoscaling is disabled")) + } + } + if min != nil { + // validates min >= 0 if *min < 0 { allErrs = append(allErrs, field.Invalid(minField, *min, "must be greater or equal zero")) } + // validates min <= max if max != nil && *max < *min { allErrs = append(allErrs, field.Invalid(maxField, *max, fmt.Sprintf("must be greater than field %s", minField.String()))) } @@ -77,19 +123,54 @@ func (r *GCPManagedMachinePool) validateScaling() field.ErrorList { return allErrs } -// ValidateCreate implements webhook.Validator so a webhook will be registered for the type. -func (r *GCPManagedMachinePool) ValidateCreate() (admission.Warnings, error) { - gcpmanagedmachinepoollog.Info("validate create", "name", r.Name) +func appendErrorIfNegative[T int32 | int64](value *T, name string, errs *field.ErrorList) { + if value != nil && *value < 0 { + *errs = append(*errs, field.Invalid(field.NewPath("spec", name), *value, "must be non-negative")) + } +} + +// validateNonNegative validates that non-negative GCPManagedMachinePool spec fields are not negative. +func (r *GCPManagedMachinePool) validateNonNegative() field.ErrorList { var allErrs field.ErrorList - if len(r.Spec.NodePoolName) > maxNodePoolNameLength { - allErrs = append(allErrs, - field.Invalid(field.NewPath("spec", "NodePoolName"), - r.Spec.NodePoolName, fmt.Sprintf("node pool name cannot have more than %d characters", maxNodePoolNameLength)), + appendErrorIfNegative(r.Spec.DiskSizeGb, "diskSizeGb", &allErrs) + appendErrorIfNegative(r.Spec.MaxPodsConstraint, "maxPodsConstraint", &allErrs) + appendErrorIfNegative(r.Spec.LocalSsdCount, "localSsdCount", &allErrs) + + return allErrs +} + +func appendErrorIfMutated(old, update interface{}, name string, errs *field.ErrorList) { + if !reflect.DeepEqual(old, update) { + *errs = append( + *errs, + field.Invalid(field.NewPath("spec", name), update, "field is immutable"), ) } +} - if errs := r.validateScaling(); errs != nil || len(errs) == 0 { +// validateImmutable validates that immutable GCPManagedMachinePool spec fields are not mutated. +func (r *GCPManagedMachinePool) validateImmutable(old *GCPManagedMachinePool) field.ErrorList { + var allErrs field.ErrorList + + appendErrorIfMutated(old.Spec.NodePoolName, r.Spec.NodePoolName, "nodePoolName", &allErrs) + appendErrorIfMutated(old.Spec.MachineType, r.Spec.MachineType, "machineType", &allErrs) + appendErrorIfMutated(old.Spec.ServiceAccount, r.Spec.ServiceAccount, "serviceAccount", &allErrs) + appendErrorIfMutated(old.Spec.DiskSizeGb, r.Spec.DiskSizeGb, "diskSizeGb", &allErrs) + appendErrorIfMutated(old.Spec.DiskType, r.Spec.DiskType, "diskType", &allErrs) + appendErrorIfMutated(old.Spec.LocalSsdCount, r.Spec.LocalSsdCount, "localSsdCount", &allErrs) + appendErrorIfMutated(old.Spec.Management, r.Spec.Management, "management", &allErrs) + appendErrorIfMutated(old.Spec.MaxPodsConstraint, r.Spec.MaxPodsConstraint, "maxPodsConstraint", &allErrs) + + return allErrs +} + +// ValidateCreate implements webhook.Validator so a webhook will be registered for the type. +func (r *GCPManagedMachinePool) ValidateCreate() (admission.Warnings, error) { + gcpmanagedmachinepoollog.Info("validate create", "name", r.Name) + var allErrs field.ErrorList + + if errs := r.validateSpec(); errs != nil || len(errs) == 0 { allErrs = append(allErrs, errs...) } @@ -106,14 +187,11 @@ func (r *GCPManagedMachinePool) ValidateUpdate(oldRaw runtime.Object) (admission var allErrs field.ErrorList old := oldRaw.(*GCPManagedMachinePool) - if !cmp.Equal(r.Spec.NodePoolName, old.Spec.NodePoolName) { - allErrs = append(allErrs, - field.Invalid(field.NewPath("spec", "NodePoolName"), - r.Spec.NodePoolName, "field is immutable"), - ) + if errs := r.validateImmutable(old); errs != nil { + allErrs = append(allErrs, errs...) } - if errs := r.validateScaling(); errs != nil || len(errs) == 0 { + if errs := r.validateSpec(); errs != nil || len(errs) == 0 { allErrs = append(allErrs, errs...) } diff --git a/exp/api/v1beta1/gcpmanagedmachinepool_webhook_test.go b/exp/api/v1beta1/gcpmanagedmachinepool_webhook_test.go new file mode 100644 index 0000000000..41187f665e --- /dev/null +++ b/exp/api/v1beta1/gcpmanagedmachinepool_webhook_test.go @@ -0,0 +1,168 @@ +/* +Copyright 2023 The Kubernetes Authors. + +Licensed under the Apache License, Version 2.0 (the "License"); +you may not use this file except in compliance with the License. +You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. +*/ + +package v1beta1 + +import ( + "strings" + + . "github.com/onsi/ginkgo/v2" + . "github.com/onsi/gomega" + infrav1 "sigs.k8s.io/cluster-api-provider-gcp/api/v1beta1" +) + +var gcpmmp *GCPManagedMachinePool + +var _ = Describe("Test GCPManagedMachinePool Webhooks", func() { + BeforeEach(func() { + machineType := "e2-medium" + serviceAccount := "default" + diskSizeGb := int32(100) + + gcpmmp = &GCPManagedMachinePool{ + Spec: GCPManagedMachinePoolSpec{ + NodePoolName: "test-gke-pool", + MachineType: &machineType, + ServiceAccount: &serviceAccount, + DiskSizeGb: &diskSizeGb, + }, + } + }) + + Context("Test validateSpec", func() { + It("should error when node pool name is too long", func() { + gcpmmp.Spec.NodePoolName = strings.Repeat("A", maxNodePoolNameLength+1) + errs := gcpmmp.validateSpec() + Expect(len(errs)).ToNot(BeZero()) + }) + It("should pass when node pool name is within limit", func() { + gcpmmp.Spec.NodePoolName = strings.Repeat("A", maxNodePoolNameLength) + errs := gcpmmp.validateSpec() + Expect(len(errs)).To(BeZero()) + }) + }) + + Context("Test validateScaling", func() { + It("should pass when scaling is not specified", func() { + errs := gcpmmp.validateScaling() + Expect(len(errs)).To(BeZero()) + }) + It("should pass when min/max count is valid", func() { + minCount := int32(1) + maxCount := int32(3) + gcpmmp.Spec.Scaling = &NodePoolAutoScaling{ + MinCount: &minCount, + MaxCount: &maxCount, + } + + errs := gcpmmp.validateScaling() + Expect(len(errs)).To(BeZero()) + }) + It("should fail when min is negative", func() { + minCount := int32(-1) + gcpmmp.Spec.Scaling = &NodePoolAutoScaling{ + MinCount: &minCount, + } + + errs := gcpmmp.validateScaling() + Expect(len(errs)).ToNot(BeZero()) + }) + It("should fail when min > max", func() { + minCount := int32(3) + maxCount := int32(1) + gcpmmp.Spec.Scaling = &NodePoolAutoScaling{ + MinCount: &minCount, + MaxCount: &maxCount, + } + + errs := gcpmmp.validateScaling() + Expect(len(errs)).ToNot(BeZero()) + }) + It("should fail when autoscaling is disabled and min/max is specified", func() { + minCount := int32(1) + maxCount := int32(3) + enabled := false + locationPolicy := ManagedNodePoolLocationPolicyAny + gcpmmp.Spec.Scaling = &NodePoolAutoScaling{ + MinCount: &minCount, + MaxCount: &maxCount, + EnableAutoscaling: &enabled, + LocationPolicy: &locationPolicy, + } + + errs := gcpmmp.validateScaling() + Expect(len(errs)).To(Equal(3)) + }) + }) + Context("Test validateImmutable", func() { + It("should pass when node pool is not mutated", func() { + old := gcpmmp.DeepCopy() + errs := gcpmmp.validateImmutable(old) + Expect(len(errs)).To(BeZero()) + }) + It("should pass when mutable fields are mutated", func() { + old := gcpmmp.DeepCopy() + gcpmmp.Spec.AdditionalLabels = infrav1.Labels{ + "testKey": "testVal", + } + + errs := gcpmmp.validateImmutable(old) + Expect(len(errs)).To(BeZero()) + }) + It("should fail when immutable fields are mutated", func() { + old := gcpmmp.DeepCopy() + diskSizeGb := int32(200) + gcpmmp.Spec.DiskSizeGb = &diskSizeGb + gcpmmp.Spec.NodePoolName = "new-name" + gcpmmp.Spec.Management = &NodePoolManagement{ + AutoUpgrade: false, + AutoRepair: false, + } + + errs := gcpmmp.validateImmutable(old) + Expect(len(errs)).To(Equal(3)) + }) + }) + + Context("Test validateNonNegative", func() { + It("should pass when number fields are not specified", func() { + errs := gcpmmp.validateNonNegative() + Expect(len(errs)).To(BeZero()) + }) + It("should pass when number fields are non-negative", func() { + maxPods := int64(10) + localSsds := int32(0) + diskSize := int32(200) + gcpmmp.Spec.MaxPodsConstraint = &maxPods + gcpmmp.Spec.LocalSsdCount = &localSsds + gcpmmp.Spec.DiskSizeGb = &diskSize + + errs := gcpmmp.validateNonNegative() + Expect(len(errs)).To(BeZero()) + }) + It("should pass when some number fields are negative", func() { + maxPods := int64(-1) + localSsds := int32(0) + diskSize := int32(-100) + gcpmmp.Spec.MaxPodsConstraint = &maxPods + gcpmmp.Spec.LocalSsdCount = &localSsds + gcpmmp.Spec.DiskSizeGb = &diskSize + + errs := gcpmmp.validateNonNegative() + Expect(len(errs)).To(Equal(2)) + }) + }) +}) diff --git a/exp/api/v1beta1/types.go b/exp/api/v1beta1/types.go index f4c87bb300..6f4baf0a2a 100644 --- a/exp/api/v1beta1/types.go +++ b/exp/api/v1beta1/types.go @@ -16,7 +16,11 @@ limitations under the License. package v1beta1 -import "cloud.google.com/go/container/apiv1/containerpb" +import ( + "strings" + + "cloud.google.com/go/container/apiv1/containerpb" +) // TaintEffect is the effect for a Kubernetes taint. type TaintEffect string @@ -63,3 +67,48 @@ func ConvertToSdkTaint(taints Taints) []*containerpb.NodeTaint { } return res } + +// convertToSdkLocationPolicy converts node pool autoscaling location policy to a value that is used by GCP SDK. +func convertToSdkLocationPolicy(locationPolicy ManagedNodePoolLocationPolicy) containerpb.NodePoolAutoscaling_LocationPolicy { + switch locationPolicy { + case ManagedNodePoolLocationPolicyBalanced: + return containerpb.NodePoolAutoscaling_BALANCED + case ManagedNodePoolLocationPolicyAny: + return containerpb.NodePoolAutoscaling_ANY + } + return containerpb.NodePoolAutoscaling_LOCATION_POLICY_UNSPECIFIED +} + +// ConvertToSdkAutoscaling converts node pool autoscaling config to a value that is used by GCP SDK. +func ConvertToSdkAutoscaling(autoscaling *NodePoolAutoScaling) *containerpb.NodePoolAutoscaling { + if autoscaling == nil { + return nil + } + sdkAutoscaling := containerpb.NodePoolAutoscaling{ + Enabled: true, // enable autoscaling by default + } + // set fields + if autoscaling.MinCount != nil { + sdkAutoscaling.TotalMinNodeCount = *autoscaling.MinCount + } + if autoscaling.MaxCount != nil { + sdkAutoscaling.TotalMaxNodeCount = *autoscaling.MaxCount + } + if autoscaling.EnableAutoscaling != nil { + sdkAutoscaling.Enabled = *autoscaling.EnableAutoscaling + } + if autoscaling.LocationPolicy != nil { + sdkAutoscaling.LocationPolicy = convertToSdkLocationPolicy(*autoscaling.LocationPolicy) + } else if sdkAutoscaling.Enabled { + // if location policy is not specified and autoscaling is enabled, default location policy to "any" + sdkAutoscaling.LocationPolicy = convertToSdkLocationPolicy(ManagedNodePoolLocationPolicyAny) + } + + return &sdkAutoscaling +} + +// ConvertFromSdkNodeVersion converts GCP SDK node version to k8s version. +func ConvertFromSdkNodeVersion(sdkNodeVersion string) string { + // For example, the node version returned from GCP SDK can be 1.27.2-gke.2100, we want to convert it to 1.27.2 + return strings.Split(sdkNodeVersion, "-")[0] +} diff --git a/exp/api/v1beta1/zz_generated.deepcopy.go b/exp/api/v1beta1/zz_generated.deepcopy.go index 1e2b33ec72..9fa2076917 100644 --- a/exp/api/v1beta1/zz_generated.deepcopy.go +++ b/exp/api/v1beta1/zz_generated.deepcopy.go @@ -319,6 +319,41 @@ func (in *GCPManagedMachinePoolList) DeepCopyObject() runtime.Object { // DeepCopyInto is an autogenerated deepcopy function, copying the receiver, writing into out. in must be non-nil. func (in *GCPManagedMachinePoolSpec) DeepCopyInto(out *GCPManagedMachinePoolSpec) { *out = *in + if in.MachineType != nil { + in, out := &in.MachineType, &out.MachineType + *out = new(string) + **out = **in + } + if in.DiskSizeGb != nil { + in, out := &in.DiskSizeGb, &out.DiskSizeGb + *out = new(int32) + **out = **in + } + if in.ServiceAccount != nil { + in, out := &in.ServiceAccount, &out.ServiceAccount + *out = new(string) + **out = **in + } + if in.ImageType != nil { + in, out := &in.ImageType, &out.ImageType + *out = new(string) + **out = **in + } + if in.LocalSsdCount != nil { + in, out := &in.LocalSsdCount, &out.LocalSsdCount + *out = new(int32) + **out = **in + } + if in.NetworkTags != nil { + in, out := &in.NetworkTags, &out.NetworkTags + *out = make([]string, len(*in)) + copy(*out, *in) + } + if in.DiskType != nil { + in, out := &in.DiskType, &out.DiskType + *out = new(string) + **out = **in + } if in.Scaling != nil { in, out := &in.Scaling, &out.Scaling *out = new(NodePoolAutoScaling) @@ -343,6 +378,16 @@ func (in *GCPManagedMachinePoolSpec) DeepCopyInto(out *GCPManagedMachinePoolSpec (*out)[key] = val } } + if in.Management != nil { + in, out := &in.Management, &out.Management + *out = new(NodePoolManagement) + **out = **in + } + if in.MaxPodsConstraint != nil { + in, out := &in.MaxPodsConstraint, &out.MaxPodsConstraint + *out = new(int64) + **out = **in + } if in.ProviderIDList != nil { in, out := &in.ProviderIDList, &out.ProviderIDList *out = make([]string, len(*in)) @@ -441,6 +486,16 @@ func (in *NodePoolAutoScaling) DeepCopyInto(out *NodePoolAutoScaling) { *out = new(int32) **out = **in } + if in.EnableAutoscaling != nil { + in, out := &in.EnableAutoscaling, &out.EnableAutoscaling + *out = new(bool) + **out = **in + } + if in.LocationPolicy != nil { + in, out := &in.LocationPolicy, &out.LocationPolicy + *out = new(ManagedNodePoolLocationPolicy) + **out = **in + } } // DeepCopy is an autogenerated deepcopy function, copying the receiver, creating a new NodePoolAutoScaling. @@ -453,6 +508,21 @@ func (in *NodePoolAutoScaling) DeepCopy() *NodePoolAutoScaling { return out } +// DeepCopyInto is an autogenerated deepcopy function, copying the receiver, writing into out. in must be non-nil. +func (in *NodePoolManagement) DeepCopyInto(out *NodePoolManagement) { + *out = *in +} + +// DeepCopy is an autogenerated deepcopy function, copying the receiver, creating a new NodePoolManagement. +func (in *NodePoolManagement) DeepCopy() *NodePoolManagement { + if in == nil { + return nil + } + out := new(NodePoolManagement) + in.DeepCopyInto(out) + return out +} + // DeepCopyInto is an autogenerated deepcopy function, copying the receiver, writing into out. in must be non-nil. func (in *Taint) DeepCopyInto(out *Taint) { *out = *in diff --git a/exp/controllers/gcpmanagedmachinepool_controller.go b/exp/controllers/gcpmanagedmachinepool_controller.go index cd314c87d5..47e2b81d4c 100644 --- a/exp/controllers/gcpmanagedmachinepool_controller.go +++ b/exp/controllers/gcpmanagedmachinepool_controller.go @@ -302,6 +302,7 @@ func (r *GCPManagedMachinePoolReconciler) Reconcile(ctx context.Context, req ctr // Always close the scope when exiting this function so we can persist any GCPMachine changes. defer func() { if err := managedMachinePoolScope.Close(); err != nil && reterr == nil { + log.Error(err, "Failed to patch GCPManagedMachinePool object", "GCPManagedMachinePool", managedMachinePoolScope.GCPManagedMachinePool.Name) reterr = err } }()