controllers/finalize: Remove finalizer for child objects for non zero…

… DeletionTimestamp (#956)

* controllers/finalize: Remove finalizer for child objects for non zero DeletionTimestamp

Kubernetes performs soft delete and waits for object hard delete until finalizers == 0.
During that period any actions for soft deleted objects aren't performend. It caues weird behavior for service accounts, deployments and etc.
When kubernetes-controller manager ignores actions that must be performend with it. For instance, pod creation for soft deleted deployment.

 Operator now detects soft delete and free objects. An object ll be recreate at the next reconcile loop and error message ll be logged.

#953

* apply review comments

controllers/factory/alertmanager/statefulset.go

@@ -514,6 +514,9 @@ func createDefaultAMConfig(ctx context.Context, cr *victoriametricsv1beta1.VMAle
 		}
 		return err
 	}
+	if err := finalize.FreeIfNeeded(ctx, rclient, &existAMSecretConfig); err != nil {
+		return err
+	}
 
 	newAMSecretConfig.Annotations = labels.Merge(existAMSecretConfig.Annotations, newAMSecretConfig.Annotations)
 	newAMSecretConfig.Finalizers = victoriametricsv1beta1.MergeFinalizers(&existAMSecretConfig, victoriametricsv1beta1.FinalizerName)

controllers/factory/finalize/common.go

@@ -118,3 +118,15 @@ func removeConfigReloaderRole(ctx context.Context, rclient client.Client, crd cr
 	}
 	return nil
 }
+
+// FreeIfNeeded checks if resource must be freed from finalizer and garbage collected by kubernetes
+func FreeIfNeeded(ctx context.Context, rclient client.Client, object client.Object) error {
+	if object.GetDeletionTimestamp().IsZero() {
+		// fast path
+		return nil
+	}
+	if err := RemoveFinalizer(ctx, rclient, object); err != nil {
+		return fmt.Errorf("cannot remove finalizer from object=%s/%s, kind=%q: %w", object.GetNamespace(), object.GetName(), object.GetObjectKind().GroupVersionKind(), err)
+	}
+	return fmt.Errorf("deletionTimestamp is not zero=%q for object=%s/%s kind=%s, recreating it at next reconcile loop. Warning never delete object manually", object.GetDeletionTimestamp(), object.GetNamespace(), object.GetName(), object.GetObjectKind().GroupVersionKind())
+}

controllers/factory/reconcile/deploy.go

@@ -6,6 +6,7 @@ import (
 	"time"
 
 	victoriametricsv1beta1 "github.com/VictoriaMetrics/operator/api/v1beta1"
+	"github.com/VictoriaMetrics/operator/controllers/factory/finalize"
 	appsv1 "k8s.io/api/apps/v1"
 	v1 "k8s.io/api/core/v1"
 	"k8s.io/apimachinery/pkg/api/errors"
@@ -30,6 +31,9 @@ func Deployment(ctx context.Context, rclient client.Client, newDeploy *appsv1.De
 			}
 			return fmt.Errorf("cannot get deployment for app: %s err: %w", newDeploy.Name, err)
 		}
+		if err := finalize.FreeIfNeeded(ctx, rclient, &currentDeploy); err != nil {
+			return err
+		}
 		if hasHPA {
 			newDeploy.Spec.Replicas = currentDeploy.Spec.Replicas
 		}

controllers/factory/reconcile/hpa.go

@@ -4,6 +4,7 @@ import (
 	"context"
 	"fmt"
 
+	"github.com/VictoriaMetrics/operator/controllers/factory/finalize"
 	"github.com/VictoriaMetrics/operator/controllers/factory/k8stools"
 
 	victoriametricsv1beta1 "github.com/VictoriaMetrics/operator/api/v1beta1"
@@ -24,6 +25,9 @@ func HPA(ctx context.Context, rclient client.Client, targetHPA client.Object) er
 			}
 			return fmt.Errorf("cannot get exist hpa object: %w", err)
 		}
+		if err := finalize.FreeIfNeeded(ctx, rclient, existHPA); err != nil {
+			return err
+		}
 		targetHPA.SetResourceVersion(existHPA.GetResourceVersion())
 		victoriametricsv1beta1.MergeFinalizers(targetHPA, victoriametricsv1beta1.FinalizerName)
 		targetHPA.SetAnnotations(labels.Merge(existHPA.GetAnnotations(), targetHPA.GetAnnotations()))

controllers/factory/reconcile/pdb.go

@@ -5,6 +5,7 @@ import (
 	"fmt"
 
 	victoriametricsv1beta1 "github.com/VictoriaMetrics/operator/api/v1beta1"
+	"github.com/VictoriaMetrics/operator/controllers/factory/finalize"
 	"github.com/VictoriaMetrics/operator/controllers/factory/logger"
 	policyv1 "k8s.io/api/policy/v1"
 	"k8s.io/apimachinery/pkg/api/errors"
@@ -26,6 +27,9 @@ func PDB(ctx context.Context, rclient client.Client, pdb *policyv1.PodDisruption
 			}
 			return fmt.Errorf("cannot get existing pdb: %s, err: %w", pdb.Name, err)
 		}
+		if err := finalize.FreeIfNeeded(ctx, rclient, currentPdb); err != nil {
+			return err
+		}
 		pdb.Annotations = labels.Merge(currentPdb.Annotations, pdb.Annotations)
 		if currentPdb.ResourceVersion != "" {
 			pdb.ResourceVersion = currentPdb.ResourceVersion

controllers/factory/reconcile/service.go

@@ -48,6 +48,9 @@ func reconcileService(ctx context.Context, rclient client.Client, newService *v1
 		}
 		return fmt.Errorf("cannot get service for existing service: %w", err)
 	}
+	if err := finalize.FreeIfNeeded(ctx, rclient, existingService); err != nil {
+		return err
+	}
 	// lets save annotations and labels even after recreation.
 	if newService.Spec.Type != existingService.Spec.Type {
 		// type mismatch.

controllers/factory/reconcile/service_account.go

@@ -5,6 +5,7 @@ import (
 	"fmt"
 
 	victoriametricsv1beta1 "github.com/VictoriaMetrics/operator/api/v1beta1"
+	"github.com/VictoriaMetrics/operator/controllers/factory/finalize"
 	v1 "k8s.io/api/core/v1"
 	"k8s.io/apimachinery/pkg/api/errors"
 	"k8s.io/apimachinery/pkg/labels"
@@ -23,6 +24,9 @@ func ServiceAccount(ctx context.Context, rclient client.Client, sa *v1.ServiceAc
 			}
 			return fmt.Errorf("cannot get ServiceAccount for given CRD Object=%q, err=%w", sa.Name, err)
 		}
+		if err := finalize.FreeIfNeeded(ctx, rclient, &existSA); err != nil {
+			return err
+		}
 
 		existSA.OwnerReferences = sa.OwnerReferences
 		existSA.Finalizers = victoriametricsv1beta1.MergeFinalizers(&existSA, victoriametricsv1beta1.FinalizerName)

controllers/factory/reconcile/statefulset.go

@@ -9,6 +9,7 @@ import (
 	"time"
 
 	victoriametricsv1beta1 "github.com/VictoriaMetrics/operator/api/v1beta1"
+	"github.com/VictoriaMetrics/operator/controllers/factory/finalize"
 	"github.com/VictoriaMetrics/operator/controllers/factory/logger"
 	"github.com/VictoriaMetrics/operator/internal/config"
 	appsv1 "k8s.io/api/apps/v1"
@@ -69,6 +70,9 @@ func HandleSTSUpdate(ctx context.Context, rclient client.Client, cr STSOptions,
 			}
 			return fmt.Errorf("cannot get sts %s under namespace %s: %w", newSts.Name, newSts.Namespace, err)
 		}
+		if err := finalize.FreeIfNeeded(ctx, rclient, &currentSts); err != nil {
+			return err
+		}
 		// will update the original cr replicaCount to propagate right num,
 		// for now, it's only used in vmselect
 		if cr.UpdateReplicaCount != nil {

controllers/factory/reconcile/vmservicescrape.go

@@ -4,6 +4,7 @@ import (
 	"context"
 
 	victoriametricsv1beta1 "github.com/VictoriaMetrics/operator/api/v1beta1"
+	"github.com/VictoriaMetrics/operator/controllers/factory/finalize"
 	"k8s.io/apimachinery/pkg/api/equality"
 	"k8s.io/apimachinery/pkg/api/errors"
 	"k8s.io/apimachinery/pkg/types"
@@ -22,6 +23,9 @@ func VMServiceScrapeForCRD(ctx context.Context, rclient client.Client, vss *vict
 			}
 			return err
 		}
+		if err := finalize.FreeIfNeeded(ctx, rclient, &existVSS); err != nil {
+			return err
+		}
 		updateIsNeeded := !equality.Semantic.DeepEqual(vss.Spec, existVSS.Spec) || !equality.Semantic.DeepEqual(vss.Labels, existVSS.Labels) || !equality.Semantic.DeepEqual(vss.Annotations, existVSS.Annotations)
 		existVSS.Spec = vss.Spec
 		existVSS.Labels = vss.Labels

controllers/factory/vmagent/rbac.go

@@ -5,6 +5,7 @@ import (
 	"fmt"
 
 	v1beta12 "github.com/VictoriaMetrics/operator/api/v1beta1"
+	"github.com/VictoriaMetrics/operator/controllers/factory/finalize"
 	rbacV1 "k8s.io/api/rbac/v1"
 	"k8s.io/apimachinery/pkg/api/errors"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
@@ -143,6 +144,9 @@ func ensureVMAgentCRExist(ctx context.Context, cr *v1beta12.VMAgent, rclient cli
 		}
 		return fmt.Errorf("cannot get exist cluster role for vmagent: %w", err)
 	}
+	if err := finalize.FreeIfNeeded(ctx, rclient, &existsClusterRole); err != nil {
+		return err
+	}
 
 	existsClusterRole.OwnerReferences = clusterRole.OwnerReferences
 	existsClusterRole.Labels = clusterRole.Labels
@@ -162,6 +166,9 @@ func ensureVMAgentCRBExist(ctx context.Context, cr *v1beta12.VMAgent, rclient cl
 		}
 		return fmt.Errorf("cannot get clusterRoleBinding for vmagent: %w", err)
 	}
+	if err := finalize.FreeIfNeeded(ctx, rclient, &existsClusterRoleBinding); err != nil {
+		return err
+	}
 
 	existsClusterRoleBinding.OwnerReferences = clusterRoleBinding.OwnerReferences
 	existsClusterRoleBinding.Labels = clusterRoleBinding.Labels
@@ -221,6 +228,9 @@ func ensureVMAgentRExist(ctx context.Context, cr *v1beta12.VMAgent, rclient clie
 		}
 		return fmt.Errorf("cannot get exist cluster role for vmagent: %w", err)
 	}
+	if err := finalize.FreeIfNeeded(ctx, rclient, &existsClusterRole); err != nil {
+		return err
+	}
 
 	existsClusterRole.OwnerReferences = nr.OwnerReferences
 	existsClusterRole.Labels = nr.Labels
@@ -240,6 +250,9 @@ func ensureVMAgentRBExist(ctx context.Context, cr *v1beta12.VMAgent, rclient cli
 		}
 		return fmt.Errorf("cannot get rb for vmagent: %w", err)
 	}
+	if err := finalize.FreeIfNeeded(ctx, rclient, &existsRB); err != nil {
+		return err
+	}
 
 	existsRB.OwnerReferences = rb.OwnerReferences
 	existsRB.Labels = rb.Labels

controllers/factory/vmagent/vmagent.go

@@ -791,6 +791,9 @@ func createOrUpdateTLSAssets(ctx context.Context, cr *victoriametricsv1beta1.VMA
 		}
 		return fmt.Errorf("cannot get existing tls secret: %s, for vmagent: %s, err: %w", tlsAssetsSecret.Name, cr.Name, err)
 	}
+	if err := finalize.FreeIfNeeded(ctx, rclient, currentAssetSecret); err != nil {
+		return err
+	}
 	tlsAssetsSecret.Annotations = labels.Merge(currentAssetSecret.Annotations, tlsAssetsSecret.Annotations)
 	victoriametricsv1beta1.MergeFinalizers(tlsAssetsSecret, victoriametricsv1beta1.FinalizerName)
 	return rclient.Update(ctx, tlsAssetsSecret)

controllers/factory/vmagent/vmagent_scrapeconfig.go

