🐛 clusterctl: update move to support new cluster validations #5625
Conversation
k8s-ci-robot
added
cncf-cla: yes
|
/test pull-cluster-api-e2e-full-main |
|
/retest |
There was a problem hiding this comment.
Only did a high-level review for now.
Q: Did I understood the clusterctl move logic correctly:
- target cluster: create one group after another
- source cluster: delete one group after another in reverse order
That would mean:
- target cluster: ClusterClasses are created before Clusters
- source cluster: Clusters are deleted before ClusterClasses
(of course only if they actually depend on each other)
If I got it right, the move will still work when we introduce a webhook on the ClusterClass, which makes sure that a ClusterClass is only deleted after it's not used anymore in any Clusters.
| // if the cluster uses a managed topoloy and uses the clusterclass | ||
| // set the cluster as a soft owner of the clusterclass. | ||
| if className, ok := cluster.additionalInfo[clusterTopologyNameKey]; ok { | ||
| if className == clusterClass.identity.Name && clusterClass.identity.Namespace == cluster.identity.Namespace { |
There was a problem hiding this comment.
Just for my understanding: why would we want to check the group?
Here we are making sure that the clusterclass used in the cluster spec (cluster.spec.topology.class) matches the clusterclass we are checking against and we are making sure that the cluster and the clusterclass belong to the same namespace because we know that this is a requirement.
By checking groups we are saying that clusters and clusterclasses should belong to the same k8s group cluster.x-k8s.io. I dont see how this is a requirement to establish a soft ownership.
Am I missing something?
There was a problem hiding this comment.
No you are right.
I did not look at the code above and I did assume we were looping through all the objects, not only clusters
ykakarap
force-pushed
the
clusterctl-move-order
branch
from
9eb4deb
to
4f8a5e8
Compare
|
/test pull-cluster-api-e2e-full-main |
ykakarap
force-pushed
the
clusterctl-move-order
branch
from
4f8a5e8
to
dc00f45
Compare
|
/test pull-cluster-api-e2e-full-main |
|
@sbueringer That is correct. Because of the new soft ownership between the clusters and the cluster classes that are in use we can guarantee that any ClusterClass that is in use will be moved to the target cluster before the cluster object is moved. |
|
/lgtm |
k8s-ci-robot
added
the
lgtm
|
[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:
Approvers can indicate their approval by writing |
k8s-ci-robot
added
the
approved
What this PR does / why we need it:
This PR fixes the clusterctl move order so that any ClusterClass that are in use are moved to the target cluster before the Clusters that used them are moved.
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 #5620