Skip to content

Remove deprecated call to DialContext in Hubble #34241

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
Merged
merged 1 commit into from
Oct 21, 2024
Merged

Remove deprecated call to DialContext in Hubble #34241

merged 1 commit into from
Oct 21, 2024

Conversation

davchos
Copy link
Contributor

@davchos davchos commented Aug 8, 2024 •
edited by rolinh
Loading

Remove deprecated call to DialContext in Hubble

Related:#32274, #33065

Signed-off-by: David Chosrova dchosrova@gmail.com

@davchos davchos requested a review from a team as a code owner August 8, 2024 11:29
@davchos davchos requested a review from tklauser August 8, 2024 11:29
@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 Aug 8, 2024
@github-actions github-actions bot added hubble-cli PRs or GitHub issues related with hubble-cli kind/community-contribution This was a contribution made by a community member. labels Aug 8, 2024
@davchos
Copy link
Contributor Author

davchos commented Aug 8, 2024

Hi,
This PR concern only the Hubble cli tool.
The rest is coming.
David

@tklauser tklauser added the release-note/misc This PR makes changes that have no direct user impact. label Aug 8, 2024
@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 Aug 8, 2024
@tklauser
Copy link
Member

tklauser commented Aug 8, 2024

/test

kaworu
kaworu reviewed Aug 9, 2024
View reviewed changes
@kaworu kaworu requested review from glrf and removed request for tklauser August 9, 2024 12:45
@davchos davchos force-pushed the pr/Remove-deprecated-call-to-DialContext-in-Hubble branch from 825654f to 81df16b Compare August 12, 2024 07:13
glrf
glrf reviewed Aug 12, 2024
View reviewed changes
Copy link
Contributor

@glrf glrf left a comment •
edited
Loading

Choose a reason for hiding this comment

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

LGTM overall.

My only nitpick is that the --timeout flag is now unused, which I think is fine as I think previously the timeout didn't quite work as expected anyways. The most common actual dial errors, because the server was not (yet) running, instantly failed anyways because of grpcOptionFailOnNonTempDialError.

So IMO this is fine, but we should mark it as deprecated (using ServerFlags.MarkDeprecated)

CC @kaworu (Maybe you have a different opinion. It will change the behavior of the CLI slightly)

@chancez
Copy link
Contributor

chancez commented Aug 12, 2024

Alternatively we could make --timeout an alias of --request-timeout or vice-versa.

@davchos
Copy link
Contributor Author

davchos commented Aug 13, 2024

Alternatively we could make --timeout an alias of --request-timeout or vice-versa.

Hi @chancez,
Just for my understanding , I thought that this flag was only used with grpc.Dial(), why make it an alias, and not just get rid of it ? the request timeout flag can still be used for grpc request.
Thanks for your help.

@chancez
Copy link
Contributor

chancez commented Aug 13, 2024

Alternatively we could make --timeout an alias of --request-timeout or vice-versa.

Hi @chancez, Just for my understanding , I thought that this flag was only used with grpc.Dial(), why make it an alias, and not just get rid of it ? the request timeout flag can still be used for grpc request. Thanks for your help.

By removing grpc.WithBlock(), we've effectively moved dialing to the RPC calls. The underlying behavior is semantically different, but the underlying purpose for the timeout is the same. There's no longer distinguishing dials from RPC calls since gRPC will handle dialing transparently as needed. The reason to keep the existing flag, and make it an alias to to minimize disruption to any users currently using --timeout.

@chancez chancez linked an issue Aug 21, 2024 that may be closed by this pull request
Replace deprecated gRPC Dial options in Hubble sub-systems #33065
Closed
@chancez chancez removed a link to an issue Aug 21, 2024
Replace deprecated gRPC Dial options in Hubble sub-systems #33065
Closed
@davchos davchos force-pushed the pr/Remove-deprecated-call-to-DialContext-in-Hubble branch from 81df16b to 0357a39 Compare August 21, 2024 18:21
@kaworu
Copy link
Member

kaworu commented Sep 2, 2024 •
edited
Loading

The reason to keep the existing flag, and make it an alias to to minimize disruption to any users currently using --timeout.

Looking into it

  • --request-timeout is only applicable to unary gRPC calls (see here and here) — notably excluding hubble observe flows / GetFlows
  • --timeout applies for all gRPC calls

If correct, then I don't think aliasing --timeout to --request-timeout make sense as the set of commands using the latter is a subset of commands using the former. We have to think about Hubble Relay as well, which has a --dial-timeout flag without a request timeout.

So I would prefer removing --timeout and --dial-timeout from the Hubble CLI and Hubble Relay, respectively. Also hiding --request-timeout from the CLI for streaming rpc calls (i.e. hubble observe …) would be nice on the way, but not necessary to get the PR merged.

@dylandreimerink
Copy link
Member

/test

@davchos davchos force-pushed the pr/Remove-deprecated-call-to-DialContext-in-Hubble branch from 0357a39 to 96b5176 Compare September 11, 2024 11:59
@dylandreimerink
Copy link
Member

/test

@davchos davchos force-pushed the pr/Remove-deprecated-call-to-DialContext-in-Hubble branch from 96b5176 to 9a3d23a Compare September 12, 2024 11:59
@chancez
Copy link
Contributor

chancez commented Sep 24, 2024

/test

@aanm aanm force-pushed the pr/Remove-deprecated-call-to-DialContext-in-Hubble branch from 9a3d23a to d3d6e63 Compare October 11, 2024 09:54
@aanm
Copy link
Member

aanm commented Oct 11, 2024

/test

@aanm aanm requested a review from glrf October 11, 2024 09:54
@aanm aanm requested a review from kaworu October 11, 2024 09:54
@aanm aanm enabled auto-merge October 11, 2024 09:54
@davchos @kaworu @aanm
Remove deprecated call to DialContext in Hubble
b44c4a7 
Related:cilium#32274

Update hubble/cmd/common/conn/conn.go

Co-authored-by: Alexandre Perrin <alex@kaworu.ch>
Signed-off-by: davchos <dchosrova@gmail.com>
@aanm aanm force-pushed the pr/Remove-deprecated-call-to-DialContext-in-Hubble branch from d3d6e63 to b44c4a7 Compare October 21, 2024 09:56
@aanm
Copy link
Member

aanm commented Oct 21, 2024

/test

@aanm aanm added this pull request to the merge queue Oct 21, 2024
Merged via the queue into cilium:main with commit 14c65f9 Oct 21, 2024
63 checks passed
@devodev devodev mentioned this pull request Nov 18, 2024
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@kaworu kaworu kaworu approved these changes

@glrf glrf Awaiting requested review from glrf

Assignees
No one assigned
Labels
hubble-cli PRs or GitHub issues related with hubble-cli kind/community-contribution This was a contribution made by a community member. release-note/misc This PR makes changes that have no direct user impact.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
8 participants
@davchos @tklauser @chancez @kaworu @dylandreimerink @aanm @glrf @rolinh