Remove the DaemonSet reconciler (#39)

Run the DaemonSet garbage collection in the Module reconciliation loop.

Author: qbarrand

.github/workflows/e2e.yaml

@@ -288,26 +288,13 @@ jobs:
           done
         timeout-minutes: 1
 
-      - name: Verify that the DaemonSet controller works
+      - name: Verify that the DaemonSet gets garbage collected
         run: |
-          set -eux
-
-          # Get the DaemonSet's name, as we cannot patch by label
-          DS_NAME=$(kubectl get daemonsets.apps -l oot.node.kubernetes.io/module.name=ooto-ci-b --template='{{ (index .items 0).metadata.name }}')
-
-          # Verify that we got the name
-          test -n "${DS_NAME}"
-
-          # Wait one minute for the DaemonSet to be considered old and deletable
-          sleep 60
-
-          # Patch the DaemonSet to trigger a reconciliation
-          kubectl patch daemonset.apps "${DS_NAME}" --patch '{"metadata":{"labels": {"trigger-reconcile":""}}}'
-
-          until ! kubectl get daemonsets.apps "${DS_NAME}"; do
+          # Cannot use kubectl wait because it will fail if there is no initial match
+          until [ $(kubectl get daemonsets.apps -l oot.node.kubernetes.io/module.name=ooto-ci-b -o go-template='{{ len .items }}') -eq 0 ]; do
             sleep 3
           done
-        timeout-minutes: 2
+        timeout-minutes: 1
 
       - name: Get all resources in the oot-operator-system namespace
         run: kubectl get all -n oot-operator-system

controllers/daemonset.go

@@ -11,13 +11,15 @@ import (
 	v1 "k8s.io/api/core/v1"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/apimachinery/pkg/runtime"
+	"k8s.io/apimachinery/pkg/util/sets"
 	"sigs.k8s.io/controller-runtime/pkg/client"
 	"sigs.k8s.io/controller-runtime/pkg/controller/controllerutil"
 )
 
 //go:generate mockgen -source=daemonset.go -package=controllers -destination=mock_daemonset.go
 
 type DaemonSetCreator interface {
+	GarbageCollect(ctx context.Context, existingDS map[string]*appsv1.DaemonSet, validKernels sets.String) ([]string, error)
 	ModuleDaemonSetsByKernelVersion(ctx context.Context, mod ootov1alpha1.Module) (map[string]*appsv1.DaemonSet, error)
 	SetAsDesired(ds *appsv1.DaemonSet, image string, mod ootov1alpha1.Module, kernelVersion string) error
 }
@@ -38,6 +40,22 @@ func NewDaemonSetCreator(client client.Client, kernelLabel, namespace string, sc
 	}
 }
 
+func (dc *daemonSetGenerator) GarbageCollect(ctx context.Context, existingDS map[string]*appsv1.DaemonSet, validKernels sets.String) ([]string, error) {
+	deleted := make([]string, 0)
+
+	for kernelVersion, ds := range existingDS {
+		if !validKernels.Has(kernelVersion) {
+			if err := dc.client.Delete(ctx, ds); err != nil {
+				return nil, fmt.Errorf("could not delete DaemonSet %s: %v", ds.Name, err)
+			}
+
+			deleted = append(deleted, ds.Name)
+		}
+	}
+
+	return deleted, nil
+}
+
 func (dc *daemonSetGenerator) ModuleDaemonSetsByKernelVersion(ctx context.Context, mod ootov1alpha1.Module) (map[string]*appsv1.DaemonSet, error) {
 	dsList := appsv1.DaemonSetList{}
 

controllers/daemonset_reconciler.go

@@ -12,6 +12,7 @@ import (
 	appsv1 "k8s.io/api/apps/v1"
 	v1 "k8s.io/api/core/v1"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
+	"k8s.io/apimachinery/pkg/util/sets"
 	"sigs.k8s.io/controller-runtime/pkg/client/fake"
 )
 
@@ -235,6 +236,68 @@ var _ = Describe("daemonSetGenerator", func() {
 		})
 	})
 
