Make IPAM idempotent

This makes IPAM functionality idempotent, so that is plays nicely with the fact that the vino controller will frequently be reconciling the input CRs that trigger IP assignment. With this change, IPAM will remember who asked for an IP already, and will give the same IP back if asked again. Change-Id: I7c6b99c2e087178a04d13bafe87909279994e26b
2021-03-02 18:28:07 -06:00 · 2021-03-02 18:28:07 -06:00 · e4d8d7d23b
parent 3c070e66b1
commit e4d8d7d23b
7 changed files with 184 additions and 57 deletions
--- a/config/crd/bases/airship.airshipit.org_ippools.yaml
+++ b/config/crd/bases/airship.airshipit.org_ippools.yaml
@ -32,14 +32,23 @@ spec:
        metadata:
          type: object
        spec:
-          description: IPPool tracks allocation ranges and statuses within a specific
+          description: IPPoolSpec tracks allocation ranges and statuses within a specific
            subnet IPv4 or IPv6 subnet.  It has a set of ranges of IPs within the
            subnet from which IPs can be allocated by IPAM, and a set of IPs that
            are currently allocated already.
          properties:
            allocatedIPs:
              items:
-                type: string
+                description: AllocatedIP Allocates an IP to an entity
+                properties:
+                  allocatedTo:
+                    type: string
+                  ip:
+                    type: string
+                required:
+                - allocatedTo
+                - ip
+                type: object
              type: array
            ranges:
              items:
--- a/config/samples/ippool.yaml
+++ b/config/samples/ippool.yaml
@ -10,6 +10,9 @@ spec:
    - start: 10.0.1.1
      stop:  10.0.1.9
  allocatedIPs:
-    - 10.0.0.1
-    - 10.0.0.2
-    - 10.0.1.1
+    - allocatedTo: default-vino-test-cr-leviathan-worker-0
+      ip: 10.0.0.1
+    - allocatedTo: default-vino-test-cr-leviathan-worker-1
+      ip: 10.0.0.2
+    - allocatedTo: default-vino-test-cr-leviathan-worker-2
+      ip: 10.0.1.1
--- a/docs/api/vino.md
+++ b/docs/api/vino.md
@ -9,6 +9,47 @@
 <p>Package v1 contains API Schema definitions for the airship v1 API group</p>
 Resource Types:
 <ul class="simple"></ul>
+<h3 id="airship.airshipit.org/v1.AllocatedIP">AllocatedIP
+</h3>
+<p>
+(<em>Appears on:</em>
+<a href="#airship.airshipit.org/v1.IPPoolSpec">IPPoolSpec</a>)
+</p>
+<p>AllocatedIP Allocates an IP to an entity</p>
+<div class="md-typeset__scrollwrap">
+<div class="md-typeset__table">
+<table>
+<thead>
+<tr>
+<th>Field</th>
+<th>Description</th>
+</tr>
+</thead>
+<tbody>
+<tr>
+<td>
+<code>ip</code><br>
+<em>
+string
+</em>
+</td>
+<td>
+</td>
+</tr>
+<tr>
+<td>
+<code>allocatedTo</code><br>
+<em>
+string
+</em>
+</td>
+<td>
+</td>
+</tr>
+</tbody>
+</table>
+</div>
+</div>
 <h3 id="airship.airshipit.org/v1.BMCCredentials">BMCCredentials
 </h3>
 <p>
@ -326,7 +367,9 @@ string
 <td>
 <code>allocatedIPs</code><br>
 <em>
-[]string
+<a href="#airship.airshipit.org/v1.AllocatedIP">
+[]AllocatedIP
+</a>
 </em>
 </td>
 <td>
@ -357,7 +400,7 @@ IPPoolStatus
 (<em>Appears on:</em>
 <a href="#airship.airshipit.org/v1.IPPool">IPPool</a>)
 </p>
-<p>IPPool tracks allocation ranges and statuses within a specific
+<p>IPPoolSpec tracks allocation ranges and statuses within a specific
 subnet IPv4 or IPv6 subnet.  It has a set of ranges of IPs
 within the subnet from which IPs can be allocated by IPAM,
 and a set of IPs that are currently allocated already.</p>
@ -397,7 +440,9 @@ string
 <td>
 <code>allocatedIPs</code><br>
 <em>
