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

Encapsulating choices of context and dial options in a library is an anti-pattern. That should be driven from top level. Lets not do that here. #446

Merged
merged 1 commit into from
Aug 21, 2020
Merged

Encapsulating choices of context and dial options in a library is an anti-pattern. That should be driven from top level. Lets not do that here. #446

merged 1 commit into from
Aug 21, 2020

Conversation

edwarnicke
Copy link
Member

No description provided.

@edwarnicke
Encapsulating choices of context and dial options in a library is an …
4fced0c 
…anti-pattern.

That should be driven from top level.  Lets not do that here.

Signed-off-by: Ed Warnicke <[email protected]>
@edwarnicke edwarnicke mentioned this pull request Aug 20, 2020
Closed
edwarnicke
edwarnicke commented Aug 20, 2020
View reviewed changes
pkg/tools/grpcutils/dial.go
// This function may be useful in the environments when not guarantee that the server socket is immediately provided.
func DialContextTimeout(ctx context.Context, target string, duration time.Duration, opts ...grpc.DialOption) (conn *grpc.ClientConn, err error) {
timeout := time.After(duration)
for {
Copy link
Member Author

Choose a reason for hiding this comment

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

Among other issues... this forces any DialContext to block until success. Since most uses of DialContext should not do that... this is inappropriate in a drop in replacement for DialContext.

edwarnicke
edwarnicke commented Aug 20, 2020
View reviewed changes
pkg/tools/grpcutils/dial.go
// DialContext tries to create a client connection to the given target during 1 minute.
// This function may be useful in the environments when not guarantee that the server socket is immediately provided.
func DialContext(ctx context.Context, target string, opts ...grpc.DialOption) (*grpc.ClientConn, error) {
return DialContextTimeout(ctx, target, time.Minute, opts...)
Copy link
Member Author

Choose a reason for hiding this comment

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

Timeout choices should come from outside... from the main or from the context of a call... not from a library.

@denis-tingaikin
Copy link
Member

denis-tingaikin commented Aug 20, 2020 •
edited
Loading

@edwarnicke ok, How then we should handle a case when we are using unix based client/server + k8s slow file mount API? Should we add in each cmd application this removed waiting?

denis-tingaikin
denis-tingaikin approved these changes Aug 21, 2020
View reviewed changes
Copy link
Member

@denis-tingaikin denis-tingaikin left a comment

Choose a reason for hiding this comment

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

I've tested different setups and here is my result:

Setup

  1. Deploy spire
  2. Deploy any unix based grpc server with TLS
  3. Deploy grpc client with TLS for the server from step 2.

Testing

Using grpc.WaitForReady

I've tried to replace grpcuitls.DialContext to grpc.DialContext and added grpc.WaitFoReady(true) on first call. In this case, 5/10 my tries client's POD freezes on the first API call. So I think we can not use grpc.WaitFoReady(true) in k8s with unix based connections

Using grpc.Withblock

I've tried to replace grpcuitls.DialContext to grpc.DialContext and added grpc.Block() into Dial function. This variant working correctly

Conclution

We can remove this dial function and use grpc.WithBlock() option in each our grpc client.

@denis-tingaikin denis-tingaikin merged commit 1be3f50 into networkservicemesh:master Aug 21, 2020
nsmbot pushed a commit to networkservicemesh/cmd-registry-memory that referenced this pull request Aug 21, 2020
Update go.mod and go.sum to latest version from networkservicemesh/sd…
a1f974e 
…k@master networkservicemesh/sdk#446

networkservicemesh/sdk PR link: networkservicemesh/sdk#446

networkservicemesh/sdk commit message:
commit 1be3f50d7da793ed1ff063c88e26e9c94bdfd44c
Author: Ed Warnicke <[email protected]>
Date:   Fri Aug 21 01:55:31 2020 -0500

    Encapsulating choices of context and dial options in a library is an anti-pattern. (#446)

    That should be driven from top level.  Lets not do that here.

    Signed-off-by: Ed Warnicke <[email protected]>

Signed-off-by: NSMBot <[email protected]>
nsmbot pushed a commit to networkservicemesh/cmd-nsmgr that referenced this pull request Aug 21, 2020
Update go.mod and go.sum to latest version from networkservicemesh/sd…
47e59d0 
…k@master networkservicemesh/sdk#446

networkservicemesh/sdk PR link: networkservicemesh/sdk#446

networkservicemesh/sdk commit message:
commit 1be3f50d7da793ed1ff063c88e26e9c94bdfd44c
Author: Ed Warnicke <[email protected]>
Date:   Fri Aug 21 01:55:31 2020 -0500

    Encapsulating choices of context and dial options in a library is an anti-pattern. (#446)

    That should be driven from top level.  Lets not do that here.

    Signed-off-by: Ed Warnicke <[email protected]>

Signed-off-by: NSMBot <[email protected]>
nsmbot pushed a commit to networkservicemesh/cmd-registry-proxy-dns that referenced this pull request Aug 21, 2020
Update go.mod and go.sum to latest version from networkservicemesh/sd…
21a7d0f 
…k@master networkservicemesh/sdk#446

networkservicemesh/sdk PR link: networkservicemesh/sdk#446

networkservicemesh/sdk commit message:
commit 1be3f50d7da793ed1ff063c88e26e9c94bdfd44c
Author: Ed Warnicke <[email protected]>
Date:   Fri Aug 21 01:55:31 2020 -0500

    Encapsulating choices of context and dial options in a library is an anti-pattern. (#446)

    That should be driven from top level.  Lets not do that here.

    Signed-off-by: Ed Warnicke <[email protected]>

Signed-off-by: NSMBot <[email protected]>
nsmbot pushed a commit to networkservicemesh/sdk-kernel that referenced this pull request Aug 21, 2020
Update go.mod and go.sum to latest version from networkservicemesh/sd…
764d7cb 
…k@master networkservicemesh/sdk#446

networkservicemesh/sdk PR link: networkservicemesh/sdk#446

networkservicemesh/sdk commit message:
commit 1be3f50d7da793ed1ff063c88e26e9c94bdfd44c
Author: Ed Warnicke <[email protected]>
Date:   Fri Aug 21 01:55:31 2020 -0500

    Encapsulating choices of context and dial options in a library is an anti-pattern. (#446)

    That should be driven from top level.  Lets not do that here.

    Signed-off-by: Ed Warnicke <[email protected]>

Signed-off-by: NSMBot <[email protected]>
nsmbot pushed a commit to networkservicemesh/sdk-vppagent that referenced this pull request Aug 21, 2020
Update go.mod and go.sum to latest version from networkservicemesh/sd…
20d2705 
…k@master networkservicemesh/sdk#446

networkservicemesh/sdk PR link: networkservicemesh/sdk#446

networkservicemesh/sdk commit message:
commit 1be3f50d7da793ed1ff063c88e26e9c94bdfd44c
Author: Ed Warnicke <[email protected]>
Date:   Fri Aug 21 01:55:31 2020 -0500

    Encapsulating choices of context and dial options in a library is an anti-pattern. (#446)

    That should be driven from top level.  Lets not do that here.

    Signed-off-by: Ed Warnicke <[email protected]>

Signed-off-by: NSMBot <[email protected]>
@denis-tingaikin denis-tingaikin mentioned this pull request Aug 21, 2020
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@denis-tingaikin denis-tingaikin denis-tingaikin approved these changes

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

Successfully merging this pull request may close these issues.
2 participants
@edwarnicke @denis-tingaikin