Skip to content

Offboarding hypervisor #209

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

Closed
fwiesel wants to merge 3 commits into from
Closed

Offboarding hypervisor #209

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
e16f187
Normalize tests on SpecContext in leaf
fwiesel Dec 19, 2025
bbaf78c
Offboarding: Unit test the happy path
fwiesel Dec 18, 2025
9bb91b2
Decomission: Base it on the hypervisor
fwiesel Dec 18, 2025
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
23 changes: 11 additions & 12 deletions internal/controller/aggregates_controller_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,6 @@ limitations under the License.
package controller

import (
"context"
"fmt"
"net/http"

Expand Down Expand Up @@ -105,7 +104,7 @@ var _ = Describe("AggregatesController", func() {
)
// Setup and teardown

BeforeEach(func(ctx context.Context) {
BeforeEach(func(ctx SpecContext) {
By("Setting up the OpenStack http mock server")
fakeServer = testhelper.SetupHTTP()
DeferCleanup(fakeServer.Teardown)
Expand Down Expand Up @@ -135,19 +134,19 @@ var _ = Describe("AggregatesController", func() {
computeClient: client.ServiceClient(fakeServer),
}

DeferCleanup(func(ctx context.Context) {
DeferCleanup(func(ctx SpecContext) {
Expect(tc.Client.Delete(ctx, hypervisor)).To(Succeed())
})
})

JustBeforeEach(func(ctx context.Context) {
JustBeforeEach(func(ctx SpecContext) {
_, err := tc.Reconcile(ctx, ctrl.Request{NamespacedName: hypervisorName})
Expect(err).NotTo(HaveOccurred())
})

// Tests
Context("Adding new Aggregate", func() {
BeforeEach(func() {
BeforeEach(func(ctx SpecContext) {
By("Setting a missing aggregate")
hypervisor := &kvmv1.Hypervisor{}
Expect(k8sClient.Get(ctx, hypervisorName, hypervisor)).To(Succeed())
Expand Down Expand Up @@ -189,7 +188,7 @@ var _ = Describe("AggregatesController", func() {
})
})

It("should update Aggregates and set status condition as Aggregates differ", func() {
It("should update Aggregates and set status condition as Aggregates differ", func(ctx SpecContext) {
updated := &kvmv1.Hypervisor{}
Expect(tc.Client.Get(ctx, hypervisorName, updated)).To(Succeed())
Expect(updated.Status.Aggregates).To(ContainElements("test-aggregate1"))
Expand All @@ -198,7 +197,7 @@ var _ = Describe("AggregatesController", func() {
})

Context("Removing Aggregate", func() {
BeforeEach(func() {
BeforeEach(func(ctx SpecContext) {
hypervisor := &kvmv1.Hypervisor{}
Expect(k8sClient.Get(ctx, hypervisorName, hypervisor)).To(Succeed())
// update status to have existing aggregate
Expand Down Expand Up @@ -233,7 +232,7 @@ var _ = Describe("AggregatesController", func() {
fakeServer.Mux.HandleFunc("POST /os-aggregates/99/action", expectRemoveHostFromAggregate)
})

It("should update Aggregates and set status condition when Aggregates differ", func() {
It("should update Aggregates and set status condition when Aggregates differ", func(ctx SpecContext) {
updated := &kvmv1.Hypervisor{}
Expect(tc.Client.Get(ctx, hypervisorName, updated)).To(Succeed())
Expect(updated.Status.Aggregates).To(BeEmpty())
Expand All @@ -242,15 +241,15 @@ var _ = Describe("AggregatesController", func() {
})

Context("before onboarding", func() {
BeforeEach(func() {
BeforeEach(func(ctx SpecContext) {
hypervisor := &kvmv1.Hypervisor{}
Expect(k8sClient.Get(ctx, hypervisorName, hypervisor)).To(Succeed())
// Remove the onboarding condition
hypervisor.Status.Conditions = []v1.Condition{}
Expect(k8sClient.Status().Update(ctx, hypervisor)).To(Succeed())
})

It("should neither update Aggregates and nor set status condition", func() {
It("should neither update Aggregates and nor set status condition", func(ctx SpecContext) {
updated := &kvmv1.Hypervisor{}
Expect(tc.Client.Get(ctx, hypervisorName, updated)).To(Succeed())
Expect(updated.Status.Aggregates).To(BeEmpty())
Expand All @@ -259,7 +258,7 @@ var _ = Describe("AggregatesController", func() {
})

Context("when terminating", func() {
BeforeEach(func() {
BeforeEach(func(ctx SpecContext) {
hypervisor := &kvmv1.Hypervisor{}
Expect(k8sClient.Get(ctx, hypervisorName, hypervisor)).To(Succeed())
// Remove the onboarding condition
Expand All @@ -272,7 +271,7 @@ var _ = Describe("AggregatesController", func() {
Expect(k8sClient.Status().Update(ctx, hypervisor)).To(Succeed())
})

It("should neither update Aggregates and nor set status condition", func() {
It("should neither update Aggregates and nor set status condition", func(ctx SpecContext) {
updated := &kvmv1.Hypervisor{}
Expect(tc.Client.Get(ctx, hypervisorName, updated)).To(Succeed())
Expect(updated.Status.Aggregates).To(BeEmpty())
Expand Down
46 changes: 19 additions & 27 deletions internal/controller/decomission_controller.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,6 @@ import (
"github.com/gophercloud/gophercloud/v2/openstack/compute/v2/aggregates"
"github.com/gophercloud/gophercloud/v2/openstack/compute/v2/services"
"github.com/gophercloud/gophercloud/v2/openstack/placement/v1/resourceproviders"
corev1 "k8s.io/api/core/v1"
"k8s.io/apimachinery/pkg/api/meta"
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
"k8s.io/apimachinery/pkg/runtime"
Expand All @@ -44,7 +43,7 @@ import (

const (
decommissionFinalizerName = "cobaltcore.cloud.sap/decommission-hypervisor"
DecommissionControllerName = "nodeDecommission"
DecommissionControllerName = "decommission"
)

type NodeDecommissionReconciler struct {
Expand All @@ -57,20 +56,13 @@ type NodeDecommissionReconciler struct {
// The counter-side in gardener is here:
// https://github.com/gardener/machine-controller-manager/blob/rel-v0.56/pkg/util/provider/machinecontroller/machine.go#L646

// +kubebuilder:rbac:groups="",resources=nodes,verbs=get;list;watch;patch;update
// +kubebuilder:rbac:groups="",resources=nodes/finalizers,verbs=update
// +kubebuilder:rbac:groups=kvm.cloud.sap,resources=hypervisors,verbs=get;list;watch
// +kubebuilder:rbac:groups=kvm.cloud.sap,resources=hypervisors/status,verbs=get;list;watch;update;patch
func (r *NodeDecommissionReconciler) Reconcile(ctx context.Context, req ctrl.Request) (ctrl.Result, error) {
hostname := req.Name
log := logger.FromContext(ctx).WithName(req.Name).WithValues("hostname", hostname)
ctx = logger.IntoContext(ctx, log)

node := &corev1.Node{}
if err := r.Get(ctx, req.NamespacedName, node); err != nil {
return ctrl.Result{}, k8sclient.IgnoreNotFound(err)
}

// Fetch HV to check if lifecycle management is enabled
hv := &kvmv1.Hypervisor{}
if err := r.Get(ctx, k8sclient.ObjectKey{Name: hostname}, hv); err != nil {
Expand All @@ -79,14 +71,14 @@ func (r *NodeDecommissionReconciler) Reconcile(ctx context.Context, req ctrl.Req
}
if !hv.Spec.LifecycleEnabled {
// Get out of the way
return r.removeFinalizer(ctx, node)
return r.removeFinalizer(ctx, hv)
}

if !controllerutil.ContainsFinalizer(node, decommissionFinalizerName) {
if !controllerutil.ContainsFinalizer(hv, decommissionFinalizerName) {
return ctrl.Result{}, retry.RetryOnConflict(retry.DefaultRetry, func() error {
patch := k8sclient.MergeFrom(node.DeepCopy())
controllerutil.AddFinalizer(node, decommissionFinalizerName)
if err := r.Patch(ctx, node, patch); err != nil {
patch := k8sclient.MergeFrom(hv.DeepCopy())
controllerutil.AddFinalizer(hv, decommissionFinalizerName)
if err := r.Patch(ctx, hv, patch); err != nil {
return fmt.Errorf("failed to add finalizer due to %w", err)
}
log.Info("Added finalizer")
Expand All @@ -95,16 +87,16 @@ func (r *NodeDecommissionReconciler) Reconcile(ctx context.Context, req ctrl.Req
}

// Not yet deleting hv, nothing more to do
if node.DeletionTimestamp.IsZero() {
if hv.DeletionTimestamp.IsZero() {
return ctrl.Result{}, nil
}

// Someone is just deleting the hv, without going through termination
// See: https://github.com/gardener/machine-controller-manager/blob/rel-v0.56/pkg/util/provider/machinecontroller/machine.go#L658-L659
if !IsNodeConditionTrue(node.Status.Conditions, "Terminating") {
if !meta.IsStatusConditionTrue(hv.Status.Conditions, "Terminating") {
log.Info("removing finalizer since not terminating")
// So we just get out of the way for now
return r.removeFinalizer(ctx, node)
return r.removeFinalizer(ctx, hv)
}

if meta.IsStatusConditionTrue(hv.Status.Conditions, kvmv1.ConditionTypeReady) {
Expand All @@ -116,7 +108,7 @@ func (r *NodeDecommissionReconciler) Reconcile(ctx context.Context, req ctrl.Req
hypervisor, err := openstack.GetHypervisorByName(ctx, r.computeClient, hostname, true)
if errors.Is(err, openstack.ErrNoHypervisor) {
// We are (hopefully) done
return r.removeFinalizer(ctx, node)
return r.removeFinalizer(ctx, hv)
}

// TODO: remove since RunningVMs is only available until micro-version 2.87, and also is updated asynchronously
Expand All @@ -141,7 +133,7 @@ func (r *NodeDecommissionReconciler) Reconcile(ctx context.Context, req ctrl.Req
return r.setDecommissioningCondition(ctx, hv, fmt.Sprintf("cannot list aggregates due to %v", err))
}

host := node.Name
host := hv.Name
for name, aggregate := range aggs {
if slices.Contains(aggregate.Hosts, host) {
opts := aggregates.RemoveHostOpts{Host: host}
Expand All @@ -168,17 +160,17 @@ func (r *NodeDecommissionReconciler) Reconcile(ctx context.Context, req ctrl.Req
return r.setDecommissioningCondition(ctx, hv, fmt.Sprintf("cannot clean up resource provider: %v", err))
}

return r.removeFinalizer(ctx, node)
return r.removeFinalizer(ctx, hv)
}

func (r *NodeDecommissionReconciler) removeFinalizer(ctx context.Context, node *corev1.Node) (ctrl.Result, error) {
if !controllerutil.ContainsFinalizer(node, decommissionFinalizerName) {
func (r *NodeDecommissionReconciler) removeFinalizer(ctx context.Context, hypervisor *kvmv1.Hypervisor) (ctrl.Result, error) {
if !controllerutil.ContainsFinalizer(hypervisor, decommissionFinalizerName) {
return ctrl.Result{}, nil
}

nodeBase := node.DeepCopy()
controllerutil.RemoveFinalizer(node, decommissionFinalizerName)
err := r.Patch(ctx, node, k8sclient.MergeFromWithOptions(nodeBase,
base := hypervisor.DeepCopy()
controllerutil.RemoveFinalizer(hypervisor, decommissionFinalizerName)
err := r.Patch(ctx, hypervisor, k8sclient.MergeFromWithOptions(base,
k8sclient.MergeFromWithOptimisticLock{}), k8sclient.FieldOwner(DecommissionControllerName))
return ctrl.Result{}, err
}
Expand All @@ -195,7 +187,7 @@ func (r *NodeDecommissionReconciler) setDecommissioningCondition(ctx context.Con
k8sclient.MergeFromWithOptimisticLock{}), k8sclient.FieldOwner(DecommissionControllerName)); err != nil {
return ctrl.Result{}, fmt.Errorf("cannot update hypervisor status due to %w", err)
}
return ctrl.Result{RequeueAfter: shortRetryTime}, nil
return ctrl.Result{}, nil
}

// SetupWithManager sets up the controller with the Manager.
Expand All @@ -217,6 +209,6 @@ func (r *NodeDecommissionReconciler) SetupWithManager(mgr ctrl.Manager) error {

return ctrl.NewControllerManagedBy(mgr).
Named(DecommissionControllerName).
For(&corev1.Node{}).
For(&kvmv1.Hypervisor{}).
Complete(r)
}
Loading