Add prevent deleting annotation #48

michaelgugino · 2019-07-03T18:01:01Z

Why we need this:

This commit adds the ability to prevent processing of deletion
of machine-objects when the annotation is present. This
is particularly useful when an automated remediation mechanism
is implemented to serve as a way for administrators to
indicate they do not wish a particular machine to be
remediated for whatever reason.

openshift-ci-robot · 2019-07-03T18:01:32Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
To complete the pull request process, please assign spangenberg
You can assign the PR to them by writing /assign @spangenberg in a comment when ready.

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

Details

Needs approval from an approver in each of these files:

OWNERS

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

vikaschoudhary16 · 2019-07-05T06:39:38Z

pkg/controller/machine/controller.go

 }

-func (r *ReconcileMachine) isDeleteAllowed(machine *machinev1.Machine) bool {
-	if r.nodeName == "" || machine.Status.NodeRef == nil {


Why we are removing this logic which ensures that the node on which this controller is running, will not be deleted?

Please update the PR description as well to reflect this change. Currently description sems to suggest that additionally annotation ios going to be supported.

I can definitely imagine scenarios where the MAO node would need to be deleted, however machine-healthcheck has a special case for masters that prevents it from asking for their deletion.

vikaschoudhary16 · 2019-07-05T06:43:53Z

This PR seems to be solving the problem similar to the one being discussed here, openshift/machine-api-operator#333

Why we need this: This commit adds the ability to prevent processing of deletion of machine-objects when the annotation is present. This is particularly useful when an automated remediation mechanism is implemented to serve as a way for administrators to indicate they do not wish a particular machine to be remediated for whatever reason.

ingvagabund · 2019-07-11T06:45:07Z

This will have an unfortunate effect of keeping a machine in Deleting state indefinitely. Right now a machine is not deleted only if the machine is also hosting the machine controller. Once such a machine is mark for deletion, it's no big deal to reschedule the controller to another node and finish the deletion. However, for a machine that is never supposed to be deleted, it will be quite confusing for users to see a machine being deleted for x hours. Someone will definitely report a bug. The same holds for a machineset that has at least one machine annotated and is supposed to be deleted. Not mentioning machine deployments that never will be able to finish rolling updated. Or, disruption budgets with max 1 machine unavailable.

Wrt. machine health checker this will help to filter out such machines so they will never get deleted.

enxebre · 2019-07-11T07:23:34Z

this (the annotation, its creation/deletion/lifecycle and what to do/no to do when is present) should be handled by MHC/remediation/upper-level-consumer so they shape its business logic on top of immutable operations create/update/delete rather than here introducing deviated behaviour for when a machine is set for deletion leaving the object permanently with a deletionTimestamp

bison · 2019-07-11T11:36:53Z

This will have an unfortunate effect of keeping a machine in Deleting state indefinitely.

Yeah, if we want this annotation, the correct place to handle it would be in an admission webhook so we can reject the delete outright.

michaelgugino · 2019-07-11T14:35:09Z

This will have an unfortunate effect of keeping a machine in Deleting state indefinitely. Right now a machine is not deleted only if the machine is also hosting the machine controller. Once such a machine is mark for deletion, it's no big deal to reschedule the controller to another node and finish the deletion. However, for a machine that is never supposed to be deleted, it will be quite confusing for users to see a machine being deleted for x hours. Someone will definitely report a bug. The same holds for a machineset that has at least one machine annotated and is supposed to be deleted. Not mentioning machine deployments that never will be able to finish rolling updated. Or, disruption budgets with max 1 machine unavailable.

Wrt. machine health checker this will help to filter out such machines so they will never get deleted.

@ingvagabund I think this is primarily opt-in, so there's not much reason for a bug to be filed if a user creates this annotation. Maybe machinesets need to be updated to account for this, but I don't think that's a blocker. We need to give users a way on a per-machine basis to prevent deletion. This prevention might be necessary for a variety of reasons such as preserving an instance for security audits, etc.

@bison I think this annotation could definitely be used in tandem with a validating webhook to avoid persisting in delete forever (which we should have an alarm on, as I suggested previously).

As far as the remediation layer, I don't want to rely on a 3rd party respecting this annotation, because what if it doesn't? The user should always be right and the buck stops with the machine-controller.

openshift-ci-robot · 2019-09-05T10:05:12Z

@michaelgugino: The following test failed, say /retest to rerun them all:

Test name	Commit	Details	Rerun command
ci/prow/goimports	`920a788`	link	`/test goimports`

Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR.

Details

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.

openshift-bot · 2019-09-25T22:57:53Z

/bugzilla refresh

openshift-ci-robot · 2019-09-25T22:57:54Z

@openshift-bot: No Bugzilla bug is referenced in the title of this pull request.
To reference a bug, add 'Bug XXX:' to the title of this pull request and request another bug refresh with /bugzilla refresh.

Details

In response to this:

/bugzilla refresh

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

openshift-bot · 2019-09-25T23:00:18Z

/bugzilla refresh

openshift-ci-robot · 2019-09-25T23:00:19Z

@openshift-bot: No Bugzilla bug is referenced in the title of this pull request.
To reference a bug, add 'Bug XXX:' to the title of this pull request and request another bug refresh with /bugzilla refresh.

Details

In response to this:

/bugzilla refresh

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

eparis · 2019-10-26T00:19:55Z

Is this still a PR we want to pursue, or should it just be closed after all this time?

enxebre · 2019-10-28T08:40:38Z

please let's elaborate an enhancement PR and reopen against master if still relevant

michaelgugino · 2019-10-28T13:10:39Z

FYI: generic upstream discussion: kubernetes-sigs#1514

openshift-ci-robot added the size/S Denotes a PR that changes 10-29 lines, ignoring generated files. label Jul 3, 2019

michaelgugino force-pushed the preserve-annotation branch from 9961632 to 6c624f3 Compare July 3, 2019 18:01

openshift-ci-robot requested review from bison and vikaschoudhary16 July 3, 2019 18:01

vikaschoudhary16 reviewed Jul 5, 2019

View reviewed changes

This was referenced Jul 5, 2019

Add support for node annotation to disable node fencing. openshift/machine-api-operator#333

Closed

Disable a machine from being fenced openshift/machine-api-operator#332

Closed

michaelgugino force-pushed the preserve-annotation branch from 6c624f3 to 1c70002 Compare July 10, 2019 16:31

openshift-ci-robot added size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. and removed size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Jul 10, 2019

michaelgugino force-pushed the preserve-annotation branch from 1c70002 to 485ee23 Compare July 10, 2019 17:29

michaelgugino force-pushed the preserve-annotation branch from 485ee23 to 920a788 Compare July 10, 2019 20:19

vikaschoudhary16 mentioned this pull request Jul 11, 2019

Bug 1752088: Refactor isDeleteAllowed to remove most logic #49

Closed

enxebre closed this Oct 28, 2019

Add prevent deleting annotation #48

Add prevent deleting annotation #48

Uh oh!

Conversation

michaelgugino commented Jul 3, 2019

Uh oh!

openshift-ci-robot commented Jul 3, 2019

Uh oh!

vikaschoudhary16 Jul 5, 2019

Choose a reason for hiding this comment

Uh oh!

beekhof Jul 8, 2019

Choose a reason for hiding this comment

Uh oh!

vikaschoudhary16 commented Jul 5, 2019

Uh oh!

ingvagabund commented Jul 11, 2019

Uh oh!

enxebre commented Jul 11, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

bison commented Jul 11, 2019

Uh oh!

michaelgugino commented Jul 11, 2019

Uh oh!

openshift-ci-robot commented Sep 5, 2019

Uh oh!

openshift-bot commented Sep 25, 2019

Uh oh!

openshift-ci-robot commented Sep 25, 2019

Uh oh!

openshift-bot commented Sep 25, 2019

Uh oh!

openshift-ci-robot commented Sep 25, 2019

Uh oh!

eparis commented Oct 26, 2019

Uh oh!

enxebre commented Oct 28, 2019

Uh oh!

michaelgugino commented Oct 28, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

9 participants

enxebre commented Jul 11, 2019 •

edited

Loading