[Misc] Operator: Reconciles improved

internal/controller/informers.go

@@ -7,6 +7,7 @@ package controller
 
 import (
 	"reflect"
+	"time"
 
 	"github.com/sap/cap-operator/pkg/apis/sme.sap.com/v1alpha1"
 	"k8s.io/apimachinery/pkg/api/meta"
@@ -33,6 +34,8 @@ const (
 
 const queuing = "queuing resource for reconciliation"
 
+const defaultDependantDelay = 3 * time.Second
+
 var (
 	KindMap = map[int]string{
 		ResourceCAPApplication:        v1alpha1.CAPApplicationKind,
@@ -56,7 +59,6 @@ var QueueMapping map[int]map[int]string = map[int]map[int]string{
 	ResourceCAPTenantOperation:    {ResourceCAPTenantOperation: v1alpha1.CAPTenantOperationKind, ResourceCAPTenant: v1alpha1.CAPTenantKind},
 	ResourceDomain:                {ResourceDomain: v1alpha1.DomainKind},
 	ResourceClusterDomain:         {ResourceClusterDomain: v1alpha1.ClusterDomainKind},
-	ResourceSecret:                {ResourceCAPApplication: v1alpha1.CAPApplicationKind},
 	ResourceJob:                   {ResourceCAPTenantOperation: v1alpha1.CAPTenantOperationKind, ResourceCAPApplicationVersion: v1alpha1.CAPApplicationVersionKind},
 	ResourceGateway:               {ResourceDomain: v1alpha1.DomainKind, ResourceClusterDomain: v1alpha1.ClusterDomainKind},
 	ResourceCertificate:           {ResourceDomain: v1alpha1.DomainKind, ResourceClusterDomain: v1alpha1.ClusterDomainKind},
@@ -77,8 +79,8 @@ func (c *Controller) initializeInformers() {
 	c.registerCAPTenantOperationListeners()
 	c.registerDomainListeners()
 	c.registerClusterDomainListeners()
-	c.registerSecretListeners()
 	c.registerJobListeners()
+	c.registerSecretListeners()
 	c.registerGatewayListeners()
 	c.registerVirtualServiceListeners()
 	c.registerDestinationRuleListeners()
@@ -98,6 +100,10 @@ func (c *Controller) initializeInformers() {
 }
 
 func (c *Controller) getEventHandlerFuncsForResource(res int) cache.ResourceEventHandlerFuncs {
+	_, ok := QueueMapping[res]
+	if !ok {
+		return cache.ResourceEventHandlerFuncs{}
+	}
 	return cache.ResourceEventHandlerFuncs{
 		AddFunc: func(new any) {
 			c.enqueueModifiedResource(res, new, nil)
@@ -106,7 +112,7 @@ func (c *Controller) getEventHandlerFuncsForResource(res int) cache.ResourceEven
 			c.enqueueModifiedResource(res, new, old)
 		},
 		DeleteFunc: func(old any) {
-			c.enqueueModifiedResource(res, old, nil)
+			c.enqueueModifiedResource(res, nil, old)
 		},
 	}
 }
@@ -182,13 +188,10 @@ func (c *Controller) registerGardenerDNSEntrytListeners() {
 }
 
 func (c *Controller) enqueueModifiedResource(sourceKey int, new, old any) {
-	newObj, ok := getMetaObject(new)
-	if !ok {
-		return
-	}
-
-	oldObj, ok := getMetaObject(old)
-	if ok && oldObj.GetResourceVersion() == newObj.GetResourceVersion() {
+	newObj, newOk := getMetaObject(new)
+	oldObj, oldOk := getMetaObject(old)
+	if newOk && oldOk && oldObj.GetResourceVersion() == newObj.GetResourceVersion() {
+		klog.V(2).InfoS("skipping update scenario", "key", sourceKey, "new", newObj.GetName(), "sourceKey", sourceKey)
 		return // no changes in update
 	}
 
@@ -200,24 +203,50 @@ func (c *Controller) enqueueModifiedResource(sourceKey int, new, old any) {
 
 	for dependentKey, dependentKind := range mapping {
 		q := c.queues[dependentKey]
-
+		item := determineQueueItem(dependentKey, sourceKey, oldObj, newObj, dependentKind)
+		if item == nil {
+			continue
+		}
+		// Change on the main resource
 		if dependentKey == sourceKey {
-			// when the change is directly on the CRO check for spec and annotation changes - omits status changes
-			if oldObj != nil && !hasReconciliationRelevantChanges(newObj, oldObj) {
-				continue // do not enqueue
-			}
-			klog.InfoS(queuing, "namespace", newObj.GetNamespace(), "name", newObj.GetName(), "kind", dependentKind)
-			q.Add(QueueItem{Key: dependentKey, ResourceKey: NamespacedResourceKey{Name: newObj.GetName(), Namespace: newObj.GetNamespace()}})
-		} else if owner, ok := getOwnerByKind(newObj.GetOwnerReferences(), dependentKind); ok {
-			klog.InfoS(queuing, "namespace", newObj.GetNamespace(), "name", owner.Name, "kind", dependentKind)
-			q.Add(QueueItem{Key: dependentKey, ResourceKey: NamespacedResourceKey{Name: owner.Name, Namespace: newObj.GetNamespace()}})
-		} else if owner, ok := getOwnerFromObjectMetadata(newObj, dependentKind); ok {
-			klog.InfoS(queuing, "namespace", owner.Namespace, "name", owner.Name, "kind", dependentKind)
-			q.Add(QueueItem{Key: dependentKey, ResourceKey: NamespacedResourceKey{Name: owner.Name, Namespace: owner.Namespace}})
+			q.Add(*item)
+		} else {
+			q.AddAfter(*item, defaultDependantDelay)
 		}
 	}
 }
 
+func determineQueueItem(dependentKey int, sourceKey int, oldObj metav1.Object, newObj metav1.Object, dependentKind string) *QueueItem {
+	if dependentKey == sourceKey {
+		// Skip queue of CRO itelf on delete
+		// When the change (Update) is directly on the CRO check for spec and annotation changes - omits status changes
+		if newObj == nil || (oldObj != nil && !hasReconciliationRelevantChanges(newObj, oldObj)) {
+			return nil // do not enqueue
+		}
+		klog.InfoS(queuing, "namespace", newObj.GetNamespace(), "name", newObj.GetName(), "kind", dependentKind)
+		return &QueueItem{Key: dependentKey, ResourceKey: NamespacedResourceKey{Name: newObj.GetName(), Namespace: newObj.GetNamespace()}}
+	}
+	// Skip Queue of Owner on create --> the owner would have created these anyway
+	if oldObj == nil {
+		return nil
+	}
+	// Get the relevant obj to find the owner (usually newObj)
+	obj := newObj
+	// In case of delete newObj doesn't exist, hence use the oldObj to determine owner
+	if newObj == nil {
+		obj = oldObj
+	}
+	if owner, ok := getOwnerByKind(obj.GetOwnerReferences(), dependentKind); ok {
+		klog.InfoS(queuing, "namespace", obj.GetNamespace(), "name", owner.Name, "kind", dependentKind)
+		return &QueueItem{Key: dependentKey, ResourceKey: NamespacedResourceKey{Name: owner.Name, Namespace: obj.GetNamespace()}}
+	} else if owner, ok := getOwnerFromObjectMetadata(obj, dependentKind); ok {
+		klog.InfoS(queuing, "namespace", owner.Namespace, "name", owner.Name, "kind", dependentKind)
+		return &QueueItem{Key: dependentKey, ResourceKey: NamespacedResourceKey{Name: owner.Name, Namespace: owner.Namespace}}
+	}
+	klog.V(2).InfoS("skipping --> owner not found", "namespace", obj.GetNamespace(), "name", obj.GetName(), "kind", dependentKind, "sourceKey", sourceKey)
+	return nil
+}
+
 func getMetaObject(obj any) (metav1.Object, bool) {
 	if obj == nil {
 		return nil, false

internal/controller/informers_test.go

@@ -29,10 +29,11 @@
 	expectedResult = true
 }
 
 func TestController_initializeInformers(t *testing.T) {
 	tests := []struct {
 		name            string
 		expectedResult  bool
+		ownerOnlyCheck  bool
 		invalidOwnerRef bool
 		res             int
 		itemName        string
@@ -55,13 +56,15 @@
 		{
 			name:           "Test enqueueModifiedResource (ResourceCertificate) valid owner",
 			res:            ResourceCertificate,
+			ownerOnlyCheck: true,
 			expectedResult: true,
 			itemName:       "test-cert",
 			itemNamespace:  corev1.NamespaceDefault,
 		},
 		{
 			name:            "Test enqueueModifiedResource (ResourceCertificate) invalid owner",
 			res:             ResourceCertificate,
+			ownerOnlyCheck:  true,
 			expectedResult:  false,
 			invalidOwnerRef: true,
 			itemName:        "test-cert",
@@ -114,7 +117,7 @@
 			}
 
 			testC.initializeInformers()
-			var res any
+			var res, oldRes any
 			switch tt.res {
 			case ResourceCAPApplication:
 				res = createCaCRO(tt.itemName, false)
@@ -135,17 +138,22 @@
 					cert.Annotations[AnnotationOwnerIdentifier] = tt.itemNamespace + "." + tt.itemName
 					cert.Labels[LabelOwnerIdentifierHash] = sha1Sum(tt.itemNamespace, tt.itemName)
 				}
-				res = cert
+				oldRes = cert
+				newCert := cert.DeepCopy()
+				newCert.SetResourceVersion("2")
+				res = newCert
 			case 999:
 				res = &corev1.Pod{ObjectMeta: metav1.ObjectMeta{Name: tt.itemName}}
 			}
 			// Add/delete
-			testC.enqueueModifiedResource(tt.res, res, nil)
-			if expectedResult != tt.expectedResult {
-				t.Error("Unexpected result", expectedResult)
+			if !tt.ownerOnlyCheck {
+				testC.enqueueModifiedResource(tt.res, res, nil)
+				if expectedResult != tt.expectedResult {
+					t.Error("Unexpected result", expectedResult)
+				}
 			}
 			// Update
-			testC.enqueueModifiedResource(tt.res, res, res)
+			testC.enqueueModifiedResource(tt.res, res, oldRes)
 			if expectedResult != tt.expectedResult {
 				t.Error("Unexpected result", expectedResult)
 			}

internal/controller/reconcile-capapplication.go

@@ -536,6 +536,9 @@ func (c *Controller) handleCAPApplicationDeletion(ctx context.Context, ca *v1alp
 		var tenantFound bool
 		util.LogInfo("Deleting dependent tenants", string(Deleting), ca, nil)
 		if tenantFound, err = c.deleteTenants(ctx, ca); tenantFound || err != nil {
+			if tenantFound {
+				return NewReconcileResultWithResource(ResourceCAPApplication, ca.Name, ca.Namespace, 10*time.Second), nil
+			}
 			util.LogError(err, "Could not delete dependent tenant", string(Deleting), ca, nil)
 			return nil, err
 		}

internal/controller/reconcile-capapplication_test.go

@@ -310,6 +310,7 @@ func TestDeletion_Case5(t *testing.T) {
 				"testdata/capapplication/cat-consumer-no-finalizers-ready.yaml",
 			},
 			expectedResources: "testdata/capapplication/ca-17.expected.yaml",
+			expectedRequeue:   map[int][]NamespacedResourceKey{ResourceCAPApplication: {{Namespace: "default", Name: "test-cap-01"}}},
 		},
 	)
 }

internal/controller/reconcile-captenant.go

@@ -20,8 +20,8 @@ import (
 )
 
 type IdentifiedCAPTenantOperations struct {
-	active    []v1alpha1.CAPTenantOperation
-	processed []v1alpha1.CAPTenantOperation
+	active    []*v1alpha1.CAPTenantOperation
+	processed []*v1alpha1.CAPTenantOperation
 }
 
 type CAPTenantOperationTypeSelector string
@@ -75,6 +75,8 @@ const (
 	EventActionUpgrade                   = "Upgrade"
 )
 
+const tenantOperationTimeout = 30 * time.Second
+
 var operationTypeMsgMap = map[v1alpha1.CAPTenantOperationType]string{
 	v1alpha1.CAPTenantOperationTypeProvisioning:   string(Provisioning),
 	v1alpha1.CAPTenantOperationTypeUpgrade:        string(Upgrading),
@@ -129,7 +131,7 @@ var TenantOperationStatusMap = map[v1alpha1.CAPTenantOperationType]StatusInfo{
 
 func getTenantReconcileResultConsideringDeletion(cat *v1alpha1.CAPTenant, fallback *ReconcileResult) *ReconcileResult {
 	if cat.DeletionTimestamp != nil && cat.Status.State != v1alpha1.CAPTenantStateDeleting {
-		return NewReconcileResultWithResource(ResourceCAPTenant, cat.Name, cat.Namespace, 15*time.Second)
+		return NewReconcileResultWithResource(ResourceCAPTenant, cat.Name, cat.Namespace, tenantOperationTimeout)
 	}
 	return fallback
 }
@@ -138,7 +140,7 @@ var handleWaitingForTenantOperation = func(ctx context.Context, c *Controller, c
 	// NOTE: not returning a requeue item is ok, as changes in CAPTenantOperation status will queue the item via the informer
 	util.LogInfo("Waiting for tenant operation to complete", operationTypeMsgMap[ctop.Spec.Operation], cat, ctop, "tenantId", cat.Spec.TenantId, "version", cat.Spec.Version)
 	cat.SetStatusWithReadyCondition(target.state, target.conditionStatus, target.conditionReason, fmt.Sprintf("waiting for %s %s.%s of type %s to complete", v1alpha1.CAPTenantOperationKind, ctop.Namespace, ctop.Name, ctop.Spec.Operation))
-	return NewReconcileResultWithResource(ResourceCAPTenant, cat.Name, cat.Namespace, 15*time.Second), nil // requeue while the tenant operation is being processed
+	return NewReconcileResultWithResource(ResourceCAPTenant, cat.Name, cat.Namespace, tenantOperationTimeout), nil // requeue while the tenant operation is being processed
 }
 
 var handleCompletedProvisioningUpgradeOperation = func(ctx context.Context, c *Controller, cat *v1alpha1.CAPTenant, target StateCondition, ctop *v1alpha1.CAPTenantOperation) (*ReconcileResult, error) {
@@ -270,15 +272,15 @@ func (c *Controller) updateCAPTenant(ctx context.Context, cat *v1alpha1.CAPTenan
 	return
 }
 
-func findLatestCreatedTenantOperation(ops []v1alpha1.CAPTenantOperation, selector CAPTenantOperationTypeSelector) (latest *v1alpha1.CAPTenantOperation) {
+func findLatestCreatedTenantOperation(ops []*v1alpha1.CAPTenantOperation, selector CAPTenantOperationTypeSelector) (latest *v1alpha1.CAPTenantOperation) {
 	for _, op := range ops {
 		// workaround to fix pointer resolution after loop -> https://stackoverflow.com/questions/45967305/copying-the-address-of-a-loop-variable-in-go
 		ctop := op
 		if selector != CAPTenantOperationTypeSelectorAll && CAPTenantOperationTypeSelector(ctop.Spec.Operation) != selector {
 			continue
 		}
 		if latest == nil || ctop.CreationTimestamp.After(latest.CreationTimestamp.Time) {
-			latest = &ctop
+			latest = ctop
 		}
 	}
 
@@ -542,14 +544,14 @@ func (c *Controller) getCAPTenantOperationsByType(ctx context.Context, cat *v1al
 		return nil, err
 	}
 
-	// NOTE: do not use cache for listing (this is not a very frequent operation)
-	ops, err := c.crdClient.SmeV1alpha1().CAPTenantOperations(cat.Namespace).List(ctx, metav1.ListOptions{LabelSelector: selector.String()})
+	// Check if tenant operations already exist via cache
+	ops, err := c.crdInformerFactory.Sme().V1alpha1().CAPTenantOperations().Lister().CAPTenantOperations(cat.Namespace).List(selector)
 	if err != nil {
 		return nil, err
 	}
 
-	var results = IdentifiedCAPTenantOperations{active: []v1alpha1.CAPTenantOperation{}, processed: []v1alpha1.CAPTenantOperation{}}
-	for _, ctop := range ops.Items {
+	var results = IdentifiedCAPTenantOperations{active: []*v1alpha1.CAPTenantOperation{}, processed: []*v1alpha1.CAPTenantOperation{}}
+	for _, ctop := range ops {
 		if isCROConditionReady(ctop.Status.GenericStatus) {
 			results.processed = append(results.processed, ctop)
 		} else {