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.

Sign up for GitHub

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

🐛 ClusterClass: use namePrefix func consistently, fix MachineDeployment template rotation #5164

Merged
merged 1 commit into from
Aug 27, 2021
Merged

🐛 ClusterClass: use namePrefix func consistently, fix MachineDeployment template rotation #5164

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 5 additions & 4 deletions controllers/topology/desired_state.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -124,7 +124,7 @@ func computeControlPlaneInfrastructureMachineTemplate(_ context.Context, s *scop
template: template,
templateClonedFromRef: templateClonedFromref,
cluster: cluster,
namePrefix: fmt.Sprintf("%s-controlplane-", cluster.Name),
namePrefix: controlPlaneInfrastructureMachineTemplateNamePrefix(cluster.Name),
currentObjectRef: currentRef,
labels: mergeMap(topologyMetadata.Labels, clusterClassMetadata.Labels),
annotations: mergeMap(topologyMetadata.Annotations, clusterClassMetadata.Annotations),
Expand Down Expand Up @@ -264,7 +264,9 @@ func computeMachineDeployment(_ context.Context, s *scope.Scope, machineDeployme
ClusterName: s.Current.Cluster.Name,
Template: clusterv1.MachineTemplateSpec{
Spec: clusterv1.MachineSpec{
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
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ykakarap this should be already in your radar, right? ^^^

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes. The cluster class upgrade PR that I am working on also deals with machine deployments.

Version: pointer.String(s.Blueprint.Topology.Version),
Bootstrap: clusterv1.Bootstrap{ConfigRef: contract.ObjToRef(desiredMachineDeployment.BootstrapTemplate)},
InfrastructureRef: *contract.ObjToRef(desiredMachineDeployment.InfrastructureMachineTemplate),
Expand Down Expand Up @@ -293,8 +295,7 @@ func computeMachineDeployment(_ context.Context, s *scope.Scope, machineDeployme
// NOTE: Topology label takes precedence on labels defined in the topology/in the ClusterClass.
desiredMachineDeploymentObj.Annotations = mergeMap(machineDeploymentTopology.Metadata.Annotations, machineDeploymentBlueprint.Metadata.Annotations)

// Sets the desired Kubernetes version for the control plane.
// TODO: improve this logic by adding support for version upgrade component by component
// Set the desired replicas.
desiredMachineDeploymentObj.Spec.Replicas = machineDeploymentTopology.Replicas

desiredMachineDeployment.Object = desiredMachineDeploymentObj
Expand Down
8 changes: 4 additions & 4 deletions controllers/topology/reconcile_state.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -147,7 +147,7 @@ func (r *ClusterReconciler) reconcileMachineDeployments(ctx context.Context, s *
for _, mdTopologyName := range diff.toUpdate {
currentMD := s.Current.MachineDeployments[mdTopologyName]
desiredMD := s.Desired.MachineDeployments[mdTopologyName]
if err := r.updateMachineDeployment(ctx, s.Current.Cluster.Name, currentMD, desiredMD); err != nil {
if err := r.updateMachineDeployment(ctx, s.Current.Cluster.Name, mdTopologyName, currentMD, desiredMD); err != nil {
return err
}
}
Expand Down Expand Up @@ -187,15 +187,15 @@ func (r *ClusterReconciler) createMachineDeployment(ctx context.Context, md *sco
}

// updateMachineDeployment updates a MachineDeployment. Also rotates the corresponding Templates if necessary.
func (r *ClusterReconciler) updateMachineDeployment(ctx context.Context, clusterName string, currentMD, desiredMD *scope.MachineDeploymentState) error {
func (r *ClusterReconciler) updateMachineDeployment(ctx context.Context, clusterName string, mdTopologyName string, currentMD, desiredMD *scope.MachineDeploymentState) error {
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)
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

},
compatibilityChecker: check.ReferencedObjectsAreCompatible,
})
Expand All @@ -208,7 +208,7 @@ func (r *ClusterReconciler) updateMachineDeployment(ctx context.Context, cluster
current: currentMD.BootstrapTemplate,
desired: desiredMD.BootstrapTemplate,
templateNamer: func() string {
return bootstrapTemplateNamePrefix(clusterName, desiredMD.Object.Name)
return bootstrapTemplateNamePrefix(clusterName, mdTopologyName)
},
compatibilityChecker: check.ObjectsAreInTheSameNamespace,
})
Expand Down
33 changes: 12 additions & 21 deletions controllers/topology/reconcile_state_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -267,14 +267,6 @@ func TestReconcileControlPlaneObject(t *testing.T) {
want: &scope.ControlPlaneState{Object: controlPlane3, InfrastructureMachineTemplate: infrastructureMachineTemplate},
wantErr: false,
},
{
// Will panic due to the design of logging.
name: "Attempt to update controlPlane on controlPlaneState with no infrastructureMachineTemplate",
class: ccWithControlPlaneInfrastructure,
current: &scope.ControlPlaneState{Object: controlPlane1},
desired: &scope.ControlPlaneState{Object: controlPlane3},
wantErr: true,
},
{
name: "Update to ControlPlaneObject with no underlying infrastructure",
class: ccWithoutControlPlaneInfrastructure,
Expand All @@ -294,19 +286,19 @@ func TestReconcileControlPlaneObject(t *testing.T) {
}
for _, tt := range tests {
t.Run(tt.name, func(t *testing.T) {
// 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())
}
}
}()

Comment on lines -297 to -307
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Removing this hack was on my list too. Thanks for addressing it

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

fakeObjs := make([]client.Object, 0)
s := scope.New(nil)

Copy link
Member Author

@sbueringer sbueringer Aug 26, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  1. The defer above wasn't really intended to be merged that way
  2. Through a few refactorings we hit a new panic because ClusterClass was nil (and it didn't surface because of the defer above)

s.Blueprint = &scope.ClusterBlueprint{
ClusterClass: &clusterv1.ClusterClass{},
}
if tt.class.InfrastructureMachineTemplate != nil {
s.Blueprint.ClusterClass.Spec.ControlPlane.MachineInfrastructure = &clusterv1.LocalObjectTemplate{
Ref: contract.ObjToRef(tt.class.InfrastructureMachineTemplate),
}
}

s.Current.ControlPlane = &scope.ControlPlaneState{}
if tt.current != nil {
s.Current.ControlPlane = tt.current
if tt.current.Object != nil {
Expand Down Expand Up @@ -481,8 +473,7 @@ func TestReconcileControlPlaneInfrastructureMachineTemplate(t *testing.T) {
if tt.current.InfrastructureMachineTemplate != nil {
item, err := contract.ControlPlane().InfrastructureMachineTemplate().Get(gotControlPlaneObject)
g.Expect(err).ToNot(HaveOccurred())
// This pattern should match return value in controlPlaneinfrastructureMachineTemplateNamePrefix
pattern := fmt.Sprintf("%s-controlplane-.*", s.Current.Cluster.Name)
pattern := fmt.Sprintf("%s.*", controlPlaneInfrastructureMachineTemplateNamePrefix(s.Current.Cluster.Name))
fmt.Println(pattern, item.Name)
ok, err := regexp.Match(pattern, []byte(item.Name))
g.Expect(err).NotTo(HaveOccurred())
Expand Down
2 changes: 1 addition & 1 deletion controllers/topology/util.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Copy link
Member Author

@sbueringer sbueringer Aug 26, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

}

// getReference gets the object referenced in ref.
Expand Down