Skip to content

Commit

Permalink
Address comments
Browse files Browse the repository at this point in the history
  • Loading branch information
@fabriziopandini
fabriziopandini committed Mar 25, 2024
1 parent d21f083 commit 7dc850c
Show file tree
Hide file tree
Showing 2 changed files with 54 additions and 26 deletions.
38 changes: 33 additions & 5 deletions internal/controllers/topology/cluster/reconcile_state.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -363,7 +363,12 @@ func (r *Reconciler) reconcileControlPlane(ctx context.Context, s *scope.Scope)
infrastructureMachineCleanupFunc = func() {
// Best effort cleanup of the InfrastructureMachineTemplate;
// If this fails, the object will be garbage collected when the cluster is deleted.
_ = r.Client.Delete(ctx, s.Desired.ControlPlane.InfrastructureMachineTemplate)
if err = r.Client.Delete(ctx, s.Desired.ControlPlane.InfrastructureMachineTemplate); err != nil {
log := tlog.LoggerFrom(ctx).
WithValues(s.Desired.ControlPlane.InfrastructureMachineTemplate.GetObjectKind().GroupVersionKind().Kind, s.Desired.ControlPlane.InfrastructureMachineTemplate.GetName()).
WithValues("err", err.Error())
log.Infof("WARNING! Failed to cleanup InfrastructureMachineTemplate for control plane while handling creation error. The object will be garbage collected when the cluster is deleted.")
}
}
}

Expand Down Expand Up @@ -611,7 +616,12 @@ func (r *Reconciler) createMachineDeployment(ctx context.Context, s *scope.Scope
infrastructureMachineCleanupFunc = func() {
// Best effort cleanup of the InfrastructureMachineTemplate;
// If this fails, the object will be garbage collected when the cluster is deleted.
_ = r.Client.Delete(ctx, md.InfrastructureMachineTemplate)
if err = r.Client.Delete(ctx, md.InfrastructureMachineTemplate); err != nil {
log := tlog.LoggerFrom(ctx).
WithValues(md.InfrastructureMachineTemplate.GetObjectKind().GroupVersionKind().Kind, md.InfrastructureMachineTemplate.GetName()).
WithValues("err", err.Error())
log.Infof("WARNING! Failed to cleanup InfrastructureMachineTemplate for MachineDeployment while handling creation error. The object will be garbage collected when the cluster is deleted.")
}
}
}

Expand All @@ -631,7 +641,12 @@ func (r *Reconciler) createMachineDeployment(ctx context.Context, s *scope.Scope
bootstrapCleanupFunc = func() {
// Best effort cleanup of the BootstrapTemplate;
// If this fails, the object will be garbage collected when the cluster is deleted.
_ = r.Client.Delete(ctx, md.BootstrapTemplate)
if err = r.Client.Delete(ctx, md.BootstrapTemplate); err != nil {
log := tlog.LoggerFrom(ctx).
WithValues(md.BootstrapTemplate.GetObjectKind().GroupVersionKind().Kind, md.BootstrapTemplate.GetName()).
WithValues("err", err.Error())
log.Infof("WARNING! Failed to cleanup BootstrapTemplate for MachineDeployment while handling creation error. The object will be garbage collected when the cluster is deleted.")
}
}
}

Expand Down Expand Up @@ -897,7 +912,13 @@ func (r *Reconciler) createMachinePool(ctx context.Context, s *scope.Scope, mp *
infrastructureMachineMachinePool = func() {
// Best effort cleanup of the InfrastructureMachinePool;
// If this fails, the object will be garbage collected when the cluster is deleted.
_ = r.Client.Delete(ctx, mp.InfrastructureMachinePoolObject)
if err = r.Client.Delete(ctx, mp.InfrastructureMachinePoolObject); err != nil {
log := tlog.LoggerFrom(ctx).
WithValues(mp.InfrastructureMachinePoolObject.GetObjectKind().GroupVersionKind().Kind, mp.InfrastructureMachinePoolObject.GetName()).
WithValues("err", err.Error())
log.Infof("WARNING! Failed to cleanup InfrastructureMachinePoolObject for MachinePool while handling creation error. The object will be garbage collected when the cluster is deleted.")
}

}

Check failure on line 922 in internal/controllers/topology/cluster/reconcile_state.go

View workflow job for this annotation

GitHub Actions / lint 

unnecessary trailing newline (whitespace)

Check failure on line 922 in internal/controllers/topology/cluster/reconcile_state.go

View workflow job for this annotation

GitHub Actions / lint 

unnecessary trailing newline (whitespace)
}

