Contiv network is not deleted when docker network is deleted (#1029)

* Contiv network is not deleted when docker network is deleted

Steps to reproduce:
1. Create a docker network with docker network CLI
2. Ensure docker network and contiv network are created (docker network ls and netctl network ls)
3. Remove docker network.
4. Check docker network ls, docker network is deleted
5. Check netctl networ ls, contiv network is still test.

The root cause is due to the dockernetworkstate mapping is planned to delete after
contiv network is delete. However, the determination of contiv network deletion is
determined by the existance of the dockernetworkstate. Thefore, if the state is still
there, the contiv network will be unable to delete.

The fix is to delete the dockernetworkstate before proceed to contiv network deletion

Signed-off-by: Kahou Lei [email protected]

* Print out error

* Add unit test and also refactor some code to make the code more testable.

* Add more unit test and fix typos

* Add more testing

* Add key value pair in fake state

* Refactor base on comments

* Add more check in unit test.

Take out return statement when docker network is not there.

Author: kahou82

mgmtfn/dockplugin/netDriver.go

@@ -742,10 +742,27 @@ func createNetworkHelper(networkID string, tag string, IPv4Data, IPv6Data []driv
 	return nil
 }
 
-// deleteNetworkHelper removes the association between docker network and contiv network
+// deleteNetworkHelper removes the association between docker network
+// and contiv network. We have to remove docker network state before
+// remove network in contiv.
 func deleteNetworkHelper(networkID string) error {
+	dnet, err := docknet.FindDocknetByUUID(networkID)
+	if err == nil {
+		// delete the dnet oper state
+		err = docknet.DeleteDockNetState(dnet.TenantName, dnet.NetworkName, dnet.ServiceName)
+		if err != nil {
+			msg := fmt.Sprintf("Could not delete docknet for nwID %s: %s", networkID, err.Error())
+			log.Errorf(msg)
+			return errors.New(msg)
+		}
+		log.Infof("Deleted docker network mapping for %v", networkID)
+	} else {
+		msg := fmt.Sprintf("Could not find Docker network %s: %s", networkID, err.Error())
+		log.Errorf(msg)
+	}
+
 	netID := networkID + ".default"
-	_, err := utils.GetNetwork(netID)
+	_, err = utils.GetNetwork(netID)
 	if err == nil {
 		// if we find a contiv network with the ID hash, then it must be
 		// a docker created network (from the libnetwork create api).
@@ -755,21 +772,14 @@ func deleteNetworkHelper(networkID string) error {
 
 		err = cluster.MasterDelReq(url)
 		if err != nil {
-			log.Errorf("Failed to delete network: %s", err.Error())
-			return errors.New("Failed to delete network")
+			msg := fmt.Sprintf("Failed to delete network: %s", err.Error())
+			log.Errorf(msg)
+			return errors.New(msg)
 		}
 		log.Infof("Deleted contiv network %v", networkID)
 	} else {
 		log.Infof("Could not find contiv network %v", networkID)
 	}
-	dnet, err := docknet.FindDocknetByUUID(networkID)
-	if err == nil {
-		// delete the dnet oper state
-		err = docknet.DeleteDockNetState(dnet.TenantName, dnet.NetworkName, dnet.ServiceName)
-		if err != nil {
-			log.Errorf("Couldn't delete docknet for nwID %s", networkID)
-		}
-		log.Infof("Deleted docker network mapping for %v", networkID)
-	}
+
 	return nil
 }

mgmtfn/dockplugin/netDriver_test.go

@@ -0,0 +1,86 @@
+package dockplugin
+
+import (
+	"fmt"
+	"github.com/contiv/netplugin/core"
+	"github.com/contiv/netplugin/netmaster/docknet"
+	"github.com/contiv/netplugin/netmaster/mastercfg"
+	"github.com/contiv/netplugin/state"
+	"github.com/contiv/netplugin/utils"
+	"testing"
+)
+
+var stateDriver *state.FakeStateDriver
+
+// initStateDriver initialize etcd state driver
+func initFakeStateDriver(t *testing.T) {
+	instInfo := core.InstanceInfo{}
+	d, err := utils.NewStateDriver("fakedriver", &instInfo)
+	if err != nil {
+		t.Fatalf("failed to init statedriver. Error: %s", err)
+		t.Fail()
+	}
+	stateDriver = d.(*state.FakeStateDriver)
+}
+
+func deinitStateDriver() {
+	utils.ReleaseStateDriver()
+}
+
+// Ensure deleteNetworkHelper can delete docker network without issue
+func TestCreateAndDeleteNetwork(t *testing.T) {
+	// Update plugin driver for unit test
+	docknet.UpdateDockerV2PluginName("bridge", "default")
+
+	initFakeStateDriver(t)
+	defer deinitStateDriver()
+
+	tenantName := "t1"
+	networkName := "net1"
+	serviceName := ""
+	nwcfg := mastercfg.CfgNetworkState{
+		Tenant:      tenantName,
+		NetworkName: networkName,
+		PktTagType:  "vlan",
+		PktTag:      1,
+		ExtPktTag:   1,
+		SubnetIP:    "10.0.0.0",
+		SubnetLen:   24,
+		Gateway:     "10.0.0.1",
+	}
+
+	err := docknet.CreateDockNet(tenantName, networkName, "", &nwcfg)
+	if err != nil {
+		t.Fatalf("Error creating docker network. Error: %v", err)
+		t.Fail()
+	}
+
+	// Get Docker network UUID
+	dnetOper := docknet.DnetOperState{}
+	dnetOper.StateDriver = stateDriver
+
+	err = dnetOper.Read(fmt.Sprintf("%s.%s.%s", tenantName, networkName, serviceName))
+	if err != nil {
+		t.Fatalf("Unable to read network state. Error: %v", err)
+		t.Fail()
+	}
+
+	testData := make([]byte, 3)
+	networkID := mastercfg.StateConfigPath + "nets/" + dnetOper.DocknetUUID + ".default"
+	dnetOper.StateDriver.Write(networkID, testData)
+
+	// Delete Docker network by using helper
+	err = deleteNetworkHelper(dnetOper.DocknetUUID)
+
+	if err != nil {
+		t.Fatalf("Unable to delete docker network. Error: %v", err)
+		t.Fail()
+	}
+
+	// ensure network is not in the oper state
+	err = dnetOper.Read(fmt.Sprintf("%s.%s.%s", tenantName, networkName, serviceName))
+	if err == nil {
+		t.Fatalf("Fail to delete docket network")
+		t.Fail()
+	}
+}

netmaster/docknet/docknet.go

@@ -101,11 +101,12 @@ func GetDocknetName(tenantName, networkName, epgName string) string {
 	return netName
 }
 
-// UpdatePluginName update the docker v2 plugin name
-func UpdatePluginName(pluginName string) {
-	log.Infof("docker v2plugin name (%s) updated to %s", netDriverName, pluginName)
-	netDriverName = pluginName
-	ipamDriverName = pluginName
+// UpdateDockerV2PluginName update the docker v2 plugin name
+func UpdateDockerV2PluginName(netDriver string, ipamDriver string) {
+	log.Infof("docker v2plugin (%s) updated to %s and ipam (%s) updated to %s",
+		netDriverName, netDriver, ipamDriverName, ipamDriver)
+	netDriverName = netDriver
+	ipamDriverName = ipamDriver
 }
 
 // CreateDockNet Creates a network in docker daemon

netmaster/docknet/docknet_test.go

@@ -76,7 +76,7 @@ func checkDocknetCreate(t *testing.T, tenantName, networkName, serviceName, subn
 	// create a docker network
 	err := CreateDockNet(tenantName, networkName, serviceName, &nwcfg)
 	if err != nil {
-		t.Fatalf("Error creating docker ntework. Err: %v", err)
+		t.Fatalf("Error creating docker network. Err: %v", err)
 	}
 
 	// verify docknet state is created
@@ -204,8 +204,7 @@ func checkDocknetDelete(t *testing.T, tenantName, networkName, serviceName strin
 
 func TestMain(m *testing.M) {
 	// change driver names for unit-testing
-	netDriverName = "bridge"
-	ipamDriverName = "default"
+	UpdateDockerV2PluginName("bridge", "default")
 
 	initStateDriver()
 
@@ -225,3 +224,21 @@ func TestDocknetCreateDelete(t *testing.T) {
 	checkDocknetCreate(t, "unit-test", "net1", "srv1", "10.1.1.1/24", "10.1.1.254")
 	checkDocknetDelete(t, "unit-test", "net1", "srv1")
 }
+
+func TestUpdateDockerV2PluginName(t *testing.T) {
+	expectNetDriver := "bridge"
+	expectIPAMDriver := "default"
+	UpdateDockerV2PluginName("bridge", "default")
+
+	if expectNetDriver != netDriverName {
+		t.Fatalf("Unexpected netdriver name. Expected: %s. Actual: %s",
+			expectNetDriver, netDriverName)
+		t.Fail()
+	}
+
+	if expectIPAMDriver != ipamDriverName {
+		t.Fatalf("Unexpected ipamdriver name. Expected: %s. Actual: %s",
+			expectIPAMDriver, ipamDriverName)
+		t.Fail()
+	}
+}

netmaster/main.go

@@ -155,7 +155,7 @@ func execOpts(opts *cliOpts) {
 	log.Infof("Control IP:Port %s:%s", controlIP, controlURL[1])
 
 	if opts.clusterMode == master.Docker || opts.clusterMode == master.SwarmMode {
-		docknet.UpdatePluginName(opts.pluginName)
+		docknet.UpdateDockerV2PluginName(opts.pluginName, opts.pluginName)
 	}
 }