Repository: metallb/metallb
Pull request #2097 (2023-09-29 13:49:41)

Remove IP verifications in SetPools
This is https://github.com/metallb/metallb/pull/1948 plus the fixes suggested in the last phase of the PR.

Pool changes might invalidate the LB IPs of services. When this happens, the pool controller ignores the new configuration and continues operating with the current one. This PR modifies this behavior by always accepting the new configuration and letting the service controller to clear the IPs of the affected services.

Fixes https://github.com/metallb/metallb/issues/462


fedepaol: cc @oribon 

Patch:
diff --git a/controller/main.go b/controller/main.go
index abbf9441..2f1ff2be 100644
--- a/controller/main.go
+++ b/controller/main.go
@@ -74,22 +74,29 @@ func (c *controller) SetBalancer(l log.Logger, name string, svcRo *v1.Service, _
 	// a reason.
 	svc := svcRo.DeepCopy()
 	syncStateRes := controllers.SyncStateSuccess
-	wasAllocated := c.isServiceAllocated(name)
+
+	prevIPs := c.ips.IPs(name)
+
 	if c.convergeBalancer(l, name, svc) != nil {
 		syncStateRes = controllers.SyncStateErrorNoRetry
 	}
 
-	if wasAllocated && !c.isServiceAllocated(name) { // convergeBalancer may deallocate our service and this means it did it.
-		// if the service was deallocated, it may have have left room
-		// for another one, so we reprocess
-		level.Info(l).Log("event", "serviceUpdated", "msg", "removed loadbalancer from service, services will be reprocessed")
-		syncStateRes = controllers.SyncStateReprocessAll
-	}
 	if reflect.DeepEqual(svcRo, svc) {
 		level.Debug(l).Log("event", "noChange", "msg", "service converged, no change")
 		return syncStateRes
 	}
 
+	if len(prevIPs) != 0 && !c.isServiceAllocated(name) {
+		// Only reprocess all if the previous IP(s) are still contained within a pool.
+		if c.ips.PoolForIP(prevIPs) != nil {
+			// convergeBalancer may deallocate our service and this means it did it.
+			// if the service was deallocated, it may have left room
+			// for another one, so we reprocess
+			level.Info(l).Log("event", "serviceUpdated", "msg", "removed loadbalancer from service, services will be reprocessed")
+			syncStateRes = controllers.SyncStateReprocessAll
+		}
+	}
+
 	toWrite := svcRo.DeepCopy()
 	if !reflect.DeepEqual(svcRo.Status, svc.Status) {
 		toWrite.Status = svc.Status
@@ -121,11 +128,9 @@ func (c *controller) SetPools(l log.Logger, pools *config.Pools) controllers.Syn
 		return controllers.SyncStateErrorNoRetry
 	}
 
-	if err := c.ips.SetPools(pools); err != nil {
-		level.Error(l).Log("op", "setConfig", "error", err, "msg", "applying new configuration failed")
-		return controllers.SyncStateError
-	}
+	c.ips.SetPools(pools)
 	c.pools = pools
+
 	return controllers.SyncStateReprocessAll
 }
 
diff --git a/controller/service.go b/controller/service.go
index dd02632d..c22a4c88 100644
--- a/controller/service.go
+++ b/controller/service.go
@@ -51,6 +51,13 @@ func (c *controller) convergeBalancer(l log.Logger, key string, svc *v1.Service)
 		return nil
 	}
 
