refactor finalize package

[AndrewChubatiuk]

list of potential improvements for internal/controller/operator/factory/finalize package:

• remove finalizers from core resources, as they don't provide any value, leads to dangling resources during cleanup or removal and rjust equire extra patch request to remove finalizers
• replace ownerreferences for cluster wide resources with selectors. cluster wide resources can only have a cluster wide resources as an owner, in our case we're using CRD as owner reference, which makes no sense. replace it with label selector.
• some namespaced resources use selectors instead of owner reference to define which objects should be removed, replace this logic with ownerreferences check

[endesapt]

I would like to give it a try. At least I can start with removing unnecessary SafeDelete from finalizers like these ones:

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

Lines 91 to 97 in 1bc4a7e

	if err := SafeDelete(ctx, rclient, &rbacv1.RoleBinding{ObjectMeta: metav1.ObjectMeta{Name: cr.GetClusterRoleName(), Namespace: cr.GetNamespace()}}); err != nil {
	return err
	}
	if err := SafeDelete(ctx, rclient, &rbacv1.Role{ObjectMeta: metav1.ObjectMeta{Name: cr.GetClusterRoleName(), Namespace: cr.GetNamespace()}}); err != nil {
	return err
	}

But i have question about finalization logic using selectors - if i understand it right, you cant filter resources natively by ownerReference, so it is needed to filter using selectorLabels (as i understand that is the function where the main improvement can be done)

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

Lines 101 to 110 in 1bc4a7e

	// removeOrphaned removes orphaned resources
	func removeOrphaned(ctx context.Context, rclient client.Client, cr orphanedCRD, gvk schema.GroupVersionKind, keepNames map[string]struct{}) error {
	var l unstructured.UnstructuredList
	l.SetGroupVersionKind(gvk)
	opts := client.ListOptions{
	Namespace: cr.GetNamespace(),
	LabelSelector: labels.SelectorFromSet(cr.SelectorLabels()),
	}
	if err := rclient.List(ctx, &l, &opts); err != nil {
	return err

But I can create an index for each CRD for each resource(depends on CRD) like that:

mgr.GetFieldIndexer().IndexField(ctx, &appsv1.Deployment{}, 
   ".metadata.controller", func(o client.Object) []string {
       owner := metav1.GetControllerOf(o)
       if owner == nil { return nil }
       return []string{owner.Name}
   })

so we can get the resources using ownerName.
Maybe i dont understand the problem and thats why i only can come up with this solution, and it seems kind of redundant.
The other solution is to remove selector entirely (there is still ownerReference check so we wont delete other resources) but it will be a problem if the namespace is full of other deployments and we need to load all that

What do you think?

[AndrewChubatiuk]

hey @endesapt

I would like to give it a try.

sure, thanks you for a help. first of all need to remove explicit delete call to k8s API when resource is removed and keep it only for orphaned resources cleanup (when only certain feature VMServiceScrape/HPA/PDB/etc is disabled)

But i have question about finalization logic using selectors

first of all i meant this function it uses labels instead of owner reference to identify resource, which can be removed.

mgr.GetFieldIndexer()

nice, haven't used this before, but let's postpone this

[endesapt]

I opened PR where i removed redundant delete calls where i think it can be removed. It will be great if you can check if its okay

[AndrewChubatiuk]

Thanks you for your work, left only one comment there

[AndrewChubatiuk]

closing this as implemented in 0.68.2, the only thing left to do is to remove finalizers cleanup logic after some period of time