-[]string
+<a href="#airship.airshipit.org/v1.AllocatedIP">
+[]AllocatedIP
+</a>
 </em>
 </td>
 <td>
--- a/pkg/api/v1/ippool_types.go
+++ b/pkg/api/v1/ippool_types.go
@ -21,14 +21,20 @@ import (
 // EDIT THIS FILE!  THIS IS SCAFFOLDING FOR YOU TO OWN!
 // NOTE: json tags are required.  Any new fields you add must have json tags for the fields to be serialized.

-// IPPool tracks allocation ranges and statuses within a specific
+// IPPoolSpec tracks allocation ranges and statuses within a specific
 // subnet IPv4 or IPv6 subnet.  It has a set of ranges of IPs
 // within the subnet from which IPs can be allocated by IPAM,
 // and a set of IPs that are currently allocated already.
 type IPPoolSpec struct {
-	Subnet       string   `json:"subnet"`
-	Ranges       []Range  `json:"ranges"`
-	AllocatedIPs []string `json:"allocatedIPs"`
+	Subnet       string        `json:"subnet"`
+	Ranges       []Range       `json:"ranges"`
+	AllocatedIPs []AllocatedIP `json:"allocatedIPs"`
+}
+
+// AllocatedIP Allocates an IP to an entity
+type AllocatedIP struct {
+	IP          string `json:"ip"`
+	AllocatedTo string `json:"allocatedTo"`
 }

 // Range has (inclusive) bounds within a subnet from which IPs can be allocated
--- a/pkg/api/v1/zz_generated.deepcopy.go
+++ b/pkg/api/v1/zz_generated.deepcopy.go
@ -25,6 +25,21 @@ import (
 	runtime "k8s.io/apimachinery/pkg/runtime"
 )

+// DeepCopyInto is an autogenerated deepcopy function, copying the receiver, writing into out. in must be non-nil.
+func (in *AllocatedIP) DeepCopyInto(out *AllocatedIP) {
+	*out = *in
+}
+
+// DeepCopy is an autogenerated deepcopy function, copying the receiver, creating a new AllocatedIP.
+func (in *AllocatedIP) DeepCopy() *AllocatedIP {
+	if in == nil {
+		return nil
+	}
+	out := new(AllocatedIP)
+	in.DeepCopyInto(out)
+	return out
+}
+
 // DeepCopyInto is an autogenerated deepcopy function, copying the receiver, writing into out. in must be non-nil.
 func (in *BMCCredentials) DeepCopyInto(out *BMCCredentials) {
 	*out = *in
@ -175,7 +190,7 @@ func (in *IPPoolSpec) DeepCopyInto(out *IPPoolSpec) {
 	}
 	if in.AllocatedIPs != nil {
 		in, out := &in.AllocatedIPs, &out.AllocatedIPs
-		*out = make([]string, len(*in))
+		*out = make([]AllocatedIP, len(*in))
 		copy(*out, *in)
 	}
 }
--- a/pkg/ipam/ipam.go
+++ b/pkg/ipam/ipam.go
@ -65,6 +65,9 @@ func NewRange(start string, stop string) (vinov1.Range, error) {

 // AddSubnetRange adds a range within a subnet for IP allocation
 // TODO error: range overlaps with existing range or subnet overlaps with existing subnet
+// NOTE: the above should only be an error if a subnet is re-added with a *different*
+// subnet range than what is already allocated -- i.e. this function should be idempotent
+// against allocating the exact same subnet+range multiple times.
 // TODO error: invalid range for subnet
 func (i *Ipam) AddSubnetRange(ctx context.Context, subnet string, subnetRange vinov1.Range) error {
 	logger := i.Log.WithValues("subnet", subnet, "subnetRange", subnetRange)
@ -73,35 +76,43 @@ func (i *Ipam) AddSubnetRange(ctx context.Context, subnet string, subnetRange vi
 	if err != nil {
 		return err
 	}
+	// Add the IPAM subnet if it doesn't exist already
 	ippool, exists := ippools[subnet]
 	if !exists {
-		logger.Info("IPAM creating subnet and adding range")
+		logger.Info("IPAM creating subnet")
 		ippool = &vinov1.IPPoolSpec{
 			Subnet:       subnet,
 			Ranges:       []vinov1.Range{},
-			AllocatedIPs: []string{},
+			AllocatedIPs: []vinov1.AllocatedIP{},
 		}
 		ippools[subnet] = ippool
-	} else {
-		logger.Info("IPAM subnet already exists; adding range")
-		// Does the subnet's requested range already exist? (this should fail)
-		for _, r := range ippool.Ranges {
-			if r == subnetRange {
-				return ErrSubnetRangeOverlapsWithExistingRange{Subnet: subnet, SubnetRange: subnetRange}
-			}
+	}
+	// Add the IPAM range to the subnet if it doesn't exist already
+	exists = false
+	for _, existingSubnetRange := range ippools[subnet].Ranges {
+		if existingSubnetRange == subnetRange {
+			exists = true
+			break
 		}
 	}
-	ippool.Ranges = append(ippool.Ranges, subnetRange)
-	err = i.applyIPPool(ctx, *ippool)
-	if err != nil {
-		return err
+	if !exists {
+		logger.Info("IPAM creating subnet")
+		ippool.Ranges = append(ippool.Ranges, subnetRange)
+		err = i.applyIPPool(ctx, *ippool)
+		if err != nil {
+			return err
+		}
 	}

 	return nil
 }

 // AllocateIP allocates an IP from a range and return it
-func (i *Ipam) AllocateIP(ctx context.Context, subnet string, subnetRange vinov1.Range) (string, error) {
+// allocatedTo: a unique identifier for the entity that is requesting / will own the
+//              allocated IP.  If the same entity requests another IP, it will be given
+//              the same one.  I.e. this function is idempotent for the same allocatedTo.
+func (i *Ipam) AllocateIP(ctx context.Context, subnet string, subnetRange vinov1.Range,
+	allocatedTo string) (string, error) {
 	ippools, err := i.getIPPools(ctx)
 	if err != nil {
 		return "", err
@ -122,20 +133,38 @@ func (i *Ipam) AllocateIP(ctx context.Context, subnet string, subnetRange vinov1
 		return "", ErrSubnetRangeNotAllocated{Subnet: subnet, SubnetRange: subnetRange}
 	}

-	ip, err := findFreeIPInRange(ippool, subnetRange)
-	if err != nil {
-		return "", err
-	}
-	i.Log.Info("Allocating IP", "ip", ip, "subnet", subnet, "subnetRange", subnetRange)
-	ippool.AllocatedIPs = append(ippool.AllocatedIPs, ip)
-	err = i.applyIPPool(ctx, *ippool)
-	if err != nil {
-		return "", err
+	// If an IP has already been allocated to this entity, return it
+	ip := findAlreadyAllocatedIP(ippool, allocatedTo)
+
+	// No IP already allocated, so allocate a new IP
+	if ip == "" {
+		ip, err = findFreeIPInRange(ippool, subnetRange)
+		if err != nil {
+			return "", err
+		}
+		i.Log.Info("Allocating IP", "ip", ip, "subnet", subnet, "subnetRange", subnetRange)
+		ippool.AllocatedIPs = append(ippool.AllocatedIPs, vinov1.AllocatedIP{IP: ip, AllocatedTo: allocatedTo})
+		err = i.applyIPPool(ctx, *ippool)
+		if err != nil {
+			return "", err
+		}
 	}

 	return ip, nil
 }

+// This returns an IP already allocated to the entity specified by `allocatedTo`
+// if it exists within the requested ippool/subnet, and a blank string
+// if no IP is already allocated.
+func findAlreadyAllocatedIP(ippool *vinov1.IPPoolSpec, allocatedTo string) string {
+	for _, allocatedIP := range ippool.AllocatedIPs {
+		if allocatedIP.AllocatedTo == allocatedTo {
+			return allocatedIP.IP
+		}
+	}
+	return ""
+}
+
 // This converts IP ranges/addresses into iterable ints,
 // steps through them looking for one that that is not already
 // in use, converts it back to a string and returns it.
@ -170,12 +199,12 @@ func findFreeIPInRange(ippool *vinov1.IPPoolSpec, subnetRange vinov1.Range) (str
 	return "", ErrSubnetRangeExhausted{ippool.Subnet, subnetRange}
 }

-// Create a map[uint64]struct{} representation of a string slice,
+// Create a map[uint64]struct{} representation of an AllocatedIP slice,
 // for efficient set lookups
-func sliceToMap(slice []string) (map[uint64]struct{}, error) {
+func sliceToMap(slice []vinov1.AllocatedIP) (map[uint64]struct{}, error) {
 	m := map[uint64]struct{}{}
 	for _, s := range slice {
-		i, err := ipStringToInt(s)
+		i, err := ipStringToInt(s.IP)
 		if err != nil {
 			return m, err
 		}
--- a/pkg/ipam/ipam_test.go
+++ b/pkg/ipam/ipam_test.go
@ -59,7 +59,9 @@ func SetUpMockClient(ctx context.Context, ctrl *gomock.Controller) *test.MockCli
 					Ranges: []vinov1.Range{
 						{Start: "192.168.0.0", Stop: "192.168.0.0"},
 					},
-					AllocatedIPs: []string{"192.168.0.0"},
+					AllocatedIPs: []vinov1.AllocatedIP{
+						{IP: "192.168.0.0", AllocatedTo: "old-vm-name"},
+					},
 				},
 			},
 			{
@ -68,7 +70,9 @@ func SetUpMockClient(ctx context.Context, ctrl *gomock.Controller) *test.MockCli
 					Ranges: []vinov1.Range{
 						{Start: "2600:1700:b031:0000::", Stop: "2600:1700:b031:0000::"},
 					},
-					AllocatedIPs: []string{"2600:1700:b031:0000::"},
+					AllocatedIPs: []vinov1.AllocatedIP{
+						{IP: "2600:1700:b031:0000::", AllocatedTo: "old-vm-name"},
+					},
 				},
 			},
 		},
@ -83,36 +87,41 @@ func SetUpMockClient(ctx context.Context, ctrl *gomock.Controller) *test.MockCli

 func TestAllocateIP(t *testing.T) {
 	tests := []struct {
-		name, subnet, expectedErr string
-		subnetRange               vinov1.Range
+		name, subnet, allocatedTo, expectedErr string
+		subnetRange                            vinov1.Range
 	}{
 		{
 			name:        "success ipv4",
 			subnet:      "10.0.0.0/16",
 			subnetRange: vinov1.Range{Start: "10.0.1.0", Stop: "10.0.1.9"},
+			allocatedTo: "new-vm-name",
 		},
 		{
 			name:        "success ipv6",
 			subnet:      "2600:1700:b030:0000::/72",
 			subnetRange: vinov1.Range{Start: "2600:1700:b030:0000::", Stop: "2600:1700:b030:0009::"},
+			allocatedTo: "new-vm-name",
 		},
 		{
 			name:        "error subnet not allocated ipv4",
 			subnet:      "10.0.0.0/20",
 			subnetRange: vinov1.Range{Start: "10.0.1.0", Stop: "10.0.1.9"},
 			expectedErr: "IPAM subnet 10.0.0.0/20 not allocated",
+			allocatedTo: "new-vm-name",
 		},
 		{
 			name:        "error subnet not allocated ipv6",
 			subnet:      "2600:1700:b030:0000::/80",
 			subnetRange: vinov1.Range{Start: "2600:1700:b030:0000::", Stop: "2600:1700:b030:0009::"},
 			expectedErr: "IPAM subnet 2600:1700:b030:0000::/80 not allocated",
+			allocatedTo: "new-vm-name",
 		},
 		{
 			name:        "error range not allocated ipv4",
 			subnet:      "10.0.0.0/16",
 			subnetRange: vinov1.Range{Start: "10.0.2.0", Stop: "10.0.2.9"},
 			expectedErr: "IPAM range [10.0.2.0,10.0.2.9] in subnet 10.0.0.0/16 is not allocated",
+			allocatedTo: "new-vm-name",
 		},
 		{
 			name:        "error range not allocated ipv6",
@ -120,12 +129,20 @@ func TestAllocateIP(t *testing.T) {
 			subnetRange: vinov1.Range{Start: "2600:1700:b030:0000::", Stop: "2600:1700:b030:1111::"},
 			expectedErr: "IPAM range [2600:1700:b030:0000::,2600:1700:b030:1111::] " +
 				"in subnet 2600:1700:b030:0000::/72 is not allocated",
+			allocatedTo: "new-vm-name",
+		},
+		{
+			name:        "success idempotency ipv4",
+			subnet:      "192.168.0.0/1",
+			subnetRange: vinov1.Range{Start: "192.168.0.0", Stop: "192.168.0.0"},
+			allocatedTo: "old-vm-name",
 		},
 		{
 			name:        "error range exhausted ipv4",
 			subnet:      "192.168.0.0/1",
 			subnetRange: vinov1.Range{Start: "192.168.0.0", Stop: "192.168.0.0"},
 			expectedErr: "IPAM range [192.168.0.0,192.168.0.0] in subnet 192.168.0.0/1 is exhausted",
+			allocatedTo: "new-vm-name",
 		},
 		{
 			name:        "error range exhausted ipv6",
@ -133,6 +150,7 @@ func TestAllocateIP(t *testing.T) {
 			subnetRange: vinov1.Range{Start: "2600:1700:b031:0000::", Stop: "2600:1700:b031:0000::"},
 			expectedErr: "IPAM range [2600:1700:b031:0000::,2600:1700:b031:0000::] " +
 				"in subnet 2600:1700:b031:0000::/64 is exhausted",
+			allocatedTo: "new-vm-name",
 		},
 	}

@ -147,7 +165,7 @@ func TestAllocateIP(t *testing.T) {
 			ipammer := NewIpam(log.Log, m, "vino-system")
 			ipammer.Log = log.Log

-			ip, err := ipammer.AllocateIP(ctx, tt.subnet, tt.subnetRange)
+			ip, err := ipammer.AllocateIP(ctx, tt.subnet, tt.subnetRange, tt.allocatedTo)
 			if tt.expectedErr != "" {
 				require.Error(t, err)
 				assert.Equal(t, "", ip)
@ -217,12 +235,6 @@ func TestAddSubnetRange(t *testing.T) {
 			subnetRange: vinov1.Range{Start: "10.0.2.0", Stop: "10.0.2.9"},
 			expectedErr: "",
 		},
-		{
-			name:        "error range already exists",
-			subnet:      "10.0.0.0/16",
-			subnetRange: vinov1.Range{Start: "10.0.1.0", Stop: "10.0.1.9"},
-			expectedErr: "IPAM range [10.0.1.0,10.0.1.9] in subnet 10.0.0.0/16 overlaps",
-		},
 		// TODO: check for partially overlapping ranges and subnets
 	}

@ -294,7 +306,10 @@ func TestFindFreeIPInRange(t *testing.T) {
 					{Start: "2600:1700:b030:1001::", Stop: "2600:1700:b030:1009::"},
 					{Start: "2600:1700:b031::", Stop: "2600:1700:b031::"},
 				},
-				AllocatedIPs: []string{"10.0.2.0", "2600:1700:b031::"},
+				AllocatedIPs: []vinov1.AllocatedIP{
+					{IP: "10.0.2.0", AllocatedTo: "old-vm-name"},
+					{IP: "2600:1700:b031::", AllocatedTo: "old-vm-name"},
+				},
 			}
 			actual, err := findFreeIPInRange(&ippool, tt.subnetRange)
 			if tt.expectedErr != "" {
@ -311,23 +326,28 @@ func TestFindFreeIPInRange(t *testing.T) {
 func TestSliceToMap(t *testing.T) {
 	tests := []struct {
 		name string
-		in   []string
+		in   []vinov1.AllocatedIP
 		out  map[uint64]struct{}
 	}{
 		{
 			name: "empty slice",
-			in:   []string{},
+			in:   []vinov1.AllocatedIP{},
 			out:  map[uint64]struct{}{},
 		},
 		{
 			name: "one-element slice",
-			in:   []string{"0.0.0.1"},
-			out:  map[uint64]struct{}{1: {}},
+			in: []vinov1.AllocatedIP{
+				{IP: "0.0.0.1", AllocatedTo: "old-vm-name"},
+			},
+			out: map[uint64]struct{}{1: {}},
 		},
 		{
 			name: "two-element slice",
-			in:   []string{"0.0.0.1", "0.0.0.2"},
-			out:  map[uint64]struct{}{1: {}, 2: {}},
+			in: []vinov1.AllocatedIP{
+				{IP: "0.0.0.1", AllocatedTo: "old-vm-name"},
+				{IP: "0.0.0.2", AllocatedTo: "old-vm-name"},
+			},
+			out: map[uint64]struct{}{1: {}, 2: {}},
 		},
 	}