Skip to content

Commit 8c4f8ac

Browse files
k8s-ci-robotk8s-ci-robot
authored
Merge pull request #7403 from sbueringer/pr-fix-md-delete
🐛 ClusterClass: fix reconciliation of MD delete
2 parents 3fca336 + 068c165 commit 8c4f8ac

File tree

2 files changed

+113
-36
lines changed

2 files changed

+113
-36
lines changed

‎internal/controllers/topology/cluster/current_state.go

Lines changed: 40 additions & 36 deletions
Original file line numberDiff line numberDiff line change
@@ -186,60 +186,64 @@ func (r *Reconciler) getCurrentMachineDeploymentState(ctx context.Context, bluep
186186
}
187187

188188
// Make sure that the name of the MachineDeployment stays unique.
189-
// If we've already have seen a MachineDeployment with the same name
189+
// If we've already seen a MachineDeployment with the same name
190190
// this is an error, probably caused from manual modifications or a race condition.
191191
if _, ok := state[mdTopologyName]; ok {
192192
return nil, fmt.Errorf("duplicate %s found for label %s: %s", tlog.KObj{Obj: m}, clusterv1.ClusterTopologyMachineDeploymentLabelName, mdTopologyName)
193193
}
194194

195-
mdClassName := getMDClassName(cluster, mdTopologyName)
196-
if mdClassName == "" {
197-
return nil, fmt.Errorf("failed to find MachineDeployment topology %s in %s", mdTopologyName, tlog.KObj{Obj: cluster})
198-
}
199-
200-
mdBluePrint, ok := blueprintMachineDeployments[mdClassName]
201-
if !ok {
202-
return nil, fmt.Errorf("failed to find MachineDeployment class %s in ClusterClass", mdClassName)
203-
}
204-
205-
// Gets the BootstrapTemplate
195+
// Gets the bootstrapRef.
206196
bootstrapRef := m.Spec.Template.Spec.Bootstrap.ConfigRef
207197
if bootstrapRef == nil {
208198
return nil, fmt.Errorf("%s does not have a reference to a Bootstrap Config", tlog.KObj{Obj: m})
209199
}
210-
ref, err := alignRefAPIVersion(mdBluePrint.BootstrapTemplate, bootstrapRef)
211-
if err != nil {
212-
return nil, errors.Wrap(err, fmt.Sprintf("%s Bootstrap reference could not be retrieved", tlog.KObj{Obj: m}))
200+
// Gets the infraRef.
201+
infraRef := &m.Spec.Template.Spec.InfrastructureRef
202+
if infraRef.Name == "" {
203+
return nil, fmt.Errorf("%s does not have a reference to a InfrastructureMachineTemplate", tlog.KObj{Obj: m})
204+
}
205+
206+
// If the mdTopology exists in the Cluster, lookup the corresponding mdBluePrint and align
207+
// the apiVersions in the bootstrapRef and infraRef.
208+
// If the mdTopology doesn't exist, do nothing (this can happen if the mdTopology was deleted).
209+
// **Note** We can't check if the MachineDeployment has a DeletionTimestamp, because at this point it could not be set yet.
210+
if mdTopologyExistsInCluster, mdClassName := getMDClassName(cluster, mdTopologyName); mdTopologyExistsInCluster {
211+
mdBluePrint, ok := blueprintMachineDeployments[mdClassName]
212+
if !ok {
213+
return nil, fmt.Errorf("failed to find MachineDeployment class %s in ClusterClass", mdClassName)
214+
}
215+
bootstrapRef, err = alignRefAPIVersion(mdBluePrint.BootstrapTemplate, bootstrapRef)
216+
if err != nil {
217+
return nil, errors.Wrap(err, fmt.Sprintf("%s Bootstrap reference could not be retrieved", tlog.KObj{Obj: m}))
218+
}
219+
infraRef, err = alignRefAPIVersion(mdBluePrint.InfrastructureMachineTemplate, infraRef)
220+
if err != nil {
221+
return nil, errors.Wrap(err, fmt.Sprintf("%s Infrastructure reference could not be retrieved", tlog.KObj{Obj: m}))
222+
}
213223
}
214-
b, err := r.getReference(ctx, ref)
224+
225+
// Get the BootstrapTemplate.
226+
bootstrapTemplate, err := r.getReference(ctx, bootstrapRef)
215227
if err != nil {
216228
return nil, errors.Wrap(err, fmt.Sprintf("%s Bootstrap reference could not be retrieved", tlog.KObj{Obj: m}))
217229
}
218230
// check that the referenced object has the ClusterTopologyOwnedLabel label.
219231
// Nb. This is to make sure that a managed topology cluster does not have a reference to an object that is not
220232
// owned by the topology.
221-
if !labels.IsTopologyOwned(b) {
222-
return nil, fmt.Errorf("BootstrapTemplate object %s referenced from MD %s is not topology owned", tlog.KObj{Obj: b}, tlog.KObj{Obj: m})
233+
if !labels.IsTopologyOwned(bootstrapTemplate) {
234+
return nil, fmt.Errorf("BootstrapTemplate object %s referenced from MD %s is not topology owned", tlog.KObj{Obj: bootstrapTemplate}, tlog.KObj{Obj: m})
223235
}
224236

225-
// Gets the InfrastructureMachineTemplate
226-
infraRef := m.Spec.Template.Spec.InfrastructureRef
227-
if infraRef.Name == "" {
228-
return nil, fmt.Errorf("%s does not have a reference to a InfrastructureMachineTemplate", tlog.KObj{Obj: m})
229-
}
230-
ref, err = alignRefAPIVersion(mdBluePrint.InfrastructureMachineTemplate, &infraRef)
231-
if err != nil {
232-
return nil, errors.Wrap(err, fmt.Sprintf("%s Infrastructure reference could not be retrieved", tlog.KObj{Obj: m}))
233-
}
234-
infra, err := r.getReference(ctx, ref)
237+
// Get the InfraMachineTemplate.
238+
infraMachineTemplate, err := r.getReference(ctx, infraRef)
235239
if err != nil {
236240
return nil, errors.Wrap(err, fmt.Sprintf("%s Infrastructure reference could not be retrieved", tlog.KObj{Obj: m}))
237241
}
238242
// check that the referenced object has the ClusterTopologyOwnedLabel label.
239243
// Nb. This is to make sure that a managed topology cluster does not have a reference to an object that is not
240244
// owned by the topology.
241-
if !labels.IsTopologyOwned(infra) {
242-
return nil, fmt.Errorf("InfrastructureMachineTemplate object %s referenced from MD %s is not topology owned", tlog.KObj{Obj: infra}, tlog.KObj{Obj: m})
245+
if !labels.IsTopologyOwned(infraMachineTemplate) {
246+
return nil, fmt.Errorf("InfrastructureMachineTemplate object %s referenced from MD %s is not topology owned", tlog.KObj{Obj: infraMachineTemplate}, tlog.KObj{Obj: m})
243247
}
244248

245249
// Gets the MachineHealthCheck.
@@ -257,8 +261,8 @@ func (r *Reconciler) getCurrentMachineDeploymentState(ctx context.Context, bluep
257261

258262
state[mdTopologyName] = &scope.MachineDeploymentState{
259263
Object: m,
260-
BootstrapTemplate: b,
261-
InfrastructureMachineTemplate: infra,
264+
BootstrapTemplate: bootstrapTemplate,
265+
InfrastructureMachineTemplate: infraMachineTemplate,
262266
MachineHealthCheck: mhc,
263267
}
264268
}
@@ -293,15 +297,15 @@ func alignRefAPIVersion(templateFromClusterClass *unstructured.Unstructured, cur
293297
}
294298

295299
// getMDClassName retrieves the MDClass name by looking up the MDTopology in the Cluster.
296-
func getMDClassName(cluster *clusterv1.Cluster, mdTopologyName string) string {
300+
func getMDClassName(cluster *clusterv1.Cluster, mdTopologyName string) (bool, string) {
297301
if cluster.Spec.Topology.Workers == nil {
298-
return ""
302+
return false, ""
299303
}
300304

301305
for _, mdTopology := range cluster.Spec.Topology.Workers.MachineDeployments {
302306
if mdTopology.Name == mdTopologyName {
303-
return mdTopology.Class
307+
return true, mdTopology.Class
304308
}
305309
}
306-
return ""
310+
return false, ""
307311
}

‎internal/controllers/topology/cluster/current_state_test.go

Lines changed: 73 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -112,6 +112,15 @@ func TestGetCurrentState(t *testing.T) {
112112
WithBootstrapTemplate(machineDeploymentBootstrap).
113113
WithInfrastructureTemplate(machineDeploymentInfrastructure).
114114
Build()
115+
machineDeployment2 := builder.MachineDeployment(metav1.NamespaceDefault, "md2").
116+
WithLabels(map[string]string{
117+
clusterv1.ClusterLabelName: "cluster1",
118+
clusterv1.ClusterTopologyOwnedLabel: "",
119+
clusterv1.ClusterTopologyMachineDeploymentLabelName: "md2",
120+
}).
121+
WithBootstrapTemplate(machineDeploymentBootstrap).
122+
WithInfrastructureTemplate(machineDeploymentInfrastructure).
123+
Build()
115124

116125
// MachineHealthChecks for the MachineDeployment and the ControlPlane.
117126
machineHealthCheckForMachineDeployment := builder.MachineHealthCheck(machineDeployment.Namespace, machineDeployment.Name).
@@ -576,6 +585,70 @@ func TestGetCurrentState(t *testing.T) {
576585
},
577586
},
578587
},
588+
{
589+
name: "Should read a full Cluster, even if a MachineDeployment topology has been deleted and the MachineDeployment still exists",
590+
cluster: builder.Cluster(metav1.NamespaceDefault, "cluster1").
591+
WithInfrastructureCluster(infraCluster).
592+
WithControlPlane(controlPlaneWithInfra).
593+
WithTopology(builder.ClusterTopology().
594+
WithMachineDeployment(clusterv1.MachineDeploymentTopology{
595+
Class: "mdClass",
596+
Name: "md1",
597+
}).
598+
Build()).
599+
Build(),
600+
blueprint: &scope.ClusterBlueprint{
601+
ClusterClass: clusterClassWithControlPlaneInfra,
602+
InfrastructureClusterTemplate: infraClusterTemplate,
603+
ControlPlane: &scope.ControlPlaneBlueprint{
604+
Template: controlPlaneTemplateWithInfrastructureMachine,
605+
InfrastructureMachineTemplate: controlPlaneInfrastructureMachineTemplate,
606+
},
607+
MachineDeployments: map[string]*scope.MachineDeploymentBlueprint{
608+
"mdClass": {
609+
BootstrapTemplate: machineDeploymentBootstrap,
610+
InfrastructureMachineTemplate: machineDeploymentInfrastructure,
611+
},
612+
},
613+
},
614+
objects: []client.Object{
615+
infraCluster,
616+
clusterClassWithControlPlaneInfra,
617+
controlPlaneInfrastructureMachineTemplate,
618+
controlPlaneWithInfra,
619+
machineDeploymentInfrastructure,
620+
machineDeploymentBootstrap,
621+
machineDeployment,
622+
machineDeployment2,
623+
},
624+
// Expect valid return of full ClusterState with MachineDeployments properly filtered by label.
625+
want: &scope.ClusterState{
626+
Cluster: builder.Cluster(metav1.NamespaceDefault, "cluster1").
627+
WithInfrastructureCluster(infraCluster).
628+
WithControlPlane(controlPlaneWithInfra).
629+
WithTopology(builder.ClusterTopology().
630+
WithMachineDeployment(clusterv1.MachineDeploymentTopology{
631+
Class: "mdClass",
632+
Name: "md1",
633+
}).
634+
Build()).
635+
Build(),
636+
ControlPlane: &scope.ControlPlaneState{Object: controlPlaneWithInfra, InfrastructureMachineTemplate: controlPlaneInfrastructureMachineTemplate},
637+
InfrastructureCluster: infraCluster,
638+
MachineDeployments: map[string]*scope.MachineDeploymentState{
639+
"md1": {
640+
Object: machineDeployment,
641+
BootstrapTemplate: machineDeploymentBootstrap,
642+
InfrastructureMachineTemplate: machineDeploymentInfrastructure,
643+
},
644+
"md2": {
645+
Object: machineDeployment2,
646+
BootstrapTemplate: machineDeploymentBootstrap,
647+
InfrastructureMachineTemplate: machineDeploymentInfrastructure,
648+
},
649+
},
650+
},
651+
},
579652
{
580653
name: "Fails if a Cluster has a MachineDeployment without the Bootstrap Template ref",
581654
cluster: builder.Cluster(metav1.NamespaceDefault, "cluster1").

0 commit comments

Comments
 (0)