Skip to content
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

resource: Make the resource lazy by default #21862

Merged
merged 1 commit into from
Oct 28, 2022

Conversation

joamaki
Copy link
Contributor

@joamaki joamaki commented Oct 24, 2022

For some resource kinds we only have observers if a feature has been enabled. Rather than guarding the creating of resource based on a set of "enabled" flags, make the resource lazy and only start the informer when the first observer (or call to Store()) is seen.

@joamaki joamaki requested a review from a team as a code owner October 24, 2022 14:07
@maintainer-s-little-helper maintainer-s-little-helper bot added the dont-merge/needs-release-note-label The author needs to describe the release impact of these changes. label Oct 24, 2022
@joamaki joamaki requested a review from squeed October 24, 2022 14:10
@joamaki
Copy link
Contributor Author

joamaki commented Oct 24, 2022

The practical use-case for this is: https://github.com/cilium/cilium/pull/21764/files#r1003189088

@joamaki joamaki added the release-note/misc This PR makes changes that have no direct user impact. label Oct 24, 2022
@maintainer-s-little-helper maintainer-s-little-helper bot removed the dont-merge/needs-release-note-label The author needs to describe the release impact of these changes. label Oct 24, 2022
@@ -135,9 +143,34 @@ func (r *resource[T]) pushDelete(lastState any) {
}

func (r *resource[T]) Start(startCtx hive.HookContext) error {
r.wg.Add(1)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we even really need Start() anymore?

Copy link
Contributor Author

@joamaki joamaki Oct 24, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We still want this to wait until being told to start, e.g. we don't want "cilium-agent objects" inspection to actually start pulling data from api-server, and I don't see any reason to forbid Observe calls from constructors.

For some resource kinds we only have observers if a feature has been
enabled. Rather than guarding the creating of resource based on a set
of "enabled" flags, make the resource lazy and only start the informer
when the first observer (or call to Store()) is seen.

Signed-off-by: Jussi Maki <jussi@isovalent.com>
@joamaki joamaki requested a review from squeed October 25, 2022 10:39
@joamaki joamaki added the ready-to-merge This PR has passed all tests and received consensus from code owners to merge. label Oct 26, 2022
@joamaki
Copy link
Contributor Author

joamaki commented Oct 28, 2022

/test not needed as pkg/k8s/resource not yet used anywhere.

@nbusseneau nbusseneau merged commit 4574392 into cilium:master Oct 28, 2022
@christarazi christarazi added area/daemon Impacts operation of the Cilium daemon. sig/k8s Impacts the kubernetes API, or kubernetes -> cilium internals translation layers. labels Oct 29, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/daemon Impacts operation of the Cilium daemon. ready-to-merge This PR has passed all tests and received consensus from code owners to merge. release-note/misc This PR makes changes that have no direct user impact. sig/k8s Impacts the kubernetes API, or kubernetes -> cilium internals translation layers.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants