Skip to content
This repository has been archived by the owner on Aug 8, 2023. It is now read-only.

Commit

Permalink
[cni] Use container IP from store for IFB delete
Browse files Browse the repository at this point in the history 
Instead of switching into the container namespace
and getting the IP addresses from the device, lookup
the container metadata in our own datastore.

This avoids switching namespaces and simplifies the IFB
delete code a bit.

See also: golang/go#20676

[#145244693]
  • Loading branch information
@mcwumbly
mcwumbly committed Jun 21, 2017
1 parent 14689f1 commit d6e723c
Show file tree
Hide file tree
Showing 4 changed files with 37 additions and 156 deletions.
12 changes: 10 additions & 2 deletions cmd/silk-cni/main.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -254,7 +254,15 @@ func (p *CNIPlugin) cmdDel(args *skel.CmdArgs) error {
return nil // can't do teardown if no netns
}

err = p.IFBCreator.Teardown(containerNS, args.IfName)
containers, err := p.Store.ReadAll(netConf.Datastore)
if err != nil {
p.Logger.Error("find container metadata: %s", err)
}

handle := filepath.Base(args.Netns)
container := containers[handle]

err = p.IFBCreator.Teardown(container.IP)
if err != nil {
p.Logger.Error("delete-ifb", err)
// continue, keep trying to cleanup
Expand All @@ -265,7 +273,7 @@ func (p *CNIPlugin) cmdDel(args *skel.CmdArgs) error {
return typedError("teardown failed", err)
}

_, err = p.Store.Delete(netConf.Datastore, filepath.Base(args.Netns))
_, err = p.Store.Delete(netConf.Datastore, handle)
if err != nil {
p.Logger.Error("write-container-metadata", err)
}
Expand Down
50 changes: 11 additions & 39 deletions cni/lib/ifb_creator.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -5,8 +5,6 @@ import (
"net"

"code.cloudfoundry.org/silk/cni/config"
"github.com/containernetworking/plugins/pkg/ns"
multierror "github.com/hashicorp/go-multierror"
"github.com/vishvananda/netlink"
)

Expand All @@ -31,46 +29,20 @@ func (ifbCreator *IFBCreator) Create(cfg *config.Config) error {
return nil
}

func (ifb *IFBCreator) Teardown(namespace ns.NetNS, deviceName string) error {
var addrs []netlink.Addr
err := namespace.Do(func(_ ns.NetNS) error {
var err error
var link netlink.Link
link, err = ifb.NetlinkAdapter.LinkByName(deviceName)
if err != nil {
return fmt.Errorf("find link: %s", err)
}

addrs, err = ifb.NetlinkAdapter.AddrList(link, netlink.FAMILY_V4)
if err != nil {
return fmt.Errorf("list addresses: %s", err)
}
return nil
})
func (ifb *IFBCreator) Teardown(ipAddr string) error {
ifbDeviceName, err := ifb.DeviceNameGenerator.GenerateForHostIFB(net.ParseIP(ipAddr))
if err != nil {
return err
return fmt.Errorf("generate ifb device name: %s", err)
}

var ifbDeviceName string
var result *multierror.Error

for _, addr := range addrs {
ifbDeviceName, err = ifb.DeviceNameGenerator.GenerateForHostIFB(addr.IPNet.IP)

if err != nil {
result = multierror.Append(result, fmt.Errorf("generate ifb device name: %s", err))
continue
}
err = ifb.NetlinkAdapter.LinkDel(&netlink.Ifb{
LinkAttrs: netlink.LinkAttrs{
Name: ifbDeviceName,
},
})

if err != nil {
result = multierror.Append(result, fmt.Errorf("delete link: %s", err))
}
err = ifb.NetlinkAdapter.LinkDel(&netlink.Ifb{
LinkAttrs: netlink.LinkAttrs{
Name: ifbDeviceName,
},
})
if err != nil {
return fmt.Errorf("delete link: %s", err)
}

return result.ErrorOrNil()
return nil
}
125 changes: 14 additions & 111 deletions cni/lib/ifb_creator_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -87,18 +87,8 @@ var _ = Describe("IfbCreator", func() {
})

Describe("Teardown", func() {
It("removes the ifb device in the specified namespace and device name", func() {
Expect(ifbCreator.Teardown(containerNS, "device-name")).To(Succeed())

Expect(containerNS.DoCallCount()).To(Equal(1))

Expect(fakeNetlinkAdapter.LinkByNameCallCount()).To(Equal(1))
Expect(fakeNetlinkAdapter.LinkByNameArgsForCall(0)).To(Equal("device-name"))

Expect(fakeNetlinkAdapter.AddrListCallCount()).To(Equal(1))
link, family := fakeNetlinkAdapter.AddrListArgsForCall(0)
Expect(link).To(Equal(containerLink))
Expect(family).To(Equal(netlink.FAMILY_V4))
It("removes the ifb device given the container handle", func() {
Expect(ifbCreator.Teardown("10.255.30.5")).To(Succeed())

Expect(fakeDeviceNameGenerator.GenerateForHostIFBCallCount()).To(Equal(1))
Expect(fakeDeviceNameGenerator.GenerateForHostIFBArgsForCall(0)).To(Equal(net.IPv4(10, 255, 30, 5)))
Expand All @@ -110,116 +100,29 @@ var _ = Describe("IfbCreator", func() {
},
}))
})
Context("when getting the link fails", func() {

Context("when generating the device name for ifb fails", func() {
BeforeEach(func() {
fakeNetlinkAdapter.LinkByNameReturns(nil, errors.New("banana"))
fakeDeviceNameGenerator.GenerateForHostIFBReturns("", errors.New("pear"))
})
It("returns a sensible error", func() {
err := ifbCreator.Teardown(containerNS, "device-name")

It("returns a sensible error", func() {
err := ifbCreator.Teardown("some-container-handle")
Expect(err).To(HaveOccurred())
Expect(err).To(MatchError("find link: banana"))

Expect(err.Error()).To(ContainSubstring("generate ifb device name: pear"))
})
})
Context("when listing the addresses fails", func() {

Context("when deleting the link fails", func() {
BeforeEach(func() {
fakeNetlinkAdapter.AddrListReturns(nil, errors.New("apple"))
fakeNetlinkAdapter.LinkDelReturns(errors.New("mango"))
})
It("returns a sensible error", func() {
err := ifbCreator.Teardown(containerNS, "device-name")

err := ifbCreator.Teardown("some-container-handle")
Expect(err).To(HaveOccurred())
Expect(err).To(MatchError("list addresses: apple"))
})
})
Context("when there are multiple container ip addresses", func() {
BeforeEach(func() {
containerAddr2 := netlink.Addr{
IPNet: &net.IPNet{
IP: net.IPv4(10, 255, 30, 6),
},
}
fakeNetlinkAdapter.AddrListReturns([]netlink.Addr{containerAddr, containerAddr2}, nil)
fakeDeviceNameGenerator.GenerateForHostIFBStub = func(ip net.IP) (string, error) {

switch ip.String() {
case "10.255.30.5":
return "ifb-device-name", nil
case "10.255.30.6":
return "ifb-device-name-1", nil
default:
return "", nil
}
}

})

It("removes every ifb associated with every container address", func() {
Expect(ifbCreator.Teardown(containerNS, "device-name")).To(Succeed())

Expect(containerNS.DoCallCount()).To(Equal(1))

Expect(fakeNetlinkAdapter.LinkByNameCallCount()).To(Equal(1))
Expect(fakeNetlinkAdapter.LinkByNameArgsForCall(0)).To(Equal("device-name"))

Expect(fakeNetlinkAdapter.AddrListCallCount()).To(Equal(1))
link, family := fakeNetlinkAdapter.AddrListArgsForCall(0)
Expect(link).To(Equal(containerLink))
Expect(family).To(Equal(netlink.FAMILY_V4))

Expect(fakeDeviceNameGenerator.GenerateForHostIFBCallCount()).To(Equal(2))
Expect(fakeDeviceNameGenerator.GenerateForHostIFBArgsForCall(0)).To(Equal(net.IPv4(10, 255, 30, 5)))
Expect(fakeDeviceNameGenerator.GenerateForHostIFBArgsForCall(1)).To(Equal(net.IPv4(10, 255, 30, 6)))

Expect(fakeNetlinkAdapter.LinkDelCallCount()).To(Equal(2))
Expect(fakeNetlinkAdapter.LinkDelArgsForCall(0)).To(Equal(&netlink.Ifb{
LinkAttrs: netlink.LinkAttrs{
Name: "ifb-device-name",
},
}))
Expect(fakeNetlinkAdapter.LinkDelArgsForCall(1)).To(Equal(&netlink.Ifb{
LinkAttrs: netlink.LinkAttrs{
Name: "ifb-device-name-1",
},
}))
})

Context("when generating the device name for ifb fails", func() {
BeforeEach(func() {
fakeDeviceNameGenerator.GenerateForHostIFBStub = func(ip net.IP) (string, error) {
switch ip.String() {
case "10.255.30.5":
return "", errors.New("pear")
case "10.255.30.6":
return "ifb-device-name-1", nil
default:
return "", nil
}
}
})

It("returns a sensible error", func() {
err := ifbCreator.Teardown(containerNS, "device-name")
Expect(err).To(HaveOccurred())

Expect(err.Error()).To(ContainSubstring("generate ifb device name: pear"))
})
})
Context("when deleting the link fails", func() {
BeforeEach(func() {
fakeNetlinkAdapter.LinkDelStub = func(link netlink.Link) error {
if link.Attrs().Name == "ifb-device-name" {
return errors.New("mango")
}
return nil
}
})
It("returns a sensible error", func() {
err := ifbCreator.Teardown(containerNS, "device-name")
Expect(err).To(HaveOccurred())

Expect(err.Error()).To(ContainSubstring("delete link: mango"))
})
Expect(err.Error()).To(ContainSubstring("delete link: mango"))
})
})
})
Expand Down
6 changes: 2 additions & 4 deletions lib/datastore/datastore_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -48,7 +48,7 @@ var _ = Describe("Datastore", func() {
locker.OpenReturns(lockedFile, nil)
})

Context("when adding an entry to store", func() {
Describe("adding an entry to store", func() {
It("deserializes the data from the file", func() {
err := store.Add(filePath, handle, ip, metadata)
Expect(err).NotTo(HaveOccurred())
Expand Down Expand Up @@ -119,8 +119,7 @@ var _ = Describe("Datastore", func() {

})

Context("when deleting an entry from store", func() {

Describe("deleting an entry from store", func() {
It("deserializes the data from the file", func() {
_, err := store.Delete(filePath, handle)
Expect(err).NotTo(HaveOccurred())
Expand Down Expand Up @@ -182,7 +181,6 @@ var _ = Describe("Datastore", func() {
Expect(err).To(MatchError("encode and overwrite: potato"))
})
})

})

Context("when reading from datastore", func() {
Expand Down

0 comments on commit d6e723c

Please sign in to comment.