feat[compositions]: realtime compositor – part 1: changes to compositions by sttts · Pull Request #4582 · crossplane/crossplane

sttts · 2023-09-06T15:34:36Z

Description of your changes

This PR makes the compositor (= the controller for the XR) to watch CompositionRevisions too, and run the compositor on CompositionRevision creation for the relevant XRs, in order to select the new revision in automatic mode, and to reapply the new manifests.

This speeds up the inner loop for composition authors to be realtime.

Demo (left changing the composition, top right the composition revision being created, middle right the revision is selected for the XR, bottom right the effect on the MR):

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.~~

sttts · 2023-09-09T12:24:39Z

@jbw976 @negz I would like to see some initial review from you. Anything blocking?

Clearly, some (unit) tests are necessary.

negz · 2023-09-15T20:34:12Z

@sttts +1 for adding this functionality. Good idea. As to the implementation, it mostly looks great. My only thought is whether it makes sense for NewReconciler to return the EventHandler. It seems like it's plumbing up types that are passed into NewReconciler. Could you export EnqueueForCompositionRevisionFunc and call that from definition/reconciler.go?

sttts · 2023-09-18T08:02:28Z

My only thought is whether it makes sense for NewReconciler to return the EventHandler. It seems like it's plumbing up types that are passed into NewReconciler. Could you export EnqueueForCompositionRevisionFunc and call that from definition/reconciler.go?

Done. You were right, looks better now.

internal/controller/apiextensions/composite/reconciler_test.go

pedjak · 2023-09-18T16:27:55Z

internal/controller/apiextensions/composite/reconciler_test.go

+					if !ok {
+						t.Errorf("list was not an UnstructuredList")
+					} else if u.GroupVersionKind() != dogList {
+						t.Errorf("list was not an UnstructuredList")


the error here is not about wron GVK.

fixed in next push.

sttts · 2023-09-19T16:49:50Z

Usage flake

sttts · 2023-09-19T18:07:44Z

Have added an e2e test.

sttts · 2023-09-20T21:05:32Z

@pedjak @negz ptal

pedjak

lgtm, a few non-blocking questions only.

pedjak · 2023-09-21T08:05:09Z

internal/controller/apiextensions/composite/reconciler.go

+		xrs.SetGroupVersionKind(schema.GroupVersionKind(of))
+		xrs.SetKind(schema.GroupVersionKind(of).Kind + "List")
+		if err := list(ctx, &xrs); err != nil {
+			log.Debug("cannot list in CompositionRevision handler", "type", schema.GroupVersionKind(of).String(), "error", err)


should this log line get a bit more attention, i.e. be Info, if such error does not happen often?

that's a more general question (out of scope here): should we log serious problems above debug if they most likely show a logic problem in code, but the end user cannot do anything with them. IMO we should. Got told elsewhere that debug is the right level. I disagree.

Since this is a new piece of code, I think we could go with Info here.

internal/controller/apiextensions/composite/reconciler_test.go

test/e2e/apiextensions_test.go

phisco

LGTM! just a minor nit comment

internal/controller/apiextensions/composite/reconciler.go

sttts · 2023-09-26T14:10:04Z

@phisco merged your suggestion.

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

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

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

sttts requested review from a team and turkenh as code owners September 6, 2023 15:34

sttts requested a review from jbw976 September 6, 2023 15:34

negz marked this pull request as draft September 6, 2023 18:26

sttts force-pushed the sttts-realtime-compositions branch from 9209c6f to 3dce52d Compare September 11, 2023 06:40

sttts changed the title ~~WIP: Realtime compositor~~ Realtime compositor Sep 11, 2023

sttts marked this pull request as ready for review September 11, 2023 06:41

sttts force-pushed the sttts-realtime-compositions branch from 0bb7327 to 3dce52d Compare September 11, 2023 21:05

sttts mentioned this pull request Sep 12, 2023

controller: add TriggeredBy custom watches to engine crossplane/crossplane-runtime#535

Merged

2 tasks

sttts force-pushed the sttts-realtime-compositions branch from 3dce52d to 4c9382a Compare September 12, 2023 18:02

sttts force-pushed the sttts-realtime-compositions branch from 4c9382a to 629c52a Compare September 18, 2023 08:02

sttts force-pushed the sttts-realtime-compositions branch 2 times, most recently from 9d0b1eb to 341efb8 Compare September 18, 2023 08:30

pedjak reviewed Sep 18, 2023

View reviewed changes

internal/controller/apiextensions/composite/reconciler_test.go Show resolved Hide resolved

sttts force-pushed the sttts-realtime-compositions branch from 341efb8 to 09f4113 Compare September 18, 2023 11:06

sttts mentioned this pull request Sep 18, 2023

feat[compositions]: realtime compositor – part 2: changes to MRs #4637

Merged

5 tasks

sttts force-pushed the sttts-realtime-compositions branch 2 times, most recently from 9b580c7 to fb5f576 Compare September 18, 2023 13:01

pedjak reviewed Sep 18, 2023

View reviewed changes

sttts force-pushed the sttts-realtime-compositions branch from fb5f576 to 79366fd Compare September 19, 2023 08:01

sttts changed the title ~~Realtime compositor~~ feat[compositions]: realtime compositor Sep 19, 2023

sttts changed the title ~~feat[compositions]: realtime compositor~~ feat[compositions]: realtime compositor – part 1: changes to compositions Sep 19, 2023

sttts force-pushed the sttts-realtime-compositions branch 2 times, most recently from 40089b8 to 523061b Compare September 19, 2023 09:19

sttts force-pushed the sttts-realtime-compositions branch from 523061b to eb6c803 Compare September 19, 2023 16:50

pedjak approved these changes Sep 21, 2023

View reviewed changes

sttts force-pushed the sttts-realtime-compositions branch from eb6c803 to 18f5367 Compare September 21, 2023 12:34

phisco approved these changes Sep 26, 2023

View reviewed changes

internal/controller/apiextensions/composite/reconciler.go Outdated Show resolved Hide resolved

sttts added 2 commits September 26, 2023 16:22

go.mod

1c8e122

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

apiextensions/composite: instantly reconcile XRs when CompositionRevi…

c82abdd

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

sttts force-pushed the sttts-realtime-compositions branch from e8f198f to 15dcd77 Compare September 26, 2023 14:22

jbw976 removed their request for review September 26, 2023 14:58

e2e: test realtime composition revision selection

a4fd13a

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

sttts force-pushed the sttts-realtime-compositions branch from 15dcd77 to a4fd13a Compare September 26, 2023 15:05

phisco merged commit 89b5fe5 into crossplane:master Sep 26, 2023

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 6, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description of your changes

Uh oh!

sttts commented Sep 9, 2023

Uh oh!

negz commented Sep 15, 2023

Uh oh!

sttts commented Sep 18, 2023

Uh oh!

Uh oh!

pedjak Sep 18, 2023

Choose a reason for hiding this comment

Uh oh!

sttts Sep 19, 2023

Choose a reason for hiding this comment

Uh oh!

sttts Sep 19, 2023

Choose a reason for hiding this comment

Uh oh!

sttts commented Sep 19, 2023

Uh oh!

sttts commented Sep 19, 2023

Uh oh!

sttts commented Sep 20, 2023

Uh oh!

pedjak left a comment

Choose a reason for hiding this comment

Uh oh!

pedjak Sep 21, 2023

Choose a reason for hiding this comment

Uh oh!

sttts Sep 21, 2023

Choose a reason for hiding this comment

Uh oh!

pedjak Sep 21, 2023

Choose a reason for hiding this comment

Uh oh!

sttts Sep 21, 2023

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

phisco left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

sttts commented Sep 26, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

sttts commented Sep 6, 2023 •

edited

Loading