multus: simplify teardown on add error and clarify error message

Signed-off-by: Dan Williams <dcbw@redhat.com>

multus/multus.go

@@ -326,16 +326,20 @@ func cmdAdd(args *skel.CmdArgs, exec invoke.Exec, kubeClient k8s.KubeClient) (cn
 
 	var result, tmpResult cnitypes.Result
 	var netStatus []*types.NetworkStatus
-	var rt *libcni.RuntimeConf
-	lastIdx := 0
 	cniArgs := os.Getenv("CNI_ARGS")
 	for idx, delegate := range n.Delegates {
-		lastIdx = idx
 		ifName := getIfname(delegate, args.IfName, idx)
-		rt, _ = types.LoadCNIRuntimeConf(args, k8sArgs, ifName, n.RuntimeConfig)
+		rt := types.CreateCNIRuntimeConf(args, k8sArgs, ifName, n.RuntimeConfig)
 		tmpResult, err = delegateAdd(exec, ifName, delegate, rt, n.BinDir, cniArgs)
 		if err != nil {
-			break
+			// If the add failed, tear down all networks we already added
+			netName := delegate.Conf.Name
+			if netName == "" {
+				netName = delegate.ConfList.Name
+			}
+			// Ignore errors; DEL must be idempotent anyway
+			_ = delPlugins(exec, args.IfName, n.Delegates, idx, rt, n.BinDir)
+			return nil, logging.Errorf("Multus: Err adding pod to network %q: %v", netName, err)
 		}
 
 		// Master plugin result is always used if present
@@ -348,7 +352,7 @@ func cmdAdd(args *skel.CmdArgs, exec invoke.Exec, kubeClient k8s.KubeClient) (cn
 			if kc.Podnamespace != "kube-system" {
 				delegateNetStatus, err := types.LoadNetworkStatus(tmpResult, delegate.Conf.Name, delegate.MasterPlugin)
 				if err != nil {
-					return nil, logging.Errorf("Multus: Err in setting  networks status: %v", err)
+					return nil, logging.Errorf("Multus: Err in setting network status: %v", err)
 				}
 
 				netStatus = append(netStatus, delegateNetStatus)
@@ -356,12 +360,6 @@ func cmdAdd(args *skel.CmdArgs, exec invoke.Exec, kubeClient k8s.KubeClient) (cn
 		}
 	}
 
-	if err != nil {
-		// Ignore errors; DEL must be idempotent anyway
-		_ = delPlugins(exec, args.IfName, n.Delegates, lastIdx, rt, n.BinDir)
-		return nil, logging.Errorf("Multus: Err in tearing down failed plugins: %v", err)
-	}
-
 	//set the network status annotation in apiserver, only in case Multus as kubeconfig
 	if n.Kubeconfig != "" && kc != nil {
 		if kc.Podnamespace != "kube-system" {
@@ -446,7 +444,7 @@ func cmdDel(args *skel.CmdArgs, exec invoke.Exec, kubeClient k8s.KubeClient) err
 		}
 	}
 
-	rt, _ := types.LoadCNIRuntimeConf(args, k8sArgs, "", in.RuntimeConfig)
+	rt := types.CreateCNIRuntimeConf(args, k8sArgs, "", in.RuntimeConfig)
 	return delPlugins(exec, args.IfName, in.Delegates, len(in.Delegates)-1, rt, in.BinDir)
 }
 

multus/multus_test.go

@@ -245,6 +245,62 @@ var _ = Describe("multus operations", func() {
 
 	})
 
+	It("executes delegates and cleans up on failure", func() {
+		expectedConf1 := `{
+    "name": "weave1",
+    "cniVersion": "0.2.0",
+    "type": "weave-net"
+}`
+		expectedConf2 := `{
+    "name": "other1",
+    "cniVersion": "0.2.0",
+    "type": "other-plugin"
+}`
+		args := &skel.CmdArgs{
+			ContainerID: "123456789",
+			Netns:       testNS.Path(),
+			IfName:      "eth0",
+			StdinData: []byte(fmt.Sprintf(`{
+    "name": "node-cni-network",
+    "type": "multus",
+    "defaultnetworkfile": "/tmp/foo.multus.conf",
+    "defaultnetworkwaitseconds": 3,
+    "delegates": [%s,%s]
+}`, expectedConf1, expectedConf2)),
+		}
+
+		// Touch the default network file.
+		configPath := "/tmp/foo.multus.conf"
+		os.OpenFile(configPath, os.O_RDONLY|os.O_CREATE, 0755)
+
+		fExec := &fakeExec{}
+		expectedResult1 := &types020.Result{
+			CNIVersion: "0.2.0",
+			IP4: &types020.IPConfig{
+				IP: *testhelpers.EnsureCIDR("1.1.1.2/24"),
+			},
+		}
+		fExec.addPlugin(nil, "eth0", expectedConf1, expectedResult1, nil)
+
+		// This plugin invocation should fail
+		err := fmt.Errorf("expected plugin failure")
+		fExec.addPlugin(nil, "net1", expectedConf2, nil, err)
+
+		os.Setenv("CNI_COMMAND", "ADD")
+		os.Setenv("CNI_IFNAME", "eth0")
+		_, err = cmdAdd(args, fExec, nil)
+		Expect(fExec.addIndex).To(Equal(2))
+		Expect(fExec.delIndex).To(Equal(2))
+		Expect(err).To(MatchError("Multus: Err adding pod to network \"other1\": Multus: error in invoke Delegate add - \"other-plugin\": expected plugin failure"))
+
+		// Cleanup default network file.
+		if _, errStat := os.Stat(configPath); errStat == nil {
+			errRemove := os.Remove(configPath)
+			Expect(errRemove).NotTo(HaveOccurred())
+		}
+
+	})
+
 	It("executes delegates with interface name and MAC and IP addr", func() {
 		podNet := `[{"name":"net1",
 			 "interface": "test1",

types/conf.go

@@ -93,7 +93,7 @@ func LoadDelegateNetConf(bytes []byte, net *NetworkSelectionElement, deviceID st
 	return delegateConf, nil
 }
 
-func LoadCNIRuntimeConf(args *skel.CmdArgs, k8sArgs *K8sArgs, ifName string, rc *RuntimeConfig) (*libcni.RuntimeConf, error) {
+func CreateCNIRuntimeConf(args *skel.CmdArgs, k8sArgs *K8sArgs, ifName string, rc *RuntimeConfig) *libcni.RuntimeConf {
 
 	logging.Debugf("LoadCNIRuntimeConf: %v, %v, %s, %v", args, k8sArgs, ifName, rc)
 	// In part, adapted from K8s pkg/kubelet/dockershim/network/cni/cni.go#buildCNIRuntimeConf
@@ -116,7 +116,7 @@ func LoadCNIRuntimeConf(args *skel.CmdArgs, k8sArgs *K8sArgs, ifName string, rc
 			"portMappings": rc.PortMaps,
 		}
 	}
-	return rt, nil
+	return rt
 }
 
 func LoadNetworkStatus(r types.Result, netName string, defaultNet bool) (*NetworkStatus, error) {