diff --git a/cloud/scope/managedmachinepool.go b/cloud/scope/managedmachinepool.go index c666814237..48dd0520cb 100644 --- a/cloud/scope/managedmachinepool.go +++ b/cloud/scope/managedmachinepool.go @@ -22,6 +22,7 @@ import ( "strings" "k8s.io/utils/pointer" + 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" @@ -155,34 +156,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), ShieldedInstanceConfig: &containerpb.ShieldedInstanceConfig{ EnableSecureBoot: pointer.BoolDeref(nodePool.Spec.NodeSecurity.EnableSecureBoot, false), EnableIntegrityMonitoring: pointer.BoolDeref(nodePool.Spec.NodeSecurity.EnableIntegrityMonitoring, false), }, + ResourceLabels: NodePoolResourceLabels(nodePool.Spec.AdditionalLabels, clusterName), }, } + 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.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 = string(*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.MaxPodsPerNode != nil { + sdkNodePool.MaxPodsConstraint = &containerpb.MaxPodsConstraint{ + MaxPodsPerNode: *nodePool.Spec.MaxPodsPerNode, } } if nodePool.Spec.InstanceType != nil { @@ -234,10 +269,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..039e56f507 --- /dev/null +++ b/cloud/scope/managedmachinepool_test.go @@ -0,0 +1,138 @@ +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), + ShieldedInstanceConfig: &containerpb.ShieldedInstanceConfig{}, + }, + })) + }) + + 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), + ShieldedInstanceConfig: &containerpb.ShieldedInstanceConfig{}, + }, + })) + }) + + It("should convert to SDK node pool using GCPManagedMachinePool", func() { + machineType := "n1-standard-1" + diskSizeGb := int32(128) + imageType := "ubuntu_containerd" + localSsdCount := int32(2) + diskType := v1beta1.SSD + maxPodsPerNode := 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", + }, + } + resourceLabels := infrav1.Labels{"test-key": "test-value"} + + TestGCPMMP.Spec.MachineType = &machineType + TestGCPMMP.Spec.DiskSizeGb = &diskSizeGb + TestGCPMMP.Spec.ImageType = &imageType + TestGCPMMP.Spec.LocalSsdCount = &localSsdCount + TestGCPMMP.Spec.DiskType = &diskType + TestGCPMMP.Spec.Scaling = &scaling + TestGCPMMP.Spec.MaxPodsPerNode = &maxPodsPerNode + TestGCPMMP.Spec.KubernetesLabels = labels + TestGCPMMP.Spec.KubernetesTaints = taints + 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), + MachineType: machineType, + DiskSizeGb: diskSizeGb, + ImageType: imageType, + LocalSsdCount: localSsdCount, + DiskType: string(diskType), + ShieldedInstanceConfig: &containerpb.ShieldedInstanceConfig{}, + }, + Autoscaling: v1beta1.ConvertToSdkAutoscaling(&scaling), + MaxPodsConstraint: &containerpb.MaxPodsConstraint{ + MaxPodsPerNode: maxPodsPerNode, + }, + })) + }) + }) +}) 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 88f42f4821..9c12598621 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 60c835ca4d..6ea2d278fd 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" @@ -44,20 +45,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()) @@ -74,7 +87,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 } @@ -84,53 +96,59 @@ 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 } - needUpdateNodePool, nodePoolUpdateNodePool := s.checkDiffAndPrepareUpdateNodePool(nodePool) - if needUpdateNodePool { + + needUpdateConfig, nodePoolUpdateConfigRequest := s.checkDiffAndPrepareUpdateConfig(nodePool) + if needUpdateConfig { log.Info("Node pool config update required") - err = s.updateNodePool(ctx, nodePoolUpdateNodePool) + err = s.updateNodePoolConfig(ctx, nodePoolUpdateConfigRequest) if err != nil { return ctrl.Result{}, fmt.Errorf("node pool config update (either version/labels/taints/locations/image type/network tag or all) failed: %s", err) } @@ -148,7 +166,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 +178,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 +199,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 +232,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 +296,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,8 +307,8 @@ func (s *Service) createNodePool(ctx context.Context, log *logr.Logger) error { return nil } -func (s *Service) updateNodePoolAutoscaling(ctx context.Context, setNodePoolAutoscalingRequest *containerpb.SetNodePoolAutoscalingRequest) error { - _, err := s.scope.ManagedMachinePoolClient().SetNodePoolAutoscaling(ctx, setNodePoolAutoscalingRequest) +func (s *Service) updateNodePoolConfig(ctx context.Context, updateNodePoolRequest *containerpb.UpdateNodePoolRequest) error { + _, err := s.scope.ManagedMachinePoolClient().UpdateNodePool(ctx, updateNodePoolRequest) if err != nil { return err } @@ -299,8 +316,8 @@ func (s *Service) updateNodePoolAutoscaling(ctx context.Context, setNodePoolAuto return nil } -func (s *Service) updateNodePoolSize(ctx context.Context, setNodePoolSizeRequest *containerpb.SetNodePoolSizeRequest) error { - _, err := s.scope.ManagedMachinePoolClient().SetNodePoolSize(ctx, setNodePoolSizeRequest) +func (s *Service) updateNodePoolAutoscaling(ctx context.Context, setNodePoolAutoscalingRequest *containerpb.SetNodePoolAutoscalingRequest) error { + _, err := s.scope.ManagedMachinePoolClient().SetNodePoolAutoscaling(ctx, setNodePoolAutoscalingRequest) if err != nil { return err } @@ -308,8 +325,8 @@ func (s *Service) updateNodePoolSize(ctx context.Context, setNodePoolSizeRequest return nil } -func (s *Service) updateNodePool(ctx context.Context, updateNodePoolRequest *containerpb.UpdateNodePoolRequest) error { - _, err := s.scope.ManagedMachinePoolClient().UpdateNodePool(ctx, updateNodePoolRequest) +func (s *Service) updateNodePoolSize(ctx context.Context, setNodePoolSizeRequest *containerpb.SetNodePoolSizeRequest) error { + _, err := s.scope.ManagedMachinePoolClient().SetNodePoolSize(ctx, setNodePoolSizeRequest) if err != nil { return err } @@ -329,29 +346,52 @@ func (s *Service) deleteNodePool(ctx context.Context) error { return nil } -func (s *Service) checkDiffAndPrepareUpdateNodePool(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 !cmp.Equal(map[string]string(s.scope.GCPManagedMachinePool.Spec.KubernetesLabels), existingNodePool.Config.Labels) { + if !cmp.Equal(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 !cmp.Equal(desiredKubernetesTaints, existingNodePool.Config.Taints) { + if !cmp.Equal(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, } } // Locations @@ -380,23 +420,12 @@ func (s *Service) checkDiffAndPrepareUpdateNodePool(existingNodePool *containerp 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 } @@ -405,6 +434,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 28678feeb2..d3d7150768 100644 --- a/config/crd/bases/infrastructure.cluster.x-k8s.io_gcpmanagedmachinepools.yaml +++ b/config/crd/bases/infrastructure.cluster.x-k8s.io_gcpmanagedmachinepools.yaml @@ -19,8 +19,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: @@ -56,6 +59,12 @@ spec: format: int64 minimum: 10 type: integer + 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 type of the disk attached to each node. enum: @@ -100,6 +109,32 @@ 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: + description: Management specifies the node pool management options. + 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 maxPodsPerNode: description: MaxPodsPerNode is constraint enforced on the max num of pods per node. @@ -185,10 +220,24 @@ 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 @@ -244,6 +293,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 68cc0207b4..b943ff6c68 100644 --- a/exp/api/v1beta1/gcpmanagedmachinepool_types.go +++ b/exp/api/v1beta1/gcpmanagedmachinepool_types.go @@ -47,6 +47,18 @@ 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"` + // LocalSsdCount is the number of local SSD disks to be attached to the node. + // +optional + LocalSsdCount *int32 `json:"localSsdCount,omitempty"` // Scaling specifies scaling for the node pool // +optional Scaling *NodePoolAutoScaling `json:"scaling,omitempty"` @@ -91,6 +103,9 @@ type GCPManagedMachinePoolSpec struct { // ones added by default. // +optional AdditionalLabels infrav1.Labels `json:"additionalLabels,omitempty"` + // Management specifies the node pool management options. + // +optional + Management *NodePoolManagement `json:"management,omitempty"` // ProviderIDList are the provider IDs of instances in the // managed instance group corresponding to the nodegroup represented by this // machine pool @@ -115,7 +130,7 @@ type NodeNetworkConfig struct { // PodRangeCidrBlock is the IP address range for pod IPs in // this node pool. // +optional - PodRangeCidrBlock *string `json:"podRangeCidrBlock"` + PodRangeCidrBlock *string `json:"podRangeCidrBlock,omitempty"` } // NodeSecurityConfig encapsulates node security configurations. @@ -151,6 +166,8 @@ type ServiceAccountConfig 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 @@ -160,10 +177,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 { @@ -185,10 +203,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 557fb8f5e1..33b2e1821a 100644 --- a/exp/api/v1beta1/gcpmanagedmachinepool_webhook.go +++ b/exp/api/v1beta1/gcpmanagedmachinepool_webhook.go @@ -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,58 @@ 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.MaxPodsPerNode, "maxPodsPerNode", &allErrs) + appendErrorIfNegative(r.Spec.LocalSsdCount, "localSsdCount", &allErrs) + + return allErrs +} + +func appendErrorIfMutated(old, update interface{}, name string, errs *field.ErrorList) { + if !cmp.Equal(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.InstanceType, r.Spec.InstanceType, "instanceType", &allErrs) + appendErrorIfMutated(old.Spec.NodePoolName, r.Spec.NodePoolName, "nodePoolName", &allErrs) + appendErrorIfMutated(old.Spec.MachineType, r.Spec.MachineType, "machineType", &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.MaxPodsPerNode, r.Spec.MaxPodsPerNode, "maxPodsPerNode", &allErrs) + appendErrorIfMutated(old.Spec.NodeNetwork.PodRangeName, r.Spec.NodeNetwork.PodRangeName, "podRangeName", &allErrs) + appendErrorIfMutated(old.Spec.NodeNetwork.CreatePodRange, r.Spec.NodeNetwork.CreatePodRange, "createPodRange", &allErrs) + appendErrorIfMutated(old.Spec.NodeNetwork.PodRangeCidrBlock, r.Spec.NodeNetwork.PodRangeCidrBlock, "podRangeCidrBlock", &allErrs) + appendErrorIfMutated(old.Spec.NodeSecurity, r.Spec.NodeSecurity, "nodeSecurity", &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,98 +191,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 !cmp.Equal(r.Spec.InstanceType, old.Spec.InstanceType) { - allErrs = append(allErrs, - field.Invalid(field.NewPath("spec", "InstanceType"), - r.Spec.NodePoolName, "field is immutable"), - ) - } - - if !cmp.Equal(r.Spec.DiskSizeGB, old.Spec.DiskSizeGB) { - allErrs = append(allErrs, - field.Invalid(field.NewPath("spec", "DiskSizeGB"), - r.Spec.NodePoolName, "field is immutable"), - ) - } - - if !cmp.Equal(r.Spec.MaxPodsPerNode, old.Spec.MaxPodsPerNode) { - allErrs = append(allErrs, - field.Invalid(field.NewPath("spec", "MaxPodsPerNode"), - r.Spec.NodePoolName, "field is immutable"), - ) - } - - if !cmp.Equal(r.Spec.NodeNetwork.CreatePodRange, old.Spec.NodeNetwork.CreatePodRange) { - allErrs = append(allErrs, - field.Invalid(field.NewPath("spec", "NodeNetwork", "CreatePodRange"), - r.Spec.NodePoolName, "field is immutable"), - ) - } - - if !cmp.Equal(r.Spec.NodeNetwork.PodRangeName, old.Spec.NodeNetwork.PodRangeName) { - allErrs = append(allErrs, - field.Invalid(field.NewPath("spec", "NodeNetwork", "PodRangeName"), - r.Spec.NodePoolName, "field is immutable"), - ) - } - - if !cmp.Equal(r.Spec.NodeNetwork.PodRangeCidrBlock, old.Spec.NodeNetwork.PodRangeCidrBlock) { - allErrs = append(allErrs, - field.Invalid(field.NewPath("spec", "NodeNetwork", "PodRangeCidrBlock"), - r.Spec.NodePoolName, "field is immutable"), - ) - } - - if !cmp.Equal(r.Spec.NodeSecurity.ServiceAccount.Email, old.Spec.NodeSecurity.ServiceAccount.Email) { - allErrs = append(allErrs, - field.Invalid(field.NewPath("spec", "NodeSecurity", "ServiceAccount", "Email"), - r.Spec.NodePoolName, "field is immutable"), - ) - } - - if !cmp.Equal(r.Spec.NodeSecurity.ServiceAccount.Scopes, old.Spec.NodeSecurity.ServiceAccount.Scopes) { - allErrs = append(allErrs, - field.Invalid(field.NewPath("spec", "NodeSecurity", "ServiceAccount", "Scopes"), - r.Spec.NodePoolName, "field is immutable"), - ) - } - - if !cmp.Equal(r.Spec.NodeSecurity.SandboxType, old.Spec.NodeSecurity.SandboxType) { - allErrs = append(allErrs, - field.Invalid(field.NewPath("spec", "NodeSecurity", "SandboxType"), - r.Spec.NodePoolName, "field is immutable"), - ) - } - - if !cmp.Equal(r.Spec.NodeSecurity.EnableSecureBoot, old.Spec.NodeSecurity.EnableSecureBoot) { - allErrs = append(allErrs, - field.Invalid(field.NewPath("spec", "NodeSecurity", "EnableSecureBoot"), - r.Spec.NodePoolName, "field is immutable"), - ) - } - - if !cmp.Equal(r.Spec.NodeSecurity.EnableIntegrityMonitoring, old.Spec.NodeSecurity.EnableIntegrityMonitoring) { - allErrs = append(allErrs, - field.Invalid(field.NewPath("spec", "NodeSecurity", "EnableIntegrityMonitoring"), - r.Spec.NodePoolName, "field is immutable"), - ) - } - - if !cmp.Equal(r.Spec.AdditionalLabels, old.Spec.AdditionalLabels) { - allErrs = append(allErrs, - field.Invalid(field.NewPath("spec", "AdditionalLabels"), - 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..c398dcd104 --- /dev/null +++ b/exp/api/v1beta1/gcpmanagedmachinepool_webhook_test.go @@ -0,0 +1,166 @@ +/* +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" + diskSizeGb := int32(100) + + gcpmmp = &GCPManagedMachinePool{ + Spec: GCPManagedMachinePoolSpec{ + NodePoolName: "test-gke-pool", + MachineType: &machineType, + 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.MaxPodsPerNode = &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.MaxPodsPerNode = &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 23058ca922..6ae3297ff2 100644 --- a/exp/api/v1beta1/zz_generated.deepcopy.go +++ b/exp/api/v1beta1/zz_generated.deepcopy.go @@ -324,6 +324,21 @@ 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.LocalSsdCount != nil { + in, out := &in.LocalSsdCount, &out.LocalSsdCount + *out = new(int32) + **out = **in + } if in.Scaling != nil { in, out := &in.Scaling, &out.Scaling *out = new(NodePoolAutoScaling) @@ -380,6 +395,11 @@ 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.ProviderIDList != nil { in, out := &in.ProviderIDList, &out.ProviderIDList *out = make([]string, len(*in)) @@ -513,6 +533,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. @@ -525,6 +555,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 *NodeSecurityConfig) DeepCopyInto(out *NodeSecurityConfig) { *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 } }()