Skip to content

Rely on spec.maintenance=termination instead of terminating #289

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
fwiesel merged 1 commit into from
Apr 15, 2026
Merged

Rely on spec.maintenance=termination instead of terminating #289

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion internal/controller/aggregates_controller.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -115,7 +115,7 @@ func (ac *AggregatesController) Reconcile(ctx context.Context, req ctrl.Request)
func (ac *AggregatesController) determineDesiredState(hv *kvmv1.Hypervisor) ([]string, metav1.Condition) {
// If terminating AND evicted, remove from all aggregates
// We must wait for eviction to complete before removing aggregates
if meta.IsStatusConditionTrue(hv.Status.Conditions, kvmv1.ConditionTypeTerminating) {
if hv.Spec.Maintenance == kvmv1.MaintenanceTermination {
evictingCondition := meta.FindStatusCondition(hv.Status.Conditions, kvmv1.ConditionTypeEvicting)
// Only remove aggregates if eviction is complete (Evicting=False)
// If Evicting condition is not set or still True, keep current aggregates
Expand Down
95 changes: 73 additions & 22 deletions internal/controller/aggregates_controller_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -650,35 +650,86 @@ var _ = Describe("AggregatesController", func() {

Context("when terminating", func() {
BeforeEach(func(ctx SpecContext) {
By("Setting terminating condition")
By("Setting spec.maintenance=termination")
hypervisor := &kvmv1.Hypervisor{}
Expect(k8sClient.Get(ctx, hypervisorName, hypervisor)).To(Succeed())
meta.SetStatusCondition(&hypervisor.Status.Conditions, metav1.Condition{
Type: kvmv1.ConditionTypeTerminating,
Status: metav1.ConditionTrue,
Reason: "dontcare",
Message: "dontcare",
hypervisor.Spec.Maintenance = kvmv1.MaintenanceTermination
Expect(k8sClient.Update(ctx, hypervisor)).To(Succeed())
})

Context("eviction not yet complete", func() {
BeforeEach(func(ctx SpecContext) {
By("Pre-setting the EvictionInProgress condition to match what controller will determine")
hypervisor := &kvmv1.Hypervisor{}
Expect(k8sClient.Get(ctx, hypervisorName, hypervisor)).To(Succeed())
meta.SetStatusCondition(&hypervisor.Status.Conditions, metav1.Condition{
Type: kvmv1.ConditionTypeAggregatesUpdated,
Status: metav1.ConditionFalse,
Reason: kvmv1.ConditionReasonEvictionInProgress,
Message: "Aggregates unchanged while terminating and eviction in progress",
})
Expect(k8sClient.Status().Update(ctx, hypervisor)).To(Succeed())
})
Expect(k8sClient.Status().Update(ctx, hypervisor)).To(Succeed())

By("Pre-setting the EvictionInProgress condition to match what controller will determine")
Expect(k8sClient.Get(ctx, hypervisorName, hypervisor)).To(Succeed())
meta.SetStatusCondition(&hypervisor.Status.Conditions, metav1.Condition{
Type: kvmv1.ConditionTypeAggregatesUpdated,
Status: metav1.ConditionFalse,
Reason: kvmv1.ConditionReasonEvictionInProgress,
Message: "Aggregates unchanged while terminating and eviction in progress",
It("should keep aggregates unchanged and set EvictionInProgress condition", func(ctx SpecContext) {
updated := &kvmv1.Hypervisor{}
Expect(aggregatesController.Client.Get(ctx, hypervisorName, updated)).To(Succeed())
Expect(updated.Status.Aggregates).To(BeEmpty())
Expect(meta.IsStatusConditionFalse(updated.Status.Conditions, kvmv1.ConditionTypeAggregatesUpdated)).To(BeTrue())
cond := meta.FindStatusCondition(updated.Status.Conditions, kvmv1.ConditionTypeAggregatesUpdated)
Expect(cond.Reason).To(Equal(kvmv1.ConditionReasonEvictionInProgress))
})
Expect(k8sClient.Status().Update(ctx, hypervisor)).To(Succeed())
})

It("should neither update Aggregates and nor set status condition", func(ctx SpecContext) {
updated := &kvmv1.Hypervisor{}
Expect(aggregatesController.Client.Get(ctx, hypervisorName, updated)).To(Succeed())
Expect(updated.Status.Aggregates).To(BeEmpty())
Expect(meta.IsStatusConditionFalse(updated.Status.Conditions, kvmv1.ConditionTypeAggregatesUpdated)).To(BeTrue())
cond := meta.FindStatusCondition(updated.Status.Conditions, kvmv1.ConditionTypeAggregatesUpdated)
Expect(cond.Reason).To(Equal(kvmv1.ConditionReasonEvictionInProgress))
Context("eviction complete (regression: aggregates must be cleared)", func() {
BeforeEach(func(ctx SpecContext) {
By("Setting Evicting=False to signal eviction is complete")
hypervisor := &kvmv1.Hypervisor{}
Expect(k8sClient.Get(ctx, hypervisorName, hypervisor)).To(Succeed())
meta.SetStatusCondition(&hypervisor.Status.Conditions, metav1.Condition{
Type: kvmv1.ConditionTypeEvicting,
Status: metav1.ConditionFalse,
Reason: kvmv1.ConditionReasonSucceeded,
Message: "Evicted",
})
hypervisor.Status.Aggregates = []kvmv1.Aggregate{
{Name: "staging", UUID: "uuid-staging"},
{Name: "qa-de-1b", UUID: "uuid-qa-de-1b"},
}
Expect(k8sClient.Status().Update(ctx, hypervisor)).To(Succeed())

By("Mocking GetAggregates and RemoveHost calls")
fakeServer.Mux.HandleFunc("GET /os-aggregates", func(w http.ResponseWriter, r *http.Request) {
w.Header().Add("Content-Type", "application/json")
w.WriteHeader(http.StatusOK)
fmt.Fprint(w, `{
"aggregates": [
{"name": "staging", "id": 1, "uuid": "uuid-staging", "hosts": ["hv-test"]},
{"name": "qa-de-1b", "id": 2, "uuid": "uuid-qa-de-1b", "hosts": ["hv-test"]}
]
}`)
})
fakeServer.Mux.HandleFunc("POST /os-aggregates/1/action", func(w http.ResponseWriter, r *http.Request) {
w.Header().Add("Content-Type", "application/json")
w.WriteHeader(http.StatusOK)
fmt.Fprint(w, `{"aggregate": {"name": "staging", "id": 1, "uuid": "uuid-staging", "hosts": []}}`)
})
fakeServer.Mux.HandleFunc("POST /os-aggregates/2/action", func(w http.ResponseWriter, r *http.Request) {
w.Header().Add("Content-Type", "application/json")
w.WriteHeader(http.StatusOK)
fmt.Fprint(w, `{"aggregate": {"name": "qa-de-1b", "id": 2, "uuid": "uuid-qa-de-1b", "hosts": []}}`)
})
})

It("should remove all aggregates and set Terminating condition", func(ctx SpecContext) {
updated := &kvmv1.Hypervisor{}
Expect(aggregatesController.Client.Get(ctx, hypervisorName, updated)).To(Succeed())
Expect(updated.Status.Aggregates).To(BeEmpty())
Expect(meta.IsStatusConditionFalse(updated.Status.Conditions, kvmv1.ConditionTypeAggregatesUpdated)).To(BeTrue())
cond := meta.FindStatusCondition(updated.Status.Conditions, kvmv1.ConditionTypeAggregatesUpdated)
Expect(cond).NotTo(BeNil())
Expect(cond.Reason).To(Equal(kvmv1.ConditionReasonTerminating))
})
})
})
})
Expand Down
2 changes: 1 addition & 1 deletion internal/controller/onboarding_controller.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -92,7 +92,7 @@ func (r *OnboardingController) Reconcile(ctx context.Context, req ctrl.Request)
}

// check if hv is terminating
if meta.IsStatusConditionTrue(hv.Status.Conditions, kvmv1.ConditionTypeTerminating) {
if hv.Spec.Maintenance == kvmv1.MaintenanceTermination {
return ctrl.Result{}, r.abortOnboarding(ctx, hv, computeHost)
}
Comment on lines +95 to 97
Copy link
Copy Markdown

@coderabbitai coderabbitai bot Apr 15, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟡 Minor

Abort status message is now misleading for termination-triggered aborts.

Line 95 routes spec.maintenance=termination through abortOnboarding, but that function always sets "Aborted due to LifecycleEnabled being false", which is incorrect for this path.

💡 Proposed fix 
- if !hv.Spec.LifecycleEnabled {
- 	return ctrl.Result{}, r.abortOnboarding(ctx, hv, computeHost)
- }
+ if !hv.Spec.LifecycleEnabled {
+ 	return ctrl.Result{}, r.abortOnboarding(ctx, hv, computeHost, "lifecycle disabled")
+ }

- if hv.Spec.Maintenance == kvmv1.MaintenanceTermination {
- 	return ctrl.Result{}, r.abortOnboarding(ctx, hv, computeHost)
- }
+ if hv.Spec.Maintenance == kvmv1.MaintenanceTermination {
+ 	return ctrl.Result{}, r.abortOnboarding(ctx, hv, computeHost, "termination requested")
+ }

-func (r *OnboardingController) abortOnboarding(ctx context.Context, hv *kvmv1.Hypervisor, computeHost string) error {
+func (r *OnboardingController) abortOnboarding(ctx context.Context, hv *kvmv1.Hypervisor, computeHost, cause string) error {
 	...
 	meta.SetStatusCondition(&hv.Status.Conditions, metav1.Condition{
 		Type:    kvmv1.ConditionTypeOnboarding,
 		Status:  metav1.ConditionFalse,
 		Reason:  kvmv1.ConditionReasonAborted,
-		Message: "Aborted due to LifecycleEnabled being false",
+		Message: fmt.Sprintf("Aborted: %s", cause),
 	})
🤖 Prompt for AI Agents 
Verify each finding against the current code and only fix it if needed.

In `@internal/controller/onboarding_controller.go` around lines 95 - 97, The abort
path for hv.Spec.Maintenance == kvmv1.MaintenanceTermination calls
abortOnboarding but abortOnboarding always logs "Aborted due to LifecycleEnabled
being false", which is incorrect for termination-triggered aborts; update
abortOnboarding usage to accept an explicit abort reason (or set the abort
message on the hv object before calling) and pass a termination-specific reason
when maintenance==MaintenanceTermination so abortOnboarding records/logs
"Aborted due to MaintenanceTermination" (or similar), ensuring abortOnboarding
still uses the provided reason when setting the status/message.


Expand Down
2 changes: 1 addition & 1 deletion internal/controller/traits_controller.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -69,7 +69,7 @@ func (tc *TraitsController) Reconcile(ctx context.Context, req ctrl.Request) (ct
return ctrl.Result{RequeueAfter: 10 * time.Second}, nil
}

if meta.IsStatusConditionTrue(hv.Status.Conditions, kvmv1.ConditionTypeTerminating) {
if hv.Spec.Maintenance == kvmv1.MaintenanceTermination {
return ctrl.Result{}, nil
}

Expand Down
9 changes: 4 additions & 5 deletions internal/controller/traits_controller_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -280,17 +280,16 @@ var _ = Describe("TraitsController", func() {
})

hypervisor := &kvmv1.Hypervisor{}
Expect(k8sClient.Get(ctx, hypervisorName, hypervisor)).To(Succeed())
hypervisor.Spec.Maintenance = kvmv1.MaintenanceTermination
Expect(k8sClient.Update(ctx, hypervisor)).To(Succeed())

Expect(k8sClient.Get(ctx, hypervisorName, hypervisor)).To(Succeed())
meta.SetStatusCondition(&hypervisor.Status.Conditions, metav1.Condition{
Type: kvmv1.ConditionTypeOnboarding,
Status: metav1.ConditionFalse,
Reason: "UnitTest",
})
meta.SetStatusCondition(&hypervisor.Status.Conditions, metav1.Condition{
Type: kvmv1.ConditionTypeTerminating,
Status: metav1.ConditionTrue,
Reason: "UnitTest",
})
hypervisor.Status.Traits = []string{"CUSTOM_FOO", "HW_CPU_X86_VMX"}
hypervisor.Status.HypervisorID = "1234"
Expect(k8sClient.Status().Update(ctx, hypervisor)).To(Succeed())
Expand Down
Loading