🌱 Preserve finalizers during MS/Machine reconciliation #10694

sbueringer · 2024-05-28T14:54:24Z

Signed-off-by: Stefan Büringer [email protected]

What this PR does / why we need it:
We had some discussion around what the proper behavior is.

My current take is:

MS controller should ensure its own finalizer is set
MD/MS controller should preserve all existing finalizers (including foregroundDeletion)

Which issue(s) this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close the issue(s) when PR gets merged):
Fixes #

sbueringer · 2024-05-28T14:54:45Z

/hold
for consensus. I'll fix unit tests afterwards

(cc @vincepri @fabriziopandini @enxebre @chrischdi please note "My current take" in the PR description)

internal/controllers/machineset/machineset_controller.go

internal/controllers/machinedeployment/machinedeployment_sync.go

sbueringer · 2024-05-28T16:12:46Z

@vincepri PTAL :)

(updated PR description)

sbueringer · 2024-05-28T17:25:50Z

test/e2e/md_scale.go

@@ -123,6 +124,17 @@ func MachineDeploymentScaleSpec(ctx context.Context, inputGetter func() MachineD
 			Replicas:                  1,
 			WaitForMachineDeployments: input.E2EConfig.GetIntervals(specName, "wait-worker-nodes"),
 		})
+
+		By("Deleting the MachineDeployment with foreground deletion")


Just added this test to verify that foreground deletion works at the moment.

Long-term we would like to have the same behavior as with the Cluster (that MD is deleted "in the foreground" independent of if foreground or background deletion is used)

Do we have an issue to discuss "Implement forced foreground deletion"?

I would create one.

xref: #10710

sbueringer · 2024-05-28T17:44:17Z

internal/controllers/machinedeployment/machinedeployment_sync.go

