add additionalTags API fied to ROSAMachinePool

- use the correct typemetav1.Duration for NodeDrainGracePeriod field
- fixed reconcileProviderIDList() listing terminated/stopped instances

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

docs/book/src/crd/index.md

@@ -22558,7 +22558,7 @@ Tags
 <h3 id="infrastructure.cluster.x-k8s.io/v1beta2.Tags">Tags
 (<code>map[string]string</code> alias)</p></h3>
 <p>
-(<em>Appears on:</em><a href="#infrastructure.cluster.x-k8s.io/v1beta2.AWSClusterSpec">AWSClusterSpec</a>, <a href="#infrastructure.cluster.x-k8s.io/v1beta2.AWSMachineSpec">AWSMachineSpec</a>, <a href="#infrastructure.cluster.x-k8s.io/v1beta2.BuildParams">BuildParams</a>, <a href="#infrastructure.cluster.x-k8s.io/v1beta2.SecurityGroup">SecurityGroup</a>, <a href="#infrastructure.cluster.x-k8s.io/v1beta2.SubnetSpec">SubnetSpec</a>, <a href="#infrastructure.cluster.x-k8s.io/v1beta2.VPCSpec">VPCSpec</a>, <a href="#bootstrap.aws.infrastructure.cluster.x-k8s.io/v1alpha1.AWSIAMRoleSpec">AWSIAMRoleSpec</a>, <a href="#bootstrap.aws.infrastructure.cluster.x-k8s.io/v1alpha1.BootstrapUser">BootstrapUser</a>, <a href="#bootstrap.aws.infrastructure.cluster.x-k8s.io/v1beta1.AWSIAMRoleSpec">AWSIAMRoleSpec</a>, <a href="#bootstrap.aws.infrastructure.cluster.x-k8s.io/v1beta1.BootstrapUser">BootstrapUser</a>, <a href="#controlplane.cluster.x-k8s.io/v1beta1.AWSManagedControlPlaneSpec">AWSManagedControlPlaneSpec</a>, <a href="#controlplane.cluster.x-k8s.io/v1beta1.OIDCIdentityProviderConfig">OIDCIdentityProviderConfig</a>, <a href="#controlplane.cluster.x-k8s.io/v1beta2.AWSManagedControlPlaneSpec">AWSManagedControlPlaneSpec</a>, <a href="#controlplane.cluster.x-k8s.io/v1beta2.OIDCIdentityProviderConfig">OIDCIdentityProviderConfig</a>, <a href="#controlplane.cluster.x-k8s.io/v1beta2.RosaControlPlaneSpec">RosaControlPlaneSpec</a>, <a href="#infrastructure.cluster.x-k8s.io/v1beta1.AWSMachinePoolSpec">AWSMachinePoolSpec</a>, <a href="#infrastructure.cluster.x-k8s.io/v1beta1.AWSManagedMachinePoolSpec">AWSManagedMachinePoolSpec</a>, <a href="#infrastructure.cluster.x-k8s.io/v1beta1.AutoScalingGroup">AutoScalingGroup</a>, <a href="#infrastructure.cluster.x-k8s.io/v1beta1.FargateProfileSpec">FargateProfileSpec</a>, <a href="#infrastructure.cluster.x-k8s.io/v1beta2.AWSMachinePoolSpec">AWSMachinePoolSpec</a>, <a href="#infrastructure.cluster.x-k8s.io/v1beta2.AWSManagedMachinePoolSpec">AWSManagedMachinePoolSpec</a>, <a href="#infrastructure.cluster.x-k8s.io/v1beta2.AutoScalingGroup">AutoScalingGroup</a>, <a href="#infrastructure.cluster.x-k8s.io/v1beta2.FargateProfileSpec">FargateProfileSpec</a>)
+(<em>Appears on:</em><a href="#infrastructure.cluster.x-k8s.io/v1beta2.AWSClusterSpec">AWSClusterSpec</a>, <a href="#infrastructure.cluster.x-k8s.io/v1beta2.AWSMachineSpec">AWSMachineSpec</a>, <a href="#infrastructure.cluster.x-k8s.io/v1beta2.BuildParams">BuildParams</a>, <a href="#infrastructure.cluster.x-k8s.io/v1beta2.SecurityGroup">SecurityGroup</a>, <a href="#infrastructure.cluster.x-k8s.io/v1beta2.SubnetSpec">SubnetSpec</a>, <a href="#infrastructure.cluster.x-k8s.io/v1beta2.VPCSpec">VPCSpec</a>, <a href="#bootstrap.aws.infrastructure.cluster.x-k8s.io/v1alpha1.AWSIAMRoleSpec">AWSIAMRoleSpec</a>, <a href="#bootstrap.aws.infrastructure.cluster.x-k8s.io/v1alpha1.BootstrapUser">BootstrapUser</a>, <a href="#bootstrap.aws.infrastructure.cluster.x-k8s.io/v1beta1.AWSIAMRoleSpec">AWSIAMRoleSpec</a>, <a href="#bootstrap.aws.infrastructure.cluster.x-k8s.io/v1beta1.BootstrapUser">BootstrapUser</a>, <a href="#controlplane.cluster.x-k8s.io/v1beta1.AWSManagedControlPlaneSpec">AWSManagedControlPlaneSpec</a>, <a href="#controlplane.cluster.x-k8s.io/v1beta1.OIDCIdentityProviderConfig">OIDCIdentityProviderConfig</a>, <a href="#controlplane.cluster.x-k8s.io/v1beta2.AWSManagedControlPlaneSpec">AWSManagedControlPlaneSpec</a>, <a href="#controlplane.cluster.x-k8s.io/v1beta2.OIDCIdentityProviderConfig">OIDCIdentityProviderConfig</a>, <a href="#controlplane.cluster.x-k8s.io/v1beta2.RosaControlPlaneSpec">RosaControlPlaneSpec</a>, <a href="#infrastructure.cluster.x-k8s.io/v1beta1.AWSMachinePoolSpec">AWSMachinePoolSpec</a>, <a href="#infrastructure.cluster.x-k8s.io/v1beta1.AWSManagedMachinePoolSpec">AWSManagedMachinePoolSpec</a>, <a href="#infrastructure.cluster.x-k8s.io/v1beta1.AutoScalingGroup">AutoScalingGroup</a>, <a href="#infrastructure.cluster.x-k8s.io/v1beta1.FargateProfileSpec">FargateProfileSpec</a>, <a href="#infrastructure.cluster.x-k8s.io/v1beta2.AWSMachinePoolSpec">AWSMachinePoolSpec</a>, <a href="#infrastructure.cluster.x-k8s.io/v1beta2.AWSManagedMachinePoolSpec">AWSManagedMachinePoolSpec</a>, <a href="#infrastructure.cluster.x-k8s.io/v1beta2.AutoScalingGroup">AutoScalingGroup</a>, <a href="#infrastructure.cluster.x-k8s.io/v1beta2.FargateProfileSpec">FargateProfileSpec</a>, <a href="#infrastructure.cluster.x-k8s.io/v1beta2.RosaMachinePoolSpec">RosaMachinePoolSpec</a>)
 </p>
 <p>
 <p>Tags defines a map of tags.</p>