+	// Return if pools are empty.
+	if len(c.pools.ByName) == 0 {
+		level.Debug(l).Log("event", "clearAssignment", "reason", "noConfig", "msg", "pools are empty")
+		c.clearServiceState(key, svc)
+		return ErrConverge
+	}
+
 	// If the ClusterIPs is malformed or not set we can't determine the
 	// ipFamily to use.
 	if len(svc.Spec.ClusterIPs) == 0 && svc.Spec.ClusterIP == "" {
diff --git a/internal/allocator/allocator.go b/internal/allocator/allocator.go
index 167860d2..4fb3ff21 100644
--- a/internal/allocator/allocator.go
+++ b/internal/allocator/allocator.go
@@ -66,18 +66,7 @@ func New() *Allocator {
 }
 
 // SetPools updates the set of address pools that the allocator owns.
-func (a *Allocator) SetPools(pools *config.Pools) error {
-	// All the fancy sharing stuff only influences how new allocations
-	// can be created. For changing the underlying configuration, the
-	// only question we have to answer is: can we fit all allocated
-	// IPs into address pools under the new configuration?
-	for svc, alloc := range a.allocated {
-		pool := poolFor(pools.ByName, alloc.ips)
-		if pool == nil {
-			return fmt.Errorf("new config not compatible with assigned IPs: service %q cannot own %q under new config", svc, alloc.ips)
-		}
-	}
-
+func (a *Allocator) SetPools(pools *config.Pools) {
 	for n := range a.pools.ByName {
 		if pools.ByName[n] == nil {
 			stats.poolCapacity.DeleteLabelValues(n)
@@ -92,7 +81,8 @@ func (a *Allocator) SetPools(pools *config.Pools) error {
 	for svc, alloc := range a.allocated {
 		pool := poolFor(a.pools.ByName, alloc.ips)
 		if pool == nil {
-			return fmt.Errorf("can't retrieve new pool for assigned IPs: service %q cannot own %q under new config", svc, alloc.ips)
+			a.Unassign(svc)
+			continue
 		}
 		if pool.Name != alloc.pool {
 			a.Unassign(svc)
@@ -108,8 +98,6 @@ func (a *Allocator) SetPools(pools *config.Pools) error {
 		stats.poolCapacity.WithLabelValues(n).Set(float64(poolCount(p)))
 		stats.poolActive.WithLabelValues(n).Set(float64(len(a.poolIPsInUse[n])))
 	}
-
-	return nil
 }
 
 // assign unconditionally updates internal state to reflect svc's
@@ -355,6 +343,19 @@ func (a *Allocator) Pool(svc string) string {
 	return ""
 }
 
+// IPs returns the allocated IPs of a service.
+func (a *Allocator) IPs(svc string) []net.IP {
+	if alloc := a.allocated[svc]; alloc != nil {
+		return alloc.ips
+	}
+	return nil
+}
+
+// PoolForIP returns the pool structure associated with an IP.
+func (a *Allocator) PoolForIP(ips []net.IP) *config.Pool {
+	return poolFor(a.pools.ByName, ips)
+}
+
 func sortPools(pools []*config.Pool) {
 	// A lower value for pool priority equals a higher priority and sort
 	// pools from higher to low priority. when no priority (0) set on
diff --git a/website/content/installation/_index.md b/website/content/installation/_index.md
index 2f7967ff..a9857cde 100644
--- a/website/content/installation/_index.md
+++ b/website/content/installation/_index.md
@@ -188,6 +188,8 @@ and change the `BGP_TYPE` environment variable of the `manager` container to `fr
   value: frr
 ```
 
+{{% /notice %}}
+
 ## FRR daemons logging level
 
 The FRR daemons logging level are configured using the speaker `--log-level` argument following the below mapping:


Test patch:
diff --git a/controller/controller_test.go b/controller/controller_test.go
index 106be5e7..274299d4 100644
--- a/controller/controller_test.go
+++ b/controller/controller_test.go
@@ -1040,12 +1040,23 @@ func TestControllerConfig(t *testing.T) {
 		t.Errorf("SetBalancer produced unexpected mutation (-want +got)\n%s", diff)
 	}
 
-	// Now that an IP is allocated, removing the IP pool is not allowed.
-	if c.SetPools(l, &config.Pools{ByName: map[string]*config.Pool{}}) != controllers.SyncStateError {
-		t.Fatalf("SetPools that deletes allocated IPs was accepted")
+	// Removing the IP pool should be accepted even if the IP is allocated
+	// This triggers a reprocess all event to force the service to get a new IP
+	if c.SetPools(l, &config.Pools{ByName: map[string]*config.Pool{}}) != controllers.SyncStateReprocessAll {
+		t.Fatalf("SetPools that deletes allocated IPs was not accepted")
 	}
 
-	// Deleting the config also makes MetalLB sad.
+	// Balancer should clear the service. Not reprocess all because previous ip is not longer assignable.
+	if c.SetBalancer(l, "test", gotSvc, epslices.EpsOrSlices{}) != controllers.SyncStateErrorNoRetry {
+		t.Fatalf("SetBalancer failed")
+	}
+
+	gotSvc = k.gotService(gotSvc)
+	if diff := diffService(svc, gotSvc); diff != "" {
+		t.Errorf("SetBalancer produced unexpected mutation (-want +got)\n%s", diff)
+	}
+
+	// Deleting the config makes MetalLB sad.
 	if c.SetPools(l, nil) != controllers.SyncStateErrorNoRetry {
 		t.Fatalf("SetPools that deletes the config was accepted")
 	}
@@ -1097,7 +1108,7 @@ func TestDeleteRecyclesIP(t *testing.T) {
 		},
 	}
 	if c.SetBalancer(l, "test2", svc2, epslices.EpsOrSlices{}) == controllers.SyncStateError {
-		t.Fatal("SetBalancer svc2 failed")
+		t.Fatal("SetBalancer svc2 was supposed to fail with no retry")
 	}
 	if k.gotService(svc2) != nil {
 		t.Fatal("SetBalancer svc2 mutated svc2 even though it should not have allocated")
@@ -1121,6 +1132,102 @@ func TestDeleteRecyclesIP(t *testing.T) {
 		t.Fatal("svc2 didn't get an IP")
 	}
 }
+func TestControllerReassign(t *testing.T) {
+	k := &testK8S{t: t}
+	c := &controller{
+		ips:    allocator.New(),
+		client: k,
+	}
+
+	l := log.NewNopLogger()
+
+	svc := &v1.Service{
+		Spec: v1.ServiceSpec{
+			Type:       "LoadBalancer",
+			ClusterIPs: []string{"1.2.3.4"},
+		},
+	}
+
+	// Set a config with some IPs.
+	pools := &config.Pools{ByName: map[string]*config.Pool{
+		"default": {
+			Name:       "default",
+			AutoAssign: true,
+			CIDR:       []*net.IPNet{ipnet("1.2.3.0/24"), ipnet("1000::1/127")},
+		},
+	}}
+
+	if c.SetPools(l, pools) != controllers.SyncStateReprocessAll {
+		t.Fatalf("SetPools failed")
+	}
+
+	if c.SetBalancer(l, "test", svc, epslices.EpsOrSlices{}) != controllers.SyncStateSuccess {
+		t.Fatalf("SetBalancer failed")
+	}
+
+	gotSvc := k.gotService(svc)
+	wantSvc := new(v1.Service)
+	*wantSvc = *svc
+	wantSvc.Status = statusAssigned([]string{"1.2.3.0"})
+	if diff := diffService(wantSvc, gotSvc); diff != "" {
+		t.Errorf("SetBalancer produced unexpected mutation (-want +got)\n%s", diff)
+	}
+
+	// Set a config with some 2 pools.
+	pools = &config.Pools{ByName: map[string]*config.Pool{
+		"default": {
+			Name:       "default",
+			AutoAssign: true,
+			CIDR:       []*net.IPNet{ipnet("1.2.3.0/24"), ipnet("1000::1/127")},
+		},
+		"second": {
+			Name:       "second",
+			AutoAssign: true,
+			CIDR:       []*net.IPNet{ipnet("4.5.6.0/24")},
+		},
+	}}
+
+	if c.SetPools(l, pools) == controllers.SyncStateError {
+		t.Fatalf("SetPools failed")
+	}
+
+	// Allocation shouldn't change.
+	if c.SetBalancer(l, "test", gotSvc, epslices.EpsOrSlices{}) != controllers.SyncStateSuccess {
+		t.Fatalf("SetBalancer failed")
+	}
+
+	k.reset()
+
+	if k.gotService(gotSvc) != nil {
+		t.Errorf("unsynced SetBalancer mutated service%s", gotSvc)
+	}
+
+	// Delete default pool.
+	pools = &config.Pools{ByName: map[string]*config.Pool{
+		"second": {
+			Name:       "second",
+			AutoAssign: true,
+			CIDR:       []*net.IPNet{ipnet("4.5.6.0/24")},
+		},
+	}}
+
+	if c.SetPools(l, pools) != controllers.SyncStateReprocessAll {
+		t.Fatalf("SetPools failed")
+	}
+
+	// It should get an IP from the second pool
+	if c.SetBalancer(l, "test", gotSvc, epslices.EpsOrSlices{}) != controllers.SyncStateSuccess {
+		t.Fatalf("SetBalancer failed")
+	}
+
+	gotSvc = k.gotService(svc)
+	*wantSvc = *svc
+	wantSvc.Status = statusAssigned([]string{"4.5.6.0"})
+	if diff := diffService(wantSvc, gotSvc); diff != "" {
+		t.Errorf("SetBalancer produced unexpected mutation (-want +got)\n%s", diff)
+	}
+}
+
 func TestControllerDualStackConfig(t *testing.T) {
 	k := &testK8S{t: t}
 	c := &controller{
@@ -1194,14 +1301,4 @@ func TestControllerDualStackConfig(t *testing.T) {
 	if diff := diffService(wantSvc, gotSvc); diff != "" {
 		t.Errorf("SetBalancer produced unexpected mutation (-want +got)\n%s", diff)
 	}
-
-	// Now that an IP is allocated, removing the IP pool is not allowed.
-	if c.SetPools(l, &config.Pools{ByName: map[string]*config.Pool{}}) != controllers.SyncStateError {
-		t.Fatalf("SetPools that deletes allocated IPs was accepted")
-	}
-
-	// Deleting the config also makes MetalLB sad.
-	if c.SetPools(l, nil) != controllers.SyncStateErrorNoRetry {
-		t.Fatalf("SetPools that deletes the config was accepted")
-	}
 }
diff --git a/e2etest/l2tests/assignment.go b/e2etest/l2tests/assignment.go
index 130bcc10..9d325fc9 100644
--- a/e2etest/l2tests/assignment.go
+++ b/e2etest/l2tests/assignment.go
@@ -9,7 +9,6 @@ import (
 
 	"github.com/onsi/ginkgo/v2"
 	"github.com/onsi/gomega"
-
 	"go.universe.tf/e2etest/pkg/config"
 	"go.universe.tf/e2etest/pkg/k8s"
 	"go.universe.tf/e2etest/pkg/metallb"
@@ -23,6 +22,7 @@ import (
 	"k8s.io/kubernetes/test/e2e/framework"
 	e2eservice "k8s.io/kubernetes/test/e2e/framework/service"
 	admissionapi "k8s.io/pod-security-admission/api"
+	"sigs.k8s.io/controller-runtime/pkg/client"
 )
 
 const secondNamespace = "test-namespace"
@@ -218,6 +218,95 @@ var _ = ginkgo.Describe("IP Assignment", func() {
 		})
 	})
 
+	ginkgo.Context("IPV4 removing pools", func() {
+		var pool1 metallbv1beta1.IPAddressPool
+		var pool2 metallbv1beta1.IPAddressPool
+
+		ginkgo.AfterEach(func() {
+			// Clean previous configuration.
+			err := ConfigUpdater.Clean()
+			framework.ExpectNoError(err)
+		})
+
+		ginkgo.BeforeEach(func() {
+			pool1 = metallbv1beta1.IPAddressPool{
+				ObjectMeta: metav1.ObjectMeta{Name: "test-ns-pool-1"},
+				Spec: metallbv1beta1.IPAddressPoolSpec{
+					Addresses: []string{
+						"192.168.5.0/32",
+					},
+					AllocateTo: &metallbv1beta1.ServiceAllocation{Priority: 20, Namespaces: []string{f.Namespace.Name}},
+				},
+			}
+			pool2 = metallbv1beta1.IPAddressPool{
+				ObjectMeta: metav1.ObjectMeta{Name: "test-ns-pool-2"},
+				Spec: metallbv1beta1.IPAddressPoolSpec{
+					Addresses: []string{
+						"192.168.10.0/32",
+					},
+				},
+			}
+
+			resources := config.Resources{
+				Pools: []metallbv1beta1.IPAddressPool{pool1, pool2},
+			}
+
+			err := ConfigUpdater.Update(resources)
+			framework.ExpectNoError(err)
+		})
+
+		ginkgo.It("removes all pools", func() {
+			svc1, _ := service.CreateWithBackend(cs, f.Namespace.Name, "svc-test-ns-pool-1")
+			defer func() {
+				service.Delete(cs, svc1)
+			}()
+
+			ginkgo.By("validate LoadBalancer IP is allocated from pool1")
+			err := config.ValidateIPInRange([]metallbv1beta1.IPAddressPool{pool1}, e2eservice.GetIngressPoint(
+				&svc1.Status.LoadBalancer.Ingress[0]))
+			framework.ExpectNoError(err)
+
+			ginkgo.By("deleting all pools")
+			err = ConfigUpdater.Client().DeleteAllOf(context.Background(), &metallbv1beta1.IPAddressPool{}, client.InNamespace(ConfigUpdater.Namespace()))
+			framework.ExpectNoError(err)
+
+			ginkgo.By("validate LoadBalancer IP is removed from the svc")
+			gomega.Eventually(func() int {
+				s, err := cs.CoreV1().Services(svc1.Namespace).Get(context.Background(), svc1.Name, metav1.GetOptions{})
+				framework.ExpectNoError(err)
+				return len(s.Status.LoadBalancer.Ingress)
+			}, time.Minute, 1*time.Second).Should(gomega.Equal(0))
+		})
+
+		ginkgo.It("reallocates svc after deleting a pool", func() {
+			svc1, _ := service.CreateWithBackend(cs, f.Namespace.Name, "svc-test-ns-pool-1")
+			defer func() {
+				service.Delete(cs, svc1)
+			}()
+
+			ginkgo.By("validate LoadBalancer IP is allocated from pool1")
+			err := config.ValidateIPInRange([]metallbv1beta1.IPAddressPool{pool1}, e2eservice.GetIngressPoint(
+				&svc1.Status.LoadBalancer.Ingress[0]))
+			framework.ExpectNoError(err)
+
+			ginkgo.By("deleting pool 1")
+			p := &metallbv1beta1.IPAddressPool{}
+			err = ConfigUpdater.Client().Get(context.Background(), client.ObjectKey{Namespace: ConfigUpdater.Namespace(), Name: pool1.Name}, p)
+			framework.ExpectNoError(err)
+			err = ConfigUpdater.Client().Delete(context.Background(), p)
+			framework.ExpectNoError(err)
+
+			ginkgo.By("validate LoadBalancer IP is re-allocated from pool2")
+			gomega.Eventually(func() error {
+				svc1, err := cs.CoreV1().Services(svc1.Namespace).Get(context.Background(), svc1.Name, metav1.GetOptions{})
+				framework.ExpectNoError(err)
+				err = config.ValidateIPInRange([]metallbv1beta1.IPAddressPool{pool2}, e2eservice.GetIngressPoint(
+					&svc1.Status.LoadBalancer.Ingress[0]))
+				return err
+			}, time.Minute, 1*time.Second).ShouldNot(gomega.HaveOccurred())
+		})
+	})
+
 	ginkgo.Context("IPV4 - Validate service allocation in address pools", func() {
 		ginkgo.AfterEach(func() {
 			// Clean previous configuration.
diff --git a/internal/allocator/allocator_test.go b/internal/allocator/allocator_test.go
index 651feeab..039d0800 100644
--- a/internal/allocator/allocator_test.go
+++ b/internal/allocator/allocator_test.go
@@ -45,7 +45,7 @@ func selector(s string) labels.Selector {
 
 func TestAssignment(t *testing.T) {
 	alloc := New()
-	if err := alloc.SetPools(&config.Pools{ByName: map[string]*config.Pool{
+	alloc.SetPools(&config.Pools{ByName: map[string]*config.Pool{
 		"test": {
 			Name:       "test",
 			AutoAssign: true,
@@ -105,9 +105,7 @@ func TestAssignment(t *testing.T) {
 				ipnet("1000::9:0/120"),
 			},
 		},
-	}}); err != nil {
-		t.Fatalf("SetPools: %s", err)
-	}
+	}})
 
 	tests := []struct {
 		desc       string
@@ -651,7 +649,7 @@ func TestPoolAllocation(t *testing.T) {
 	// This test only allocates from the "test" pool, so it will run
 	// out of IPs quickly even though there are tons available in
 	// other pools.
-	if err := alloc.SetPools(&config.Pools{ByName: map[string]*config.Pool{
+	alloc.SetPools(&config.Pools{ByName: map[string]*config.Pool{
 		"not_this_one": {
 			Name:       "not_this_one",
 			AutoAssign: true,
@@ -672,9 +670,7 @@ func TestPoolAllocation(t *testing.T) {
 			AutoAssign: true,
 			CIDR:       []*net.IPNet{ipnet("10.20.30.0/24"), ipnet("fc00::2:0/120")},
 		},
-	}}); err != nil {
-		t.Fatalf("SetPools: %s", err)
-	}
+	}})
 
 	validIP4s := map[string]bool{
 		"1.2.3.4":  true,
@@ -1103,7 +1099,7 @@ func TestPoolAllocation(t *testing.T) {
 
 func TestAllocation(t *testing.T) {
 	alloc := New()
-	if err := alloc.SetPools(&config.Pools{ByName: map[string]*config.Pool{
+	alloc.SetPools(&config.Pools{ByName: map[string]*config.Pool{
 		"test1": {
 			Name:       "test1",
 			AutoAssign: true,
@@ -1114,9 +1110,7 @@ func TestAllocation(t *testing.T) {
 			AutoAssign: true,
 			CIDR:       []*net.IPNet{ipnet("1.2.3.10/31"), ipnet("1000::10/127")},
 		},
-	}}); err != nil {
-		t.Fatalf("SetPools: %s", err)
-	}
+	}})
 
 	validIP4s := map[string]bool{
 		"1.2.3.4":  true,
@@ -1476,7 +1470,7 @@ func TestAllocation(t *testing.T) {
 
 func TestBuggyIPs(t *testing.T) {
 	alloc := New()
-	if err := alloc.SetPools(&config.Pools{ByName: map[string]*config.Pool{
+	alloc.SetPools(&config.Pools{ByName: map[string]*config.Pool{
 		"test": {
 			Name:       "test",
 			AutoAssign: true,
@@ -1499,9 +1493,7 @@ func TestBuggyIPs(t *testing.T) {
 			AutoAssign:    true,
 			CIDR:          []*net.IPNet{ipnet("1.2.4.254/31")},
 		},
-	}}); err != nil {
-		t.Fatalf("SetPools: %s", err)
-	}
+	}})
 
 	validIPs := map[string]bool{
 		"1.2.3.0":   true,
@@ -1550,27 +1542,11 @@ func TestBuggyIPs(t *testing.T) {
 }
 
 func TestConfigReload(t *testing.T) {
-	alloc := New()
-	if err := alloc.SetPools(&config.Pools{ByName: map[string]*config.Pool{
-		"test": {
-			Name:       "test",
-			AutoAssign: true,
-			CIDR:       []*net.IPNet{ipnet("1.2.3.0/30"), ipnet("1000::/126")},
-		},
-	}}); err != nil {
-		t.Fatalf("SetPools: %s", err)
-	}
-	if err := alloc.Assign("s1", svc, []net.IP{net.ParseIP("1.2.3.0")}, nil, "", ""); err != nil {
-		t.Fatalf("Assign(s1, 1.2.3.0): %s", err)
-	}
-	if err := alloc.Assign("s2", svc, []net.IP{net.ParseIP("1000::")}, nil, "", ""); err != nil {
-		t.Fatalf("Assign(s2, 1000::): %s", err)
-	}
 	tests := []struct {
-		desc    string
-		pools   map[string]*config.Pool
-		wantErr bool
-		pool    string // Pool that 1.2.3.0 and 1000:: should be in
+		desc   string
+		pools  map[string]*config.Pool
+		poolS1 string // Pool that 1.2.3.0 should be in
+		poolS2 string
 	}{
 		{
 			desc: "set same config is no-op",
@@ -1581,7 +1557,8 @@ func TestConfigReload(t *testing.T) {
 					CIDR:       []*net.IPNet{ipnet("1.2.3.0/30"), ipnet("1000::/126")},
 				},
 			},
-			pool: "test",
+			poolS1: "test",
+			poolS2: "test",
 		},
 		{
 			desc: "expand pool",
@@ -1592,7 +1569,8 @@ func TestConfigReload(t *testing.T) {
 					CIDR:       []*net.IPNet{ipnet("1.2.3.0/24"), ipnet("1000::/120")},
 				},
 			},
-			pool: "test",
+			poolS1: "test",
+			poolS2: "test",
 		},
 		{
 			desc: "shrink pool",
@@ -1603,10 +1581,11 @@ func TestConfigReload(t *testing.T) {
 					CIDR:       []*net.IPNet{ipnet("1.2.3.0/30"), ipnet("1000::/126")},
 				},
 			},
-			pool: "test",
+			poolS1: "test",
+			poolS2: "test",
 		},
 		{
-			desc: "can't shrink further",
+			desc: "can't shrink further, removing the service allocation",
 			pools: map[string]*config.Pool{
 				"test": {
 					Name:       "test",
@@ -1614,11 +1593,11 @@ func TestConfigReload(t *testing.T) {
 					CIDR:       []*net.IPNet{ipnet("1.2.3.2/31"), ipnet("1000::0/126")},
 				},
 			},
-			pool:    "test",
-			wantErr: true,
+			poolS1: "",
+			poolS2: "test",
 		},
 		{
-			desc: "can't shrink further ipv6",
+			desc: "can't shrink further ipv6, removing the service allocation",
 			pools: map[string]*config.Pool{
 				"test": {
 					Name:       "test",
@@ -1626,8 +1605,8 @@ func TestConfigReload(t *testing.T) {
 					CIDR:       []*net.IPNet{ipnet("1.2.3.0/30"), ipnet("1000::2/127")},
 				},
 			},
-			pool:    "test",
-			wantErr: true,
+			poolS1: "test",
+			poolS2: "",
 		},
 		{
 			desc: "rename the pool",
@@ -1638,7 +1617,8 @@ func TestConfigReload(t *testing.T) {
 					CIDR:       []*net.IPNet{ipnet("1.2.3.0/30"), ipnet("1000::0/126")},
 				},
 			},
-			pool: "test2",
+			poolS1: "test2",
+			poolS2: "test2",
 		},
 		{
 			desc: "split pool",
@@ -1646,66 +1626,16 @@ func TestConfigReload(t *testing.T) {
 				"test": {
 					Name:       "test",
 					AutoAssign: true,
-					CIDR:       []*net.IPNet{ipnet("1.2.3.0/31"), ipnet("1000::/127")},
+					CIDR:       []*net.IPNet{ipnet("1.2.3.0/31"), ipnet("1000::2/127")},
 				},
 				"test2": {
 					Name:       "test2",
 					AutoAssign: true,
-					CIDR:       []*net.IPNet{ipnet("1.2.3.2/31"), ipnet("1000::2/127")},
+					CIDR:       []*net.IPNet{ipnet("1.2.3.2/31"), ipnet("1000::/127")},
 				},
 			},
-			pool: "test",
-		},
-		{
-			desc: "swap pool names",
-			pools: map[string]*config.Pool{
-				"test2": {
-					Name:       "test2",
-					AutoAssign: true,
-					CIDR:       []*net.IPNet{ipnet("1.2.3.0/31"), ipnet("1000::/127")},
-				},
-				"test": {
-					Name:       "test",
-					AutoAssign: true,
-					CIDR:       []*net.IPNet{ipnet("1.2.3.2/31"), ipnet("1000::2/127")},
-				},
-			},
-			pool: "test2",
-		},
-		{
-			desc: "delete used pool",
-			pools: map[string]*config.Pool{
-				"test": {
-					Name:       "test",
-					AutoAssign: true,
-					CIDR:       []*net.IPNet{ipnet("1.2.3.2/31"), ipnet("1000::/126")},
-				},
-			},
-			pool:    "test2",
-			wantErr: true,
-		},
-		{
-			desc: "delete used pool ipv6",
-			pools: map[string]*config.Pool{
-				"test": {
-					Name:       "test",
-					AutoAssign: true,
-					CIDR:       []*net.IPNet{ipnet("1.2.3.0/30"), ipnet("1000::2/127")},
-				},
-			},
-			pool:    "test2",
-			wantErr: true,
-		},
-		{
-			desc: "delete unused pool",
-			pools: map[string]*config.Pool{
-				"test2": {
-					Name:       "test2",
-					AutoAssign: true,
-					CIDR:       []*net.IPNet{ipnet("1.2.3.0/31"), ipnet("1000::/127")},
-				},
-			},
-			pool: "test2",
+			poolS1: "test",
+			poolS2: "test2",
 		},
 		{
 			desc: "enable buggy IPs not allowed",
@@ -1717,30 +1647,47 @@ func TestConfigReload(t *testing.T) {
 					CIDR:          []*net.IPNet{ipnet("1.2.3.0/31"), ipnet("1000::/127")},
 				},
 			},
-			pool:    "test2",
-			wantErr: true,
+			poolS1: "",
+			poolS2: "test2",
 		},
 	}
 
 	for _, test := range tests {
-		err := alloc.SetPools(&config.Pools{ByName: test.pools})
-		if test.wantErr {
-			if err == nil {
-				t.Errorf("%q should have failed to SetPools, but succeeded", test.desc)
-			}
-		} else if err != nil {
-			t.Errorf("%q failed to SetPools: %s", test.desc, err)
+		alloc := New()
+		alloc.SetPools(&config.Pools{ByName: map[string]*config.Pool{
+			"test": {
+				Name:       "test",
+				AutoAssign: true,
+				CIDR:       []*net.IPNet{ipnet("1.2.3.0/30"), ipnet("1000::/126")},
+			},
+			"testunused": {
+				Name:       "unused",
+				AutoAssign: true,
+				CIDR:       []*net.IPNet{ipnet("1.2.4.0/30"), ipnet("1001::/126")},
+			},
+		}})
+		if err := alloc.Assign("s1", svc, []net.IP{net.ParseIP("1.2.3.0")}, nil, "", ""); err != nil {
+			t.Fatalf("Assign(s1, 1.2.3.0): %s", err)
+		}
+		if err := alloc.Assign("s2", svc, []net.IP{net.ParseIP("1000::")}, nil, "", ""); err != nil {
+			t.Fatalf("Assign(s2, 1000::): %s", err)
 		}
+
+		alloc.SetPools(&config.Pools{ByName: test.pools})
 		gotPool := alloc.Pool("s1")
-		if gotPool != test.pool {
-			t.Errorf("%q: s1 is in wrong pool, want %q, got %q", test.desc, test.pool, gotPool)
+		if gotPool != test.poolS1 {
+			t.Errorf("%q: s1 is in wrong pool, want %q, got %q", test.desc, test.poolS1, gotPool)
+		}
+		gotPool = alloc.Pool("s2")
+		if gotPool != test.poolS2 {
+			t.Errorf("%q: s2 is in wrong pool, want %q, got %q", test.desc, test.poolS2, gotPool)
 		}
 	}
 }
 
 func TestAutoAssign(t *testing.T) {
 	alloc := New()
-	if err := alloc.SetPools(&config.Pools{ByName: map[string]*config.Pool{
+	alloc.SetPools(&config.Pools{ByName: map[string]*config.Pool{
 		"test1": {
 			Name:       "test1",
 			AutoAssign: false,
@@ -1751,9 +1698,7 @@ func TestAutoAssign(t *testing.T) {
 			AutoAssign: true,
 			CIDR:       []*net.IPNet{ipnet("1.2.3.10/31"), ipnet("1000::10/127")},
 		},
-	}}); err != nil {
-		t.Fatalf("SetPools: %s", err)
-	}
+	}})
 
 	validIP4s := map[string]bool{
 		"1.2.3.4":  false,
@@ -1988,7 +1933,7 @@ func TestPoolCount(t *testing.T) {
 
 func TestPoolMetrics(t *testing.T) {
 	alloc := New()
-	if err := alloc.SetPools(&config.Pools{ByName: map[string]*config.Pool{
+	alloc.SetPools(&config.Pools{ByName: map[string]*config.Pool{
 		"test": {
 			Name:       "test",
 			AutoAssign: true,
@@ -1997,9 +1942,7 @@ func TestPoolMetrics(t *testing.T) {
 				ipnet("1000::4/126"),
 			},
 		},
-	}}); err != nil {
-		t.Fatalf("SetPools: %s", err)
-	}
+	}})
 
 	tests := []struct {
 		desc       string