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

discoverServer: handle Close #571

Merged
merged 1 commit into from
Nov 12, 2020
Merged

discoverServer: handle Close #571

merged 1 commit into from
Nov 12, 2020

Conversation

xzfc
Copy link
Contributor

@xzfc xzfc commented Nov 6, 2020

This PR fixes #570 for Local/Remote one-hop NSMgr use-cases.

@xzfc xzfc mentioned this pull request Nov 6, 2020
Closed
3 tasks
denis-tingaikin
denis-tingaikin requested changes Nov 6, 2020
View reviewed changes
pkg/networkservice/chains/nsmgr/server_test.go Outdated
Comment on lines 59 to 64
var nodesCount, nscNode, expectedPathSegments int
if remote {
nodesCount = 2
nscNode = 1
expectedPathSegments = 8
} else {
nodesCount = 1
nscNode = 0
expectedPathSegments = 5
}
Copy link
Member

@denis-tingaikin denis-tingaikin Nov 6, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

Please consider extracting testSample struct and using for loop over samples

type testSample struct {
  nodesCount uint
  nscNode uint
  nseNode uint
  reRequestsCount unit
  expectedPathSegments unit
  expectedreRequestsCount unit 
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't feel that the only two cases worth generalizing.

Copy link
Member

@denis-tingaikin denis-tingaikin Nov 6, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

  1. Actually you already generalized these two cases
  2. With testSample we can cover many cases such as
    2.1. nsc - node1, nse - node 2
    2.2. nsc - node2, nse - node1
    2.3. nsc - node1, nse -node1 (nodesCount=1),
    2.4. nsc - node2, nse -node2 (nodesCount=2)
    2.6. all above scenarious with reRequestsCount = 2
    2.7. all above scenarious with reRequestsCount = 5
    2.7. all above scenarious with reRequestsCount = 10
  3. test pattern with testSample looks more better than adding method with bolean logic.

But I would recommend do not touch these tests and just add test for close into discover pkg.

@xzfc xzfc marked this pull request as draft November 10, 2020 14:24
@xzfc xzfc marked this pull request as ready for review November 12, 2020 06:31
denis-tingaikin
denis-tingaikin reviewed Nov 12, 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.

Looks good, please add using of atomic to prevent race conditions

pkg/networkservice/chains/nsmgr/server_test.go Outdated
@@ -318,3 +328,17 @@ func (p *passThroughClient) Close(ctx context.Context, conn *networkservice.Conn
conn = conn.Clone()
return next.Client(ctx).Close(ctx, conn, opts...)
}

type counterServer struct {
Requests, Closes int
Copy link
Member

Choose a reason for hiding this comment

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

Please use atomic int32 to prevent race condition in case of a testing parallel request from a few clients

@xzfc
discoverServer: handle Close
9872cb5 
Signed-off-by: Albert Safin <albert.safin@xored.com>
@denis-tingaikin denis-tingaikin merged commit 636615f into networkservicemesh:master Nov 12, 2020
nsmbot pushed a commit to networkservicemesh/cmd-registry-memory that referenced this pull request Nov 12, 2020
Update go.mod and go.sum to latest version from networkservicemesh/sd…
cc46d21 
…k@master networkservicemesh/sdk#571

networkservicemesh/sdk PR link: networkservicemesh/sdk#571

networkservicemesh/sdk commit message:
commit 636615f67bfe8d351eb73223acb52e697cecf4ce
Author: xzfc <xzfc@users.noreply.github.com>
Date:   Thu Nov 12 15:23:06 2020 +0000

    discoverServer: handle Close (#571)

    Signed-off-by: Albert Safin <albert.safin@xored.com>

Signed-off-by: NSMBot <nsmbot@networkservicmesh.io>
nsmbot pushed a commit to networkservicemesh/cmd-nsmgr-proxy that referenced this pull request Nov 12, 2020
Update go.mod and go.sum to latest version from networkservicemesh/sd…
088d2da 
…k@master networkservicemesh/sdk#571

networkservicemesh/sdk PR link: networkservicemesh/sdk#571

networkservicemesh/sdk commit message:
commit 636615f67bfe8d351eb73223acb52e697cecf4ce
Author: xzfc <xzfc@users.noreply.github.com>
Date:   Thu Nov 12 15:23:06 2020 +0000

    discoverServer: handle Close (#571)

    Signed-off-by: Albert Safin <albert.safin@xored.com>

Signed-off-by: NSMBot <nsmbot@networkservicmesh.io>
nsmbot pushed a commit to networkservicemesh/sdk-kernel that referenced this pull request Nov 12, 2020
Update go.mod and go.sum to latest version from networkservicemesh/sd…
759bf72 
…k@master networkservicemesh/sdk#571

networkservicemesh/sdk PR link: networkservicemesh/sdk#571

networkservicemesh/sdk commit message:
commit 636615f67bfe8d351eb73223acb52e697cecf4ce
Author: xzfc <xzfc@users.noreply.github.com>
Date:   Thu Nov 12 15:23:06 2020 +0000

    discoverServer: handle Close (#571)

    Signed-off-by: Albert Safin <albert.safin@xored.com>

Signed-off-by: NSMBot <nsmbot@networkservicmesh.io>
nsmbot pushed a commit to networkservicemesh/cmd-nse-icmp-responder that referenced this pull request Nov 12, 2020
Update go.mod and go.sum to latest version from networkservicemesh/sd…
679417f 
…k@master networkservicemesh/sdk#571

networkservicemesh/sdk PR link: networkservicemesh/sdk#571

networkservicemesh/sdk commit message:
commit 636615f67bfe8d351eb73223acb52e697cecf4ce
Author: xzfc <xzfc@users.noreply.github.com>
Date:   Thu Nov 12 15:23:06 2020 +0000

    discoverServer: handle Close (#571)

    Signed-off-by: Albert Safin <albert.safin@xored.com>

Signed-off-by: NSMBot <nsmbot@networkservicmesh.io>
nsmbot pushed a commit to networkservicemesh/cmd-registry-proxy-dns that referenced this pull request Nov 12, 2020
Update go.mod and go.sum to latest version from networkservicemesh/sd…
cfa1f84 
…k@master networkservicemesh/sdk#571

networkservicemesh/sdk PR link: networkservicemesh/sdk#571

networkservicemesh/sdk commit message:
commit 636615f67bfe8d351eb73223acb52e697cecf4ce
Author: xzfc <xzfc@users.noreply.github.com>
Date:   Thu Nov 12 15:23:06 2020 +0000

    discoverServer: handle Close (#571)

    Signed-off-by: Albert Safin <albert.safin@xored.com>

Signed-off-by: NSMBot <nsmbot@networkservicmesh.io>
nsmbot pushed a commit to networkservicemesh/cmd-nsmgr that referenced this pull request Nov 12, 2020
Update go.mod and go.sum to latest version from networkservicemesh/sd…
4ba45a2 
…k@master networkservicemesh/sdk#571

networkservicemesh/sdk PR link: networkservicemesh/sdk#571

networkservicemesh/sdk commit message:
commit 636615f67bfe8d351eb73223acb52e697cecf4ce
Author: xzfc <xzfc@users.noreply.github.com>
Date:   Thu Nov 12 15:23:06 2020 +0000

    discoverServer: handle Close (#571)

    Signed-off-by: Albert Safin <albert.safin@xored.com>

Signed-off-by: NSMBot <nsmbot@networkservicmesh.io>
nsmbot pushed a commit to networkservicemesh/sdk-vppagent that referenced this pull request Nov 12, 2020
Update go.mod and go.sum to latest version from networkservicemesh/sd…
cd4b721 
…k@master networkservicemesh/sdk#571

networkservicemesh/sdk PR link: networkservicemesh/sdk#571

networkservicemesh/sdk commit message:
commit 636615f67bfe8d351eb73223acb52e697cecf4ce
Author: xzfc <xzfc@users.noreply.github.com>
Date:   Thu Nov 12 15:23:06 2020 +0000

    discoverServer: handle Close (#571)

    Signed-off-by: Albert Safin <albert.safin@xored.com>

Signed-off-by: NSMBot <nsmbot@networkservicmesh.io>
This was referenced Nov 12, 2020
Merged
Merged
Merged
Merged
illbegood pushed a commit to illbegood/sdk that referenced this pull request Dec 20, 2020
@xzfc
discoverServer: handle Close (networkservicemesh#571)
e15e576 
Signed-off-by: Albert Safin <albert.safin@xored.com>
Signed-off-by: Sergey Ershov <sergey.ershov@xored.com>
illbegood pushed a commit to illbegood/sdk that referenced this pull request Dec 20, 2020
@xzfc
discoverServer: handle Close (networkservicemesh#571)
a5b82a0 
Signed-off-by: Albert Safin <albert.safin@xored.com>
Signed-off-by: Sergey Ershov <sergey.ershov@xored.com>
illbegood pushed a commit to illbegood/sdk that referenced this pull request Dec 23, 2020
@xzfc
discoverServer: handle Close (networkservicemesh#571)
9b4f8c2 
Signed-off-by: Albert Safin <albert.safin@xored.com>
Signed-off-by: Sergey Ershov <sergey.ershov@xored.com>
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.

NSMgr should pass Close() calls
2 participants
@xzfc @denis-tingaikin