[sdk#1111] Fix `sendfd` issue on URL change #1112

Bolodya1997 · 2021-10-20T05:34:27Z

Description

Clones Request in roundrobin, interpose, discover.
Puts dial before additionalFunctionality in client chain.
Copies opts in sendfd, recvfd clients.

Issue link

Closes #1111.

How Has This Been Tested?

Added unit testing to cover
Tested manually
Tested by integration testing
Have not tested

Types of changes

Signed-off-by: Vladimir Popov <vladimir.popov@xored.com>

denis-tingaikin · 2021-10-20T06:41:53Z

pkg/networkservice/chains/client/client.go

-			append(
-				opts.additionalFunctionality,


Why do we need to change this?

In case of URL change dial.Client does the following on Request:

Close old URL.

Request new URL.

If we have sendfd.Client in the chain before it does the following on Request:

insert grpcfd.PerRPCCredentials to the call options.

grpcfd.PerRPCCredentials does the following:

Store fds that needs to be sent.

On first gRPC interaction send them and remember connection as a FDTransceiver.

Cleanup all stored fds.

So it results in the following:

dial closes old URL - PerRPCCredentials sends all fds to the old URL.

dial requests new URL - PerRPCCredentials has nothing to send so it does nothing.

So here are 2 solutions that I can currently see:

Put dial client before additionalFunctionality.

Edit PerRPCCredentials not to capture FDTransceiver and not to cleanup stored fds on send.

We have discussed this with @edwarnicke and so it actually looks like both of these solutions have their pros and cons, but [1] is easier to implement.

@denis-tingaikin I discussed this this morning with @Bolodya1997 on slack. It's a subtle bug, but once explained it makes sense.

pkg/networkservice/chains/nsmgr/single_test.go

pkg/networkservice/common/discover/server.go

pkg/networkservice/common/interpose/server.go

denis-tingaikin · 2021-10-20T06:42:56Z

pkg/networkservice/common/mechanisms/recvfd/client.go

@@ -48,7 +48,7 @@ func NewClient() networkservice.NetworkServiceClient {
 func (r *recvFDClient) Request(ctx context.Context, request *networkservice.NetworkServiceRequest, opts ...grpc.CallOption) (*networkservice.Connection, error) {
 	// Get the grpcfd.FDRecver
 	rpcCredentials := grpcfd.PerRPCCredentials(grpcfd.PerRPCCredentialsFromCallOptions(opts...))
-	opts = append(opts, grpc.PerRPCCredentials(rpcCredentials))
+	opts = append([]grpc.CallOption{grpc.PerRPCCredentials(rpcCredentials)}, opts...)


Why do we need to change the order of call options?

My fault, I did a mistake during the testing, there is no need for this.

pkg/networkservice/common/mechanisms/recvfd/client.go

pkg/networkservice/common/mechanisms/sendfd/client.go

pkg/networkservice/common/roundrobin/server_test.go

denis-tingaikin · 2021-10-20T06:46:51Z

pkg/networkservice/common/roundrobin/server_test.go

+func TestSelectEndpointServer_CleanRequest(t *testing.T) {
+	t.Cleanup(func() { goleak.VerifyNone(t) })
+
+	var flag bool


Could you use a more understandable name?

Suggested change

var flag bool

var TODO bool

Fixed with hasBeenRequested.

This reverts commit d14a71d. Signed-off-by: Vladimir Popov <vladimir.popov@xored.com>

Signed-off-by: Vladimir Popov <vladimir.popov@xored.com>

…k@main PR link: networkservicemesh/sdk#1112 Commit: a94971d Author: Vladimir Popov Date: 2021-10-20 21:43:53 +0700 Message: - [sdk#1111] Fix `sendfd` issue on URL change (#1112) * Set dial client before additional functionality in client chain Signed-off-by: Vladimir Popov <vladimir.popov@xored.com> * Fix recvfd/senfd clients Signed-off-by: Vladimir Popov <vladimir.popov@xored.com> * Clone request in remote iteration points Signed-off-by: Vladimir Popov <vladimir.popov@xored.com> * Add multiforwarder sendfd test Signed-off-by: Vladimir Popov <vladimir.popov@xored.com> * Revert "Fix recvfd/senfd clients" This reverts commit d14a71d11a2561b847d73dc6ae17f9394fa12125. Signed-off-by: Vladimir Popov <vladimir.popov@xored.com> * Fix naming Signed-off-by: NSMBot <nsmbot@networkservicmesh.io>

Vladimir Popov added 4 commits October 20, 2021 10:15

Set dial client before additional functionality in client chain

107a789

Signed-off-by: Vladimir Popov <vladimir.popov@xored.com>

Fix recvfd/senfd clients

d14a71d

Signed-off-by: Vladimir Popov <vladimir.popov@xored.com>

Clone request in remote iteration points

d8f7077

Signed-off-by: Vladimir Popov <vladimir.popov@xored.com>

Add multiforwarder sendfd test

983934c

Signed-off-by: Vladimir Popov <vladimir.popov@xored.com>

Bolodya1997 requested review from denis-tingaikin, Mixaster995, glazychev-art and ThetaDR October 20, 2021 05:43

denis-tingaikin reviewed Oct 20, 2021

View reviewed changes

Bolodya1997 marked this pull request as draft October 20, 2021 08:20

Vladimir Popov added 2 commits October 20, 2021 20:41

Revert "Fix recvfd/senfd clients"

b0778cd

This reverts commit d14a71d. Signed-off-by: Vladimir Popov <vladimir.popov@xored.com>

Fix naming

6ff3c75

Signed-off-by: Vladimir Popov <vladimir.popov@xored.com>

Bolodya1997 marked this pull request as ready for review October 20, 2021 14:00

edwarnicke merged commit a94971d into networkservicemesh:main Oct 20, 2021

nsmbot mentioned this pull request Oct 20, 2021

Update from update/networkservicemesh/sdk networkservicemesh/sdk-kernel#353

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[sdk#1111] Fix `sendfd` issue on URL change #1112

[sdk#1111] Fix `sendfd` issue on URL change #1112

Bolodya1997 commented Oct 20, 2021

denis-tingaikin Oct 20, 2021

Bolodya1997 Oct 20, 2021

edwarnicke Oct 20, 2021

denis-tingaikin Oct 20, 2021

Bolodya1997 Oct 20, 2021

denis-tingaikin Oct 20, 2021 •

edited

Loading

Bolodya1997 Oct 20, 2021

[sdk#1111] Fix sendfd issue on URL change #1112

[sdk#1111] Fix sendfd issue on URL change #1112

Conversation

Bolodya1997 commented Oct 20, 2021

Description

Issue link

How Has This Been Tested?

Types of changes

denis-tingaikin Oct 20, 2021

Choose a reason for hiding this comment

Bolodya1997 Oct 20, 2021

Choose a reason for hiding this comment

edwarnicke Oct 20, 2021

Choose a reason for hiding this comment

denis-tingaikin Oct 20, 2021

Choose a reason for hiding this comment

Bolodya1997 Oct 20, 2021

Choose a reason for hiding this comment

denis-tingaikin Oct 20, 2021 • edited Loading

Choose a reason for hiding this comment

Bolodya1997 Oct 20, 2021

Choose a reason for hiding this comment

[sdk#1111] Fix `sendfd` issue on URL change #1112

[sdk#1111] Fix `sendfd` issue on URL change #1112

denis-tingaikin Oct 20, 2021 •

edited

Loading