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

Rework memory registry server #699

Merged
merged 3 commits into from
Feb 10, 2021
Merged

Rework memory registry server #699

merged 3 commits into from
Feb 10, 2021

Conversation

Bolodya1997
Copy link

Issues

There are data races, deadlocks and invalid behavior in both NSE, NS memory registry servers.

Solution

  1. Add unit tests for data races, slow receiver, all Register/Unregister received.
  2. Rework NSE, NS memory registry servers.

denis-tingaikin
denis-tingaikin requested changes Feb 4, 2021
View reviewed changes
Copy link
Member

@denis-tingaikin denis-tingaikin 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.

Most covered issues related to tests and not related to code of memory chain element.

I guess we could add the unlimited buffers for channels to avoid potential channel deadlocks
OR do not try to send an update if the channel is blocked.

pkg/registry/common/memory/nse_server_test.go
wg.Wait()
}

func TestNetworkServiceEndpointRegistryServer_SlowReceiver(t *testing.T) {
Copy link
Member

@denis-tingaikin denis-tingaikin Feb 4, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

This test is invalid.

Here is a try to write 100 entries to a channel that has a limited 10 by default. Please look https://github.com/networkservicemesh/sdk/blob/master/pkg/registry/common/memory/common.go#L19

Please use option setChannelSize(100) or
Rework this test to correctly read/write from the channel to simulate the real client/server.

Copy link
Author

Choose a reason for hiding this comment

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

It is actually very similar to the discover server using watching Find and finishing receiving after the first match.

pkg/registry/common/memory/nse_server_test.go
findCancel()
}

func TestNetworkServiceEndpointRegistryServer_ShouldReceiveAllRegisters(t *testing.T) {
Copy link
Member

Choose a reason for hiding this comment

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

Also works fine on master with increasing buffer size inside the test...

Copy link
Author

Choose a reason for hiding this comment

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

Yes, setting unlimited buffer size fixes almost all found issues :)

pkg/registry/common/memory/nse_server_test.go
wg.Wait()
}

