WIP: vmservice/ip_test.go: correctly requeue machine when creating ipclaim

Author: 65278

internal/service/vmservice/ip.go

@@ -48,9 +48,12 @@ func reconcileIPAddresses(ctx context.Context, machineScope *scope.MachineScope)
 	netPoolAddresses := make(map[string]map[string][]string)
 
 	if machineScope.ProxmoxMachine.Spec.Network != nil {
-		// fmt.Println( handleDevices(ctx, machineScope, addresses))
 		if requeue, err = handleDevices(ctx, machineScope, netPoolAddresses); err != nil || requeue {
-			return true, errors.Wrap(err, "unable to handle network devices")
+			if err == nil {
+				return true, errors.Wrap(err, "requeuing network reconcillation")
+			} else {
+				return true, errors.Wrap(err, "unable to handle network devices")
+			}
 		}
 	}
 
@@ -82,8 +85,8 @@ func formatIPAddressName(name, device string) string {
 	return fmt.Sprintf("%s-%s", name, device)
 }
 
-// findIPAddressV2 takes the likely name of an ipaddress and returns an ipamv1.IPAddress object or nil
-func findIPAddressV2(ctx context.Context, poolRef *corev1.TypedLocalObjectReference, machineScope *scope.MachineScope) ([]ipamv1.IPAddress, error) {
+// findIPAddress returns all IPAddresses owned by a pool and a machine
+func findIPAddress(ctx context.Context, poolRef *corev1.TypedLocalObjectReference, machineScope *scope.MachineScope) ([]ipamv1.IPAddress, error) {
 	return machineScope.IPAMHelper.GetIPAddressV2(ctx, *poolRef, machineScope.ProxmoxMachine)
 }
 
@@ -121,24 +124,35 @@ func findIPAddressGatewayMetric(ctx context.Context, machineScope *scope.Machine
 }
 
 func machineHasIPAddress(machine *infrav1.ProxmoxMachine) bool {
-	// TODO: does this work?
-	return machine.Status.IPAddresses[infrav1.DefaultNetworkDevice] != nil
+	// Every machine needs to have at least one IPv4 or IPv6 host network address
+	if machine.Status.IPAddresses[infrav1.DefaultNetworkDevice] == nil {
+		return false
+	} else {
+		return len(machine.Status.IPAddresses[infrav1.DefaultNetworkDevice].IPV4) > 0 ||
+			len(machine.Status.IPAddresses[infrav1.DefaultNetworkDevice].IPV6) > 0
+	}
 }
 
-func handleIPAddress(ctx context.Context, machineScope *scope.MachineScope, dev *string, poolNum int, ipamRef *corev1.TypedLocalObjectReference) ([]string, error) {
+func handleIPAddresses(ctx context.Context, machineScope *scope.MachineScope, dev *string, poolNum int, poolRef *corev1.TypedLocalObjectReference) ([]string, error) {
 	device := ptr.Deref(dev, infrav1.DefaultNetworkDevice)
 
-	ipAddresses, err := findIPAddressV2(ctx, ipamRef, machineScope)
+	ipAddresses, err := findIPAddress(ctx, poolRef, machineScope)
 	if err != nil {
+		// Technically this error can not occure, as fieldselectors just return empty lists
 		if !apierrors.IsNotFound(err) {
 			return []string{}, err
 		}
+	}
+
+	if len(ipAddresses) == 0 {
 		machineScope.Logger.V(4).Info("IPAddress not found, creating it.", "device", device)
 		// IpAddress not yet created.
-		err = machineScope.IPAMHelper.CreateIPAddressClaimV2(ctx, machineScope.ProxmoxMachine, device, poolNum, machineScope.InfraCluster.Cluster.GetName(), ipamRef)
+		err = machineScope.IPAMHelper.CreateIPAddressClaimV2(ctx, machineScope.ProxmoxMachine, device, poolNum, machineScope.InfraCluster.Cluster.GetName(), poolRef)
 		if err != nil {
 			return []string{}, errors.Wrapf(err, "unable to create Ip address claim for machine %s", machineScope.Name())
 		}
+
+		// send the machine to requeue so ipaddress can be created
 		return []string{}, nil
 	}
 
@@ -160,6 +174,8 @@ func handleIPAddress(ctx context.Context, machineScope *scope.MachineScope, dev
 				return []string{}, errors.Wrapf(err, "unable to add Ip tag to VirtualMachine %s", machineScope.Name())
 			}
 			machineScope.ProxmoxMachine.Status.TaskRef = ptr.To(string(t.UPID))
+
+			// send the machine to requeue so promoxclient can execute
 			return []string{}, nil
 		}
 	}
@@ -170,7 +186,13 @@ func handleIPAddress(ctx context.Context, machineScope *scope.MachineScope, dev
 func handleDevices(ctx context.Context, machineScope *scope.MachineScope, addresses map[string]map[string][]string) (bool, error) {
 	for _, net := range machineScope.ProxmoxMachine.Spec.Network.NetworkDevices {
 		for i, ipPool := range net.InterfaceConfig.IPPoolRef {
-			ipAddresses, err := handleIPAddress(ctx, machineScope, net.Name, i, &ipPool)
+			ipAddresses, err := handleIPAddresses(ctx, machineScope, net.Name, i, &ipPool)
+
+			// requeue machine if tag or ipaddress need creation
+			if len(ipAddresses) == 0 {
+				return true, nil
+			}
+
 			for _, ip := range ipAddresses {
 				if err != nil || ip == "" {
 					fmt.Println("handleDevices", "err", err, "ip", ip)

internal/service/vmservice/ip_test.go

@@ -42,11 +42,12 @@ func TestReconcileIPAddresses_CreateAdditionalClaim(t *testing.T) {
 	machineScope, _, kubeClient := setupReconcilerTest(t)
 
 	defaultPool := corev1.TypedLocalObjectReference{APIGroup: ptr.To("ipam.cluster.x-k8s.io"), Kind: "InClusterIPPool", Name: "custom"}
+	extraPool0 := corev1.TypedLocalObjectReference{APIGroup: ptr.To("ipam.cluster.x-k8s.io"), Kind: "GlobalInClusterIPPool", Name: "additional0"}
 
 	machineScope.ProxmoxMachine.Spec.Network = &infrav1.NetworkSpec{
 		NetworkDevices: []infrav1.NetworkDevice{
 			{Name: ptr.To("net0"), InterfaceConfig: infrav1.InterfaceConfig{IPPoolRef: []corev1.TypedLocalObjectReference{defaultPool}}},
-			{Name: ptr.To("net1"), InterfaceConfig: infrav1.InterfaceConfig{}},
+			{Name: ptr.To("net1"), InterfaceConfig: infrav1.InterfaceConfig{IPPoolRef: []corev1.TypedLocalObjectReference{extraPool0}}},
 		},
 	}
 
@@ -58,27 +59,43 @@ func TestReconcileIPAddresses_CreateAdditionalClaim(t *testing.T) {
 	createIP4AddressResource(t, kubeClient, machineScope, infrav1.DefaultNetworkDevice, "10.10.10.10", &defaultPool)
 
 	requeue, err := reconcileIPAddresses(context.Background(), machineScope)
-	require.NotNil(t, machineScope.ProxmoxMachine.Status.IPAddresses["net0"])
-	require.Equal(t, *(machineScope.ProxmoxMachine.Status.IPAddresses["net0"]), infrav1.IPAddresses{IPV4: []string{"10.10.10.10"}, IPV6: nil})
+
+	// Since an IPAddress for extraPool0 still needs to be created, the machine should
+	// requeue without error.
 	require.NoError(t, err)
 	require.True(t, requeue)
+
+	// net0 should not exist yet
+	require.Nil(t, machineScope.ProxmoxMachine.Status.IPAddresses["net0"])
+
 	requireConditionIsFalse(t, machineScope.ProxmoxMachine, infrav1.VMProvisionedCondition)
 }
 
-/*func TestReconcileIPAddresses_AddIPTag(t *testing.T) {
+func TestReconcileIPAddresses_AddIPTag(t *testing.T) {
 	machineScope, proxmoxClient, kubeClient := setupReconcilerTest(t)
+
+	defaultPool := corev1.TypedLocalObjectReference{APIGroup: ptr.To("ipam.cluster.x-k8s.io"), Kind: "InClusterIPPool", Name: "custom"}
+	machineScope.ProxmoxMachine.Spec.Network = &infrav1.NetworkSpec{
+		NetworkDevices: []infrav1.NetworkDevice{
+			{Name: ptr.To("net0"), InterfaceConfig: infrav1.InterfaceConfig{IPPoolRef: []corev1.TypedLocalObjectReference{defaultPool}}},
+		},
+	}
+
 	vm := newStoppedVM()
 	task := newTask()
 	machineScope.SetVirtualMachine(vm)
-	createIP4AddressResource(t, kubeClient, machineScope, infrav1.DefaultNetworkDevice, "10.10.10.10")
+
+	createIPPools(t, kubeClient, machineScope)
+	createIP4AddressResource(t, kubeClient, machineScope, infrav1.DefaultNetworkDevice, "10.10.10.10", &defaultPool)
 
 	proxmoxClient.EXPECT().TagVM(context.Background(), vm, ipTag).Return(task, nil).Once()
 
 	requeue, err := reconcileIPAddresses(context.Background(), machineScope)
 	require.NoError(t, err)
 	require.True(t, requeue)
+	require.Equal(t, ipTag, machineScope.VirtualMachine.VirtualMachineConfig.Tags)
 	requireConditionIsFalse(t, machineScope.ProxmoxMachine, infrav1.VMProvisionedCondition)
-}*/
+}
 
 func TestReconcileIPAddresses_SetIPAddresses(t *testing.T) {
 	machineScope, _, kubeClient := setupReconcilerTest(t)