From 0e0e4741d3572505342cd2e2acf461aa9ee6a6ea Mon Sep 17 00:00:00 2001 From: Mulham Raee Date: Thu, 14 Mar 2024 13:33:05 +0100 Subject: [PATCH] add additionalTags API fied to ROSAMachinePool - use the correct typemetav1.Duration for NodeDrainGracePeriod field - fixed reconcileProviderIDList() listing terminated/stopped instances --- ...ure.cluster.x-k8s.io_rosamachinepools.yaml | 15 +++-- docs/book/src/crd/index.md | 66 ++++++++++++++++++- exp/api/v1beta2/rosamachinepool_types.go | 16 +++-- exp/api/v1beta2/rosamachinepool_webhook.go | 24 +++++++ exp/api/v1beta2/zz_generated.deepcopy.go | 13 ++++ exp/controllers/rosamachinepool_controller.go | 39 ++++++----- .../rosamachinepool_controller_test.go | 9 +++ 7 files changed, 155 insertions(+), 27 deletions(-) diff --git a/config/crd/bases/infrastructure.cluster.x-k8s.io_rosamachinepools.yaml b/config/crd/bases/infrastructure.cluster.x-k8s.io_rosamachinepools.yaml index 63af6ab635..699aa25701 100644 --- a/config/crd/bases/infrastructure.cluster.x-k8s.io_rosamachinepools.yaml +++ b/config/crd/bases/infrastructure.cluster.x-k8s.io_rosamachinepools.yaml @@ -53,6 +53,12 @@ spec: items: type: string type: array + additionalTags: + additionalProperties: + type: string + description: AdditionalTags are user-defined tags to be added on the + underlying EC2 instances associated with this machine pool. + type: object autoRepair: default: false description: AutoRepair specifies whether health checks should be @@ -83,14 +89,13 @@ spec: description: Labels specifies labels for the Kubernetes node objects type: object nodeDrainGracePeriod: - description: NodeDrainGracePeriod is grace period for how long Pod + description: "NodeDrainGracePeriod is grace period for how long Pod Disruption Budget-protected workloads will be respected during upgrades. After this grace period, any workloads protected by Pod Disruption Budgets that have not been successfully drained from a node will - be forcibly evicted. The nodeDrainGracePeriod can be defined in - minutes, hours ex; 30m, 10h. The max value can be assigned is 10080m|168h - (1 week). - pattern: ^(([0-9])+[m|h])$ + be forcibly evicted. \n Valid values are from 0 to 1 week(10080m|168h) + . 0 or empty value means that the MachinePool can be drained without + any time limitation." type: string nodePoolName: description: NodePoolName specifies the name of the nodepool in Rosa diff --git a/docs/book/src/crd/index.md b/docs/book/src/crd/index.md index f543796d61..d901e3f6b2 100644 --- a/docs/book/src/crd/index.md +++ b/docs/book/src/crd/index.md @@ -22558,7 +22558,7 @@ Tags

Tags (map[string]string alias)

-(Appears on:AWSClusterSpec, AWSMachineSpec, BuildParams, SecurityGroup, SubnetSpec, VPCSpec, AWSIAMRoleSpec, BootstrapUser, AWSIAMRoleSpec, BootstrapUser, AWSManagedControlPlaneSpec, OIDCIdentityProviderConfig, AWSManagedControlPlaneSpec, OIDCIdentityProviderConfig, RosaControlPlaneSpec, AWSMachinePoolSpec, AWSManagedMachinePoolSpec, AutoScalingGroup, FargateProfileSpec, AWSMachinePoolSpec, AWSManagedMachinePoolSpec, AutoScalingGroup, FargateProfileSpec) +(Appears on:AWSClusterSpec, AWSMachineSpec, BuildParams, SecurityGroup, SubnetSpec, VPCSpec, AWSIAMRoleSpec, BootstrapUser, AWSIAMRoleSpec, BootstrapUser, AWSManagedControlPlaneSpec, OIDCIdentityProviderConfig, AWSManagedControlPlaneSpec, OIDCIdentityProviderConfig, RosaControlPlaneSpec, AWSMachinePoolSpec, AWSManagedMachinePoolSpec, AutoScalingGroup, FargateProfileSpec, AWSMachinePoolSpec, AWSManagedMachinePoolSpec, AutoScalingGroup, FargateProfileSpec, RosaMachinePoolSpec)

Tags defines a map of tags.

@@ -25938,6 +25938,20 @@ map[string]string +additionalTags
+ + +Tags + + + + +(Optional) +

AdditionalTags are user-defined tags to be added on the underlying EC2 instances associated with this machine pool.

