Fix ephemeral resource handling

- Fix the cleanup of ephemeral resources {nic,prefix,volume,vip}
- Update test cases

Co-authored-by: Axel Christ <[email protected]>

api/common/v1alpha1/common_types.go

@@ -40,6 +40,13 @@ const (
 	// ManagedByAnnotation is an annotation that can be applied to resources to signify that
 	// some external system is managing the resource.
 	ManagedByAnnotation = "common.api.onmetal.de/managed-by"
+
+	// EphemeralManagedByAnnotation is an annotation that can be applied to resources to signify that
+	// some ephemeral controller is managing the resource.
+	EphemeralManagedByAnnotation = "common.api.onmetal.de/ephemeral-managed-by"
+
+	// DefaultEphemeralManager is the default onmetal-api ephemeral manager.
+	DefaultEphemeralManager = "ephemeral-manager"
 )
 
 // ConfigMapKeySelector is a reference to a specific 'key' within a ConfigMap resource.

internal/controllers/compute/machine_ephemeralnetworkinterface_controller.go

@@ -19,6 +19,9 @@ import (
 	"errors"
 	"fmt"
 
+	"github.com/onmetal/onmetal-api/utils/annotations"
+	"golang.org/x/exp/maps"
+
 	"github.com/go-logr/logr"
 	commonv1alpha1 "github.com/onmetal/onmetal-api/api/common/v1alpha1"
 	computev1alpha1 "github.com/onmetal/onmetal-api/api/compute/v1alpha1"
@@ -73,22 +76,23 @@ func (r *MachineEphemeralNetworkInterfaceReconciler) ephemeralNetworkInterfaceBy
 				Namespace:   machine.Namespace,
 				Name:        nicName,
 				Labels:      ephemeral.NetworkInterfaceTemplate.Labels,
-				Annotations: ephemeral.NetworkInterfaceTemplate.Annotations,
+				Annotations: maps.Clone(ephemeral.NetworkInterfaceTemplate.Annotations),
 			},
 			Spec: ephemeral.NetworkInterfaceTemplate.Spec,
 		}
+		annotations.SetDefaultEphemeralManagedBy(nic)
+		_ = ctrl.SetControllerReference(machine, nic, r.Scheme())
 		nic.Spec.MachineRef = &commonv1alpha1.LocalUIDReference{
 			Name: machine.Name,
 			UID:  machine.UID,
 		}
