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.

Sign up for GitHub

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

Jump to bottom

Enable metadata service to track the events from a subset of namespaces #1368

Merged
merged 10 commits into from
Jul 14, 2023
Merged

Enable metadata service to track the events from a subset of namespaces #1368

merged 10 commits into from
Jul 14, 2023

Conversation

MrAta
Copy link
Contributor

@MrAta MrAta commented May 25, 2023 •
edited by JamesMBartlett
Loading

Summary: Enable metadata service to track the events from a subset of namespaces when desired, instead of tracking the events from the entire cluster.

Relevant Issues: #1364

Type of change: /kind feature

Test Plan: Added unit tests for the new behaviour. @JamesMBartlett tested by skaffolding a dev vizier version to a cluster both with and without metadata_namespaces selected, and both behaved as expected.

@aimichelle aimichelle requested a review from vihangm May 25, 2023 17:48
@MrAta MrAta marked this pull request as ready for review May 25, 2023 18:03
@MrAta MrAta had a problem deploying to pr-actions-approval June 29, 2023 14:26 — with GitHub Actions Error
@MrAta MrAta had a problem deploying to pr-actions-approval June 29, 2023 14:54 — with GitHub Actions Error
@MrAta MrAta requested a review from vihangm June 29, 2023 14:55
@MrAta MrAta had a problem deploying to pr-actions-approval June 29, 2023 15:09 — with GitHub Actions Error
@MrAta MrAta had a problem deploying to pr-actions-approval June 29, 2023 16:01 — with GitHub Actions Error
@MrAta MrAta temporarily deployed to pr-actions-approval June 29, 2023 18:11 — with GitHub Actions Inactive
@MrAta MrAta temporarily deployed to pr-actions-approval June 29, 2023 20:10 — with GitHub Actions Inactive
@MrAta MrAta requested a review from vihangm June 29, 2023 20:10
@vihangm
Copy link
Member

vihangm commented Jul 1, 2023

This looks mostly ok, but I think #1595 will actually make this easier by allowing us to filter in only one location (i.e. at the SharedIndexInformerFactory creation).

Also we should see if it's possible to add some testing coverage to ensure that the namespace restriction works as expected.

@MrAta MrAta had a problem deploying to pr-actions-approval July 2, 2023 02:44 — with GitHub Actions Error
@MrAta MrAta had a problem deploying to pr-actions-approval July 2, 2023 02:45 — with GitHub Actions Error
@MrAta
Copy link
Contributor Author

MrAta commented Jul 2, 2023

Thanks for the insights, @vihangm!
Re #1595, either of the PRs will need a rebase and conflict resolution once the other one is merged :D I'm not sure which way is more convenient, up to you.

Re the test case, I just added a simple test case for Pod creation only. Please let me know if it sounds right to you and I can add other resource types and events too.

@MrAta MrAta temporarily deployed to pr-actions-approval July 2, 2023 02:52 — with GitHub Actions Inactive
@vihangm
Copy link
Member

vihangm commented Jul 2, 2023

Test case looks good. Adding a little bit of simple coverage on all resources types would be nice.

@MrAta MrAta temporarily deployed to pr-actions-approval July 4, 2023 07:26 — with GitHub Actions Inactive
@MrAta
Copy link
Contributor Author

MrAta commented Jul 4, 2023 •
edited
Loading

@vihangm added more test cases for other resource types.

@MrAta MrAta temporarily deployed to pr-actions-approval July 6, 2023 19:46 — with GitHub Actions Inactive
JamesMBartlett
JamesMBartlett approved these changes Jul 13, 2023
View reviewed changes
Copy link
Member

@JamesMBartlett JamesMBartlett left a comment

Choose a reason for hiding this comment

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

LGTM, discussed with @vihangm who's out at the moment and he said its good to go from his end. So if we can get the merge conflicts fixed we should be good to go. Sorry for the delay

@vihangm
Copy link
Member

vihangm commented Jul 13, 2023