+ + + + autoRepair
bool @@ -26013,6 +26027,24 @@ with all node instances of the machine pool.

ProviderIDList contain a ProviderID for each machine instance that’s currently managed by this machine pool.

+ + +nodeDrainGracePeriod
+ + +Kubernetes meta/v1.Duration + + + + +(Optional) +

NodeDrainGracePeriod is grace period for how long Pod Disruption Budget-protected workloads will be +respected during upgrades. After this grace period, any workloads protected by Pod Disruption +Budgets that have not been successfully drained from a node will be forcibly evicted.

+

Valid values are from 0 to 1 week(10080m|168h) . +0 or empty value means that the MachinePool can be drained without any time limitation.

+ + @@ -26233,6 +26265,20 @@ map[string]string +additionalTags
+ + +Tags + + + + +(Optional) +

AdditionalTags are user-defined tags to be added on the underlying EC2 instances associated with this machine pool.

+ + + + autoRepair
bool @@ -26308,6 +26354,24 @@ with all node instances of the machine pool.

ProviderIDList contain a ProviderID for each machine instance that’s currently managed by this machine pool.

+ + +nodeDrainGracePeriod
+ + +Kubernetes meta/v1.Duration + + + + +(Optional) +

NodeDrainGracePeriod is grace period for how long Pod Disruption Budget-protected workloads will be +respected during upgrades. After this grace period, any workloads protected by Pod Disruption +Budgets that have not been successfully drained from a node will be forcibly evicted.

+

Valid values are from 0 to 1 week(10080m|168h) . +0 or empty value means that the MachinePool can be drained without any time limitation.

+ +

