Skip to content
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.

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

🌱 Propagate timeout fields from MachineSet to Machine during Machine deletion #10589

Merged

Conversation

davidvossel
Copy link
Contributor

Fixes #10588
/area machine

What this PR does / why we need it:

Machine's default to nodeDrainTimeout: 0s, which blocks indefinitely if a pod can't be evicted. We can't change the nodeDrainTimeout in-place from the MachineDeployment or MachineSet after a machine is marked for deletion.

This results in a machine that wedged forever but can't be updated using the top level objects that own the machine.

To fix this, this PR allows fields related to machine deletion to be updated in place even when the machine is marked for deletion.

NOTE - I did not add unit tests yet for this PR. I want confirmation this is an acceptable approve before investing time into testing.

@k8s-ci-robot k8s-ci-robot added area/machine Issues or PRs related to machine lifecycle management cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels May 10, 2024
@enxebre
Copy link
Member

enxebre commented May 13, 2024

Thanks @davidvossel change makes sense to me. Smoother deletion is actually one of the supporting use cases for inplace propagation. Let's include some unit tests.

See related #5880
#9285

@k8s-ci-robot k8s-ci-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels May 13, 2024
@davidvossel
Copy link
Contributor Author

Let's include some unit tests.

@enxebre i extended the existing unit test to cover the case of updating a deleting machine.

@sbueringer sbueringer changed the title Allow inplace update of fields related to deletion during Machine deletion 🌱 Allow inplace update of fields related to deletion during Machine deletion May 14, 2024
@enxebre
Copy link
Member

enxebre commented May 20, 2024

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label May 20, 2024
@k8s-ci-robot
Copy link
Contributor

LGTM label has been added.

Git tree hash: 44936fae936d0eab3c39b86c432c24c5e199979d

@k8s-ci-robot k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label May 30, 2024
@k8s-ci-robot k8s-ci-robot requested a review from enxebre May 30, 2024 14:45
@enxebre
Copy link
Member

enxebre commented May 31, 2024

/lgtm
/assign @sbueringer

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label May 31, 2024
@k8s-ci-robot
Copy link
Contributor

LGTM label has been added.

Git tree hash: a7820897401d291e168e73cfc2ea745d5f2c8d87

@sbueringer
Copy link
Member

All good from my side. I would open a follow-up issue once this PR is merged to track further work to get this behavior across all MD workflows (e.g. MD rollout, deletion).

/approve

/hold
In case someone else wants to take a look (@fabriziopandini @chrischdi @vincepri)

Otherwise let's merge in a few days

@k8s-ci-robot k8s-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jun 3, 2024
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: sbueringer

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@k8s-ci-robot k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jun 3, 2024
@vincepri
Copy link
Member

/lgtm

Copy link
Member

@chrischdi chrischdi left a comment

Choose a reason for hiding this comment

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

Could we update the PR title to more reflect what is happening?

E.g.

🌱 Allow inplace update of fields related to deletion during MachineSet scale to 0

(as of that lgtm)

@sbueringer sbueringer changed the title 🌱 Allow inplace update of fields related to deletion during Machine deletion 🌱 Propagate timeout fields from MachineSet to Machine during Machine deletion Jun 12, 2024
@sbueringer
Copy link
Member

sbueringer commented Jun 12, 2024

I renamed slightly differently. The scenarios are not distinct and this is also part of fixing the other cases

@sbueringer
Copy link
Member

Thx folks!

/hold cancel

I'll open the follow-up issue tomorrow

@k8s-ci-robot k8s-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jun 12, 2024
@k8s-ci-robot k8s-ci-robot merged commit 28fbfbb into kubernetes-sigs:main Jun 12, 2024
26 checks passed
@k8s-ci-robot k8s-ci-robot added this to the v1.8 milestone Jun 12, 2024
@sbueringer
Copy link
Member

Thx folks!

/hold cancel

I'll open the follow-up issue tomorrow

Here's the follow-up issue: #10753

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. area/machine Issues or PRs related to machine lifecycle management cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. lgtm "Looks good to me", indicates that a PR is ready to be merged. size/M Denotes a PR that changes 30-99 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

MachineSet Inplace update does not work during machine deletion
6 participants