feat[compositions]: realtime compositor – part 2: changes to MRs by sttts · Pull Request #4637 · crossplane/crossplane

sttts · 2023-09-18T12:05:51Z

Description of your changes

Follow-up of #4582: also reconcile XRs when MRs change. This PR starts informers dynamically for GVRs used in XRs, including function XRs.

This feature is under the EnableRealtimeCompositions feature gate (--enable-realtime-compositions).

Design:

                    ┌─────────────────┐
                    │                 │
                    │     Manager     │
                    │                 │
                    │    ┌───────┐    │
                    │    │       │    │
                    └────┤ Cache ├────┘
                         │       │
                         └───▲───┘◄─────┬──────────┬───────────────────┐
                             │          │          │                   │
                    ┌────────┴────────┐ │ ┌────────┴────────┐ ┌────────┴────────┐
       ┌────────────┤ APIExtensions   │ │ │ APIExtensions   │ │ APIExtensions   │
       │ Composed   │ Definition      │ │ │ Compositions    │ │ Offered         │
┌──────┤ Resource   │ Controller      │ │ │ Controller      │ │ Controller      │ ...
│      │ Informers  │                 │ │ │                 │ │                 │
│      └─────┬──────┤  ┌───────────┐  │ │ │                 │ │                 │
│    creates │      └──┤           ├──┘ │ └─────────────────┘ └─────────────────┘
│  & deletes │         │ GVK Routed│    │
│     ┌──────▼────┐    │ Cache     ├────┘
│     │ Composed  │◄───┤           │fallback
│     │ Resource  │ N:1└─────▲─────┘
│     │ Cache     │          │
│     └───────────┘          │
│                            │ N:1 (dynamically started when an XRD changes)
│                   ┌────────┴────────┐
│                   │ APIExctensions  │
│ informs about gvks│ Composite       │
└───────────────────┤ Controller      │
                    │                 │
                    │                 │
                    └─────────────────┘

tl/dr:

the APIExtensions definitions controller (which spawns the composite controllers dynamically through the controller engine), maintains a dynamic set of cached (= informers) per gvk.
The composite controllers report (via the KindObserver interface) to the definition controller which GVKs the referenced objects have. For those, the "composedResourceInformers" (part of the definition controller) will start informers as part of a cache.
When those caches become synced, they are registered in the GVK-routed cache as delegates. From that moment on that cache serves those objects. The composite controllers use that GVK-routed-cache for their lookups.
Last but not least the "composedResourceInformers" distributes events of changed composed resources to the right composite controllers. For that, an index is added to the XR informer by GVK of the referenced object of each XR instance. Through that it is efficient to find the right XRs for every event.

I have:

Read and followed Crossplane's contribution process.
Added or updated unit and E2E tests for my change.
Run make reviewable to ensure this PR is ready for review.
~~Added backport release-x.y labels to auto-backport this PR, if necessary.~~
~~Opened a PR updating the docs, if necessary.~~

negz · 2023-09-18T19:32:50Z

Adding a reference to #4316, which I think is related.

test/e2e/realtimecompositions_test.go

pedjak · 2023-10-16T22:15:53Z

internal/controller/apiextensions/composite/reconciler.go

+		for _, ref := range xr.GetResourceReferences() {
+			gvks = append(gvks, ref.GroupVersionKind())
+		}
+		r.kindObserver.WatchComposedResources(gvks...)


if we now watch composed resources and trigger the reconcile in realtime, should we stop ending the loop with:

return reconcile.Result{RequeueAfter: r.pollInterval}, errors.Wrap(r.client.Status().Update(ctx, xr), errUpdateStatus)

and do instead:

return reconcile.Result{}, errors.Wrap(r.client.Status().Update(ctx, xr), errUpdateStatus)

We can't I guess. 1. environments 2. functions relying on 60s poll.

Should we do everything towards removing poll? Yes, please. @negz

if environment feature is not enabled and composition is not based on functions, then we could? Also, we could watch environment for changes using the same WatchComposeResource approach, but keep in memory relationships between environments and compositions, to know later what composition to reconcile.

Am not against that direction, quite the opposite. Worth an attempt in a follow-up to see how far we get (in e2e).

For functions, I would like to see us being explicit that there is no 60s poll guarantee. Maybe we need an API to ask for one if necessary.

That will be step 3: #4822

pedjak · 2023-10-17T08:53:00Z

