Fixes for things uncovered when vppApiErrs were considered. (#263)

networkservicemesh · Jun 13, 2021 · 66d5129 · 66d5129
1 parent eb49752
commit 66d5129
Show file tree

Hide file tree

Showing 8 changed files with 99 additions and 44 deletions.
diff --git a/pkg/networkservice/connectioncontext/mtu/client.go b/pkg/networkservice/connectioncontext/mtu/client.go
@@ -22,6 +22,7 @@ import (
 	"git.fd.io/govpp.git/api"
 	"github.com/golang/protobuf/ptypes/empty"
 	"github.com/networkservicemesh/api/pkg/api/networkservice"
+	"github.com/networkservicemesh/api/pkg/api/networkservice/payload"
 	"github.com/networkservicemesh/sdk/pkg/networkservice/core/next"
 	"google.golang.org/grpc"
 
@@ -66,10 +67,19 @@ func (m *mtuClient) Request(ctx context.Context, request *networkservice.Network
 	if err != nil {
 		return nil, err
 	}
-	if err := setVPPMTU(ctx, conn, m.vppConn, metadata.IsClient(m)); err != nil {
-		_, _ = m.Close(ctx, conn, opts...)
-		return nil, err
+	if conn.GetPayload() == payload.Ethernet {
+		if err := setVPPL2MTU(ctx, conn, m.vppConn, metadata.IsClient(m)); err != nil {
+			_, _ = m.Close(ctx, conn, opts...)
+			return nil, err
+		}
+	}
+	if conn.GetPayload() == payload.IP {
+		if err := setVPPL3MTU(ctx, conn, m.vppConn, metadata.IsClient(m)); err != nil {
+			_, _ = m.Close(ctx, conn, opts...)
+			return nil, err
+		}
 	}
+
 	return conn, nil
 }
 

diff --git a/pkg/networkservice/connectioncontext/mtu/common.go b/pkg/networkservice/connectioncontext/mtu/common.go
@@ -23,6 +23,7 @@ import (
 	"git.fd.io/govpp.git/api"
 	interfaces "github.com/edwarnicke/govpp/binapi/interface"
 	"github.com/networkservicemesh/sdk/pkg/tools/log"
+	"github.com/pkg/errors"
 
 	"github.com/networkservicemesh/api/pkg/api/networkservice"
 
@@ -33,7 +34,7 @@ const (
 	jumboFrameSize = 9000
 )
 
-func setVPPMTU(ctx context.Context, conn *networkservice.Connection, vppConn api.Connection, isClient bool) error {
+func setVPPL2MTU(ctx context.Context, conn *networkservice.Connection, vppConn api.Connection, isClient bool) error {
 	now := time.Now()
 	swIfIndex, ok := ifindex.Load(ctx, isClient)
 	if !ok || conn.GetContext().GetMTU() == 0 {
@@ -45,6 +46,13 @@ func setVPPMTU(ctx context.Context, conn *networkservice.Connection, vppConn api
 	}
 	_, err := interfaces.NewServiceClient(vppConn).HwInterfaceSetMtu(ctx, setMTU)
 	if err != nil {
+		err = errors.WithStack(err)
+		log.FromContext(ctx).
+			WithField("SwIfIndex", setMTU.SwIfIndex).
+			WithField("MTU", setMTU.Mtu).
+			WithField("duration", time.Since(now)).
+			WithField("error", err).
+			WithField("vppapi", "HwInterfaceSetMtu").Debug("error")
 		return err
 	}
 	log.FromContext(ctx).
@@ -55,6 +63,40 @@ func setVPPMTU(ctx context.Context, conn *networkservice.Connection, vppConn api
 	return nil
 }
 
+func setVPPL3MTU(ctx context.Context, conn *networkservice.Connection, vppConn api.Connection, isClient bool) error {
+	now := time.Now()
+	swIfIndex, ok := ifindex.Load(ctx, isClient)
+	if !ok || conn.GetContext().GetMTU() == 0 {
+		return nil
+	}
+	setMTU := &interfaces.SwInterfaceSetMtu{
+		SwIfIndex: swIfIndex,
+		Mtu: []uint32{
+			conn.GetContext().GetMTU(),
+			conn.GetContext().GetMTU(),
+			conn.GetContext().GetMTU(),
+			conn.GetContext().GetMTU(),
+		},
+	}
+	_, err := interfaces.NewServiceClient(vppConn).SwInterfaceSetMtu(ctx, setMTU)
+	if err != nil {
+		err = errors.WithStack(err)
+		log.FromContext(ctx).
+			WithField("SwIfIndex", setMTU.SwIfIndex).
+			WithField("MTU", setMTU.Mtu).
+			WithField("duration", time.Since(now)).
+			WithField("error", err).
+			WithField("vppapi", "SwInterfaceSetMtu").Debug("error")
+		return err
+	}
+	log.FromContext(ctx).
+		WithField("SwIfIndex", setMTU.SwIfIndex).
+		WithField("MTU", setMTU.Mtu).
+		WithField("duration", time.Since(now)).
+		WithField("vppapi", "SwInterfaceSetMtu").Debug("completed")
+	return nil
+}
+
 func setConnContextMTU(request *networkservice.NetworkServiceRequest) {
 	if request.GetConnection().GetContext().GetMTU() != 0 {
 		return

diff --git a/pkg/networkservice/connectioncontext/mtu/server.go b/pkg/networkservice/connectioncontext/mtu/server.go
@@ -22,6 +22,7 @@ import (
 	"git.fd.io/govpp.git/api"
 	"github.com/golang/protobuf/ptypes/empty"
 	"github.com/networkservicemesh/api/pkg/api/networkservice"
+	"github.com/networkservicemesh/api/pkg/api/networkservice/payload"
 	"github.com/networkservicemesh/sdk/pkg/networkservice/core/next"
 	"github.com/networkservicemesh/sdk/pkg/networkservice/utils/metadata"
 )
@@ -64,9 +65,17 @@ func (m *mtuServer) Request(ctx context.Context, request *networkservice.Network
 	if err != nil {
 		return nil, err
 	}
-	if err := setVPPMTU(ctx, conn, m.vppConn, metadata.IsClient(m)); err != nil {
-		_, _ = m.Close(ctx, conn)
-		return nil, err
+	if conn.GetPayload() == payload.Ethernet {
+		if err := setVPPL2MTU(ctx, conn, m.vppConn, metadata.IsClient(m)); err != nil {
+			_, _ = m.Close(ctx, conn)
+			return nil, err
+		}
+	}
+	if conn.GetPayload() == payload.IP {
+		if err := setVPPL3MTU(ctx, conn, m.vppConn, metadata.IsClient(m)); err != nil {
+			_, _ = m.Close(ctx, conn)
+			return nil, err
+		}
 	}
 	return conn, nil
 }

diff --git a/pkg/networkservice/up/client.go b/pkg/networkservice/up/client.go
@@ -1,4 +1,4 @@
-// Copyright (c) 2020 Cisco and/or its affiliates.
+// Copyright (c) 2020-2021 Cisco and/or its affiliates.
 //
 // SPDX-License-Identifier: Apache-2.0
 //
@@ -21,17 +21,16 @@ import (
 	"context"
 	"sync"
 
-	"git.fd.io/govpp.git/api"
 	"github.com/golang/protobuf/ptypes/empty"
 	"github.com/networkservicemesh/api/pkg/api/networkservice"
 	"github.com/networkservicemesh/sdk/pkg/networkservice/core/next"
+	"github.com/networkservicemesh/sdk/pkg/networkservice/utils/metadata"
 	"google.golang.org/grpc"
 )
 
 type upClient struct {
-	ctx        context.Context
-	vppConn    Connection
-	apiChannel api.Channel
+	ctx     context.Context
+	vppConn Connection
 	sync.Once
 	initErr error
 }
@@ -54,7 +53,7 @@ func (u *upClient) Request(ctx context.Context, request *networkservice.NetworkS
 		return nil, err
 	}
 
-	if err := up(ctx, u.vppConn, u.apiChannel, true); err != nil {
+	if err := up(ctx, u.vppConn, metadata.IsClient(u)); err != nil {
 		_, _ = u.Close(ctx, conn, opts...)
 		return nil, err
 	}
@@ -67,7 +66,7 @@ func (u *upClient) Close(ctx context.Context, conn *networkservice.Connection, o
 
 func (u *upClient) init(ctx context.Context) error {
 	u.Do(func() {
-		u.apiChannel, u.initErr = initFunc(ctx, u.vppConn)
+		u.initErr = initFunc(ctx, u.vppConn)
 	})
 	return u.initErr
 }
diff --git a/pkg/networkservice/up/common.go b/pkg/networkservice/up/common.go
@@ -36,12 +36,18 @@ type Connection interface {
 	api.ChannelProvider
 }
 
-func up(ctx context.Context, vppConn api.Connection, apiChannel api.Channel, isClient bool) error {
+func up(ctx context.Context, vppConn Connection, isClient bool) error {
 	swIfIndex, ok := ifindex.Load(ctx, isClient)
 	if !ok {
 		return nil
 	}
 
+	apiChannel, err := vppConn.NewAPIChannelBuffered(256, 256)
+	if err != nil {
+		return errors.WithStack(err)
+	}
+	defer apiChannel.Close()
+
 	now := time.Now()
 	if _, err := interfaces.NewServiceClient(vppConn).SwInterfaceSetFlags(ctx, &interfaces.SwInterfaceSetFlags{
 		SwIfIndex: swIfIndex,
@@ -110,27 +116,22 @@ func waitForUpLinkUp(ctx context.Context, vppConn api.Connection, apiChannel api
 	}
 }
 
-func initFunc(ctx context.Context, vppConn Connection) (api.Channel, error) {
-	apiChannel, err := vppConn.NewAPIChannelBuffered(256, 256)
-	if err != nil {
-		return nil, errors.WithStack(err)
-	}
-
+func initFunc(ctx context.Context, vppConn api.Connection) error {
 	now := time.Now()
-	if _, err = interfaces.NewServiceClient(vppConn).WantInterfaceEvents(ctx, &interfaces.WantInterfaceEvents{
+	_, err := interfaces.NewServiceClient(vppConn).WantInterfaceEvents(ctx, &interfaces.WantInterfaceEvents{
 		EnableDisable: 1,
 		PID:           uint32(os.Getpid()),
-	}); err != nil {
-		apiChannel.Close()
-		return nil, errors.WithStack(err)
+	})
+	// If we've already registered, then we are done here.  api.INVALID_REGISTRATION  is returned when we attempt to
+	// register for the second time.
+	if vppAPIError, ok := err.(api.VPPApiError); ok && vppAPIError == api.INVALID_REGISTRATION {
+		return nil
+	}
+	if err != nil {
+		return errors.WithStack(err)
 	}
 	log.FromContext(ctx).
 		WithField("duration", time.Since(now)).
 		WithField("vppapi", "WantInterfaceEvents").Info("completed")
-
-	go func() {
-		<-ctx.Done()
-		apiChannel.Close()
-	}()
-	return apiChannel, nil
+	return nil
 }
diff --git a/pkg/networkservice/up/server.go b/pkg/networkservice/up/server.go
@@ -20,16 +20,14 @@ import (
 	"context"
 	"sync"
 
-	"git.fd.io/govpp.git/api"
 	"github.com/golang/protobuf/ptypes/empty"
 	"github.com/networkservicemesh/api/pkg/api/networkservice"
 	"github.com/networkservicemesh/sdk/pkg/networkservice/core/next"
 )
 
 type upServer struct {
-	ctx        context.Context
-	vppConn    Connection
-	apiChannel api.Channel
+	ctx     context.Context
+	vppConn Connection
 	sync.Once
 	initErr error
 }
@@ -51,11 +49,11 @@ func (u *upServer) Request(ctx context.Context, request *networkservice.NetworkS
 		return nil, err
 	}
 
-	if err := up(ctx, u.vppConn, u.apiChannel, true); err != nil {
+	if err := up(ctx, u.vppConn, true); err != nil {
 		_, _ = u.Close(ctx, conn)
 		return nil, err
 	}
-	if err := up(ctx, u.vppConn, u.apiChannel, false); err != nil {
+	if err := up(ctx, u.vppConn, false); err != nil {
 		_, _ = u.Close(ctx, conn)
 		return nil, err
 	}
@@ -68,7 +66,7 @@ func (u *upServer) Close(ctx context.Context, conn *networkservice.Connection) (
 
 func (u *upServer) init(ctx context.Context) error {
 	u.Do(func() {
-		u.apiChannel, u.initErr = initFunc(ctx, u.vppConn)
+		u.initErr = initFunc(ctx, u.vppConn)
 	})
 	return u.initErr
 }
diff --git a/pkg/networkservice/xconnect/l2xconnect/server.go b/pkg/networkservice/xconnect/l2xconnect/server.go
@@ -57,12 +57,10 @@ func (v *l2XconnectServer) Close(ctx context.Context, conn *networkservice.Conne
 	if conn.GetPayload() != payload.Ethernet {
 		return next.Server(ctx).Close(ctx, conn)
 	}
+	_ = addDel(ctx, v.vppConn, false)
 	rv, err := next.Server(ctx).Close(ctx, conn)
 	if err != nil {
 		return nil, err
 	}
-	if err := addDel(ctx, v.vppConn, false); err != nil {
-		return nil, err
-	}
 	return rv, nil
 }
diff --git a/pkg/networkservice/xconnect/l3xconnect/server.go b/pkg/networkservice/xconnect/l3xconnect/server.go
@@ -58,12 +58,10 @@ func (v *l3XconnectServer) Close(ctx context.Context, conn *networkservice.Conne
 	if conn.GetPayload() != payload.IP {
 		return next.Server(ctx).Close(ctx, conn)
 	}
+	_ = del(ctx, v.vppConn)
 	rv, err := next.Server(ctx).Close(ctx, conn)
 	if err != nil {
 		return nil, err
 	}
-	if err := del(ctx, v.vppConn); err != nil {
-		return nil, err
-	}
 	return rv, nil
 }