WIP: Fix several issues in realtime-compositions by sttts · Pull Request #5422 · crossplane/crossplane

sttts · 2024-02-22T16:48:35Z

Description of your changes

In collaboration with @haarchri, fixing a number of issues in realtime-compositions. In particular, ensure the right life-cycle of watches and informers to match that of the composite controller:

~~apiextensions/definition: don't attempt to start multiple composite controllers~~ merged through apiextension/definition: don't attempt to start composite controllers multiple times #5437
apiextensions/definition: fix life-cycle of composite informers to be that of the composite controller
~~apiextensions/definition: fix leak of watch on composition revisions due to wrong referenced cache~~ moved to apiextensions/definition: fix leak of watch on composition revisions due to wrong referenced cache #5463
~~apiextensions/definition: unregister composite gvk from composedResourceInformers everywhere~~ merged through apiextension/definition: don't attempt to start composite controllers multiple times #5437
apiextensions/definition: wait for sync in composedResourceInformers and delay cd informer deletion
revert Don't run Realtime Composition E2E tests in CI #5296.

~~Depends on crossplane/crossplane-runtime#672.~~

Fixes #5400

I have:

Read and followed Crossplane's contribution process.
Run make reviewable to ensure this PR is ready for review.
~~[ ] Added or updated unit tests.~~
Added or updated e2e tests.
~~[ ] Linked a PR or a docs tracking issue to document this change.~~
~~[ ] Added backport release-x.y labels to auto-backport this PR.~~

Need help with this checklist? See the cheat sheet.

negz · 2024-02-22T18:50:18Z

internal/controller/apiextensions/composite/reconciler.go

 			log.Info("cannot list in CompositionRevision handler", "type", schema.GroupVersionKind(of).String(), "error", err)
 			return
 		}
+		log.Debug("Enqueueing composite resources because a new CompositionRevision was created", "count", len(xrs.Items))


It might be useful to include the GVK of the XRs that will be enqueued, like the log line above.

negz · 2024-02-22T18:50:36Z

internal/controller/apiextensions/definition/composed.go

 	xrCaches map[schema.GroupVersionKind]cache.Cache
-	sinks    map[string]func(ev runtimeevent.UpdateEvent) // by some uid
+	// xrCachesSynced holds the composite resource informers that are synced. We
+	// don't


Looks like this trails off.

negz · 2024-02-22T18:51:54Z

internal/controller/apiextensions/definition/composed.go

+			i.lock.Lock()
+			defer i.lock.Unlock()
+			if _, ok := i.xrCaches[gvk]; ok {
+				i.log.Debug("Composite resource cache seen synced", "gvk", gvk.String(), "after", time.Since(startAt))


Nit: In the informers above the GVK uses type rather than gvk. Being consistent would help anyone processing these structured logs who wanted to filter on GVK.

have switched to gvk.

negz · 2024-02-22T18:54:04Z

internal/controller/apiextensions/definition/composed.go

 // RegisterComposite registers a composite resource cache with its GVK.
 // Instances of this GVK will be considered to keep composed resource informers
-// alive.
+// alive. The cache does not have to be synced yet.


Is this saying that it's safe to call this method before the cache is synced? It reads a little ambiguously to me.

negz · 2024-02-22T18:55:07Z

internal/controller/apiextensions/definition/composed.go

 	}
 	i.xrCaches[gvk] = ca
+
+	// asynchronously mark the cache as synced. We won't list XRs before this


Style nit: The prevailing pattern is for comments to use proper grammar, i.e. start with a capital letter, unless the first word is the name of an unexported type.

negz · 2024-02-22T19:03:08Z

internal/controller/apiextensions/definition/reconciler.go

