Rough draft for changes for path+healing+forwarder changes. #2050

edwarnicke · 2020-01-14T01:02:40Z

Note: This whole PR is too large, and will need to be pulled
into smaller pieces.

The purpose of this PR is to get directional feedback.

Please also note this is not quite finished yet. The goal
is to get feedback, and start breaking pieces of this off into
things which can be merged in easy stages and other folks
can start working on.

Description

Motivation and Context

How Has This Been Tested?

Covered by existing integration testing
Added integration testing to cover
Tested locally
Have not tested

Types of changes

Bug fix (non-breaking change which fixes an issue)
New feature (non-breaking change which adds functionality)
Breaking change (fix or feature that would cause existing functionality to not work as expected)

Checklist:

My change requires a change to the documentation.
I have updated the documentation accordingly.

edwarnicke · 2020-01-14T01:06:58Z

If you are going to check this out and walk through it, I recommend starting with:

https://github.com/networkservicemesh/networkservicemesh/blob/ec4610ffa577161457ccbf9142a6e9d8c45b5a41/new/cmd/k8s-nsmgr/main.go

and

https://github.com/networkservicemesh/networkservicemesh/blob/ec4610ffa577161457ccbf9142a6e9d8c45b5a41/new/sdk-vppagent/crossapi/chains/xconnect/server.go

lobkovilya · 2020-01-14T11:27:24Z

new/sdk/tools/serialize/serialize.go

+
+func NewExecutor() Executor {
+ rv := &executor{execCh: make(chan func(), 100)}
+ rv.eventLoop()


Did you mean go rv.eventLoop()?

Yes, fixed :)

lobkovilya · 2020-01-14T12:11:32Z

new/sdk/networkservicemesh/common/heal/client.go

