From 51d6bbaf234f360b35ea79468b18ae1823ad6f44 Mon Sep 17 00:00:00 2001 From: gunjan5 Date: Mon, 30 Oct 2017 18:07:41 -0700 Subject: [PATCH] bugfix: CNI plugin throws file exists while programming container route sometimes --- .gitignore | 1 + Makefile | 26 +++++++++++++++++++++++--- calico_cni_test.go | 44 ++++++++++++++++++++++++++++++++++++++++++++ glide.lock | 9 ++++----- utils/network.go | 28 ++++++++++++++-------------- 5 files changed, 86 insertions(+), 22 deletions(-) diff --git a/.gitignore b/.gitignore index 930abf431..ad281e4bf 100644 --- a/.gitignore +++ b/.gitignore @@ -14,3 +14,4 @@ tmp-cni k8s-install/scripts/tmp/ install_cni.test *.swp +*.coverprofile diff --git a/Makefile b/Makefile index fdb650cbe..5a41f518e 100644 --- a/Makefile +++ b/Makefile @@ -140,7 +140,7 @@ test-containerized: run-etcd run-k8s-apiserver build-containerized dist/host-loc -v $(CURDIR):/go/src/github.com/projectcalico/cni-plugin:rw \ $(CALICO_BUILD) sh -c '\ cd /go/src/github.com/projectcalico/cni-plugin && \ - ginkgo' + make run-tests' make stop-etcd # This does not currently work, kubernetes needs additional configuration @@ -179,11 +179,11 @@ run-test-containerized-without-building: run-etcd run-k8s-apiserver -v $(CURDIR):/go/src/github.com/projectcalico/cni-plugin:rw \ $(CALICO_BUILD) sh -c '\ cd /go/src/github.com/projectcalico/cni-plugin && \ - ginkgo' + make run-tests' make stop-etcd ## Run the tests in a container (as root) for different CNI spec versions -## to make sure we don't break backwards compatiblity. +## to make sure we don't break backwards compatibility. .PHONY: test-containerized-cni-versions test-containerized-cni-versions: build-containerized dist/host-local; for cniversion in "0.2.0" "0.3.1" ; do \ @@ -204,6 +204,26 @@ build-containerized: vendor cd /go/src/github.com/projectcalico/cni-plugin && \ make binary' +.PHONY: run-tests +## Run the tests locally, must have local etcd running +run-tests: + # Run tests recursively (-r). + ginkgo -cover -r -skipPackage vendor -skipPackage k8s-install + + @echo + @echo '+==============+' + @echo '| All coverage |' + @echo '+==============+' + @echo + @find . -iname '*.coverprofile' | xargs -I _ go tool cover -func=_ + + @echo + @echo '+==================+' + @echo '| Missing coverage |' + @echo '+==================+' + @echo + @find . -iname '*.coverprofile' | xargs -I _ go tool cover -func=_ | grep -v '100.0%' + ## Etcd is used by the tests run-etcd: stop-etcd docker run --detach \ diff --git a/calico_cni_test.go b/calico_cni_test.go index 23ea471c3..7ec6297da 100644 --- a/calico_cni_test.go +++ b/calico_cni_test.go @@ -505,4 +505,48 @@ var _ = Describe("CalicoCni", func() { }) }) }) + + Describe("SetupRoutes works fine when the route is already programmed", func() { + Context("container route already exists on the host", func() { + netconf := fmt.Sprintf(` + { + "cniVersion": "%s", + "name": "net1", + "type": "calico", + "etcd_endpoints": "http://%s:2379", + "datastore_type": "%s", + "ipam": { + "type": "host-local", + "subnet": "10.0.0.0/8" + }, + "log_level":"debug" + }`, cniVersion, os.Getenv("ETCD_IP"), os.Getenv("DATASTORE_TYPE")) + + It("route setup should be resilient to existing route", func() { + By("creating a CNI networked container, which should also install the container route in the host namespace") + containerID, session, _, _, _, contNs, err := testutils.CreateContainerWithId(netconf, "", testutils.TEST_DEFAULT_NS, "", "meep1337") + Expect(err).ShouldNot(HaveOccurred()) + Eventually(session).Should(gexec.Exit()) + + // CNI plugin generates host side vEth name from containerID if used for "cni" orchestrator. + hostVethName := "cali" + containerID[:utils.Min(11, len(containerID))] //"cali" + containerID + hostVeth, err := netlink.LinkByName(hostVethName) + Expect(err).ToNot(HaveOccurred()) + + result, err := testutils.GetResultForCurrent(session, cniVersion) + if err != nil { + log.Fatalf("Error getting result from the session: %v\n", err) + } + + log.Printf("Unmarshalled result: %v\n", result) + + By("setting up the same route CNI plugin installed in the initial run for the hostVeth") + err = utils.SetupRoutes(hostVeth, result) + Expect(err).NotTo(HaveOccurred()) + + _, err = testutils.DeleteContainerWithId(netconf, contNs.Path(), "", testutils.TEST_DEFAULT_NS, containerID) + Expect(err).ShouldNot(HaveOccurred()) + }) + }) + }) }) diff --git a/glide.lock b/glide.lock index c759de08b..45402e5dd 100644 --- a/glide.lock +++ b/glide.lock @@ -1,5 +1,5 @@ hash: 7cbe04139d9022924c0b4c3cbbef9364acb6716ef161ca577f6bb32757d32683 -updated: 2017-11-21T16:56:20.20326469Z +updated: 2017-11-21T10:06:40.064431162-08:00 imports: - name: cloud.google.com/go version: 3b1ae45394a234c385be014e9a488f2bb6eef821 @@ -132,7 +132,7 @@ imports: subpackages: - pbutil - name: github.com/mcuadros/go-version - version: 88e56e02bea1c203c99222c365fa52a69996ccac + version: 257f7b9a7d87427c8d7f89469a5958d57f8abd7c - name: github.com/onsi/ginkgo version: 9eda700730cba42af70d53180f9dcce9266bc2bc subpackages: @@ -259,7 +259,7 @@ imports: - name: github.com/vishvananda/netns version: 54f0e4339ce73702a0607f49922aaa1e749b418d - name: golang.org/x/crypto - version: 81e90905daefcd6fd217b62423c0908922eadb30 + version: 1351f936d976c60a0a48d728281922cf63eafb8d subpackages: - ssh/terminal - name: golang.org/x/net @@ -288,7 +288,6 @@ imports: version: 076b546753157f758b316e59bcb51e6807c04057 subpackages: - unix - - windows - name: golang.org/x/text version: b19bf474d317b857955b12035d2c5acb57ce8b01 subpackages: @@ -315,7 +314,7 @@ imports: - unicode/norm - width - name: google.golang.org/appengine - version: 9d8544a6b2c7df9cff240fcf92d7b2f59bc13416 + version: 12d5545dc1cfa6047a286d5e853841b6471f4c19 subpackages: - internal - internal/app_identity diff --git a/utils/network.go b/utils/network.go index cc4d1bb54..2e19d8a1d 100644 --- a/utils/network.go +++ b/utils/network.go @@ -5,7 +5,7 @@ import ( "io" "net" "os" - "reflect" + "syscall" "github.com/containernetworking/cni/pkg/skel" "github.com/containernetworking/cni/pkg/types/current" @@ -194,7 +194,7 @@ func DoNetworking(args *skel.CmdArgs, conf types.NetConf, result *current.Result } // Now that the host side of the veth is moved, state set to UP, and configured with sysctls, we can add the routes to it in the host namespace. - err = setupRoutes(hostVeth, result) + err = SetupRoutes(hostVeth, result) if err != nil { return "", "", fmt.Errorf("error adding host side routes for interface: %s, error: %s", hostVeth.Attrs().Name, err) } @@ -202,10 +202,8 @@ func DoNetworking(args *skel.CmdArgs, conf types.NetConf, result *current.Result return hostVethName, contVethMAC, err } -var errFileExists = fmt.Errorf("file exists") - -// setupRoutes sets up the routes for the host side of the veth pair. -func setupRoutes(hostVeth netlink.Link, result *current.Result) error { +// SetupRoutes sets up the routes for the host side of the veth pair. +func SetupRoutes(hostVeth netlink.Link, result *current.Result) error { // Go through all the IPs and add routes for each IP in the result. for _, ipAddr := range result.IPs { @@ -220,7 +218,7 @@ func setupRoutes(hostVeth netlink.Link, result *current.Result) error { switch err { // Route already exists, but not necessarily pointing to the same interface. - case errFileExists: + case syscall.EEXIST: // List all the routes for the interface. routes, err := netlink.RouteList(hostVeth, netlink.FAMILY_ALL) if err != nil { @@ -231,22 +229,24 @@ func setupRoutes(hostVeth netlink.Link, result *current.Result) error { // exactly what we are intending to program. // If the route we want is already there then most likely it's programmed by Felix, so we ignore it, // and we return an error if none of the routes match the route we're trying to program. + logrus.WithFields(logrus.Fields{"route": route, "scope": route.Scope}).Debug("Constructed route") for _, r := range routes { - if reflect.DeepEqual(r, route) { + logrus.WithFields(logrus.Fields{"interface": hostVeth.Attrs().Name, "route": r, "scope": r.Scope}).Debug("Routes for the interface") + if r.LinkIndex == route.LinkIndex && r.Dst.IP.Equal(route.Dst.IP) && r.Scope == route.Scope { // Route was already present on the host. - logrus.Infof("CNI skipping add route. Route already exists for %s\n", hostVeth.Attrs().Name) + logrus.WithFields(logrus.Fields{"interface": hostVeth.Attrs().Name}).Infof("CNI skipping add route. Route already exists") return nil } } - return fmt.Errorf("route (Dst: %s, Scope: %v) already exists for an interface other than '%s'", - route.Dst.String(), route.Scope, hostVeth.Attrs().Name) + return fmt.Errorf("route (Ifindex: %d, Dst: %s, Scope: %v) already exists for an interface other than '%s'", + route.LinkIndex, route.Dst.String(), route.Scope, hostVeth.Attrs().Name) default: - return fmt.Errorf("failed to add route (Dst: %s, Scope: %v, Iface: %s): %v", - route.Dst.String(), route.Scope, hostVeth.Attrs().Name, err) + return fmt.Errorf("failed to add route (Ifindex: %d, Dst: %s, Scope: %v, Iface: %s): %v", + route.LinkIndex, route.Dst.String(), route.Scope, hostVeth.Attrs().Name, err) } } - logrus.Debugf("CNI adding route for interface: %v, IP: %s", hostVeth, ipAddr.Address) + logrus.WithFields(logrus.Fields{"interface": hostVeth, "IP": ipAddr.Address}).Debugf("CNI adding route") } return nil }