fix(vm,vmip): improve VMIP management

api/core/v1alpha2/virtual_machine_ip_address.go

@@ -80,6 +80,7 @@ type VirtualMachineIPAddressStatus struct {
 type VirtualMachineIPAddressPhase string
 
 const (
-	VirtualMachineIPAddressPhasePending VirtualMachineIPAddressPhase = "Pending"
-	VirtualMachineIPAddressPhaseBound   VirtualMachineIPAddressPhase = "Bound"
+	VirtualMachineIPAddressPhasePending  VirtualMachineIPAddressPhase = "Pending"
+	VirtualMachineIPAddressPhaseBound    VirtualMachineIPAddressPhase = "Bound"
+	VirtualMachineIPAddressPhaseAttached VirtualMachineIPAddressPhase = "Attached"
 )

crds/doc-ru-virtualmachineipaddresses.yaml

@@ -46,6 +46,7 @@ spec:
 
                     * Pending - создание ресурса находится в процессе выполнения.
                     * Bound - ресурс `VirtualMachineIPAddress` привязан к ресурсу `VirtualMachineIPAddressLease`.
+                    * Attached - ресурс `VirtualMachineIPAddress` подключен к ресурсу `VirtualMachine`.
                 virtualMachineName:
                   description: |
                     Имя виртуальной машины, которая в настоящее время использует этот IP-адрес.

crds/virtualmachineipaddresses.yaml

@@ -96,11 +96,13 @@ spec:
                   enum:
                     - "Pending"
                     - "Bound"
+                    - "Attached"
                   description: |
                     Represents the current state of IP address.
 
                     * Pending - the process of creating is in progress.
                     * Bound - the IP address is bound to IP address lease.
+                    * Attached - the IP address is attached to VirtualMachine.
                 observedGeneration:
                   type: integer
                   description: |

images/virtualization-artifact/pkg/controller/common/util.go

@@ -246,7 +246,7 @@ const (
 	// LabelsPrefix is a prefix for virtualization-controller labels.
 	LabelsPrefix = "virtualization.deckhouse.io"
 
-	// LabelVirtualMachineName is a label to link ip address back to virtual machine.
+	// LabelVirtualMachineName is a label to link VirtualMachineOperation to VirtualMachine.
 	LabelVirtualMachineName = LabelsPrefix + "/virtual-machine-name"
 
 	UploaderServiceLabel = "service"

images/virtualization-artifact/pkg/controller/indexer/indexer.go

@@ -34,6 +34,8 @@ const (
 	IndexFieldVMIPLeaseByVMIP = "spec.virtualMachineIPAddressRef.Name"
 
 	IndexFieldVDByVDSnapshot = "spec.DataSource.ObjectRef.Name,.Kind=VirtualDiskSnapshot"
+
+	IndexFieldVMIPByVM = "status.virtualMachine"
 )
 
 type indexFunc func(ctx context.Context, mgr manager.Manager) error
@@ -46,6 +48,7 @@ func IndexALL(ctx context.Context, mgr manager.Manager) error {
 		IndexVMByCVI,
 		IndexVMIPLeaseByVMIP,
 		IndexVDByVDSnapshot,
+		IndexVMIPByVM,
 	} {
 		if err := fn(ctx, mgr); err != nil {
 			return err
@@ -106,3 +109,13 @@ func IndexVMIPLeaseByVMIP(ctx context.Context, mgr manager.Manager) error {
 		return []string{lease.Spec.VirtualMachineIPAddressRef.Name}
 	})
 }
+
+func IndexVMIPByVM(ctx context.Context, mgr manager.Manager) error {
+	return mgr.GetFieldIndexer().IndexField(ctx, &virtv2.VirtualMachineIPAddress{}, IndexFieldVMIPByVM, func(object client.Object) []string {
+		vmip, ok := object.(*virtv2.VirtualMachineIPAddress)
+		if !ok || vmip == nil {
+			return nil
+		}
+		return []string{vmip.Status.VirtualMachine}
+	})
+}

images/virtualization-artifact/pkg/controller/ipam/ipam.go

@@ -19,13 +19,11 @@ package ipam
 import (
 	"context"
 	"errors"
-	"fmt"
 	"strings"
 
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"sigs.k8s.io/controller-runtime/pkg/client"
 
-	"github.com/deckhouse/virtualization-controller/pkg/controller/common"
 	virtv2 "github.com/deckhouse/virtualization/api/core/v1alpha2"
 )
 
@@ -42,7 +40,7 @@ func (m IPAM) IsBound(vmName string, vmip *virtv2.VirtualMachineIPAddress) bool
 		return false
 	}
 
-	if vmip.Status.Phase != virtv2.VirtualMachineIPAddressPhaseBound {
+	if vmip.Status.Phase != virtv2.VirtualMachineIPAddressPhaseBound && vmip.Status.Phase != virtv2.VirtualMachineIPAddressPhaseAttached {
 		return false
 	}
 
@@ -54,25 +52,13 @@ func (m IPAM) CheckIpAddressAvailableForBinding(vmName string, vmip *virtv2.Virt
 		return errors.New("cannot to bind with empty ip address")
 	}
 
-	boundVMName := vmip.Status.VirtualMachine
-	if boundVMName == "" || boundVMName == vmName {
-		return nil
-	}
-
-	return fmt.Errorf(
-		"unable to bind the ip address (%s) to the virtual machine (%s): the ip address has already been linked to another one",
-		vmip.Name,
-		vmName,
-	)
+	return nil
 }
 
 func (m IPAM) CreateIPAddress(ctx context.Context, vm *virtv2.VirtualMachine, client client.Client) error {
 	ownerRef := metav1.NewControllerRef(vm, vm.GroupVersionKind())
 	return client.Create(ctx, &virtv2.VirtualMachineIPAddress{
 		ObjectMeta: metav1.ObjectMeta{
-			Labels: map[string]string{
-				common.LabelVirtualMachineName: vm.Name,
-			},
 			GenerateName:    GenerateName(vm),
 			Namespace:       vm.Namespace,
 			OwnerReferences: []metav1.OwnerReference{*ownerRef},
@@ -99,9 +85,7 @@ func GetVirtualMachineName(vmip *virtv2.VirtualMachineIPAddress) string {
 	if gn := vmip.GenerateName; gn != "" {
 		return strings.TrimSuffix(vmip.GenerateName, generateNameSuffix)
 	}
-	if name := vmip.GetLabels()[common.LabelVirtualMachineName]; name != "" {
-		return name
-	}
+
 	name := vmip.GetName()
 	for _, ow := range vmip.GetOwnerReferences() {
 		if ow.Kind == virtv2.VirtualMachineKind {

images/virtualization-artifact/pkg/controller/vm/internal/ipam.go

@@ -21,7 +21,6 @@ import (
 	"fmt"
 	"log/slog"
 	"strings"
-	"time"
 
 	corev1 "k8s.io/api/core/v1"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
@@ -100,7 +99,9 @@ func (h *IPAMHandler) Handle(ctx context.Context, s state.VirtualMachineState) (
 			Reason(vmcondition.ReasonIPAddressReady).
 			Condition())
 		changed.Status.VirtualMachineIPAddress = ipAddress.GetName()
-		changed.Status.IPAddress = ipAddress.Status.Address
+		if changed.Status.Phase != virtv2.MachineRunning && changed.Status.Phase != virtv2.MachineStopping {
+			changed.Status.IPAddress = ipAddress.Status.Address
+		}
 		kvvmi, err := s.KVVMI(ctx)
 		if err != nil {
 			return reconcile.Result{}, err
@@ -120,7 +121,6 @@ func (h *IPAMHandler) Handle(ctx context.Context, s state.VirtualMachineState) (
 							Reason(vmcondition.ReasonIPAddressNotAssigned).
 							Message(msg).
 							Condition())
-						h.recorder.Event(changed, corev1.EventTypeWarning, vmcondition.ReasonIPAddressNotAssigned.String(), msg)
 						log.Error(msg)
 					}
 					break
@@ -139,13 +139,15 @@ func (h *IPAMHandler) Handle(ctx context.Context, s state.VirtualMachineState) (
 		if current.Spec.VirtualMachineIPAddress != "" {
 			log.Info(fmt.Sprintf("The requested ip address (%s) for the virtual machine not found: waiting for the ip address", current.Spec.VirtualMachineIPAddress))
 			cb.Message(fmt.Sprintf("The requested ip address (%s) for the virtual machine not found: waiting for the ip address", current.Spec.VirtualMachineIPAddress))
-			return reconcile.Result{RequeueAfter: 2 * time.Second}, nil
+			mgr.Update(cb.Condition())
+			changed.Status.Conditions = mgr.Generate()
+			return reconcile.Result{}, nil
 		}
 		log.Info("VirtualMachineIPAddress not found: create the new one", slog.String("vmipName", current.GetName()))
 		cb.Message(fmt.Sprintf("VirtualMachineIPAddress %q not found: it may be in the process of being created", current.GetName()))
 		mgr.Update(cb.Condition())
 		changed.Status.Conditions = mgr.Generate()
-		return reconcile.Result{RequeueAfter: 2 * time.Second}, h.ipam.CreateIPAddress(ctx, changed, h.client)
+		return reconcile.Result{}, h.ipam.CreateIPAddress(ctx, changed, h.client)
 	}
 
 	// 3. Check if possible to bind virtual machine with the found ip address.
@@ -159,13 +161,23 @@ func (h *IPAMHandler) Handle(ctx context.Context, s state.VirtualMachineState) (
 		return reconcile.Result{}, nil
 	}
 
-	// 4. Ip address exists and available for binding with virtual machine: waiting for the ip address.
+	// 4. Ip address exist and attached to another VirtualMachine
+	if ipAddress.Status.VirtualMachine != "" && ipAddress.Status.VirtualMachine != changed.Name {
+		msg := fmt.Sprintf("The requested ip address (%s) attached to VirtualMachine '%s': waiting for the ip address", current.Spec.VirtualMachineIPAddress, ipAddress.Status.VirtualMachine)
+		log.Info(msg)
+		mgr.Update(cb.Reason(vmcondition.ReasonIPAddressNotReady).
+			Message(msg).Condition())
+		changed.Status.Conditions = mgr.Generate()
+		return reconcile.Result{}, nil
+	}
+
+	// 5. Ip address exists and available for binding with virtual machine: waiting for the ip address.
 	log.Info("Waiting for the ip address to be bound to VM", "vmipName", current.Spec.VirtualMachineIPAddress)
 	mgr.Update(cb.Reason(vmcondition.ReasonIPAddressNotReady).
 		Message("Ip address not bound: waiting for the ip address").Condition())
 	changed.Status.Conditions = mgr.Generate()
 
-	return reconcile.Result{RequeueAfter: 2 * time.Second}, nil
+	return reconcile.Result{}, nil
 }
 
 func (h *IPAMHandler) Name() string {

images/virtualization-artifact/pkg/controller/vm/internal/state/state.go

@@ -29,7 +29,7 @@ import (
 	"sigs.k8s.io/controller-runtime/pkg/client"
 
 	kvvmutil "github.com/deckhouse/virtualization-controller/pkg/common/kvvm"
-	"github.com/deckhouse/virtualization-controller/pkg/controller/common"
+	"github.com/deckhouse/virtualization-controller/pkg/controller/indexer"
 	"github.com/deckhouse/virtualization-controller/pkg/controller/powerstate"
 	"github.com/deckhouse/virtualization-controller/pkg/controller/service"
 	"github.com/deckhouse/virtualization-controller/pkg/sdk/framework/helper"
@@ -303,10 +303,13 @@ func (s *state) IPAddress(ctx context.Context) (*virtv2.VirtualMachineIPAddress,
 	vmipName := s.vm.Current().Spec.VirtualMachineIPAddress
 	if vmipName == "" {
 		vmipList := &virtv2.VirtualMachineIPAddressList{}
-		err := s.client.List(ctx, vmipList, &client.ListOptions{
-			Namespace:     s.vm.Current().GetNamespace(),
-			LabelSelector: labels.SelectorFromSet(map[string]string{common.LabelVirtualMachineName: s.vm.Current().GetName()}),
-		})
+
+		err := s.client.List(ctx, vmipList,
+			client.InNamespace(s.vm.Current().GetNamespace()),
+			&client.MatchingFields{
+				indexer.IndexFieldVMIPByVM: s.vm.Current().GetName(),
+			},
+		)
 		if err != nil {
 			return nil, fmt.Errorf("failed to list VirtualMachineIPAddress: %w", err)
 		}

images/virtualization-artifact/pkg/controller/vm/internal/validators/ipam_validator.go

@@ -69,7 +69,7 @@ func (v *IPAMValidator) ValidateUpdate(ctx context.Context, oldVM, newVM *v1alph
 	}
 
 	if newVM.Spec.VirtualMachineIPAddress == "" {
-		return nil, fmt.Errorf("spec.virtualMachineIPAddress cannot be changed to an empty value once set")
+		return nil, nil
 	}
 
 	vmipKey := types.NamespacedName{Name: newVM.Spec.VirtualMachineIPAddress, Namespace: newVM.Namespace}

images/virtualization-artifact/pkg/controller/vmchange/comparators.go

@@ -58,7 +58,7 @@ func compareVirtualMachineIPAddress(current, desired *v1alpha2.VirtualMachineSpe
 		current.VirtualMachineIPAddress,
 		desired.VirtualMachineIPAddress,
 		"",
-		ActionNone,
+		ActionRestart,
 	)
 }
 

images/virtualization-artifact/pkg/controller/vmip/internal/lifecycle_handler.go

@@ -66,11 +66,6 @@ func (h *LifecycleHandler) Handle(ctx context.Context, state state.VMIPState) (r
 			Reason(vmipcondition.VirtualMachineNotFound).
 			Message("Virtual machine not found").
 			Condition())
-	} else {
-		vmipStatus.VirtualMachine = vm.Name
-		mgr.Update(conditionAttach.Status(metav1.ConditionTrue).
-			Reason(vmipcondition.Attached).
-			Condition())
 	}
 
 	lease, err := state.VirtualMachineIPLease(ctx)
@@ -99,6 +94,15 @@ func (h *LifecycleHandler) Handle(ctx context.Context, state state.VMIPState) (r
 				Condition())
 		}
 
+	case vm != nil && vm.GetDeletionTimestamp().IsZero():
+		if vmipStatus.Phase != virtv2.VirtualMachineIPAddressPhaseAttached {
+			vmipStatus.Phase = virtv2.VirtualMachineIPAddressPhaseAttached
+			vmipStatus.VirtualMachine = vm.Name
+			mgr.Update(conditionAttach.Status(metav1.ConditionTrue).
+				Reason(vmipcondition.Attached).
+				Condition())
+		}
+
 	case util.IsBoundLease(lease, vmip):
 		if vmipStatus.Phase != virtv2.VirtualMachineIPAddressPhaseBound {
 			vmipStatus.Phase = virtv2.VirtualMachineIPAddressPhaseBound

images/virtualization-artifact/pkg/controller/vmip/internal/protection_handler.go

@@ -19,6 +19,7 @@ package internal
 import (
 	"context"
 
+	"sigs.k8s.io/controller-runtime/pkg/client"
 	"sigs.k8s.io/controller-runtime/pkg/controller/controllerutil"
 	"sigs.k8s.io/controller-runtime/pkg/reconcile"
 
@@ -29,10 +30,14 @@ import (
 
 const ProtectionHandlerName = "ProtectionHandler"
 
-type ProtectionHandler struct{}
+type ProtectionHandler struct {
+	client client.Client
+}
 
-func NewProtectionHandler() *ProtectionHandler {
-	return &ProtectionHandler{}
+func NewProtectionHandler(client client.Client) *ProtectionHandler {
+	return &ProtectionHandler{
+		client: client,
+	}
 }
 
 func (h *ProtectionHandler) Handle(ctx context.Context, state state.VMIPState) (reconcile.Result, error) {
@@ -45,6 +50,22 @@ func (h *ProtectionHandler) Handle(ctx context.Context, state state.VMIPState) (
 		return reconcile.Result{}, err
 	}
 
+	attachedVMs, err := h.getAttachedVM(ctx, vmip)
+	if err != nil {
+		return reconcile.Result{}, err
+	}
+
+	switch {
+	case len(attachedVMs) == 0:
+		log.Debug("Allow VirtualMachineIPAddress deletion")
+		controllerutil.RemoveFinalizer(vmip, virtv2.FinalizerIPAddressProtection)
+	case vmip.DeletionTimestamp == nil:
+		log.Debug("Protect VirtualMachineIPAddress from deletion")
+		controllerutil.AddFinalizer(vmip, virtv2.FinalizerIPAddressProtection)
+	default:
+		log.Debug("VirtualMachineIPAddress deletion is delayed: it's protected by virtual machines")
+	}
+
 	if vm == nil || vm.DeletionTimestamp != nil {
 		log.Info("VirtualMachineIP is no longer attached to any VM, proceeding with detachment", "VirtualMachineIPName", vmip.Name)
 		controllerutil.RemoveFinalizer(vmip, virtv2.FinalizerIPAddressCleanup)
@@ -59,3 +80,22 @@ func (h *ProtectionHandler) Handle(ctx context.Context, state state.VMIPState) (
 func (h *ProtectionHandler) Name() string {
 	return ProtectionHandlerName
 }
+
+func (h *ProtectionHandler) getAttachedVM(ctx context.Context, vmip *virtv2.VirtualMachineIPAddress) ([]virtv2.VirtualMachine, error) {
+	var vms virtv2.VirtualMachineList
+	err := h.client.List(ctx, &vms, &client.ListOptions{
+		Namespace: vmip.Namespace,
+	})
+	if err != nil {
+		return nil, err
+	}
+
+	var attachedVMs []virtv2.VirtualMachine
+	for _, vm := range vms.Items {
+		if vm.Spec.VirtualMachineIPAddress == vmip.Name || vm.Status.VirtualMachineIPAddress == vmip.Name {
+			attachedVMs = append(attachedVMs, vm)
+		}
+	}
+
+	return attachedVMs, nil
+}

images/virtualization-artifact/pkg/controller/vmip/internal/state/state.go

@@ -113,10 +113,18 @@ func (s *state) VirtualMachine(ctx context.Context) (*virtv2.VirtualMachine, err
 	var err error
 	if s.vmip.Status.VirtualMachine != "" {
 		vmKey := types.NamespacedName{Name: s.vmip.Status.VirtualMachine, Namespace: s.vmip.Namespace}
-		s.vm, err = helper.FetchObject(ctx, vmKey, s.client, &virtv2.VirtualMachine{})
+		vm, err := helper.FetchObject(ctx, vmKey, s.client, &virtv2.VirtualMachine{})
 		if err != nil {
 			return nil, fmt.Errorf("unable to get VM %s: %w", vmKey, err)
 		}
+
+		if vm == nil {
+			return s.vm, nil
+		}
+
+		if vm.Status.VirtualMachineIPAddress == s.vmip.Name && vm.Status.IPAddress == s.vmip.Status.Address {
+			s.vm = vm
+		}
 	}
 
 	if s.vm == nil {
@@ -129,8 +137,7 @@ func (s *state) VirtualMachine(ctx context.Context) (*virtv2.VirtualMachine, err
 		}
 
 		for i, vm := range vms.Items {
-			if vm.Spec.VirtualMachineIPAddress == s.vmip.Name ||
-				vm.Spec.VirtualMachineIPAddress == "" && vm.Name == ipam.GetVirtualMachineName(s.vmip) {
+			if vm.Spec.VirtualMachineIPAddress == s.vmip.Name || vm.Spec.VirtualMachineIPAddress == "" && vm.Name == ipam.GetVirtualMachineName(s.vmip) {
 				s.vm = &vms.Items[i]
 				break
 			}

images/virtualization-artifact/pkg/controller/vmip/vmip_controller.go

@@ -47,7 +47,7 @@ func NewController(
 	ipService := service.NewIpAddressService(log, virtualMachineCIDRs)
 
 	handlers := []Handler{
-		internal.NewProtectionHandler(),
+		internal.NewProtectionHandler(mgr.GetClient()),
 		internal.NewIPLeaseHandler(mgr.GetClient(), ipService, recorder),
 		internal.NewLifecycleHandler(),
 	}