func TestNetworkServiceEndpointRegistryServer_ShouldReceiveAllUnregisters(t *testing.T) {
Copy link
Member

Choose a reason for hiding this comment

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

Also works fine on master with increasing buffer size inside the test...

@Bolodya1997 Bolodya1997 marked this pull request as draft February 5, 2021 04:37
@Bolodya1997 Bolodya1997 force-pushed the fix-memory-data-race branch 5 times, most recently from 3c6b04a to c4eb674 Compare February 5, 2021 06:31
@Bolodya1997 Bolodya1997 marked this pull request as ready for review February 5, 2021 06:52
@Bolodya1997
Copy link
Author

Most covered issues related to tests and not related to code of memory chain element.

I guess we could add the unlimited buffers for channels to avoid potential channel deadlocks
OR do not try to send an update if the channel is blocked.

I am not sure that increasing buffer size is an option if we can find a solution without it.

@denis-tingaikin
Copy link
Member

@Bolodya1997 Do we still have a problem with channel limitation with these changes?

@Bolodya1997
Copy link
Author

@Bolodya1997 Do we still have a problem with channel limitation with these changes?

Nope, there is no more need to add unlimited channels with these changes.

Vladimir Popov added 3 commits February 8, 2021 16:42
Rework memory registry server
3dafd78 
Signed-off-by: Vladimir Popov <vladimir.popov@xored.com>
Fix memory registry slow receiver test
20ddc09 
Signed-off-by: Vladimir Popov <vladimir.popov@xored.com>
Fix tests
1d74a75 
Signed-off-by: Vladimir Popov <vladimir.popov@xored.com>
@denis-tingaikin denis-tingaikin merged commit 536f38f into networkservicemesh:master Feb 10, 2021
nsmbot pushed a commit to networkservicemesh/cmd-nsmgr that referenced this pull request Feb 10, 2021
Update go.mod and go.sum to latest version from networkservicemesh/sd…
5ffda7e 
…k@master networkservicemesh/sdk#699

networkservicemesh/sdk PR link: networkservicemesh/sdk#699

networkservicemesh/sdk commit message:
commit 536f38f8986817e62259be5f258203006ef96a7e
Author: Vladimir Popov <vladimir.popov@xored.com>
Date:   Wed Feb 10 16:16:43 2021 +0700

    Rework memory registry server (#699)

    * Rework memory registry server

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

    * Fix memory registry slow receiver test

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

    * Fix tests

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

Signed-off-by: NSMBot <nsmbot@networkservicmesh.io>
nsmbot pushed a commit to networkservicemesh/sdk-kernel that referenced this pull request Feb 10, 2021
Update go.mod and go.sum to latest version from networkservicemesh/sd…
2630c63 
…k@master networkservicemesh/sdk#699

networkservicemesh/sdk PR link: networkservicemesh/sdk#699

networkservicemesh/sdk commit message:
commit 536f38f8986817e62259be5f258203006ef96a7e
Author: Vladimir Popov <vladimir.popov@xored.com>
Date:   Wed Feb 10 16:16:43 2021 +0700

    Rework memory registry server (#699)

    * Rework memory registry server

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

    * Fix memory registry slow receiver test

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

    * Fix tests

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

Signed-off-by: NSMBot <nsmbot@networkservicmesh.io>
nsmbot pushed a commit to networkservicemesh/cmd-nsmgr-proxy that referenced this pull request Feb 10, 2021
Update go.mod and go.sum to latest version from networkservicemesh/sd…
cf00d72 
…k@master networkservicemesh/sdk#699

networkservicemesh/sdk PR link: networkservicemesh/sdk#699

networkservicemesh/sdk commit message:
commit 536f38f8986817e62259be5f258203006ef96a7e
Author: Vladimir Popov <vladimir.popov@xored.com>
Date:   Wed Feb 10 16:16:43 2021 +0700

    Rework memory registry server (#699)

    * Rework memory registry server

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

    * Fix memory registry slow receiver test

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

    * Fix tests

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

Signed-off-by: NSMBot <nsmbot@networkservicmesh.io>
nsmbot pushed a commit to networkservicemesh/cmd-registry-memory that referenced this pull request Feb 10, 2021
Update go.mod and go.sum to latest version from networkservicemesh/sd…
e4a713f 
…k@master networkservicemesh/sdk#699

networkservicemesh/sdk PR link: networkservicemesh/sdk#699

networkservicemesh/sdk commit message:
commit 536f38f8986817e62259be5f258203006ef96a7e
Author: Vladimir Popov <vladimir.popov@xored.com>
Date:   Wed Feb 10 16:16:43 2021 +0700

    Rework memory registry server (#699)

    * Rework memory registry server

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

    * Fix memory registry slow receiver test

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

    * Fix tests

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

Signed-off-by: NSMBot <nsmbot@networkservicmesh.io>
nsmbot pushed a commit to networkservicemesh/cmd-nse-vfio that referenced this pull request Feb 10, 2021
Update go.mod and go.sum to latest version from networkservicemesh/sd…
7bc34f2 
…k@master networkservicemesh/sdk#699

networkservicemesh/sdk PR link: networkservicemesh/sdk#699

networkservicemesh/sdk commit message:
commit 536f38f8986817e62259be5f258203006ef96a7e
Author: Vladimir Popov <vladimir.popov@xored.com>
Date:   Wed Feb 10 16:16:43 2021 +0700

    Rework memory registry server (#699)

    * Rework memory registry server

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

    * Fix memory registry slow receiver test

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

    * Fix tests

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

Signed-off-by: NSMBot <nsmbot@networkservicmesh.io>
nsmbot pushed a commit to networkservicemesh/sdk-k8s that referenced this pull request Feb 10, 2021
Update go.mod and go.sum to latest version from networkservicemesh/sd…
a15f046 
…k@master networkservicemesh/sdk#699

networkservicemesh/sdk PR link: networkservicemesh/sdk#699

networkservicemesh/sdk commit message:
commit 536f38f8986817e62259be5f258203006ef96a7e
Author: Vladimir Popov <vladimir.popov@xored.com>
Date:   Wed Feb 10 16:16:43 2021 +0700

    Rework memory registry server (#699)

    * Rework memory registry server

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

    * Fix memory registry slow receiver test

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

    * Fix tests

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

Signed-off-by: NSMBot <nsmbot@networkservicmesh.io>
nsmbot pushed a commit to networkservicemesh/cmd-registry-proxy-dns that referenced this pull request Feb 10, 2021
Update go.mod and go.sum to latest version from networkservicemesh/sd…
6f4e6a9 
…k@master networkservicemesh/sdk#699

networkservicemesh/sdk PR link: networkservicemesh/sdk#699

networkservicemesh/sdk commit message:
commit 536f38f8986817e62259be5f258203006ef96a7e
Author: Vladimir Popov <vladimir.popov@xored.com>
Date:   Wed Feb 10 16:16:43 2021 +0700

    Rework memory registry server (#699)

    * Rework memory registry server

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

    * Fix memory registry slow receiver test

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

    * Fix tests

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

Signed-off-by: NSMBot <nsmbot@networkservicmesh.io>
nsmbot pushed a commit to networkservicemesh/cmd-nse-icmp-responder that referenced this pull request Feb 10, 2021
Update go.mod and go.sum to latest version from networkservicemesh/sd…
a743aaf 
…k@master networkservicemesh/sdk#699

networkservicemesh/sdk PR link: networkservicemesh/sdk#699

networkservicemesh/sdk commit message:
commit 536f38f8986817e62259be5f258203006ef96a7e
Author: Vladimir Popov <vladimir.popov@xored.com>
Date:   Wed Feb 10 16:16:43 2021 +0700

    Rework memory registry server (#699)

    * Rework memory registry server

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

    * Fix memory registry slow receiver test

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

    * Fix tests

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

Signed-off-by: NSMBot <nsmbot@networkservicmesh.io>
nsmbot pushed a commit to networkservicemesh/sdk-vpp that referenced this pull request Feb 10, 2021
Update go.mod and go.sum to latest version from networkservicemesh/sd…
228c009 
…k@master networkservicemesh/sdk#699

networkservicemesh/sdk PR link: networkservicemesh/sdk#699

networkservicemesh/sdk commit message:
commit 536f38f8986817e62259be5f258203006ef96a7e
Author: Vladimir Popov <vladimir.popov@xored.com>
Date:   Wed Feb 10 16:16:43 2021 +0700

    Rework memory registry server (#699)

    * Rework memory registry server

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

    * Fix memory registry slow receiver test

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

    * Fix tests

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

Signed-off-by: NSMBot <nsmbot@networkservicmesh.io>
This was referenced Feb 10, 2021
Merged
Merged
Merged
This was referenced Feb 10, 2021
Merged
Merged
Merged
Merged
Merged
Merged
@Bolodya1997 Bolodya1997 deleted the fix-memory-data-race branch February 12, 2021 05:39
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
@Bolodya1997 @denis-tingaikin