RosaMachinePoolStatus diff --git a/exp/api/v1beta2/rosamachinepool_types.go b/exp/api/v1beta2/rosamachinepool_types.go index d4e86ddb7b..8db3d5a380 100644 --- a/exp/api/v1beta2/rosamachinepool_types.go +++ b/exp/api/v1beta2/rosamachinepool_types.go @@ -20,6 +20,7 @@ import ( corev1 "k8s.io/api/core/v1" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + infrav1 "sigs.k8s.io/cluster-api-provider-aws/v2/api/v1beta2" clusterv1 "sigs.k8s.io/cluster-api/api/v1beta1" ) @@ -58,6 +59,11 @@ type RosaMachinePoolSpec struct { // +optional Taints []RosaTaint `json:"taints,omitempty"` + // AdditionalTags are user-defined tags to be added on the underlying EC2 instances associated with this machine pool. + // +immutable + // +optional + AdditionalTags infrav1.Tags `json:"additionalTags,omitempty"` + // AutoRepair specifies whether health checks should be enabled for machines // in the NodePool. The default is false. // +kubebuilder:default=false @@ -92,11 +98,13 @@ type RosaMachinePoolSpec struct { // NodeDrainGracePeriod is grace period for how long Pod Disruption Budget-protected workloads will be // respected during upgrades. After this grace period, any workloads protected by Pod Disruption - // Budgets that have not been successfully drained from a node will be forcibly evicted. The nodeDrainGracePeriod - // can be defined in minutes, hours ex; 30m, 10h. The max value can be assigned is 10080m|168h (1 week). - // +kubebuilder:validation:Pattern="^(([0-9])+[m|h])$" + // Budgets that have not been successfully drained from a node will be forcibly evicted. + // + // Valid values are from 0 to 1 week(10080m|168h) . + // 0 or empty value means that the MachinePool can be drained without any time limitation. + // // +optional - NodeDrainGracePeriod string `json:"nodeDrainGracePeriod,omitempty"` + NodeDrainGracePeriod *metav1.Duration `json:"nodeDrainGracePeriod,omitempty"` } // RosaTaint represents a taint to be applied to a node. diff --git a/exp/api/v1beta2/rosamachinepool_webhook.go b/exp/api/v1beta2/rosamachinepool_webhook.go index 6335d20a47..acae78576e 100644 --- a/exp/api/v1beta2/rosamachinepool_webhook.go +++ b/exp/api/v1beta2/rosamachinepool_webhook.go @@ -33,6 +33,12 @@ func (r *ROSAMachinePool) ValidateCreate() (warnings admission.Warnings, err err allErrs = append(allErrs, err) } + if err := r.validateNodeDrainGracePeriod(); err != nil { + allErrs = append(allErrs, err) + } + + allErrs = append(allErrs, r.Spec.AdditionalTags.Validate()...) + if len(allErrs) == 0 { return nil, nil } @@ -58,7 +64,12 @@ func (r *ROSAMachinePool) ValidateUpdate(old runtime.Object) (warnings admission allErrs = append(allErrs, err) } + if err := r.validateNodeDrainGracePeriod(); err != nil { + allErrs = append(allErrs, err) + } + allErrs = append(allErrs, validateImmutable(oldPool.Spec.AdditionalSecurityGroups, r.Spec.AdditionalSecurityGroups, "additionalSecurityGroups")...) + allErrs = append(allErrs, validateImmutable(oldPool.Spec.AdditionalTags, r.Spec.AdditionalTags, "additionalTags")...) if len(allErrs) == 0 { return nil, nil @@ -88,6 +99,19 @@ func (r *ROSAMachinePool) validateVersion() *field.Error { return nil } +func (r *ROSAMachinePool) validateNodeDrainGracePeriod() *field.Error { + if r.Spec.NodeDrainGracePeriod == nil { + return nil + } + + if r.Spec.NodeDrainGracePeriod.Minutes() > 10080 { + return field.Invalid(field.NewPath("spec.nodeDrainGracePeriod"), r.Spec.NodeDrainGracePeriod, + "max supported duration is 1 week (10080m|168h)") + } + + return nil +} + func validateImmutable(old, updated interface{}, name string) field.ErrorList { var allErrs field.ErrorList diff --git a/exp/api/v1beta2/zz_generated.deepcopy.go b/exp/api/v1beta2/zz_generated.deepcopy.go index 45f6a806ee..a916ebc059 100644 --- a/exp/api/v1beta2/zz_generated.deepcopy.go +++ b/exp/api/v1beta2/zz_generated.deepcopy.go @@ -21,6 +21,7 @@ limitations under the License. package v1beta2 import ( + "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/runtime" apiv1beta2 "sigs.k8s.io/cluster-api-provider-aws/v2/api/v1beta2" "sigs.k8s.io/cluster-api/api/v1beta1" @@ -1095,6 +1096,13 @@ func (in *RosaMachinePoolSpec) DeepCopyInto(out *RosaMachinePoolSpec) { *out = make([]RosaTaint, len(*in)) copy(*out, *in) } + if in.AdditionalTags != nil { + in, out := &in.AdditionalTags, &out.AdditionalTags + *out = make(apiv1beta2.Tags, len(*in)) + for key, val := range *in { + (*out)[key] = val + } + } if in.Autoscaling != nil { in, out := &in.Autoscaling, &out.Autoscaling *out = new(RosaMachinePoolAutoScaling) @@ -1115,6 +1123,11 @@ func (in *RosaMachinePoolSpec) DeepCopyInto(out *RosaMachinePoolSpec) { *out = make([]string, len(*in)) copy(*out, *in) } + if in.NodeDrainGracePeriod != nil { + in, out := &in.NodeDrainGracePeriod, &out.NodeDrainGracePeriod + *out = new(v1.Duration) + **out = **in + } } // DeepCopy is an autogenerated deepcopy function, copying the receiver, creating a new RosaMachinePoolSpec. diff --git a/exp/controllers/rosamachinepool_controller.go b/exp/controllers/rosamachinepool_controller.go index b8c29d1b56..f809f83b3f 100644 --- a/exp/controllers/rosamachinepool_controller.go +++ b/exp/controllers/rosamachinepool_controller.go @@ -14,6 +14,7 @@ import ( "github.com/pkg/errors" corev1 "k8s.io/api/core/v1" apierrors "k8s.io/apimachinery/pkg/api/errors" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/runtime/schema" "k8s.io/client-go/tools/record" "k8s.io/klog/v2" @@ -362,6 +363,7 @@ func (r *ROSAMachinePoolReconciler) updateNodePool(machinePoolScope *scope.RosaM // zero-out fields that shouldn't be part of the update call. desiredSpec.Version = "" desiredSpec.AdditionalSecurityGroups = nil + desiredSpec.AdditionalTags = nil npBuilder := nodePoolBuilder(*desiredSpec, machinePoolScope.MachinePool.Spec) nodePoolSpec, err := npBuilder.Build() @@ -401,18 +403,6 @@ func validateMachinePoolSpec(machinePoolScope *scope.RosaMachinePoolScope) (*str return &message, nil } - if machinePoolScope.RosaMachinePool.Spec.NodeDrainGracePeriod != "" { - nodeDrainDuration, err := time.ParseDuration(machinePoolScope.RosaMachinePool.Spec.NodeDrainGracePeriod) - if err != nil { - return nil, err - } - // Check if node grace period duration is > 10080m (168h - 1 week) - if nodeDrainDuration.Minutes() > 10080 { - message := "Max supported NodeDrainGracePeriod duration is 10080m|168h (1 week)" - return &message, nil - } - } - // TODO: add more input validations return nil, nil } @@ -456,16 +446,17 @@ func nodePoolBuilder(rosaMachinePoolSpec expinfrav1.RosaMachinePoolSpec, machine if rosaMachinePoolSpec.AdditionalSecurityGroups != nil { awsNodePool = awsNodePool.AdditionalSecurityGroupIds(rosaMachinePoolSpec.AdditionalSecurityGroups...) } + if rosaMachinePoolSpec.AdditionalTags != nil { + awsNodePool = awsNodePool.Tags(rosaMachinePoolSpec.AdditionalTags) + } npBuilder.AWSNodePool(awsNodePool) if rosaMachinePoolSpec.Version != "" { npBuilder.Version(cmv1.NewVersion().ID(ocm.CreateVersionID(rosaMachinePoolSpec.Version, ocm.DefaultChannelGroup))) } - if rosaMachinePoolSpec.NodeDrainGracePeriod != "" { - duration, _ := time.ParseDuration(rosaMachinePoolSpec.NodeDrainGracePeriod) - valueBuilder := cmv1.NewValue() - valueBuilder.Value(duration.Minutes()).Unit("minutes") + if rosaMachinePoolSpec.NodeDrainGracePeriod != nil { + valueBuilder := cmv1.NewValue().Value(rosaMachinePoolSpec.NodeDrainGracePeriod.Minutes()).Unit("minutes") npBuilder.NodeDrainGracePeriod(valueBuilder) } @@ -479,6 +470,7 @@ func nodePoolToRosaMachinePoolSpec(nodePool *cmv1.NodePool) expinfrav1.RosaMachi AvailabilityZone: nodePool.AvailabilityZone(), Subnet: nodePool.Subnet(), Labels: nodePool.Labels(), + AdditionalTags: nodePool.AWSNodePool().Tags(), AutoRepair: nodePool.AutoRepair(), InstanceType: nodePool.AWSNodePool().InstanceType(), TuningConfigs: nodePool.TuningConfigs(), @@ -502,6 +494,11 @@ func nodePoolToRosaMachinePoolSpec(nodePool *cmv1.NodePool) expinfrav1.RosaMachi } spec.Taints = rosaTaints } + if nodePool.NodeDrainGracePeriod() != nil { + spec.NodeDrainGracePeriod = &metav1.Duration{ + Duration: time.Minute * time.Duration(nodePool.NodeDrainGracePeriod().Value()), + } + } return spec } @@ -534,7 +531,7 @@ func (r *ROSAMachinePoolReconciler) reconcileProviderIDList(ctx context.Context, } func buildEC2FiltersFromTags(tags map[string]string) []*ec2.Filter { - filters := make([]*ec2.Filter, len(tags)) + filters := make([]*ec2.Filter, len(tags)+1) for key, value := range tags { filters = append(filters, &ec2.Filter{ Name: ptr.To(fmt.Sprintf("tag:%s", key)), @@ -544,6 +541,14 @@ func buildEC2FiltersFromTags(tags map[string]string) []*ec2.Filter { }) } + // only list instances that are running or just started + filters = append(filters, &ec2.Filter{ + Name: ptr.To("instance-state-name"), + Values: aws.StringSlice([]string{ + "running", "pending", + }), + }) + return filters } diff --git a/exp/controllers/rosamachinepool_controller_test.go b/exp/controllers/rosamachinepool_controller_test.go index 5df3df6f4b..58f1963ed4 100644 --- a/exp/controllers/rosamachinepool_controller_test.go +++ b/exp/controllers/rosamachinepool_controller_test.go @@ -2,10 +2,13 @@ package controllers import ( "testing" + "time" . "github.com/onsi/gomega" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/utils/ptr" + infrav1 "sigs.k8s.io/cluster-api-provider-aws/v2/api/v1beta2" expinfrav1 "sigs.k8s.io/cluster-api-provider-aws/v2/exp/api/v1beta2" expclusterv1 "sigs.k8s.io/cluster-api/exp/api/v1beta1" ) @@ -20,6 +23,12 @@ func TestNodePoolToRosaMachinePoolSpec(t *testing.T) { AutoRepair: true, InstanceType: "m5.large", TuningConfigs: []string{"config1"}, + NodeDrainGracePeriod: &metav1.Duration{ + Duration: time.Minute * 10, + }, + AdditionalTags: infrav1.Tags{ + "tag1": "value1", + }, } machinePoolSpec := expclusterv1.MachinePoolSpec{