Also can you update the test plan in the PR since this now has unit tests (and also should be tested with a dev deploy of vizier before merging).

@JamesMBartlett
Copy link
Member

I tested this out on a cluster, both with all namespaces and with a subset of namespaces and both seemed to work as expected.

@MrAta MrAta had a problem deploying to pr-actions-approval July 14, 2023 00:29 — with GitHub Actions Error
MrAta added 10 commits July 13, 2023 17:45
@MrAta
Add namespace to resource informers
e5f5c47 
Signed-off-by: Ata Fatahi <afatahibaarzi@linkedin.com>
Signed-off-by: Ata Fatahi <immrata@gmail.com>
@MrAta
Create controller with namespaces
eadf257 
Signed-off-by: Ata Fatahi <afatahibaarzi@linkedin.com>
Signed-off-by: Ata Fatahi <immrata@gmail.com>
@MrAta
Get namespaces from METADATA_NAMESPACES env var
93d2f9c 
Signed-off-by: Ata Fatahi <afatahibaarzi@linkedin.com>
Signed-off-by: Ata Fatahi <immrata@gmail.com>
@MrAta
add k8s v1 api dep
cce8071 
Signed-off-by: Ata Fatahi <afatahibaarzi@linkedin.com>
Signed-off-by: Ata Fatahi <immrata@gmail.com>
@MrAta
use StringSlice for namespaces
f87811f 
Signed-off-by: Ata Fatahi <afatahibaarzi@linkedin.com>
Signed-off-by: Ata Fatahi <immrata@gmail.com>
@MrAta
pass namespaces to the new controller
17ab1d4 
Signed-off-by: Ata Fatahi <afatahibaarzi@linkedin.com>
Signed-off-by: Ata Fatahi <immrata@gmail.com>
@MrAta
restric inital pods to the configured namespaces in metadata service
6beba41 
Signed-off-by: Ata Fatahi <afatahibaarzi@linkedin.com>
Signed-off-by: Ata Fatahi <immrata@gmail.com>
@MrAta
add test case
f86a433 
Signed-off-by: Ata Fatahi <afatahibaarzi@linkedin.com>
Signed-off-by: Ata Fatahi <immrata@gmail.com>
@MrAta
add more test coverage
b3fc1b1 
Signed-off-by: Ata Fatahi <afatahibaarzi@linkedin.com>
Signed-off-by: Ata Fatahi <immrata@gmail.com>
@MrAta
fix lint issues
a731173 
Signed-off-by: Ata Fatahi <afatahibaarzi@linkedin.com>
Signed-off-by: Ata Fatahi <immrata@gmail.com>
@MrAta MrAta temporarily deployed to pr-actions-approval July 14, 2023 00:45 — with GitHub Actions Inactive
@JamesMBartlett JamesMBartlett dismissed vihangm’s stale review July 14, 2023 01:02

Vihang's out but he's approved of these changes.

@JamesMBartlett JamesMBartlett merged commit 2e7b467 into pixie-io:main Jul 14, 2023
@aimichelle aimichelle mentioned this pull request Jul 21, 2023
Merged
aimichelle added a commit that referenced this pull request Jul 21, 2023
@aimichelle
Fix default metadata namespaces (#1633)
0a41aaa 
Summary: #1368 added the ability to limit the metadata service to
specific namespaces. By default, if no namespaces are mentioned, the
flag is supposed to choose all namespaces.
However, if the environment variable is not specified, it is actually
parsed as an empty list and the default is not properly chosen. Instead,
we can assume no users actually want to watch 0 namespaces, and set it
to all namespaces when that occurs.

Relevant Issues: N/A

Type of change: /kind bug

Test Plan: Skaffold deploy metadata service

Signed-off-by: Michelle Nguyen <michellenguyen@pixielabs.ai>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@JamesMBartlett JamesMBartlett JamesMBartlett approved these changes

@vihangm vihangm Awaiting requested review from vihangm

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@MrAta @vihangm @JamesMBartlett