-
Notifications
You must be signed in to change notification settings - Fork 897
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat[compositions]: realtime compositor – part 2: changes to MRs #4637
feat[compositions]: realtime compositor – part 2: changes to MRs #4637
Conversation
f0fb10b
to
b0f0ed8
Compare
Adding a reference to #4316, which I think is related. |
b0f0ed8
to
5858c89
Compare
3642e40
to
a0e8a69
Compare
ba73b18
to
3197040
Compare
10c91b7
to
b74ec8d
Compare
43bfdb7
to
4e3fd74
Compare
for _, ref := range xr.GetResourceReferences() { | ||
gvks = append(gvks, ref.GroupVersionKind()) | ||
} | ||
r.kindObserver.WatchComposedResources(gvks...) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can't I guess. 1. environments 2. functions relying on 60s poll.
Should we do everything towards removing poll? Yes, please. @negz
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That will be step 3: #4822
4e3fd74
to
7cf75af
Compare
@@ -53,6 +69,9 @@ func TestCompositionMinimal(t *testing.T) { | |||
)). | |||
Assess("CreateClaim", funcs.AllOf( | |||
funcs.ApplyResources(FieldManager, manifests, "claim.yaml"), | |||
funcs.InBackground(funcs.LogResources(nopList)), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
how is this change related to the work done in this PR? I looks like a leftover of some debugging?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
)). | ||
Assess("CreateClaim", funcs.AllOf( | ||
funcs.ApplyResources(FieldManager, manifests, "claim.yaml"), | ||
funcs.InBackground(funcs.LogResources(nopList, withTestLabels)), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
InBackground
calls are for debugging? Could we make them implicit, for example call them within ApplyResource
, so that all tests can automatically have benefits?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ApplyResource could become a bit smarter and understand that we are applying claim, and then via references follow to composite and MRs, or?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
PR welcome for an automatic related object tracker :-)
Signed-off-by: Dr. Stefan Schimanski <stefan.schimanski@upbound.io>
Signed-off-by: Dr. Stefan Schimanski <stefan.schimanski@upbound.io>
Signed-off-by: Dr. Stefan Schimanski <stefan.schimanski@upbound.io>
Signed-off-by: Dr. Stefan Schimanski <stefan.schimanski@upbound.io>
7cf75af
to
2348236
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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>
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>
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>
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>
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:
tl/dr:
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.I have:
make reviewable
to ensure this PR is ready for review.Addedbackport release-x.y
labels to auto-backport this PR, if necessary.Opened a PR updating the docs, if necessary.