controllers: properly perform objects delete operation

Previously, it was not possible to detect should additional Object must be deleted.
Such as `HPA` or `PodDisruptionBudget`, because operator didn't have enough context for that.
If current object spec is nil, it was not possible to check it's state before changes were applied.

 This commit, adds a check for previous Spec state and performs delete operations based on it.
It allows to keep unmanaged by operator resources with the same names. And it should reduce amount of DELETE api requests.

 Related issues:
#758
#817

Signed-off-by: f41gh7 <[email protected]>

internal/controller/operator/controllers.go

@@ -290,6 +290,7 @@ func reconcileAndTrackStatus(ctx context.Context, c client.Client, object object
 		if err := createGenericEventForObject(ctx, c, object, "starting object update"); err != nil {
 			logger.WithContext(ctx).Error(err, " cannot create k8s api event")
 		}
+		logger.WithContext(ctx).Info("object has changes with previous state, applying changes")
 	}
 
 	result, err = cb()
@@ -316,6 +317,8 @@ func reconcileAndTrackStatus(ctx context.Context, c client.Client, object object
 		if err := createGenericEventForObject(ctx, c, object, "reconcile of object finished successfully"); err != nil {
 			logger.WithContext(ctx).Error(err, " cannot create k8s api event")
 		}
+		logger.WithContext(ctx).Info("object was successfully reconciled")
+
 	}
 
 	return result, nil

internal/controller/operator/factory/alertmanager/alertmanager.go

