Skip to content

Commit

Permalink
Bug 2112817: Stop reconcile loop after changing CR inside BMAC (opens…
Browse files Browse the repository at this point in the history 
…hift#4227)

This PR fixes the issue where returning `reconcileComplete{dirty: true}`
does not stop the reconciliation loop what leads to unexpected races and
objects being modified multiple times.

Cherry-picks: openshift#4201
Closes: Bug-2112817

Co-authored-by: Mat Kowalski <[email protected]>
  • Loading branch information
@openshift-cherrypick-robot @mkowalski
openshift-cherrypick-robot and mkowalski authored Aug 4, 2022
1 parent f0ff488 commit 0808954
Show file tree
Hide file tree
Showing 2 changed files with 40 additions and 28 deletions.
8 changes: 4 additions & 4 deletions internal/controller/controllers/bmh_agent_controller.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -387,7 +387,7 @@ func (r *BMACReconciler) addBMHDetachedAnnotationIfAgentHasStartedInstallation(c

bmh.ObjectMeta.Annotations[BMH_DETACHED_ANNOTATION] = "assisted-service-controller"

return reconcileComplete{dirty: true}
return reconcileComplete{dirty: true, stop: true}
}

// Reconcile BMH's HardwareDetails using the agent's inventory
Expand Down Expand Up @@ -500,7 +500,7 @@ func (r *BMACReconciler) reconcileAgentInventory(log logrus.FieldLogger, bmh *bm

bmh.ObjectMeta.Annotations[BMH_HARDWARE_DETAILS_ANNOTATION] = string(bytes)
log.Debugf("Agent Inventory reconciled to BMH \n %v \n %v", agent, bmh)
return reconcileComplete{dirty: true}
return reconcileComplete{dirty: true, stop: true}

}

Expand Down Expand Up @@ -752,7 +752,7 @@ func (r *BMACReconciler) reconcileSpokeBMH(ctx context.Context, log logrus.Field
bmh.ObjectMeta.Annotations = make(map[string]string)
}
bmh.ObjectMeta.Annotations[BMH_DETACHED_ANNOTATION] = "assisted-service-controller"
return reconcileComplete{dirty: true}
return reconcileComplete{dirty: true, stop: true}
}
return reconcileComplete{}
}
Expand Down Expand Up @@ -1085,7 +1085,7 @@ func (r *BMACReconciler) ensureMCSCert(ctx context.Context, log logrus.FieldLogg
bmh.ObjectMeta.Annotations[BMH_AGENT_IGNITION_CONFIG_OVERRIDES] = ignitionWithMCSCert
}
log.Info("MCS certificate injected")
return reconcileComplete{dirty: true}
return reconcileComplete{dirty: true, stop: true}
}

