Minor enhancements and code cleanups (#1420)

* Minor enhancement for transaction summary log

Signed-off-by: Ondrej Fabry <[email protected]>

* Add make target for remote debugging with delve

Signed-off-by: Ondrej Fabry <[email protected]>

* Minor code cleanup in vpp ifplugin

Signed-off-by: Ondrej Fabry <[email protected]>

* Fix error check when renaming tap and cleanup code

Signed-off-by: Ondrej Fabry <[email protected]>

Makefile

@@ -95,6 +95,9 @@ clean-examples:
 	cd examples/localclient_vpp/nat      	&& go clean
 	cd examples/localclient_vpp/plugins	 	&& go clean
 
+debug-remote:
+	cd ./cmd/vpp-agent && dlv debug --headless --listen=:2345 --api-version=2 --accept-multiclient
+
 # -------------------------------
 #  Testing
 # -------------------------------

plugins/kvscheduler/api/txn_options.go

@@ -268,4 +268,4 @@ func WithSimulation(ctx context.Context) context.Context {
 func IsWithSimulation(ctx context.Context) bool {
 	_, withSimulation := ctx.Value(txnSimulationCtxKey).(*txnSimulationOpt)
 	return withSimulation
-}
+}

plugins/kvscheduler/api/txn_record.go

@@ -49,9 +49,9 @@ func (t TxnType) String() string {
 	case NBTransaction:
 		return "NB Transaction"
 	case RetryFailedOps:
-		return "RETRY"
+		return "Retry Transaction"
 	}
-	return "UNKNOWN"
+	return "UndefinedTxnType"
 }
 
 // RecordedTxn is used to record executed transaction.