-		_ = ctrl.SetControllerReference(machine, nic, r.Scheme())
 		res[nicName] = nic
 	}
 	return res
 }
 
 func (r *MachineEphemeralNetworkInterfaceReconciler) handleExistingNetworkInterface(ctx context.Context, log logr.Logger, machine *computev1alpha1.Machine, shouldManage bool, nic *networkingv1alpha1.NetworkInterface) error {
-	if metav1.IsControlledBy(nic, machine) {
+	if annotations.IsDefaultEphemeralControlledBy(nic, machine) {
 		if shouldManage {
 			log.V(1).Info("Ephemeral network interface is present and controlled by machine")
 			return nil
@@ -107,9 +111,8 @@ func (r *MachineEphemeralNetworkInterfaceReconciler) handleExistingNetworkInterf
 	}
 
 	if shouldManage {
-		return fmt.Errorf("network interface %s was not created for machine %s (machine is not owner)", nic.Name, machine.Name)
+		log.V(1).Info("Won't adopt unmanaged network interface")
 	}
-	// Network interface is not desired but also not controlled by the machine.
 	return nil
 }
 

internal/controllers/compute/machine_ephemeralnetworkinterface_controller_test.go

@@ -18,6 +18,7 @@ import (
 	commonv1alpha1 "github.com/onmetal/onmetal-api/api/common/v1alpha1"
 	computev1alpha1 "github.com/onmetal/onmetal-api/api/compute/v1alpha1"
 	networkingv1alpha1 "github.com/onmetal/onmetal-api/api/networking/v1alpha1"
+	"github.com/onmetal/onmetal-api/utils/annotations"
 	. "github.com/onmetal/onmetal-api/utils/testing"
 	. "github.com/onsi/ginkgo/v2"
 	. "github.com/onsi/gomega"
@@ -78,7 +79,7 @@ var _ = Describe("MachineEphemeralNetworkInterfaceController", func() {
 		))
 	})
 
-	It("should delete undesired ephemeral network interfaces", MustPassRepeatedly(10), func(ctx SpecContext) {
+	It("should delete undesired ephemeral network interfaces", func(ctx SpecContext) {
 		By("creating a machine")
 		machine := &computev1alpha1.Machine{
 			ObjectMeta: metav1.ObjectMeta{
@@ -104,10 +105,44 @@ var _ = Describe("MachineEphemeralNetworkInterfaceController", func() {
 				},
 			},
 		}
+		annotations.SetDefaultEphemeralManagedBy(undesiredNic)
 		Expect(ctrl.SetControllerReference(machine, undesiredNic, k8sClient.Scheme())).To(Succeed())
 		Expect(k8sClient.Create(ctx, undesiredNic)).To(Succeed())
 
 		By("waiting for the undesired network interface to be gone")
 		Eventually(Get(undesiredNic)).Should(Satisfy(apierrors.IsNotFound))
 	})
+
+	It("should not delete an externally managed network interface", func(ctx SpecContext) {
+		By("creating a machine")
+		machine := &computev1alpha1.Machine{
+			ObjectMeta: metav1.ObjectMeta{
+				Namespace:    ns.Name,
+				GenerateName: "machine-",
+			},
+			Spec: computev1alpha1.MachineSpec{
+				MachineClassRef: corev1.LocalObjectReference{Name: machineClass.Name},
+			},
+		}
+		Expect(k8sClient.Create(ctx, machine)).To(Succeed())
+
+		By("creating an externally managed network interface")
+		externalNic := &networkingv1alpha1.NetworkInterface{
+			ObjectMeta: metav1.ObjectMeta{
+				Namespace:    ns.Name,
+				GenerateName: "external-nic-",
+			},
+			Spec: networkingv1alpha1.NetworkInterfaceSpec{
+				NetworkRef: corev1.LocalObjectReference{Name: "my-network"},
+				IPs: []networkingv1alpha1.IPSource{
+					{Value: commonv1alpha1.MustParseNewIP("10.0.0.1")},
+				},
+			},
+		}
+		Expect(ctrl.SetControllerReference(machine, externalNic, k8sClient.Scheme())).To(Succeed())
+		Expect(k8sClient.Create(ctx, externalNic)).To(Succeed())
+
+		By("asserting the network interface is not being deleted")
+		Consistently(Object(externalNic)).Should(HaveField("DeletionTimestamp", BeNil()))
+	})
 })

internal/controllers/compute/machine_ephemeralvolume_controller.go

@@ -18,6 +18,9 @@ import (
 	"context"
 	"errors"
 	"fmt"
+	"maps"
+
+	"github.com/onmetal/onmetal-api/utils/annotations"
 
 	"github.com/go-logr/logr"
 	computev1alpha1 "github.com/onmetal/onmetal-api/api/compute/v1alpha1"
@@ -33,6 +36,10 @@ import (
 	"sigs.k8s.io/controller-runtime/pkg/predicate"
 )
 
+const (
+	MachineEphemeralVolumeManager = "machine-ephemeral-volume"
+)
+
 type MachineEphemeralVolumeReconciler struct {
 	client.Client
 }
@@ -73,18 +80,19 @@ func (r *MachineEphemeralVolumeReconciler) ephemeralMachineVolumeByName(machine
 				Namespace:   machine.Namespace,
 				Name:        volumeName,
 				Labels:      ephemeral.VolumeTemplate.Labels,
-				Annotations: ephemeral.VolumeTemplate.Annotations,
+				Annotations: maps.Clone(ephemeral.VolumeTemplate.Annotations),
 			},
 			Spec: ephemeral.VolumeTemplate.Spec,
 		}
+		annotations.SetDefaultEphemeralManagedBy(volume)
 		_ = ctrl.SetControllerReference(machine, volume, r.Scheme())
 		res[volumeName] = volume
 	}
 	return res
 }
 
 func (r *MachineEphemeralVolumeReconciler) handleExistingVolume(ctx context.Context, log logr.Logger, machine *computev1alpha1.Machine, shouldManage bool, volume *storagev1alpha1.Volume) error {
-	if metav1.IsControlledBy(volume, machine) {
+	if annotations.IsDefaultEphemeralControlledBy(volume, machine) {
 		if shouldManage {
 			log.V(1).Info("Ephemeral volume is present and controlled by machine")
 			return nil
@@ -103,9 +111,8 @@ func (r *MachineEphemeralVolumeReconciler) handleExistingVolume(ctx context.Cont
 	}
 
 	if shouldManage {
-		return fmt.Errorf("volume %s was not created for machine %s (machine is not owner)", volume.Name, machine.Name)
+		log.V(1).Info("Won't adopt unmanaged volume")
 	}
-	// Volume is not desired but also not controlled by the machine.
 	return nil
 }
 

internal/controllers/compute/machine_ephemeralvolume_controller_test.go

@@ -17,6 +17,7 @@ package compute
 import (
 	computev1alpha1 "github.com/onmetal/onmetal-api/api/compute/v1alpha1"
 	storagev1alpha1 "github.com/onmetal/onmetal-api/api/storage/v1alpha1"
+	"github.com/onmetal/onmetal-api/utils/annotations"
 	. "github.com/onmetal/onmetal-api/utils/testing"
 	. "github.com/onsi/ginkgo/v2"
 	. "github.com/onsi/gomega"
@@ -80,6 +81,7 @@ var _ = Describe("MachineEphemeralVolumeController", func() {
 			},
 			Spec: storagev1alpha1.VolumeSpec{},
 		}
+		annotations.SetDefaultEphemeralManagedBy(undesiredControlledVolume)
 		_ = ctrl.SetControllerReference(machine, undesiredControlledVolume, k8sClient.Scheme())
 		Expect(k8sClient.Create(ctx, undesiredControlledVolume)).To(Succeed())
 
@@ -98,4 +100,32 @@ var _ = Describe("MachineEphemeralVolumeController", func() {
 		By("waiting for the undesired controlled volume to be gone")
 		Eventually(Get(undesiredControlledVolume)).Should(Satisfy(apierrors.IsNotFound))
 	})
+
+	It("should not delete externally managed volumes for a machine", func(ctx SpecContext) {
+		By("creating a machine")
+		machine := &computev1alpha1.Machine{
+			ObjectMeta: metav1.ObjectMeta{
+				Namespace:    ns.Name,
+				GenerateName: "machine-",
+			},
+			Spec: computev1alpha1.MachineSpec{
+				MachineClassRef: corev1.LocalObjectReference{Name: machineClass.Name},
+			},
+		}
+		Expect(k8sClient.Create(ctx, machine)).To(Succeed())
+
+		By("creating an undesired controlled volume")
+		externalVolume := &storagev1alpha1.Volume{
+			ObjectMeta: metav1.ObjectMeta{
+				Namespace:    ns.Name,
+				GenerateName: "external-volume-",
+			},
+			Spec: storagev1alpha1.VolumeSpec{},
+		}
+		_ = ctrl.SetControllerReference(machine, externalVolume, k8sClient.Scheme())
+		Expect(k8sClient.Create(ctx, externalVolume)).To(Succeed())
+
+		By("asserting that the external volume is not being deleted")
+		Consistently(Object(externalVolume)).Should(HaveField("DeletionTimestamp", BeNil()))
+	})
 })

internal/controllers/networking/loadbalancer_ephemeralprefix_controller.go

@@ -18,6 +18,9 @@ import (
 	"context"
 	"errors"
 	"fmt"
+	"maps"
+
+	"github.com/onmetal/onmetal-api/utils/annotations"
 
 	"github.com/go-logr/logr"
 	ipamv1alpha1 "github.com/onmetal/onmetal-api/api/ipam/v1alpha1"
@@ -74,10 +77,11 @@ func (r *LoadBalancerEphemeralPrefixReconciler) ephemeralLoadBalancerPrefixByNam
 				Namespace:   loadBalancer.Namespace,
 				Name:        prefixName,
 				Labels:      ephemeral.PrefixTemplate.Labels,
-				Annotations: ephemeral.PrefixTemplate.Annotations,
+				Annotations: maps.Clone(ephemeral.PrefixTemplate.Annotations),
 			},
 			Spec: ephemeral.PrefixTemplate.Spec,
 		}
+		annotations.SetDefaultEphemeralManagedBy(prefix)
 		_ = ctrl.SetControllerReference(loadBalancer, prefix, r.Scheme())
 		res[prefixName] = prefix
 	}