@@ -15,6 +15,7 @@ import (
 	"github.com/VictoriaMetrics/VictoriaMetrics/app/vmalert/utils"
 	"github.com/VictoriaMetrics/metricsql"
 	victoriametricsv1beta1 "github.com/VictoriaMetrics/operator/api/v1beta1"
+	"github.com/VictoriaMetrics/operator/controllers/factory/finalize"
 	"github.com/VictoriaMetrics/operator/controllers/factory/k8stools"
 	"github.com/VictoriaMetrics/operator/controllers/factory/logger"
 	"github.com/VictoriaMetrics/operator/internal/config"
@@ -154,6 +155,10 @@ func createOrUpdateConfigurationSecret(ctx context.Context, cr *victoriametricsv
 		return nil, fmt.Errorf("cannot get secret for vmagent: %q : %w", cr.Name, err)
 	}
 
+	if err := finalize.FreeIfNeeded(ctx, rclient, curSecret); err != nil {
+		return nil, err
+	}
+
 	s.Annotations = labels.Merge(curSecret.Annotations, s.Annotations)
 	s.Finalizers = victoriametricsv1beta1.MergeFinalizers(curSecret, victoriametricsv1beta1.FinalizerName)
 	return ssCache, rclient.Update(ctx, s)

controllers/factory/vmalert/rules.go

@@ -10,6 +10,7 @@ import (
 	"strings"
 
 	victoriametricsv1beta1 "github.com/VictoriaMetrics/operator/api/v1beta1"
+	"github.com/VictoriaMetrics/operator/controllers/factory/finalize"
 	"github.com/VictoriaMetrics/operator/controllers/factory/k8stools"
 	"github.com/VictoriaMetrics/operator/controllers/factory/logger"
 	"github.com/ghodss/yaml"
@@ -130,20 +131,20 @@ func CreateOrUpdateRuleConfigMaps(ctx context.Context, cr *victoriametricsv1beta
 		}
 	}
 	for _, cm := range toUpdate {
+		if err := finalize.FreeIfNeeded(ctx, rclient, &cm); err != nil {
+			return nil, err
+		}
 		err = rclient.Update(ctx, &cm)
 		if err != nil {
 			return nil, fmt.Errorf("failed to update rules Configmap: %s, err: %w", cm.Name, err)
 		}
 	}
 	for _, cm := range toDelete {
-		if victoriametricsv1beta1.IsContainsFinalizer(cm.Finalizers, victoriametricsv1beta1.FinalizerName) {
-			cm.Finalizers = victoriametricsv1beta1.RemoveFinalizer(cm.Finalizers, victoriametricsv1beta1.FinalizerName)
-			if err := rclient.Update(ctx, &cm); err != nil {
-				return nil, fmt.Errorf("cannot remove finalizer from configmap: %s, err: %w", cm.Name, err)
-			}
+		if err := finalize.RemoveFinalizer(ctx, rclient, &cm); err != nil {
+			return nil, fmt.Errorf("cannot remove finalizer for vmalert cm: %w", err)
 		}
-		err = rclient.Delete(ctx, &cm)
-		if err != nil {
+
+		if err := finalize.SafeDelete(ctx, rclient, &cm); err != nil {
 			return nil, fmt.Errorf("failed to delete rules Configmap: %s, err: %w", cm.Name, err)
 		}
 	}

controllers/factory/vmalert/vmalert.go

@@ -128,6 +128,9 @@ func createOrUpdateVMAlertSecret(ctx context.Context, rclient client.Client, cr
 		}
 		return nil
 	}
+	if err := finalize.FreeIfNeeded(ctx, rclient, curSecret); err != nil {
+		return err
+	}
 	s.Annotations = labels.Merge(curSecret.Annotations, s.Annotations)
 	return rclient.Update(ctx, s)
 }
@@ -739,6 +742,9 @@ func createOrUpdateTLSAssetsForVMAlert(ctx context.Context, cr *victoriametricsv
 		}
 		return fmt.Errorf("cannot get existing tls secret: %s, for vmalert: %s, err: %w", tlsAssetsSecret.Name, cr.Name, err)
 	}
+	if err := finalize.FreeIfNeeded(ctx, rclient, currentAssetSecret); err != nil {
+		return err
+	}
 	for annotation, value := range currentAssetSecret.Annotations {
 		tlsAssetsSecret.Annotations[annotation] = value
 	}

controllers/factory/vmauth/rbac.go

@@ -5,6 +5,7 @@ import (
 	"fmt"
 
 	v1beta12 "github.com/VictoriaMetrics/operator/api/v1beta1"
+	"github.com/VictoriaMetrics/operator/controllers/factory/finalize"
 	v1 "k8s.io/api/rbac/v1"
 	"k8s.io/apimachinery/pkg/api/errors"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
@@ -33,6 +34,9 @@ func ensureVMAuthRoleExist(ctx context.Context, cr *v1beta12.VMAuth, rclient cli
 		}
 		return fmt.Errorf("cannot get role for vmauth: %w", err)
 	}