test/e2e/apiextensions_test.go

 			)).
 			Assess("CreateClaim", funcs.AllOf(
 				funcs.ApplyResources(FieldManager, manifests, "claim.yaml"),
+				funcs.InBackground(funcs.LogResources(nopList)),


how is this change related to the work done in this PR? I looks like a leftover of some debugging?

These tests run under realtime composition as well. And in general, I would like us to have this kind of output to understand the core mechanisms at play in such a test. Hence, I added it also here. There are very likely more places where we need visibility of the main flow of a test.

pedjak · 2023-10-17T09:04:44Z

test/e2e/realtimecompositions_test.go

+			)).
+			Assess("CreateClaim", funcs.AllOf(
+				funcs.ApplyResources(FieldManager, manifests, "claim.yaml"),
+				funcs.InBackground(funcs.LogResources(nopList, withTestLabels)),


InBackground calls are for debugging? Could we make them implicit, for example call them within ApplyResource, so that all tests can automatically have benefits?

Not (only) for debugging. For visibility of the core mechanism at play in a test.

We cannot do these automatically as e.g. ApplyResources has no knowledge about what happens with those resources, i.e. the related resources they spawn.

ApplyResource could become a bit smarter and understand that we are applying claim, and then via references follow to composite and MRs, or?

PR welcome for an automatic related object tracker :-)

test/e2e/realtimecompositions_test.go

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

phisco

This should also delete the claim first and then check all nop resources have been deleted, no?

This was removed in crossplane#4637, with the options instead passed as a 'first-class' argument. I don't think these need to be a first-class argument. This reverts to the old behaviour, which matches the claim 'offered' controller. I noticed this asymmetry while reminding myself how feature flags are plumbed down to the offered and definition controllers. The former is responsible for managing claim controllers, the latter XR controllers. Signed-off-by: Nic Cope <nicc@rk0n.org>

sttts requested review from a team and turkenh as code owners September 18, 2023 12:05

sttts requested a review from ytsarev September 18, 2023 12:05

sttts marked this pull request as draft September 18, 2023 12:06

sttts force-pushed the sttts-realtime-compositions-resources branch from f0fb10b to b0f0ed8 Compare September 18, 2023 12:17

sttts mentioned this pull request Sep 20, 2023

controller/engine: use local cache for client read requests crossplane/crossplane-runtime#554

Merged

2 tasks

sttts force-pushed the sttts-realtime-compositions-resources branch from b0f0ed8 to 5858c89 Compare September 20, 2023 14:06

sttts changed the title ~~WIP: realtime composition for MRs~~ feat[compositions]: realtime compositor – part 2: changes to MRs Sep 20, 2023

sttts force-pushed the sttts-realtime-compositions-resources branch 4 times, most recently from 3642e40 to a0e8a69 Compare September 20, 2023 14:48

sttts force-pushed the sttts-realtime-compositions-resources branch 2 times, most recently from ba73b18 to 3197040 Compare October 5, 2023 09:09

sttts marked this pull request as ready for review October 5, 2023 09:10

sttts requested a review from negz as a code owner October 5, 2023 09:10

sttts force-pushed the sttts-realtime-compositions-resources branch 12 times, most recently from 10c91b7 to b74ec8d Compare October 10, 2023 08:46

sttts force-pushed the sttts-realtime-compositions-resources branch 8 times, most recently from 43bfdb7 to 4e3fd74 Compare October 16, 2023 13:37

negz approved these changes Oct 16, 2023

View reviewed changes

pedjak reviewed Oct 16, 2023

View reviewed changes

test/e2e/realtimecompositions_test.go Outdated Show resolved Hide resolved

pedjak reviewed Oct 16, 2023

View reviewed changes

sttts force-pushed the sttts-realtime-compositions-resources branch from 4e3fd74 to 7cf75af Compare October 17, 2023 07:43

pedjak reviewed Oct 17, 2023

View reviewed changes

test/e2e/realtimecompositions_test.go Show resolved Hide resolved

sttts added 4 commits October 17, 2023 14:37

Bump crossplane-runtime

f4546c6

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

test/e2e: add funcs.LogResources

cba02fd

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

apiextensions/composite: instantly reconcile XRs when MRs change

996919f

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

apiextensions/composite: add realtime-compositions e2e test

2348236

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

sttts force-pushed the sttts-realtime-compositions-resources branch from 7cf75af to 2348236 Compare October 17, 2023 12:37

negz merged commit 5bb2284 into crossplane:master Oct 17, 2023

sttts mentioned this pull request Oct 17, 2023

Promote Realtime Compositions to Beta #4828

Closed

phisco reviewed Oct 18, 2023

View reviewed changes

sttts mentioned this pull request Oct 18, 2023

e2e: in realtime compositions test delete claim and wait for nop resources #4836

Merged

6 tasks

This was referenced Oct 27, 2023

Document Realtime compositions feature crossplane/docs#587

Closed

Document Realtime Compositions alpha feature crossplane/docs#588

Closed

Conversation

sttts commented Sep 18, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description of your changes

Uh oh!

negz commented Sep 18, 2023

Uh oh!

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!

Uh oh!

phisco left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

sttts commented Sep 18, 2023 •

edited

Loading