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

Ensure owner references are correctly re-reconciled #7575

Closed
killianmuldoon opened this issue Nov 21, 2022 · 8 comments · Fixed by #7606
Closed

Ensure owner references are correctly re-reconciled #7575

killianmuldoon opened this issue Nov 21, 2022 · 8 comments · Fixed by #7606
Assignees
Labels
kind/bug Categorizes issue or PR as related to a bug. triage/accepted Indicates an issue or PR is ready to be actively worked on.

Comments

@killianmuldoon
Copy link
Contributor

killianmuldoon commented Nov 21, 2022

The test in ##7569 shows that some of the ownerReferences in Cluster API are not correctly re-reconciled if they are removed for any reason. This is an umbrella issue to capture the different cases under which this can happen.

The expectation is that the ownerReference tree should look the same as on cluster creation after being re-reconciled. The following differences can be seen in #7569

  1. KCP machines and standalone MachineSets can be wrongly adopted by the Cluster on re-reconcile
  2. Secret ownerReferences are not re-rereconciled correctly Secret ownerReferences not restored #6979
  3. ClusterResourceSet configmap references are not re-reconciled
  4. KubeadmConfig and DockerMachines created by a MachineSet are initially created with two OwnerReferences: 1 for the Machine it's linked to and a second for the higher level controlling type or MachineSet.
  5. ClusterClass-owned templates are not re-reconciled with their ownerReferences (There is an issue here where the ClusterClass is reconciled infrequently so it takes longer for these references to be re-reconciled. The ownerReference tree is evenually as expected in this case though, as the ClusterClass controller reconciles these references correctly.)

ClusterClass clusters

The ownerReference tree on creation:
owner-references-c28fk6-before

The ownerReference tree after removing and re-reconciling:
owner-references-c28fk6-after

Non-ClusterClass clusters

The ownerReference tree on
owner-references-mmgybv-before
creation:

The ownerReference tree after removing and re-reconciling
owner-references-mmgybv-after

/kind bug

@k8s-ci-robot k8s-ci-robot added kind/bug Categorizes issue or PR as related to a bug. needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one. labels Nov 21, 2022
@sbueringer
Copy link
Member

sbueringer commented Nov 22, 2022

/triage accepted

@killianmuldoon I'm not sure who is already working on which sub-task. Please let me know if I should take over one of them.

@k8s-ci-robot k8s-ci-robot added triage/accepted Indicates an issue or PR is ready to be actively worked on. and removed needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one. labels Nov 22, 2022
@killianmuldoon
Copy link
Contributor Author

/assign

@fabriziopandini
Copy link
Member

fabriziopandini commented Nov 22, 2022

Thanks for this extensive work of research.
Most of the issues are due to support for some "legacy" stuff, like control-plane machines without a control-plane object controlling them, or stand-alone machines. I will trigger some discussion about dropping support for some of this, which is healthy for the entire project.

But, for now, praise for this work making the project more robust and resilient to ownerRefs being accidentally removed.

@sbueringer
Copy link
Member

I will trigger some discussion about triggering the process for dropping support for some of this, which is healthy for the entire project.

Thx. Absolutely agree. We should continuously try to get rid of things which are not needed anymore.

But, for now, praise for this work making the project more robust and resilient to ownerRefs being accidentally removed.

+100. Thx @killianmuldoon great work!!

@sbueringer
Copy link
Member

@sbueringer
Copy link
Member

@killianmuldoon Just out of curiosity in case you just know it.

Is the ClusterClass ownerRef tree equivalent to the non-ClusterClass tree just with more ownerRefs for the ClusterClass+referenced templates? (it's a bit hard to diff in the pictures)

@killianmuldoon
Copy link
Contributor Author

All tasks now done - will close this once #7606 is merged.

@killianmuldoon
Copy link
Contributor Author

/close

#7606 is approved

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/bug Categorizes issue or PR as related to a bug. triage/accepted Indicates an issue or PR is ready to be actively worked on.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants