WIP: Fix several issues in realtime-compositions #5422
Conversation
sttts
force-pushed
the
sttts-realtime-compositions-xrrev-race
branch
from
5e40e58
to
fddd833
Compare
| @@ -712,6 +712,7 @@ func EnqueueForCompositionRevisionFunc(of resource.CompositeKind, list func(ctx | |||
| 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)) | |||
There was a problem hiding this comment.
It might be useful to include the GVK of the XRs that will be enqueued, like the log line above.
| @@ -63,12 +70,16 @@ type composedResourceInformers struct { | |||
| // composites referencing a certain composed resource GVK. If no composite | |||
| // is left doing so, a composed resource informer is stopped. | |||
| 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 | |||
| 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)) |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
have switched to gvk.
| @@ -105,15 +116,34 @@ func (i *composedResourceInformers) Start(ctx context.Context, h handler.EventHa | |||
|
|
|||
| // 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. | |||
There was a problem hiding this comment.
Is this saying that it's safe to call this method before the cache is synced? It reads a little ambiguously to me.
| if i.xrCaches == nil { | ||
| i.xrCaches = make(map[schema.GroupVersionKind]cache.Cache) | ||
| } | ||
| i.xrCaches[gvk] = ca | ||
|
|
||
| // asynchronously mark the cache as synced. We won't list XRs before this |
There was a problem hiding this comment.
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.
| 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), |
There was a problem hiding this comment.
Style nit: This is a bit of a handful to read due to the nested function definitions with long signatures.
There was a problem hiding this comment.
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, ...).
sttts
force-pushed
the
sttts-realtime-compositions-xrrev-race
branch
2 times, most recently
from
82136ca
to
199edad
Compare
sttts
changed the title
sttts
force-pushed
the
sttts-realtime-compositions-xrrev-race
branch
4 times, most recently
from
26a1b40
to
d8b23b9
Compare
sttts
force-pushed
the
sttts-realtime-compositions-xrrev-race
branch
from
d8b23b9
to
2aa0c76
Compare
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>
sttts
force-pushed
the
sttts-realtime-compositions-xrrev-race
branch
from
2aa0c76
to
6cf5e57
Compare
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
|
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 💪 |
|
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 😉 |
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 controllersmerged through apiextension/definition: don't attempt to start composite controllers multiple times #5437apiextensions/definition: fix leak of watch on composition revisions due to wrong referenced cachemoved to apiextensions/definition: fix leak of watch on composition revisions due to wrong referenced cache #5463apiextensions/definition: unregister composite gvk from composedResourceInformers everywheremerged through apiextension/definition: don't attempt to start composite controllers multiple times #5437Depends on crossplane/crossplane-runtime#672.Fixes #5400
I have:
make reviewableto ensure this PR is ready for review.[ ] Added or updated unit tests.[ ] Linked a PR or a docs tracking issue to document this change.[ ] Addedbackport release-x.ylabels to auto-backport this PR.Need help with this checklist? See the cheat sheet.