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

cmd/observe: Add --first to support querying for earlier flows and events #719

Merged
merged 1 commit into from
May 31, 2022

Conversation

chancez
Copy link
Contributor

@chancez chancez commented May 13, 2022

Depends on cilium/cilium#19819

@gandro
Copy link
Member

gandro commented May 30, 2022

@chancez I'd like to get this into the next release. Now that the upstream PR has been merged, do you think you could rebase this?

@rolinh rolinh added the :basecamp: needs-rebase This PR needs to be rebased because it has merge conflicts. label May 30, 2022
@chancez
Copy link
Contributor Author

chancez commented May 31, 2022

@gandro I was waiting for a new release branch to be cut that has my changes so I can update the reference to vendor. Do we have a branch I can update the vendored cilium/cilium to yet?

@rolinh
Copy link
Member

rolinh commented May 31, 2022

@chancez No need for a new release branch to be cut. Just pick latest master, we'll take care of updating the dependency to the next RC once it's out.

@chancez chancez force-pushed the pr/chancez/hubble_observe_first branch from fd65e67 to e38d82d Compare May 31, 2022 16:21
@chancez
Copy link
Contributor Author

chancez commented May 31, 2022

@gandro @rolinh ok I rebased & updated the vendored cilium dependency to use master. PTAL.

@chancez chancez marked this pull request as ready for review May 31, 2022 16:22
@chancez chancez requested a review from a team as a code owner May 31, 2022 16:22
@chancez chancez requested review from a team and rolinh and removed request for a team May 31, 2022 16:22
@chancez
Copy link
Contributor Author

chancez commented May 31, 2022

How I tested:

I ran the following two commands at "roughly the same time" and they typically contained the exact same 5 flows (if I ran them "at the same time"). This required building cilium & Hubble-relay from master and installing cilium with those images.

./hubble observe --all | head -n 5
./hubble observe --first 5

Copy link
Member

@gandro gandro left a comment

Choose a reason for hiding this comment

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

Perfect, thanks!

@gandro gandro added ⌨️ area/cli Impacts the command line interface of any command in the repository. release-note/major This PR introduces major new functionality to Hubble. and removed :basecamp: needs-rebase This PR needs to be rebased because it has merge conflicts. dont-merge/needs-release-note-label PR is blocked until the release note is set labels May 31, 2022
Copy link
Member

@rolinh rolinh left a comment

Choose a reason for hiding this comment

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

LGTM except that copy paste type in the flag help message.

@@ -198,6 +198,7 @@ more.`,
selectorFlags := pflag.NewFlagSet("selectors", pflag.ContinueOnError)
selectorFlags.BoolVar(&selectorOpts.all, "all", false, "Get all flows stored in Hubble's buffer")
selectorFlags.Uint64Var(&selectorOpts.last, "last", 0, fmt.Sprintf("Get last N flows stored in Hubble's buffer (default %d)", defaults.FlowPrintCount))
selectorFlags.Uint64Var(&selectorOpts.first, "first", 0, fmt.Sprintf("Get last N flows stored in Hubble's buffer (default %d)", defaults.FlowPrintCount))
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
selectorFlags.Uint64Var(&selectorOpts.first, "first", 0, fmt.Sprintf("Get last N flows stored in Hubble's buffer (default %d)", defaults.FlowPrintCount))
selectorFlags.Uint64Var(&selectorOpts.first, "first", 0, fmt.Sprintf("Get first N flows stored in Hubble's buffer (default %d)", defaults.FlowPrintCount))

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oof, good catch. I'm embarrassed by that one.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed

…ents

Also updates vendored cilium/cilium to pickup new First field in observer APIs

Signed-off-by: Chance Zibolski <chance.zibolski@gmail.com>
@chancez chancez force-pushed the pr/chancez/hubble_observe_first branch from e38d82d to 8cd73fa Compare May 31, 2022 18:45
@maintainer-s-little-helper maintainer-s-little-helper bot added the ready-to-merge This PR has passed all tests and received consensus from code owners to merge. label May 31, 2022
@rolinh rolinh merged commit 5508dd9 into cilium:master May 31, 2022
@chancez chancez deleted the pr/chancez/hubble_observe_first branch May 31, 2022 20:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
⌨️ area/cli Impacts the command line interface of any command in the repository. ready-to-merge This PR has passed all tests and received consensus from code owners to merge. release-note/major This PR introduces major new functionality to Hubble.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants