diff --git a/cni/ipam/ipam.go b/cni/ipam/ipam.go index 785e05616e..2590e8f6bb 100644 --- a/cni/ipam/ipam.go +++ b/cni/ipam/ipam.go @@ -149,20 +149,23 @@ func (plugin *ipamPlugin) Add(args *cniSkel.CmdArgs) error { return err } + // assign the container id + options := make(map[string]string) + options[ipam.OptAddressID] = args.ContainerID + // Check if an address pool is specified. if nwCfg.Ipam.Subnet == "" { var poolID string var subnet string - // Select the requested interface. - options := make(map[string]string) - options[ipam.OptInterfaceName] = nwCfg.Master - isIpv6 := false if nwCfg.Ipam.Type == ipamV6 { isIpv6 = true } + // Select the requested interface. + options[ipam.OptInterfaceName] = nwCfg.Master + // Allocate an address pool. poolID, subnet, err = plugin.am.RequestPool(nwCfg.Ipam.AddrSpace, "", "", options, isIpv6) if err != nil { @@ -183,7 +186,7 @@ func (plugin *ipamPlugin) Add(args *cniSkel.CmdArgs) error { } // Allocate an address for the endpoint. - address, err := plugin.am.RequestAddress(nwCfg.Ipam.AddrSpace, nwCfg.Ipam.Subnet, nwCfg.Ipam.Address, nil) + address, err := plugin.am.RequestAddress(nwCfg.Ipam.AddrSpace, nwCfg.Ipam.Subnet, nwCfg.Ipam.Address, options) if err != nil { err = plugin.Errorf("Failed to allocate address: %v", err) return err @@ -193,7 +196,7 @@ func (plugin *ipamPlugin) Add(args *cniSkel.CmdArgs) error { defer func() { if err != nil && address != "" { log.Printf("[cni-ipam] Releasing address %v.", address) - plugin.am.ReleaseAddress(nwCfg.Ipam.AddrSpace, nwCfg.Ipam.Subnet, address, nil) + plugin.am.ReleaseAddress(nwCfg.Ipam.AddrSpace, nwCfg.Ipam.Subnet, address, options) } }() @@ -280,23 +283,20 @@ func (plugin *ipamPlugin) Delete(args *cniSkel.CmdArgs) error { return err } - // If an address is specified, release that address. Otherwise, release the pool. - if nwCfg.Ipam.Address != "" { - // Release the address. - err := plugin.am.ReleaseAddress(nwCfg.Ipam.AddrSpace, nwCfg.Ipam.Subnet, nwCfg.Ipam.Address, nil) - if err != nil { - err = plugin.Errorf("Failed to release address: %v", err) - return err - } - } else { - // Release the pool. - err := plugin.am.ReleasePool(nwCfg.Ipam.AddrSpace, nwCfg.Ipam.Subnet) - if err != nil { - err = plugin.Errorf("Failed to release pool: %v", err) - return err - } + // Select the requested interface. + options := make(map[string]string) + options[ipam.OptAddressID] = args.ContainerID + + err = plugin.am.ReleaseAddress(nwCfg.Ipam.AddrSpace, nwCfg.Ipam.Subnet, nwCfg.Ipam.Address, options) + + if err != nil { + err = plugin.Errorf("Failed to release address: %v", err) + return err } + // Release the pool. + plugin.am.ReleasePool(nwCfg.Ipam.AddrSpace, nwCfg.Ipam.Subnet) + return nil } diff --git a/cni/ipam/ipam_test.go b/cni/ipam/ipam_test.go index 5a36ec15b7..d480e31b1e 100644 --- a/cni/ipam/ipam_test.go +++ b/cni/ipam/ipam_test.go @@ -8,15 +8,16 @@ import ( "fmt" cniSkel "github.com/containernetworking/cni/pkg/skel" cniTypesCurr "github.com/containernetworking/cni/pkg/types/current" + "github.com/google/uuid" . "github.com/onsi/ginkgo" . "github.com/onsi/gomega" + "net" "net/http" "net/url" "testing" "time" "github.com/Azure/azure-container-networking/common" - "github.com/Azure/azure-container-networking/platform" ) var ipamQueryUrl = "localhost:42424" @@ -27,6 +28,7 @@ var ipamQueryResponse = "" + " " + " " + " " + + " " + " " + " " + "" @@ -59,15 +61,24 @@ func getStdinData(cniversion, subnet, ipAddress string) []byte { "ipAddress": "%s" } }`, cniversion, subnet, ipAddress) + return []byte(stdinData) } var ( + plugin *ipamPlugin + testAgent *common.Listener + arg *cniSkel.CmdArgs + err error + endpointID1 = uuid.New().String() + + //this usedAddresses map is to test not duplicate IP's + // have been provided throughout this test execution + UsedAddresses = map[string]string{} - plugin *ipamPlugin - testAgent *common.Listener - arg *cniSkel.CmdArgs - err error + //below is the network,used to test if the IP's provided by IPAM + // is in the the space requested. + network = net.IPNet{IP: net.ParseIP("10.0.0.0"), Mask: net.CIDRMask(16, 32)} _ = BeforeSuite(func() { // TODO: Ensure that the other testAgent has bees released. @@ -125,10 +136,12 @@ var ( Expect(err).ShouldNot(HaveOccurred()) result, err = parseResult(arg.StdinData) Expect(err).ShouldNot(HaveOccurred()) - address1, _ := platform.ConvertStringToIPNet("10.0.0.5/16") - address2, _ := platform.ConvertStringToIPNet("10.0.0.6/16") - Expect(result.IPs[0].Address.IP).Should(Or(Equal(address1.IP), Equal(address2.IP))) - Expect(result.IPs[0].Address.Mask).Should(Equal(address1.Mask)) + + AssertAddressNotInUse(result.IPs[0].Address.IP.String()) + + TrackAddressUsage(result.IPs[0].Address.IP.String(), "") + + AssertProperAddressSpace(result.IPs[0].Address) }) }) @@ -137,6 +150,9 @@ var ( arg.StdinData = getStdinData("0.4.0", "10.0.0.0/16", result.IPs[0].Address.IP.String()) err = plugin.Delete(arg) Expect(err).ShouldNot(HaveOccurred()) + + delete(UsedAddresses, result.IPs[0].Address.IP.String()) + }) }) @@ -158,9 +174,12 @@ var ( Expect(err).ShouldNot(HaveOccurred()) result, err := parseResult(arg.StdinData) Expect(err).ShouldNot(HaveOccurred()) - address, _ := platform.ConvertStringToIPNet("10.0.0.6/16") - Expect(result.IPs[0].Address.IP).Should(Equal(address.IP)) - Expect(result.IPs[0].Address.Mask).Should(Equal(address.Mask)) + + AssertAddressNotInUse(result.IPs[0].Address.IP.String()) + + TrackAddressUsage(result.IPs[0].Address.IP.String(), "") + + AssertProperAddressSpace(result.IPs[0].Address) }) }) @@ -168,31 +187,86 @@ var ( It("Request a usable address successfully", func() { arg.StdinData = getStdinData("0.4.0", "10.0.0.0/16", "") err = plugin.Add(arg) + + Expect(err).ShouldNot(HaveOccurred()) + result, err := parseResult(arg.StdinData) + Expect(err).ShouldNot(HaveOccurred()) + + AssertAddressNotInUse(result.IPs[0].Address.IP.String()) + + TrackAddressUsage(result.IPs[0].Address.IP.String(), "") + + AssertProperAddressSpace(result.IPs[0].Address) + }) + }) + + Context("When container id is given with subnet", func() { + It("Request a usable address successfully", func() { + arg.StdinData = getStdinData("0.4.0", "10.0.0.0/16", "") + arg.ContainerID = endpointID1 + err = plugin.Add(arg) + Expect(err).ShouldNot(HaveOccurred()) result, err := parseResult(arg.StdinData) Expect(err).ShouldNot(HaveOccurred()) - address, _ := platform.ConvertStringToIPNet("10.0.0.5/16") - Expect(result.IPs[0].Address.IP).Should(Equal(address.IP)) - Expect(result.IPs[0].Address.Mask).Should(Equal(address.Mask)) + + AssertAddressNotInUse(result.IPs[0].Address.IP.String()) + + TrackAddressUsage(result.IPs[0].Address.IP.String(), arg.ContainerID) + + AssertProperAddressSpace(result.IPs[0].Address) + + //release the container ID for next test + arg.ContainerID = "" }) }) }) Describe("Test IPAM DELETE", func() { + Context("Delete when only container id is given", func() { + It("Deleted", func() { + arg.StdinData = getStdinData("0.4.0", "10.0.0.0/16", "") + arg.ContainerID = endpointID1 + + err = plugin.Delete(arg) + Expect(err).ShouldNot(HaveOccurred()) + + address := UsedAddresses[arg.ContainerID] + + RemoveAddressUsage(address, arg.ContainerID) + + }) + }) + Context("When address and subnet is given", func() { It("Release address successfully", func() { - arg.StdinData = getStdinData("0.4.0", "10.0.0.0/16", "10.0.0.5") + + nextAddress := GetNextAddress() + arg.StdinData = getStdinData("0.4.0", "10.0.0.0/16", nextAddress) err = plugin.Delete(arg) Expect(err).ShouldNot(HaveOccurred()) + + RemoveAddressUsage(nextAddress, "") + }) + }) + + Context("When pool is in use", func() { + It("Fail to request pool", func() { + arg.StdinData = getStdinData("0.4.0", "", "") + err = plugin.Add(arg) + Expect(err).Should(HaveOccurred()) }) }) Context("When address and subnet is given", func() { It("Release address successfully", func() { - arg.StdinData = getStdinData("0.4.0", "10.0.0.0/16", "10.0.0.6") + nextAddress := GetNextAddress() + arg.StdinData = getStdinData("0.4.0", "10.0.0.0/16", nextAddress) err = plugin.Delete(arg) Expect(err).ShouldNot(HaveOccurred()) + + RemoveAddressUsage(nextAddress, "") }) }) @@ -203,6 +277,54 @@ var ( Expect(err).ShouldNot(HaveOccurred()) }) }) + + Context("When pool is not use", func() { + It("Confirm pool was released by succesfully requesting pool", func() { + arg.StdinData = getStdinData("0.4.0", "", "") + err = plugin.Add(arg) + Expect(err).ShouldNot(HaveOccurred()) + }) + }) }) }) ) + +func GetNextAddress() string { + //return first value + for a := range UsedAddresses { + return a + } + return "" +} + +func AssertAddressNotInUse(address string) { + //confirm if IP is in use by other invocation + _, exists := UsedAddresses[address] + + Expect(exists).Should(BeFalse()) +} + +func TrackAddressUsage(address, containerId string) { + // set the IP as in use + // this is just for tracking in this test + UsedAddresses[address] = address + + if containerId != "" { + // set the container as in use + UsedAddresses[containerId] = address + } +} + +func RemoveAddressUsage(address, containerId string) { + delete(UsedAddresses, address) + if containerId != "" { + delete(UsedAddresses, arg.ContainerID) + arg.ContainerID = "" + } +} + +func AssertProperAddressSpace(address net.IPNet) { + //validate the IP is part of this network IP space + Expect(network.Contains(address.IP)).Should(Equal(true)) + Expect(address.Mask).Should(Equal(network.Mask)) +} diff --git a/cni/network/network.go b/cni/network/network.go index e576b004bd..dbf2691dd2 100644 --- a/cni/network/network.go +++ b/cni/network/network.go @@ -837,6 +837,13 @@ func (plugin *netPlugin) Delete(args *cniSkel.CmdArgs) error { // Query the network. if nwInfo, err = plugin.nm.GetNetworkInfo(networkId); err != nil { + + // attempt to release address associated with this Endpoint id + // This is to ensure clean up is done even in failure cases + if err = plugin.DelegateDel(nwCfg.Ipam.Type, nwCfg); err != nil { + log.Printf("Network not found, attempted to release address with error: %v", err) + } + // Log the error but return success if the endpoint being deleted is not found. plugin.Errorf("[cni-net] Failed to query network: %v", err) err = nil @@ -845,6 +852,13 @@ func (plugin *netPlugin) Delete(args *cniSkel.CmdArgs) error { // Query the endpoint. if epInfo, err = plugin.nm.GetEndpointInfo(networkId, endpointId); err != nil { + + // attempt to release address associated with this Endpoint id + // This is to ensure clean up is done even in failure cases + if err = plugin.DelegateDel(nwCfg.Ipam.Type, nwCfg); err != nil { + log.Printf("Endpoint not found, attempted to release address with error: %v", err) + } + // Log the error but return success if the endpoint being deleted is not found. plugin.Errorf("[cni-net] Failed to query endpoint: %v", err) err = nil diff --git a/cnm/ipam/ipam_test.go b/cnm/ipam/ipam_test.go index c83231fe5b..018296cc44 100644 --- a/cnm/ipam/ipam_test.go +++ b/cnm/ipam/ipam_test.go @@ -234,19 +234,18 @@ func TestRequestAddress(t *testing.T) { address1 = address.String() } -// Tests IpamDriver.ReleaseAddress functionality. -func TestReleaseAddress(t *testing.T) { +// Tests IpamDriver.GetPoolInfo functionality. +func TestGetPoolInfo(t *testing.T) { var body bytes.Buffer - var resp ReleaseAddressResponse + var resp GetPoolInfoResponse - payload := &ReleaseAddressRequest{ - PoolID: poolId1, - Address: address1, + payload := &GetPoolInfoRequest{ + PoolID: poolId1, } json.NewEncoder(&body).Encode(payload) - req, err := http.NewRequest(http.MethodGet, ReleaseAddressPath, &body) + req, err := http.NewRequest(http.MethodGet, GetPoolInfoPath, &body) if err != nil { t.Fatal(err) } @@ -257,22 +256,23 @@ func TestReleaseAddress(t *testing.T) { err = decodeResponse(w, &resp) if err != nil || resp.Err != "" { - t.Errorf("ReleaseAddress response is invalid %+v", resp) + t.Errorf("GetPoolInfo response is invalid %+v", resp) } } -// Tests IpamDriver.ReleasePool functionality. -func TestReleasePool(t *testing.T) { +// Tests IpamDriver.ReleaseAddress functionality. +func TestReleaseAddress(t *testing.T) { var body bytes.Buffer - var resp ReleasePoolResponse + var resp ReleaseAddressResponse - payload := &ReleasePoolRequest{ - PoolID: poolId1, + payload := &ReleaseAddressRequest{ + PoolID: poolId1, + Address: address1, } json.NewEncoder(&body).Encode(payload) - req, err := http.NewRequest(http.MethodGet, ReleasePoolPath, &body) + req, err := http.NewRequest(http.MethodGet, ReleaseAddressPath, &body) if err != nil { t.Fatal(err) } @@ -283,22 +283,22 @@ func TestReleasePool(t *testing.T) { err = decodeResponse(w, &resp) if err != nil || resp.Err != "" { - t.Errorf("ReleasePool response is invalid %+v", resp) + t.Errorf("ReleaseAddress response is invalid %+v", resp) } } -// Tests IpamDriver.GetPoolInfo functionality. -func TestGetPoolInfo(t *testing.T) { +// Tests IpamDriver.ReleasePool functionality. +func TestReleasePool(t *testing.T) { var body bytes.Buffer - var resp GetPoolInfoResponse + var resp ReleasePoolResponse - payload := &GetPoolInfoRequest{ + payload := &ReleasePoolRequest{ PoolID: poolId1, } json.NewEncoder(&body).Encode(payload) - req, err := http.NewRequest(http.MethodGet, GetPoolInfoPath, &body) + req, err := http.NewRequest(http.MethodGet, ReleasePoolPath, &body) if err != nil { t.Fatal(err) } @@ -309,7 +309,7 @@ func TestGetPoolInfo(t *testing.T) { err = decodeResponse(w, &resp) if err != nil || resp.Err != "" { - t.Errorf("GetPoolInfo response is invalid %+v", resp) + t.Errorf("ReleasePool response is invalid %+v", resp) } } diff --git a/ipam/azure_test.go b/ipam/azure_test.go index b754e23eac..0553dedca7 100644 --- a/ipam/azure_test.go +++ b/ipam/azure_test.go @@ -4,11 +4,11 @@ package ipam import ( + "encoding/json" "net/http" "net/url" "testing" "time" - "encoding/json" cniTypesCurr "github.com/containernetworking/cni/pkg/types/current" . "github.com/onsi/ginkgo" @@ -54,8 +54,8 @@ var ( var ( testAgent *common.Listener - source *azureSource - err error + source *azureSource + err error ) BeforeSuite(func() { @@ -111,11 +111,11 @@ var ( options := make(map[string]interface{}) options[common.OptEnvironment] = common.OptEnvironmentAzure options[common.OptAPIServerURL] = "null" - options[common.OptIpamQueryUrl] = "http://"+ipamQueryUrl + options[common.OptIpamQueryUrl] = "http://" + ipamQueryUrl source, err = newAzureSource(options) Expect(err).ShouldNot(HaveOccurred()) Expect(source.name).Should(Equal("Azure")) - Expect(source.queryUrl).Should(Equal("http://"+ipamQueryUrl)) + Expect(source.queryUrl).Should(Equal("http://" + ipamQueryUrl)) Expect(source.queryInterval).Should(Equal(azureQueryInterval)) }) }) @@ -139,6 +139,12 @@ var ( err = source.start(sink) Expect(err).NotTo(HaveOccurred()) Expect(source.sink).NotTo(BeNil()) + // this is to avoid a race condition that fails this test + //source.queryInterval defaults to 1 nanosecond + // if this test moves fast enough, it will have the refresh method + // return on this check if time.Since(s.lastRefresh) < s.queryInterval + + source.lastRefresh = time.Now().Add(-1 * time.Second) err = source.refresh() Expect(err).To(HaveOccurred()) }) @@ -153,6 +159,14 @@ var ( err = source.start(sink) Expect(err).NotTo(HaveOccurred()) Expect(source.sink).NotTo(BeNil()) + + // this is to avoid a race condition that fails this test + //source.queryInterval defaults to 1 nanosecond + // if this test moves fast enough, it will have the refresh method + // return on this check if time.Since(s.lastRefresh) < s.queryInterval + + source.lastRefresh = time.Now().Add(-1 * time.Second) + err = source.refresh() Expect(err).To(HaveOccurred()) }) @@ -163,7 +177,7 @@ var ( options := make(map[string]interface{}) options[common.OptEnvironment] = common.OptEnvironmentAzure options[common.OptAPIServerURL] = "null" - options[common.OptIpamQueryUrl] = "http://"+ipamQueryUrl + options[common.OptIpamQueryUrl] = "http://" + ipamQueryUrl am, err := createAddressManager(options) Expect(err).ToNot(HaveOccurred()) diff --git a/ipam/manager.go b/ipam/manager.go index 06c697699d..af99fbb242 100644 --- a/ipam/manager.go +++ b/ipam/manager.go @@ -164,12 +164,14 @@ func (am *addressManager) restore() error { func (am *addressManager) save() error { // Skip if a store is not provided. if am.store == nil { + log.Printf("[ipam] ipam store is nil.\n") return nil } // Update time stamp. am.TimeStamp = time.Now() + log.Printf("[ipam] saving ipam state.\n") err := am.store.Write(storeKey, am) if err == nil { log.Printf("[ipam] Save succeeded.\n") diff --git a/ipam/manager_test.go b/ipam/manager_test.go index 30e0e05018..0c1f0ec251 100644 --- a/ipam/manager_test.go +++ b/ipam/manager_test.go @@ -102,6 +102,9 @@ func setupTestAddressSpace(am AddressManager) error { ap, err := localAs.newAddressPool(anyInterface, anyPriority, &subnet1) ap.newAddressRecord(&addr11) ap.newAddressRecord(&addr12) + ap.newAddressRecord(&addr13) + ap.newAddressRecord(&addr22) + ap.newAddressRecord(&addr32) // Add subnet2 with addr21. ap, err = localAs.newAddressPool(anyInterface, anyPriority, &subnet2) @@ -233,17 +236,17 @@ var ( ModificationTime: timeReboot.Add(time.Hour), } ap := &addressPool{ - Id: "ap-test", - RefCount: 1, - Addresses: make(map[string]*addressRecord), + Id: "ap-test", + RefCount: 1, + Addresses: make(map[string]*addressRecord), } ap.Addresses["ar-test"] = &addressRecord{ - ID: "ar-test", - InUse: true, + ID: "ar-test", + InUse: true, } as := &addressSpace{ - Id: "as-test", - Pools: make(map[string]*addressPool), + Id: "as-test", + Pools: make(map[string]*addressPool), } as.Pools["ap-test"] = ap am.AddrSpaces["as-test"] = as @@ -267,17 +270,17 @@ var ( GetModificationTimeError: errors.New("Error"), } ap := &addressPool{ - Id: "ap-test", - RefCount: 1, - Addresses: make(map[string]*addressRecord), + Id: "ap-test", + RefCount: 1, + Addresses: make(map[string]*addressRecord), } ap.Addresses["ar-test"] = &addressRecord{ - ID: "ar-test", - InUse: true, + ID: "ar-test", + InUse: true, } as := &addressSpace{ - Id: "as-test", - Pools: make(map[string]*addressPool), + Id: "as-test", + Pools: make(map[string]*addressPool), } as.Pools["ap-test"] = ap am.AddrSpaces["as-test"] = as @@ -299,17 +302,17 @@ var ( } am.store = &testutils.KeyValueStoreMock{} ap := &addressPool{ - Id: "ap-test", - RefCount: 1, - Addresses: make(map[string]*addressRecord), + Id: "ap-test", + RefCount: 1, + Addresses: make(map[string]*addressRecord), } ap.Addresses["ar-test"] = &addressRecord{ - ID: "ar-test", - InUse: true, + ID: "ar-test", + InUse: true, } as := &addressSpace{ - Id: "as-test", - Pools: make(map[string]*addressPool), + Id: "as-test", + Pools: make(map[string]*addressPool), } as.Pools["ap-test"] = ap am.AddrSpaces["as-test"] = as @@ -409,8 +412,8 @@ var ( am := &addressManager{ AddrSpaces: make(map[string]*addressSpace), } - am.AddrSpaces[LocalDefaultAddressSpaceId] = &addressSpace{Id:"localId"} - am.AddrSpaces[GlobalDefaultAddressSpaceId] = &addressSpace{Id:"globalId"} + am.AddrSpaces[LocalDefaultAddressSpaceId] = &addressSpace{Id: "localId"} + am.AddrSpaces[GlobalDefaultAddressSpaceId] = &addressSpace{Id: "globalId"} localId, globalId := am.GetDefaultAddressSpaces() Expect(localId).To(Equal("localId")) Expect(globalId).To(Equal("globalId")) diff --git a/ipam/pool.go b/ipam/pool.go index f7ebbaff01..45e664a585 100644 --- a/ipam/pool.go +++ b/ipam/pool.go @@ -159,6 +159,9 @@ func (am *addressManager) setAddressSpace(as *addressSpace) error { if !ok { am.AddrSpaces[as.Id] = as } else { + // merges the address set refreshed from the source + // and the ones we have currently in this address space + log.Printf("[ipam] merging address space") as1.merge(as) } @@ -177,6 +180,9 @@ func (am *addressManager) setAddressSpace(as *addressSpace) error { // Merges a new address space to an existing one. func (as *addressSpace) merge(newas *addressSpace) { // The new epoch after the merge. + // epoch is essentially the count of invocations + // used to ensure if certain addresses refreshed from the source + // are still relevant as.epoch++ // Add new pools and addresses. @@ -308,6 +314,12 @@ func (as *addressSpace) requestPool(poolId string, subPoolId string, options map // Skip if pool is already in use. if pool.isInUse() { log.Printf("[ipam] Pool is in use.") + + // in case the pool is actually not in use, + // attempt to release it + if !pool.IsAnyRecordInUse() { + as.releasePool(poolId) + } continue } @@ -360,16 +372,16 @@ func (as *addressSpace) requestPool(poolId string, subPoolId string, options map // Releases a previously requested address pool back to its address space. func (as *addressSpace) releasePool(poolId string) error { var err error + var addressesInUse bool - log.Printf("[ipam] Releasing pool with poolId:%v.", poolId) + //merges the address set refreshed from the source log.Printf("[ipam] Attempting to release pool with poolId:%v.", poolId) ap, ok := as.Pools[poolId] if !ok { err = errAddressPoolNotFound - } else { - if !ap.isInUse() { - err = errAddressPoolNotInUse - } + } else if addressesInUse = ap.IsAnyRecordInUse(); addressesInUse { + log.Printf("[ipam] Skip releasing pool with poolId:%s. due to address being in use", + poolId) } if err != nil { @@ -377,12 +389,14 @@ func (as *addressSpace) releasePool(poolId string) error { return err } - ap.RefCount-- + if !addressesInUse { + ap.RefCount-- - // Delete address pool if it is no longer available. - if ap.epoch < as.epoch && !ap.isInUse() { - log.Printf("[ipam] Deleting stale pool with poolId:%v.", poolId) - delete(as.Pools, poolId) + // Delete address pool if it is no longer available. + if !ap.isInUse() { + log.Printf("[ipam] Deleting stale pool with poolId:%s.", poolId) + delete(as.Pools, poolId) + } } return nil @@ -423,6 +437,16 @@ func (ap *addressPool) isInUse() bool { return ap.RefCount > 0 } +// Returns if any address in the pool is currently in use. +func (ap *addressPool) IsAnyRecordInUse() bool { + for _, address := range ap.Addresses { + if address.InUse || address.ID != "" { + return true + } + } + return false +} + // Creates a new addressRecord object. func (ap *addressPool) newAddressRecord(addr *net.IP) (*addressRecord, error) { id := addr.String() @@ -498,10 +522,10 @@ func (ap *addressPool) requestAddress(address string, options map[string]string) if id != "" { ap.addrsByID[id] = ar ar.ID = id - } else { - ar.InUse = true } + ar.InUse = true + // Return address in CIDR notation. addr = &net.IPNet{ IP: ar.Addr, diff --git a/ipam/pool_test.go b/ipam/pool_test.go index 99eb1e1018..99b89f8c38 100644 --- a/ipam/pool_test.go +++ b/ipam/pool_test.go @@ -1,6 +1,7 @@ package ipam import ( + "github.com/google/uuid" "net" "testing" @@ -654,7 +655,7 @@ var ( Addresses: map[string]*addressRecord{}, } as.Pools["10.1.0.0/16"] = &addressPool{ - Id: "10.1.0.0/16", + Id: "10.1.0.0/16", Addresses: map[string]*addressRecord{ "10.1.0.1/16": &addressRecord{}, }, @@ -678,39 +679,39 @@ var ( }) }) - Context("When pool is not in use", func() { - It("Should raise an error", func() { + Context("When pool's addresses are all not in use", func() { + It("Should release the pool ", func() { poolId := "10.0.0.0/16" as := &addressSpace{ + epoch: 2, Pools: map[string]*addressPool{}, } as.Pools[poolId] = &addressPool{ - RefCount: 0, + epoch: 1, + RefCount: 1, + Addresses: map[string]*addressRecord{}, } - err := as.releasePool("10.0.0.0/16") - Expect(err).To(Equal(errAddressPoolNotInUse)) - Expect(as.Pools[poolId]).NotTo(BeNil()) - }) - }) - Context("When pool's epoch is less than the space's epoch and pool is never in use", func() { - It("Should release the pool ", func() { - poolId := "10.0.0.0/16" - as := &addressSpace{ - epoch: 2, - Pools: map[string]*addressPool{}, + as.Pools[poolId].Addresses["10.0.0.1"] = &addressRecord{ + InUse: false, + Addr: net.IPv4zero, } - as.Pools[poolId] = &addressPool{ - epoch: 1, - RefCount: 1, + as.Pools[poolId].Addresses["10.0.0.2"] = &addressRecord{ + InUse: false, + Addr: net.IPv4zero, } + as.Pools[poolId].Addresses["10.0.0.3"] = &addressRecord{ + InUse: false, + Addr: net.IPv4zero, + } + err := as.releasePool("10.0.0.0/16") Expect(err).NotTo(HaveOccurred()) Expect(as.Pools[poolId]).To(BeNil()) }) }) - Context("When the epoch of pool is equal to the epoch of addressSpace", func() { + Context("When the pool has in use addresses", func() { It("Should not delete the pool", func() { poolId := "10.0.0.0/16" as := &addressSpace{ @@ -718,9 +719,24 @@ var ( Pools: map[string]*addressPool{}, } as.Pools[poolId] = &addressPool{ - epoch: 1, - RefCount: 1, + epoch: 1, + RefCount: 1, + Addresses: map[string]*addressRecord{}, + } + + as.Pools[poolId].Addresses["10.0.0.1"] = &addressRecord{ + InUse: true, + Addr: net.IPv4zero, } + as.Pools[poolId].Addresses["10.0.0.2"] = &addressRecord{ + InUse: false, + Addr: net.IPv4zero, + } + as.Pools[poolId].Addresses["10.0.0.3"] = &addressRecord{ + InUse: false, + Addr: net.IPv4zero, + } + err := as.releasePool("10.0.0.0/16") Expect(err).NotTo(HaveOccurred()) Expect(as.Pools[poolId]).NotTo(BeNil()) @@ -829,6 +845,43 @@ var ( }) }) + Context("When id is not found and a address is available", func() { + It("Should return a new address", func() { + ap := &addressPool{ + Addresses: map[string]*addressRecord{}, + addrsByID: map[string]*addressRecord{}, + Subnet: subnet1, + } + arId := uuid.New().String() + + ap.Addresses["0"] = &addressRecord{ + ID: "", + Addr: addr11, + InUse: false, + } + + ap.Addresses["1"] = &addressRecord{ + ID: "", + Addr: addr12, + InUse: false, + } + + ap.Addresses["3"] = &addressRecord{ + ID: "", + Addr: addr13, + InUse: false, + } + + options := map[string]string{} + options[OptAddressID] = arId + + addr, err := ap.requestAddress("", options) + Expect(err).NotTo(HaveOccurred()) + Expect(addr).NotTo(BeEmpty()) + Expect(ap.addrsByID[arId].ID).To(Equal(arId)) + }) + }) + Context("When addressRecord is in use and id is empty", func() { It("Should raise errAddressInUse", func() { ap := &addressPool{