@@ -25938,6 +25938,20 @@ map[string]string
 </tr>
 <tr>
 <td>
+<code>additionalTags</code><br/>
+<em>
+<a href="#infrastructure.cluster.x-k8s.io/v1beta2.Tags">
+Tags
+</a>
+</em>
+</td>
+<td>
+<em>(Optional)</em>
+<p>AdditionalTags are user-defined tags to be added on the underlying EC2 instances associated with this machine pool.</p>
+</td>
+</tr>
+<tr>
+<td>
 <code>autoRepair</code><br/>
 <em>
 bool
@@ -26013,6 +26027,24 @@ with all node instances of the machine pool.</p>
 <p>ProviderIDList contain a ProviderID for each machine instance that&rsquo;s currently managed by this machine pool.</p>
 </td>
 </tr>
+<tr>
+<td>
+<code>nodeDrainGracePeriod</code><br/>
+<em>
+<a href="https://pkg.go.dev/k8s.io/apimachinery/pkg/apis/meta/v1#Duration">
+Kubernetes meta/v1.Duration
+</a>
+</em>
+</td>
+<td>
+<em>(Optional)</em>
+<p>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.</p>
+<p>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.</p>
+</td>
+</tr>
 </table>
 </td>
 </tr>
@@ -26233,6 +26265,20 @@ map[string]string
 </tr>
 <tr>
 <td>
+<code>additionalTags</code><br/>
+<em>
+<a href="#infrastructure.cluster.x-k8s.io/v1beta2.Tags">
+Tags
+</a>
+</em>
+</td>
+<td>
+<em>(Optional)</em>
+<p>AdditionalTags are user-defined tags to be added on the underlying EC2 instances associated with this machine pool.</p>
+</td>
+</tr>
+<tr>
+<td>
 <code>autoRepair</code><br/>
 <em>
 bool
@@ -26308,6 +26354,24 @@ with all node instances of the machine pool.</p>
 <p>ProviderIDList contain a ProviderID for each machine instance that&rsquo;s currently managed by this machine pool.</p>
 </td>
 </tr>
+<tr>
+<td>
+<code>nodeDrainGracePeriod</code><br/>
+<em>
+<a href="https://pkg.go.dev/k8s.io/apimachinery/pkg/apis/meta/v1#Duration">
+Kubernetes meta/v1.Duration
+</a>
+</em>
+</td>
+<td>
+<em>(Optional)</em>
+<p>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.</p>
+<p>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.</p>
+</td>
+</tr>
 </tbody>
 </table>
 <h3 id="infrastructure.cluster.x-k8s.io/v1beta2.RosaMachinePoolStatus">RosaMachinePoolStatus

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.

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
 

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
 }
 

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{