Expand All @@ -917,7 +938,12 @@ func (r *Reconciler) createMachinePool(ctx context.Context, s *scope.Scope, mp *
bootstrapCleanupFunc = func() {
// Best effort cleanup of the BootstrapConfig;
// If this fails, the object will be garbage collected when the cluster is deleted.
_ = r.Client.Delete(ctx, mp.BootstrapObject)
if err = r.Client.Delete(ctx, mp.BootstrapObject); err != nil {
log := tlog.LoggerFrom(ctx).
WithValues(mp.BootstrapObject.GetObjectKind().GroupVersionKind().Kind, mp.BootstrapObject.GetName()).
WithValues("err", err.Error())
log.Infof("WARNING! Failed to cleanup BootstrapObject for MachinePool while handling creation error. The object will be garbage collected when the cluster is deleted.")
}
}
}

Expand Down Expand Up @@ -1108,6 +1134,7 @@ type reconcileReferencedObjectInput struct {
}

// reconcileReferencedObject reconciles the desired state of the referenced object.
// Returns true if the referencedObject is created.
// NOTE: After a referenced object is created it is assumed that the reference should
// never change (only the content of the object can eventually change). Thus, we are checking for strict compatibility.
func (r *Reconciler) reconcileReferencedObject(ctx context.Context, in reconcileReferencedObjectInput) (bool, error) {
Expand Down Expand Up @@ -1180,6 +1207,7 @@ type reconcileReferencedTemplateInput struct {
}

// reconcileReferencedTemplate reconciles the desired state of a referenced Template.
// Returns true if the referencedTemplate is created.
// NOTE: According to Cluster API operational practices, when a referenced Template changes a template rotation is required:
// 1. create a new Template
// 2. update the reference
Expand Down
42 changes: 21 additions & 21 deletions internal/controllers/topology/cluster/reconcile_state_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -3416,7 +3416,7 @@ func TestReconcileState(t *testing.T) {
t.Run("Cluster get reconciled with infrastructure Ref only when reconcileInfrastructureCluster pass and reconcileControlPlane fails ", func(t *testing.T) {
g := NewWithT(t)

currentdCluster := builder.Cluster(metav1.NamespaceDefault, "cluster1").Build()
currentCluster := builder.Cluster(metav1.NamespaceDefault, "cluster1").Build()

infrastructureCluster := builder.TestInfrastructureCluster(metav1.NamespaceDefault, "infrastructure-cluster1").Build()
controlPlane := builder.TestControlPlane(metav1.NamespaceDefault, "controlplane-cluster1").Build()
Expand All @@ -3428,12 +3428,12 @@ func TestReconcileState(t *testing.T) {
// cluster requires a UID because reconcileClusterShim will create a cluster shim
// which has the cluster set as Owner in an OwnerReference.
// A valid OwnerReferences requires a uid.
currentdCluster.SetUID("foo")
currentCluster.SetUID("foo")

// NOTE: it is ok to use create given that the Cluster are created by user.
g.Expect(env.CreateAndWait(ctx, currentdCluster)).To(Succeed())
g.Expect(env.CreateAndWait(ctx, currentCluster)).To(Succeed())

s := scope.New(currentdCluster)
s := scope.New(currentCluster)
s.Blueprint = &scope.ClusterBlueprint{ClusterClass: &clusterv1.ClusterClass{}}
s.Current.ControlPlane = &scope.ControlPlaneState{}
s.Desired = &scope.ClusterState{Cluster: desiredCluster, InfrastructureCluster: infrastructureCluster, ControlPlane: &scope.ControlPlaneState{Object: controlPlane}}
Expand All @@ -3454,19 +3454,19 @@ func TestReconcileState(t *testing.T) {
err = r.reconcileState(ctx, s)
g.Expect(err).To(HaveOccurred())

got := currentdCluster.DeepCopy()
err = env.GetAPIReader().Get(ctx, client.ObjectKeyFromObject(currentdCluster), got)
got := currentCluster.DeepCopy()
err = env.GetAPIReader().Get(ctx, client.ObjectKeyFromObject(currentCluster), got)
g.Expect(err).ToNot(HaveOccurred())

g.Expect(got.Spec.InfrastructureRef).ToNot(BeNil())
g.Expect(got.Spec.ControlPlaneRef).To(BeNil())

g.Expect(env.CleanupAndWait(ctx, infrastructureCluster, currentdCluster)).To(Succeed())
g.Expect(env.CleanupAndWait(ctx, infrastructureCluster, currentCluster)).To(Succeed())
})
t.Run("Cluster get reconciled with both infrastructure Ref and control plane ref when both reconcileInfrastructureCluster and reconcileControlPlane pass", func(t *testing.T) {
g := NewWithT(t)

currentdCluster := builder.Cluster(metav1.NamespaceDefault, "cluster1").Build()
currentCluster := builder.Cluster(metav1.NamespaceDefault, "cluster1").Build()

infrastructureCluster := builder.TestInfrastructureCluster(metav1.NamespaceDefault, "infrastructure-cluster1").Build()
controlPlane := builder.TestControlPlane(metav1.NamespaceDefault, "controlplane-cluster1").Build()
Expand All @@ -3478,12 +3478,12 @@ func TestReconcileState(t *testing.T) {
// cluster requires a UID because reconcileClusterShim will create a cluster shim
// which has the cluster set as Owner in an OwnerReference.
// A valid OwnerReferences requires a uid.
currentdCluster.SetUID("foo")
currentCluster.SetUID("foo")

// NOTE: it is ok to use create given that the Cluster are created by user.
g.Expect(env.CreateAndWait(ctx, currentdCluster)).To(Succeed())
g.Expect(env.CreateAndWait(ctx, currentCluster)).To(Succeed())

s := scope.New(currentdCluster)
s := scope.New(currentCluster)
s.Blueprint = &scope.ClusterBlueprint{ClusterClass: &clusterv1.ClusterClass{}}
s.Current.ControlPlane = &scope.ControlPlaneState{}
s.Desired = &scope.ClusterState{Cluster: desiredCluster, InfrastructureCluster: infrastructureCluster, ControlPlane: &scope.ControlPlaneState{Object: controlPlane}}
Expand All @@ -3501,22 +3501,22 @@ func TestReconcileState(t *testing.T) {
err = r.reconcileState(ctx, s)
g.Expect(err).ToNot(HaveOccurred())

got := currentdCluster.DeepCopy()
err = env.GetAPIReader().Get(ctx, client.ObjectKeyFromObject(currentdCluster), got)
got := currentCluster.DeepCopy()
err = env.GetAPIReader().Get(ctx, client.ObjectKeyFromObject(currentCluster), got)
g.Expect(err).ToNot(HaveOccurred())

g.Expect(got.Spec.InfrastructureRef).ToNot(BeNil())
g.Expect(got.Spec.ControlPlaneRef).ToNot(BeNil())

g.Expect(env.CleanupAndWait(ctx, infrastructureCluster, controlPlane, currentdCluster)).To(Succeed())
g.Expect(env.CleanupAndWait(ctx, infrastructureCluster, controlPlane, currentCluster)).To(Succeed())
})
t.Run("Cluster does not get reconciled when reconcileControlPlane fails and infrastructure Ref is set", func(t *testing.T) {
g := NewWithT(t)

infrastructureCluster := builder.TestInfrastructureCluster(metav1.NamespaceDefault, "infrastructure-cluster1").Build()
controlPlane := builder.TestControlPlane(metav1.NamespaceDefault, "controlplane-cluster1").Build()

currentdCluster := builder.Cluster(metav1.NamespaceDefault, "cluster1").
currentCluster := builder.Cluster(metav1.NamespaceDefault, "cluster1").
WithInfrastructureCluster(infrastructureCluster).
Build()

Expand All @@ -3528,12 +3528,12 @@ func TestReconcileState(t *testing.T) {
// cluster requires a UID because reconcileClusterShim will create a cluster shim
// which has the cluster set as Owner in an OwnerReference.
// A valid OwnerReferences requires a uid.
currentdCluster.SetUID("foo")
currentCluster.SetUID("foo")

// NOTE: it is ok to use create given that the Cluster are created by user.
g.Expect(env.CreateAndWait(ctx, currentdCluster)).To(Succeed())
g.Expect(env.CreateAndWait(ctx, currentCluster)).To(Succeed())

s := scope.New(currentdCluster)
s := scope.New(currentCluster)
s.Blueprint = &scope.ClusterBlueprint{ClusterClass: &clusterv1.ClusterClass{}}
s.Current.ControlPlane = &scope.ControlPlaneState{}
s.Desired = &scope.ClusterState{Cluster: desiredCluster, InfrastructureCluster: infrastructureCluster, ControlPlane: &scope.ControlPlaneState{Object: controlPlane}}
Expand All @@ -3554,14 +3554,14 @@ func TestReconcileState(t *testing.T) {
err = r.reconcileState(ctx, s)
g.Expect(err).To(HaveOccurred())

got := currentdCluster.DeepCopy()
err = env.GetAPIReader().Get(ctx, client.ObjectKeyFromObject(currentdCluster), got)
got := currentCluster.DeepCopy()
err = env.GetAPIReader().Get(ctx, client.ObjectKeyFromObject(currentCluster), got)
g.Expect(err).ToNot(HaveOccurred())

g.Expect(got.Spec.InfrastructureRef).ToNot(BeNil())
g.Expect(got.Spec.ControlPlaneRef).To(BeNil())

g.Expect(env.CleanupAndWait(ctx, infrastructureCluster, controlPlane, currentdCluster)).To(Succeed())
g.Expect(env.CleanupAndWait(ctx, infrastructureCluster, controlPlane, currentCluster)).To(Succeed())
})
}

Expand Down

0 comments on commit 7dc850c

Please sign in to comment.