-
Notifications
You must be signed in to change notification settings - Fork 19
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
Govpp update #519
Govpp update #519
Conversation
Signed-off-by: Artem Glazychev <artem.glazychev@xored.com>
@edwarnicke Could you have a look? |
@@ -122,7 +121,6 @@ func NewServer(ctx context.Context, name string, authzServer networkservice.Netw | |||
// mechanisms | |||
memif.NewClient(vppConn, | |||
memif.WithChangeNetNS(), | |||
memif.WithExternalVPP(), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@glazychev-art I like this simplification a lot... but could you help me understand what allowed us to drop the WithExternalVPP... I don't quite follow.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@edwarnicke
Honestly, I don't understand - why was it necessary at all. We use this option only for forwarder, and there is only one place where we refer to it:
https://github.com/networkservicemesh/sdk-vpp/blob/main/pkg/networkservice/mechanisms/memif/common.go#L254-L256
As I understand, we try to figure out, is currentNetNS
is equal to targetNetNS
.
But this condition is always false for forwarder - https://github.com/networkservicemesh/sdk-vpp/blob/main/pkg/networkservice/mechanisms/memif/common.go#L264 and getNamespace
returns u.Path, nil
anyway (with ExternalVPP
option and without).
Locally everything works without this option. If there are any problems on ci
, I think we can always revert these changes.
…k-vpp@main PR link: networkservicemesh/sdk-vpp#519 Commit: 79250f4 Author: Artem Glazychev Date: 2022-03-14 21:31:22 +0700 Message: - Govpp update (#519) Signed-off-by: NSMBot <nsmbot@networkservicmesh.io>
…k-vpp@main PR link: networkservicemesh/sdk-vpp#519 Commit: 79250f4 Author: Artem Glazychev Date: 2022-03-14 21:31:22 +0700 Message: - Govpp update (#519) Signed-off-by: NSMBot <nsmbot@networkservicmesh.io>
…k-vpp@main PR link: networkservicemesh/sdk-vpp#519 Commit: 79250f4 Author: Artem Glazychev Date: 2022-03-14 21:31:22 +0700 Message: - Govpp update (#519) Signed-off-by: NSMBot <nsmbot@networkservicmesh.io>
…k-vpp@main PR link: networkservicemesh/sdk-vpp#519 Commit: 79250f4 Author: Artem Glazychev Date: 2022-03-14 21:31:22 +0700 Message: - Govpp update (#519) Signed-off-by: NSMBot <nsmbot@networkservicmesh.io>
…k-vpp@main PR link: networkservicemesh/sdk-vpp#519 Commit: 79250f4 Author: Artem Glazychev Date: 2022-03-14 21:31:22 +0700 Message: - Govpp update (#519) Signed-off-by: NSMBot <nsmbot@networkservicmesh.io>
…k-kernel@main PR link: networkservicemesh/sdk-kernel#519 Commit: ee21a48 Author: Denis Tingaikin Date: 2022-09-15 02:12:41 +0300 Message: - Merge pull request #519 from ThetaDR/linter-fix Signed-off-by: NSMBot <nsmbot@networkservicmesh.io>
Issue: #508
The main difference is that
MemifSocketFilenameAddDelV2
API doesn't haveNamespace
now. Instead, we should act like this:@netns:ns0@somename
Signed-off-by: Artem Glazychev artem.glazychev@xored.com