From d303210f91c22a6f181113d57cbf1e84dfc3ffeb Mon Sep 17 00:00:00 2001 From: Pavan <25031267+Pavan-SAP@users.noreply.github.com> Date: Tue, 19 May 2026 22:38:58 +0200 Subject: [PATCH] [Misc] Operator: Reconciles improved Ignore some unnecessary reconciles. --- internal/controller/informers.go | 14 +++++++++++--- internal/controller/informers_test.go | 20 ++++++++++++++------ internal/controller/reconcile-captenant.go | 22 ++++++++++++---------- 3 files changed, 37 insertions(+), 19 deletions(-) diff --git a/internal/controller/informers.go b/internal/controller/informers.go index 529c6e00..9b6dbbd9 100644 --- a/internal/controller/informers.go +++ b/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 = 5 * time.Second + var ( KindMap = map[int]string{ ResourceCAPApplication: v1alpha1.CAPApplicationKind, @@ -106,7 +109,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) }, } } @@ -183,6 +186,7 @@ func (c *Controller) registerGardenerDNSEntrytListeners() { func (c *Controller) enqueueModifiedResource(sourceKey int, new, old any) { newObj, ok := getMetaObject(new) + // Skip deletes in general! if !ok { return } @@ -208,12 +212,16 @@ func (c *Controller) enqueueModifiedResource(sourceKey int, new, old any) { } klog.InfoS(queuing, "namespace", newObj.GetNamespace(), "name", newObj.GetName(), "kind", dependentKind) q.Add(QueueItem{Key: dependentKey, ResourceKey: NamespacedResourceKey{Name: newObj.GetName(), Namespace: newObj.GetNamespace()}}) + } + // Only queue parent items on updates + if old == nil { + continue } 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()}}) + q.AddAfter(QueueItem{Key: dependentKey, ResourceKey: NamespacedResourceKey{Name: owner.Name, Namespace: newObj.GetNamespace()}}, defaultDependantDelay) } 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.AddAfter(QueueItem{Key: dependentKey, ResourceKey: NamespacedResourceKey{Name: owner.Name, Namespace: owner.Namespace}}, defaultDependantDelay) } } } diff --git a/internal/controller/informers_test.go b/internal/controller/informers_test.go index 16c3d194..dfce2cb4 100644 --- a/internal/controller/informers_test.go +++ b/internal/controller/informers_test.go @@ -33,6 +33,7 @@ func TestController_initializeInformers(t *testing.T) { tests := []struct { name string expectedResult bool + ownerOnlyCheck bool invalidOwnerRef bool res int itemName string @@ -55,6 +56,7 @@ func TestController_initializeInformers(t *testing.T) { { name: "Test enqueueModifiedResource (ResourceCertificate) valid owner", res: ResourceCertificate, + ownerOnlyCheck: true, expectedResult: true, itemName: "test-cert", itemNamespace: corev1.NamespaceDefault, @@ -62,6 +64,7 @@ func TestController_initializeInformers(t *testing.T) { { name: "Test enqueueModifiedResource (ResourceCertificate) invalid owner", res: ResourceCertificate, + ownerOnlyCheck: true, expectedResult: false, invalidOwnerRef: true, itemName: "test-cert", @@ -114,7 +117,7 @@ func TestController_initializeInformers(t *testing.T) { } testC.initializeInformers() - var res any + var res, oldRes any switch tt.res { case ResourceCAPApplication: res = createCaCRO(tt.itemName, false) @@ -135,17 +138,22 @@ func TestController_initializeInformers(t *testing.T) { 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) } diff --git a/internal/controller/reconcile-captenant.go b/internal/controller/reconcile-captenant.go index ea81baa7..e0d40752 100644 --- a/internal/controller/reconcile-captenant.go +++ b/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,7 +272,7 @@ 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 @@ -278,7 +280,7 @@ func findLatestCreatedTenantOperation(ops []v1alpha1.CAPTenantOperation, selecto continue } if latest == nil || ctop.CreationTimestamp.After(latest.CreationTimestamp.Time) { - latest = &ctop + latest = ctop } } @@ -545,14 +547,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 {