func (r *BMACReconciler) createIgnitionWithMCSCert(ctx context.Context, log logrus.FieldLogger, spokeClient client.Client) (string, string, error) {
Expand Down
60 changes: 36 additions & 24 deletions internal/controller/controllers/bmh_agent_controller_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -797,12 +797,14 @@ var _ = Describe("bmac reconcile", func() {

Context("when agent role worker and cluster deployment is set", func() {
It("should set spoke BMH", func() {
result, err := bmhr.Reconcile(ctx, newBMHRequest(host))
Expect(err).To(BeNil())
Expect(result).To(Equal(ctrl.Result{}))
for range [3]int{} {
result, err := bmhr.Reconcile(ctx, newBMHRequest(host))
Expect(err).To(BeNil())
Expect(result).To(Equal(ctrl.Result{}))
}

updatedHost := &bmh_v1alpha1.BareMetalHost{}
err = c.Get(ctx, types.NamespacedName{Name: host.Name, Namespace: testNamespace}, updatedHost)
err := c.Get(ctx, types.NamespacedName{Name: host.Name, Namespace: testNamespace}, updatedHost)
Expect(err).To(BeNil())
Expect(updatedHost.ObjectMeta.Annotations).To(HaveKey(BMH_HARDWARE_DETAILS_ANNOTATION))
Expect(updatedHost.ObjectMeta.Annotations).To(HaveKey(BMH_DETACHED_ANNOTATION))
Expand Down Expand Up @@ -842,9 +844,11 @@ var _ = Describe("bmac reconcile", func() {
})

It("validate label on Secrets", func() {
result, err := bmhr.Reconcile(ctx, newBMHRequest(host))
Expect(err).To(BeNil())
Expect(result).To(Equal(ctrl.Result{}))
for range [3]int{} {
result, err := bmhr.Reconcile(ctx, newBMHRequest(host))
Expect(err).To(BeNil())
Expect(result).To(Equal(ctrl.Result{}))
}

secret := &corev1.Secret{}
By("Checking if the secret has the custom label")
Expand Down Expand Up @@ -933,12 +937,14 @@ var _ = Describe("bmac reconcile", func() {
}
Expect(c.Update(ctx, agent)).To(BeNil())

result, err := bmhr.Reconcile(ctx, newBMHRequest(host))
Expect(err).To(BeNil())
Expect(result).To(Equal(ctrl.Result{}))
for range [3]int{} {
result, err := bmhr.Reconcile(ctx, newBMHRequest(host))
Expect(err).To(BeNil())
Expect(result).To(Equal(ctrl.Result{}))
}

updatedHost := &bmh_v1alpha1.BareMetalHost{}
err = c.Get(ctx, types.NamespacedName{Name: host.Name, Namespace: testNamespace}, updatedHost)
err := c.Get(ctx, types.NamespacedName{Name: host.Name, Namespace: testNamespace}, updatedHost)
Expect(err).To(BeNil())
Expect(updatedHost.ObjectMeta.Annotations).To(HaveKey(BMH_DETACHED_ANNOTATION))
Expect(updatedHost.ObjectMeta.Annotations[BMH_DETACHED_ANNOTATION]).To(Equal("assisted-service-controller"))
Expand All @@ -954,12 +960,14 @@ var _ = Describe("bmac reconcile", func() {
}
Expect(c.Update(ctx, agent)).To(BeNil())

result, err := bmhr.Reconcile(ctx, newBMHRequest(host))
Expect(err).To(BeNil())
Expect(result).To(Equal(ctrl.Result{}))
for range [3]int{} {
result, err := bmhr.Reconcile(ctx, newBMHRequest(host))
Expect(err).To(BeNil())
Expect(result).To(Equal(ctrl.Result{}))
}

updatedHost := &bmh_v1alpha1.BareMetalHost{}
err = c.Get(ctx, types.NamespacedName{Name: host.Name, Namespace: testNamespace}, updatedHost)
err := c.Get(ctx, types.NamespacedName{Name: host.Name, Namespace: testNamespace}, updatedHost)
Expect(err).To(BeNil())
Expect(updatedHost.ObjectMeta.Annotations).To(HaveKey(BMH_DETACHED_ANNOTATION))
Expect(updatedHost.ObjectMeta.Annotations[BMH_DETACHED_ANNOTATION]).To(Equal("assisted-service-controller"))
Expand All @@ -975,12 +983,14 @@ var _ = Describe("bmac reconcile", func() {
}
Expect(c.Update(ctx, agent)).To(BeNil())

result, err := bmhr.Reconcile(ctx, newBMHRequest(host))
Expect(err).To(BeNil())
Expect(result).To(Equal(ctrl.Result{}))
for range [3]int{} {
result, err := bmhr.Reconcile(ctx, newBMHRequest(host))
Expect(err).To(BeNil())
Expect(result).To(Equal(ctrl.Result{}))
}

updatedHost := &bmh_v1alpha1.BareMetalHost{}
err = c.Get(ctx, types.NamespacedName{Name: host.Name, Namespace: testNamespace}, updatedHost)
err := c.Get(ctx, types.NamespacedName{Name: host.Name, Namespace: testNamespace}, updatedHost)
Expect(err).To(BeNil())
Expect(updatedHost.ObjectMeta.Annotations).To(HaveKey(BMH_DETACHED_ANNOTATION))
Expect(updatedHost.ObjectMeta.Annotations[BMH_DETACHED_ANNOTATION]).To(Equal("assisted-service-controller"))
Expand Down Expand Up @@ -1031,20 +1041,22 @@ var _ = Describe("bmac reconcile", func() {
}
Expect(c.Update(ctx, agent)).To(BeNil())

result, err := bmhr.Reconcile(ctx, newBMHRequest(host))
Expect(err).To(BeNil())
Expect(result).To(Equal(ctrl.Result{}))
for range [3]int{} {
result, err := bmhr.Reconcile(ctx, newBMHRequest(host))
Expect(err).To(BeNil())
Expect(result).To(Equal(ctrl.Result{}))
}

updatedHost := &bmh_v1alpha1.BareMetalHost{}
err = c.Get(ctx, types.NamespacedName{Name: host.Name, Namespace: testNamespace}, updatedHost)
err := c.Get(ctx, types.NamespacedName{Name: host.Name, Namespace: testNamespace}, updatedHost)
Expect(err).To(BeNil())
Expect(updatedHost.ObjectMeta.Annotations).To(HaveKey(BMH_DETACHED_ANNOTATION))
Expect(updatedHost.ObjectMeta.Annotations[BMH_DETACHED_ANNOTATION]).To(Equal("assisted-service-controller"))

infraEnv.Status = v1beta1.InfraEnvStatus{ISODownloadURL: "http://go.find.it"}
Expect(c.Update(ctx, infraEnv)).To(BeNil())

result, err = bmhr.Reconcile(ctx, newBMHRequest(host))
result, err := bmhr.Reconcile(ctx, newBMHRequest(host))
Expect(err).To(BeNil())
Expect(result).To(Equal(ctrl.Result{}))

Expand Down

0 comments on commit 0808954

Please sign in to comment.