@@ -86,7 +90,7 @@ func (r *LoadBalancerEphemeralPrefixReconciler) ephemeralLoadBalancerPrefixByNam
 }
 
 func (r *LoadBalancerEphemeralPrefixReconciler) handleExistingPrefix(ctx context.Context, log logr.Logger, loadBalancer *networkingv1alpha1.LoadBalancer, shouldManage bool, prefix *ipamv1alpha1.Prefix) error {
-	if metav1.IsControlledBy(prefix, loadBalancer) {
+	if annotations.IsDefaultEphemeralControlledBy(prefix, loadBalancer) {
 		if shouldManage {
 			log.V(1).Info("Ephemeral prefix is present and controlled by load balancer")
 			return nil
@@ -105,9 +109,8 @@ func (r *LoadBalancerEphemeralPrefixReconciler) handleExistingPrefix(ctx context
 	}
 
 	if shouldManage {
-		return fmt.Errorf("prefix %s was not created for load balancer %s (load balancer is not owner)", prefix.Name, loadBalancer.Name)
+		log.V(1).Info("Won't adopt unmanaged prefix")
 	}
-	// Prefix is not desired but also not controlled by the load balancer.
 	return nil
 }
 

internal/controllers/networking/loadbalancer_ephemeralprefix_controller_test.go

@@ -19,6 +19,7 @@ import (
 	commonv1alpha1 "github.com/onmetal/onmetal-api/api/common/v1alpha1"
 	ipamv1alpha1 "github.com/onmetal/onmetal-api/api/ipam/v1alpha1"
 	networkingv1alpha1 "github.com/onmetal/onmetal-api/api/networking/v1alpha1"
+	"github.com/onmetal/onmetal-api/utils/annotations"
 	. "github.com/onmetal/onmetal-api/utils/testing"
 	. "github.com/onsi/ginkgo/v2"
 	. "github.com/onsi/gomega"
@@ -100,10 +101,44 @@ var _ = Describe("LoadBalancerEphemeralPrefix", func() {
 				Prefix:   commonv1alpha1.MustParseNewIPPrefix("10.0.0.1/32"),
 			},
 		}
+		annotations.SetDefaultEphemeralManagedBy(prefix)
 		Expect(ctrl.SetControllerReference(loadBalancer, prefix, k8sClient.Scheme())).To(Succeed())
 		Expect(k8sClient.Create(ctx, prefix)).To(Succeed())
 
 		By("waiting for the prefix to be marked for deletion")
 		Eventually(Get(prefix)).Should(Satisfy(apierrors.IsNotFound))
 	})
+
+	It("should not delete externally managed prefix for a load balancer", MustPassRepeatedly(10), func(ctx SpecContext) {
+		By("creating a load balancer")
+		loadBalancer := &networkingv1alpha1.LoadBalancer{
+			ObjectMeta: metav1.ObjectMeta{
+				Namespace:    ns.Name,
+				GenerateName: "load-balancer-",
+			},
+			Spec: networkingv1alpha1.LoadBalancerSpec{
+				Type:       networkingv1alpha1.LoadBalancerTypeInternal,
+				NetworkRef: corev1.LocalObjectReference{Name: "my-network"},
+				IPs:        []networkingv1alpha1.IPSource{{Value: commonv1alpha1.MustParseNewIP("10.0.0.1")}},
+			},
+		}
+		Expect(k8sClient.Create(ctx, loadBalancer)).To(Succeed())
+
+		By("creating an undesired prefix")
+		externalPrefix := &ipamv1alpha1.Prefix{
+			ObjectMeta: metav1.ObjectMeta{
+				Namespace:    ns.Name,
+				GenerateName: "external-prefix-",
+			},
+			Spec: ipamv1alpha1.PrefixSpec{
+				IPFamily: corev1.IPv4Protocol,
+				Prefix:   commonv1alpha1.MustParseNewIPPrefix("10.0.0.1/32"),
+			},
+		}
+		Expect(ctrl.SetControllerReference(loadBalancer, externalPrefix, k8sClient.Scheme())).To(Succeed())
+		Expect(k8sClient.Create(ctx, externalPrefix)).To(Succeed())
+
+		By("asserting that the external prefix is not being deleted")
+		Eventually(Object(externalPrefix)).Should(HaveField("DeletionTimestamp", BeNil()))
+	})
 })