+	if err := finalize.FreeIfNeeded(ctx, rclient, &existRole); err != nil {
+		return err
+	}
 
 	existRole.OwnerReferences = role.OwnerReferences
 	existRole.Labels = role.Labels
@@ -51,6 +55,9 @@ func ensureVMAgentRBExist(ctx context.Context, cr *v1beta12.VMAuth, rclient clie
 		}
 		return fmt.Errorf("cannot get rolebinding for vmauth: %w", err)
 	}
+	if err := finalize.FreeIfNeeded(ctx, rclient, &existRoleBinding); err != nil {
+		return err
+	}
 
 	existRoleBinding.OwnerReferences = roleBinding.OwnerReferences
 	existRoleBinding.Labels = roleBinding.Labels

controllers/factory/vmauth/vmauth.go

@@ -357,6 +357,9 @@ func CreateOrUpdateVMAuthConfig(ctx context.Context, rclient client.Client, cr *
 		}
 		return err
 	}
+	if err := finalize.FreeIfNeeded(ctx, rclient, &curSecret); err != nil {
+		return err
+	}
 	var (
 		generatedConf             = s.Data[vmAuthConfigNameGz]
 		curConfig, curConfigFound = curSecret.Data[vmAuthConfigNameGz]
@@ -414,6 +417,9 @@ func CreateOrUpdateVMAuthIngress(ctx context.Context, rclient client.Client, cr
 		}
 		return err
 	}
+	if err := finalize.FreeIfNeeded(ctx, rclient, &existIngress); err != nil {
+		return err
+	}
 	newIngress.Annotations = labels.Merge(existIngress.Annotations, newIngress.Annotations)
 	newIngress.Finalizers = victoriametricsv1beta1.MergeFinalizers(&existIngress, victoriametricsv1beta1.FinalizerName)
 	return rclient.Update(ctx, newIngress)

controllers/factory/vmsingle/vmsingle.go

@@ -42,7 +42,9 @@ func CreateVMSingleStorage(ctx context.Context, cr *victoriametricsv1beta1.VMSin
 			if err := rclient.Create(ctx, newPvc); err != nil {
 				return fmt.Errorf("cannot create new pvc for vmsingle: %w", err)
 			}
-
+			if !existPvc.DeletionTimestamp.IsZero() {
+				l.Info("exist pvc for vmsingle has non zero DeletionTimestamp. Ignore it or make backup, delete VMSingle object and restore it from backup.")
+			}
 			return nil
 		}
 		return fmt.Errorf("cannot get existing pvc for vmsingle: %w", err)
@@ -447,6 +449,10 @@ func CreateOrUpdateVMSingleStreamAggrConfig(ctx context.Context, cr *victoriamet
 		if errors.IsNotFound(err) {
 			return rclient.Create(ctx, streamAggrCM)
 		}
+		return fmt.Errorf("cannot fetch exist configmap for vmsingle streamAggr: %w", err)
+	}
+	if err := finalize.FreeIfNeeded(ctx, rclient, &existCM); err != nil {
+		return err
 	}
 	streamAggrCM.Annotations = labels.Merge(existCM.Annotations, streamAggrCM.Annotations)
 	victoriametricsv1beta1.MergeFinalizers(streamAggrCM, victoriametricsv1beta1.FinalizerName)

docs/CHANGELOG.md

@@ -17,6 +17,7 @@ aliases:
 ## Next release
 
 - [vmalertmanager](./api.md#vmalertmanager): ignores content of `cr.spec.configSecret` if it's name clashes with secret used by operator for storing alertmanager config. See this [issue](https://github.com/VictoriaMetrics/operator/issues/954) for details.
+- [operator](./README.md): remove finalizer for child objects with non-empty `DeletetionTimestamp`.  See this [issue](https://github.com/VictoriaMetrics/operator/issues/953) for details.
 - [operator](./README.md): skip storageClass check if there is no PVC size change. See this [issue](https://github.com/VictoriaMetrics/operator/issues/957) for details.
 
 ## [v0.44.0](https://github.com/VictoriaMetrics/operator/releases/tag/v0.44.0) - 9 May 2024