diff --git a/internal/controller/informers.go b/internal/controller/informers.go index 529c6e00..d6b5935c 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 = 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 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-capapplication.go b/internal/controller/reconcile-capapplication.go index 6db34b64..7a18c54b 100644 --- a/internal/controller/reconcile-capapplication.go +++ b/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 } diff --git a/internal/controller/reconcile-capapplication_test.go b/internal/controller/reconcile-capapplication_test.go index 032a5991..101c0e4e 100644 --- a/internal/controller/reconcile-capapplication_test.go +++ b/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"}}}, }, ) } diff --git a/internal/controller/reconcile-captenant.go b/internal/controller/reconcile-captenant.go index 09e6664a..05f0592e 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 } } @@ -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 {