+	Describe("GarbageCollect", func() {
+		It("should only delete one of the two DaemonSets if only one is not used", func() {
+			const (
+				legitKernelVersion    = "legit-kernel-version"
+				legitName             = "legit"
+				notLegitKernelVersion = "not-legit-kernel-version"
+				notLegitName          = "not-legit"
+			)
+
+			dsLegit := appsv1.DaemonSet{
+				ObjectMeta: metav1.ObjectMeta{Name: legitName, Namespace: dsNamespace},
+			}
+
+			dsNotLegit := appsv1.DaemonSet{
+				ObjectMeta: metav1.ObjectMeta{Name: notLegitName, Namespace: dsNamespace},
+			}
+
+			client := fake.NewClientBuilder().WithObjects(&dsLegit, &dsNotLegit).Build()
+
+			dc := controllers.NewDaemonSetCreator(client, "", dsNamespace, scheme)
+
+			existingDS := map[string]*appsv1.DaemonSet{
+				legitKernelVersion:    &dsLegit,
+				notLegitKernelVersion: &dsNotLegit,
+			}
+
+			validKernels := sets.NewString(legitKernelVersion)
+
+			Expect(
+				dc.GarbageCollect(context.TODO(), existingDS, validKernels),
+			).To(
+				Equal([]string{notLegitName}),
+			)
+
+			afterDeleteDaemonSetList := appsv1.DaemonSetList{}
+
+			Expect(
+				client.List(context.TODO(), &afterDeleteDaemonSetList),
+			).To(
+				Succeed(),
+			)
+
+			Expect(afterDeleteDaemonSetList.Items).To(HaveLen(1))
+			Expect(afterDeleteDaemonSetList.Items[0].Name).To(Equal(legitName))
+		})
+
+		It("should return an error if a deletion failed", func() {
+			dc := controllers.NewDaemonSetCreator(
+				fake.NewClientBuilder().Build(),
+				"",
+				dsNamespace,
+				scheme)
+
+			existingDS := map[string]*appsv1.DaemonSet{
+				"some-kernel-version": {},
+			}
+
+			_, err := dc.GarbageCollect(context.TODO(), existingDS, sets.NewString())
+			Expect(err).To(HaveOccurred())
+		})
+	})
+
 	Describe("ModuleDaemonSetsByKernelVersion", func() {
 		It("should return an empty map if no DaemonSets are present", func() {
 			dc := controllers.NewDaemonSetCreator(

controllers/daemonset_reconciler_test.go

@@ -28,6 +28,7 @@ import (
 	batchv1 "k8s.io/api/batch/v1"
 	v1 "k8s.io/api/core/v1"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
+	"k8s.io/apimachinery/pkg/util/sets"
 	ctrl "sigs.k8s.io/controller-runtime"
 	"sigs.k8s.io/controller-runtime/pkg/builder"
 	"sigs.k8s.io/controller-runtime/pkg/client"
@@ -70,7 +71,7 @@ func NewModuleReconciler(
 //+kubebuilder:rbac:groups=ooto.sigs.k8s.io,resources=modules,verbs=get;list;watch;create;update;patch;delete
 //+kubebuilder:rbac:groups=ooto.sigs.k8s.io,resources=modules/status,verbs=get;update;patch
 //+kubebuilder:rbac:groups=ooto.sigs.k8s.io,resources=modules/finalizers,verbs=update
-//+kubebuilder:rbac:groups=apps,resources=daemonsets,verbs=create;get;list;patch;watch
+//+kubebuilder:rbac:groups=apps,resources=daemonsets,verbs=create;delete;get;list;patch;watch
 //+kubebuilder:rbac:groups="core",resources=nodes,verbs=get;list;watch
 //+kubebuilder:rbac:groups="batch",resources=jobs,verbs=create;list;watch
 
@@ -100,11 +101,6 @@ func (r *ModuleReconciler) Reconcile(ctx context.Context, req ctrl.Request) (ctr
 		return res, fmt.Errorf("could not list nodes: %v", err)
 	}
 
-	if len(nodes.Items) == 0 {
-		logger.Info("No nodes matching the selector; skipping module")
-		return res, nil
-	}
-
 	mappings := make(map[string]*ootov1alpha1.KernelMapping)
 
 	for _, node := range nodes.Items {
@@ -185,8 +181,17 @@ func (r *ModuleReconciler) Reconcile(ctx context.Context, req ctrl.Request) (ctr
 		logger.Info("Reconciled DaemonSet", "name", ds.Name, "result", opRes)
 	}
 
-	// TODO we need to garbage collect DaemonSets here.
-	// If we removed a kernel mapping from the Module, the corresponding DaemonSet is currently not deleted.
+	logger.Info("Garbage-collecting DaemonSets")
+
+	// Garbage collect old DaemonSets for which there are no nodes.
+	validKernels := sets.StringKeySet(mappings)
+
+	deleted, err := r.dc.GarbageCollect(ctx, dsByKernelVersion, validKernels)
+	if err != nil {
+		return res, fmt.Errorf("could not garbage collect DaemonSets: %v", err)
+	}
+
+	logger.Info("Garbage-collected DaemonSets", "names", deleted)
 
 	return res, nil
 }