Improved workload finalization logic, added workload deployment finalizer.

Author: joshlreese

internal/controller/workload_controller.go

@@ -177,7 +177,6 @@ func (r *WorkloadReconciler) Reconcile(ctx context.Context, req mcreconcile.Requ
 		_, err := controllerutil.CreateOrUpdate(ctx, cl.GetClient(), deployment, func() error {
 			if deployment.CreationTimestamp.IsZero() {
 				logger.Info("creating deployment", "deployment_name", deployment.Name)
-				deployment.Finalizers = desiredDeployment.Finalizers
 				if err := controllerutil.SetControllerReference(&workload, deployment, cl.GetScheme()); err != nil {
 					return fmt.Errorf("failed to set controller on workload deployment: %w", err)
 				}
@@ -337,21 +336,9 @@ func (r *WorkloadReconciler) Finalize(ctx context.Context, obj client.Object) (f
 		if deployment.DeletionTimestamp.IsZero() {
 			// Deletion will result in another reconcile of the workload, where we
 			// will remove the finalizers.
-			if err := cl.GetClient().Delete(ctx, &deployment); err != nil {
-				if apierrors.IsNotFound(err) {
-					// List cache was not up to date
-					continue
-				}
+			if err := cl.GetClient().Delete(ctx, &deployment); client.IgnoreNotFound(err) != nil {
 				return finalizer.Result{}, fmt.Errorf("failed deleting workload deployment: %w", err)
 			}
-		} else {
-			// Remove the finalizer if it's still present
-			if controllerutil.RemoveFinalizer(&deployment, workloadControllerFinalizer) {
-				if err := cl.GetClient().Update(ctx, &deployment); err != nil {
-					return finalizer.Result{}, fmt.Errorf("failed removing finalizer from workload deployment: %w", err)
-				}
-			}
-
 		}
 	}
 
@@ -426,9 +413,6 @@ func (r *WorkloadReconciler) getDeploymentsForWorkload(
 					Labels: map[string]string{
 						computev1alpha.WorkloadUIDLabel: string(workload.UID),
 					},
-					Finalizers: []string{
-						workloadControllerFinalizer,
-					},
 				},
 				Spec: computev1alpha.WorkloadDeploymentSpec{
 					WorkloadRef: computev1alpha.WorkloadReference{

internal/controller/workloaddeployment_controller.go

@@ -4,17 +4,20 @@ package controller
 
 import (
 	"context"
+	"errors"
 	"fmt"
 	"slices"
 
 	apierrors "k8s.io/apimachinery/pkg/api/errors"
 	apimeta "k8s.io/apimachinery/pkg/api/meta"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/apimachinery/pkg/types"
+	kerrors "k8s.io/apimachinery/pkg/util/errors"
 	ctrl "sigs.k8s.io/controller-runtime"
 	"sigs.k8s.io/controller-runtime/pkg/client"
 	"sigs.k8s.io/controller-runtime/pkg/cluster"
 	"sigs.k8s.io/controller-runtime/pkg/controller/controllerutil"
+	"sigs.k8s.io/controller-runtime/pkg/finalizer"
 	"sigs.k8s.io/controller-runtime/pkg/handler"
 	"sigs.k8s.io/controller-runtime/pkg/log"
 	"sigs.k8s.io/controller-runtime/pkg/reconcile"
@@ -32,7 +35,8 @@ import (
 
 // WorkloadDeploymentReconciler reconciles a WorkloadDeployment object
 type WorkloadDeploymentReconciler struct {
-	mgr mcmanager.Manager
+	mgr        mcmanager.Manager
+	finalizers finalizer.Finalizers
 }
 
 // +kubebuilder:rbac:groups=compute.datumapis.com,resources=workloaddeployments,verbs=get;list;watch;create;update;patch;delete
@@ -57,6 +61,24 @@ func (r *WorkloadDeploymentReconciler) Reconcile(ctx context.Context, req mcreco
 		return ctrl.Result{}, err
 	}
 
+	finalizationResult, err := r.finalizers.Finalize(ctx, &deployment)
+	if err != nil {
+		if v, ok := err.(kerrors.Aggregate); ok && v.Is(errDeploymentHasInstances) {
+			// Don't produce an error in this case and let the watch on deployments
+			// result in another reconcile schedule.
+			logger.Info("deployment still has instances, waiting until removal")
+			return ctrl.Result{}, nil
+		} else {
+			return ctrl.Result{}, fmt.Errorf("failed to finalize: %w", err)
+		}
+	}
+	if finalizationResult.Updated {
+		if err = cl.GetClient().Update(ctx, &deployment); err != nil {
+			return ctrl.Result{}, fmt.Errorf("failed to update based on finalization result: %w", err)
+		}
+		return ctrl.Result{}, nil
+	}
+
 	if !deployment.DeletionTimestamp.IsZero() {
 		return ctrl.Result{}, nil
 	}
@@ -332,10 +354,58 @@ func (r *WorkloadDeploymentReconciler) reconcileNetworks(
 	return result
 }
 
+var errDeploymentHasInstances = errors.New("deployment has instances")
+
+func (r *WorkloadDeploymentReconciler) Finalize(ctx context.Context, obj client.Object) (finalizer.Result, error) {
+	clusterName, ok := mccontext.ClusterFrom(ctx)
+	if !ok {
+		return finalizer.Result{}, fmt.Errorf("cluster name not found in context")
+	}
+
+	cl, err := r.mgr.GetCluster(ctx, clusterName)
+	if err != nil {
+		return finalizer.Result{}, err
+	}
+
+	var instanceList computev1alpha.InstanceList
+	listOpts := []client.ListOption{
+		client.InNamespace(obj.GetNamespace()),
+		client.MatchingLabels{
+			computev1alpha.WorkloadDeploymentUIDLabel: string(obj.GetUID()),
+		},
+	}
+
+	if err := cl.GetClient().List(ctx, &instanceList, listOpts...); err != nil {
+		return finalizer.Result{}, fmt.Errorf("failed listing instances: %w", err)
+	}
+
+	if len(instanceList.Items) == 0 {
+		log.FromContext(ctx).Info("instances have been removed")
+		return finalizer.Result{}, nil
+	}
+
+	// All instances need to be deleted before the deployment may be deleted
+	for _, instance := range instanceList.Items {
+		if instance.DeletionTimestamp.IsZero() {
+			if err := cl.GetClient().Delete(ctx, &instance); client.IgnoreNotFound(err) != nil {
+				return finalizer.Result{}, fmt.Errorf("failed deleting instance: %w", err)
+			}
+		}
+	}
+
+	// Really don't like using errors for communication here. I think we'd need
+	// to move away from the finalizer helper to ensure we can wait on child
+	// resources to be gone before allowing the finalizer to be removed.
+	return finalizer.Result{}, errDeploymentHasInstances
+}
+
 // SetupWithManager sets up the controller with the Manager.
 func (r *WorkloadDeploymentReconciler) SetupWithManager(mgr mcmanager.Manager) error {
 	r.mgr = mgr
-	// TODO(jreese) finalizers
+	r.finalizers = finalizer.NewFinalizers()
+	if err := r.finalizers.Register(workloadControllerFinalizer, r); err != nil {
+		return fmt.Errorf("failed to register finalizer: %w", err)
+	}
 	return mcbuilder.ControllerManagedBy(mgr).
 		For(&computev1alpha.WorkloadDeployment{}, mcbuilder.WithEngageWithLocalCluster(false)).
 		Owns(&computev1alpha.Instance{}).