@@ -6,9 +6,13 @@ import (
 
 	vmv1beta1 "github.com/VictoriaMetrics/operator/api/operator/v1beta1"
 	"github.com/VictoriaMetrics/operator/internal/controller/operator/factory/build"
+	"github.com/VictoriaMetrics/operator/internal/controller/operator/factory/finalize"
 	"github.com/VictoriaMetrics/operator/internal/controller/operator/factory/reconcile"
+
 	"github.com/prometheus/client_golang/prometheus"
 	appsv1 "k8s.io/api/apps/v1"
+	policyv1 "k8s.io/api/policy/v1"
+	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/utils/ptr"
 	"sigs.k8s.io/controller-runtime/pkg/client"
 	"sigs.k8s.io/controller-runtime/pkg/metrics"
@@ -30,6 +34,9 @@ func init() {
 
 // CreateOrUpdateAlertManager creates alertmanagerand and bulds config for it
 func CreateOrUpdateAlertManager(ctx context.Context, cr *vmv1beta1.VMAlertmanager, rclient client.Client) error {
+	if err := deletePrevStateResources(ctx, cr, rclient); err != nil {
+		return fmt.Errorf("cannot delete objects from prev state: %w", err)
+	}
 	if cr.IsOwnsServiceAccount() {
 		if err := reconcile.ServiceAccount(ctx, rclient, build.ServiceAccount(cr)); err != nil {
 			return fmt.Errorf("failed create service account: %w", err)
@@ -41,8 +48,18 @@ func CreateOrUpdateAlertManager(ctx context.Context, cr *vmv1beta1.VMAlertmanage
 		}
 	}
 
+	service, err := createOrUpdateAlertManagerService(ctx, cr, rclient)
+	if err != nil {
+		return err
+	}
+	if !ptr.Deref(cr.Spec.DisableSelfServiceScrape, false) {
+		err := reconcile.VMServiceScrapeForCRD(ctx, rclient, build.VMServiceScrapeForAlertmanager(service, cr))
+		if err != nil {
+			return err
+		}
+	}
+
 	if cr.Spec.PodDisruptionBudget != nil {
-		// TODO verify lastSpec for missing PDB and detete it if needed
 		if err := reconcile.PDB(ctx, rclient, build.PodDisruptionBudget(cr, cr.Spec.PodDisruptionBudget)); err != nil {
 			return err
 		}
@@ -69,3 +86,27 @@ func CreateOrUpdateAlertManager(ctx context.Context, cr *vmv1beta1.VMAlertmanage
 	}
 	return reconcile.HandleSTSUpdate(ctx, rclient, stsOpts, newSts, prevSts)
 }
+
+func deletePrevStateResources(ctx context.Context, cr *vmv1beta1.VMAlertmanager, rclient client.Client) error {
+	if cr.Spec.ParsedLastAppliedSpec == nil {
+		return nil
+	}
+	prevSvc, currSvc := cr.Spec.ParsedLastAppliedSpec.ServiceSpec, cr.Spec.ServiceSpec
+	if err := reconcile.AdditionalServices(ctx, rclient, cr.PrefixedName(), cr.Namespace, prevSvc, currSvc); err != nil {
+		return fmt.Errorf("cannot remove additional service: %w", err)
+	}
+
+	objMeta := metav1.ObjectMeta{Name: cr.PrefixedName(), Namespace: cr.Namespace}
+	if cr.Spec.PodDisruptionBudget == nil && cr.Spec.ParsedLastAppliedSpec.PodDisruptionBudget != nil {
+		if err := finalize.SafeDeleteWithFinalizer(ctx, rclient, &policyv1.PodDisruptionBudget{ObjectMeta: objMeta}); err != nil {
+			return fmt.Errorf("cannot delete PDB from prev state: %w", err)
+		}
+	}
+	if ptr.Deref(cr.Spec.DisableSelfServiceScrape, false) && !ptr.Deref(cr.Spec.ParsedLastAppliedSpec.DisableSelfServiceScrape, false) {
+		if err := finalize.SafeDeleteWithFinalizer(ctx, rclient, &vmv1beta1.VMServiceScrape{ObjectMeta: objMeta}); err != nil {
+			return fmt.Errorf("cannot remove serviceScrape: %w", err)
+		}
+	}
+
+	return nil
+}

internal/controller/operator/factory/alertmanager/statefulset.go

@@ -12,7 +12,6 @@ import (
 
 	vmv1beta1 "github.com/VictoriaMetrics/operator/api/operator/v1beta1"
 	"github.com/VictoriaMetrics/operator/internal/controller/operator/factory/build"
-	"github.com/VictoriaMetrics/operator/internal/controller/operator/factory/finalize"
 	"github.com/VictoriaMetrics/operator/internal/controller/operator/factory/k8stools"
 	"github.com/VictoriaMetrics/operator/internal/controller/operator/factory/logger"
 	"github.com/VictoriaMetrics/operator/internal/controller/operator/factory/reconcile"
@@ -77,8 +76,7 @@ func newStsForAlertManager(cr *vmv1beta1.VMAlertmanager) (*appsv1.StatefulSet, e
 	return statefulset, nil
 }
 
-// CreateOrUpdateAlertManagerService creates service for alertmanager
-func CreateOrUpdateAlertManagerService(ctx context.Context, cr *vmv1beta1.VMAlertmanager, rclient client.Client) (*corev1.Service, error) {
+func createOrUpdateAlertManagerService(ctx context.Context, cr *vmv1beta1.VMAlertmanager, rclient client.Client) (*corev1.Service, error) {
 	port, err := strconv.ParseInt(cr.Port(), 10, 32)
 	if err != nil {
 		return nil, fmt.Errorf("cannot reconcile additional service for vmalertmanager: failed to parse port: %w", err)
@@ -140,11 +138,6 @@ func CreateOrUpdateAlertManagerService(ctx context.Context, cr *vmv1beta1.VMAler
 	}); err != nil {
 		return nil, err
 	}
-
-	rca := finalize.RemoveSvcArgs{SelectorLabels: cr.SelectorLabels, GetNameSpace: cr.GetNamespace, PrefixedName: cr.PrefixedName}
-	if err := finalize.RemoveOrphanedServices(ctx, rclient, rca, cr.Spec.ServiceSpec); err != nil {
-		return nil, err
-	}
 	if err := reconcile.Service(ctx, rclient, newService, prevService); err != nil {
 		return nil, fmt.Errorf("cannot reconcile service for vmalertmanager: %w", err)
 	}

internal/controller/operator/factory/finalize/common.go

@@ -87,6 +87,36 @@ func SafeDelete(ctx context.Context, rclient client.Client, r client.Object) err
 	return nil
 }
 
+// SafeDeleteWithFinalizer removes object, ignores notfound error.
+func SafeDeleteWithFinalizer(ctx context.Context, rclient client.Client, r client.Object) error {
+	objName, objNs := r.GetName(), r.GetNamespace()
+	if objName == "" || objNs == "" {
+		return fmt.Errorf("BUG: object name=%q or object namespace=%q cannot be empty", objName, objNs)
+	}
+	// reload object from API to properly remove finalizer
+	if err := rclient.Get(ctx, types.NamespacedName{
+		Namespace: objNs,
+		Name:      objName,
+	}, r); err != nil {
+		// fast path
+		if errors.IsNotFound(err) {
+			return nil
+		}
+		return err
+	}
+	if err := RemoveFinalizer(ctx, rclient, r); err != nil {
+		if !errors.IsNotFound(err) {
+			return err
+		}
+	}
+	if err := rclient.Delete(ctx, r); err != nil {
+		if !errors.IsNotFound(err) {
+			return err
+		}
+	}
+	return nil
+}
+
 func deleteSA(ctx context.Context, rclient client.Client, crd crdObject) error {
 	if !crd.IsOwnsServiceAccount() {
 		return nil
@@ -101,10 +131,6 @@ func finalizePBD(ctx context.Context, rclient client.Client, crd crdObject) erro
 	return removeFinalizeObjByName(ctx, rclient, &policyv1.PodDisruptionBudget{}, crd.PrefixedName(), crd.GetNSName())
 }
 
-func finalizePBDWithName(ctx context.Context, rclient client.Client, name, ns string) error {
-	return removeFinalizeObjByName(ctx, rclient, &policyv1.PodDisruptionBudget{}, name, ns)
-}
-
 func removeConfigReloaderRole(ctx context.Context, rclient client.Client, crd crdObject) error {
 	if err := removeFinalizeObjByName(ctx, rclient, &rbacv1.RoleBinding{}, crd.PrefixedName(), crd.GetNSName()); err != nil {
 		return err

internal/controller/operator/factory/finalize/orphaned.go

@@ -3,9 +3,6 @@ package finalize
 import (
 	"context"
 
-	vmv1beta1 "github.com/VictoriaMetrics/operator/api/operator/v1beta1"
-	corev1 "k8s.io/api/core/v1"
-
 	appsv1 "k8s.io/api/apps/v1"
 	"k8s.io/apimachinery/pkg/labels"
 	"sigs.k8s.io/controller-runtime/pkg/client"
@@ -61,64 +58,6 @@ type RemoveSvcArgs struct {
 	GetNameSpace   func() string
 }
 
-// RemoveOrphanedServices removes services that no longer belongs to given crd by its args.
-func RemoveOrphanedServices(ctx context.Context, rclient client.Client, args RemoveSvcArgs, spec *vmv1beta1.AdditionalServiceSpec) error {
-	svcsToRemove, err := discoverServicesByLabels(ctx, rclient, args)
-	if err != nil {
-		return err
-	}
-	handleDelete := func(s *corev1.Service) error {
-		if err := RemoveFinalizer(ctx, rclient, s); err != nil {
-			return err
-		}
-		return SafeDelete(ctx, rclient, s)
-	}
-
-	var additionalSvcName string
-	if spec != nil && !spec.UseAsDefault {
-		additionalSvcName = spec.NameOrDefault(args.PrefixedName())
-	}
-	cnt := 0
-	// filter in-place,
-	// keep services that doesn't match prefixedName and additional serviceName.
-	for i := range svcsToRemove {
-		svc := svcsToRemove[i]
-		switch svc.Name {
-		case args.PrefixedName():
-		case additionalSvcName:
-		default:
-			// service must be removed
-			svcsToRemove[cnt] = svc
-			cnt++
-		}
-	}
-	// remove left services.
-	svcsToRemove = svcsToRemove[:cnt]
-	for i := range svcsToRemove {
-		if err := handleDelete(svcsToRemove[i]); err != nil {
-			return err
-		}
-	}
-	return nil
-}
-
-// discoverServicesByLabels - returns services with given args.
-func discoverServicesByLabels(ctx context.Context, rclient client.Client, args RemoveSvcArgs) ([]*corev1.Service, error) {
-	var svcs corev1.ServiceList
-	opts := client.ListOptions{
-		Namespace:     args.GetNameSpace(),
-		LabelSelector: labels.SelectorFromSet(args.SelectorLabels()),
-	}
-	if err := rclient.List(ctx, &svcs, &opts); err != nil {
-		return nil, err
-	}
-	resp := make([]*corev1.Service, 0, len(svcs.Items))
-	for i := range svcs.Items {
-		resp = append(resp, &svcs.Items[i])
-	}
-	return resp, nil
-}
-
 // RemoveOrphanedSTSs removes deployments detached from given object
 func RemoveOrphanedSTSs(ctx context.Context, rclient client.Client, cr orphanedCRD, keepSTSNames map[string]struct{}) error {
 	deployToRemove, err := discoverSTSsByLabels(ctx, rclient, cr.GetNSName(), cr.SelectorLabels())

internal/controller/operator/factory/finalize/orphaned_test.go

@@ -10,93 +10,10 @@ import (
 
 	vmv1beta1 "github.com/VictoriaMetrics/operator/api/operator/v1beta1"
 	"github.com/VictoriaMetrics/operator/internal/controller/operator/factory/k8stools"
-	corev1 "k8s.io/api/core/v1"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/apimachinery/pkg/runtime"
 )
 
-func Test_reconcileMissingServices(t *testing.T) {
-	type args struct {
-		ctx  context.Context
-		args RemoveSvcArgs
-		spec *vmv1beta1.AdditionalServiceSpec
-	}
-	tests := []struct {
-		name              string
-		args              args
-		wantErr           bool
-		wantServiceNames  map[string]struct{}
-		predefinedObjects []runtime.Object
-	}{
-		{
-			name: "remove 1 missing",
-			args: args{
-				args: RemoveSvcArgs{
-					SelectorLabels: func() map[string]string {
-						return map[string]string{
-							"selector": "app-1",
-						}
-					},
-					PrefixedName: func() string {
-						return "keep-this-one"
-					},
-					GetNameSpace: func() string {
-						return "default"
-					},
-				},
-				ctx: context.TODO(),
-				spec: &vmv1beta1.AdditionalServiceSpec{
-					EmbeddedObjectMetadata: vmv1beta1.EmbeddedObjectMetadata{
-						Name: "keep-another-one",
-					},
-				},
-			},
-			wantServiceNames: map[string]struct{}{
-				"keep-another-one": {},
-				"keep-this-one":    {},
-			},
-			predefinedObjects: []runtime.Object{
-				&corev1.Service{
-					ObjectMeta: metav1.ObjectMeta{
-						Namespace: "default",
-						Name:      "keep-another-one",
-						Labels:    map[string]string{"selector": "app-1"},
-					},
-				},
-				&corev1.Service{
-					ObjectMeta: metav1.ObjectMeta{
-						Namespace: "default",
-						Name:      "keep-this-one",
-						Labels:    map[string]string{"selector": "app-1"},
-					},
-				},
-				&corev1.Service{
-					ObjectMeta: metav1.ObjectMeta{
-						Namespace: "default",
-						Name:      "remove-this-1",
-						Labels:    map[string]string{"selector": "app-1"},
-					},
-				},
-			},
-		},
-	}
-	for _, tt := range tests {
-		t.Run(tt.name, func(t *testing.T) {
-			cl := k8stools.GetTestClientWithObjects(tt.predefinedObjects)
-			if err := RemoveOrphanedServices(tt.args.ctx, cl, tt.args.args, tt.args.spec); (err != nil) != tt.wantErr {
-				t.Errorf("RemoveOrphanedServices() error = %v, wantErr %v", err, tt.wantErr)
-			}
-			var wantSvcs corev1.ServiceList
-			if err := cl.List(tt.args.ctx, &wantSvcs); err != nil {
-				t.Fatalf("unexpected error: %v", err)
-			}
-			if len(wantSvcs.Items) != len(tt.wantServiceNames) {
-				t.Fatalf("unexpected count if services: %v", len(wantSvcs.Items))
-			}
-		})
-	}
-}
-
 func TestRemoveOrphanedDeployments(t *testing.T) {
 	type args struct {
 		ctx             context.Context

internal/controller/operator/factory/finalize/vmauth.go

@@ -11,23 +11,6 @@ import (
 	"sigs.k8s.io/controller-runtime/pkg/client"
 )
 
-// VMAuthIngressDelete handles case, when user wants to remove spec.Ingress from vmauth config.
-func VMAuthIngressDelete(ctx context.Context, rclient client.Client, crd *vmv1beta1.VMAuth) error {
-	vmauthIngress := &networkingv1.Ingress{
-		ObjectMeta: metav1.ObjectMeta{
-			Name:      crd.PrefixedName(),
-			Namespace: crd.Namespace,
-		},
-	}
-	if err := removeFinalizeObjByName(ctx, rclient, vmauthIngress, crd.PrefixedName(), crd.Namespace); err != nil {
-		return err
-	}
-	if err := SafeDelete(ctx, rclient, vmauthIngress); err != nil {
-		return err
-	}
-	return nil
-}
-
 // OnVMAuthDelete deletes all vmauth related resources
 func OnVMAuthDelete(ctx context.Context, rclient client.Client, crd *vmv1beta1.VMAuth) error {
 	// check deployment
@@ -56,6 +39,20 @@ func OnVMAuthDelete(ctx context.Context, rclient client.Client, crd *vmv1beta1.V
 			return err
 		}
 	}
+	if crd.Spec.Ingress != nil {
+		vmauthIngress := &networkingv1.Ingress{
+			ObjectMeta: metav1.ObjectMeta{
+				Name:      crd.PrefixedName(),
+				Namespace: crd.Namespace,
+			},
+		}
+		if err := removeFinalizeObjByName(ctx, rclient, vmauthIngress, crd.PrefixedName(), crd.Namespace); err != nil {
+			return err
+		}
+		if err := SafeDelete(ctx, rclient, vmauthIngress); err != nil {
+			return err
+		}
+	}
 
 	// check ingress
 	if err := removeFinalizeObjByName(ctx, rclient, &networkingv1.Ingress{}, crd.PrefixedName(), crd.Namespace); err != nil {