From c60920c6272b21b838bbdbe17bed9b213b22a286 Mon Sep 17 00:00:00 2001 From: Zack Zlotnik Date: Thu, 13 Aug 2026 12:40:09 -0400 Subject: [PATCH] controller: unlabel nodes when pool is deleted Whenever a BootcNodePool is deleted, the nodes which belong to that pool were still being managed because the labels were not removed from the nodes which belong to that pool. This meant that the daemon continued to run on those nodes. Adding a finalizer to the pool blocks deletion until all of the nodes which belong to that pool have been unlabeled. Assisted-by: AI Signed-off-by: Zack Zlotnik --- api/v1alpha1/constants.go | 5 + .../controller/bootcnodepool_controller.go | 58 ++++++++++++ internal/controller/membership_test.go | 93 +++++++++++++++++++ test/util/builders.go | 5 + 4 files changed, 161 insertions(+) diff --git a/api/v1alpha1/constants.go b/api/v1alpha1/constants.go index 71fae9d..eea2361 100644 --- a/api/v1alpha1/constants.go +++ b/api/v1alpha1/constants.go @@ -2,6 +2,11 @@ package v1alpha1 +// FinalizerPoolCleanup is added to every BootcNodePool so the controller +// can remove the bootc.dev/managed label from member nodes before the +// pool object is fully deleted. +const FinalizerPoolCleanup = "bootc.dev/pool-cleanup" + // Well-known labels and annotations applied to Nodes by the controller. const ( // LabelManaged is set on Nodes that are managed by a BootcNodePool. diff --git a/internal/controller/bootcnodepool_controller.go b/internal/controller/bootcnodepool_controller.go index 59eb7fe..5d6770e 100644 --- a/internal/controller/bootcnodepool_controller.go +++ b/internal/controller/bootcnodepool_controller.go @@ -231,6 +231,22 @@ func (r *BootcNodePoolReconciler) Reconcile(ctx context.Context, req ctrl.Reques return ctrl.Result{}, fmt.Errorf("fetching pool: %w", err) } + // If the pool is being deleted, clean up managed nodes before allowing + // Kubernetes to remove the object. + if pool.DeletionTimestamp != nil { + return r.handlePoolDeletion(ctx, &pool) + } + + // Ensure the cleanup finalizer is present so we can react to deletion. + if !controllerutil.ContainsFinalizer(&pool, bootcv1alpha1.FinalizerPoolCleanup) { + controllerutil.AddFinalizer(&pool, bootcv1alpha1.FinalizerPoolCleanup) + if err := r.Update(ctx, &pool); err != nil { + return ctrl.Result{}, fmt.Errorf("adding finalizer: %w", err) + } + // Re-fetch will be triggered by the update event; return early. + return ctrl.Result{}, nil + } + // Snapshot status so we can detect changes and write once at the end. statusOrig := pool.Status.DeepCopy() @@ -303,6 +319,48 @@ func (r *BootcNodePoolReconciler) Reconcile(ctx context.Context, req ctrl.Reques return complete(resolveResult) } +// handlePoolDeletion is called when a pool's DeletionTimestamp is set. +// It removes the bootc.dev/managed label from all member nodes and deletes +// the owned BootcNode objects, then removes the cleanup finalizer so +// Kubernetes can complete the deletion. +func (r *BootcNodePoolReconciler) handlePoolDeletion(ctx context.Context, pool *bootcv1alpha1.BootcNodePool) (ctrl.Result, error) { + log := logf.FromContext(ctx).WithValues("pool", pool.Name) + + if !controllerutil.ContainsFinalizer(pool, bootcv1alpha1.FinalizerPoolCleanup) { + // Finalizer already removed; nothing left to do. + return ctrl.Result{}, nil + } + + log.Info("Pool is being deleted; cleaning up managed nodes") + + // List all BootcNodes owned by this pool. + allBootcNodes, err := r.listAllBootcNodes(ctx) + if err != nil { + return ctrl.Result{}, fmt.Errorf("listing BootcNodes for deletion cleanup: %w", err) + } + + for _, bn := range allBootcNodes { + if !metav1.IsControlledBy(bn, pool) { + continue + } + log.Info("Removing BootcNode for pool deletion", "node", bn.Name) + if err := r.removeBootcNode(ctx, bn); err != nil { + return ctrl.Result{}, fmt.Errorf("removing BootcNode %s during pool deletion: %w", bn.Name, err) + } + } + + // All nodes cleaned up — remove the finalizer to unblock deletion. + controllerutil.RemoveFinalizer(pool, bootcv1alpha1.FinalizerPoolCleanup) + if err := r.Update(ctx, pool); err != nil { + if apierrors.IsNotFound(err) { + // Pool was already fully deleted (e.g. race with another reconcile). + return ctrl.Result{}, nil + } + return ctrl.Result{}, fmt.Errorf("removing finalizer: %w", err) + } + return ctrl.Result{}, nil +} + // resolveTargetDigest resolves the target digest from the pool's image // ref. Digest refs are extracted directly. Tag refs are resolved via // the registry, respecting the re-resolution interval. diff --git a/internal/controller/membership_test.go b/internal/controller/membership_test.go index d97b7f4..be21109 100644 --- a/internal/controller/membership_test.go +++ b/internal/controller/membership_test.go @@ -203,6 +203,99 @@ func TestMembershipSyncsDesiredImage(t *testing.T) { )) } +// TestPoolDeletionRemovesManagedLabel verifies that when a BootcNodePool is +// deleted, the controller removes the bootc.dev/managed label from all member +// nodes and deletes all owned BootcNode objects. It also verifies that +// control-plane nodes belonging to a separate pool are unaffected. +func TestPoolDeletionRemovesManagedLabel(t *testing.T) { + g := NewWithT(t) + g.SetDefaultEventuallyTimeout(pollTimeout) + g.SetDefaultEventuallyPollingInterval(pollInterval) + ctx := context.Background() + + workerNodes := []*corev1.Node{ + testutil.NewK8sNode("del-worker-1", testutil.WorkerLabels()), + testutil.NewK8sNode("del-worker-2", testutil.WorkerLabels()), + } + + controlPlaneNodes := []*corev1.Node{ + testutil.NewK8sNode("del-control-plane-1", testutil.ControlPlaneLabels()), + testutil.NewK8sNode("del-control-plane-2", testutil.ControlPlaneLabels()), + } + + allNodes := append(workerNodes, controlPlaneNodes...) + + // Create nodes. + for _, node := range allNodes { + g.Expect(k8sClient.Create(ctx, node)).To(Succeed()) + t.Cleanup(func() { + _ = k8sClient.Delete(ctx, node) + }) + } + + // Create a worker pool and a separate control-plane pool. + workerPool := testutil.NewPool("del-workers", testImageDigestRefA, testutil.WithWorkerSelector()) + g.Expect(k8sClient.Create(ctx, workerPool)).To(Succeed()) + + cpPool := testutil.NewPool("del-control-plane", testImageDigestRefA, + testutil.WithNodeSelector(testutil.ControlPlaneLabels())) + g.Expect(k8sClient.Create(ctx, cpPool)).To(Succeed()) + t.Cleanup(func() { + _ = k8sClient.Delete(ctx, cpPool) + }) + + // Wait for BootcNodes to appear and managed labels to be applied on all nodes. + for _, node := range allNodes { + g.Eventually(func() error { + return k8sClient.Get(ctx, client.ObjectKey{Name: node.Name}, &bootcv1alpha1.BootcNode{}) + }).Should(Succeed(), "BootcNode %s should exist", node.Name) + + g.Eventually(func() (map[string]string, error) { + var n corev1.Node + err := k8sClient.Get(ctx, client.ObjectKey{Name: node.Name}, &n) + return n.Labels, err + }).Should(HaveKey(bootcv1alpha1.LabelManaged), "node %s should have managed label", node.Name) + } + + // Delete only the worker pool. + g.Expect(k8sClient.Delete(ctx, workerPool)).To(Succeed()) + + // The worker pool's finalizer must remove the managed label from worker nodes. + for _, node := range workerNodes { + g.Eventually(func() (map[string]string, error) { + var n corev1.Node + err := k8sClient.Get(ctx, client.ObjectKey{Name: node.Name}, &n) + return n.Labels, err + }).ShouldNot(HaveKey(bootcv1alpha1.LabelManaged), "worker node %s should not have managed label after pool deletion", node.Name) + } + + // Worker BootcNodes should be deleted. + for _, node := range workerNodes { + g.Eventually(func() error { + return k8sClient.Get(ctx, client.ObjectKey{Name: node.Name}, &bootcv1alpha1.BootcNode{}) + }).Should(MatchError(apierrors.IsNotFound, "IsNotFound"), "BootcNode %s should be deleted", node.Name) + } + + // The worker pool itself should be fully deleted (finalizer removed). + g.Eventually(func() error { + return k8sClient.Get(ctx, client.ObjectKeyFromObject(workerPool), &bootcv1alpha1.BootcNodePool{}) + }).Should(MatchError(apierrors.IsNotFound, "IsNotFound"), "worker pool should be fully deleted") + + // Control-plane nodes must still carry the managed label — their pool was not deleted. + for _, node := range controlPlaneNodes { + var n corev1.Node + g.Expect(k8sClient.Get(ctx, client.ObjectKey{Name: node.Name}, &n)).To(Succeed()) + g.Expect(n.Labels).To(HaveKey(bootcv1alpha1.LabelManaged), + "control-plane node %s should still have managed label", node.Name) + } + + // Control-plane BootcNodes must still exist. + for _, node := range controlPlaneNodes { + g.Expect(k8sClient.Get(ctx, client.ObjectKey{Name: node.Name}, &bootcv1alpha1.BootcNode{})).To(Succeed(), + "BootcNode %s should still exist", node.Name) + } +} + // TestMembershipConflictDetection verifies that when a node matches two // pools, the conflicting pool is marked Degraded with reason // NodeConflict for the contested node, but non-contested nodes in diff --git a/test/util/builders.go b/test/util/builders.go index 21d9c2e..40f286a 100644 --- a/test/util/builders.go +++ b/test/util/builders.go @@ -37,6 +37,11 @@ func WorkerLabels() map[string]string { return map[string]string{"node-role.kubernetes.io/worker": ""} } +// ControlPlaneLabels returns the conventional control-plane node label map. +func ControlPlaneLabels() map[string]string { + return map[string]string{"node-role.kubernetes.io/control-plane": ""} +} + // NewPool creates a BootcNodePool with the given name and image ref. // A nodeSelector must be provided via WithNodeSelector or // WithWorkerSelector. Override fields via functional options.