@@ -257,15 +257,8 @@ func (r *Reconciler) computeDesiredMachineSet(ctx context.Context, deployment *c
 		name = existingMS.Name


// Add foregroundDeletion finalizer to MachineSet if the MachineDeployment has it. if sets.New[string](deployment.Finalizers...).Has(metav1.FinalizerDeleteDependents) { finalizers = []string{metav1.FinalizerDeleteDependents} }

in l.234

I think this code is basically unreachable. Because we never create a MachineSet after the deletionTimestamp is already set. And the finalizer and the deletionTimestamp seem to be set by the kube-apiserver through the delete call at the same time:

The Kubernetes API server sets the object's metadata.deletionTimestamp field to the time the object was marked for deletion.

The Kubernetes API server also sets the metadata.finalizers field to foregroundDeletion.

The object remains visible through the Kubernetes API until the deletion process is complete.
https://kubernetes.io/docs/concepts/architecture/garbage-collection/#foreground-deletion

Technically someone could send the foregroundDeletion finalizer already earlier, but I think in any case the finalizer would be propagated down (see e2e test coverage)

wouldn't this bit of code ensure that when you are removing a MD with foreground (via client), then foreground would be also honoured between the owned MS deletion and their child Machines?

I think it doesn't change that behavior. I did the following test with this PR:

Create Cluster & MD

Add additional "test" finalizer to MS & Machine

Delete MD with foreground

Check that:

MD gets the foregroundDeletion finalizer + deletionTimestamp

MS gets the foregroundDeletion finalizer + deletionTimestamp

Machine gets the deletionTimestamp

Then I:

Removed test finalizer from Machine => Machine went away

Removed test finalizer from MachinSet => MachineSet and then MachineDeployment went away

Please note that computeDesiredMachineSet is not executed anymore after the deletionTimestamp has been set on MachineDeployment (because we run reconcileDelete instead)

So I think this is:

unreachable code (except in the case where someone already sets the foregroundDeletion finalizer on the MD manually before MD deletion)

and anyway already additionally done by the kube-controller-manager

vincepri · 2024-05-28T22:58:55Z

/lgtm

k8s-ci-robot · 2024-05-28T22:59:01Z

LGTM label has been added.

Git tree hash: 59d8b33236837b721d8bf625c4326200c6b63c34

sbueringer · 2024-05-29T09:08:23Z

/test pull-cluster-api-e2e-main

enxebre · 2024-05-29T15:59:34Z

/lgtm

k8s-ci-robot · 2024-05-29T15:59:41Z

LGTM label has been added.

Git tree hash: d9c59de32cd9277b532cca2838f4d94fa6bc45d9

fabriziopandini

Nice! Preserving finalizers makes more sense than the current behaviour
q: should we retitle the PR to surface this as a main point of this PR?

fabriziopandini · 2024-05-30T14:35:14Z

test/framework/machinedeployment_helpers.go

+	Expect(input.ClusterProxy.GetClient().Delete(ctx, input.MachineDeployment, &client.DeleteOptions{PropagationPolicy: input.DeletePropagationPolicy})).To(Succeed())
+
+	log.Logf("Waiting for MD to be deleted")
+	Eventually(func(g Gomega) {


q: should we also check that all the machines are gone? (so we check one layer more of the hierarchy)

Signed-off-by: Stefan Büringer [email protected]

sbueringer · 2024-05-31T14:07:34Z

/test pull-cluster-api-e2e-main

sbueringer · 2024-05-31T14:07:49Z

q: should we retitle the PR to surface this as a main point of this PR?

Done

enxebre · 2024-06-03T08:30:17Z

/lgtm

k8s-ci-robot · 2024-06-03T08:30:23Z

LGTM label has been added.

Git tree hash: 4cdcfed36001da741a45ae48c046fdd94c119e42

sbueringer · 2024-06-03T09:22:11Z

@fabriziopandini @chrischdi @vincepri ready to merge?

sbueringer · 2024-06-10T09:21:50Z

@fabriziopandini @chrischdi @vincepri can we merge this PR?

vincepri · 2024-06-11T17:06:02Z

/approve

k8s-ci-robot · 2024-06-11T17:06:11Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: vincepri

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:

~~OWNERS~~ [vincepri]

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

sbueringer · 2024-06-11T17:11:39Z

/hold cancel

k8s-ci-robot requested review from chrischdi and vincepri May 28, 2024 14:54

k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. do-not-merge/needs-area PR is missing an area label size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels May 28, 2024

k8s-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label May 28, 2024

vincepri reviewed May 28, 2024

View reviewed changes

internal/controllers/machineset/machineset_controller.go Outdated Show resolved Hide resolved

sbueringer force-pushed the pr-fixup-finalizers branch from 7c008ab to cfddc5b Compare May 28, 2024 15:00

sbueringer commented May 28, 2024

View reviewed changes

internal/controllers/machinedeployment/machinedeployment_sync.go Outdated Show resolved Hide resolved

sbueringer force-pushed the pr-fixup-finalizers branch from cfddc5b to 38c0b38 Compare May 28, 2024 16:10

k8s-ci-robot added size/S Denotes a PR that changes 10-29 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels May 28, 2024

sbueringer force-pushed the pr-fixup-finalizers branch from 38c0b38 to 1f83a1f Compare May 28, 2024 17:24

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 28, 2024

sbueringer commented May 28, 2024

View reviewed changes

sbueringer added area/machineset Issues or PRs related to machinesets area/machinedeployment Issues or PRs related to machinedeployments labels May 28, 2024

k8s-ci-robot removed do-not-merge/needs-area PR is missing an area label labels May 28, 2024

k8s-ci-robot assigned vincepri May 28, 2024

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

sbueringer force-pushed the pr-fixup-finalizers branch from 1f83a1f to 4d0e5f9 Compare May 29, 2024 08:56

k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label May 29, 2024

k8s-ci-robot requested a review from vincepri May 29, 2024 08:56

k8s-ci-robot assigned enxebre May 29, 2024

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

fabriziopandini reviewed May 30, 2024

View reviewed changes

sbueringer changed the title ~~🌱 Fix finalizer calculation in MD/MS controller~~ 🌱 Preserve finalizers during MS/Machine reconciliation May 31, 2024

Fix finalizer calculation in MD/MS controller

049a781

Signed-off-by: Stefan Büringer [email protected]

sbueringer force-pushed the pr-fixup-finalizers branch from 4d0e5f9 to 049a781 Compare May 31, 2024 14:07

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

k8s-ci-robot requested a review from enxebre May 31, 2024 14:07

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jun 3, 2024

vincepri approved these changes Jun 11, 2024

View reviewed changes

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jun 11, 2024

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 11, 2024

k8s-ci-robot merged commit f1f8f38 into kubernetes-sigs:main Jun 11, 2024
21 checks passed

k8s-ci-robot added this to the v1.8 milestone Jun 11, 2024

sbueringer deleted the pr-fixup-finalizers branch June 11, 2024 18:08

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

🌱 Preserve finalizers during MS/Machine reconciliation #10694

🌱 Preserve finalizers during MS/Machine reconciliation #10694

sbueringer commented May 28, 2024 •

edited

Loading

sbueringer commented May 28, 2024 •

edited

Loading

sbueringer commented May 28, 2024

sbueringer May 28, 2024 •

edited

Loading

enxebre May 29, 2024

sbueringer May 29, 2024

sbueringer May 31, 2024

sbueringer May 28, 2024 •

edited

Loading

sbueringer May 29, 2024

enxebre May 29, 2024 •

edited

Loading

sbueringer May 29, 2024 •

edited

Loading

vincepri commented May 28, 2024

k8s-ci-robot commented May 28, 2024

sbueringer commented May 29, 2024

enxebre commented May 29, 2024

k8s-ci-robot commented May 29, 2024

fabriziopandini left a comment

fabriziopandini May 30, 2024

sbueringer May 31, 2024

sbueringer commented May 31, 2024

sbueringer commented May 31, 2024

enxebre commented Jun 3, 2024

k8s-ci-robot commented Jun 3, 2024

sbueringer commented Jun 3, 2024

sbueringer commented Jun 10, 2024 •

edited

Loading

vincepri commented Jun 11, 2024

k8s-ci-robot commented Jun 11, 2024

sbueringer commented Jun 11, 2024

		@@ -257,15 +257,8 @@ func (r Reconciler) computeDesiredMachineSet(ctx context.Context, deployment c
		name = existingMS.Name

🌱 Preserve finalizers during MS/Machine reconciliation #10694

🌱 Preserve finalizers during MS/Machine reconciliation #10694

Conversation

sbueringer commented May 28, 2024 • edited Loading

sbueringer commented May 28, 2024 • edited Loading

sbueringer commented May 28, 2024

sbueringer May 28, 2024 • edited Loading

Choose a reason for hiding this comment

enxebre May 29, 2024

Choose a reason for hiding this comment

sbueringer May 29, 2024

Choose a reason for hiding this comment

sbueringer May 31, 2024

Choose a reason for hiding this comment

sbueringer May 28, 2024 • edited Loading

Choose a reason for hiding this comment

sbueringer May 29, 2024

Choose a reason for hiding this comment

enxebre May 29, 2024 • edited Loading

Choose a reason for hiding this comment

sbueringer May 29, 2024 • edited Loading

Choose a reason for hiding this comment

vincepri commented May 28, 2024

k8s-ci-robot commented May 28, 2024

sbueringer commented May 29, 2024

enxebre commented May 29, 2024

k8s-ci-robot commented May 29, 2024

fabriziopandini left a comment

Choose a reason for hiding this comment

fabriziopandini May 30, 2024

Choose a reason for hiding this comment

sbueringer May 31, 2024

Choose a reason for hiding this comment

sbueringer commented May 31, 2024

sbueringer commented May 31, 2024

enxebre commented Jun 3, 2024

k8s-ci-robot commented Jun 3, 2024

sbueringer commented Jun 3, 2024

sbueringer commented Jun 10, 2024 • edited Loading

vincepri commented Jun 11, 2024

k8s-ci-robot commented Jun 11, 2024

sbueringer commented Jun 11, 2024

sbueringer commented May 28, 2024 •

edited

Loading

sbueringer commented May 28, 2024 •

edited

Loading

sbueringer May 28, 2024 •

edited

Loading

sbueringer May 28, 2024 •

edited

Loading

enxebre May 29, 2024 •

edited

Loading

sbueringer May 29, 2024 •

edited

Loading

sbueringer commented Jun 10, 2024 •

edited

Loading