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

Get rid of deprecated grpc options #1455

Open
glazychev-art opened this issue May 10, 2023 · 3 comments
Open

Get rid of deprecated grpc options #1455

glazychev-art opened this issue May 10, 2023 · 3 comments
Labels
enhancement New feature or request

Comments

@glazychev-art
Copy link
Contributor

Expected Behavior

All applications use the same grpc options

Current Behavior

Some cmds use a different set of options

Failure Information (for bugs)

Need to figure out how grpc.WaitForReady() works (and possibly delete it), add tracing interceptors.
We have to check networkservice and registry chains.

Example of correct option sets.
Client:
https://github.com/networkservicemesh/cmd-nsmgr/blob/3767a9554e633ace231e600512354740071f5519/internal/manager/manager.go#L134-L146

Server:
https://github.com/networkservicemesh/cmd-nsmgr/blob/3767a9554e633ace231e600512354740071f5519/internal/manager/manager.go#L159-L166

Context

  • Kubernetes Version:
  • etc.

Failure Logs

@d-uzlov d-uzlov moved this to In Progress in Release v1.10.0 May 10, 2023
@glazychev-art glazychev-art moved this from In Progress to Under review in Release v1.10.0 May 24, 2023
@denis-tingaikin denis-tingaikin moved this from Under review to Moved to next release in Release v1.10.0 Aug 2, 2023
@NikitaSkrynnik NikitaSkrynnik moved this to Under review in Release v1.11.0 Aug 7, 2023
@glazychev-art
Copy link
Contributor Author

It looks like grpc.WithBlock() is anti-pattern - https://pkg.go.dev/google.golang.org/grpc#WithBlock
We need to consider whether we can refuse it. As far as I remember, it is worth paying attention to DialTimeout.

@NikitaSkrynnik NikitaSkrynnik moved this from Under review to In Progress in Release v1.11.0 Aug 10, 2023
@denis-tingaikin denis-tingaikin moved this from In Progress to Todo in Release v1.11.0 Aug 15, 2023
@denis-tingaikin
Copy link
Member

denis-tingaikin commented Sep 28, 2023

I think we need get rid of using deperecated options

@denis-tingaikin denis-tingaikin changed the title Make the use of grpc options consistent across all cmds Get rid of deprecated grpc options Sep 28, 2023
@bellycat77 bellycat77 moved this from Todo to In Progress in Release v1.11.0 Oct 3, 2023
@bellycat77 bellycat77 moved this from In Progress to Under review in Release v1.11.0 Oct 6, 2023
@bellycat77 bellycat77 moved this from Under review to Blocked in Release v1.11.0 Oct 6, 2023
@denis-tingaikin denis-tingaikin moved this from Blocked to Under review in Release v1.11.0 Oct 8, 2023
@NikitaSkrynnik NikitaSkrynnik moved this to Moved to next release in Release v1.11.0 Oct 9, 2023
@NikitaSkrynnik NikitaSkrynnik moved this to Blocked in Release v1.12.0 Oct 18, 2023
@NikitaSkrynnik NikitaSkrynnik moved this from Blocked to Todo in Release v1.12.0 Nov 7, 2023
@dualBreath dualBreath moved this from Todo to In Progress in Release v1.12.0 Nov 8, 2023
@denis-tingaikin denis-tingaikin moved this from In Progress to Blocked in Release v1.12.0 Nov 16, 2023
@NikitaSkrynnik
Copy link
Contributor

NikitaSkrynnik commented Jan 12, 2024

Current State

We found that there are only a few cases where NSM doesn't work without grpc.WithBlock().

Without grpc.WithBlock() NSM usually fails on heal tests where we delete remote nsmgr. In this case the forwarder cannot connect to the NSE because it has IP address of the deleted remote nsmgr. Usually, grpc immediatelly returns an error if we try to dial to the server which doesn't listen on a port (even without grpc.WithBlock()). But in case of the dead remote nsmgr we don't have this server and IP on the cluster at all and grpc hangs forever or until timeout set via grpc.WaitForReady() option.

This article explains this grpc behavior in detail.

@denis-tingaikin denis-tingaikin moved this from Blocked to Moved to next release in Release v1.12.0 Jan 30, 2024
@denis-tingaikin denis-tingaikin moved this to In Progress in Release v1.14.0 Apr 9, 2024
@denis-tingaikin denis-tingaikin moved this from In Progress to Todo in Release v1.14.0 Apr 24, 2024
@denis-tingaikin denis-tingaikin removed their assignment Sep 24, 2024
@denis-tingaikin denis-tingaikin added the enhancement New feature or request label Sep 24, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
Status: Moved to next release
Status: Moved to next release
Status: No status
Development

No branches or pull requests

3 participants