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

Add status watch label filter #639

Merged
merged 1 commit into from
May 21, 2024

Conversation

karlkfi
Copy link
Contributor

@karlkfi karlkfi commented May 8, 2024

Using a label filter significantly cuts down on watch events and memory used by the informer's watch cache. But you'll need to set the labels on the objects yourself before providing them to the applier.

I've tested it with Config Sync end-to-end tests and shown that it significantly reduces memory usage when there are other objects on the cluster with resource types and namespaces that overlap with the apply set being watched. This is because the informer caches objects it receives events for, even if they're not in the apply set.

Unfortunately, FakeDynamicClient (client-go) only supports list label selector, not watch label selectors. So the unit tests added here aren't particularly useful. We will need to upstream changes to client-go to make the unit tests more functional. But that shouldn't be a blocker.

I added field selectors as well, but these are even harder to test than label selectors, because the FakeDynamicClient doesn't support them at all. It's also not super clear how indexers relate to label filters, since the indexer is only used client-side and the label filter is server-side (i think).

@k8s-ci-robot k8s-ci-robot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels May 8, 2024
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: karlkfi

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@k8s-ci-robot k8s-ci-robot requested review from ash2k and seans3 May 8, 2024 23:09
@k8s-ci-robot k8s-ci-robot added approved Indicates a PR has been approved by an approver from all required OWNERS files. size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels May 8, 2024
@karlkfi karlkfi requested a review from mortent May 8, 2024 23:29
@karlkfi karlkfi force-pushed the karl-watch-filter branch from 688f8e0 to e2fc183 Compare May 9, 2024 23:15
@karlkfi karlkfi changed the title [WIP] Add status watch label filter Add status watch label filter May 9, 2024
@k8s-ci-robot k8s-ci-robot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels May 9, 2024
@karlkfi karlkfi force-pushed the karl-watch-filter branch 2 times, most recently from 61d8f5a to 914ed15 Compare May 10, 2024 03:26
@karlkfi karlkfi force-pushed the karl-watch-filter branch from 914ed15 to d65e28e Compare May 20, 2024 23:46
@karlkfi
Copy link
Contributor Author

karlkfi commented May 20, 2024

I've started a rather large upstream refactor to make field selection reproducible in the upstream fake clients: kubernetes/kubernetes#124801

However, even if that merges, we won't be able to update the k8s version here until it's published and has a chance to get into at least the rapid and maybe the standard channels of GKE. So I'd like to merge this PR without ideal coverage of the field selectors. This should be good enough, since we're only really planning to use the label selectors in Config Sync. Then I can circle back later and update the tests to better validate the field selectors after the upstream PR merges.

@karlkfi karlkfi force-pushed the karl-watch-filter branch from d65e28e to 6dd25f9 Compare May 20, 2024 23:53
Copy link
Member

@mortent mortent left a comment

Choose a reason for hiding this comment

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

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label May 21, 2024
Using a label filter significantly cuts down on watch events and
memory used by the informer's watch cache. But you'll need to set
the labels on the objects yourself before providing them to the
applier.
@karlkfi karlkfi force-pushed the karl-watch-filter branch from 6dd25f9 to fe123cb Compare May 21, 2024 16:54
@k8s-ci-robot k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label May 21, 2024
@mortent
Copy link
Member

mortent commented May 21, 2024

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label May 21, 2024
@k8s-ci-robot k8s-ci-robot merged commit 9702fb8 into kubernetes-sigs:master May 21, 2024
5 checks passed
@karlkfi karlkfi deleted the karl-watch-filter branch May 22, 2024 20:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. lgtm "Looks good to me", indicates that a PR is ready to be merged. size/XL Denotes a PR that changes 500-999 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants