Skip to content

Commit fa1a300

Browse files
asergeant01asergeant01
committed
Updating webhooks for maintenance plan
Signed-off-by: Alan Sergeant <alan.sergeant@sap.com>
1 parent fc12ccd commit fa1a300

13 files changed

Lines changed: 396 additions & 58 deletions

‎internal/controller/biossettings_controller.go‎

Lines changed: 4 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -1023,6 +1023,9 @@ func (r *BIOSSettingsReconciler) handleAppliedState(ctx context.Context, bmcClie
10231023
if err != nil {
10241024
return ctrl.Result{}, fmt.Errorf("failed to get BIOS version and settings diff: %w", err)
10251025
}
1026+
if settings.Status.State == metalv1alpha1.BIOSSettingsStateApplied && len(settingsDiff) == 0 {
1027+
return ctrl.Result{}, nil
1028+
}
10261029
if len(settingsDiff) > 0 {
10271030
log.V(1).Info("Found BIOS setting difference after applied state", "SettingsDiff", settingsDiff)
10281031
return ctrl.Result{}, r.updateStatus(ctx, settings, metalv1alpha1.BIOSSettingsStatePending, nil)
@@ -1490,9 +1493,7 @@ func (r *BIOSSettingsReconciler) enqueueBiosSettingsByServerRefs(ctx context.Con
14901493
if settings.Spec.ServerMaintenanceRef == nil ||
14911494
(server.Spec.ServerMaintenanceRef != nil &&
14921495
(server.Spec.ServerMaintenanceRef.Name != settings.Spec.ServerMaintenanceRef.Name ||
1493-
server.Spec.ServerMaintenanceRef.Namespace != settings.Spec.ServerMaintenanceRef.Namespace)) ||
1494-
settings.Status.State == metalv1alpha1.BIOSSettingsStateApplied ||
1495-
settings.Status.State == metalv1alpha1.BIOSSettingsStateFailed {
1496+
server.Spec.ServerMaintenanceRef.Namespace != settings.Spec.ServerMaintenanceRef.Namespace)) {
14961497
continue
14971498
}
14981499
reqs = append(reqs, ctrl.Request{NamespacedName: types.NamespacedName{Name: settings.Name}})

‎internal/controller/biosversion_controller.go‎

Lines changed: 6 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -614,7 +614,11 @@ func (r *BIOSVersionReconciler) cleanup(ctx context.Context, bmcClient bmc.BMC,
614614
if err != nil {
615615
return err
616616
}
617-
617+
if biosVersion.Status.State == metalv1alpha1.BIOSVersionStateCompleted &&
618+
currentBiosVersion == biosVersion.Spec.Version &&
619+
biosVersion.Spec.ServerMaintenanceRef == nil {
620+
return nil
621+
}
618622
if currentBiosVersion == biosVersion.Spec.Version {
619623
if err := r.cleanupServerMaintenanceReferences(ctx, biosVersion); err != nil {
620624
return err
@@ -988,9 +992,7 @@ func (r *BIOSVersionReconciler) enqueueBiosVersionByServerRefs(ctx context.Conte
988992
continue
989993
}
990994
// states where we do not need to requeue for host changes
991-
if biosVersion.Spec.ServerMaintenanceRef == nil ||
992-
biosVersion.Status.State == metalv1alpha1.BIOSVersionStateCompleted ||
993-
biosVersion.Status.State == metalv1alpha1.BIOSVersionStateFailed {
995+
if biosVersion.Spec.ServerMaintenanceRef == nil {
994996
return nil
995997
}
996998
if biosVersion.Spec.ServerMaintenanceRef.Name != host.Spec.ServerMaintenanceRef.Name {

‎internal/controller/bmcsettings_controller.go‎

Lines changed: 7 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -221,6 +221,10 @@ func (r *BMCSettingsReconciler) reconcile(ctx context.Context, settings *metalv1
221221

222222
bmcObj, err := r.getBMC(ctx, settings)
223223
if err != nil {
224+
if apierrors.IsNotFound(err) {
225+
log.V(1).Info("Referred BMC object not found, skipping")
226+
return ctrl.Result{}, nil
227+
}
224228
log.V(1).Info("Failed to fetch referred BMC object")
225229
return ctrl.Result{}, err
226230
}
@@ -581,6 +585,9 @@ func (r *BMCSettingsReconciler) handleSettingAppliedState(ctx context.Context, s
581585
if err != nil {
582586
return fmt.Errorf("failed to fetch and check BMCSettings: %w", err)
583587
}
588+
if settings.Status.State == metalv1alpha1.BMCSettingsStateApplied && len(settingsDiff) == 0 {
589+
return nil
590+
}
584591
if len(settingsDiff) > 0 {
585592
// Drift detected — reset all conditions and state so the next reconcile
586593
// re-enters the apply phase from scratch.
@@ -1268,9 +1275,6 @@ func (r *BMCSettingsReconciler) enqueueBMCSettingsByBMCRefs(ctx context.Context,
12681275
var requests []ctrl.Request
12691276
for _, settings := range settingsList.Items {
12701277
if settings.Spec.BMCRef != nil && settings.Spec.BMCRef.Name == bmcObj.Name {
1271-
if settings.Status.State == metalv1alpha1.BMCSettingsStateApplied || settings.Status.State == metalv1alpha1.BMCSettingsStateFailed {
1272-
continue
1273-
}
12741278
requests = append(requests, ctrl.Request{NamespacedName: types.NamespacedName{Namespace: settings.Namespace, Name: settings.Name}})
12751279
}
12761280
}

‎internal/controller/bmcversion_controller.go‎

Lines changed: 16 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -179,6 +179,10 @@ func (r *BMCVersionReconciler) ensureBMCVersionStateTransition(ctx context.Conte
179179
log := ctrl.LoggerFrom(ctx)
180180
bmcObj, err := r.getBMCFromBMCVersion(ctx, bmcVersion)
181181
if err != nil {
182+
if apierrors.IsNotFound(err) {
183+
log.V(1).Info("Referred BMC object not found, skipping", "BMCVersion", bmcVersion.Name)
184+
return ctrl.Result{}, nil
185+
}
182186
log.V(1).Info("Referred BMC object could not be fetched", "BMCVersion", bmcVersion.Name)
183187
return ctrl.Result{}, err
184188
}
@@ -263,7 +267,7 @@ func (r *BMCVersionReconciler) ensureBMCVersionStateTransition(ctx context.Conte
263267
}
264268

265269
if ok, err := r.resetBMC(ctx, bmcVersion, bmcObj, ConditionResetIssued); !ok || err != nil {
266-
return ctrl.Result{}, fmt.Errorf("failed to reset bmc %s: %w", client.ObjectKeyFromObject(bmcObj), err)
270+
return ctrl.Result{}, err
267271
}
268272

269273
return r.handleUpgradeInProgressState(ctx, bmcVersion, bmcClient, bmcObj)
@@ -561,6 +565,11 @@ func (r *BMCVersionReconciler) removeServerMaintenanceRefAndResetConditions(
561565
if err != nil {
562566
return err
563567
}
568+
if bmcVersion.Status.State == metalv1alpha1.BMCVersionStateCompleted &&
569+
currentBMCVersion == bmcVersion.Spec.Version &&
570+
len(bmcVersion.Spec.ServerMaintenanceRefs) == 0 {
571+
return nil
572+
}
564573
state := metalv1alpha1.BMCVersionStateInProgress
565574
if currentBMCVersion == bmcVersion.Spec.Version {
566575
if err := r.removeServerMaintenances(ctx, bmcVersion); err != nil {
@@ -1223,8 +1232,13 @@ func (r *BMCVersionReconciler) enqueueBMCVersionByBMCRefs(ctx context.Context, o
12231232

12241233
for _, bmcVersion := range bmcVersionList.Items {
12251234
if bmcVersion.Spec.BMCRef != nil && bmcVersion.Spec.BMCRef.Name == bmcObj.Name {
1235+
// For Completed/Failed BMCVersions, only re-enqueue if the BMC firmware version
1236+
// has actually changed away from spec.version — indicating genuine hardware drift.
1237+
// Routine BMC status updates (polling, maintenance activity) must not retrigger them.
12261238
if bmcVersion.Status.State == metalv1alpha1.BMCVersionStateCompleted || bmcVersion.Status.State == metalv1alpha1.BMCVersionStateFailed {
1227-
return nil
1239+
if bmcObj.Status.FirmwareVersion == bmcVersion.Spec.Version {
1240+
continue
1241+
}
12281242
}
12291243
return []ctrl.Request{{NamespacedName: types.NamespacedName{Namespace: bmcVersion.Namespace, Name: bmcVersion.Name}}}
12301244
}

‎internal/controller/bmcversion_controller_test.go‎

Lines changed: 49 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -636,6 +636,55 @@ var _ = Describe("BMCVersion Controller", func() {
636636
// cleanup
637637
Expect(k8sClient.Delete(ctx, bmcVersion)).To(Succeed())
638638
})
639+
It("should re-upgrade BMC when firmware is rolled back after completion", func(ctx SpecContext) {
640+
By("Updating the server state to Available with power off for maintenance")
641+
Eventually(UpdateStatus(server, func() {
642+
server.Status.State = metalv1alpha1.ServerStateAvailable
643+
server.Status.PowerState = metalv1alpha1.ServerOffPowerState
644+
})).Should(Succeed())
645+
646+
By("Creating a BMCVersion targeting the upgraded version")
647+
bmcVersion := &metalv1alpha1.BMCVersion{
648+
ObjectMeta: metav1.ObjectMeta{
649+
GenerateName: "test-",
650+
},
651+
Spec: metalv1alpha1.BMCVersionSpec{
652+
BMCRef: &v1.LocalObjectReference{Name: bmcObj.Name},
653+
BMCVersionTemplate: metalv1alpha1.BMCVersionTemplate{
654+
Version: upgradeServerBMCVersion,
655+
Image: metalv1alpha1.ImageSpec{URI: upgradeServerBMCVersion},
656+
ServerMaintenancePolicy: metalv1alpha1.ServerMaintenancePolicyEnforced,
657+
},
658+
},
659+
}
660+
Expect(k8sClient.Create(ctx, bmcVersion)).To(Succeed())
661+
662+
By("Waiting for BMCVersion to reach Completed state")
663+
Eventually(Object(bmcVersion), "60s").Should(
664+
HaveField("Status.State", metalv1alpha1.BMCVersionStateCompleted),
665+
)
666+
667+
By("Simulating a firmware rollback on the mock BMC")
668+
mockServers[0].ResetUpgradeTask("/redfish/v1/Managers/BMC")
669+
670+
By("Triggering a BMC watch event to prompt re-reconciliation")
671+
Eventually(Update(bmcObj, func() {
672+
metautils.SetAnnotation(bmcObj, "trigger-reconcile", "rollback-test")
673+
})).Should(Succeed())
674+
675+
By("Ensuring BMCVersion transitions back to InProgress after detecting the rollback")
676+
Eventually(Object(bmcVersion), "30s").Should(
677+
HaveField("Status.State", metalv1alpha1.BMCVersionStateInProgress),
678+
)
679+
680+
By("Ensuring BMCVersion reaches Completed again after re-upgrade")
681+
Eventually(Object(bmcVersion), "60s").Should(
682+
HaveField("Status.State", metalv1alpha1.BMCVersionStateCompleted),
683+
)
684+
685+
Expect(k8sClient.Delete(ctx, bmcVersion)).To(Succeed())
686+
})
687+
639688
})
640689

641690
func ensureBMCVersionConditionTransition(acc *conditionutils.Accessor, bmcVersion *metalv1alpha1.BMCVersion) {

‎internal/webhook/v1alpha1/biossettings_webhook.go‎

Lines changed: 23 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -69,6 +69,13 @@ func (v *BIOSSettingsCustomValidator) ValidateUpdate(ctx context.Context, oldObj
6969
}
7070
}
7171

72+
// If neither the serverRef nor the version changed, no new duplicate can be introduced.
73+
if oldObj.Spec.ServerRef != nil && newObj.Spec.ServerRef != nil &&
74+
oldObj.Spec.ServerRef.Name == newObj.Spec.ServerRef.Name &&
75+
oldObj.Spec.Version == newObj.Spec.Version {
76+
return nil, nil
77+
}
78+
7279
settingsList := &metalv1alpha1.BIOSSettingsList{}
7380
if err := v.List(ctx, settingsList); err != nil {
7481
return nil, fmt.Errorf("failed to list BIOSSettings: %w", err)
@@ -100,16 +107,23 @@ func checkForDuplicateBIOSSettingsRefToServer(settingsList *metalv1alpha1.BIOSSe
100107
if settings.Name == bs.Name {
101108
continue
102109
}
103-
if settings.Spec.ServerRef.Name == bs.Spec.ServerRef.Name {
104-
err := fmt.Errorf("server (%s) referred in %s is duplicate of server (%s) referred in %s",
105-
settings.Spec.ServerRef.Name,
106-
settings.Name,
107-
bs.Spec.ServerRef.Name,
108-
bs.Name)
109-
return nil, apierrors.NewInvalid(
110-
schema.GroupKind{Group: settings.GroupVersionKind().Group, Kind: settings.Kind},
111-
settings.GetName(), field.ErrorList{field.Duplicate(field.NewPath("spec").Child("serverRef"), err)})
110+
if settings.Spec.ServerRef == nil || bs.Spec.ServerRef == nil {
111+
continue
112+
}
113+
if settings.Spec.ServerRef.Name != bs.Spec.ServerRef.Name {
114+
continue
115+
}
116+
if bs.Spec.Version != settings.Spec.Version {
117+
continue
112118
}
119+
err := fmt.Errorf("server (%s) referred in %s is duplicate of server (%s) referred in %s",
120+
settings.Spec.ServerRef.Name,
121+
settings.Name,
122+
bs.Spec.ServerRef.Name,
123+
bs.Name)
124+
return nil, apierrors.NewInvalid(
125+
schema.GroupKind{Group: settings.GroupVersionKind().Group, Kind: settings.Kind},
126+
settings.GetName(), field.ErrorList{field.Duplicate(field.NewPath("spec").Child("serverRef"), err)})
113127
}
114128
return nil, nil
115129
}

‎internal/webhook/v1alpha1/biossettings_webhook_test.go‎

Lines changed: 57 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -98,7 +98,7 @@ var _ = Describe("BIOSSettings Webhook", func() {
9898
Expect(k8sClient.Create(ctx, biosSettingsV2)).To(Succeed())
9999
})
100100

101-
It("should deny update if spec.serverRef is duplicate", func() {
101+
It("should deny update if spec.serverRef and version are both duplicate", func() {
102102
By("Creating a BIOSSettings with different ServerRef")
103103
biosSettingsV2 := &metalv1alpha1.BIOSSettings{
104104
ObjectMeta: metav1.ObjectMeta{
@@ -119,12 +119,67 @@ var _ = Describe("BIOSSettings Webhook", func() {
119119
}
120120
Expect(k8sClient.Create(ctx, biosSettingsV2)).To(Succeed())
121121

122-
By("Updating an biosSettingsV2 with a conflicting ServerRef")
122+
By("Updating biosSettingsV2 with both a conflicting ServerRef and matching version")
123123
biosSettingsV2Updated := biosSettingsV2.DeepCopy()
124124
biosSettingsV2Updated.Spec.ServerRef = &v1.LocalObjectReference{Name: "foo"}
125+
biosSettingsV2Updated.Spec.Version = defaultMockUpServerBiosVersion
125126
Expect(validator.ValidateUpdate(ctx, biosSettingsV2, biosSettingsV2Updated)).Error().To(HaveOccurred())
126127
})
127128

129+
It("should allow update if spec.serverRef changes to an existing one but version differs", func() {
130+
By("Creating a BIOSSettings with different ServerRef")
131+
biosSettingsV2 := &metalv1alpha1.BIOSSettings{
132+
ObjectMeta: metav1.ObjectMeta{
133+
GenerateName: "test-",
134+
},
135+
Spec: metalv1alpha1.BIOSSettingsSpec{
136+
ServerRef: &v1.LocalObjectReference{Name: "bar"},
137+
BIOSSettingsTemplate: metalv1alpha1.BIOSSettingsTemplate{
138+
Version: anotherMockUpServerBiosVersion,
139+
SettingsFlow: []metalv1alpha1.SettingsFlowItem{{
140+
Settings: map[string]string{},
141+
Priority: 1,
142+
Name: "one",
143+
}},
144+
ServerMaintenancePolicy: metalv1alpha1.ServerMaintenancePolicyEnforced,
145+
},
146+
},
147+
}
148+
Expect(k8sClient.Create(ctx, biosSettingsV2)).To(Succeed())
149+
150+
By("Updating biosSettingsV2 with a serverRef matching V1 but a different version")
151+
biosSettingsV2Updated := biosSettingsV2.DeepCopy()
152+
biosSettingsV2Updated.Spec.ServerRef = &v1.LocalObjectReference{Name: "foo"}
153+
Expect(validator.ValidateUpdate(ctx, biosSettingsV2, biosSettingsV2Updated)).Error().ToNot(HaveOccurred())
154+
})
155+
156+
It("should allow update when spec.serverRef is unchanged even if version now matches another record", func() {
157+
By("Creating a BIOSSettings with different ServerRef")
158+
biosSettingsV2 := &metalv1alpha1.BIOSSettings{
159+
ObjectMeta: metav1.ObjectMeta{
160+
GenerateName: "test-",
161+
},
162+
Spec: metalv1alpha1.BIOSSettingsSpec{
163+
ServerRef: &v1.LocalObjectReference{Name: "bar"},
164+
BIOSSettingsTemplate: metalv1alpha1.BIOSSettingsTemplate{
165+
Version: anotherMockUpServerBiosVersion,
166+
SettingsFlow: []metalv1alpha1.SettingsFlowItem{{
167+
Settings: map[string]string{},
168+
Priority: 1,
169+
Name: "one",
170+
}},
171+
ServerMaintenancePolicy: metalv1alpha1.ServerMaintenancePolicyEnforced,
172+
},
173+
},
174+
}
175+
Expect(k8sClient.Create(ctx, biosSettingsV2)).To(Succeed())
176+
177+
By("Updating biosSettingsV2 version to match V1 without changing serverRef")
178+
biosSettingsV2Updated := biosSettingsV2.DeepCopy()
179+
biosSettingsV2Updated.Spec.Version = defaultMockUpServerBiosVersion
180+
Expect(validator.ValidateUpdate(ctx, biosSettingsV2, biosSettingsV2Updated)).Error().ToNot(HaveOccurred())
181+
})
182+
128183
It("should allow update if a different field is duplicate", func() {
129184
By("Creating a BIOSSetting with different ServerRef")
130185
biosSettingsV2 := &metalv1alpha1.BIOSSettings{

‎internal/webhook/v1alpha1/biosversion_webhook.go‎

Lines changed: 21 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -69,6 +69,13 @@ func (v *BIOSVersionCustomValidator) ValidateUpdate(ctx context.Context, oldObj,
6969
}
7070
}
7171

72+
// If neither the serverRef nor the version changed, no new duplicate can be introduced.
73+
if oldObj.Spec.ServerRef != nil && newObj.Spec.ServerRef != nil &&
74+
oldObj.Spec.ServerRef.Name == newObj.Spec.ServerRef.Name &&
75+
oldObj.Spec.Version == newObj.Spec.Version {
76+
return nil, nil
77+
}
78+
7279
versions := &metalv1alpha1.BIOSVersionList{}
7380
if err := v.List(ctx, versions); err != nil {
7481
return nil, fmt.Errorf("failed to list BIOSVersion: %w", err)
@@ -106,17 +113,21 @@ func checkForDuplicateBIOSVersionRefToServer(versions *metalv1alpha1.BIOSVersion
106113
if bv.Spec.ServerRef == nil {
107114
continue
108115
}
109-
if version.Spec.ServerRef.Name == bv.Spec.ServerRef.Name {
110-
err := fmt.Errorf("server (%s) referred in %s is duplicate of server (%s) referred in %s",
111-
version.Spec.ServerRef.Name,
112-
version.Name,
113-
bv.Spec.ServerRef.Name,
114-
bv.Name,
115-
)
116-
return nil, apierrors.NewInvalid(
117-
schema.GroupKind{Group: version.GroupVersionKind().Group, Kind: version.Kind},
118-
version.GetName(), field.ErrorList{field.Duplicate(field.NewPath("spec").Child("serverRef").Child("name"), err)})
116+
if version.Spec.ServerRef.Name != bv.Spec.ServerRef.Name {
117+
continue
118+
}
119+
if bv.Spec.Version != version.Spec.Version {
120+
continue
119121
}
122+
err := fmt.Errorf("server (%s) referred in %s is duplicate of server (%s) referred in %s",
123+
version.Spec.ServerRef.Name,
124+
version.Name,
125+
bv.Spec.ServerRef.Name,
126+
bv.Name,
127+
)
128+
return nil, apierrors.NewInvalid(
129+
schema.GroupKind{Group: version.GroupVersionKind().Group, Kind: version.Kind},
130+
version.GetName(), field.ErrorList{field.Duplicate(field.NewPath("spec").Child("serverRef").Child("name"), err)})
120131
}
121132
return nil, nil
122133
}

0 commit comments

Comments
 (0)