Rearranging control plane migration task order to ensure that all objects are deleted

[krgostev]

How to categorize this PR?

/area control-plane-migration
/kind enhancement
/priority normal

What this PR does / why we need it:
When the control plane migration is executed there is a chance that some objects are not deleted due to the task arrangement and the dependencies between them. With this PR the task order of the control plane migration are rearranged in a way that the deletion of the managed resources from the Shoot`s namespace task will wait for the deletion of the extension CRs from the Shoot namespace and the deletion of the BackupEntires from Seed.

Which issue(s) this PR fixes:
Fixes #

Special notes for your reviewer:
The commented code in shoot.go is preventing the "seedName" field in the shoot from being edited. Should this be deleted in not it it will be included in some other PR?
Release note:

The control plane migration task order is rearranged to ensure the deletion of all objects.

[gardener-robot]

@kris94 Thank you for your contribution.

[CLAassistant]

All committers have signed the CLA.

[krgostev]

/assign @plkokanov

[gardener-robot]

@kris94 Only code owners may change assignees. Commenters may only assign themselves.

[rfranzke]

/invite @plkokanov @vlvasilev @stoyanr @swilen-iwanow

[rfranzke]

Thanks, please squash the commits.

[stoyanr]

I tested the change, it works, and I also have no comments to the code itself. The only concern that I have is that the description in the PR about the potential issue, "there is a chance that some objects are not deleted due to the task arrangement and the dependencies between them", is fairly vague. How could one reproduce an issue with the previous arrangement, so that we could test that the new arrangement actually fixes it? Have you actually tested this?

[plkokanov]

The problem that this should fix is the following:

1. The managed resources are flagged with keepObjects: true
2. The managed resources are deleted
3. A reconciliation is triggered in one of the extension controllers which deploys new managed resources (this happens as the deletion of extension resources is later in the flow and maybe the extension controller had an error on its previous reconciliation)
4. This causes the namespace deletion to fail
5. On subsequent executions of the flow we just try to scale up the APIServer and ResourceManager if they already exist and the namespace is not in Terminating state (which it is) if there is no deployments we don't redeploy them. This will cause the migration flow to hang as the leftover managed resource never gets deleted.

I just want to double check the deletion of the backupentry before the deletion of the apiserver and if flagging the managedresources with keepObjects: true is ok to be after the migration of the extensions.

[plkokanov]

I wasn't able to reproduce the original issue, but imho the new ordering makes sense considering extension controllers could have special handling for managedresources that they deploy during the Migration operation. So it seems better to try to clean up all managedresources only after the extension controllers have been migrated and deleted.

However, I tried to migrate a hibernated shoot cluster and the migration seems to hang on:

level=info msg=Succeeded flow="Shoot's control plane preparation for migration" operation=reconcile shoot=garden-mig/migration1 task="Preparing kube-apiserver in Shoot's namespace for migration, by deleting it and its respective hvpa"

level=error msg="Failed to prepare Shoot \"migration1\" for migration: flow \"Shoot's control plane preparation for migration\" encountered task errors: [task \"Waiting until kube-apiserver doesn't exist\" failed: retry failed with context deadline exceeded]" operation=reconcile shoot=garden-mig/migration1

This could not be caused directly from this PR but could you check?

[rfranzke]

/status author-action

[gardener-robot]

@kris94 The pull request was assigned to you under author-action. Please unassign yourself when you are done. Thank you.

[swilen-iwanow]

I wasn't able to reproduce the original issue, but imho the new ordering makes sense considering extension controllers could have special handling for managedresources that they deploy during the Migration operation. So it seems better to try to clean up all managedresources only after the extension controllers have been migrated and deleted.

However, I tried to migrate a hibernated shoot cluster and the migration seems to hang on:

level=info msg=Succeeded flow="Shoot's control plane preparation for migration" operation=reconcile shoot=garden-mig/migration1 task="Preparing kube-apiserver in Shoot's namespace for migration, by deleting it and its respective hvpa"

level=error msg="Failed to prepare Shoot \"migration1\" for migration: flow \"Shoot's control plane preparation for migration\" encountered task errors: [task \"Waiting until kube-apiserver doesn't exist\" failed: retry failed with context deadline exceeded]" operation=reconcile shoot=garden-mig/migration1

This could not be caused directly from this PR but could you check?

This is very strange I will try to reproduce it also.

[swilen-iwanow]

I can confirm that the issue described by Plamen is not caused by this PR. For couple of reasons we've added a wakeup step in the migration flow for the kube-apiserver and this is causing an issue for hibernated Shoots because the scale down of the deployment for hibernated Shoots is skipped and the wake up step timeouts . I managed to pass around the problematic step by removing the ".Skip", but we should look into it and check if only removing the skip is OK, or more work should be done. Anyway, this is not part of this PR and we can fix this in a separate one.

[rfranzke]

@swilen-iwanow thanks for your check. So is this PR ready to be merged?

[plkokanov]

Just needs a rebase to include the changes for the DNSEntries and then it should be ok.

[plkokanov]

lgtm