From e341dd1f866a139462cf47f6b00a0a9552f0a828 Mon Sep 17 00:00:00 2001 From: Lubron Zhan Date: Thu, 1 May 2025 16:19:38 -0700 Subject: [PATCH 1/7] Introduce NodeDeletionStrategy to allow drain node when deleting cluster Signed-off-by: Lubron Zhan --- api/v1beta2/cluster_types.go | 15 ++++++ .../controllers/machine/machine_controller.go | 4 +- .../machine/machine_controller_test.go | 53 +++++++++++++++++++ 3 files changed, 70 insertions(+), 2 deletions(-) diff --git a/api/v1beta2/cluster_types.go b/api/v1beta2/cluster_types.go index c48ff2f8e36e..09ba6d0edbec 100644 --- a/api/v1beta2/cluster_types.go +++ b/api/v1beta2/cluster_types.go @@ -585,6 +585,14 @@ type Topology struct { // +listMapKey=name // +kubebuilder:validation:MaxItems=1000 Variables []ClusterVariable `json:"variables,omitempty"` + + // nodeDeletionStrategy specifies the strategy to delete nodes in the cluster. + // Avilable options: + // - graceful + // - force + // By default it's "force" + // +optional + NodeDeletionStrategy *NodeDrainStrategyType `json:"nodeDrainStrategy,omitempty"` } // ControlPlaneTopology specifies the parameters for the control plane nodes in the cluster. @@ -901,6 +909,13 @@ type MachinePoolVariables struct { Overrides []ClusterVariable `json:"overrides,omitempty"` } +type NodeDrainStrategyType string + +const ( + NodeDrainStrategyForce NodeDrainStrategyType = "force" + NodeDrainStrategyGracefulWithTimeout NodeDrainStrategyType = "gracefulWithTimeout" +) + // ANCHOR_END: ClusterSpec // ANCHOR: ClusterNetwork diff --git a/internal/controllers/machine/machine_controller.go b/internal/controllers/machine/machine_controller.go index 4456c45dad23..bff3fe2f6842 100644 --- a/internal/controllers/machine/machine_controller.go +++ b/internal/controllers/machine/machine_controller.go @@ -714,8 +714,8 @@ func (r *Reconciler) nodeVolumeDetachTimeoutExceeded(machine *clusterv1.Machine) // and if the Machine is not the last control plane node in the cluster. func (r *Reconciler) isDeleteNodeAllowed(ctx context.Context, cluster *clusterv1.Cluster, machine *clusterv1.Machine) error { log := ctrl.LoggerFrom(ctx) - // Return early if the cluster is being deleted. - if !cluster.DeletionTimestamp.IsZero() { + // Return early if the cluster is being deleted and cluster's nodeDeletionStrategy is not set or set to `force`. + if !cluster.DeletionTimestamp.IsZero() && (cluster.Spec.Topology.NodeDeletionStrategy == nil || *cluster.Spec.Topology.NodeDeletionStrategy == clusterv1.NodeDrainStrategyForce) { return errClusterIsBeingDeleted } diff --git a/internal/controllers/machine/machine_controller_test.go b/internal/controllers/machine/machine_controller_test.go index ef164675515a..0c9510a706e5 100644 --- a/internal/controllers/machine/machine_controller_test.go +++ b/internal/controllers/machine/machine_controller_test.go @@ -2668,6 +2668,59 @@ func TestIsDeleteNodeAllowed(t *testing.T) { machine: &clusterv1.Machine{}, expectedError: errClusterIsBeingDeleted, }, + { + name: "has nodeRef and cluster is being deleted, nodeDeletionStrategy set to force", + cluster: &clusterv1.Cluster{ + ObjectMeta: metav1.ObjectMeta{ + Name: "test-cluster", + Namespace: metav1.NamespaceDefault, + DeletionTimestamp: &deletionts, + Finalizers: []string{clusterv1.ClusterFinalizer}, + }, + Spec: clusterv1.ClusterSpec{ + Topology: &clusterv1.Topology{ + NodeDeletionStrategy: ptr.To(clusterv1.NodeDrainStrategyForce), + }, + }, + }, + machine: &clusterv1.Machine{}, + expectedError: errClusterIsBeingDeleted, + }, + { + name: "has nodeRef and control plane is healthy, with nodeDeletionStrategy set to graceful", + cluster: &clusterv1.Cluster{ + ObjectMeta: metav1.ObjectMeta{ + Name: "test-cluster", + Namespace: metav1.NamespaceDefault, + }, + Spec: clusterv1.ClusterSpec{ + Topology: &clusterv1.Topology{ + NodeDeletionStrategy: ptr.To(clusterv1.NodeDrainStrategyForce), + }, + }, + }, + machine: &clusterv1.Machine{ + ObjectMeta: metav1.ObjectMeta{ + Name: "created", + Namespace: metav1.NamespaceDefault, + Labels: map[string]string{ + clusterv1.ClusterNameLabel: "test-cluster", + }, + Finalizers: []string{clusterv1.MachineFinalizer}, + }, + Spec: clusterv1.MachineSpec{ + ClusterName: "test-cluster", + InfrastructureRef: corev1.ObjectReference{}, + Bootstrap: clusterv1.Bootstrap{DataSecretName: ptr.To("data")}, + }, + Status: clusterv1.MachineStatus{ + NodeRef: &corev1.ObjectReference{ + Name: "test", + }, + }, + }, + expectedError: nil, + }, { name: "has nodeRef and control plane is healthy and externally managed", cluster: &clusterv1.Cluster{ From c3c360ad04b62cf3ab7b3cc9312a2ef923a16d87 Mon Sep 17 00:00:00 2001 From: Lubron Zhan Date: Thu, 1 May 2025 17:01:54 -0700 Subject: [PATCH 2/7] make generate Signed-off-by: Lubron Zhan --- api/v1beta1/conversion.go | 5 ++++ api/v1beta1/zz_generated.conversion.go | 26 ++++++++++++++----- api/v1beta2/zz_generated.deepcopy.go | 5 ++++ api/v1beta2/zz_generated.openapi.go | 7 +++++ .../crd/bases/cluster.x-k8s.io_clusters.yaml | 8 ++++++ .../core/v1alpha4/zz_generated.conversion.go | 1 + 6 files changed, 45 insertions(+), 7 deletions(-) diff --git a/api/v1beta1/conversion.go b/api/v1beta1/conversion.go index 3a7a039877e1..c1b50103d419 100644 --- a/api/v1beta1/conversion.go +++ b/api/v1beta1/conversion.go @@ -573,3 +573,8 @@ func Convert_v1beta1_Condition_To_v1_Condition(_ *Condition, _ *metav1.Condition // NOTE: legacy (v1beta1) conditions should not be automatically converted into v1beta2 conditions. return nil } + +// Convert_v1beta2_Topology_To_v1beta1_Topology is an autogenerated conversion function. +func Convert_v1beta2_Topology_To_v1beta1_Topology(in *clusterv1.Topology, out *Topology, s apimachineryconversion.Scope) error { + return autoConvert_v1beta2_Topology_To_v1beta1_Topology(in, out, s) +} diff --git a/api/v1beta1/zz_generated.conversion.go b/api/v1beta1/zz_generated.conversion.go index d97375e884f0..703a885fb4df 100644 --- a/api/v1beta1/zz_generated.conversion.go +++ b/api/v1beta1/zz_generated.conversion.go @@ -1431,7 +1431,15 @@ func autoConvert_v1beta1_ClusterSpec_To_v1beta2_ClusterSpec(in *ClusterSpec, out } out.ControlPlaneRef = (*corev1.ObjectReference)(unsafe.Pointer(in.ControlPlaneRef)) out.InfrastructureRef = (*corev1.ObjectReference)(unsafe.Pointer(in.InfrastructureRef)) - out.Topology = (*v1beta2.Topology)(unsafe.Pointer(in.Topology)) + if in.Topology != nil { + in, out := &in.Topology, &out.Topology + *out = new(v1beta2.Topology) + if err := Convert_v1beta1_Topology_To_v1beta2_Topology(*in, *out, s); err != nil { + return err + } + } else { + out.Topology = nil + } out.AvailabilityGates = *(*[]v1beta2.ClusterAvailabilityGate)(unsafe.Pointer(&in.AvailabilityGates)) return nil } @@ -1449,7 +1457,15 @@ func autoConvert_v1beta2_ClusterSpec_To_v1beta1_ClusterSpec(in *v1beta2.ClusterS } out.ControlPlaneRef = (*corev1.ObjectReference)(unsafe.Pointer(in.ControlPlaneRef)) out.InfrastructureRef = (*corev1.ObjectReference)(unsafe.Pointer(in.InfrastructureRef)) - out.Topology = (*Topology)(unsafe.Pointer(in.Topology)) + if in.Topology != nil { + in, out := &in.Topology, &out.Topology + *out = new(Topology) + if err := Convert_v1beta2_Topology_To_v1beta1_Topology(*in, *out, s); err != nil { + return err + } + } else { + out.Topology = nil + } out.AvailabilityGates = *(*[]ClusterAvailabilityGate)(unsafe.Pointer(&in.AvailabilityGates)) return nil } @@ -3442,14 +3458,10 @@ func autoConvert_v1beta2_Topology_To_v1beta1_Topology(in *v1beta2.Topology, out } out.Workers = (*WorkersTopology)(unsafe.Pointer(in.Workers)) out.Variables = *(*[]ClusterVariable)(unsafe.Pointer(&in.Variables)) + // WARNING: in.NodeDeletionStrategy requires manual conversion: does not exist in peer-type return nil } -// Convert_v1beta2_Topology_To_v1beta1_Topology is an autogenerated conversion function. -func Convert_v1beta2_Topology_To_v1beta1_Topology(in *v1beta2.Topology, out *Topology, s conversion.Scope) error { - return autoConvert_v1beta2_Topology_To_v1beta1_Topology(in, out, s) -} - func autoConvert_v1beta1_UnhealthyCondition_To_v1beta2_UnhealthyCondition(in *UnhealthyCondition, out *v1beta2.UnhealthyCondition, s conversion.Scope) error { out.Type = corev1.NodeConditionType(in.Type) out.Status = corev1.ConditionStatus(in.Status) diff --git a/api/v1beta2/zz_generated.deepcopy.go b/api/v1beta2/zz_generated.deepcopy.go index 83057039ce06..6e2fbf1813b2 100644 --- a/api/v1beta2/zz_generated.deepcopy.go +++ b/api/v1beta2/zz_generated.deepcopy.go @@ -2863,6 +2863,11 @@ func (in *Topology) DeepCopyInto(out *Topology) { (*in)[i].DeepCopyInto(&(*out)[i]) } } + if in.NodeDeletionStrategy != nil { + in, out := &in.NodeDeletionStrategy, &out.NodeDeletionStrategy + *out = new(NodeDrainStrategyType) + **out = **in + } } // DeepCopy is an autogenerated deepcopy function, copying the receiver, creating a new Topology. diff --git a/api/v1beta2/zz_generated.openapi.go b/api/v1beta2/zz_generated.openapi.go index 1f585c6fbfbe..3e7fbd806396 100644 --- a/api/v1beta2/zz_generated.openapi.go +++ b/api/v1beta2/zz_generated.openapi.go @@ -4904,6 +4904,13 @@ func schema_sigsk8sio_cluster_api_api_v1beta2_Topology(ref common.ReferenceCallb }, }, }, + "nodeDrainStrategy": { + SchemaProps: spec.SchemaProps{ + Description: "nodeDeletionStrategy specifies the strategy to delete nodes in the cluster. Avilable options: - graceful - force By default it's \"force\"", + Type: []string{"string"}, + Format: "", + }, + }, }, Required: []string{"class", "version"}, }, diff --git a/config/crd/bases/cluster.x-k8s.io_clusters.yaml b/config/crd/bases/cluster.x-k8s.io_clusters.yaml index f1e5cc17e5e9..86cfb5b55011 100644 --- a/config/crd/bases/cluster.x-k8s.io_clusters.yaml +++ b/config/crd/bases/cluster.x-k8s.io_clusters.yaml @@ -2682,6 +2682,14 @@ spec: x-kubernetes-list-type: map type: object type: object + nodeDrainStrategy: + description: |- + nodeDeletionStrategy specifies the strategy to delete nodes in the cluster. + Avilable options: + - graceful + - force + By default it's "force" + type: string rolloutAfter: description: |- rolloutAfter performs a rollout of the entire cluster one component at a time, diff --git a/internal/apis/core/v1alpha4/zz_generated.conversion.go b/internal/apis/core/v1alpha4/zz_generated.conversion.go index f28c0b10647b..8cb30d32c358 100644 --- a/internal/apis/core/v1alpha4/zz_generated.conversion.go +++ b/internal/apis/core/v1alpha4/zz_generated.conversion.go @@ -1884,6 +1884,7 @@ func autoConvert_v1beta2_Topology_To_v1alpha4_Topology(in *v1beta2.Topology, out out.Workers = nil } // WARNING: in.Variables requires manual conversion: does not exist in peer-type + // WARNING: in.NodeDeletionStrategy requires manual conversion: does not exist in peer-type return nil } From 9f9da49b64a1c7961b8e767df1b5e6ce1e539492 Mon Sep 17 00:00:00 2001 From: Lubron Zhan Date: Fri, 2 May 2025 11:07:26 -0700 Subject: [PATCH 3/7] Fix lint Signed-off-by: Lubron Zhan --- api/v1beta1/zz_generated.conversion.go | 10 +++++----- api/v1beta2/cluster_types.go | 16 ++++++++++------ api/v1beta2/zz_generated.deepcopy.go | 2 +- api/v1beta2/zz_generated.openapi.go | 4 ++-- config/crd/bases/cluster.x-k8s.io_clusters.yaml | 9 ++++++--- .../controllers/machine/machine_controller.go | 2 +- .../machine/machine_controller_test.go | 4 ++-- 7 files changed, 27 insertions(+), 20 deletions(-) diff --git a/api/v1beta1/zz_generated.conversion.go b/api/v1beta1/zz_generated.conversion.go index 703a885fb4df..47f40643d524 100644 --- a/api/v1beta1/zz_generated.conversion.go +++ b/api/v1beta1/zz_generated.conversion.go @@ -785,11 +785,6 @@ func RegisterConversions(s *runtime.Scheme) error { }); err != nil { return err } - if err := s.AddGeneratedConversionFunc((*v1beta2.Topology)(nil), (*Topology)(nil), func(a, b interface{}, scope conversion.Scope) error { - return Convert_v1beta2_Topology_To_v1beta1_Topology(a.(*v1beta2.Topology), b.(*Topology), scope) - }); err != nil { - return err - } if err := s.AddGeneratedConversionFunc((*UnhealthyCondition)(nil), (*v1beta2.UnhealthyCondition)(nil), func(a, b interface{}, scope conversion.Scope) error { return Convert_v1beta1_UnhealthyCondition_To_v1beta2_UnhealthyCondition(a.(*UnhealthyCondition), b.(*v1beta2.UnhealthyCondition), scope) }); err != nil { @@ -930,6 +925,11 @@ func RegisterConversions(s *runtime.Scheme) error { }); err != nil { return err } + if err := s.AddConversionFunc((*v1beta2.Topology)(nil), (*Topology)(nil), func(a, b interface{}, scope conversion.Scope) error { + return Convert_v1beta2_Topology_To_v1beta1_Topology(a.(*v1beta2.Topology), b.(*Topology), scope) + }); err != nil { + return err + } return nil } diff --git a/api/v1beta2/cluster_types.go b/api/v1beta2/cluster_types.go index 09ba6d0edbec..654708c88c27 100644 --- a/api/v1beta2/cluster_types.go +++ b/api/v1beta2/cluster_types.go @@ -587,12 +587,13 @@ type Topology struct { Variables []ClusterVariable `json:"variables,omitempty"` // nodeDeletionStrategy specifies the strategy to delete nodes in the cluster. - // Avilable options: + // Available options: // - graceful // - force - // By default it's "force" + // By default it's "force". // +optional - NodeDeletionStrategy *NodeDrainStrategyType `json:"nodeDrainStrategy,omitempty"` + // +kubebuilder:validation:Enum=force;graceful + NodeDeletionStrategy *NodeDeletionStrategyType `json:"nodeDeletionStrategy,omitempty"` } // ControlPlaneTopology specifies the parameters for the control plane nodes in the cluster. @@ -909,11 +910,14 @@ type MachinePoolVariables struct { Overrides []ClusterVariable `json:"overrides,omitempty"` } -type NodeDrainStrategyType string +// NodeDeletionStrategyType defines type of NodeDeletionStrategy. +type NodeDeletionStrategyType string const ( - NodeDrainStrategyForce NodeDrainStrategyType = "force" - NodeDrainStrategyGracefulWithTimeout NodeDrainStrategyType = "gracefulWithTimeout" + // NodeDeletionStrategyForce defines a force type strategy that node will be deleted immediately without drain. + NodeDeletionStrategyForce NodeDeletionStrategyType = "force" + // NodeDeletionStrategyTypeGracefulWithTimeout defines a force type strategy that node will be deleted with drain. + NodeDeletionStrategyGracefulWithTimeout NodeDeletionStrategyType = "gracefulWithTimeout" ) // ANCHOR_END: ClusterSpec diff --git a/api/v1beta2/zz_generated.deepcopy.go b/api/v1beta2/zz_generated.deepcopy.go index 6e2fbf1813b2..2a3ae0cc6294 100644 --- a/api/v1beta2/zz_generated.deepcopy.go +++ b/api/v1beta2/zz_generated.deepcopy.go @@ -2865,7 +2865,7 @@ func (in *Topology) DeepCopyInto(out *Topology) { } if in.NodeDeletionStrategy != nil { in, out := &in.NodeDeletionStrategy, &out.NodeDeletionStrategy - *out = new(NodeDrainStrategyType) + *out = new(NodeDeletionStrategyType) **out = **in } } diff --git a/api/v1beta2/zz_generated.openapi.go b/api/v1beta2/zz_generated.openapi.go index 3e7fbd806396..f98657df8303 100644 --- a/api/v1beta2/zz_generated.openapi.go +++ b/api/v1beta2/zz_generated.openapi.go @@ -4904,9 +4904,9 @@ func schema_sigsk8sio_cluster_api_api_v1beta2_Topology(ref common.ReferenceCallb }, }, }, - "nodeDrainStrategy": { + "nodeDeletionStrategy": { SchemaProps: spec.SchemaProps{ - Description: "nodeDeletionStrategy specifies the strategy to delete nodes in the cluster. Avilable options: - graceful - force By default it's \"force\"", + Description: "nodeDeletionStrategy specifies the strategy to delete nodes in the cluster. Available options: - graceful - force By default it's \"force\".", Type: []string{"string"}, Format: "", }, diff --git a/config/crd/bases/cluster.x-k8s.io_clusters.yaml b/config/crd/bases/cluster.x-k8s.io_clusters.yaml index 86cfb5b55011..eb48a398ffe5 100644 --- a/config/crd/bases/cluster.x-k8s.io_clusters.yaml +++ b/config/crd/bases/cluster.x-k8s.io_clusters.yaml @@ -2682,13 +2682,16 @@ spec: x-kubernetes-list-type: map type: object type: object - nodeDrainStrategy: + nodeDeletionStrategy: description: |- nodeDeletionStrategy specifies the strategy to delete nodes in the cluster. - Avilable options: + Available options: - graceful - force - By default it's "force" + By default it's "force". + enum: + - force + - graceful type: string rolloutAfter: description: |- diff --git a/internal/controllers/machine/machine_controller.go b/internal/controllers/machine/machine_controller.go index bff3fe2f6842..3720f58c8af3 100644 --- a/internal/controllers/machine/machine_controller.go +++ b/internal/controllers/machine/machine_controller.go @@ -715,7 +715,7 @@ func (r *Reconciler) nodeVolumeDetachTimeoutExceeded(machine *clusterv1.Machine) func (r *Reconciler) isDeleteNodeAllowed(ctx context.Context, cluster *clusterv1.Cluster, machine *clusterv1.Machine) error { log := ctrl.LoggerFrom(ctx) // Return early if the cluster is being deleted and cluster's nodeDeletionStrategy is not set or set to `force`. - if !cluster.DeletionTimestamp.IsZero() && (cluster.Spec.Topology.NodeDeletionStrategy == nil || *cluster.Spec.Topology.NodeDeletionStrategy == clusterv1.NodeDrainStrategyForce) { + if !cluster.DeletionTimestamp.IsZero() && (cluster.Spec.Topology.NodeDeletionStrategy == nil || *cluster.Spec.Topology.NodeDeletionStrategy == clusterv1.NodeDeletionStrategyForce) { return errClusterIsBeingDeleted } diff --git a/internal/controllers/machine/machine_controller_test.go b/internal/controllers/machine/machine_controller_test.go index 0c9510a706e5..4846b5577e15 100644 --- a/internal/controllers/machine/machine_controller_test.go +++ b/internal/controllers/machine/machine_controller_test.go @@ -2679,7 +2679,7 @@ func TestIsDeleteNodeAllowed(t *testing.T) { }, Spec: clusterv1.ClusterSpec{ Topology: &clusterv1.Topology{ - NodeDeletionStrategy: ptr.To(clusterv1.NodeDrainStrategyForce), + NodeDeletionStrategy: ptr.To(clusterv1.NodeDeletionStrategyForce), }, }, }, @@ -2695,7 +2695,7 @@ func TestIsDeleteNodeAllowed(t *testing.T) { }, Spec: clusterv1.ClusterSpec{ Topology: &clusterv1.Topology{ - NodeDeletionStrategy: ptr.To(clusterv1.NodeDrainStrategyForce), + NodeDeletionStrategy: ptr.To(clusterv1.NodeDeletionStrategyForce), }, }, }, From a35d904f0ed03f1d03be3e8b37b9426fa3af331f Mon Sep 17 00:00:00 2001 From: Lubron Zhan Date: Fri, 2 May 2025 11:33:55 -0700 Subject: [PATCH 4/7] Fix test Signed-off-by: Lubron Zhan --- internal/controllers/machine/machine_controller_test.go | 3 +++ 1 file changed, 3 insertions(+) diff --git a/internal/controllers/machine/machine_controller_test.go b/internal/controllers/machine/machine_controller_test.go index 4846b5577e15..bb9b32fe9166 100644 --- a/internal/controllers/machine/machine_controller_test.go +++ b/internal/controllers/machine/machine_controller_test.go @@ -2664,6 +2664,9 @@ func TestIsDeleteNodeAllowed(t *testing.T) { DeletionTimestamp: &deletionts, Finalizers: []string{clusterv1.ClusterFinalizer}, }, + Spec: clusterv1.ClusterSpec{ + Topology: &clusterv1.Topology{}, + }, }, machine: &clusterv1.Machine{}, expectedError: errClusterIsBeingDeleted, From 9ed080e0219c1b7a65afdd9b2112d5eb244de26f Mon Sep 17 00:00:00 2001 From: Lubron Zhan Date: Fri, 2 May 2025 11:43:03 -0700 Subject: [PATCH 5/7] Fix lint Signed-off-by: Lubron Zhan --- api/v1beta2/cluster_types.go | 2 +- internal/controllers/machine/machine_controller.go | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/api/v1beta2/cluster_types.go b/api/v1beta2/cluster_types.go index 654708c88c27..a333f20d79eb 100644 --- a/api/v1beta2/cluster_types.go +++ b/api/v1beta2/cluster_types.go @@ -916,7 +916,7 @@ type NodeDeletionStrategyType string const ( // NodeDeletionStrategyForce defines a force type strategy that node will be deleted immediately without drain. NodeDeletionStrategyForce NodeDeletionStrategyType = "force" - // NodeDeletionStrategyTypeGracefulWithTimeout defines a force type strategy that node will be deleted with drain. + // NodeDeletionStrategyGracefulWithTimeout defines a force type strategy that node will be deleted with drain. NodeDeletionStrategyGracefulWithTimeout NodeDeletionStrategyType = "gracefulWithTimeout" ) diff --git a/internal/controllers/machine/machine_controller.go b/internal/controllers/machine/machine_controller.go index 3720f58c8af3..d1b720830e88 100644 --- a/internal/controllers/machine/machine_controller.go +++ b/internal/controllers/machine/machine_controller.go @@ -715,7 +715,7 @@ func (r *Reconciler) nodeVolumeDetachTimeoutExceeded(machine *clusterv1.Machine) func (r *Reconciler) isDeleteNodeAllowed(ctx context.Context, cluster *clusterv1.Cluster, machine *clusterv1.Machine) error { log := ctrl.LoggerFrom(ctx) // Return early if the cluster is being deleted and cluster's nodeDeletionStrategy is not set or set to `force`. - if !cluster.DeletionTimestamp.IsZero() && (cluster.Spec.Topology.NodeDeletionStrategy == nil || *cluster.Spec.Topology.NodeDeletionStrategy == clusterv1.NodeDeletionStrategyForce) { + if !cluster.DeletionTimestamp.IsZero() && (cluster.Spec.Topology == nil || cluster.Spec.Topology.NodeDeletionStrategy == nil || *cluster.Spec.Topology.NodeDeletionStrategy == clusterv1.NodeDeletionStrategyForce) { return errClusterIsBeingDeleted } From d5ac066492025fac335e49e0a351ed7714011b2b Mon Sep 17 00:00:00 2001 From: Lubron Zhan Date: Tue, 6 May 2025 16:12:39 -0700 Subject: [PATCH 6/7] Resolve comments Signed-off-by: Lubron Zhan --- api/v1beta2/cluster_types.go | 16 ++++++++-------- api/v1beta2/zz_generated.deepcopy.go | 5 ----- api/v1beta2/zz_generated.openapi.go | 2 +- config/crd/bases/cluster.x-k8s.io_clusters.yaml | 8 ++++---- .../controllers/machine/machine_controller.go | 2 +- .../machine/machine_controller_test.go | 4 ++-- 6 files changed, 16 insertions(+), 21 deletions(-) diff --git a/api/v1beta2/cluster_types.go b/api/v1beta2/cluster_types.go index a333f20d79eb..de8f4fd2c116 100644 --- a/api/v1beta2/cluster_types.go +++ b/api/v1beta2/cluster_types.go @@ -587,13 +587,13 @@ type Topology struct { Variables []ClusterVariable `json:"variables,omitempty"` // nodeDeletionStrategy specifies the strategy to delete nodes in the cluster. - // Available options: - // - graceful - // - force - // By default it's "force". + // Valid values are Force, Graceful and omitted. + // When omitted, the default behaviour will be Force. + // Graceful means that nodes will be deleted with drain. + // Force means that nodes will be deleted immediately without drain. // +optional // +kubebuilder:validation:Enum=force;graceful - NodeDeletionStrategy *NodeDeletionStrategyType `json:"nodeDeletionStrategy,omitempty"` + NodeDeletionStrategy NodeDeletionStrategyType `json:"nodeDeletionStrategy,omitempty"` } // ControlPlaneTopology specifies the parameters for the control plane nodes in the cluster. @@ -915,9 +915,9 @@ type NodeDeletionStrategyType string const ( // NodeDeletionStrategyForce defines a force type strategy that node will be deleted immediately without drain. - NodeDeletionStrategyForce NodeDeletionStrategyType = "force" - // NodeDeletionStrategyGracefulWithTimeout defines a force type strategy that node will be deleted with drain. - NodeDeletionStrategyGracefulWithTimeout NodeDeletionStrategyType = "gracefulWithTimeout" + NodeDeletionStrategyForce NodeDeletionStrategyType = "Force" + // NodeDeletionStrategyGraceful defines a graceful type strategy that node will be deleted with drain. + NodeDeletionStrategyGraceful NodeDeletionStrategyType = "Graceful" ) // ANCHOR_END: ClusterSpec diff --git a/api/v1beta2/zz_generated.deepcopy.go b/api/v1beta2/zz_generated.deepcopy.go index 2a3ae0cc6294..83057039ce06 100644 --- a/api/v1beta2/zz_generated.deepcopy.go +++ b/api/v1beta2/zz_generated.deepcopy.go @@ -2863,11 +2863,6 @@ func (in *Topology) DeepCopyInto(out *Topology) { (*in)[i].DeepCopyInto(&(*out)[i]) } } - if in.NodeDeletionStrategy != nil { - in, out := &in.NodeDeletionStrategy, &out.NodeDeletionStrategy - *out = new(NodeDeletionStrategyType) - **out = **in - } } // DeepCopy is an autogenerated deepcopy function, copying the receiver, creating a new Topology. diff --git a/api/v1beta2/zz_generated.openapi.go b/api/v1beta2/zz_generated.openapi.go index f98657df8303..652de441bc45 100644 --- a/api/v1beta2/zz_generated.openapi.go +++ b/api/v1beta2/zz_generated.openapi.go @@ -4906,7 +4906,7 @@ func schema_sigsk8sio_cluster_api_api_v1beta2_Topology(ref common.ReferenceCallb }, "nodeDeletionStrategy": { SchemaProps: spec.SchemaProps{ - Description: "nodeDeletionStrategy specifies the strategy to delete nodes in the cluster. Available options: - graceful - force By default it's \"force\".", + Description: "nodeDeletionStrategy specifies the strategy to delete nodes in the cluster. Valid values are Force, Graceful and omitted. When omitted, the default behaviour will be Force. Graceful means that nodes will be deleted with drain. Force means that nodes will be deleted immediately without drain.", Type: []string{"string"}, Format: "", }, diff --git a/config/crd/bases/cluster.x-k8s.io_clusters.yaml b/config/crd/bases/cluster.x-k8s.io_clusters.yaml index eb48a398ffe5..e65c61393af5 100644 --- a/config/crd/bases/cluster.x-k8s.io_clusters.yaml +++ b/config/crd/bases/cluster.x-k8s.io_clusters.yaml @@ -2685,10 +2685,10 @@ spec: nodeDeletionStrategy: description: |- nodeDeletionStrategy specifies the strategy to delete nodes in the cluster. - Available options: - - graceful - - force - By default it's "force". + Valid values are Force, Graceful and omitted. + When omitted, the default behaviour will be Force. + Graceful means that nodes will be deleted with drain. + Force means that nodes will be deleted immediately without drain. enum: - force - graceful diff --git a/internal/controllers/machine/machine_controller.go b/internal/controllers/machine/machine_controller.go index d1b720830e88..184f8fa9147e 100644 --- a/internal/controllers/machine/machine_controller.go +++ b/internal/controllers/machine/machine_controller.go @@ -715,7 +715,7 @@ func (r *Reconciler) nodeVolumeDetachTimeoutExceeded(machine *clusterv1.Machine) func (r *Reconciler) isDeleteNodeAllowed(ctx context.Context, cluster *clusterv1.Cluster, machine *clusterv1.Machine) error { log := ctrl.LoggerFrom(ctx) // Return early if the cluster is being deleted and cluster's nodeDeletionStrategy is not set or set to `force`. - if !cluster.DeletionTimestamp.IsZero() && (cluster.Spec.Topology == nil || cluster.Spec.Topology.NodeDeletionStrategy == nil || *cluster.Spec.Topology.NodeDeletionStrategy == clusterv1.NodeDeletionStrategyForce) { + if !cluster.DeletionTimestamp.IsZero() && (cluster.Spec.Topology == nil || cluster.Spec.Topology.NodeDeletionStrategy == clusterv1.NodeDeletionStrategyForce) { return errClusterIsBeingDeleted } diff --git a/internal/controllers/machine/machine_controller_test.go b/internal/controllers/machine/machine_controller_test.go index bb9b32fe9166..5a5795bb97d4 100644 --- a/internal/controllers/machine/machine_controller_test.go +++ b/internal/controllers/machine/machine_controller_test.go @@ -2682,7 +2682,7 @@ func TestIsDeleteNodeAllowed(t *testing.T) { }, Spec: clusterv1.ClusterSpec{ Topology: &clusterv1.Topology{ - NodeDeletionStrategy: ptr.To(clusterv1.NodeDeletionStrategyForce), + NodeDeletionStrategy: clusterv1.NodeDeletionStrategyForce, }, }, }, @@ -2698,7 +2698,7 @@ func TestIsDeleteNodeAllowed(t *testing.T) { }, Spec: clusterv1.ClusterSpec{ Topology: &clusterv1.Topology{ - NodeDeletionStrategy: ptr.To(clusterv1.NodeDeletionStrategyForce), + NodeDeletionStrategy: clusterv1.NodeDeletionStrategyForce, }, }, }, From 1b057297a5149a377feac0791c8b769f4b88d210 Mon Sep 17 00:00:00 2001 From: Lubron Zhan Date: Tue, 6 May 2025 16:14:19 -0700 Subject: [PATCH 7/7] Resolve conversion test failure Signed-off-by: Lubron Zhan --- api/v1beta1/conversion.go | 7 +++++-- api/v1beta1/zz_generated.conversion.go | 10 +++++----- internal/controllers/machine/machine_controller.go | 2 +- 3 files changed, 11 insertions(+), 8 deletions(-) diff --git a/api/v1beta1/conversion.go b/api/v1beta1/conversion.go index c1b50103d419..fac29f39f3b9 100644 --- a/api/v1beta1/conversion.go +++ b/api/v1beta1/conversion.go @@ -574,7 +574,10 @@ func Convert_v1beta1_Condition_To_v1_Condition(_ *Condition, _ *metav1.Condition return nil } -// Convert_v1beta2_Topology_To_v1beta1_Topology is an autogenerated conversion function. func Convert_v1beta2_Topology_To_v1beta1_Topology(in *clusterv1.Topology, out *Topology, s apimachineryconversion.Scope) error { - return autoConvert_v1beta2_Topology_To_v1beta1_Topology(in, out, s) + if err := autoConvert_v1beta2_Topology_To_v1beta1_Topology(in, out, s); err != nil { + return err + } + + return nil } diff --git a/api/v1beta1/zz_generated.conversion.go b/api/v1beta1/zz_generated.conversion.go index 47f40643d524..703a885fb4df 100644 --- a/api/v1beta1/zz_generated.conversion.go +++ b/api/v1beta1/zz_generated.conversion.go @@ -785,6 +785,11 @@ func RegisterConversions(s *runtime.Scheme) error { }); err != nil { return err } + if err := s.AddGeneratedConversionFunc((*v1beta2.Topology)(nil), (*Topology)(nil), func(a, b interface{}, scope conversion.Scope) error { + return Convert_v1beta2_Topology_To_v1beta1_Topology(a.(*v1beta2.Topology), b.(*Topology), scope) + }); err != nil { + return err + } if err := s.AddGeneratedConversionFunc((*UnhealthyCondition)(nil), (*v1beta2.UnhealthyCondition)(nil), func(a, b interface{}, scope conversion.Scope) error { return Convert_v1beta1_UnhealthyCondition_To_v1beta2_UnhealthyCondition(a.(*UnhealthyCondition), b.(*v1beta2.UnhealthyCondition), scope) }); err != nil { @@ -925,11 +930,6 @@ func RegisterConversions(s *runtime.Scheme) error { }); err != nil { return err } - if err := s.AddConversionFunc((*v1beta2.Topology)(nil), (*Topology)(nil), func(a, b interface{}, scope conversion.Scope) error { - return Convert_v1beta2_Topology_To_v1beta1_Topology(a.(*v1beta2.Topology), b.(*Topology), scope) - }); err != nil { - return err - } return nil } diff --git a/internal/controllers/machine/machine_controller.go b/internal/controllers/machine/machine_controller.go index 184f8fa9147e..90079020c474 100644 --- a/internal/controllers/machine/machine_controller.go +++ b/internal/controllers/machine/machine_controller.go @@ -715,7 +715,7 @@ func (r *Reconciler) nodeVolumeDetachTimeoutExceeded(machine *clusterv1.Machine) func (r *Reconciler) isDeleteNodeAllowed(ctx context.Context, cluster *clusterv1.Cluster, machine *clusterv1.Machine) error { log := ctrl.LoggerFrom(ctx) // Return early if the cluster is being deleted and cluster's nodeDeletionStrategy is not set or set to `force`. - if !cluster.DeletionTimestamp.IsZero() && (cluster.Spec.Topology == nil || cluster.Spec.Topology.NodeDeletionStrategy == clusterv1.NodeDeletionStrategyForce) { + if !cluster.DeletionTimestamp.IsZero() && (cluster.Spec.Topology == nil || cluster.Spec.Topology.NodeDeletionStrategy != clusterv1.NodeDeletionStrategyGraceful) { return errClusterIsBeingDeleted }