@@ -128,7 +128,7 @@ func (txn *RecordedTxn) StringWithOpts(resultOnly, verbose bool, indent int) str
 	if !resultOnly {
 		// transaction arguments
 		str += indent1 + "* transaction arguments:\n"
-		str += indent2 + fmt.Sprintf("- seq-num: %d\n", txn.SeqNum)
+		str += indent2 + fmt.Sprintf("- seqNum: %d\n", txn.SeqNum)
 		if txn.TxnType == NBTransaction && txn.ResyncType != NotResync {
 			ResyncType := "Full Resync"
 			if txn.ResyncType == DownstreamResync {

plugins/kvscheduler/plugin_scheduler.go

@@ -288,7 +288,7 @@ func (s *Scheduler) TransactionBarrier() {
 
 // PushSBNotification notifies about a spontaneous value change(s) in the SB
 // plane (i.e. not triggered by NB transaction).
-func (s *Scheduler) PushSBNotification(notif... kvs.KVWithMetadata) error {
+func (s *Scheduler) PushSBNotification(notif ...kvs.KVWithMetadata) error {
 	txn := &transaction{
 		txnType: kvs.SBNotification,
 	}

plugins/kvscheduler/refresh.go

@@ -571,4 +571,4 @@ func (s *Scheduler) validRetrievedDerivedKV(node graph.Node, descriptor *kvs.KVD
 		// return true -> let's overwrite invalidly retrieved derived value
 	}
 	return true
-}
+}

plugins/kvscheduler/txn_process.go

@@ -207,7 +207,7 @@ func (s *Scheduler) preProcessNBTransaction(txn *transaction) (skip bool) {
 	}
 
 	// for resync refresh the graph + collect deletes
-	graphW := s.graph.Write(true,false)
+	graphW := s.graph.Write(true, false)
 	defer graphW.Release()
 	s.resyncCount++
 
@@ -344,7 +344,7 @@ func (s *Scheduler) postProcessTransaction(txn *transaction, executed kvs.Record
 	if toRefresh.Length() > 0 {
 		// changes brought by refresh triggered solely for the verification are
 		// not saved into the graph
-		graphW := s.graph.Write(afterErrRefresh,false)
+		graphW := s.graph.Write(afterErrRefresh, false)
 		s.refreshGraph(graphW, toRefresh, nil, afterErrRefresh)
 		s.scheduleRetries(txn, graphW, toRetry)
 
@@ -420,7 +420,7 @@ func (s *Scheduler) postProcessTransaction(txn *transaction, executed kvs.Record
 
 	// delete removed values from the graph after the notifications have been sent
 	if removed.Length() > 0 {
-		graphW := s.graph.Write(true,true)
+		graphW := s.graph.Write(true, true)
 		for _, key := range removed.Iterate() {
 			graphW.DeleteNode(key)
 		}
@@ -429,7 +429,7 @@ func (s *Scheduler) postProcessTransaction(txn *transaction, executed kvs.Record
 }
 
 // scheduleRetries schedules a series of re-try transactions for failed values
-func (s *Scheduler) scheduleRetries(txn *transaction, graphR graph.ReadAccess, toRetry utils.KeySet,) {
+func (s *Scheduler) scheduleRetries(txn *transaction, graphR graph.ReadAccess, toRetry utils.KeySet) {
 	// split values based on the retry metadata
 	retryTxns := make(map[retryTxnMeta]*retryTxn)
 	for _, retryKey := range toRetry.Iterate() {

plugins/kvscheduler/txn_record.go

@@ -137,21 +137,16 @@ func (s *Scheduler) preRecordTransaction(txn *transaction, planned kvs.RecordedT
 	// if enabled, print txn summary
 	if s.config.PrintTxnSummary {
 		// build header for the log
-		txnInfo := txn.txnType.String()
+		txnInfo := ""
 		if txn.txnType == kvs.NBTransaction && txn.nb.resyncType != kvs.NotResync {
-			resyncType := "Full Resync"
-			if txn.nb.resyncType == kvs.DownstreamResync {
-				resyncType = "SB Sync"
-			}
-			if txn.nb.resyncType == kvs.UpstreamResync {
-				resyncType = "NB Sync"
-			}
-			txnInfo = fmt.Sprintf("%s (%s)", txn.txnType.String(), resyncType)
+			txnInfo = fmt.Sprintf("%s", txn.nb.resyncType.String())
+		} else if txn.txnType == kvs.RetryFailedOps && txn.retry != nil {
+			txnInfo = fmt.Sprintf("retrying TX #%d (attempt %d)", txn.retry.txnSeqNum, txn.retry.attempt)
 		}
+		msg := fmt.Sprintf("#%d - %s", record.SeqNum, txn.txnType.String())
+		n := 115 - len(msg)
 		var buf strings.Builder
 		buf.WriteString("+======================================================================================================================+\n")
-		msg := fmt.Sprintf("Transaction #%d", record.SeqNum)
-		n := 115 - len(msg)
 		buf.WriteString(fmt.Sprintf("| %s %"+strconv.Itoa(n)+"s |\n", msg, txnInfo))
 		buf.WriteString("+======================================================================================================================+\n")
 		buf.WriteString(record.StringWithOpts(false, false, 2))
@@ -172,12 +167,15 @@ func (s *Scheduler) recordTransaction(txn *transaction, txnRecord *kvs.RecordedT
 	txnRecord.Executed = executed
 
 	if s.config.PrintTxnSummary {
+		txnType := txn.txnType.String()
+		msg := fmt.Sprintf("#%d - %s", txnRecord.SeqNum, txnType)
+		elapsed := stop.Sub(start)
+		msg2 := fmt.Sprintf("took %v", elapsed.Round(time.Microsecond*100))
+
 		var buf strings.Builder
 		buf.WriteString("o----------------------------------------------------------------------------------------------------------------------o\n")
 		buf.WriteString(txnRecord.StringWithOpts(true, false, 2))
 		buf.WriteString("x----------------------------------------------------------------------------------------------------------------------x\n")
-		msg := fmt.Sprintf("#%d", txnRecord.SeqNum)
-		msg2 := fmt.Sprintf("took %v", stop.Sub(start).Round(time.Microsecond*100))
 		buf.WriteString(fmt.Sprintf("| %s %"+fmt.Sprint(115-len(msg))+"s |\n", msg, msg2))
 		buf.WriteString("x----------------------------------------------------------------------------------------------------------------------x\n")
 		fmt.Println(buf.String())

plugins/linux/ifplugin/descriptor/interface.go

@@ -27,7 +27,6 @@ import (
 	"github.com/gogo/protobuf/proto"
 	prototypes "github.com/gogo/protobuf/types"
 	"github.com/pkg/errors"
-	"github.com/vishvananda/netlink"
 	"golang.org/x/sys/unix"
 
 	"github.com/ligato/cn-infra/idxmap"
@@ -249,28 +248,42 @@ func (d *InterfaceDescriptor) MetadataFactory() idxmap.NamedMappingRW {
 
 // Validate validates Linux interface configuration.
 func (d *InterfaceDescriptor) Validate(key string, linuxIf *interfaces.Interface) error {
+	// validate name (this should never happen, since key is derived from name)
 	if linuxIf.GetName() == "" {
 		return kvs.NewInvalidValueError(ErrInterfaceWithoutName, "name")
 	}
-	addrs := linuxIf.GetIpAddresses()
-	for _, a := range addrs {
+
+	// validate IP addresses
+	for _, a := range linuxIf.GetIpAddresses() {
+		// TODO: perhaps we could assume default mask if there isnt one?
 		if _, _, err := net.ParseCIDR(a); err != nil {
 			return kvs.NewInvalidValueError(ErrInvalidIPWithMask, "ip_addresses")
 		}
 	}
 
-	if linuxIf.GetType() == interfaces.Interface_UNDEFINED {
-		return kvs.NewInvalidValueError(ErrInterfaceWithoutType, "type")
-	}
-	if linuxIf.GetType() == interfaces.Interface_TAP_TO_VPP && d.vppIfPlugin == nil {
-		return ErrTAPRequiresVPPIfPlugin
+	// validate namespace
+	if ns := linuxIf.GetNamespace(); ns != nil {
+		if ns.GetType() == namespace.NetNamespace_UNDEFINED || ns.GetReference() == "" {
+			return kvs.NewInvalidValueError(ErrNamespaceWithoutReference, "namespace")
+		}
 	}
-	if linuxIf.GetNamespace() != nil &&
-		(linuxIf.GetNamespace().GetType() == namespace.NetNamespace_UNDEFINED ||
-			linuxIf.GetNamespace().GetReference() == "") {
-		return kvs.NewInvalidValueError(ErrNamespaceWithoutReference, "namespace")
+
+	// validate type
+	switch linuxIf.GetType() {
+	case interfaces.Interface_LOOPBACK:
+		if linuxIf.GetLink() != nil {
+			return kvs.NewInvalidValueError(ErrInterfaceReferenceMismatch, "link")
+		}
+	case interfaces.Interface_TAP_TO_VPP:
+		if d.vppIfPlugin == nil {
+			return ErrTAPRequiresVPPIfPlugin
+		}
+	case interfaces.Interface_UNDEFINED:
+		return kvs.NewInvalidValueError(ErrInterfaceWithoutType, "type")
 	}
-	switch linuxIf.Link.(type) {
+
+	// validate link
+	switch linuxIf.GetLink().(type) {
 	case *interfaces.Interface_Tap:
 		if linuxIf.GetType() != interfaces.Interface_TAP_TO_VPP {
 			return kvs.NewInvalidValueError(ErrInterfaceReferenceMismatch, "link")
@@ -723,6 +736,7 @@ func (d *InterfaceDescriptor) Retrieve(correlate []adapter.InterfaceKVWithMetada
 	// receive results from the go routines
 	ifaces := make(map[string]adapter.InterfaceKVWithMetadata) // interface logical name -> interface data
 	indexes := make(map[int]struct{})                          // already retrieved interfaces by their Linux indexes
+
 	for idx := 0; idx < goRoutinesCnt; idx++ {
 		retrieved := <-ch
 		if retrieved.err != nil {
@@ -732,7 +746,7 @@ func (d *InterfaceDescriptor) Retrieve(correlate []adapter.InterfaceKVWithMetada
 			// skip if this interface was already retrieved and this is not the expected
 			// namespace from correlation - remember, the same namespace may have
 			// multiple different references
-			rewrite := false
+			var rewrite bool
 			if _, alreadyRetrieved := indexes[kv.Metadata.LinuxIfIndex]; alreadyRetrieved {
 				if expCfg, hasExpCfg := ifCfg[kv.Value.Name]; hasExpCfg {
 					if proto.Equal(expCfg.Namespace, kv.Value.Namespace) {
@@ -857,19 +871,23 @@ func (d *InterfaceDescriptor) retrieveInterfaces(nsList []*namespace.NetNamespac
 			alias = strings.TrimPrefix(alias, agentPrefix)
 
 			// parse alias to obtain logical references
-			var vppTapIfName string
-			if link.Type() == (&netlink.Veth{}).Type() {
-				var vethPeerIfName string
+			if link.Type() == "veth" {
 				iface.Type = interfaces.Interface_VETH
+				var vethPeerIfName string
 				iface.Name, vethPeerIfName = parseVethAlias(alias)
 				iface.Link = &interfaces.Interface_Veth{
-					Veth: &interfaces.VethLink{PeerIfName: vethPeerIfName},
+					Veth: &interfaces.VethLink{
+						PeerIfName: vethPeerIfName,
+					},
 				}
-			} else if link.Type() == (&netlink.Tuntap{}).Type() || link.Type() == "tun" /* not defined in vishvananda */ {
+			} else if link.Type() == "tuntap" || link.Type() == "tun" /* not defined in vishvananda */ {
 				iface.Type = interfaces.Interface_TAP_TO_VPP
+				var vppTapIfName string
 				iface.Name, vppTapIfName, _ = parseTapAlias(alias)
 				iface.Link = &interfaces.Interface_Tap{
-					Tap: &interfaces.TapLink{VppTapIfName: vppTapIfName},
+					Tap: &interfaces.TapLink{
+						VppTapIfName: vppTapIfName,
+					},
 				}
 			} else if link.Attrs().Name == defaultLoopbackName {
 				iface.Type = interfaces.Interface_LOOPBACK
@@ -878,9 +896,8 @@ func (d *InterfaceDescriptor) retrieveInterfaces(nsList []*namespace.NetNamespac
 				// unsupported interface type supposedly configured by agent => print warning
 				d.log.WithFields(logging.Fields{
 					"if-host-name": link.Attrs().Name,
-					"if-type":      link.Type(),
 					"namespace":    nsRef,
-				}).Warn("Managed interface of unsupported type")
+				}).Warnf("Managed interface of unsupported type: %s", link.Type())
 				continue
 			}
 
@@ -941,7 +958,7 @@ func (d *InterfaceDescriptor) retrieveInterfaces(nsList []*namespace.NetNamespac
 				Origin: kvs.FromNB,
 				Metadata: &ifaceidx.LinuxIfMetadata{
 					LinuxIfIndex: link.Attrs().Index,
-					VPPTapName:   vppTapIfName,
+					VPPTapName:   iface.GetTap().GetVppTapIfName(),
 					Namespace:    nsRef,
 				},
 			})
@@ -983,8 +1000,7 @@ func (d *InterfaceDescriptor) setInterfaceNamespace(ctx nslinuxcalls.NamespaceMg
 	}
 
 	// Move the interface into the namespace.
-	err = d.ifHandler.SetLinkNamespace(link, ns)
-	if err != nil {
+	if err := d.ifHandler.SetLinkNamespace(link, ns); err != nil {
 		return errors.Errorf("failed to set interface %s file descriptor: %v", link.Attrs().Name, err)
 	}
 
@@ -1009,8 +1025,7 @@ func (d *InterfaceDescriptor) setInterfaceNamespace(ctx nslinuxcalls.NamespaceMg
 		if !isIPv6 && address.IP.IsLinkLocalUnicast() {
 			continue
 		}
-		err = d.ifHandler.AddInterfaceIP(ifName, address)
-		if err != nil {
+		if err := d.ifHandler.AddInterfaceIP(ifName, address); err != nil {
 			if err.Error() == "file exists" {
 				continue
 			}
@@ -1098,7 +1113,6 @@ func getSysctl(name string) (string, error) {
 	if err != nil {
 		return "", err
 	}
-
 	return string(data[:len(data)-1]), nil
 }
 
@@ -1108,6 +1122,5 @@ func setSysctl(name, value string) (string, error) {
 	if err := ioutil.WriteFile(fullName, []byte(value), 0644); err != nil {
 		return "", err
 	}
-
 	return getSysctl(name)
 }

plugins/linux/ifplugin/descriptor/interface_tap.go

@@ -27,8 +27,10 @@ import (
 
 // createTAPToVPP moves Linux-side of the VPP-TAP interface to the destination namespace
 // and sets the requested host name, IP addresses, etc.
-func (d *InterfaceDescriptor) createTAPToVPP(nsCtx nslinuxcalls.NamespaceMgmtCtx, key string,
-	linuxIf *interfaces.Interface) (metadata *ifaceidx.LinuxIfMetadata, err error) {
+func (d *InterfaceDescriptor) createTAPToVPP(
+	nsCtx nslinuxcalls.NamespaceMgmtCtx, key string, linuxIf *interfaces.Interface,
+) (
+	md *ifaceidx.LinuxIfMetadata, err error) {
 
 	// determine TAP interface name as set by VPP ifplugin
 	vppTapName := linuxIf.GetTap().GetVppTapIfName()
@@ -45,7 +47,8 @@ func (d *InterfaceDescriptor) createTAPToVPP(nsCtx nslinuxcalls.NamespaceMgmtCtx
 	agentPrefix := d.serviceLabel.GetAgentPrefix()
 
 	// add alias to associate TAP with the logical name and VPP-TAP reference
-	err = d.ifHandler.SetInterfaceAlias(vppTapHostName, agentPrefix+getTapAlias(linuxIf, vppTapHostName))
+	alias := agentPrefix + getTapAlias(linuxIf, vppTapHostName)
+	err = d.ifHandler.SetInterfaceAlias(vppTapHostName, alias)
 	if err != nil {
 		d.log.Error(err)
 		return nil, err
@@ -67,8 +70,7 @@ func (d *InterfaceDescriptor) createTAPToVPP(nsCtx nslinuxcalls.NamespaceMgmtCtx
 	defer revert()
 
 	// rename from temporary host name to the request host name
-	d.ifHandler.RenameInterface(vppTapHostName, hostName)
-	if err != nil {
+	if err := d.ifHandler.RenameInterface(vppTapHostName, hostName); err != nil {
 		d.log.Error(err)
 		return nil, err
 	}
@@ -79,13 +81,12 @@ func (d *InterfaceDescriptor) createTAPToVPP(nsCtx nslinuxcalls.NamespaceMgmtCtx
 		d.log.Error(err)
 		return nil, err
 	}
-	metadata = &ifaceidx.LinuxIfMetadata{
+
+	return &ifaceidx.LinuxIfMetadata{
 		VPPTapName:   vppTapName,
 		Namespace:    linuxIf.Namespace,
 		LinuxIfIndex: link.Attrs().Index,
-	}
-
-	return metadata, nil
+	}, nil
 }
 
 // deleteAutoTAP returns TAP interface back to the default namespace and renames

plugins/linux/ifplugin/descriptor/interface_veth.go

@@ -26,8 +26,10 @@ import (
 
 // createVETH creates a new VETH pair if neither of VETH-ends are configured, or just
 // applies configuration to the unfinished VETH-end with a temporary host name.
-func (d *InterfaceDescriptor) createVETH(nsCtx nslinuxcalls.NamespaceMgmtCtx, key string,
-	linuxIf *interfaces.Interface) (metadata *ifaceidx.LinuxIfMetadata, err error) {
+func (d *InterfaceDescriptor) createVETH(
+	nsCtx nslinuxcalls.NamespaceMgmtCtx, key string, linuxIf *interfaces.Interface,
+) (
+	md *ifaceidx.LinuxIfMetadata, err error) {
 
 	// determine host/logical/temporary interface names
 	hostName := getHostIfName(linuxIf)
@@ -92,12 +94,11 @@ func (d *InterfaceDescriptor) createVETH(nsCtx nslinuxcalls.NamespaceMgmtCtx, ke
 		d.log.Error(err)
 		return nil, err
 	}
-	metadata = &ifaceidx.LinuxIfMetadata{
+
+	return &ifaceidx.LinuxIfMetadata{
 		Namespace:    linuxIf.Namespace,
 		LinuxIfIndex: link.Attrs().Index,
-	}
-
-	return metadata, nil
+	}, nil
 }
 
 // deleteVETH either un-configures one VETH-end if the other end is still configured, or