Include PendingRelease when calculating IP pool for scale-down (#1237)

* fix scale down

Signed-off-by: Evan Baker <rbtr@users.noreply.github.com>

* update pool state variable naming

Signed-off-by: Evan Baker <rbtr@users.noreply.github.com>

* add test for pending release

Signed-off-by: Evan Baker <rbtr@users.noreply.github.com>

cns/ipampool/metrics.go

@@ -8,14 +8,8 @@ import (
 var (
 	ipamAllocatedIPCount = prometheus.NewGauge(
 		prometheus.GaugeOpts{
-			Name: "ipam_allocated_ips",
-			Help: "CNS's allocated IP pool size.",
-		},
-	)
-	ipamAssignedIPCount = prometheus.NewGauge(
-		prometheus.GaugeOpts{
-			Name: "ipam_assigned_ips",
-			Help: "Assigned IP count.",
+			Name: "ipam_pod_allocated_ips",
+			Help: "Count of IPs CNS has allocated to Pods.",
 		},
 	)
 	ipamAvailableIPCount = prometheus.NewGauge(
@@ -30,6 +24,18 @@ var (
 			Help: "IPAM IP pool batch size.",
 		},
 	)
+	ipamCurrentAvailableIPcount = prometheus.NewGauge(
+		prometheus.GaugeOpts{
+			Name: "ipam_current_available_ips",
+			Help: "Current available IP count.",
+		},
+	)
+	ipamExpectedAvailableIPCount = prometheus.NewGauge(
+		prometheus.GaugeOpts{
+			Name: "ipam_expect_available_ips",
+			Help: "Expected future available IP count assuming the Requested IP count is honored.",
+		},
+	)
 	ipamMaxIPCount = prometheus.NewGauge(
 		prometheus.GaugeOpts{
 			Name: "ipam_max_ips",
@@ -54,16 +60,10 @@ var (
 			Help: "Requested IP count.",
 		},
 	)
-	ipamRequestedUnassignedIPConfigCount = prometheus.NewGauge(
+	ipamTotalIPCount = prometheus.NewGauge(
 		prometheus.GaugeOpts{
-			Name: "ipam_requested_unassigned_ips",
-			Help: "Future unassigned IP count assuming the Requested IP count is honored.",
-		},
-	)
-	ipamUnassignedIPCount = prometheus.NewGauge(
-		prometheus.GaugeOpts{
-			Name: "ipam_unassigned_ips",
-			Help: "Unassigned IP count.",
+			Name: "ipam_total_ips",
+			Help: "Count of total IP pool size allocated to CNS by DNC.",
 		},
 	)
 )
@@ -71,27 +71,27 @@ var (
 func init() {
 	metrics.Registry.MustRegister(
 		ipamAllocatedIPCount,
-		ipamAssignedIPCount,
 		ipamAvailableIPCount,
 		ipamBatchSize,
+		ipamCurrentAvailableIPcount,
+		ipamExpectedAvailableIPCount,
 		ipamMaxIPCount,
 		ipamPendingProgramIPCount,
 		ipamPendingReleaseIPCount,
 		ipamRequestedIPConfigCount,
-		ipamRequestedUnassignedIPConfigCount,
-		ipamUnassignedIPCount,
+		ipamTotalIPCount,
 	)
 }
 
 func observeIPPoolState(state ipPoolState, meta metaState) {
-	ipamAllocatedIPCount.Set(float64(state.allocated))
-	ipamAssignedIPCount.Set(float64(state.assigned))
+	ipamAllocatedIPCount.Set(float64(state.allocatedToPods))
 	ipamAvailableIPCount.Set(float64(state.available))
 	ipamBatchSize.Set(float64(meta.batch))
+	ipamCurrentAvailableIPcount.Set(float64(state.currentAvailableIPs))
+	ipamExpectedAvailableIPCount.Set(float64(state.expectedAvailableIPs))
 	ipamMaxIPCount.Set(float64(meta.max))
 	ipamPendingProgramIPCount.Set(float64(state.pendingProgramming))
 	ipamPendingReleaseIPCount.Set(float64(state.pendingRelease))
-	ipamRequestedIPConfigCount.Set(float64(state.requested))
-	ipamRequestedUnassignedIPConfigCount.Set(float64(state.requestedUnassigned))
-	ipamUnassignedIPCount.Set(float64(state.unassigned))
+	ipamRequestedIPConfigCount.Set(float64(state.requestedIPs))
+	ipamTotalIPCount.Set(float64(state.totalIPs))
 }

cns/ipampool/monitor.go

@@ -102,33 +102,33 @@ func (pm *Monitor) Start(ctx context.Context) error {
 
 // ipPoolState is the current actual state of the CNS IP pool.
 type ipPoolState struct {
-	// allocated are the IPs given to CNS.
-	allocated int64
-	// assigned are the IPs CNS gives to Pods.
-	assigned int64
-	// available are the allocated IPs in state "Available".
+	// allocatedToPods are the IPs CNS gives to Pods.
+	allocatedToPods int64
+	// available are the IPs in state "Available".
 	available int64
-	// pendingProgramming are the allocated IPs in state "PendingProgramming".
+	// currentAvailableIPs are the current available IPs: allocated - assigned - pendingRelease.
+	currentAvailableIPs int64
+	// expectedAvailableIPs are the "future" available IPs, if the requested IP count is honored: requested - assigned.
+	expectedAvailableIPs int64
+	// pendingProgramming are the IPs in state "PendingProgramming".
 	pendingProgramming int64
-	// pendingRelease are the allocated IPs in state "PendingRelease".
+	// pendingRelease are the IPs in state "PendingRelease".
 	pendingRelease int64
-	// requested are the IPs CNS has requested that it be allocated.
-	requested int64
-	// requestedUnassigned are the "future" unassigned IPs, if the requested IP count is honored: requested - assigned.
-	requestedUnassigned int64
-	// unassigned are the currently unassigned IPs: allocated - assigned.
-	unassigned int64
+	// requestedIPs are the IPs CNS has requested that it be allocated by DNC.
+	requestedIPs int64
+	// totalIPs are all the IPs given to CNS by DNC.
+	totalIPs int64
 }
 
 func buildIPPoolState(ips map[string]cns.IPConfigurationStatus, spec v1alpha.NodeNetworkConfigSpec) ipPoolState {
 	state := ipPoolState{
-		allocated: int64(len(ips)),
-		requested: spec.RequestedIPCount,
+		totalIPs:     int64(len(ips)),
+		requestedIPs: spec.RequestedIPCount,
 	}
 	for _, v := range ips {
 		switch v.GetState() {
 		case types.Assigned:
-			state.assigned++
+			state.allocatedToPods++
 		case types.Available:
 			state.available++
 		case types.PendingProgramming:
@@ -137,8 +137,8 @@ func buildIPPoolState(ips map[string]cns.IPConfigurationStatus, spec v1alpha.Nod
 			state.pendingRelease++
 		}
 	}
-	state.unassigned = state.allocated - state.assigned
-	state.requestedUnassigned = state.requested - state.assigned
+	state.currentAvailableIPs = state.totalIPs - state.allocatedToPods - state.pendingRelease
+	state.expectedAvailableIPs = state.requestedIPs - state.allocatedToPods
 	return state
 }
 
@@ -150,8 +150,8 @@ func (pm *Monitor) reconcile(ctx context.Context) error {
 
 	switch {
 	// pod count is increasing
-	case state.requestedUnassigned < pm.metastate.minFreeCount:
-		if state.requested == pm.metastate.max {
+	case state.expectedAvailableIPs < pm.metastate.minFreeCount:
+		if state.requestedIPs == pm.metastate.max {
 			// If we're already at the maxIPCount, don't try to increase
 			return nil
 		}
@@ -160,7 +160,7 @@ func (pm *Monitor) reconcile(ctx context.Context) error {
 		return pm.increasePoolSize(ctx, state)
 
 	// pod count is decreasing
-	case state.unassigned >= pm.metastate.maxFreeCount:
+	case state.currentAvailableIPs >= pm.metastate.maxFreeCount:
 		logger.Printf("[ipam-pool-monitor] Decreasing pool size...")
 		return pm.decreasePoolSize(ctx, state)
 
@@ -171,7 +171,7 @@ func (pm *Monitor) reconcile(ctx context.Context) error {
 		return pm.cleanPendingRelease(ctx)
 
 	// no pods scheduled
-	case state.assigned == 0:
+	case state.allocatedToPods == 0:
 		logger.Printf("[ipam-pool-monitor] No pods scheduled")
 		return nil
 	}

cns/ipampool/monitor_test.go

@@ -37,8 +37,10 @@ type testState struct {
 	assigned                int
 	batch                   int64
 	max                     int64
+	pendingRelease          int64
 	releaseThresholdPercent int64
 	requestThresholdPercent int64
+	totalIPs                int64
 }
 
 func initFakes(state testState) (*fakes.HTTPServiceFake, *fakes.RequestControllerFake, *Monitor) {
@@ -52,8 +54,11 @@ func initFakes(state testState) (*fakes.HTTPServiceFake, *fakes.RequestControlle
 	}
 	subnetaddresspace := "10.0.0.0/8"
 
+	if state.totalIPs == 0 {
+		state.totalIPs = state.allocated
+	}
 	fakecns := fakes.NewHTTPServiceFake()
-	fakerc := fakes.NewRequestControllerFake(fakecns, scalarUnits, subnetaddresspace, state.allocated)
+	fakerc := fakes.NewRequestControllerFake(fakecns, scalarUnits, subnetaddresspace, state.totalIPs)
 
 	poolmonitor := NewMonitor(fakecns, &fakeNodeNetworkConfigUpdater{fakerc.NNC}, &Options{RefreshDelay: 100 * time.Second})
 	poolmonitor.metastate = metaState{
@@ -64,6 +69,9 @@ func initFakes(state testState) (*fakes.HTTPServiceFake, *fakes.RequestControlle
 	if err := fakecns.SetNumberOfAssignedIPs(state.assigned); err != nil {
 		logger.Printf("%s", err)
 	}
+	if _, err := fakecns.MarkIPAsPendingRelease(int(state.pendingRelease)); err != nil {
+		logger.Printf("%s", err)
+	}
 
 	return fakecns, fakerc, poolmonitor
 }
@@ -389,6 +397,40 @@ func TestDecreaseAfterNodeLimitReached(t *testing.T) {
 	assert.Len(t, poolmonitor.spec.IPsNotInUse, int(initState.max%initState.batch))
 }
 
+func TestDecreaseWithPendingRelease(t *testing.T) {
+	initState := testState{
+		batch:                   16,
+		assigned:                46,
+		allocated:               64,
+		pendingRelease:          8,
+		requestThresholdPercent: 50,
+		releaseThresholdPercent: 150,
+		totalIPs:                64,
+		max:                     250,
+	}
+	fakecns, fakerc, poolmonitor := initFakes(initState)
+	fakerc.NNC.Spec.RequestedIPCount = 48
+	assert.NoError(t, fakerc.Reconcile(true))
+
+	assert.NoError(t, poolmonitor.reconcile(context.Background()))
+
+	// reallocate some IPs
+	assert.NoError(t, fakecns.SetNumberOfAssignedIPs(40))
+	assert.NoError(t, poolmonitor.reconcile(context.Background()))
+
+	// Ensure poolmonitor asked for a multiple of batch size
+	assert.EqualValues(t, 64, poolmonitor.spec.RequestedIPCount)
+	assert.Len(t, poolmonitor.spec.IPsNotInUse, int(initState.pendingRelease))
+
+	// trigger a batch release
+	assert.NoError(t, fakecns.SetNumberOfAssignedIPs(30))
+	assert.NoError(t, poolmonitor.reconcile(context.Background()))
+
+	// Ensure poolmonitor asked for a multiple of batch size
+	assert.EqualValues(t, 48, poolmonitor.spec.RequestedIPCount)
+	assert.Len(t, poolmonitor.spec.IPsNotInUse, int(initState.batch)+int(initState.pendingRelease))
+}
+
 func TestPoolDecreaseBatchSizeGreaterThanMaxPodIPCount(t *testing.T) {
 	initState := testState{
 		batch:                   31,

test/integration/setup_test.go

@@ -94,6 +94,7 @@ func TestMain(m *testing.M) {
 		// create dirty cns ds
 		if installCNS, err := strconv.ParseBool(installopt); err == nil && installCNS == true {
 			if cnscleanup, err = installCNSDaemonset(ctx, clientset, os.Getenv(envTag), logDir); err != nil {
+				log.Print(err)
 				exitCode = 2
 				return
 			}