+		controller.TriggeredBy(&lazySyncingSource{Create: func() source.SyncingSource {
+			return source.Kind(xrCache, &v1.CompositionRevision{})
+		}}, handler.Funcs{
+			CreateFunc: composite.EnqueueForCompositionRevisionFunc(ck, func(ctx context.Context, list client.ObjectList, opts ...client.ListOption) error {
+				return xrCache.List(ctx, list, opts...)
+			}, r.log),


Style nit: This is a bit of a handful to read due to the nested function definitions with long signatures.

have made it similar to the other source. A little bit easier to read. The closured for delaying evaluation are little ugly. But Go doesn't allow a better syntax. Wished there was a foo(bla, lazy xrCache, ...).

Signed-off-by: Dr. Stefan Schimanski <stefan.schimanski@upbound.io>

This reverts commit 53b143f. Signed-off-by: Dr. Stefan Schimanski <stefan.schimanski@upbound.io>

jbw976 · 2024-03-11T09:14:58Z

Looks like confidence in this approach is growing as per #5400 (comment). I've added the backport labels for v1.14 and v1.15 so once this is merged we can likely get patch releases out with this fix 💪

sttts · 2024-03-11T15:17:23Z

@jbw976 this PR is not ready yet. It includes a couple of improvement all in one PR, not all ready. #5437 merged into master already. That one should be backported.

jbw976 · 2024-04-23T17:42:33Z

ah and this one may be the most impactful of the bunch @sttts if you're able to make progress on this for v1.16. Sorry for the multiple notifications, just going through all the open PRs now that could be impactful for v1.16 😉

haarchri · 2024-12-19T18:17:56Z

this part #5400 is still open - which gets fixed here - so i wonder if we can cherry pick this

jbw976 · 2024-12-19T23:11:23Z

Thanks for the reminder on this PR and that you're still seeing issues with realtime compositions even after the big changes in #5651 @haarchri!

I'll make sure to peruse through the changes in this PR and work with @sttts and @negz to see if they can be ported to the state of the world after #5651. This PR can't be re-opened since GitHub says the master branch has been deleted, but I've added it to the beta tracking epic as a reminder to look more deeply soon 🙇‍♂️

sttts requested review from a team and negz as code owners February 22, 2024 16:48

sttts requested a review from phisco February 22, 2024 16:48

sttts force-pushed the sttts-realtime-compositions-xrrev-race branch from 5e40e58 to fddd833 Compare February 22, 2024 16:50

negz reviewed Feb 22, 2024

View reviewed changes

sttts force-pushed the sttts-realtime-compositions-xrrev-race branch 2 times, most recently from 82136ca to 199edad Compare February 23, 2024 09:15

sttts changed the title ~~Fix several issues in realtime-compositions~~ WIP: Fix several issues in realtime-compositions Feb 23, 2024

sttts marked this pull request as draft February 23, 2024 09:37

sttts force-pushed the sttts-realtime-compositions-xrrev-race branch 4 times, most recently from 26a1b40 to d8b23b9 Compare February 28, 2024 12:46

sttts mentioned this pull request Feb 29, 2024

apiextension/definition: don't attempt to start composite controllers multiple times #5437

Merged

3 tasks

sttts force-pushed the sttts-realtime-compositions-xrrev-race branch from d8b23b9 to 2aa0c76 Compare March 1, 2024 07:52

jbw976 mentioned this pull request Mar 4, 2024

Crossplane fails to synchronize claims with XRs #5400

Closed

sttts added 3 commits March 5, 2024 21:30

apiextensions/definition: delay cd informer deletion

c26819c

Signed-off-by: Dr. Stefan Schimanski <stefan.schimanski@upbound.io>

apiextensions/definition: wait for sync in composedResourceInformers

be020be

Signed-off-by: Dr. Stefan Schimanski <stefan.schimanski@upbound.io>

apiextensions/definition: cosmetics

63b3da4

Signed-off-by: Dr. Stefan Schimanski <stefan.schimanski@upbound.io>

sttts force-pushed the sttts-realtime-compositions-xrrev-race branch from 2aa0c76 to 6cf5e57 Compare March 5, 2024 20:30

Revert "Don't run Realtime Composition E2E tests in CI"

f27e25e

This reverts commit 53b143f. Signed-off-by: Dr. Stefan Schimanski <stefan.schimanski@upbound.io>

sttts force-pushed the sttts-realtime-compositions-xrrev-race branch from 6cf5e57 to f27e25e Compare March 8, 2024 23:10

sttts mentioned this pull request Mar 11, 2024

apiextensions/definition: wait for sync in composedResourceInformers #5468

Closed

3 tasks

jbw976 added backport release-1.14 backport release-1.15 labels Mar 11, 2024

negz mentioned this pull request May 8, 2024

Use a single cache for all dynamic controllers (i.e. XRs and claims) #5651

Merged

6 tasks

negz closed this in #5651 May 21, 2024

jbw976 mentioned this pull request Dec 19, 2024

Promote Realtime Compositions to Beta #4828

Closed

Conversation

sttts commented Feb 22, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description of your changes

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jbw976 commented Mar 11, 2024

Uh oh!

sttts commented Mar 11, 2024

Uh oh!

jbw976 commented Apr 23, 2024

Uh oh!

haarchri commented Dec 19, 2024

Uh oh!

jbw976 commented Dec 19, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

sttts commented Feb 22, 2024 •

edited

Loading