neonvm-controller: fail releasing IP (#1278)

We must not leak IP addresses. The release will be retried.

Also, we have to make it idempotent.

Signed-off-by: Oleg Vasilev <[email protected]>

Author: Omrigan

pkg/neonvm/controllers/vm_controller.go

@@ -151,7 +151,10 @@ func (r *VMReconciler) Reconcile(ctx context.Context, req ctrl.Request) (ctrl.Re
 		if controllerutil.ContainsFinalizer(&vm, virtualmachineFinalizer) {
 			// our finalizer is present, so lets handle any external dependency
 			log.Info("Performing Finalizer Operations for VirtualMachine before delete it")
-			r.doFinalizerOperationsForVirtualMachine(ctx, &vm)
+			if err := r.doFinalizerOperationsForVirtualMachine(ctx, &vm); err != nil {
+				log.Error(err, "Failed to perform finalizer operations for VirtualMachine")
+				return ctrl.Result{}, err
+			}
 
 			// remove our finalizer from the list and update it.
 			log.Info("Removing Finalizer for VirtualMachine after successfully perform the operations")
@@ -226,7 +229,7 @@ func (r *VMReconciler) Reconcile(ctx context.Context, req ctrl.Request) (ctrl.Re
 }
 
 // doFinalizerOperationsForVirtualMachine will perform the required operations before delete the CR.
-func (r *VMReconciler) doFinalizerOperationsForVirtualMachine(ctx context.Context, vm *vmv1.VirtualMachine) {
+func (r *VMReconciler) doFinalizerOperationsForVirtualMachine(ctx context.Context, vm *vmv1.VirtualMachine) error {
 	// Note: It is not recommended to use finalizers with the purpose of delete resources which are
 	// created and managed in the reconciliation. These ones, such as the Pod created on this reconcile,
 	// are defined as depended of the custom resource. See that we use the method ctrl.SetControllerReference.
@@ -245,14 +248,13 @@ func (r *VMReconciler) doFinalizerOperationsForVirtualMachine(ctx context.Contex
 	if vm.Spec.ExtraNetwork != nil {
 		ip, err := r.IPAM.ReleaseIP(ctx, types.NamespacedName{Name: vm.Name, Namespace: vm.Namespace})
 		if err != nil {
-			// ignore error
-			log.Error(err, "fail to release IP, error ignored")
-			return
+			return fmt.Errorf("fail to release IP: %w", err)
 		}
 		message := fmt.Sprintf("Released IP %s", ip.String())
 		log.Info(message)
 		r.Recorder.Event(vm, "Normal", "OverlayNet", message)
 	}
+	return nil
 }
 
 func getRunnerVersion(pod *corev1.Pod) (api.RunnerProtoVersion, error) {

pkg/neonvm/ipam/allocate.go

@@ -1,11 +1,13 @@
 package ipam
 
 import (
+	"context"
 	"net"
 
 	whereaboutsallocate "github.com/k8snetworkplumbingwg/whereabouts/pkg/allocate"
 	whereaboutslogging "github.com/k8snetworkplumbingwg/whereabouts/pkg/logging"
 	whereaboutstypes "github.com/k8snetworkplumbingwg/whereabouts/pkg/types"
+	"sigs.k8s.io/controller-runtime/pkg/log"
 
 	"k8s.io/apimachinery/pkg/types"
 )
@@ -16,20 +18,21 @@ type ipamAction = func(
 ) (net.IPNet, []whereaboutstypes.IPReservation, error)
 
 // makeAcquireAction creates a callback which changes IPPool state to include a new IP reservation.
-func makeAcquireAction(vmName types.NamespacedName) ipamAction {
+func makeAcquireAction(ctx context.Context, vmName types.NamespacedName) ipamAction {
 	return func(ipRange RangeConfiguration, reservation []whereaboutstypes.IPReservation) (net.IPNet, []whereaboutstypes.IPReservation, error) {
-		return doAcquire(ipRange, reservation, vmName)
+		return doAcquire(ctx, ipRange, reservation, vmName)
 	}
 }
 
 // makeReleaseAction creates a callback which changes IPPool state to deallocate an IP reservation.
-func makeReleaseAction(vmName types.NamespacedName) ipamAction {
+func makeReleaseAction(ctx context.Context, vmName types.NamespacedName) ipamAction {
 	return func(ipRange RangeConfiguration, reservation []whereaboutstypes.IPReservation) (net.IPNet, []whereaboutstypes.IPReservation, error) {
-		return doRelease(ipRange, reservation, vmName)
+		return doRelease(ctx, ipRange, reservation, vmName)
 	}
 }
 
 func doAcquire(
+	_ context.Context,
 	ipRange RangeConfiguration,
 	reservation []whereaboutstypes.IPReservation,
 	vmName types.NamespacedName,
@@ -57,19 +60,27 @@ func doAcquire(
 }
 
 func doRelease(
+	ctx context.Context,
 	ipRange RangeConfiguration,
 	reservation []whereaboutstypes.IPReservation,
 	vmName types.NamespacedName,
 ) (net.IPNet, []whereaboutstypes.IPReservation, error) {
 	// reduce whereabouts logging
 	whereaboutslogging.SetLogLevel("error")
 
+	log := log.FromContext(ctx)
+
 	_, ipnet, _ := net.ParseCIDR(ipRange.Range)
 
 	// try to release IP for given VM
 	newReservation, ip, err := whereaboutsallocate.IterateForDeallocation(reservation, vmName.String(), getMatchingIPReservationIndex)
 	if err != nil {
-		return net.IPNet{}, nil, err
+		// The only reason to get an error here is if we are trying
+		// to deallocate the same IP twice.
+		log.Info("Failed to deallocate IP", "error", err)
+
+		// Ignore the error.
+		return net.IPNet{IP: ip, Mask: ipnet.Mask}, newReservation, nil
 	}
 
 	return net.IPNet{IP: ip, Mask: ipnet.Mask}, newReservation, nil

pkg/neonvm/ipam/ipam.go

@@ -56,15 +56,15 @@ type IPAM struct {
 }
 
 func (i *IPAM) AcquireIP(ctx context.Context, vmName types.NamespacedName) (net.IPNet, error) {
-	ip, err := i.runIPAM(ctx, makeAcquireAction(vmName))
+	ip, err := i.runIPAM(ctx, makeAcquireAction(ctx, vmName))
 	if err != nil {
 		return net.IPNet{}, fmt.Errorf("failed to acquire IP: %w", err)
 	}
 	return ip, nil
 }
 
 func (i *IPAM) ReleaseIP(ctx context.Context, vmName types.NamespacedName) (net.IPNet, error) {
-	ip, err := i.runIPAM(ctx, makeReleaseAction(vmName))
+	ip, err := i.runIPAM(ctx, makeReleaseAction(ctx, vmName))
 	if err != nil {
 		return net.IPNet{}, fmt.Errorf("failed to release IP: %w", err)
 	}

pkg/neonvm/ipam/ipam_test.go

@@ -3,6 +3,7 @@ package ipam_test
 import (
 	"context"
 	"fmt"
+	"net"
 	"testing"
 
 	nadv1 "github.com/k8snetworkplumbingwg/network-attachment-definition-client/pkg/apis/k8s.cni.cncf.io/v1"
@@ -95,3 +96,42 @@ func TestIPAM(t *testing.T) {
 	require.NotNil(t, ip3)
 	assert.Equal(t, ip2, ip3)
 }
+
+func TestIPAMReleaseTwice(t *testing.T) {
+	ipam := makeIPAM(t,
+		`{
+			"ipRanges": [
+				{
+					"range":"10.100.123.0/24",
+					"range_start":"10.100.123.1",
+					"range_end":"10.100.123.254"
+				}
+			]
+		}`,
+	)
+
+	defer ipam.Close()
+
+	name := types.NamespacedName{
+		Namespace: "default",
+		Name:      "vm",
+	}
+
+	ip, err := ipam.AcquireIP(context.Background(), name)
+	require.NoError(t, err)
+	require.NotNil(t, ip)
+
+	// Release the IP
+	ipResult, err := ipam.ReleaseIP(context.Background(), name)
+	require.NoError(t, err)
+	require.Equal(t, ip, ipResult)
+
+	// Release the IP again
+	ipResult, err = ipam.ReleaseIP(context.Background(), name)
+	require.NoError(t, err)
+	// This time we get nil IP
+	require.Equal(t, net.IPNet{
+		IP:   nil,
+		Mask: ip.Mask,
+	}, ipResult)
+}