🐛 ClusterClass: use namePrefix func consistently, fix MachineDeployment template rotation #5164
Conversation
k8s-ci-robot
added
cncf-cla: yes
… template rotation, fix unit test panic Signed-off-by: Stefan Büringer [email protected]
sbueringer
force-pushed
the
pr-misc-clusterclass-fixes
branch
from
de069d9 to
cf6797b
Compare
k8s-ci-robot
removed
the
needs-rebase
sbueringer
changed the title
| log := ctrl.LoggerFrom(ctx) | ||
|
|
||
| cleanupOldInfrastructureTemplate, err := r.reconcileReferencedTemplate(ctx, reconcileReferencedTemplateInput{ | ||
| ref: &desiredMD.Object.Spec.Template.Spec.InfrastructureRef, | ||
| current: currentMD.InfrastructureMachineTemplate, | ||
| desired: desiredMD.InfrastructureMachineTemplate, | ||
| templateNamer: func() string { | ||
| return infrastructureMachineTemplateNamePrefix(clusterName, desiredMD.Object.Name) | ||
| return infrastructureMachineTemplateNamePrefix(clusterName, mdTopologyName) |
There was a problem hiding this comment.
Previously after the first rotation the templates were looking like this: my-cluster-my-cluster-mdTopologyName...
Now the name is calculated exactly the same way as during compute.
| fakeObjs := make([]client.Object, 0) | ||
| s := scope.New(nil) | ||
|
|
There was a problem hiding this comment.
- The defer above wasn't really intended to be merged that way
- Through a few refactorings we hit a new panic because ClusterClass was nil (and it didn't surface because of the defer above)
| @@ -39,7 +39,7 @@ func infrastructureMachineTemplateNamePrefix(clusterName, machineDeploymentTopol | |||
|
|
|||
| // infrastructureMachineTemplateNamePrefix calculates the name prefix for a InfrastructureMachineTemplate. | |||
| func controlPlaneInfrastructureMachineTemplateNamePrefix(clusterName string) string { | |||
| return fmt.Sprintf("%s-controlplane-", clusterName) | |||
| return fmt.Sprintf("%s-control-plane-", clusterName) | |||
There was a problem hiding this comment.
I think it's better to align to how we call it in our existing CAPD templates. And it matches how we "camelcase" our func/variable names as well.
| ClusterName: s.Current.Cluster.Name, | ||
| ClusterName: s.Current.Cluster.Name, | ||
| // Sets the desired Kubernetes version for the MachineDeployment. | ||
| // TODO: improve this logic by adding support for version upgrade component by component |
There was a problem hiding this comment.
@ykakarap this should be already in your radar, right? ^^^
There was a problem hiding this comment.
Yes. The cluster class upgrade PR that I am working on also deals with machine deployments.
| // this panic catcher catches the case when there is some issue with the clusterClass controlPlaneInfrastructureCheck that causes it to falsely proceed | ||
| // the test case that throws this panic shows that the structure of our logs is prone to panic if some of our assumptions are off. | ||
| defer func() { | ||
| if r := recover(); r != nil { | ||
| if tt.wantErr { | ||
| err := fmt.Errorf("panic occurred during testing") | ||
| g.Expect(err).To(HaveOccurred()) | ||
| } | ||
| } | ||
| }() | ||
|
|
There was a problem hiding this comment.
Removing this hack was on my list too. Thanks for addressing it
There was a problem hiding this comment.
I cleaned it up together with Killian :)
I actually only found it because I wanted to run all tests of this package at once via Intellij to make sure they all still work and then my Intellij panic break point found it.
k8s-ci-robot
added
the
lgtm
|
/lgtm thanks for this! |
|
[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
Signed-off-by: Stefan Büringer [email protected]
What this PR does / why we need it:
A few fixes from issues found today during manual testing.
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 #