feat: Add support for point-to-multipoint forwarders in NSM #1122

denis-tingaikin · 2021-10-27T14:58:37Z

Signed-off-by: Denis Tingaikin denis.tingajkin@xored.com

Description

This PR adds a part of p2mp rework related to SDK

Issue link

#1094

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: Denis Tingaikin <denis.tingajkin@xored.com>

Signed-off-by: denis-tingaikin <denis.tingajkin@xored.com>

denis-tingaikin · 2021-11-07T09:37:08Z

pkg/networkservice/chains/nsmgr/single_test.go

@@ -253,3 +254,78 @@ func Test_ShouldParseNetworkServiceLabelsTemplate(t *testing.T) {
 	_, err = nse.Unregister(ctx, nseReg)
 	require.NoError(t, err)
 }
+
+func Test_UsecasePoint2MultiPoint(t *testing.T) {


@edwarnicke Is this test look valid?

denis-tingaikin · 2021-11-07T09:37:39Z

pkg/networkservice/chains/nsmgr/server.go

+		nsRegistry,
+	)
+
+	var nseInMemoryRegistry = registrychain.NewNetworkServiceEndpointRegistryServer(


Note: now nsmgr has its own memory registry cache by default.

denis-tingaikin · 2021-11-07T09:39:00Z

pkg/networkservice/chains/nsmgr/single_test.go

@@ -119,7 +119,8 @@ func Test_ShouldCorrectlyAddForwardersWithSameNames(t *testing.T) {
 	require.NoError(t, err)

 	forwarderReg := &registry.NetworkServiceEndpoint{
-		Name: "forwarder",
+		Name:                "forwarder",
+		NetworkServiceNames: []string{"forwarder"},


@edwarnicke

Should we add const for forwarder networkservicename into api?

Should we create a registry network service endpoint client chain element for registration forwarders?

Should we create a registry network service endpoint client chain element for registration forwarders?

I don't quite follow this, could you say more?

Sure, we can add something like:

type setForwarderNetworkServiceNameNSEClient struct{} func (n *setNetworkServiceNamesNSEClient) Register(ctx context.Context, in *registry.NetworkServiceEndpoint, opts ...grpc.CallOption) (*registry.NetworkServiceEndpoint, error) { in.NetworkServiceNames = []string{"forwarder"} ...

This client can be used by forwarders on registration.

Ah... that's interesting... but how does it handle the case of that being overriden for some reason by env variables in the forwarder?

Better question... where is the 'forwarder' string encoded in the NSMgr ?

Better question... where is the 'forwarder' string encoded in the NSMgr ?

See at discovercrossnse chain element.

if you are fine with this, then I'll add the option as you suggested.

Forwarders should already be able to overload the NS name via ENV variable... do they not?

Yes, but all forwarders and nsmgrs on the cluster should use the same network service name. So constant in api can simplify it

Mostly, I like your suggestion, but just shared another possible way

Fixed in 1a0be07

denis-tingaikin · 2021-11-07T09:40:15Z

pkg/networkservice/chains/nsmgr/unix_test.go

@@ -39,6 +39,7 @@ import (
 )

 func Test_Local_NoURLUsecase(t *testing.T) {
+	t.Skip("https://github.com/networkservicemesh/sdk/issues/1118")


Note: This is disabled because with p2mp rework forwarder should recv fd from the nsmgr registry. Currently, it is not working in these unit tests by #1118 but these scenarios work well in integration tests.

denis-tingaikin · 2021-11-07T09:40:25Z

pkg/networkservice/chains/nsmgr/unix_test.go

@@ -84,6 +85,7 @@ func Test_Local_NoURLUsecase(t *testing.T) {
 }

 func Test_MultiForwarderSendfd(t *testing.T) {
+	t.Skip("https://github.com/networkservicemesh/sdk/issues/1118")


Note: This is disabled because with p2mp rework forwarder should recv fd from the nsmgr registry. Currently, it is not working in these unit tests by #1118 but these scenarios work well in integration tests.

denis-tingaikin · 2021-11-07T09:41:03Z

pkg/networkservice/common/discover/match_selector.go

 			continue
 		}
 		nseCandidates := make([]*registry.NetworkServiceEndpoint, 0)
 		// Check all Destinations in that match
 		for _, destination := range match.GetRoutes() {
 			// Each NSE should be matched against that destination
 			for _, nse := range validNetworkServiceEndpoints {
-				if isSubset(nse.GetNetworkServiceLabels()[ns.Name].Labels, destination.GetDestinationSelector(), nsLabels) {


potential nil panic.

nse.GetNetworkServiceLabels()[ns.Name] can be nil.

Fixed.

denis-tingaikin · 2021-11-07T09:41:55Z

pkg/networkservice/common/discovercrossnse/metadata.go

+func loadForwarderName(ctx context.Context) string {
+	v, ok := metadata.Map(ctx, false).Load(selectedForworderKey{})
+	if !ok {
+		return ""
+	}
+	return v.(string)
 }

-// Is returns true if passed name contains interpose identity
-func Is(name string) bool {
-	return strings.HasSuffix(name, nameSuffix)
+func storeForwarderName(ctx context.Context, v string) {


Potential improvement:

@edwarnicke What if we'll load the forwarder name from the next path segment instead of metadata?

How would that work on initial request?

How would that work on initial request?

For the initial request, we do have not the next segment.
So simply go to discover a forwarder (see at branch where forwarderName == "")

And what if the next segment is no longer valid?

@edwarnicke That's a good question. It looks like in this case we should select another forwarder.

denis-tingaikin · 2021-11-07T09:43:16Z

pkg/networkservice/common/interpose/server.go

-
-// Package interpose provides a NetworkServiceServer chain element that tracks local Cross connect Endpoints and call them first
-// their unix file socket as the clienturl.ClientURL(ctx) used to connect to them.
-package interpose


Note: interpose totally deleted.

denis-tingaikin · 2021-11-07T09:44:15Z

pkg/registry/common/recvfd/client.go

+
+// +build linux
+
+package recvfd


Note: forwarder should be able to get fd from the local nse registration

denis-tingaikin · 2021-11-07T09:44:31Z

pkg/registry/common/sendfd/server_linux.go

+
+// +build linux
+
+package sendfd


Note: nsmgr should be able to send fd from the local nse registration

denis-tingaikin · 2021-11-07T09:45:12Z

pkg/tools/sandbox/node.go

+						discover.NewServer(nsClient, nseClient),
+						roundrobin.NewServer(),


@edwarnicke Should we add chains/forwarder with these two chain elements?

Should we add chains/forwarder with these two chain elements?

Could you say more... I don't quite follow...

we can add a new pkg forwarder into sdk/pkg/networkservice/chains/.

So currently, it seems to me we have motivation because many forwarders may be interested in default SDK chain elements such as roundrobin, disocver, recvfd, sendfd. So we can add a default preset for them.

Hmm... lets try getting the existing forwarders going first

Because discover/roundrobin are kind of bespoke to our p2p forwarders... p2mp forwarders may chose to do something quite different.

@edwarnicke Load balancing sound like option :)

edwarnicke · 2021-11-07T17:35:56Z

pkg/networkservice/common/discovercrossnse/server.go

+			return nil, errors.New("no candidates found")
+		}
+
+		u, err := url.Parse(nses[0].Url) // TODO: Should we consider about load balancing?


Yes, we should consider load balancing... open an issue :)

I think this can be resolved in this PR if we go with #1122 (comment)

How would it be resolved?

cut this line https://github.com/networkservicemesh/sdk/blob/main/pkg/networkservice/common/roundrobin/server.go#L64

add into sdk/pkg/networkservice/setednpoint chain element that will set this
https://github.com/networkservicemesh/sdk/blob/main/pkg/networkservice/common/roundrobin/server.go#L64 based on candidates and cleinturl from the context only if request.GetConnection().NetworkServiceEndpointName is empty.

discovercrossnse should pass into context candates.

Finally, we can have chains like this:

nsmgr:

... discoverforwarder.NewServer(...) // works based on path segments. If it is presented into connection just get it otherwise add canidates for the next load balancing chain element roundrobin.NewServer()

forwarder

discover.NewServer(...) roundrobin.NewServer() setednpointname.NewServer() //sets nse name into connection.

Ah... I see... you are separating the setting of the value in the connection from the selection of where to go next.

Would it be possible to open an issue to come back to this and go ahead and land the P2MP now?

Would it be possible to open an issue to come back to this and go ahead and land the P2MP now?

Yes.

Let's do that then :)

edwarnicke · 2021-11-07T17:40:28Z

Do we have corresponding PRs for the Forwarders to have them do selection?

Signed-off-by: denis-tingaikin <denis.tingajkin@xored.com>

…k@main PR link: networkservicemesh/sdk#1122 Commit: c57ed95 Author: Denis Tingaikin Date: 2021-11-08 00:18:37 +0300 Message: - feat: Add support for point-to-multipoint forwarders in NSM (#1122) * initial p2mp re-work Signed-off-by: Denis Tingaikin <denis.tingajkin@xored.com> * add fixes after manual testing Signed-off-by: Denis Tingaikin <denis.tingajkin@xored.com> * delete interpose pkg Signed-off-by: denis-tingaikin <denis.tingajkin@xored.com> * rework discovercrossnse and add p2mp unit test Signed-off-by: denis-tingaikin <denis.tingajkin@xored.com> * apply review comments fixes Signed-off-by: denis-tingaikin <denis.tingajkin@xored.com> * apply self code review Signed-off-by: NSMBot <nsmbot@networkservicmesh.io>

initial p2mp re-work

b4fe3f8

Signed-off-by: Denis Tingaikin <denis.tingajkin@xored.com>

denis-tingaikin force-pushed the p2mp branch from 3da2e94 to 46cbabb Compare November 3, 2021 20:35

add fixes after manual testing

c972a6b

Signed-off-by: Denis Tingaikin <denis.tingajkin@xored.com>

denis-tingaikin force-pushed the p2mp branch from 46cbabb to c972a6b Compare November 3, 2021 20:40

delete interpose pkg

8526aa1

Signed-off-by: denis-tingaikin <denis.tingajkin@xored.com>

denis-tingaikin force-pushed the p2mp branch 2 times, most recently from 26f4994 to 83f9185 Compare November 7, 2021 09:33

rework discovercrossnse and add p2mp unit test

755f53b

Signed-off-by: denis-tingaikin <denis.tingajkin@xored.com>

denis-tingaikin force-pushed the p2mp branch from 83f9185 to 755f53b Compare November 7, 2021 09:34

denis-tingaikin changed the title ~~[WIP] feat: Add support for point-to-multipoint forwarders in NSM~~ feat: Add support for point-to-multipoint forwarders in NSM Nov 7, 2021

denis-tingaikin requested a review from edwarnicke November 7, 2021 09:35

denis-tingaikin commented Nov 7, 2021

View reviewed changes

denis-tingaikin marked this pull request as ready for review November 7, 2021 09:45

edwarnicke reviewed Nov 7, 2021

View reviewed changes

denis-tingaikin added 2 commits November 7, 2021 22:22

apply review comments fixes

1a0be07

Signed-off-by: denis-tingaikin <denis.tingajkin@xored.com>

apply self code review

31eadf8

Signed-off-by: denis-tingaikin <denis.tingajkin@xored.com>

edwarnicke merged commit c57ed95 into networkservicemesh:main Nov 7, 2021

		discover.NewServer(nsClient, nseClient),
		roundrobin.NewServer(),

feat: Add support for point-to-multipoint forwarders in NSM #1122

feat: Add support for point-to-multipoint forwarders in NSM #1122

Conversation

denis-tingaikin commented Oct 27, 2021

Description

Issue link

How Has This Been Tested?

Types of changes

Choose a reason for hiding this comment

Choose a reason for hiding this comment

denis-tingaikin Nov 7, 2021 • edited Loading

Choose a reason for hiding this comment

denis-tingaikin Nov 7, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

denis-tingaikin Nov 7, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

denis-tingaikin Nov 7, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

denis-tingaikin Nov 7, 2021 • edited Loading

Choose a reason for hiding this comment

denis-tingaikin Nov 7, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

denis-tingaikin Nov 7, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

denis-tingaikin Nov 7, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

denis-tingaikin Nov 7, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

edwarnicke commented Nov 7, 2021

denis-tingaikin Nov 7, 2021 •

edited

Loading

denis-tingaikin Nov 7, 2021 •

edited

Loading

denis-tingaikin Nov 7, 2021 •

edited

Loading

denis-tingaikin Nov 7, 2021 •

edited

Loading

denis-tingaikin Nov 7, 2021 •

edited

Loading

denis-tingaikin Nov 7, 2021 •

edited

Loading

denis-tingaikin Nov 7, 2021 •

edited

Loading

denis-tingaikin Nov 7, 2021 •

edited

Loading

denis-tingaikin Nov 7, 2021 •

edited

Loading