Skip to content

Commit 4854c23

Browse files
stefanhipfelstefanhipfel
authored
Ensure ServerMaintenance is removed when performing a BMC version upgrade (#712)
* Fixes #691 * Enhance ServerMaintenance deletion logic and add test for garbage collection * Handle NotFound error in ServerMaintenance deletion logic * Remove orphaned ServerMaintenance cleanup test from BMCVersion controller tests * Refactor BMCVersion controller tests to remove unnecessary context parameter and simplify assertions * improves controller test
1 parent 56e4833 commit 4854c23

2 files changed

Lines changed: 254 additions & 41 deletions

File tree

‎internal/controller/bmcversion_controller.go‎

Lines changed: 21 additions & 28 deletions
Original file line numberDiff line numberDiff line change
@@ -118,41 +118,34 @@ func (r *BMCVersionReconciler) delete(ctx context.Context, bmcVersion *metalv1al
118118

119119
func (r *BMCVersionReconciler) removeServerMaintenances(ctx context.Context, bmcVersion *metalv1alpha1.BMCVersion) error {
120120
log := ctrl.LoggerFrom(ctx)
121-
if bmcVersion.Spec.ServerMaintenanceRefs == nil {
122-
return nil
123-
}
124121

125122
log.V(1).Info("Removing orphan server Maintenances")
126-
serverMaintenances, errs := r.getServerMaintenances(ctx, bmcVersion)
127-
128-
var finalErr []error
129-
var missingServerMaintenanceRef []error
130123

131-
if len(errs) > 0 {
132-
for _, err := range errs {
133-
if apierrors.IsNotFound(err) {
134-
missingServerMaintenanceRef = append(missingServerMaintenanceRef, err)
135-
} else {
136-
finalErr = append(finalErr, err)
137-
}
138-
}
124+
// List all ServerMaintenance objects owned by this BMCVersion using owner references
125+
serverMaintenanceList := &metalv1alpha1.ServerMaintenanceList{}
126+
if err := clientutils.ListAndFilterControlledBy(ctx, r.Client, bmcVersion, serverMaintenanceList); err != nil {
127+
return fmt.Errorf("failed to list ServerMaintenance objects: %w", err)
139128
}
140129

141-
if len(missingServerMaintenanceRef) != len(bmcVersion.Spec.ServerMaintenanceRefs) {
142-
// delete the ServerMaintenance if not marked for deletion already
143-
for _, serverMaintenance := range serverMaintenances {
144-
if serverMaintenance.DeletionTimestamp.IsZero() && metav1.IsControlledBy(serverMaintenance, bmcVersion) {
145-
log.V(1).Info("Deleting ServerMaintenance", "ServerMaintenance", client.ObjectKeyFromObject(serverMaintenance))
146-
if err := r.Delete(ctx, serverMaintenance); err != nil {
147-
log.V(1).Info("Failed to delete ServerMaintenance", "ServerMaintenance", client.ObjectKeyFromObject(serverMaintenance))
148-
finalErr = append(finalErr, err)
130+
var finalErr []error
131+
132+
// Delete all ServerMaintenance objects owned by this BMCVersion
133+
for i := range serverMaintenanceList.Items {
134+
serverMaintenance := &serverMaintenanceList.Items[i]
135+
if serverMaintenance.DeletionTimestamp.IsZero() && metav1.IsControlledBy(serverMaintenance, bmcVersion) {
136+
log.V(1).Info("Deleting ServerMaintenance", "ServerMaintenance", client.ObjectKeyFromObject(serverMaintenance))
137+
if err := r.Delete(ctx, serverMaintenance); err != nil {
138+
if apierrors.IsNotFound(err) {
139+
continue
149140
}
150-
} else {
151-
log.V(1).Info(
152-
"Skipping deletion of ServerMaintenance as it has a different owner",
153-
"ServerMaintenance", client.ObjectKeyFromObject(serverMaintenance),
154-
"State", serverMaintenance.Status.State)
141+
log.V(1).Info("Failed to delete ServerMaintenance", "ServerMaintenance", client.ObjectKeyFromObject(serverMaintenance))
142+
finalErr = append(finalErr, err)
155143
}
144+
} else {
145+
log.V(1).Info(
146+
"Skipping deletion of ServerMaintenance as it has a different owner",
147+
"ServerMaintenance", client.ObjectKeyFromObject(serverMaintenance),
148+
"State", serverMaintenance.Status.State)
156149
}
157150
}
158151

‎internal/controller/bmcversion_controller_test.go‎

Lines changed: 233 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -4,11 +4,10 @@
44
package controller
55

66
import (
7-
"context"
7+
"fmt"
88

99
. "github.com/onsi/ginkgo/v2"
1010
. "github.com/onsi/gomega"
11-
"sigs.k8s.io/controller-runtime/pkg/client"
1211
. "sigs.k8s.io/controller-runtime/pkg/envtest/komega"
1312

1413
v1 "k8s.io/api/core/v1"
@@ -213,7 +212,7 @@ var _ = Describe("BMCVersion Controller", func() {
213212
}),
214213
))
215214

216-
ensureBMCVersionConditionTransition(ctx, acc, bmcVersion)
215+
ensureBMCVersionConditionTransition(acc, bmcVersion)
217216

218217
By("Ensuring that BMC upgrade has completed")
219218
Eventually(Object(bmcVersion)).Should(
@@ -348,7 +347,7 @@ var _ = Describe("BMCVersion Controller", func() {
348347
}),
349348
))
350349

351-
ensureBMCVersionConditionTransition(ctx, acc, bmcVersion)
350+
ensureBMCVersionConditionTransition(acc, bmcVersion)
352351

353352
By("Ensuring that BMC upgrade has completed")
354353
Eventually(Object(bmcVersion)).Should(
@@ -419,50 +418,271 @@ var _ = Describe("BMCVersion Controller", func() {
419418
server.Status.State = metalv1alpha1.ServerStateAvailable
420419
})).Should(Succeed())
421420
})
421+
422+
It("Should cleanup ServerMaintenance when references are cleared", func(ctx SpecContext) {
423+
By("Creating a BMCVersion with maintenance upgrade")
424+
bmcVersion := &metalv1alpha1.BMCVersion{
425+
ObjectMeta: metav1.ObjectMeta{
426+
Namespace: ns.Name,
427+
GenerateName: "test-",
428+
},
429+
Spec: metalv1alpha1.BMCVersionSpec{
430+
BMCRef: &v1.LocalObjectReference{Name: bmcObj.Name},
431+
BMCVersionTemplate: metalv1alpha1.BMCVersionTemplate{
432+
Version: upgradeServerBMCVersion,
433+
Image: metalv1alpha1.ImageSpec{URI: upgradeServerBMCVersion},
434+
ServerMaintenancePolicy: metalv1alpha1.ServerMaintenancePolicyEnforced,
435+
},
436+
},
437+
}
438+
Expect(k8sClient.Create(ctx, bmcVersion)).To(Succeed())
439+
440+
By("Ensuring that the Maintenance resource has been created")
441+
var serverMaintenanceList metalv1alpha1.ServerMaintenanceList
442+
Eventually(ObjectList(&serverMaintenanceList)).Should(HaveField("Items", HaveLen(1)))
443+
444+
serverMaintenance := &metalv1alpha1.ServerMaintenance{
445+
ObjectMeta: metav1.ObjectMeta{
446+
Namespace: ns.Name,
447+
Name: serverMaintenanceList.Items[0].Name,
448+
},
449+
}
450+
Eventually(Get(serverMaintenance)).Should(Succeed())
451+
452+
By("Manually clearing spec.serverMaintenanceRefs while keeping the object alive")
453+
Eventually(Update(bmcVersion, func() {
454+
bmcVersion.Spec.ServerMaintenanceRefs = nil
455+
})).Should(Succeed())
456+
457+
By("Ensuring that the orphaned ServerMaintenance is eventually cleaned up")
458+
Eventually(Get(serverMaintenance)).Should(Satisfy(apierrors.IsNotFound))
459+
460+
By("Verifying no ServerMaintenance objects remain")
461+
Expect(ObjectList(&serverMaintenanceList)()).Should(HaveField("Items", BeEmpty()))
462+
463+
// cleanup
464+
Expect(k8sClient.Delete(ctx, bmcVersion)).To(Succeed())
465+
466+
// Wait for garbage collection to delete owned ServerMaintenance objects
467+
Eventually(ObjectList(&serverMaintenanceList)).Should(HaveField("Items", BeEmpty()))
468+
})
469+
470+
It("Should maintain cleanup functionality with populated spec.serverMaintenanceRefs", func(ctx SpecContext) {
471+
By("Creating a BMCVersion with maintenance upgrade")
472+
bmcVersion := &metalv1alpha1.BMCVersion{
473+
ObjectMeta: metav1.ObjectMeta{
474+
Namespace: ns.Name,
475+
GenerateName: "test-",
476+
},
477+
Spec: metalv1alpha1.BMCVersionSpec{
478+
BMCRef: &v1.LocalObjectReference{Name: bmcObj.Name},
479+
BMCVersionTemplate: metalv1alpha1.BMCVersionTemplate{
480+
Version: upgradeServerBMCVersion,
481+
Image: metalv1alpha1.ImageSpec{URI: upgradeServerBMCVersion},
482+
ServerMaintenancePolicy: metalv1alpha1.ServerMaintenancePolicyEnforced,
483+
},
484+
},
485+
}
486+
Expect(k8sClient.Create(ctx, bmcVersion)).To(Succeed())
487+
488+
By("Ensuring that the bmcVersion has entered Inprogress state")
489+
Eventually(Object(bmcVersion)).Should(
490+
HaveField("Status.State", metalv1alpha1.BMCVersionStateInProgress),
491+
)
492+
493+
By("Ensuring that the Maintenance resource has been created")
494+
var serverMaintenanceList metalv1alpha1.ServerMaintenanceList
495+
Eventually(ObjectList(&serverMaintenanceList)).Should(HaveField("Items", HaveLen(1)))
496+
497+
serverMaintenance := &metalv1alpha1.ServerMaintenance{
498+
ObjectMeta: metav1.ObjectMeta{
499+
Namespace: ns.Name,
500+
Name: serverMaintenanceList.Items[0].Name,
501+
},
502+
}
503+
Eventually(Get(serverMaintenance)).Should(Succeed())
504+
505+
By("Ensuring that spec.serverMaintenanceRefs is populated")
506+
Eventually(Object(bmcVersion)).Should(
507+
HaveField("Spec.ServerMaintenanceRefs", ContainElement(
508+
HaveField("Name", serverMaintenance.Name),
509+
)),
510+
)
511+
512+
By("Simulating cleanup by mocking upgrade completion")
513+
Eventually(UpdateStatus(bmcVersion, func() {
514+
bmcVersion.Status.State = metalv1alpha1.BMCVersionStateCompleted
515+
})).Should(Succeed())
516+
517+
By("Triggering reconciliation to cleanup references")
518+
Eventually(Update(bmcVersion, func() {
519+
metautils.SetAnnotation(bmcVersion, "cleanup-trigger", "true")
520+
})).Should(Succeed())
521+
522+
By("Ensuring that ServerMaintenance objects are cleaned up")
523+
Eventually(ObjectList(&serverMaintenanceList)).Should(HaveField("Items", BeEmpty()))
524+
525+
By("Verifying spec.serverMaintenanceRefs is cleared")
526+
Eventually(Object(bmcVersion)).Should(
527+
HaveField("Spec.ServerMaintenanceRefs", BeNil()),
528+
)
529+
// cleanup
530+
Expect(k8sClient.Delete(ctx, bmcVersion)).To(Succeed())
531+
Eventually(ObjectList(&serverMaintenanceList)).Should(HaveField("Items", BeEmpty()))
532+
})
533+
534+
It("Should not delete ServerMaintenance objects not owned by BMCVersion", func(ctx SpecContext) {
535+
By("Creating a standalone ServerMaintenance without owner reference")
536+
orphanMaintenance := &metalv1alpha1.ServerMaintenance{
537+
ObjectMeta: metav1.ObjectMeta{
538+
Namespace: ns.Name,
539+
GenerateName: "orphan-",
540+
},
541+
Spec: metalv1alpha1.ServerMaintenanceSpec{
542+
ServerRef: &v1.LocalObjectReference{Name: server.Name},
543+
Policy: metalv1alpha1.ServerMaintenancePolicyEnforced,
544+
},
545+
}
546+
Expect(k8sClient.Create(ctx, orphanMaintenance)).To(Succeed())
547+
548+
By("Creating a BMCVersion with orphaned state")
549+
bmcVersion := &metalv1alpha1.BMCVersion{
550+
ObjectMeta: metav1.ObjectMeta{
551+
Namespace: ns.Name,
552+
GenerateName: "test-",
553+
},
554+
Spec: metalv1alpha1.BMCVersionSpec{
555+
BMCRef: &v1.LocalObjectReference{Name: bmcObj.Name},
556+
BMCVersionTemplate: metalv1alpha1.BMCVersionTemplate{
557+
Version: defaultMockUpServerBMCVersion,
558+
Image: metalv1alpha1.ImageSpec{URI: defaultMockUpServerBMCVersion},
559+
ServerMaintenancePolicy: metalv1alpha1.ServerMaintenancePolicyEnforced,
560+
},
561+
},
562+
}
563+
Expect(k8sClient.Create(ctx, bmcVersion)).To(Succeed())
564+
565+
By("Setting spec.serverMaintenanceRefs to nil")
566+
Eventually(Update(bmcVersion, func() {
567+
bmcVersion.Spec.ServerMaintenanceRefs = nil
568+
})).Should(Succeed())
569+
570+
By("Recording the non-owned ServerMaintenance for later verification")
571+
nonOwnedMaintenance := &metalv1alpha1.ServerMaintenance{
572+
ObjectMeta: metav1.ObjectMeta{
573+
Namespace: ns.Name,
574+
Name: orphanMaintenance.Name,
575+
},
576+
}
577+
Expect(Get(nonOwnedMaintenance)()).To(Succeed())
578+
579+
By("Triggering reconciliation")
580+
Eventually(Update(bmcVersion, func() {
581+
metautils.SetAnnotation(bmcVersion, "test", "verify-ownership")
582+
})).Should(Succeed())
583+
584+
By("Verifying that the non-owned ServerMaintenance still exists")
585+
Consistently(Get(nonOwnedMaintenance)).Should(Succeed())
586+
587+
By("Verifying no other ServerMaintenance objects were created")
588+
var serverMaintenanceList metalv1alpha1.ServerMaintenanceList
589+
Expect(ObjectList(&serverMaintenanceList)()).To(HaveField("Items", HaveLen(1)))
590+
Expect(serverMaintenanceList.Items[0].Name).To(Equal(orphanMaintenance.Name))
591+
592+
// cleanup
593+
Expect(k8sClient.Delete(ctx, nonOwnedMaintenance)).To(Succeed())
594+
Expect(k8sClient.Delete(ctx, bmcVersion)).To(Succeed())
595+
Eventually(ObjectList(&serverMaintenanceList)).Should(HaveField("Items", BeEmpty()))
596+
})
597+
598+
It("Should be idempotent when cleaning up orphaned ServerMaintenance", func(ctx SpecContext) {
599+
By("Creating a BMCVersion with maintenance upgrade")
600+
bmcVersion := &metalv1alpha1.BMCVersion{
601+
ObjectMeta: metav1.ObjectMeta{
602+
Namespace: ns.Name,
603+
GenerateName: "test-",
604+
},
605+
Spec: metalv1alpha1.BMCVersionSpec{
606+
BMCRef: &v1.LocalObjectReference{Name: bmcObj.Name},
607+
BMCVersionTemplate: metalv1alpha1.BMCVersionTemplate{
608+
Version: upgradeServerBMCVersion,
609+
Image: metalv1alpha1.ImageSpec{URI: upgradeServerBMCVersion},
610+
ServerMaintenancePolicy: metalv1alpha1.ServerMaintenancePolicyEnforced,
611+
},
612+
},
613+
}
614+
Expect(k8sClient.Create(ctx, bmcVersion)).To(Succeed())
615+
616+
By("Ensuring that the Maintenance resource has been created")
617+
var serverMaintenanceList metalv1alpha1.ServerMaintenanceList
618+
Eventually(ObjectList(&serverMaintenanceList)).Should(HaveField("Items", HaveLen(1)))
619+
620+
By("Clearing spec.serverMaintenanceRefs")
621+
Eventually(Update(bmcVersion, func() {
622+
bmcVersion.Spec.ServerMaintenanceRefs = nil
623+
})).Should(Succeed())
624+
625+
By("Triggering reconciliation multiple times")
626+
for i := range 3 {
627+
Eventually(Update(bmcVersion, func() {
628+
metautils.SetAnnotation(bmcVersion, "iteration", fmt.Sprintf("%d", i))
629+
})).Should(Succeed())
630+
}
631+
632+
By("Ensuring that cleanup is successful and idempotent")
633+
Eventually(ObjectList(&serverMaintenanceList)).Should(HaveField("Items", BeEmpty()))
634+
635+
By("Verifying no errors occurred during reconciliation")
636+
Expect(Get(bmcVersion)()).To(Succeed())
637+
// If reconciliation failed, Status would have error conditions
638+
// This is a simplified check; detailed error checking would depend on status fields
639+
640+
// cleanup
641+
Expect(k8sClient.Delete(ctx, bmcVersion)).To(Succeed())
642+
})
422643
})
423644

424-
func ensureBMCVersionConditionTransition(ctx context.Context, acc *conditionutils.Accessor, bmcVersion *metalv1alpha1.BMCVersion) {
645+
func ensureBMCVersionConditionTransition(acc *conditionutils.Accessor, bmcVersion *metalv1alpha1.BMCVersion) {
425646
GinkgoHelper()
426-
427647
By("Ensuring that BMC Conditions have reached expected state 'biosVersionUpgradeIssued'")
428648
condIssue := &metav1.Condition{}
429649
Eventually(func(g Gomega) int {
430-
g.Expect(k8sClient.Get(ctx, client.ObjectKey{Name: bmcVersion.Name}, bmcVersion)).To(Succeed())
650+
g.Expect(Get(bmcVersion)()).To(Succeed())
431651
return len(bmcVersion.Status.Conditions)
432652
}).Should(BeNumerically(">=", 1))
433653
Eventually(func(g Gomega) bool {
434-
g.Expect(k8sClient.Get(ctx, client.ObjectKey{Name: bmcVersion.Name}, bmcVersion)).To(Succeed())
654+
g.Expect(Get(bmcVersion)()).To(Succeed())
435655
g.Expect(acc.FindSlice(bmcVersion.Status.Conditions, bmcVersionUpgradeIssued, condIssue)).To(BeTrue())
436656
return condIssue.Status == metav1.ConditionTrue
437657
}).Should(BeTrue())
438658

439659
By("Ensuring that BMCVersion has updated the task Status with task URI")
440660
Eventually(func(g Gomega) {
441-
g.Expect(k8sClient.Get(ctx, client.ObjectKeyFromObject(bmcVersion), bmcVersion)).To(Succeed())
661+
g.Expect(Get(bmcVersion)()).To(Succeed())
442662
g.Expect(bmcVersion.Status.UpgradeTask).NotTo(BeNil())
443663
g.Expect(bmcVersion.Status.UpgradeTask.URI).To(Equal(bmc.DummyMockTaskForUpgrade))
444664
}).Should(Succeed())
445665

446666
By("Ensuring that BMC Conditions have reached expected state 'biosVersionUpgradeCompleted'")
447667
condComplete := &metav1.Condition{}
448668
Eventually(func(g Gomega) int {
449-
g.Expect(k8sClient.Get(ctx, client.ObjectKey{Name: bmcVersion.Name}, bmcVersion)).To(Succeed())
669+
g.Expect(Get(bmcVersion)()).To(Succeed())
450670
return len(bmcVersion.Status.Conditions)
451671
}).Should(BeNumerically(">=", 2))
452672
Eventually(func(g Gomega) bool {
453-
g.Expect(k8sClient.Get(ctx, client.ObjectKey{Name: bmcVersion.Name}, bmcVersion)).To(Succeed())
673+
g.Expect(Get(bmcVersion)()).To(Succeed())
454674
g.Expect(acc.FindSlice(bmcVersion.Status.Conditions, bmcVersionUpgradeCompleted, condComplete)).To(BeTrue())
455675
return condComplete.Status == metav1.ConditionTrue
456676
}).Should(BeTrue())
457677

458678
By("Ensuring that BMC Conditions have reached expected state 'biosVersionUpgradeVerificationCondition'")
459679
verificationComplete := &metav1.Condition{}
460680
Eventually(func(g Gomega) int {
461-
g.Expect(k8sClient.Get(ctx, client.ObjectKey{Name: bmcVersion.Name}, bmcVersion)).To(Succeed())
681+
g.Expect(Get(bmcVersion)()).To(Succeed())
462682
return len(bmcVersion.Status.Conditions)
463683
}).Should(BeNumerically(">=", 4))
464684
Eventually(func(g Gomega) bool {
465-
g.Expect(k8sClient.Get(ctx, client.ObjectKey{Name: bmcVersion.Name}, bmcVersion)).To(Succeed())
685+
g.Expect(Get(bmcVersion)()).To(Succeed())
466686
g.Expect(acc.FindSlice(bmcVersion.Status.Conditions, bmcVersionUpgradeVerificationCondition, verificationComplete)).To(BeTrue())
467687
return verificationComplete.Status == metav1.ConditionTrue
468688
}).Should(BeTrue())

0 commit comments

Comments
 (0)