diff --git a/images/virtualization-artifact/cmd/virtualization-controller/main.go b/images/virtualization-artifact/cmd/virtualization-controller/main.go index 4e8ffa798a..2c86e84209 100644 --- a/images/virtualization-artifact/cmd/virtualization-controller/main.go +++ b/images/virtualization-artifact/cmd/virtualization-controller/main.go @@ -300,7 +300,7 @@ func main() { } vmipLogger := logger.NewControllerLogger(vmip.ControllerName, logLevel, logOutput, logDebugVerbosity, logDebugControllerList) - if _, err = vmip.NewController(ctx, mgr, vmipLogger, virtualMachineCIDRs); err != nil { + if _, err = vmip.NewController(ctx, mgr, virtClient, vmipLogger, virtualMachineCIDRs); err != nil { log.Error(err.Error()) os.Exit(1) } diff --git a/images/virtualization-artifact/pkg/common/annotations/annotations.go b/images/virtualization-artifact/pkg/common/annotations/annotations.go index 6c5998908f..2be408717d 100644 --- a/images/virtualization-artifact/pkg/common/annotations/annotations.go +++ b/images/virtualization-artifact/pkg/common/annotations/annotations.go @@ -97,6 +97,9 @@ const ( // LabelVirtualMachineUID is a label to link VirtualMachineIPAddress to VirtualMachine. LabelVirtualMachineUID = LabelsPrefix + "/virtual-machine-uid" + // LabelVirtualMachineIPAddressUID is a label to link VirtualMachineIPAddressLease to VirtualMachineIPAddress. + LabelVirtualMachineIPAddressUID = LabelsPrefix + "/virtual-machine-ip-address-uid" + UploaderServiceLabel = "service" // AppKubernetesManagedByLabel is the Kubernetes recommended managed-by label. diff --git a/images/virtualization-artifact/pkg/controller/indexer/indexer.go b/images/virtualization-artifact/pkg/controller/indexer/indexer.go index 7122f105a1..f3421a24a8 100644 --- a/images/virtualization-artifact/pkg/controller/indexer/indexer.go +++ b/images/virtualization-artifact/pkg/controller/indexer/indexer.go @@ -36,8 +36,6 @@ const ( IndexFieldVMByCVI = "spec.blockDeviceRefs.ClusterVirtualImage" IndexFieldVMByNode = "status.node" - IndexFieldVMIPLeaseByVMIP = "spec.virtualMachineIPAddressRef.Name" - IndexFieldVDByVDSnapshot = "vd,spec.DataSource.ObjectRef.Name,.Kind=VirtualDiskSnapshot" IndexFieldVIByVDSnapshot = "vi,spec.DataSource.ObjectRef.Name,.Kind=VirtualDiskSnapshot" IndexFieldCVIByVDSnapshot = "cvi,spec.DataSource.ObjectRef.Name,.Kind=VirtualDiskSnapshot" @@ -62,7 +60,6 @@ var IndexGetters = []IndexGetter{ IndexVMByVI, IndexVMByCVI, IndexVMByNode, - IndexVMIPLeaseByVMIP, IndexVMSnapshotByVM, IndexVMSnapshotByVDSnapshot, IndexVMRestoreByVMSnapshot, @@ -140,13 +137,3 @@ func getBlockDeviceNamesByKind(obj client.Object, kind virtv2.BlockDeviceKind) [ } return res } - -func IndexVMIPLeaseByVMIP() (obj client.Object, field string, extractValue client.IndexerFunc) { - return &virtv2.VirtualMachineIPAddressLease{}, IndexFieldVMIPLeaseByVMIP, func(object client.Object) []string { - lease, ok := object.(*virtv2.VirtualMachineIPAddressLease) - if !ok || lease == nil { - return nil - } - return []string{lease.Spec.VirtualMachineIPAddressRef.Name} - } -} diff --git a/images/virtualization-artifact/pkg/controller/service/ip_address_service.go b/images/virtualization-artifact/pkg/controller/service/ip_address_service.go index 6c608bd44f..deccc1cdaf 100644 --- a/images/virtualization-artifact/pkg/controller/service/ip_address_service.go +++ b/images/virtualization-artifact/pkg/controller/service/ip_address_service.go @@ -25,6 +25,7 @@ import ( k8snet "k8s.io/utils/net" "github.com/deckhouse/deckhouse/pkg/log" + "github.com/deckhouse/virtualization-controller/pkg/common/ip" ) @@ -94,6 +95,7 @@ func (s IpAddressService) AllocateNewIP(allocatedIPs ip.AllocatedIPs) (string, e } if _, ok := allocatedIPs[ip.String()]; !ok { + return ip.String(), nil } } diff --git a/images/virtualization-artifact/pkg/controller/vmip/internal/iplease_handler.go b/images/virtualization-artifact/pkg/controller/vmip/internal/iplease_handler.go index 992add325a..4d611cba96 100644 --- a/images/virtualization-artifact/pkg/controller/vmip/internal/iplease_handler.go +++ b/images/virtualization-artifact/pkg/controller/vmip/internal/iplease_handler.go @@ -20,12 +20,16 @@ import ( "context" "errors" "fmt" + "log/slog" + "time" corev1 "k8s.io/api/core/v1" + k8serrors "k8s.io/apimachinery/pkg/api/errors" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "sigs.k8s.io/controller-runtime/pkg/client" "sigs.k8s.io/controller-runtime/pkg/reconcile" + "github.com/deckhouse/virtualization-controller/pkg/common/annotations" "github.com/deckhouse/virtualization-controller/pkg/common/ip" "github.com/deckhouse/virtualization-controller/pkg/controller/conditions" "github.com/deckhouse/virtualization-controller/pkg/controller/service" @@ -56,20 +60,9 @@ func NewIPLeaseHandler(client client.Client, ipAddressService *service.IpAddress func (h IPLeaseHandler) Handle(ctx context.Context, state state.VMIPState) (reconcile.Result, error) { log, ctx := logger.GetHandlerContext(ctx, IpLeaseHandlerName) - vmip := state.VirtualMachineIP() - vmipStatus := &vmip.Status - - lease, err := state.VirtualMachineIPLease(ctx) - if err != nil { - return reconcile.Result{}, err - } - condition, _ := conditions.GetCondition(vmipcondition.BoundType, vmipStatus.Conditions) + vmip, lease := state.VirtualMachineIP(), state.VirtualMachineIPLease() switch { - case lease == nil && vmipStatus.Address != "" && condition.Reason != vmipcondition.VirtualMachineIPAddressLeaseAlreadyExists.String(): - log.Info("Lease by name not found: waiting for the lease to be available") - return reconcile.Result{}, nil - case lease == nil: log.Info("No Lease for VirtualMachineIP: create the new one", "type", vmip.Spec.Type, "address", vmip.Spec.StaticIP) return h.createNewLease(ctx, state) @@ -78,8 +71,12 @@ func (h IPLeaseHandler) Handle(ctx context.Context, state state.VMIPState) (reco log.Info("Lease is not ready: waiting for the lease") return reconcile.Result{}, nil + case vmip.Status.Address == "": + vmip.Status.Address = ip.LeaseNameToIP(lease.Name) + return reconcile.Result{}, nil + case util.IsBoundLease(lease, vmip): - log.Info("Lease already exists, VirtualMachineIP ref is valid") + log.Info("Lease is bound, VirtualMachineIP ref is valid") return reconcile.Result{}, nil case lease.Status.Phase == virtv2.VirtualMachineIPAddressLeasePhaseBound: @@ -87,87 +84,85 @@ func (h IPLeaseHandler) Handle(ctx context.Context, state state.VMIPState) (reco return reconcile.Result{}, nil default: - log.Info("Lease is released: set binding") - - if lease.Spec.VirtualMachineIPAddressRef.Namespace != vmip.Namespace { - log.Warn(fmt.Sprintf("The VirtualMachineIPLease belongs to a different namespace: %s", lease.Spec.VirtualMachineIPAddressRef.Namespace)) - h.recorder.Event(vmip, corev1.EventTypeWarning, vmipcondition.VirtualMachineIPAddressLeaseAlreadyExists.String(), "The VirtualMachineIPLease belongs to a different namespace") - - return reconcile.Result{}, nil - } + return h.updateLease(ctx, lease, vmip, log) + } +} - lease.Spec.VirtualMachineIPAddressRef = &virtv2.VirtualMachineIPAddressLeaseIpAddressRef{ - Name: vmip.Name, - Namespace: vmip.Namespace, - } +func (h IPLeaseHandler) updateLease(ctx context.Context, lease *virtv2.VirtualMachineIPAddressLease, vmip *virtv2.VirtualMachineIPAddress, log *slog.Logger) (reconcile.Result, error) { + log.Info("Lease is released: set binding") - err := h.client.Update(ctx, lease) - if err != nil { - return reconcile.Result{}, err - } + if lease.Spec.VirtualMachineIPAddressRef.Namespace != vmip.Namespace { + log.Warn(fmt.Sprintf("The VirtualMachineIPLease belongs to a different namespace: %s", lease.Spec.VirtualMachineIPAddressRef.Namespace)) + h.recorder.Event(vmip, corev1.EventTypeWarning, vmipcondition.VirtualMachineIPAddressLeaseAlreadyExists.String(), "The VirtualMachineIPLease belongs to a different namespace") - vmipStatus.Address = ip.LeaseNameToIP(lease.Name) return reconcile.Result{}, nil } + + lease.Spec.VirtualMachineIPAddressRef = &virtv2.VirtualMachineIPAddressLeaseIpAddressRef{ + Name: vmip.Name, + Namespace: vmip.Namespace, + } + annotations.AddLabel(lease, annotations.LabelVirtualMachineIPAddressUID, string(vmip.GetUID())) + + return reconcile.Result{}, h.client.Update(ctx, lease) } func (h IPLeaseHandler) createNewLease(ctx context.Context, state state.VMIPState) (reconcile.Result, error) { log := logger.FromContext(ctx) vmip := state.VirtualMachineIP() - vmipStatus := &vmip.Status - + ipAddress := "" if vmip.Spec.Type == virtv2.VirtualMachineIPAddressTypeAuto { log.Info("allocate the new VirtualMachineIP address") var err error - vmipStatus.Address, err = h.ipService.AllocateNewIP(state.AllocatedIPs()) + ipAddress, err = h.ipService.AllocateNewIP(state.AllocatedIPs()) if err != nil { return reconcile.Result{}, err } } else { - vmipStatus.Address = vmip.Spec.StaticIP + ipAddress = vmip.Spec.StaticIP } - err := h.ipService.IsAvailableAddress(vmipStatus.Address, state.AllocatedIPs()) + err := h.ipService.IsAvailableAddress(ipAddress, state.AllocatedIPs()) if err != nil { - vmipStatus.Address = "" msg := fmt.Sprintf("the VirtualMachineIP cannot be created: %s", err.Error()) - log.Info(msg) + log.Warn(msg) conditionBound := conditions.NewConditionBuilder(vmipcondition.BoundType). Generation(vmip.GetGeneration()) switch { case errors.Is(err, service.ErrIPAddressOutOfRange): - vmipStatus.Phase = virtv2.VirtualMachineIPAddressPhasePending - msg := fmt.Sprintf("The requested address %s is out of the valid range.", vmip.Spec.StaticIP) + vmip.Status.Phase = virtv2.VirtualMachineIPAddressPhasePending + msg = fmt.Sprintf("The requested address %s is out of the valid range.", vmip.Spec.StaticIP) conditionBound.Status(metav1.ConditionFalse). Reason(vmipcondition.VirtualMachineIPAddressIsOutOfTheValidRange). Message(msg) h.recorder.Event(vmip, corev1.EventTypeWarning, virtv2.ReasonFailed, msg) case errors.Is(err, service.ErrIPAddressAlreadyExist): - vmipStatus.Phase = virtv2.VirtualMachineIPAddressPhasePending - msg := fmt.Sprintf("VirtualMachineIPAddressLease %s is bound to another VirtualMachineIPAddress.", - ip.IpToLeaseName(vmipStatus.Address)) + vmip.Status.Phase = virtv2.VirtualMachineIPAddressPhasePending + msg = fmt.Sprintf("VirtualMachineIPAddressLease %s is bound to another VirtualMachineIPAddress.", + ip.IpToLeaseName(ipAddress)) conditionBound.Status(metav1.ConditionFalse). Reason(vmipcondition.VirtualMachineIPAddressLeaseAlreadyExists). Message(msg) h.recorder.Event(vmip, corev1.EventTypeWarning, virtv2.ReasonBound, msg) } - conditions.SetCondition(conditionBound, &vmipStatus.Conditions) + conditions.SetCondition(conditionBound, &vmip.Status.Conditions) return reconcile.Result{}, nil } - leaseName := ip.IpToLeaseName(vmipStatus.Address) + leaseName := ip.IpToLeaseName(ipAddress) - log.Info("Create lease", - "leaseName", leaseName, - "refName", vmip.Name, - "refNamespace", vmip.Namespace, + log.Info("Create lease", "leaseName", leaseName, + "refName", vmip.Name, "refNamespace", vmip.Namespace, ) err = h.client.Create(ctx, &virtv2.VirtualMachineIPAddressLease{ ObjectMeta: metav1.ObjectMeta{ + Labels: map[string]string{ + annotations.LabelVirtualMachineIPAddressUID: string(vmip.GetUID()), + }, Name: leaseName, }, Spec: virtv2.VirtualMachineIPAddressLeaseSpec{ @@ -178,6 +173,11 @@ func (h IPLeaseHandler) createNewLease(ctx context.Context, state state.VMIPStat }, }) if err != nil { + if k8serrors.IsAlreadyExists(err) { + log.Warn("Lease already exists: requeue 2s") + return reconcile.Result{RequeueAfter: 2 * time.Second}, nil + } + return reconcile.Result{}, err } diff --git a/images/virtualization-artifact/pkg/controller/vmip/internal/lifecycle_handler.go b/images/virtualization-artifact/pkg/controller/vmip/internal/lifecycle_handler.go index 50ff935621..750dc1f066 100644 --- a/images/virtualization-artifact/pkg/controller/vmip/internal/lifecycle_handler.go +++ b/images/virtualization-artifact/pkg/controller/vmip/internal/lifecycle_handler.go @@ -19,6 +19,7 @@ package internal import ( "context" "fmt" + "log/slog" "time" corev1 "k8s.io/api/core/v1" @@ -51,12 +52,7 @@ func (h *LifecycleHandler) Handle(ctx context.Context, state state.VMIPState) (r log := logger.FromContext(ctx).With(logger.SlogHandler(LifecycleHandlerName)) vmip := state.VirtualMachineIP() - vmipStatus := &vmip.Status - - vm, err := state.VirtualMachine(ctx) - if err != nil { - return reconcile.Result{}, err - } + vm := state.VirtualMachine() conditionBound := conditions.NewConditionBuilder(vmipcondition.BoundType). Generation(vmip.GetGeneration()). @@ -69,54 +65,76 @@ func (h *LifecycleHandler) Handle(ctx context.Context, state state.VMIPState) (r Status(metav1.ConditionUnknown) defer func() { - conditions.SetCondition(conditionBound, &vmipStatus.Conditions) - conditions.SetCondition(conditionAttach, &vmipStatus.Conditions) + conditions.SetCondition(conditionBound, &vmip.Status.Conditions) + conditions.SetCondition(conditionAttach, &vmip.Status.Conditions) }() + lease := state.VirtualMachineIPLease() + + needRequeue := false + switch { + case lease == nil && vmip.Status.Address != "": + h.handleLostLease(vmip, conditionBound) + case lease == nil: + h.handleNotFoundLease(vmip, conditionBound) + default: + needRequeue = h.handleBoundLease(lease, vmip, log, conditionBound) + h.handleAttachedLease(vmip, vm, conditionAttach) + } + + log.Debug("Set VirtualMachineIP phase", "phase", vmip.Status.Phase) + vmip.Status.ObservedGeneration = vmip.GetGeneration() + if !needRequeue { + return reconcile.Result{}, nil + } else { + // TODO add requeue with with exponential BackOff time interval using condition Bound -> probeTime + return reconcile.Result{RequeueAfter: 30 * time.Second}, nil + } +} + +func (h *LifecycleHandler) handleNotFoundLease(vmip *virtv2.VirtualMachineIPAddress, conditionBound *conditions.ConditionBuilder) { + vmip.Status.Phase = virtv2.VirtualMachineIPAddressPhasePending + conditionBound.Status(metav1.ConditionFalse). + Reason(vmipcondition.VirtualMachineIPAddressLeaseNotFound). + Message("VirtualMachineIPAddressLease is not found") +} + +func (h *LifecycleHandler) handleLostLease(vmip *virtv2.VirtualMachineIPAddress, conditionBound *conditions.ConditionBuilder) { + vmip.Status.Phase = virtv2.VirtualMachineIPAddressPhasePending + conditionBound.Status(metav1.ConditionFalse). + Reason(vmipcondition.VirtualMachineIPAddressLeaseLost). + Message(fmt.Sprintf("VirtualMachineIPAddressLease %s doesn't exist", + ip.IpToLeaseName(vmip.Status.Address))) +} + +func (h *LifecycleHandler) handleAttachedLease(vmip *virtv2.VirtualMachineIPAddress, vm *virtv2.VirtualMachine, conditionAttach *conditions.ConditionBuilder) { if vm == nil || vm.DeletionTimestamp != nil { - vmipStatus.VirtualMachine = "" + vmip.Status.VirtualMachine = "" conditionAttach.Status(metav1.ConditionFalse). Reason(vmipcondition.VirtualMachineNotFound). Message("Virtual machine not found") h.recorder.Event(vmip, corev1.EventTypeWarning, virtv2.ReasonNotAttached, "Virtual machine not found.") + return } - lease, err := state.VirtualMachineIPLease(ctx) - if err != nil { - return reconcile.Result{}, err - } + vmip.Status.Phase = virtv2.VirtualMachineIPAddressPhaseAttached + vmip.Status.VirtualMachine = vm.Name + conditionAttach.Status(metav1.ConditionTrue). + Reason(vmipcondition.Attached) + h.recorder.Eventf(vmip, corev1.EventTypeNormal, virtv2.ReasonAttached, "VirtualMachineIPAddress is attached to %q/%q.", vm.Namespace, vm.Name) +} +func (h *LifecycleHandler) handleBoundLease(lease *virtv2.VirtualMachineIPAddressLease, vmip *virtv2.VirtualMachineIPAddress, log *slog.Logger, conditionBound *conditions.ConditionBuilder) bool { needRequeue := false switch { - case lease == nil && vmipStatus.Address != "": - vmipStatus.Phase = virtv2.VirtualMachineIPAddressPhasePending - conditionBound.Status(metav1.ConditionFalse). - Reason(vmipcondition.VirtualMachineIPAddressLeaseLost). - Message(fmt.Sprintf("VirtualMachineIPAddressLease %s doesn't exist", - ip.IpToLeaseName(vmipStatus.Address))) - - case lease == nil: - vmipStatus.Phase = virtv2.VirtualMachineIPAddressPhasePending - conditionBound.Status(metav1.ConditionFalse). - Reason(vmipcondition.VirtualMachineIPAddressLeaseNotFound). - Message("VirtualMachineIPAddressLease is not found") - case util.IsBoundLease(lease, vmip): - vmipStatus.Phase = virtv2.VirtualMachineIPAddressPhaseBound - vmipStatus.Address = ip.LeaseNameToIP(lease.Name) + vmip.Status.Phase = virtv2.VirtualMachineIPAddressPhaseBound + vmip.Status.Address = ip.LeaseNameToIP(lease.Name) conditionBound.Status(metav1.ConditionTrue). Reason(vmipcondition.Bound) - if vm != nil && vm.GetDeletionTimestamp().IsZero() { - vmipStatus.Phase = virtv2.VirtualMachineIPAddressPhaseAttached - vmipStatus.VirtualMachine = vm.Name - conditionAttach.Status(metav1.ConditionTrue). - Reason(vmipcondition.Attached) - h.recorder.Eventf(vmip, corev1.EventTypeNormal, virtv2.ReasonAttached, "VirtualMachineIPAddress is attached to %q/%q.", vm.Namespace, vm.Name) - } - case lease.Status.Phase == virtv2.VirtualMachineIPAddressLeasePhaseBound: - vmipStatus.Phase = virtv2.VirtualMachineIPAddressPhasePending + vmip.Status.Phase = virtv2.VirtualMachineIPAddressPhasePending log.Warn(fmt.Sprintf("VirtualMachineIPAddressLease %s is bound to another VirtualMachineIPAddress resource: %s/%s", lease.Name, lease.Spec.VirtualMachineIPAddressRef.Name, lease.Spec.VirtualMachineIPAddressRef.Namespace)) conditionBound.Status(metav1.ConditionFalse). @@ -125,29 +143,21 @@ func (h *LifecycleHandler) Handle(ctx context.Context, state state.VMIPState) (r lease.Name)) case lease.Spec.VirtualMachineIPAddressRef.Namespace != vmip.Namespace: - vmipStatus.Phase = virtv2.VirtualMachineIPAddressPhasePending + vmip.Status.Phase = virtv2.VirtualMachineIPAddressPhasePending conditionBound.Status(metav1.ConditionFalse). Reason(vmipcondition.VirtualMachineIPAddressLeaseAlreadyExists). Message(fmt.Sprintf("The VirtualMachineIPLease %s belongs to a different namespace", lease.Name)) needRequeue = true default: - vmipStatus.Phase = virtv2.VirtualMachineIPAddressPhasePending + vmip.Status.Phase = virtv2.VirtualMachineIPAddressPhasePending conditionBound.Status(metav1.ConditionFalse). Reason(vmipcondition.VirtualMachineIPAddressLeaseNotReady). Message(fmt.Sprintf("VirtualMachineIPAddressLease %s is not ready", lease.Name)) } - log.Debug("Set VirtualMachineIP phase", "phase", vmipStatus.Phase) - - vmipStatus.ObservedGeneration = vmip.GetGeneration() - if !needRequeue { - return reconcile.Result{}, nil - } else { - // TODO add requeue with with exponential BackOff time interval using condition Bound -> probeTime - return reconcile.Result{RequeueAfter: 30 * time.Second}, nil - } + return needRequeue } func (h *LifecycleHandler) Name() string { diff --git a/images/virtualization-artifact/pkg/controller/vmip/internal/protection_handler.go b/images/virtualization-artifact/pkg/controller/vmip/internal/protection_handler.go index 26b53f9a0f..e4468c6bfd 100644 --- a/images/virtualization-artifact/pkg/controller/vmip/internal/protection_handler.go +++ b/images/virtualization-artifact/pkg/controller/vmip/internal/protection_handler.go @@ -45,11 +45,6 @@ func (h *ProtectionHandler) Handle(ctx context.Context, state state.VMIPState) ( vmip := state.VirtualMachineIP() - vm, err := state.VirtualMachine(ctx) - if err != nil { - return reconcile.Result{}, err - } - configuredVms, err := h.getConfiguredVM(ctx, vmip) if err != nil { return reconcile.Result{}, err @@ -66,12 +61,14 @@ func (h *ProtectionHandler) Handle(ctx context.Context, state state.VMIPState) ( log.Debug("VirtualMachineIPAddress deletion is delayed: it's protected by virtual machines") } + vm := state.VirtualMachine() + if vm == nil || vm.DeletionTimestamp != nil { log.Info("VirtualMachineIP is no longer attached to any VM: remove cleanup finalizer", "VirtualMachineIPName", vmip.Name) controllerutil.RemoveFinalizer(vmip, virtv2.FinalizerIPAddressCleanup) } else if vmip.GetDeletionTimestamp() == nil { - controllerutil.AddFinalizer(vmip, virtv2.FinalizerIPAddressCleanup) log.Info("VirtualMachineIP is still attached, finalizer added", "VirtualMachineIPName", vmip.Name) + controllerutil.AddFinalizer(vmip, virtv2.FinalizerIPAddressCleanup) } return reconcile.Result{}, nil diff --git a/images/virtualization-artifact/pkg/controller/vmip/internal/state/state.go b/images/virtualization-artifact/pkg/controller/vmip/internal/state/state.go index 2304c938bb..5d99424b36 100644 --- a/images/virtualization-artifact/pkg/controller/vmip/internal/state/state.go +++ b/images/virtualization-artifact/pkg/controller/vmip/internal/state/state.go @@ -18,72 +18,106 @@ package state import ( "context" + "errors" "fmt" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/labels" "k8s.io/apimachinery/pkg/types" "sigs.k8s.io/controller-runtime/pkg/client" + "github.com/deckhouse/virtualization-controller/pkg/common/annotations" "github.com/deckhouse/virtualization-controller/pkg/common/ip" "github.com/deckhouse/virtualization-controller/pkg/common/object" "github.com/deckhouse/virtualization-controller/pkg/controller/conditions" - "github.com/deckhouse/virtualization-controller/pkg/controller/indexer" "github.com/deckhouse/virtualization-controller/pkg/controller/ipam" "github.com/deckhouse/virtualization-controller/pkg/controller/vmip/internal/util" + "github.com/deckhouse/virtualization-controller/pkg/logger" + "github.com/deckhouse/virtualization/api/client/kubeclient" virtv2 "github.com/deckhouse/virtualization/api/core/v1alpha2" "github.com/deckhouse/virtualization/api/core/v1alpha2/vmiplcondition" ) type VMIPState interface { + Reload(ctx context.Context) error VirtualMachineIP() *virtv2.VirtualMachineIPAddress - VirtualMachineIPLease(ctx context.Context) (*virtv2.VirtualMachineIPAddressLease, error) - VirtualMachine(ctx context.Context) (*virtv2.VirtualMachine, error) + VirtualMachineIPLease() *virtv2.VirtualMachineIPAddressLease + VirtualMachine() *virtv2.VirtualMachine AllocatedIPs() ip.AllocatedIPs } type state struct { client client.Client + virtClient kubeclient.Client vmip *virtv2.VirtualMachineIPAddress lease *virtv2.VirtualMachineIPAddressLease vm *virtv2.VirtualMachine allocatedIPs ip.AllocatedIPs } -func New(c client.Client, vmip *virtv2.VirtualMachineIPAddress) VMIPState { - return &state{client: c, vmip: vmip} +func New(c client.Client, virtClient kubeclient.Client, vmip *virtv2.VirtualMachineIPAddress) VMIPState { + return &state{client: c, virtClient: virtClient, vmip: vmip} +} + +func (s *state) Reload(ctx context.Context) error { + if err := s.reloadVirtualMachineIPLease(ctx); err != nil { + return err + } + + if err := s.reloadVirtualMachine(ctx); err != nil { + return err + } + + return nil } func (s *state) VirtualMachineIP() *virtv2.VirtualMachineIPAddress { return s.vmip } -func (s *state) VirtualMachineIPLease(ctx context.Context) (*virtv2.VirtualMachineIPAddressLease, error) { - if s.lease != nil { - return s.lease, nil - } +func (s *state) VirtualMachineIPLease() *virtv2.VirtualMachineIPAddressLease { + return s.lease +} + +func (s *state) VirtualMachine() *virtv2.VirtualMachine { + return s.vm +} + +func (s *state) AllocatedIPs() ip.AllocatedIPs { + return s.allocatedIPs +} +func (s *state) reloadVirtualMachineIPLease(ctx context.Context) error { var err error + leaseName := ip.IpToLeaseName(s.vmip.Spec.StaticIP) - leaseName := ip.IpToLeaseName(s.vmip.Status.Address) + if leaseName == "" { + leaseName = ip.IpToLeaseName(s.vmip.Status.Address) + } if leaseName != "" { leaseKey := types.NamespacedName{Name: leaseName} s.lease, err = object.FetchObject(ctx, leaseKey, s.client, &virtv2.VirtualMachineIPAddressLease{}) if err != nil { - return nil, fmt.Errorf("unable to get Lease %s: %w", leaseKey, err) + return fmt.Errorf("unable to get Lease %s: %w", leaseKey, err) } } + log := logger.FromContext(ctx) if s.lease == nil { - var leases virtv2.VirtualMachineIPAddressLeaseList - err = s.client.List(ctx, &leases, - client.InNamespace(s.vmip.Namespace), - &client.MatchingFields{ - indexer.IndexFieldVMIPLeaseByVMIP: s.vmip.Name, - }) + leases := &virtv2.VirtualMachineIPAddressLeaseList{} + + err = s.client.List(ctx, leases, &client.ListOptions{ + LabelSelector: labels.SelectorFromSet(map[string]string{annotations.LabelVirtualMachineIPAddressUID: string(s.vmip.GetUID())}), + }) + if err != nil { - return nil, err + return err + } + + if len(leases.Items) > 1 { + log.Error("More than one VirtualMachineIPAddressLease found", "count", len(leases.Items)) } for i, lease := range leases.Items { @@ -95,31 +129,44 @@ func (s *state) VirtualMachineIPLease(ctx context.Context) (*virtv2.VirtualMachi } } + if s.lease == nil { + leases, err := s.virtClient.VirtualMachineIPAddressLeases().List(ctx, metav1.ListOptions{ + LabelSelector: fmt.Sprintf("%s=%s", annotations.LabelVirtualMachineIPAddressUID, string(s.vmip.GetUID())), + }) + if err != nil { + return err + } + + if len(leases.Items) != 0 { + if len(leases.Items) > 1 { + log.Error("More than one VirtualMachineIPAddressLease found in kubeclient without cache", "count", len(leases.Items)) + } + log.Warn("VirtualMachineIPAddressLease found in kubeclient without cache", "vmip", s.vmip.Name) + return errors.New("VirtualMachineIPAddressLease found in kubeclient without cache") + } + } + if s.lease == nil { s.allocatedIPs, err = util.GetAllocatedIPs(ctx, s.client, s.vmip.Spec.Type) if err != nil { - return nil, err + return err } } - return s.lease, nil + return nil } -func (s *state) VirtualMachine(ctx context.Context) (*virtv2.VirtualMachine, error) { - if s.vm != nil { - return s.vm, nil - } - +func (s *state) reloadVirtualMachine(ctx context.Context) error { var err error if s.vmip.Status.VirtualMachine != "" { vmKey := types.NamespacedName{Name: s.vmip.Status.VirtualMachine, Namespace: s.vmip.Namespace} vm, err := object.FetchObject(ctx, vmKey, s.client, &virtv2.VirtualMachine{}) if err != nil { - return nil, fmt.Errorf("unable to get VM %s: %w", vmKey, err) + return fmt.Errorf("unable to get VM %s: %w", vmKey, err) } if vm == nil { - return s.vm, nil + return nil } if vm.Status.VirtualMachineIPAddress == s.vmip.Name && vm.Status.IPAddress == s.vmip.Status.Address { @@ -133,7 +180,7 @@ func (s *state) VirtualMachine(ctx context.Context) (*virtv2.VirtualMachine, err Namespace: s.vmip.Namespace, }) if err != nil { - return nil, err + return err } for i, vm := range vms.Items { @@ -144,9 +191,5 @@ func (s *state) VirtualMachine(ctx context.Context) (*virtv2.VirtualMachine, err } } - return s.vm, nil -} - -func (s *state) AllocatedIPs() ip.AllocatedIPs { - return s.allocatedIPs + return nil } diff --git a/images/virtualization-artifact/pkg/controller/vmip/vmip_controller.go b/images/virtualization-artifact/pkg/controller/vmip/vmip_controller.go index e548d4528e..2704c28236 100644 --- a/images/virtualization-artifact/pkg/controller/vmip/vmip_controller.go +++ b/images/virtualization-artifact/pkg/controller/vmip/vmip_controller.go @@ -26,10 +26,12 @@ import ( "sigs.k8s.io/controller-runtime/pkg/manager" "github.com/deckhouse/deckhouse/pkg/log" + "github.com/deckhouse/virtualization-controller/pkg/controller/service" "github.com/deckhouse/virtualization-controller/pkg/controller/vmip/internal" "github.com/deckhouse/virtualization-controller/pkg/eventrecord" "github.com/deckhouse/virtualization-controller/pkg/logger" + "github.com/deckhouse/virtualization/api/client/kubeclient" "github.com/deckhouse/virtualization/api/core/v1alpha2" ) @@ -40,6 +42,7 @@ const ( func NewController( ctx context.Context, mgr manager.Manager, + virtClient kubeclient.Client, log *log.Logger, virtualMachineCIDRs []string, ) (controller.Controller, error) { @@ -52,7 +55,7 @@ func NewController( internal.NewLifecycleHandler(recorder), } - r, err := NewReconciler(mgr.GetClient(), handlers...) + r, err := NewReconciler(mgr.GetClient(), virtClient, handlers...) if err != nil { return nil, err } diff --git a/images/virtualization-artifact/pkg/controller/vmip/vmip_reconciler.go b/images/virtualization-artifact/pkg/controller/vmip/vmip_reconciler.go index 804c72f494..32b2e48813 100644 --- a/images/virtualization-artifact/pkg/controller/vmip/vmip_reconciler.go +++ b/images/virtualization-artifact/pkg/controller/vmip/vmip_reconciler.go @@ -19,7 +19,6 @@ package vmip import ( "context" "fmt" - "time" "k8s.io/apimachinery/pkg/types" "sigs.k8s.io/controller-runtime/pkg/client" @@ -34,7 +33,7 @@ import ( "github.com/deckhouse/virtualization-controller/pkg/controller/indexer" "github.com/deckhouse/virtualization-controller/pkg/controller/reconciler" "github.com/deckhouse/virtualization-controller/pkg/controller/vmip/internal/state" - "github.com/deckhouse/virtualization-controller/pkg/logger" + "github.com/deckhouse/virtualization/api/client/kubeclient" virtv2 "github.com/deckhouse/virtualization/api/core/v1alpha2" ) @@ -44,14 +43,16 @@ type Handler interface { } type Reconciler struct { - handlers []Handler - client client.Client + handlers []Handler + client client.Client + virtClient kubeclient.Client } -func NewReconciler(client client.Client, handlers ...Handler) (*Reconciler, error) { +func NewReconciler(client client.Client, virtClient kubeclient.Client, handlers ...Handler) (*Reconciler, error) { return &Reconciler{ - client: client, - handlers: handlers, + client: client, + virtClient: virtClient, + handlers: handlers, }, nil } @@ -73,14 +74,32 @@ func (r *Reconciler) SetupController(_ context.Context, mgr manager.Manager, ctr handler.EnqueueRequestsFromMapFunc(r.enqueueRequestsFromVMs), predicate.Funcs{ CreateFunc: func(e event.CreateEvent) bool { return false }, - DeleteFunc: func(e event.DeleteEvent) bool { return true }, - UpdateFunc: func(e event.UpdateEvent) bool { return true }, + DeleteFunc: func(e event.DeleteEvent) bool { + return true + }, + UpdateFunc: func(e event.UpdateEvent) bool { + oldVm := e.ObjectOld.(*virtv2.VirtualMachine) + newVm := e.ObjectNew.(*virtv2.VirtualMachine) + return newVm.Spec.VirtualMachineIPAddress != oldVm.Spec.VirtualMachineIPAddress || + newVm.Status.VirtualMachineIPAddress != oldVm.Status.VirtualMachineIPAddress + }, }, ); err != nil { return fmt.Errorf("error setting watch on vms: %w", err) } - return ctr.Watch(source.Kind(mgr.GetCache(), &virtv2.VirtualMachineIPAddress{}), &handler.EnqueueRequestForObject{}) + return ctr.Watch( + source.Kind(mgr.GetCache(), &virtv2.VirtualMachineIPAddress{}), + &handler.EnqueueRequestForObject{}, + predicate.Funcs{ + CreateFunc: func(e event.CreateEvent) bool { return true }, + DeleteFunc: func(e event.DeleteEvent) bool { return false }, + UpdateFunc: func(e event.UpdateEvent) bool { + oldVmip := e.ObjectOld.(*virtv2.VirtualMachineIPAddress) + newVmip := e.ObjectNew.(*virtv2.VirtualMachineIPAddress) + return oldVmip.Spec != newVmip.Spec + }, + }) } func (r *Reconciler) enqueueRequestsFromVMs(ctx context.Context, obj client.Object) []reconcile.Request { @@ -153,7 +172,16 @@ func (r *Reconciler) Reconcile(ctx context.Context, req reconcile.Request) (reco return reconcile.Result{}, nil } - s := state.New(r.client, vmip.Changed()) + s := state.New(r.client, r.virtClient, vmip.Changed()) + err = s.Reload(ctx) + if err != nil { + if err.Error() == "VirtualMachineIPAddressLease found in kubeclient without cache" { + return reconcile.Result{}, nil + } + + return reconcile.Result{}, err + } + rec := reconciler.NewBaseReconciler[Handler](r.handlers) rec.SetHandlerExecutor(func(ctx context.Context, h Handler) (reconcile.Result, error) { return h.Handle(ctx, s) @@ -164,21 +192,7 @@ func (r *Reconciler) Reconcile(ctx context.Context, req reconcile.Request) (reco return vmip.Update(ctx) }) - // TODO: This code addresses the issue of creating vmipl within the controller. - // The object is saved in etcd but does not get updated in the cache in time. - // As a result, we encounter the creation of multiple vmipl during a single reconcile operation. - // Adding reconcile.Result{RequeueAfter: 2 * time.Second} helps to fix this issue in 90% of cases. - // In the future, this code should be architecturally redesigned to prevent such situations. - result, err := rec.Reconcile(ctx) - if err != nil { - logger.FromContext(ctx).Error("Failed to reconcile VMIP", logger.SlogErr(err)) - return reconcile.Result{RequeueAfter: 2 * time.Second}, nil - } - if result.Requeue { - return reconcile.Result{RequeueAfter: 2 * time.Second}, nil - } - - return result, nil + return rec.Reconcile(ctx) } func (r *Reconciler) factory() *virtv2.VirtualMachineIPAddress { diff --git a/images/virtualization-artifact/pkg/controller/vmiplease/internal/lifecycle_handler.go b/images/virtualization-artifact/pkg/controller/vmiplease/internal/lifecycle_handler.go index 61c579f63e..09df427174 100644 --- a/images/virtualization-artifact/pkg/controller/vmiplease/internal/lifecycle_handler.go +++ b/images/virtualization-artifact/pkg/controller/vmiplease/internal/lifecycle_handler.go @@ -22,7 +22,6 @@ import ( metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "sigs.k8s.io/controller-runtime/pkg/reconcile" - "github.com/deckhouse/virtualization-controller/pkg/common/ip" "github.com/deckhouse/virtualization-controller/pkg/controller/conditions" "github.com/deckhouse/virtualization-controller/pkg/controller/vmiplease/internal/state" virtv2 "github.com/deckhouse/virtualization/api/core/v1alpha2" @@ -55,7 +54,7 @@ func (h *LifecycleHandler) Handle(ctx context.Context, state state.VMIPLeaseStat return reconcile.Result{}, err } - if vmip != nil && vmip.Status.Address == ip.LeaseNameToIP(lease.Name) { + if vmip != nil { leaseStatus.Phase = virtv2.VirtualMachineIPAddressLeasePhaseBound cb.Status(metav1.ConditionTrue). Reason(vmiplcondition.Bound) diff --git a/images/virtualization-artifact/pkg/controller/vmiplease/internal/protection_handler.go b/images/virtualization-artifact/pkg/controller/vmiplease/internal/protection_handler.go index 7323a3e63e..6ff435d94e 100644 --- a/images/virtualization-artifact/pkg/controller/vmiplease/internal/protection_handler.go +++ b/images/virtualization-artifact/pkg/controller/vmiplease/internal/protection_handler.go @@ -22,7 +22,6 @@ import ( "sigs.k8s.io/controller-runtime/pkg/controller/controllerutil" "sigs.k8s.io/controller-runtime/pkg/reconcile" - "github.com/deckhouse/virtualization-controller/pkg/common/ip" "github.com/deckhouse/virtualization-controller/pkg/controller/vmiplease/internal/state" "github.com/deckhouse/virtualization-controller/pkg/logger" virtv2 "github.com/deckhouse/virtualization/api/core/v1alpha2" @@ -45,7 +44,7 @@ func (h *ProtectionHandler) Handle(ctx context.Context, state state.VMIPLeaseSta return reconcile.Result{}, err } - if vmip != nil && vmip.Status.Address == ip.LeaseNameToIP(lease.Name) { + if vmip != nil { controllerutil.AddFinalizer(lease, virtv2.FinalizerIPAddressLeaseCleanup) } else { log.Info("Deletion observed: remove cleanup finalizer from VirtualMachineIPAddressLease") diff --git a/images/virtualization-artifact/pkg/controller/vmiplease/internal/retention_handler.go b/images/virtualization-artifact/pkg/controller/vmiplease/internal/retention_handler.go index 71082fe0f6..10d6fff63b 100644 --- a/images/virtualization-artifact/pkg/controller/vmiplease/internal/retention_handler.go +++ b/images/virtualization-artifact/pkg/controller/vmiplease/internal/retention_handler.go @@ -23,6 +23,7 @@ import ( "sigs.k8s.io/controller-runtime/pkg/reconcile" + "github.com/deckhouse/virtualization-controller/pkg/common/annotations" "github.com/deckhouse/virtualization-controller/pkg/common/ip" "github.com/deckhouse/virtualization-controller/pkg/controller/conditions" "github.com/deckhouse/virtualization-controller/pkg/controller/vmiplease/internal/state" @@ -52,10 +53,11 @@ func (h *RetentionHandler) Handle(ctx context.Context, state state.VMIPLeaseStat return reconcile.Result{}, err } - if vmip == nil || vmip.Status.Address != ip.LeaseNameToIP(lease.Name) { - if lease.Spec.VirtualMachineIPAddressRef.Name != "" { - log.Debug("VirtualMachineIP not found: remove this ref from the spec and retain VMIPLease") + if vmip == nil || vmip.Status.Address != "" && vmip.Status.Address != ip.LeaseNameToIP(lease.Name) { + if lease.Spec.VirtualMachineIPAddressRef.Name != "" || lease.Labels[annotations.LabelVirtualMachineIPAddressUID] != "" { + log.Debug("VirtualMachineIP not found: remove this ref from the spec, delete label value and retain VMIPLease") lease.Spec.VirtualMachineIPAddressRef.Name = "" + annotations.AddLabel(lease, annotations.LabelVirtualMachineIPAddressUID, "") return reconcile.Result{RequeueAfter: h.retentionDuration}, nil }