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.

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

NSE Composition: Discover should update Source/Destination labels #878

Closed
Bolodya1997 opened this issue Apr 27, 2021 · 7 comments
Closed
Assignees
Labels
blocked Something is blocking this Planning The issue that related to current SOW

Comments

@Bolodya1997
Copy link

Overview

Discover chain element (or added new) should update Request.Connection.Labels to the next Source labels:

func (s *discoverServer) Request(ctx context.Context, request *networkservice.NetworkServiceRequest) (*networkservice.Connection, error) {
	nsStream, _ := nsClient.Find(ctx, &registry.NetworkServiceQuery{
		NetworkService: &registry.NetworkService{
			Name:    request.Connection.NetworkService,
			Payload: request.Connection.Payload,
		},
	})
	ns, _ := nsStream.Recv()

	sourceLabels, destinationLabels := computeLabels(request.Connection.Labels, ns.Matches) // <-- compute labels

	nseStream, _ := nseClient.Find(ctx, &registry.NetworkServiceEndpointQuery{
		NetworkServiceEndpoint: &registry.NetworkServiceEndpoint{
			NetworkServiceNames: []string{ns.Name},
			NetworkServiceLabels: map[string]string{
				ns.Name: &registry.NetworkServiceLabels{
					Labels: sourceLabels, // <-- request NSE with computed source labels
				}
			}
		},
	})
	candidates := registry.ReadNetworkServiceEndpointList(nseStream)

	request.Connection.Labels = destinationLabels // <-- set new destination labels

	conn, _ := next.Server(ctx).Request(ctx, request)

	conn.Labels = sourceLabels // <-- set back source labels on return

	return conn
}

References

Tests

  1. Unit tests in discover.
  2. Sandbox test with passthrough endpoint and composite NSE.
@denis-tingaikin
Copy link
Member

We need also somehow store labels for conn.Id in metadata map.

@Bolodya1997 Will your proposal work correctly for cases when all nses locate on different nodes with their own nsmgr?

@Bolodya1997
Copy link
Author

We need also somehow store labels for conn.Id in metadata map.

@Bolodya1997 Will your proposal work correctly for cases when all nses locate on different nodes with their own nsmgr?

Yes, each NSMgr would store destination labels for its own clients.

@edwarnicke
Copy link
Member

@Bolodya1997 NSMgr shouldn't generally be updating source destination labels except in some rare cases (like making sure we have Node, Cluster, etc labels correctly set).

The NSMgr should not be 'computing' source labels for the next hop... that's the NSEs responsiblity, not the NSMgrs.

@Bolodya1997
Copy link
Author

Bolodya1997 commented May 5, 2021

@edwarnicke
What do we actually mean by such request?

request: { NetworkService: ns, Labels: { label:a } }

1. I need ns implemented with labels { label: a }

If so, NSE just knows the labels it is implementing. It doesn't know what is the next hop in the NS route and so it cannot request right labels for the next hop.

Solution:

In such case NSMgr should find NSE by { label: a } labels and switch labels in the request to the destination labels for the source matching { label: a }.

2. I need ns and my source labels are { label: a }

If so, NSE still just knows the labels it is implementing. It doesn't know what was the previous hop in the NS route and so if NSE implementing labels for more than one NS route step, it cannot select right labels as a source for the next hop.

ns:
 - label:a -> label:b
 - step:0  -> step:1
 - label:b -> label:c
 - step:1  -> step:2
nse-1: { label:b, step:1 }
nse-2: { label:c }
nse-3: { step:2 }

If nse-1 is requested with label: a it should use label: b as a source labels. If it is requested with step: 0 it should use step: 1.

Solution:

In such case NSMgr should switch labels in request to the destination labels for the source matching { label: a } and find NSE for these new labels.

So

So it looks like for the both cases we still need NSMgr to rotate labels before requesting NSE.

@denis-tingaikin denis-tingaikin added the blocked Something is blocking this label May 5, 2021
@Bolodya1997
Copy link
Author

Bolodya1997 commented May 5, 2021

So the usecase I have meant in (2) is the following:

---
apiVersion: networkservicemesh.io/v1alpha1
kind: NetworkService
metadata:
  name: ns-example
spec:
  payload: IP
  matches:
    - match:
      route:
        - destination:
          destinationSelector:
            zip: compress
        - destination:
          destinationSelector:
            tar: compress
    - match:
      sourceSelector:
        zip: compress
      route:
        - destination:
          destinationSelector:
            zip: passthrough-1
    - match:
      sourceSelector:
        tar: compress
      route:
        - destination:
          destinationSelector:
            tar: passthrough-1
...
    - match:
      sourceSelector:
        zip: passthrough-3
      route:
        - destination:
          destinationSelector:
            zip: decompress
    - match:
      sourceSelector:
        tar: passthrough-3
      route:
        - destination:
          destinationSelector:
            tar: decompress
    - match:
      sourceSelector:
        zip: decompress
      route:
        - destination:
          destinationSelector:
            zip: vpn
    - match:
      sourceSelector:
        tar: decompress
      route:
        - destination:
          destinationSelector:
            tar: vpn

image

Client requests for ns-example and it doesn't actually know what would be the compression used for this connection. It means that we need just one of zip: compress + zip: decompress or tar: compress + tar: decompress to start ns-example working.

We probably can split this complex service into ns-example-zip and ns-example-tar but it would mean that client should now something about the compression across the way from start to vpn.

The problem with the current solution from the monorepo is the following: what labels should use passthrough-3 to make a request? It supports both zip and tar subchains but it should specify only one of them. NSMgr has enough information to resolve this issue, NSE doesn't.

@edwarnicke
Copy link
Member

@Bolodya1997 Did we sort out the confusion here over IM? If so, can we close this?

@Bolodya1997
Copy link
Author

@Bolodya1997 Did we sort out the confusion here over IM? If so, can we close this?

Yep, I will do it tomorrow after some internal communications with the team.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
blocked Something is blocking this Planning The issue that related to current SOW
Projects
None yet
Development

No branches or pull requests

4 participants