internal/controllers/networking/networkinterface_ephemeralprefix_controller.go

@@ -18,6 +18,9 @@ import (
 	"context"
 	"errors"
 	"fmt"
+	"maps"
+
+	"github.com/onmetal/onmetal-api/utils/annotations"
 
 	"github.com/go-logr/logr"
 	ipamv1alpha1 "github.com/onmetal/onmetal-api/api/ipam/v1alpha1"
@@ -74,10 +77,11 @@ func (r *NetworkInterfaceEphemeralPrefixReconciler) ephemeralNetworkInterfacePre
 				Namespace:   nic.Namespace,
 				Name:        prefixName,
 				Labels:      ephemeral.PrefixTemplate.Labels,
-				Annotations: ephemeral.PrefixTemplate.Annotations,
+				Annotations: maps.Clone(ephemeral.PrefixTemplate.Annotations),
 			},
 			Spec: ephemeral.PrefixTemplate.Spec,
 		}
+		annotations.SetDefaultEphemeralManagedBy(prefix)
 		_ = ctrl.SetControllerReference(nic, prefix, r.Scheme())
 		res[prefixName] = prefix
 	}
@@ -106,7 +110,7 @@ func (r *NetworkInterfaceEphemeralPrefixReconciler) ephemeralNetworkInterfacePre
 }
 
 func (r *NetworkInterfaceEphemeralPrefixReconciler) handleExistingPrefix(ctx context.Context, log logr.Logger, nic *networkingv1alpha1.NetworkInterface, shouldManage bool, prefix *ipamv1alpha1.Prefix) error {
-	if metav1.IsControlledBy(prefix, nic) {
+	if annotations.IsDefaultEphemeralControlledBy(prefix, nic) {
 		if shouldManage {
 			log.V(1).Info("Ephemeral prefix is present and controlled by network interface")
 			return nil
@@ -125,9 +129,8 @@ func (r *NetworkInterfaceEphemeralPrefixReconciler) handleExistingPrefix(ctx con
 	}
 
 	if shouldManage {
-		return fmt.Errorf("prefix %s was not created for network interface %s (network interface is not owner)", prefix.Name, nic.Name)
+		log.V(1).Info("Won't adopt unmanaged prefix")
 	}
-	// Prefix is not desired but also not controlled by the networkInterface.
 	return nil
 }