+ }
+ }
+ for id, request := range f.requestors {
+ if _, ok := f.reported[id]; !ok && f.reported[id] != nil {


If ok equals false then f.reported[id] equals nil. That whole line is not really clear to me, maybe there is some typo?

Also correct, fixed :)

denis-tingaikin

Added a few comments.
In general, I very like these simplifications.
I'll look more and I'll provide a more detailed review.

denis-tingaikin · 2020-01-14T15:16:06Z

new/sdk/tools/serialize/serialize.go

+func NewExecutor() Executor {
+ rv := &executor{execCh: make(chan func(), 100)}
+ go rv.eventLoop()
+ runtime.SetFinalizer(rv, func(f *executor) {


It is an interesting solution for closing worker goroutine but looks like finalizedCh channel is not initialized and it could panic close of nil channel

denis-tingaikin · 2020-01-14T15:17:35Z

new/sdk/test/basic_test.go

+
+func TestBasic(t *testing.T) {
+ nsmgr_forwarder := chain.NewNetworkServiceServer(
+ authorize.NewServer(),


denis-tingaikin · 2020-01-14T15:21:23Z

new/sdk/registry/core/trace/context.go

+}
+
+// Log - return FieldLogger from context
+func Log(ctx context.Context) logrus.FieldLogger {


I saw a similar function for forwarder chained request handlers and for nsm request chained handlers.
Should we create some base sdk/context helper and put there all popular context functions to exclude code duplication?

I'm tentatively leaning towards a little bit of code repetition fo what seems to me to be a lot of increased comprehensibility.

denis-tingaikin · 2020-01-14T15:34:14Z

new/sdk/networkservicemesh/common/timeout/server.go

+
+type timeoutServer struct {
+ connections map[string]*time.Timer
+ executor serialize.Executor


Interesting is this pattern better than using map with a locker or using sync.map

It is for some uses. It preserves type safety (type checking with sync.Map is a PITA) and its guarantees are quite specific: order and one at a time. So if you have something that needs to manipulate various parts of the struct (maybe more than one) and you don't care about it happening before you go on (but it needs to happen, and in order), serializer.Executor is great.

Its not perfect for everything, but its good for a lot of things :)

Its also super simple to use :)

denis-tingaikin · 2020-01-14T15:44:00Z

new/sdk/networkservicemesh/common/heal/client.go

+ event = nil
+ }
+ f.updateExecutor.Exec(func() {
+ if event.GetType() == connection.ConnectionEventType_INITIAL_STATE_TRANSFER {


[minor] probably switch event.GetType() will look better here

Super good idea :)

Done.

Also switch-case variant can be more simple, for example:

Suggested change

if event.GetType() == connection.ConnectionEventType_INITIAL_STATE_TRANSFER {

switch event.GetType(){

case connection.ConnectionEventType_INITIAL_STATE_TRANSFER:

f.reported = event.GetConnections()

case connection.ConnectionEventType_UPDATE:

for _, conn := range event.GetConnections() {

f.reported[conn.GetId()] = conn

}

case connection.ConnectionEventType_DELETE:

for _, conn := range event.GetConnections() {

delete(f.reported, conn.GetId())

if f.requestors[conn.GetId()] != nil {

f.requestors[conn.GetId()]()

}

}

}

denis-tingaikin · 2020-01-14T15:44:30Z

pkg/tools/socket.go

@@ -137,6 +138,13 @@ func DialTCPInsecure(address string, opts ...grpc.DialOption) (*grpc.ClientConn,
 return dialCtx(context.Background(), address, opts...)
 }

+func DialUrl(u *url.URL, opts ...grpc.DialOption) (*grpc.ClientConn, error) {
+ if u.Scheme == "file" {


Should we use 'unix' here?

Yes... fixed.

denis-tingaikin · 2020-01-14T15:47:04Z

controlplane/api/connection/mechanisms/kernel/helpers.go

+ }
+ err = rv.IsValid()
+ if err != nil {
+ return nil, err


[minor] Should we use errors.WithStack here?

denis-tingaikin · 2020-01-14T16:08:35Z

new/cmd/k8s-nsmgr/pkg/deviceplugin/server.go

+ }
+ })
+ select {
+ case <-s.Context().Done():


Looks like we can get rid of using select here because we have only one channel.

Excellent point! Fixed :)

See: https://docs.google.com/presentation/d/1QU5FEq7QloLqEjJs-MMMWvcgPzkz6j-OYk-9k2gDTjc/edit#slide=id.g73e6edae28_0_0 https://drive.google.com/file/d/12AhnjrZnqwT5w8rP7Gv91xjKMgbQjPan/view?usp=sharing This is the minimal patch that can introduce the new API. As Path is replacing: repeated string network_service_managers = 6; in the Connection Message, this PR simply replaces how that was being used with Path. It does not introduce the new style healing. Signed-off-by: Ed Warnicke <[email protected]>

api has *really* clean go.mod deps. Signed-off-by: Ed Warnicke <[email protected]>

Note: This whole PR is to large, and will need to be pulled into smaller pieces. The purpose of this PR is to get directional feedback. Please also note this is not *quite* finished yet. The goal is to get feedback, and start breaking pieces of this off into things which can be merged in easy stages and other folks can start working on. Signed-off-by: Ed Warnicke <[email protected]>

Signed-off-by: Ed Warnicke <[email protected]>

lobkovilya · 2020-01-14T23:48:02Z

new/sdk/networkservicemesh/common/heal/client.go

+ cancelFunc context.CancelFunc
+}
+
+func NewClient(client connection.MonitorConnectionClient, onHeal ...networkservice.NetworkServiceClient) networkservice.NetworkServiceClient {


Why onHeal is an array? Each item in that array is networkservice.NetworkServiceClient which is potentially implies a chain. So onHeal in other words is an array of chains. But seems like we need only one chain to rerun the request, don't we?

Good point. I've fixed this to only take one onHeal.

lobkovilya · 2020-01-14T23:50:16Z

new/sdk/networkservicemesh/common/heal/client.go

+ eventReceiver: nil, // This is intentionally nil
+ recvEventExecutor: nil, // This is intentionally nil
+ }
+ rv.onHeal = append(rv.onHeal, rv)


And it is not clear why here we add ourselves to onHeal array? I think healClient already a piece of passed chain?

You are correct in the case where the client is part of a pass through Endpoint. To handle the case where the client is the origin of the chain, if a nil onHeal is passed in we add the Client itself as onHeal.

lobkovilya · 2020-01-15T00:12:59Z

new/sdk/networkservicemesh/common/refresh/client.go

+ return nil, errors.Wrapf(err, "Error creating timer from Request.Connection.Path.PathSegment[%d].ExpireTime", request.GetConnection().GetPath().GetIndex())
+ }
+ t.executor.AsyncExec(func() {
+ if timer, ok := t.connectionTimers[request.GetConnection().GetId()]; !ok || timer.Stop() {


Looks like you were about to use req here?

lobkovilya · 2020-01-15T00:13:08Z

new/sdk/networkservicemesh/common/refresh/client.go

+ }
+ t.executor.AsyncExec(func() {
+ if timer, ok := t.connectionTimers[request.GetConnection().GetId()]; !ok || timer.Stop() {
+ t.connectionTimers[request.GetConnection().GetId()] = ct


lobkovilya · 2020-01-15T00:13:35Z

new/sdk/networkservicemesh/common/heal/client.go

+ _, _ = healer.Request(ctx, req, opts...)
+ }
+ }
+ f.closers[request.GetConnection().GetId()] = func() {


lobkovilya · 2020-01-15T00:13:50Z

new/sdk/networkservicemesh/common/heal/client.go

+ // TODO handle deadline err
+ deadline, _ := ctx.Deadline()
+ duration := deadline.Sub(time.Now())
+ f.requestors[request.GetConnection().GetId()] = func() {


lobkovilya · 2020-01-15T00:19:33Z

new/sdk/networkservicemesh/common/heal/client.go

+ // TODO handle deadline err
+ deadline, _ := ctx.Deadline()
+ duration := deadline.Sub(time.Now())
+ f.requestors[request.GetConnection().GetId()] = func() {


Should modification of f.requestors map happens inside AsyncExec?

lobkovilya · 2020-01-15T00:20:18Z

new/sdk/networkservicemesh/common/heal/client.go

+ _, _ = healer.Request(ctx, req, opts...)
+ }
+ }
+ f.closers[request.GetConnection().GetId()] = func() {


And the same for f.closers?

Signed-off-by: Ed Warnicke <[email protected]>

lobkovilya reviewed Jan 14, 2020

View reviewed changes

denis-tingaikin reviewed Jan 14, 2020

View reviewed changes

edwarnicke added 9 commits January 14, 2020 15:00

Fix the last of the API change issues and cleanup so that

39858f5

api has *really* clean go.mod deps. Signed-off-by: Ed Warnicke <[email protected]>

Minor fix to serializer.

0f1b57e

Signed-off-by: Ed Warnicke <[email protected]>

Minor fix to healing code

d08ec23

Signed-off-by: Ed Warnicke <[email protected]>

Minor fix to serializer finalizeCh.

b193804

Signed-off-by: Ed Warnicke <[email protected]>

Simplifed heal with a switch statement

ab215cc

Signed-off-by: Ed Warnicke <[email protected]>

Removed redundant select in deviceplugin

3cd9316

Signed-off-by: Ed Warnicke <[email protected]>

Minor update to serializer to allow both Async and Sync Exec

9733c51

Signed-off-by: Ed Warnicke <[email protected]>

edwarnicke force-pushed the new branch from aa0a12d to 9733c51 Compare January 14, 2020 23:31

edwarnicke added 3 commits January 14, 2020 17:32

Small fix to switch in heal client.

cf33b74

Signed-off-by: Ed Warnicke <[email protected]>

Fix DialURL to use 'unix' rather than 'file'

523c60c

Signed-off-by: Ed Warnicke <[email protected]>

Make format fixes.

fddbc7e

Signed-off-by: Ed Warnicke <[email protected]>

lobkovilya reviewed Jan 14, 2020

View reviewed changes

lobkovilya reviewed Jan 15, 2020

View reviewed changes

matrohon mentioned this pull request Jan 15, 2020

[PoC, WIP] add SRIOV forwarder #1925

Draft

9 tasks

edwarnicke added 2 commits January 15, 2020 12:59

Simplification of heal to no longer take an array of onHeal.

b8d2ba8

Signed-off-by: Ed Warnicke <[email protected]>

Fixes to heal and refresh from comments

29aae25

Signed-off-by: Ed Warnicke <[email protected]>

- if event.GetType() == connection.ConnectionEventType_INITIAL_STATE_TRANSFER {
+ switch event.GetType(){
+ case connection.ConnectionEventType_INITIAL_STATE_TRANSFER:
+ f.reported = event.GetConnections()
+ case connection.ConnectionEventType_UPDATE:
+ for _, conn := range event.GetConnections() {
+ f.reported[conn.GetId()] = conn
+ }
+ case connection.ConnectionEventType_DELETE:
+ for _, conn := range event.GetConnections() {
+ delete(f.reported, conn.GetId())
+ if f.requestors[conn.GetId()] != nil {
+ f.requestors[conn.GetId()]()
+ }
+ }
+ }

Rough draft for changes for path+healing+forwarder changes. #2050

Are you sure you want to change the base?

Rough draft for changes for path+healing+forwarder changes. #2050

Conversation

edwarnicke commented Jan 14, 2020

Description

Motivation and Context

How Has This Been Tested?

Types of changes

Checklist:

edwarnicke commented Jan 14, 2020

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 left a comment

Choose a reason for hiding this comment

denis-tingaikin Jan 14, 2020 • edited

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 Jan 14, 2020 • edited

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

denis-tingaikin Jan 14, 2020 •

edited

denis-tingaikin Jan 14, 2020 •

edited