Skip to content

✨ Add nodeDeletionTimeout property to Machine #5608

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Feb 22, 2022
Merged

✨ Add nodeDeletionTimeout property to Machine #5608

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 8 additions & 0 deletions api/v1alpha3/conversion.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -97,6 +97,7 @@ func (src *Machine) ConvertTo(dstRaw conversion.Hub) error {
return err
}

dst.Spec.NodeDeletionTimeout = restored.Spec.NodeDeletionTimeout
dst.Status.NodeInfo = restored.Status.NodeInfo
return nil
}
Expand Down Expand Up @@ -139,6 +140,7 @@ func (src *MachineSet) ConvertTo(dstRaw conversion.Hub) error {
if ok, err := utilconversion.UnmarshalData(src, restored); err != nil || !ok {
return err
}
dst.Spec.Template.Spec.NodeDeletionTimeout = restored.Spec.Template.Spec.NodeDeletionTimeout
dst.Status.Conditions = restored.Status.Conditions
return nil
}
Expand Down Expand Up @@ -192,6 +194,7 @@ func (src *MachineDeployment) ConvertTo(dstRaw conversion.Hub) error {
dst.Spec.Strategy.RollingUpdate.DeletePolicy = restored.Spec.Strategy.RollingUpdate.DeletePolicy
}

dst.Spec.Template.Spec.NodeDeletionTimeout = restored.Spec.Template.Spec.NodeDeletionTimeout
dst.Status.Conditions = restored.Status.Conditions
return nil
}
Expand Down Expand Up @@ -304,6 +307,11 @@ func Convert_v1beta1_MachineStatus_To_v1alpha3_MachineStatus(in *clusterv1.Machi
return autoConvert_v1beta1_MachineStatus_To_v1alpha3_MachineStatus(in, out, s)
}

func Convert_v1beta1_MachineSpec_To_v1alpha3_MachineSpec(in *clusterv1.MachineSpec, out *MachineSpec, s apiconversion.Scope) error {
// spec.nodeDeletionTimeout has been added with v1beta1.
return autoConvert_v1beta1_MachineSpec_To_v1alpha3_MachineSpec(in, out, s)
}

