🐛 Implement template deletion for topology-owned MD and MS

[sbueringer]

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

What this PR does / why we need it:

Before this PR, our topology code:

• deleted "old" templates after template rotation of MachineDeployments immediately
  • thus reconciliation of the old MachineSets was immediately broken as the MachineSet controller requires the templates
• didn't delete corresponding templates when deleting MachineDeployments

This PR aims to solve this problem. The high-level concept is:

• topology code doesn't delete templates in "successful" reconciles (template rotation without errors / MachineDeployment deletions)
  • Note: we will have to introduce another separate mechanism which cleans up templates when the template rotation fails (e.g. new templates were never referenced in any MachineDeployment or MachineSet)
• we add new MD and MS topology reconcilers which on MD/MS delete:
  • calculate which templates are still used by MachineDeployments or MachineSets not in deleting
  • deletes unused templates

Note: this is only done for topology-owned MD and MS

tl;dr we cleanup templates which would otherwise be orphaned during deletion of topology-owned MachineDeployments and MachineSets

I tested the following locally:

• rotate infra/bootstrap templates in MDs and ensure old templates are properly deleted
• delete MDs and ensure old templates are properly deleted

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]

/assign @vincepri
(feedback about the general approach would be great, it's more than a bit tricky and full of assumptions)

[sbueringer]

Removed WIP. Fixed a few minor things and tested it locally (for details see PR description)

[vincepri]

Reviewing now

[sbueringer]

A few notes about why the current implementation is only one MD reconciler:

• Initially it was not possible to detect if a MachineSet is topology owned. That's why I wanted to always reconcile MDs, but later on I found out that during MD deletion MDs are deleted first, so we had to propagated the topology owned label to MS anyway.
• I though it's a good idea to "synchronize" reconciliation of MD and its related MS to avoid race conditions. Given what I know now about how MD/MS deletion works, I'm not 100% sure, but I think there are no race conditions. For example if multiple MS are deleted concurrently we always have one reconciliation loop which will see the deletionTimestamp on both MS and can then delete templates accordingly.

Overall, I now think it would be better to split up the reconciler in one MD and one MS reconciler to simplify the logic. The new logic would just be:

• MD reconciler
  • if MD in deletion and not paused: check if templates are used in corresponding MS, otherwise delete them
• MS reconciler
  • if MS in deletion and not paused: check if templates are used in MD/ corresponding MS, otherwise delete them

I think it would be simpler because in the current implementation one problem is that I'm reconciling a MachineDeployment which could be already gone.

Give @vincepri's suggestion here. I would also inline this behaviour in the current MD/MS controllers.

@fabriziopandini / @vincepri WDYT

[fabriziopandini]

(in case we are not moving this into exiting controllers)

WRT to the discussion one controller/two controllers,I'm ok with both approaches; in this case if doable having separated controllers, this gives a nice symmetry with existing controllers and a cleaner separation of concerns.

[sbueringer]

@enxebre @vincepri @fabriziopandini PTAL. I separated the reconcilers as discussed in Slack. Now it should be pretty clear how the reconciliation logic looks like.

[sbueringer]

Findings should be addressed. PTAL @enxebre @fabriziopandini @vincepri

@fabriziopandini After moving the template utils back into the package I'm now down to 3 TODOs caused by #5153. So I guess depending on which PR gets ready first we could also merge the current PR before #5153.

#5153 has been merged, rebased on top of it and also squashed the commits

[sbueringer]

/hold cancel (#5153 merged + resolved open TODOs accordingly)

[fabriziopandini]

Only a nit then lgtm for me

[sbueringer]

@enxebre @vincepri @fabriziopandini Should work again (ci works without topology, and locally template rotation and MD deletion works - we really need topology e2e tests :))

[fabriziopandini]

changes lgtm, pending squash / others feedback after latest changes

[enxebre]

lgtm after squashing

[vincepri]

/approve
/lgtm

[k8s-ci-robot]

[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

[vincepri]

@enxebre Over to you to unhold

[enxebre]

/hold cancel