func Convert_v1beta1_MachineDeploymentStatus_To_v1alpha3_MachineDeploymentStatus(in *clusterv1.MachineDeploymentStatus, out *MachineDeploymentStatus, s apiconversion.Scope) error {
// Status.Conditions was introduced in v1alpha4, thus requiring a custom conversion function; the values is going to be preserved in an annotation thus allowing roundtrip without loosing informations
return autoConvert_v1beta1_MachineDeploymentStatus_To_v1alpha3_MachineDeploymentStatus(in, out, s)
Expand Down
16 changes: 6 additions & 10 deletions api/v1alpha3/zz_generated.conversion.go
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

69 changes: 63 additions & 6 deletions api/v1alpha4/conversion.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -131,13 +131,33 @@ func (dst *ClusterClassList) ConvertFrom(srcRaw conversion.Hub) error {
func (src *Machine) ConvertTo(dstRaw conversion.Hub) error {
dst := dstRaw.(*clusterv1.Machine)

return Convert_v1alpha4_Machine_To_v1beta1_Machine(src, dst, nil)
if err := Convert_v1alpha4_Machine_To_v1beta1_Machine(src, dst, nil); err != nil {
return err
}

// Manually restore data.
restored := &clusterv1.Machine{}
if ok, err := utilconversion.UnmarshalData(src, restored); err != nil || !ok {
return err
}

dst.Spec.NodeDeletionTimeout = restored.Spec.NodeDeletionTimeout
return nil
}

func (dst *Machine) ConvertFrom(srcRaw conversion.Hub) error {
src := srcRaw.(*clusterv1.Machine)

return Convert_v1beta1_Machine_To_v1alpha4_Machine(src, dst, nil)
if err := Convert_v1beta1_Machine_To_v1alpha4_Machine(src, dst, nil); err != nil {
return err
}

// Preserve Hub data on down-conversion except for metadata
if err := utilconversion.MarshalData(src, dst); err != nil {
return err
}

return nil
}

func (src *MachineList) ConvertTo(dstRaw conversion.Hub) error {
Expand All @@ -155,13 +175,29 @@ func (dst *MachineList) ConvertFrom(srcRaw conversion.Hub) error {
func (src *MachineSet) ConvertTo(dstRaw conversion.Hub) error {
dst := dstRaw.(*clusterv1.MachineSet)

return Convert_v1alpha4_MachineSet_To_v1beta1_MachineSet(src, dst, nil)
if err := Convert_v1alpha4_MachineSet_To_v1beta1_MachineSet(src, dst, nil); err != nil {
return err
}

// Manually restore data.
restored := &clusterv1.MachineSet{}
if ok, err := utilconversion.UnmarshalData(src, restored); err != nil || !ok {
return err
}

dst.Spec.Template.Spec.NodeDeletionTimeout = restored.Spec.Template.Spec.NodeDeletionTimeout
return nil
}

func (dst *MachineSet) ConvertFrom(srcRaw conversion.Hub) error {
src := srcRaw.(*clusterv1.MachineSet)

return Convert_v1beta1_MachineSet_To_v1alpha4_MachineSet(src, dst, nil)
if err := Convert_v1beta1_MachineSet_To_v1alpha4_MachineSet(src, dst, nil); err != nil {
return err
}

// Preserve Hub data on down-conversion except for metadata
return utilconversion.MarshalData(src, dst)
}

func (src *MachineSetList) ConvertTo(dstRaw conversion.Hub) error {
Expand All @@ -179,13 +215,29 @@ func (dst *MachineSetList) ConvertFrom(srcRaw conversion.Hub) error {
func (src *MachineDeployment) ConvertTo(dstRaw conversion.Hub) error {
dst := dstRaw.(*clusterv1.MachineDeployment)

return Convert_v1alpha4_MachineDeployment_To_v1beta1_MachineDeployment(src, dst, nil)
if err := Convert_v1alpha4_MachineDeployment_To_v1beta1_MachineDeployment(src, dst, nil); err != nil {
return err
}

// Manually restore data.
restored := &clusterv1.MachineDeployment{}
if ok, err := utilconversion.UnmarshalData(src, restored); err != nil || !ok {
return err
}

dst.Spec.Template.Spec.NodeDeletionTimeout = restored.Spec.Template.Spec.NodeDeletionTimeout
return nil
}

func (dst *MachineDeployment) ConvertFrom(srcRaw conversion.Hub) error {
src := srcRaw.(*clusterv1.MachineDeployment)

return Convert_v1beta1_MachineDeployment_To_v1alpha4_MachineDeployment(src, dst, nil)
if err := Convert_v1beta1_MachineDeployment_To_v1alpha4_MachineDeployment(src, dst, nil); err != nil {
return err
}

// Preserve Hub data on down-conversion except for metadata
return utilconversion.MarshalData(src, dst)
}

func (src *MachineDeploymentList) ConvertTo(dstRaw conversion.Hub) error {
Expand Down Expand Up @@ -234,6 +286,11 @@ func Convert_v1beta1_ClusterClassSpec_To_v1alpha4_ClusterClassSpec(in *clusterv1
return autoConvert_v1beta1_ClusterClassSpec_To_v1alpha4_ClusterClassSpec(in, out, s)
}

func Convert_v1beta1_MachineSpec_To_v1alpha4_MachineSpec(in *clusterv1.MachineSpec, out *MachineSpec, s apiconversion.Scope) error {
// spec.nodeDeletionTimeout has been added with v1beta1.
return autoConvert_v1beta1_MachineSpec_To_v1alpha4_MachineSpec(in, out, s)
}

func Convert_v1beta1_Topology_To_v1alpha4_Topology(in *clusterv1.Topology, out *Topology, s apiconversion.Scope) error {
// spec.topology.variables has been added with v1beta1.
return autoConvert_v1beta1_Topology_To_v1alpha4_Topology(in, out, s)
Expand Down
64 changes: 50 additions & 14 deletions api/v1alpha4/zz_generated.conversion.go
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

6 changes: 6 additions & 0 deletions api/v1beta1/machine_types.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -96,6 +96,12 @@ type MachineSpec struct {
// NOTE: NodeDrainTimeout is different from `kubectl drain --timeout`
// +optional
NodeDrainTimeout *metav1.Duration `json:"nodeDrainTimeout,omitempty"`

// NodeDeletionTimeout defines how long the controller will attempt to delete the Node that the Machine
// hosts after the Machine is marked for deletion. A duration of 0 will retry deletion indefinitely.
// Defaults to 10 seconds.
// +optional
NodeDeletionTimeout *metav1.Duration `json:"nodeDeletionTimeout,omitempty"`
}

// ANCHOR_END: MachineSpec
Expand Down
8 changes: 8 additions & 0 deletions api/v1beta1/machine_webhook.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,8 +19,10 @@ package v1beta1
import (
"fmt"
"strings"
"time"

apierrors "k8s.io/apimachinery/pkg/api/errors"
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
"k8s.io/apimachinery/pkg/runtime"
"k8s.io/apimachinery/pkg/util/validation/field"
ctrl "sigs.k8s.io/controller-runtime"
Expand All @@ -29,6 +31,8 @@ import (
"sigs.k8s.io/cluster-api/util/version"
)

const defaultNodeDeletionTimeout = 10 * time.Second

func (m *Machine) SetupWebhookWithManager(mgr ctrl.Manager) error {
return ctrl.NewWebhookManagedBy(mgr).
For(m).
Expand Down Expand Up @@ -60,6 +64,10 @@ func (m *Machine) Default() {
normalizedVersion := "v" + *m.Spec.Version
m.Spec.Version = &normalizedVersion
}

if m.Spec.NodeDeletionTimeout == nil {
m.Spec.NodeDeletionTimeout = &metav1.Duration{Duration: defaultNodeDeletionTimeout}
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does it make sense to use // kubebuilder:default="10s" instead? That would also allow omitting the pointer, but the default would be applied on MachineDeployments, MachineSets and MachinePools as well, and not just on Machines.

In general, what's preferred here? The spec default or custom logic in the webhook?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Webhook is probably better

}
}

// ValidateCreate implements webhook.Validator so a webhook will be registered for the type.
Expand Down
1 change: 1 addition & 0 deletions api/v1beta1/machine_webhook_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -46,6 +46,7 @@ func TestMachineDefault(t *testing.T) {
g.Expect(m.Spec.Bootstrap.ConfigRef.Namespace).To(Equal(m.Namespace))
g.Expect(m.Spec.InfrastructureRef.Namespace).To(Equal(m.Namespace))
g.Expect(*m.Spec.Version).To(Equal("v1.17.5"))
g.Expect(m.Spec.NodeDeletionTimeout.Duration).To(Equal(defaultNodeDeletionTimeout))
}

func TestMachineBootstrapValidation(t *testing.T) {
Expand Down
5 changes: 5 additions & 0 deletions api/v1beta1/zz_generated.deepcopy.go
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

6 changes: 6 additions & 0 deletions config/crd/bases/cluster.x-k8s.io_machinedeployments.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1285,6 +1285,12 @@ spec:
description: 'UID of the referent. More info: https://kubernetes.io/docs/concepts/overview/working-with-objects/names/#uids'
type: string
type: object
nodeDeletionTimeout:
description: NodeDeletionTimeout defines how long the controller
will attempt to delete the Node that the Machine hosts after
the Machine is marked for deletion. A duration of 0 will
retry deletion indefinitely. Defaults to 10 seconds.
type: string
nodeDrainTimeout:
description: 'NodeDrainTimeout is the total amount of time
that the controller will spend on draining a node. The default
Expand Down
6 changes: 6 additions & 0 deletions config/crd/bases/cluster.x-k8s.io_machinepools.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1165,6 +1165,12 @@ spec:
description: 'UID of the referent. More info: https://kubernetes.io/docs/concepts/overview/working-with-objects/names/#uids'
type: string
type: object
nodeDeletionTimeout:
description: NodeDeletionTimeout defines how long the controller
will attempt to delete the Node that the Machine hosts after
the Machine is marked for deletion. A duration of 0 will
retry deletion indefinitely. Defaults to 10 seconds.
type: string
nodeDrainTimeout:
description: 'NodeDrainTimeout is the total amount of time
that the controller will spend on draining a node. The default
Expand Down
Loading