From cfe7245d37e5a1cc0e50ee4fcc7c52e1c7585d5a Mon Sep 17 00:00:00 2001 From: Daniil Antoshin Date: Tue, 3 Mar 2026 13:00:06 +0200 Subject: [PATCH 1/3] feat(core): add backoff Signed-off-by: Daniil Antoshin --- .../pkg/common/backoff/backgoff.go | 39 ---- .../evacuation/evacuation_controller.go | 4 +- .../evacuation/internal/handler/evacuation.go | 15 +- .../internal/handler/evacuation_test.go | 5 +- .../pkg/controller/service/backoff_service.go | 176 ++++++++++++++++++ .../service/backoff_service_test.go | 153 +++++++++++++++ .../vm/internal/usb_device_attach_handler.go | 10 +- .../usb_device_attach_handler_test.go | 3 +- .../vm/internal/usb_device_detach_handler.go | 15 +- .../usb_device_detach_handler_test.go | 5 +- .../pkg/controller/vm/vm_controller.go | 5 +- .../internal/handler/migration.go | 57 ++---- .../internal/handler/migration_test.go | 17 +- .../volumemigration_controller.go | 4 +- 14 files changed, 398 insertions(+), 110 deletions(-) delete mode 100644 images/virtualization-artifact/pkg/common/backoff/backgoff.go create mode 100644 images/virtualization-artifact/pkg/controller/service/backoff_service.go create mode 100644 images/virtualization-artifact/pkg/controller/service/backoff_service_test.go diff --git a/images/virtualization-artifact/pkg/common/backoff/backgoff.go b/images/virtualization-artifact/pkg/common/backoff/backgoff.go deleted file mode 100644 index f3a3fe31ba..0000000000 --- a/images/virtualization-artifact/pkg/common/backoff/backgoff.go +++ /dev/null @@ -1,39 +0,0 @@ -/* -Copyright 2026 Flant JSC - -Licensed under the Apache License, Version 2.0 (the "License"); -you may not use this file except in compliance with the License. -You may obtain a copy of the License at - - http://www.apache.org/licenses/LICENSE-2.0 - -Unless required by applicable law or agreed to in writing, software -distributed under the License is distributed on an "AS IS" BASIS, -WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. -See the License for the specific language governing permissions and -limitations under the License. -*/ - -package backoff - -import ( - "time" - - "k8s.io/apimachinery/pkg/util/wait" -) - -func CalculateBackOff(failedCount int) time.Duration { - if failedCount == 0 { - return 0 - } - - evacuationBackoff := wait.Backoff{ - Duration: 2 * time.Second, - Factor: 2.0, - Jitter: 0, - Cap: 5 * time.Minute, - Steps: failedCount, - } - - return evacuationBackoff.Step() -} diff --git a/images/virtualization-artifact/pkg/controller/evacuation/evacuation_controller.go b/images/virtualization-artifact/pkg/controller/evacuation/evacuation_controller.go index 058b876cfb..e5890da803 100644 --- a/images/virtualization-artifact/pkg/controller/evacuation/evacuation_controller.go +++ b/images/virtualization-artifact/pkg/controller/evacuation/evacuation_controller.go @@ -26,6 +26,7 @@ import ( "github.com/deckhouse/deckhouse/pkg/log" "github.com/deckhouse/virtualization-controller/pkg/controller/evacuation/internal/handler" + "github.com/deckhouse/virtualization-controller/pkg/controller/service" "github.com/deckhouse/virtualization-controller/pkg/logger" "github.com/deckhouse/virtualization/api/client/kubeclient" ) @@ -41,9 +42,10 @@ func SetupController( log *log.Logger, ) error { client := mgr.GetClient() + backoffSvc := service.NewBackoffService() handlers := []Handler{ - handler.NewEvacuationHandler(client, newCanceler(virtClient)), + handler.NewEvacuationHandler(client, newCanceler(virtClient), backoffSvc), } r := NewReconciler(client, handlers) diff --git a/images/virtualization-artifact/pkg/controller/evacuation/internal/handler/evacuation.go b/images/virtualization-artifact/pkg/controller/evacuation/internal/handler/evacuation.go index 0565ad4560..e28919199d 100644 --- a/images/virtualization-artifact/pkg/controller/evacuation/internal/handler/evacuation.go +++ b/images/virtualization-artifact/pkg/controller/evacuation/internal/handler/evacuation.go @@ -30,7 +30,6 @@ import ( vmopbuilder "github.com/deckhouse/virtualization-controller/pkg/builder/vmop" "github.com/deckhouse/virtualization-controller/pkg/common/annotations" - "github.com/deckhouse/virtualization-controller/pkg/common/backoff" commonvmop "github.com/deckhouse/virtualization-controller/pkg/common/vmop" "github.com/deckhouse/virtualization-controller/pkg/controller/conditions" "github.com/deckhouse/virtualization-controller/pkg/logger" @@ -40,10 +39,15 @@ import ( const nameEvacuationHandler = "EvacuationHandler" -func NewEvacuationHandler(client client.Client, evacuateCanceler EvacuateCanceler) *EvacuationHandler { +type BackoffServicer interface { + CalculateBackoff(failedCount int) time.Duration +} + +func NewEvacuationHandler(client client.Client, evacuateCanceler EvacuateCanceler, backoffSvc BackoffServicer) *EvacuationHandler { return &EvacuationHandler{ client: client, evacuateCanceler: evacuateCanceler, + backoffSvc: backoffSvc, } } @@ -55,6 +59,7 @@ type EvacuateCanceler interface { type EvacuationHandler struct { client client.Client evacuateCanceler EvacuateCanceler + backoffSvc BackoffServicer } func (h *EvacuationHandler) Handle(ctx context.Context, vm *v1alpha2.VirtualMachine) (reconcile.Result, error) { @@ -98,9 +103,9 @@ func (h *EvacuationHandler) Handle(ctx context.Context, vm *v1alpha2.VirtualMach } } - backoff := backoff.CalculateBackOff(failedCount) - if backoff > 0 { - return reconcile.Result{RequeueAfter: backoff}, nil + delay := h.backoffSvc.CalculateBackoff(failedCount) + if delay > 0 { + return reconcile.Result{RequeueAfter: delay}, nil } log.Info("Create evacuation vmop") diff --git a/images/virtualization-artifact/pkg/controller/evacuation/internal/handler/evacuation_test.go b/images/virtualization-artifact/pkg/controller/evacuation/internal/handler/evacuation_test.go index e95c27aad1..9036ca7bda 100644 --- a/images/virtualization-artifact/pkg/controller/evacuation/internal/handler/evacuation_test.go +++ b/images/virtualization-artifact/pkg/controller/evacuation/internal/handler/evacuation_test.go @@ -30,6 +30,7 @@ import ( vmbuilder "github.com/deckhouse/virtualization-controller/pkg/builder/vm" "github.com/deckhouse/virtualization-controller/pkg/common/annotations" "github.com/deckhouse/virtualization-controller/pkg/common/testutil" + "github.com/deckhouse/virtualization-controller/pkg/controller/service" "github.com/deckhouse/virtualization/api/core/v1alpha2" "github.com/deckhouse/virtualization/api/core/v1alpha2/vmcondition" ) @@ -74,7 +75,7 @@ var _ = Describe("TestEvacuationHandler", func() { h := NewEvacuationHandler(fakeClient, &EvacuateCancelerMock{CancelFunc: func(_ context.Context, _, _ string) error { return nil - }}) + }}, service.NewBackoffService()) _, err := h.Handle(ctx, vm) Expect(err).NotTo(HaveOccurred()) @@ -121,7 +122,7 @@ var _ = Describe("TestEvacuationHandler", func() { vmop.Name = "evacuation-12345" fakeClient = setupEnvironment(newVM(true), vmop) - h := NewEvacuationHandler(fakeClient, canceler) + h := NewEvacuationHandler(fakeClient, canceler, service.NewBackoffService()) err := fakeClient.Delete(ctx, vmop) Expect(err).NotTo(HaveOccurred()) diff --git a/images/virtualization-artifact/pkg/controller/service/backoff_service.go b/images/virtualization-artifact/pkg/controller/service/backoff_service.go new file mode 100644 index 0000000000..bccd9c008b --- /dev/null +++ b/images/virtualization-artifact/pkg/controller/service/backoff_service.go @@ -0,0 +1,176 @@ +/* +Copyright 2026 Flant JSC + +Licensed under the Apache License, Version 2.0 (the "License"); +you may not use this file except in compliance with the License. +You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. +*/ + +package service + +import ( + "reflect" + "sync" + "time" + + "k8s.io/apimachinery/pkg/util/wait" + "sigs.k8s.io/controller-runtime/pkg/client" +) + +const ( + defaultBaseDelay = 2 * time.Second + defaultFactor = 2.0 + defaultMaxDelay = 5 * time.Minute +) + +// BackoffService tracks per-object-type failure counts and calculates exponential backoff durations. +// It is safe for concurrent use. All methods are nil-safe: if the object is nil, +// the service returns the base delay instead of an error. +type BackoffService struct { + mu sync.RWMutex + // stores maps object-type key -> (object UID string -> failed attempts count). + stores map[string]map[string]int + baseDelay time.Duration + factor float64 + maxDelay time.Duration +} + +type BackoffOption func(*BackoffService) + +func WithBaseDelay(d time.Duration) BackoffOption { + return func(s *BackoffService) { s.baseDelay = d } +} + +func WithFactor(f float64) BackoffOption { + return func(s *BackoffService) { s.factor = f } +} + +func WithMaxDelay(d time.Duration) BackoffOption { + return func(s *BackoffService) { s.maxDelay = d } +} + +func NewBackoffService(opts ...BackoffOption) *BackoffService { + s := &BackoffService{ + stores: make(map[string]map[string]int), + baseDelay: defaultBaseDelay, + factor: defaultFactor, + maxDelay: defaultMaxDelay, + } + for _, opt := range opts { + opt(s) + } + return s +} + +// RegisterFailure increments the failure counter for the given object and returns the new count. +// If obj is nil, returns 1 (treated as a single failure). +func (s *BackoffService) RegisterFailure(obj client.Object) int { + typeKey, objKey := objectKeys(obj) + if typeKey == "" { + return 1 + } + + s.mu.Lock() + defer s.mu.Unlock() + + store := s.stores[typeKey] + if store == nil { + store = make(map[string]int) + s.stores[typeKey] = store + } + store[objKey]++ + return store[objKey] +} + +// ResetFailures resets the failure counter for the given object. +// If obj is nil, this is a no-op. +func (s *BackoffService) ResetFailures(obj client.Object) { + typeKey, objKey := objectKeys(obj) + if typeKey == "" { + return + } + + s.mu.Lock() + defer s.mu.Unlock() + + if store, ok := s.stores[typeKey]; ok { + delete(store, objKey) + } +} + +// GetFailures returns the current failure count for the given object. +// If obj is nil, returns 0. +func (s *BackoffService) GetFailures(obj client.Object) int { + typeKey, objKey := objectKeys(obj) + if typeKey == "" { + return 0 + } + + s.mu.RLock() + defer s.mu.RUnlock() + + if store, ok := s.stores[typeKey]; ok { + return store[objKey] + } + return 0 +} + +// Backoff calculates the backoff duration for the given object based on its failure count. +// If obj is nil, returns the base delay. +func (s *BackoffService) Backoff(obj client.Object) time.Duration { + return s.calculateBackoff(s.GetFailures(obj)) +} + +// RegisterFailureAndBackoff increments the failure counter and returns the backoff duration. +// If obj is nil, returns the base delay. +func (s *BackoffService) RegisterFailureAndBackoff(obj client.Object) time.Duration { + return s.calculateBackoff(s.RegisterFailure(obj)) +} + +// CalculateBackoff computes the backoff duration for the given failure count without modifying any state. +func (s *BackoffService) CalculateBackoff(failedCount int) time.Duration { + return s.calculateBackoff(failedCount) +} + +func (s *BackoffService) calculateBackoff(failedCount int) time.Duration { + if failedCount == 0 { + return 0 + } + + b := wait.Backoff{ + Duration: s.baseDelay, + Factor: s.factor, + Jitter: 0, + Cap: s.maxDelay, + Steps: failedCount, + } + + var d time.Duration + for range failedCount { + d = b.Step() + } + return d +} + +func objectKeys(obj client.Object) (typeKey, objKey string) { + if obj == nil { + return "", "" + } + t := reflect.TypeOf(obj) + if t == nil { + return "", "" + } + uid := string(obj.GetUID()) + if uid == "" { + uid = obj.GetNamespace() + "/" + obj.GetName() + } + return t.String(), uid +} diff --git a/images/virtualization-artifact/pkg/controller/service/backoff_service_test.go b/images/virtualization-artifact/pkg/controller/service/backoff_service_test.go new file mode 100644 index 0000000000..1d31ce2ec8 --- /dev/null +++ b/images/virtualization-artifact/pkg/controller/service/backoff_service_test.go @@ -0,0 +1,153 @@ +/* +Copyright 2026 Flant JSC + +Licensed under the Apache License, Version 2.0 (the "License"); +you may not use this file except in compliance with the License. +You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. +*/ + +package service + +import ( + "time" + + . "github.com/onsi/ginkgo/v2" + . "github.com/onsi/gomega" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/types" + + "github.com/deckhouse/virtualization/api/core/v1alpha2" +) + +var _ = Describe("BackoffService", func() { + var svc *BackoffService + + BeforeEach(func() { + svc = NewBackoffService() + }) + + newVM := func(name string) *v1alpha2.VirtualMachine { + return &v1alpha2.VirtualMachine{ + ObjectMeta: metav1.ObjectMeta{ + Name: name, + Namespace: "default", + UID: types.UID("default/" + name), + }, + } + } + + newVD := func(name string) *v1alpha2.VirtualDisk { + return &v1alpha2.VirtualDisk{ + ObjectMeta: metav1.ObjectMeta{ + Name: name, + Namespace: "default", + UID: types.UID("default/" + name), + }, + } + } + + It("should return 0 failures and 0 backoff for new object", func() { + vm := newVM("test-vm") + Expect(svc.GetFailures(vm)).To(Equal(0)) + Expect(svc.Backoff(vm)).To(Equal(time.Duration(0))) + }) + + It("should increment failure count", func() { + vm := newVM("test-vm") + + Expect(svc.RegisterFailure(vm)).To(Equal(1)) + Expect(svc.RegisterFailure(vm)).To(Equal(2)) + Expect(svc.GetFailures(vm)).To(Equal(2)) + }) + + It("should reset failures", func() { + vm := newVM("test-vm") + + svc.RegisterFailure(vm) + svc.RegisterFailure(vm) + svc.ResetFailures(vm) + + Expect(svc.GetFailures(vm)).To(Equal(0)) + }) + + It("should track different objects of the same type independently", func() { + vm1 := newVM("vm-1") + vm2 := newVM("vm-2") + + svc.RegisterFailure(vm1) + svc.RegisterFailure(vm1) + svc.RegisterFailure(vm2) + + Expect(svc.GetFailures(vm1)).To(Equal(2)) + Expect(svc.GetFailures(vm2)).To(Equal(1)) + }) + + It("should track different object types independently", func() { + vm := newVM("obj-1") + vd := newVD("obj-1") + + svc.RegisterFailure(vm) + svc.RegisterFailure(vm) + svc.RegisterFailure(vm) + svc.RegisterFailure(vd) + + Expect(svc.GetFailures(vm)).To(Equal(3)) + Expect(svc.GetFailures(vd)).To(Equal(1)) + }) + + It("should calculate exponential backoff", func() { + vm := newVM("test-vm") + + // 1st failure: baseDelay * factor^0 = 2s + Expect(svc.RegisterFailureAndBackoff(vm)).To(Equal(2 * time.Second)) + + // 2nd failure: should be > 2s + Expect(svc.RegisterFailureAndBackoff(vm)).To(BeNumerically(">", 2*time.Second)) + }) + + It("should cap backoff at max delay", func() { + vm := newVM("test-vm") + + for range 30 { + svc.RegisterFailure(vm) + } + + Expect(svc.Backoff(vm)).To(Equal(defaultMaxDelay)) + }) + + It("should respect custom options", func() { + custom := NewBackoffService( + WithBaseDelay(5*time.Second), + WithFactor(3.0), + WithMaxDelay(1*time.Minute), + ) + + vm := newVM("test-vm") + Expect(custom.RegisterFailureAndBackoff(vm)).To(Equal(5 * time.Second)) + + for range 20 { + custom.RegisterFailure(vm) + } + + Expect(custom.Backoff(vm)).To(Equal(1 * time.Minute)) + }) + + It("should return base delay for nil object", func() { + Expect(svc.RegisterFailure(nil)).To(Equal(1)) + Expect(svc.GetFailures(nil)).To(Equal(0)) + Expect(svc.Backoff(nil)).To(Equal(time.Duration(0))) + Expect(svc.RegisterFailureAndBackoff(nil)).To(Equal(defaultBaseDelay)) + }) + + It("should be no-op reset for nil object", func() { + Expect(func() { svc.ResetFailures(nil) }).NotTo(Panic()) + }) +}) diff --git a/images/virtualization-artifact/pkg/controller/vm/internal/usb_device_attach_handler.go b/images/virtualization-artifact/pkg/controller/vm/internal/usb_device_attach_handler.go index 1075afa870..0a7efce5d0 100644 --- a/images/virtualization-artifact/pkg/controller/vm/internal/usb_device_attach_handler.go +++ b/images/virtualization-artifact/pkg/controller/vm/internal/usb_device_attach_handler.go @@ -20,7 +20,6 @@ import ( "context" "fmt" "strings" - "time" apierrors "k8s.io/apimachinery/pkg/api/errors" virtv1 "kubevirt.io/api/core/v1" @@ -28,6 +27,7 @@ import ( "sigs.k8s.io/controller-runtime/pkg/reconcile" "github.com/deckhouse/virtualization-controller/pkg/controller/conditions" + "github.com/deckhouse/virtualization-controller/pkg/controller/service" "github.com/deckhouse/virtualization-controller/pkg/controller/vm/internal/state" "github.com/deckhouse/virtualization-controller/pkg/logger" "github.com/deckhouse/virtualization/api/core/v1alpha2" @@ -36,17 +36,19 @@ import ( const nameUSBDeviceAttachHandler = "USBDeviceAttachHandler" -func NewUSBDeviceAttachHandler(cl client.Client, virtClient VirtClient) *USBDeviceAttachHandler { +func NewUSBDeviceAttachHandler(cl client.Client, virtClient VirtClient, backoffSvc *service.BackoffService) *USBDeviceAttachHandler { return &USBDeviceAttachHandler{ usbDeviceHandlerBase: usbDeviceHandlerBase{ client: cl, virtClient: virtClient, }, + backoffSvc: backoffSvc, } } type USBDeviceAttachHandler struct { usbDeviceHandlerBase + backoffSvc *service.BackoffService } func (h *USBDeviceAttachHandler) Name() string { @@ -163,7 +165,7 @@ func (h *USBDeviceAttachHandler) Handle(ctx context.Context, s state.VirtualMach requestName := h.getResourceClaimRequestName(deviceName) err := h.attachUSBDevice(ctx, vm, deviceName, templateName, requestName) if err != nil && !apierrors.IsAlreadyExists(err) && !strings.Contains(err.Error(), "already exists") { - return reconcile.Result{RequeueAfter: 5 * time.Second}, fmt.Errorf("failed to attach USB device %s: %w", deviceName, err) + return reconcile.Result{RequeueAfter: h.backoffSvc.RegisterFailureAndBackoff(vm)}, fmt.Errorf("failed to attach USB device %s: %w", deviceName, err) } nextStatusRefs = append(nextStatusRefs, h.buildDetachedStatus(existingStatus, deviceName, isReady)) @@ -171,6 +173,8 @@ func (h *USBDeviceAttachHandler) Handle(ctx context.Context, s state.VirtualMach changed.Status.USBDevices = nextStatusRefs + h.backoffSvc.ResetFailures(vm) + return reconcile.Result{}, nil } diff --git a/images/virtualization-artifact/pkg/controller/vm/internal/usb_device_attach_handler_test.go b/images/virtualization-artifact/pkg/controller/vm/internal/usb_device_attach_handler_test.go index 6bf8831ba7..2b00265105 100644 --- a/images/virtualization-artifact/pkg/controller/vm/internal/usb_device_attach_handler_test.go +++ b/images/virtualization-artifact/pkg/controller/vm/internal/usb_device_attach_handler_test.go @@ -34,6 +34,7 @@ import ( "sigs.k8s.io/controller-runtime/pkg/reconcile" "github.com/deckhouse/virtualization-controller/pkg/controller/reconciler" + "github.com/deckhouse/virtualization-controller/pkg/controller/service" "github.com/deckhouse/virtualization-controller/pkg/controller/vm/internal/state" "github.com/deckhouse/virtualization-controller/pkg/logger" virtualizationv1alpha2 "github.com/deckhouse/virtualization/api/client/generated/clientset/versioned/typed/core/v1alpha2" @@ -174,7 +175,7 @@ var _ = Describe("USBDeviceAttachHandler", func() { runHandle := func(vm *v1alpha2.VirtualMachine, objs ...client.Object) (reconcile.Result, *reconciler.Resource[*v1alpha2.VirtualMachine, v1alpha2.VirtualMachineStatus], state.VirtualMachineState, error) { fakeClient, vmResource, vmState = setupEnvironment(vm, objs...) - handler = NewUSBDeviceAttachHandler(fakeClient, mockVirtCl) + handler = NewUSBDeviceAttachHandler(fakeClient, mockVirtCl, service.NewBackoffService()) result, err := handler.Handle(ctx, vmState) _ = mockVirtCl.VirtualMachines(vmNamespace) diff --git a/images/virtualization-artifact/pkg/controller/vm/internal/usb_device_detach_handler.go b/images/virtualization-artifact/pkg/controller/vm/internal/usb_device_detach_handler.go index fcd1618f4f..30c0b0a3b5 100644 --- a/images/virtualization-artifact/pkg/controller/vm/internal/usb_device_detach_handler.go +++ b/images/virtualization-artifact/pkg/controller/vm/internal/usb_device_detach_handler.go @@ -25,6 +25,7 @@ import ( "sigs.k8s.io/controller-runtime/pkg/client" "sigs.k8s.io/controller-runtime/pkg/reconcile" + "github.com/deckhouse/virtualization-controller/pkg/controller/service" "github.com/deckhouse/virtualization-controller/pkg/controller/vm/internal/state" "github.com/deckhouse/virtualization-controller/pkg/logger" "github.com/deckhouse/virtualization/api/core/v1alpha2" @@ -32,17 +33,19 @@ import ( const nameUSBDeviceDetachHandler = "USBDeviceDetachHandler" -func NewUSBDeviceDetachHandler(cl client.Client, virtClient VirtClient) *USBDeviceDetachHandler { +func NewUSBDeviceDetachHandler(cl client.Client, virtClient VirtClient, backoffSvc *service.BackoffService) *USBDeviceDetachHandler { return &USBDeviceDetachHandler{ usbDeviceHandlerBase: usbDeviceHandlerBase{ client: cl, virtClient: virtClient, }, + backoffSvc: backoffSvc, } } type USBDeviceDetachHandler struct { usbDeviceHandlerBase + backoffSvc *service.BackoffService } func (h *USBDeviceDetachHandler) Name() string { @@ -80,7 +83,7 @@ func (h *USBDeviceDetachHandler) Handle(ctx context.Context, s state.VirtualMach err := h.detachUSBDevice(ctx, vm, existingStatus.Name) if err != nil && !apierrors.IsNotFound(err) && !strings.Contains(err.Error(), "it does not exist") { log.Error("failed to detach USB device", "error", err, "usbDevice", existingStatus.Name) - return reconcile.Result{}, fmt.Errorf("failed to detach USB device %s: %w", existingStatus.Name, err) + return reconcile.Result{RequeueAfter: h.backoffSvc.RegisterFailureAndBackoff(vm)}, fmt.Errorf("failed to detach USB device %s: %w", existingStatus.Name, err) } } } @@ -96,7 +99,7 @@ func (h *USBDeviceDetachHandler) Handle(ctx context.Context, s state.VirtualMach err := h.detachUSBDevice(ctx, vm, usbDeviceRef.Name) if err != nil && !apierrors.IsNotFound(err) && !strings.Contains(err.Error(), "it does not exist") { log.Error("failed to detach USB device (device not found)", "error", err, "usbDevice", usbDeviceRef.Name) - return reconcile.Result{}, fmt.Errorf("failed to detach USB device %s (device not found): %w", usbDeviceRef.Name, err) + return reconcile.Result{RequeueAfter: h.backoffSvc.RegisterFailureAndBackoff(vm)}, fmt.Errorf("failed to detach USB device %s (device not found): %w", usbDeviceRef.Name, err) } continue } @@ -105,7 +108,7 @@ func (h *USBDeviceDetachHandler) Handle(ctx context.Context, s state.VirtualMach err := h.detachUSBDevice(ctx, vm, usbDeviceRef.Name) if err != nil && !apierrors.IsNotFound(err) && !strings.Contains(err.Error(), "it does not exist") { log.Error("failed to detach USB device (device deleting)", "error", err, "usbDevice", usbDeviceRef.Name) - return reconcile.Result{}, fmt.Errorf("failed to detach USB device %s (device deleting): %w", usbDeviceRef.Name, err) + return reconcile.Result{RequeueAfter: h.backoffSvc.RegisterFailureAndBackoff(vm)}, fmt.Errorf("failed to detach USB device %s (device deleting): %w", usbDeviceRef.Name, err) } continue } @@ -114,10 +117,12 @@ func (h *USBDeviceDetachHandler) Handle(ctx context.Context, s state.VirtualMach err := h.detachUSBDevice(ctx, vm, usbDeviceRef.Name) if err != nil && !apierrors.IsNotFound(err) && !strings.Contains(err.Error(), "it does not exist") { log.Error("failed to detach USB device (absent on device)", "error", err, "usbDevice", usbDeviceRef.Name) - return reconcile.Result{}, fmt.Errorf("failed to detach USB device %s (device not ready): %w", usbDeviceRef.Name, err) + return reconcile.Result{RequeueAfter: h.backoffSvc.RegisterFailureAndBackoff(vm)}, fmt.Errorf("failed to detach USB device %s (device not ready): %w", usbDeviceRef.Name, err) } } } + h.backoffSvc.ResetFailures(vm) + return reconcile.Result{}, nil } diff --git a/images/virtualization-artifact/pkg/controller/vm/internal/usb_device_detach_handler_test.go b/images/virtualization-artifact/pkg/controller/vm/internal/usb_device_detach_handler_test.go index 795db9c278..3adbeb5ae7 100644 --- a/images/virtualization-artifact/pkg/controller/vm/internal/usb_device_detach_handler_test.go +++ b/images/virtualization-artifact/pkg/controller/vm/internal/usb_device_detach_handler_test.go @@ -28,6 +28,7 @@ import ( "sigs.k8s.io/controller-runtime/pkg/client" "sigs.k8s.io/controller-runtime/pkg/reconcile" + "github.com/deckhouse/virtualization-controller/pkg/controller/service" "github.com/deckhouse/virtualization-controller/pkg/controller/vm/internal/state" "github.com/deckhouse/virtualization-controller/pkg/logger" "github.com/deckhouse/virtualization/api/core/v1alpha2" @@ -82,7 +83,7 @@ var _ = Describe("USBDeviceDetachHandler", func() { fakeClient, _, st := setupEnvironment(vm, objs...) vmState = st - handler = NewUSBDeviceDetachHandler(fakeClient, mockVirtCl) + handler = NewUSBDeviceDetachHandler(fakeClient, mockVirtCl, service.NewBackoffService()) result, err := handler.Handle(ctx, vmState) Expect(err).NotTo(HaveOccurred()) @@ -114,7 +115,7 @@ var _ = Describe("USBDeviceDetachHandler", func() { fakeClient, _, st := setupEnvironment(vm) vmState = st - handler = NewUSBDeviceDetachHandler(fakeClient, mockVirtCl) + handler = NewUSBDeviceDetachHandler(fakeClient, mockVirtCl, service.NewBackoffService()) mockVM := mockVirtCl.VirtualMachines("default").(*mockVirtualMachines) mockVM.removeResourceClaimErr = removeErr diff --git a/images/virtualization-artifact/pkg/controller/vm/vm_controller.go b/images/virtualization-artifact/pkg/controller/vm/vm_controller.go index dd22ee3236..192fefbf96 100644 --- a/images/virtualization-artifact/pkg/controller/vm/vm_controller.go +++ b/images/virtualization-artifact/pkg/controller/vm/vm_controller.go @@ -59,6 +59,7 @@ func SetupController( vmClassService := service.NewVirtualMachineClassService(client) migrateVolumesService := vmservice.NewMigrationVolumesService(client, internal.MakeKVVMFromVMSpec, 10*time.Second) + backoffSvc := service.NewBackoffService() handlers := []Handler{ internal.NewMaintenanceHandler(client), @@ -67,8 +68,8 @@ func SetupController( internal.NewIPAMHandler(netmanager.NewIPAM(), client, recorder), internal.NewMACHandler(netmanager.NewMACManager(), client, recorder), internal.NewBlockDeviceHandler(client, blockDeviceService), - internal.NewUSBDeviceDetachHandler(client, virtClient), - internal.NewUSBDeviceAttachHandler(client, virtClient), + internal.NewUSBDeviceDetachHandler(client, virtClient, backoffSvc), + internal.NewUSBDeviceAttachHandler(client, virtClient, backoffSvc), internal.NewProvisioningHandler(client), internal.NewAgentHandler(), internal.NewFilesystemHandler(), diff --git a/images/virtualization-artifact/pkg/controller/volumemigration/internal/handler/migration.go b/images/virtualization-artifact/pkg/controller/volumemigration/internal/handler/migration.go index aa84dea95c..a5e9b4871a 100644 --- a/images/virtualization-artifact/pkg/controller/volumemigration/internal/handler/migration.go +++ b/images/virtualization-artifact/pkg/controller/volumemigration/internal/handler/migration.go @@ -26,7 +26,6 @@ import ( corev1 "k8s.io/api/core/v1" k8serrors "k8s.io/apimachinery/pkg/api/errors" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" - "k8s.io/apimachinery/pkg/types" "sigs.k8s.io/controller-runtime/pkg/client" "sigs.k8s.io/controller-runtime/pkg/reconcile" @@ -45,20 +44,21 @@ const ( MigrationHandlerName = "MigrationHandler" ) -type MigrationHandler struct { - client client.Client - recorder eventrecord.EventRecorderLogger +type VolumeMigrationBackoffServicer interface { + CalculateBackoff(failedCount int) time.Duration +} - backoff map[types.UID]time.Duration - nextTime map[types.UID]time.Time +type MigrationHandler struct { + client client.Client + recorder eventrecord.EventRecorderLogger + backoffSvc VolumeMigrationBackoffServicer } -func NewMigrationHandler(client client.Client, recorder eventrecord.EventRecorderLogger) *MigrationHandler { +func NewMigrationHandler(client client.Client, recorder eventrecord.EventRecorderLogger, backoffSvc VolumeMigrationBackoffServicer) *MigrationHandler { return &MigrationHandler{ - client: client, - recorder: recorder, - backoff: make(map[types.UID]time.Duration), - nextTime: make(map[types.UID]time.Time), + client: client, + recorder: recorder, + backoffSvc: backoffSvc, } } @@ -104,19 +104,10 @@ func (h *MigrationHandler) Handle(ctx context.Context, vd *v1alpha2.VirtualDisk) return reconcile.Result{}, nil } - setBackoff := h.backoff[vm.UID] - calculatedBackoff := h.calculateBackoff(finishedVMOPs, vm.GetCreationTimestamp()) - if calculatedBackoff > setBackoff { - h.backoff[vm.UID] = calculatedBackoff - h.nextTime[vm.UID] = time.Now().Add(calculatedBackoff) - } - - backoff := h.backoff[vm.UID] - nextTime := h.nextTime[vm.UID] - - if nextTime.After(time.Now()) { - h.recorder.Eventf(vd, corev1.EventTypeNormal, v1alpha2.ReasonVolumeMigrationCannotBeProcessed, "VMOP will be created after the backoff. backoff: %q", backoff.String()) - return reconcile.Result{RequeueAfter: backoff}, nil + delay := h.calculateBackoff(finishedVMOPs, vm.GetCreationTimestamp()) + if delay > 0 { + h.recorder.Eventf(vd, corev1.EventTypeNormal, v1alpha2.ReasonVolumeMigrationCannotBeProcessed, "VMOP will be created after the backoff. backoff: %q", delay.String()) + return reconcile.Result{RequeueAfter: delay}, nil } vmop := newVolumeMigrationVMOP(vm.Name, vm.Namespace) @@ -127,9 +118,6 @@ func (h *MigrationHandler) Handle(ctx context.Context, vd *v1alpha2.VirtualDisk) h.recorder.Eventf(vd, corev1.EventTypeNormal, v1alpha2.ReasonVMOPStarted, "Volume migration is started. vmop.name: %q, vmop.namespace: %q", vmop.Name, vmop.Namespace) - delete(h.backoff, vm.UID) - delete(h.nextTime, vm.UID) - return reconcile.Result{}, nil } @@ -186,20 +174,7 @@ func (h *MigrationHandler) calculateBackoff(finishedVMOPs []*v1alpha2.VirtualMac break } - if failedCount == 0 { - return 0 - } - - baseDelay := 5 * time.Second - maxDelay := 5 * time.Minute - - // exponential backoff formula = baseDelay * 2^(failedCount - 1) - backoff := baseDelay * time.Duration(1<<(failedCount-1)) - if backoff > maxDelay { - backoff = maxDelay - } - - return backoff + return h.backoffSvc.CalculateBackoff(failedCount) } func newVolumeMigrationVMOP(vmName, namespace string) *v1alpha2.VirtualMachineOperation { diff --git a/images/virtualization-artifact/pkg/controller/volumemigration/internal/handler/migration_test.go b/images/virtualization-artifact/pkg/controller/volumemigration/internal/handler/migration_test.go index 91afa16bab..d666f71f10 100644 --- a/images/virtualization-artifact/pkg/controller/volumemigration/internal/handler/migration_test.go +++ b/images/virtualization-artifact/pkg/controller/volumemigration/internal/handler/migration_test.go @@ -28,6 +28,7 @@ import ( vmbuilder "github.com/deckhouse/virtualization-controller/pkg/builder/vm" "github.com/deckhouse/virtualization-controller/pkg/common/testutil" + "github.com/deckhouse/virtualization-controller/pkg/controller/service" "github.com/deckhouse/virtualization-controller/pkg/eventrecord" "github.com/deckhouse/virtualization/api/core/v1alpha2" ) @@ -73,7 +74,7 @@ var _ = Describe("TestMigrationHandler", func() { vm := newVM() fakeClient = setupEnvironment(vd, vm) - h := NewMigrationHandler(fakeClient, newEventRecorder()) + h := NewMigrationHandler(fakeClient, newEventRecorder(), service.NewBackoffService(service.WithBaseDelay(5*time.Second))) result, err := h.Handle(ctx, vd) Expect(err).NotTo(HaveOccurred()) @@ -91,7 +92,7 @@ var _ = Describe("TestMigrationHandler", func() { vm := newVM() fakeClient = setupEnvironment(vd, vm) - h := NewMigrationHandler(fakeClient, newEventRecorder()) + h := NewMigrationHandler(fakeClient, newEventRecorder(), service.NewBackoffService(service.WithBaseDelay(5*time.Second))) result, err := h.Handle(ctx, vd) Expect(err).NotTo(HaveOccurred()) @@ -114,7 +115,7 @@ var _ = Describe("TestMigrationHandler", func() { Expect(reason).To(Equal(v1alpha2.ReasonVolumeMigrationCannotBeProcessed)) } - h := NewMigrationHandler(fakeClient, eventRecorder) + h := NewMigrationHandler(fakeClient, eventRecorder, service.NewBackoffService(service.WithBaseDelay(5*time.Second))) result, err := h.Handle(ctx, vd) Expect(err).NotTo(HaveOccurred()) @@ -132,7 +133,7 @@ var _ = Describe("TestMigrationHandler", func() { vm := newVM() fakeClient = setupEnvironment(vd, vm) - h := NewMigrationHandler(fakeClient, newEventRecorder()) + h := NewMigrationHandler(fakeClient, newEventRecorder(), service.NewBackoffService(service.WithBaseDelay(5*time.Second))) result, err := h.Handle(ctx, vd) Expect(err).NotTo(HaveOccurred()) @@ -151,7 +152,7 @@ var _ = Describe("TestMigrationHandler", func() { vmop := newVMOP("volume-migration", v1alpha2.VMOPPhaseInProgress) fakeClient = setupEnvironment(vd, vm, vmop) - h := NewMigrationHandler(fakeClient, newEventRecorder()) + h := NewMigrationHandler(fakeClient, newEventRecorder(), service.NewBackoffService(service.WithBaseDelay(5*time.Second))) result, err := h.Handle(ctx, vd) Expect(err).NotTo(HaveOccurred()) @@ -181,7 +182,7 @@ var _ = Describe("TestMigrationHandler", func() { Expect(messageFmt).To(ContainSubstring("VMOP will be created after the backoff")) } - h := NewMigrationHandler(fakeClient, eventRecorder) + h := NewMigrationHandler(fakeClient, eventRecorder, service.NewBackoffService(service.WithBaseDelay(5*time.Second))) result, err := h.Handle(ctx, vd) Expect(err).NotTo(HaveOccurred()) @@ -213,7 +214,7 @@ var _ = Describe("TestMigrationHandler", func() { Expect(messageFmt).To(ContainSubstring("VMOP will be created after the backoff")) } - h := NewMigrationHandler(fakeClient, eventRecorder) + h := NewMigrationHandler(fakeClient, eventRecorder, service.NewBackoffService(service.WithBaseDelay(5*time.Second))) result, err := h.Handle(ctx, vd) Expect(err).NotTo(HaveOccurred()) @@ -239,7 +240,7 @@ var _ = Describe("TestMigrationHandler", func() { BeforeEach(func() { firstTime = metav1.Now() secondTime = metav1.NewTime(firstTime.Add(time.Second)) - handler = NewMigrationHandler(fakeClient, newEventRecorder()) + handler = NewMigrationHandler(fakeClient, newEventRecorder(), service.NewBackoffService(service.WithBaseDelay(5*time.Second))) }) withCreationTime := func(time metav1.Time, vmops ...*v1alpha2.VirtualMachineOperation) { diff --git a/images/virtualization-artifact/pkg/controller/volumemigration/volumemigration_controller.go b/images/virtualization-artifact/pkg/controller/volumemigration/volumemigration_controller.go index 30e08e9ebe..bb0fa0656b 100644 --- a/images/virtualization-artifact/pkg/controller/volumemigration/volumemigration_controller.go +++ b/images/virtualization-artifact/pkg/controller/volumemigration/volumemigration_controller.go @@ -25,6 +25,7 @@ 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/volumemigration/internal/handler" "github.com/deckhouse/virtualization-controller/pkg/eventrecord" "github.com/deckhouse/virtualization-controller/pkg/featuregates" @@ -45,10 +46,11 @@ func SetupController( } client := mgr.GetClient() + backoffSvc := service.NewBackoffService(service.WithBaseDelay(5 * time.Second)) recorder := eventrecord.NewEventRecorderLogger(mgr, ControllerName) handlers := []Handler{ - handler.NewMigrationHandler(client, recorder), + handler.NewMigrationHandler(client, recorder, backoffSvc), handler.NewCancelHandler(client), } r := NewReconciler(client, handlers) From 59d473a2cd4d71a68b149bd8c3a66f81e803f5ed Mon Sep 17 00:00:00 2001 From: Daniil Antoshin Date: Tue, 3 Mar 2026 15:15:09 +0200 Subject: [PATCH 2/3] fix Signed-off-by: Daniil Antoshin --- .../evacuation/evacuation_controller.go | 4 +--- .../evacuation/internal/handler/evacuation.go | 11 ++++------- .../internal/handler/evacuation_test.go | 5 ++--- .../vm/internal/usb_device_attach_handler.go | 4 ++-- .../internal/usb_device_attach_handler_test.go | 3 +-- .../vm/internal/usb_device_detach_handler.go | 4 ++-- .../internal/usb_device_detach_handler_test.go | 5 ++--- .../pkg/controller/vm/vm_controller.go | 5 ++--- .../internal/handler/migration.go | 11 ++++------- .../internal/handler/migration_test.go | 16 ++++++++-------- .../volumemigration_controller.go | 3 +-- 11 files changed, 29 insertions(+), 42 deletions(-) diff --git a/images/virtualization-artifact/pkg/controller/evacuation/evacuation_controller.go b/images/virtualization-artifact/pkg/controller/evacuation/evacuation_controller.go index e5890da803..058b876cfb 100644 --- a/images/virtualization-artifact/pkg/controller/evacuation/evacuation_controller.go +++ b/images/virtualization-artifact/pkg/controller/evacuation/evacuation_controller.go @@ -26,7 +26,6 @@ import ( "github.com/deckhouse/deckhouse/pkg/log" "github.com/deckhouse/virtualization-controller/pkg/controller/evacuation/internal/handler" - "github.com/deckhouse/virtualization-controller/pkg/controller/service" "github.com/deckhouse/virtualization-controller/pkg/logger" "github.com/deckhouse/virtualization/api/client/kubeclient" ) @@ -42,10 +41,9 @@ func SetupController( log *log.Logger, ) error { client := mgr.GetClient() - backoffSvc := service.NewBackoffService() handlers := []Handler{ - handler.NewEvacuationHandler(client, newCanceler(virtClient), backoffSvc), + handler.NewEvacuationHandler(client, newCanceler(virtClient)), } r := NewReconciler(client, handlers) diff --git a/images/virtualization-artifact/pkg/controller/evacuation/internal/handler/evacuation.go b/images/virtualization-artifact/pkg/controller/evacuation/internal/handler/evacuation.go index e28919199d..568285a0a1 100644 --- a/images/virtualization-artifact/pkg/controller/evacuation/internal/handler/evacuation.go +++ b/images/virtualization-artifact/pkg/controller/evacuation/internal/handler/evacuation.go @@ -32,6 +32,7 @@ import ( "github.com/deckhouse/virtualization-controller/pkg/common/annotations" commonvmop "github.com/deckhouse/virtualization-controller/pkg/common/vmop" "github.com/deckhouse/virtualization-controller/pkg/controller/conditions" + "github.com/deckhouse/virtualization-controller/pkg/controller/service" "github.com/deckhouse/virtualization-controller/pkg/logger" "github.com/deckhouse/virtualization/api/core/v1alpha2" "github.com/deckhouse/virtualization/api/core/v1alpha2/vmcondition" @@ -39,15 +40,11 @@ import ( const nameEvacuationHandler = "EvacuationHandler" -type BackoffServicer interface { - CalculateBackoff(failedCount int) time.Duration -} - -func NewEvacuationHandler(client client.Client, evacuateCanceler EvacuateCanceler, backoffSvc BackoffServicer) *EvacuationHandler { +func NewEvacuationHandler(client client.Client, evacuateCanceler EvacuateCanceler) *EvacuationHandler { return &EvacuationHandler{ client: client, evacuateCanceler: evacuateCanceler, - backoffSvc: backoffSvc, + backoffSvc: service.NewBackoffService(), } } @@ -59,7 +56,7 @@ type EvacuateCanceler interface { type EvacuationHandler struct { client client.Client evacuateCanceler EvacuateCanceler - backoffSvc BackoffServicer + backoffSvc *service.BackoffService } func (h *EvacuationHandler) Handle(ctx context.Context, vm *v1alpha2.VirtualMachine) (reconcile.Result, error) { diff --git a/images/virtualization-artifact/pkg/controller/evacuation/internal/handler/evacuation_test.go b/images/virtualization-artifact/pkg/controller/evacuation/internal/handler/evacuation_test.go index 9036ca7bda..e95c27aad1 100644 --- a/images/virtualization-artifact/pkg/controller/evacuation/internal/handler/evacuation_test.go +++ b/images/virtualization-artifact/pkg/controller/evacuation/internal/handler/evacuation_test.go @@ -30,7 +30,6 @@ import ( vmbuilder "github.com/deckhouse/virtualization-controller/pkg/builder/vm" "github.com/deckhouse/virtualization-controller/pkg/common/annotations" "github.com/deckhouse/virtualization-controller/pkg/common/testutil" - "github.com/deckhouse/virtualization-controller/pkg/controller/service" "github.com/deckhouse/virtualization/api/core/v1alpha2" "github.com/deckhouse/virtualization/api/core/v1alpha2/vmcondition" ) @@ -75,7 +74,7 @@ var _ = Describe("TestEvacuationHandler", func() { h := NewEvacuationHandler(fakeClient, &EvacuateCancelerMock{CancelFunc: func(_ context.Context, _, _ string) error { return nil - }}, service.NewBackoffService()) + }}) _, err := h.Handle(ctx, vm) Expect(err).NotTo(HaveOccurred()) @@ -122,7 +121,7 @@ var _ = Describe("TestEvacuationHandler", func() { vmop.Name = "evacuation-12345" fakeClient = setupEnvironment(newVM(true), vmop) - h := NewEvacuationHandler(fakeClient, canceler, service.NewBackoffService()) + h := NewEvacuationHandler(fakeClient, canceler) err := fakeClient.Delete(ctx, vmop) Expect(err).NotTo(HaveOccurred()) diff --git a/images/virtualization-artifact/pkg/controller/vm/internal/usb_device_attach_handler.go b/images/virtualization-artifact/pkg/controller/vm/internal/usb_device_attach_handler.go index 0a7efce5d0..07e47def2a 100644 --- a/images/virtualization-artifact/pkg/controller/vm/internal/usb_device_attach_handler.go +++ b/images/virtualization-artifact/pkg/controller/vm/internal/usb_device_attach_handler.go @@ -36,13 +36,13 @@ import ( const nameUSBDeviceAttachHandler = "USBDeviceAttachHandler" -func NewUSBDeviceAttachHandler(cl client.Client, virtClient VirtClient, backoffSvc *service.BackoffService) *USBDeviceAttachHandler { +func NewUSBDeviceAttachHandler(cl client.Client, virtClient VirtClient) *USBDeviceAttachHandler { return &USBDeviceAttachHandler{ usbDeviceHandlerBase: usbDeviceHandlerBase{ client: cl, virtClient: virtClient, }, - backoffSvc: backoffSvc, + backoffSvc: service.NewBackoffService(), } } diff --git a/images/virtualization-artifact/pkg/controller/vm/internal/usb_device_attach_handler_test.go b/images/virtualization-artifact/pkg/controller/vm/internal/usb_device_attach_handler_test.go index 2b00265105..6bf8831ba7 100644 --- a/images/virtualization-artifact/pkg/controller/vm/internal/usb_device_attach_handler_test.go +++ b/images/virtualization-artifact/pkg/controller/vm/internal/usb_device_attach_handler_test.go @@ -34,7 +34,6 @@ import ( "sigs.k8s.io/controller-runtime/pkg/reconcile" "github.com/deckhouse/virtualization-controller/pkg/controller/reconciler" - "github.com/deckhouse/virtualization-controller/pkg/controller/service" "github.com/deckhouse/virtualization-controller/pkg/controller/vm/internal/state" "github.com/deckhouse/virtualization-controller/pkg/logger" virtualizationv1alpha2 "github.com/deckhouse/virtualization/api/client/generated/clientset/versioned/typed/core/v1alpha2" @@ -175,7 +174,7 @@ var _ = Describe("USBDeviceAttachHandler", func() { runHandle := func(vm *v1alpha2.VirtualMachine, objs ...client.Object) (reconcile.Result, *reconciler.Resource[*v1alpha2.VirtualMachine, v1alpha2.VirtualMachineStatus], state.VirtualMachineState, error) { fakeClient, vmResource, vmState = setupEnvironment(vm, objs...) - handler = NewUSBDeviceAttachHandler(fakeClient, mockVirtCl, service.NewBackoffService()) + handler = NewUSBDeviceAttachHandler(fakeClient, mockVirtCl) result, err := handler.Handle(ctx, vmState) _ = mockVirtCl.VirtualMachines(vmNamespace) diff --git a/images/virtualization-artifact/pkg/controller/vm/internal/usb_device_detach_handler.go b/images/virtualization-artifact/pkg/controller/vm/internal/usb_device_detach_handler.go index 30c0b0a3b5..063bc3033d 100644 --- a/images/virtualization-artifact/pkg/controller/vm/internal/usb_device_detach_handler.go +++ b/images/virtualization-artifact/pkg/controller/vm/internal/usb_device_detach_handler.go @@ -33,13 +33,13 @@ import ( const nameUSBDeviceDetachHandler = "USBDeviceDetachHandler" -func NewUSBDeviceDetachHandler(cl client.Client, virtClient VirtClient, backoffSvc *service.BackoffService) *USBDeviceDetachHandler { +func NewUSBDeviceDetachHandler(cl client.Client, virtClient VirtClient) *USBDeviceDetachHandler { return &USBDeviceDetachHandler{ usbDeviceHandlerBase: usbDeviceHandlerBase{ client: cl, virtClient: virtClient, }, - backoffSvc: backoffSvc, + backoffSvc: service.NewBackoffService(), } } diff --git a/images/virtualization-artifact/pkg/controller/vm/internal/usb_device_detach_handler_test.go b/images/virtualization-artifact/pkg/controller/vm/internal/usb_device_detach_handler_test.go index 3adbeb5ae7..795db9c278 100644 --- a/images/virtualization-artifact/pkg/controller/vm/internal/usb_device_detach_handler_test.go +++ b/images/virtualization-artifact/pkg/controller/vm/internal/usb_device_detach_handler_test.go @@ -28,7 +28,6 @@ import ( "sigs.k8s.io/controller-runtime/pkg/client" "sigs.k8s.io/controller-runtime/pkg/reconcile" - "github.com/deckhouse/virtualization-controller/pkg/controller/service" "github.com/deckhouse/virtualization-controller/pkg/controller/vm/internal/state" "github.com/deckhouse/virtualization-controller/pkg/logger" "github.com/deckhouse/virtualization/api/core/v1alpha2" @@ -83,7 +82,7 @@ var _ = Describe("USBDeviceDetachHandler", func() { fakeClient, _, st := setupEnvironment(vm, objs...) vmState = st - handler = NewUSBDeviceDetachHandler(fakeClient, mockVirtCl, service.NewBackoffService()) + handler = NewUSBDeviceDetachHandler(fakeClient, mockVirtCl) result, err := handler.Handle(ctx, vmState) Expect(err).NotTo(HaveOccurred()) @@ -115,7 +114,7 @@ var _ = Describe("USBDeviceDetachHandler", func() { fakeClient, _, st := setupEnvironment(vm) vmState = st - handler = NewUSBDeviceDetachHandler(fakeClient, mockVirtCl, service.NewBackoffService()) + handler = NewUSBDeviceDetachHandler(fakeClient, mockVirtCl) mockVM := mockVirtCl.VirtualMachines("default").(*mockVirtualMachines) mockVM.removeResourceClaimErr = removeErr diff --git a/images/virtualization-artifact/pkg/controller/vm/vm_controller.go b/images/virtualization-artifact/pkg/controller/vm/vm_controller.go index 192fefbf96..dd22ee3236 100644 --- a/images/virtualization-artifact/pkg/controller/vm/vm_controller.go +++ b/images/virtualization-artifact/pkg/controller/vm/vm_controller.go @@ -59,7 +59,6 @@ func SetupController( vmClassService := service.NewVirtualMachineClassService(client) migrateVolumesService := vmservice.NewMigrationVolumesService(client, internal.MakeKVVMFromVMSpec, 10*time.Second) - backoffSvc := service.NewBackoffService() handlers := []Handler{ internal.NewMaintenanceHandler(client), @@ -68,8 +67,8 @@ func SetupController( internal.NewIPAMHandler(netmanager.NewIPAM(), client, recorder), internal.NewMACHandler(netmanager.NewMACManager(), client, recorder), internal.NewBlockDeviceHandler(client, blockDeviceService), - internal.NewUSBDeviceDetachHandler(client, virtClient, backoffSvc), - internal.NewUSBDeviceAttachHandler(client, virtClient, backoffSvc), + internal.NewUSBDeviceDetachHandler(client, virtClient), + internal.NewUSBDeviceAttachHandler(client, virtClient), internal.NewProvisioningHandler(client), internal.NewAgentHandler(), internal.NewFilesystemHandler(), diff --git a/images/virtualization-artifact/pkg/controller/volumemigration/internal/handler/migration.go b/images/virtualization-artifact/pkg/controller/volumemigration/internal/handler/migration.go index a5e9b4871a..8eff735b7d 100644 --- a/images/virtualization-artifact/pkg/controller/volumemigration/internal/handler/migration.go +++ b/images/virtualization-artifact/pkg/controller/volumemigration/internal/handler/migration.go @@ -34,6 +34,7 @@ import ( commonvd "github.com/deckhouse/virtualization-controller/pkg/common/vd" commonvmop "github.com/deckhouse/virtualization-controller/pkg/common/vmop" "github.com/deckhouse/virtualization-controller/pkg/controller/conditions" + "github.com/deckhouse/virtualization-controller/pkg/controller/service" "github.com/deckhouse/virtualization-controller/pkg/eventrecord" "github.com/deckhouse/virtualization-controller/pkg/logger" "github.com/deckhouse/virtualization/api/core/v1alpha2" @@ -44,21 +45,17 @@ const ( MigrationHandlerName = "MigrationHandler" ) -type VolumeMigrationBackoffServicer interface { - CalculateBackoff(failedCount int) time.Duration -} - type MigrationHandler struct { client client.Client recorder eventrecord.EventRecorderLogger - backoffSvc VolumeMigrationBackoffServicer + backoffSvc *service.BackoffService } -func NewMigrationHandler(client client.Client, recorder eventrecord.EventRecorderLogger, backoffSvc VolumeMigrationBackoffServicer) *MigrationHandler { +func NewMigrationHandler(client client.Client, recorder eventrecord.EventRecorderLogger) *MigrationHandler { return &MigrationHandler{ client: client, recorder: recorder, - backoffSvc: backoffSvc, + backoffSvc: service.NewBackoffService(), } } diff --git a/images/virtualization-artifact/pkg/controller/volumemigration/internal/handler/migration_test.go b/images/virtualization-artifact/pkg/controller/volumemigration/internal/handler/migration_test.go index d666f71f10..067b0f3e32 100644 --- a/images/virtualization-artifact/pkg/controller/volumemigration/internal/handler/migration_test.go +++ b/images/virtualization-artifact/pkg/controller/volumemigration/internal/handler/migration_test.go @@ -74,7 +74,7 @@ var _ = Describe("TestMigrationHandler", func() { vm := newVM() fakeClient = setupEnvironment(vd, vm) - h := NewMigrationHandler(fakeClient, newEventRecorder(), service.NewBackoffService(service.WithBaseDelay(5*time.Second))) + h := NewMigrationHandler(fakeClient, newEventRecorder(), service.WithBaseDelay(5*time.Second)) result, err := h.Handle(ctx, vd) Expect(err).NotTo(HaveOccurred()) @@ -92,7 +92,7 @@ var _ = Describe("TestMigrationHandler", func() { vm := newVM() fakeClient = setupEnvironment(vd, vm) - h := NewMigrationHandler(fakeClient, newEventRecorder(), service.NewBackoffService(service.WithBaseDelay(5*time.Second))) + h := NewMigrationHandler(fakeClient, newEventRecorder(), service.WithBaseDelay(5*time.Second)) result, err := h.Handle(ctx, vd) Expect(err).NotTo(HaveOccurred()) @@ -115,7 +115,7 @@ var _ = Describe("TestMigrationHandler", func() { Expect(reason).To(Equal(v1alpha2.ReasonVolumeMigrationCannotBeProcessed)) } - h := NewMigrationHandler(fakeClient, eventRecorder, service.NewBackoffService(service.WithBaseDelay(5*time.Second))) + h := NewMigrationHandler(fakeClient, eventRecorder, service.WithBaseDelay(5*time.Second)) result, err := h.Handle(ctx, vd) Expect(err).NotTo(HaveOccurred()) @@ -133,7 +133,7 @@ var _ = Describe("TestMigrationHandler", func() { vm := newVM() fakeClient = setupEnvironment(vd, vm) - h := NewMigrationHandler(fakeClient, newEventRecorder(), service.NewBackoffService(service.WithBaseDelay(5*time.Second))) + h := NewMigrationHandler(fakeClient, newEventRecorder(), service.WithBaseDelay(5*time.Second)) result, err := h.Handle(ctx, vd) Expect(err).NotTo(HaveOccurred()) @@ -152,7 +152,7 @@ var _ = Describe("TestMigrationHandler", func() { vmop := newVMOP("volume-migration", v1alpha2.VMOPPhaseInProgress) fakeClient = setupEnvironment(vd, vm, vmop) - h := NewMigrationHandler(fakeClient, newEventRecorder(), service.NewBackoffService(service.WithBaseDelay(5*time.Second))) + h := NewMigrationHandler(fakeClient, newEventRecorder(), service.WithBaseDelay(5*time.Second)) result, err := h.Handle(ctx, vd) Expect(err).NotTo(HaveOccurred()) @@ -182,7 +182,7 @@ var _ = Describe("TestMigrationHandler", func() { Expect(messageFmt).To(ContainSubstring("VMOP will be created after the backoff")) } - h := NewMigrationHandler(fakeClient, eventRecorder, service.NewBackoffService(service.WithBaseDelay(5*time.Second))) + h := NewMigrationHandler(fakeClient, eventRecorder, service.WithBaseDelay(5*time.Second)) result, err := h.Handle(ctx, vd) Expect(err).NotTo(HaveOccurred()) @@ -214,7 +214,7 @@ var _ = Describe("TestMigrationHandler", func() { Expect(messageFmt).To(ContainSubstring("VMOP will be created after the backoff")) } - h := NewMigrationHandler(fakeClient, eventRecorder, service.NewBackoffService(service.WithBaseDelay(5*time.Second))) + h := NewMigrationHandler(fakeClient, eventRecorder, service.WithBaseDelay(5*time.Second)) result, err := h.Handle(ctx, vd) Expect(err).NotTo(HaveOccurred()) @@ -240,7 +240,7 @@ var _ = Describe("TestMigrationHandler", func() { BeforeEach(func() { firstTime = metav1.Now() secondTime = metav1.NewTime(firstTime.Add(time.Second)) - handler = NewMigrationHandler(fakeClient, newEventRecorder(), service.NewBackoffService(service.WithBaseDelay(5*time.Second))) + handler = NewMigrationHandler(fakeClient, newEventRecorder(), service.WithBaseDelay(5*time.Second)) }) withCreationTime := func(time metav1.Time, vmops ...*v1alpha2.VirtualMachineOperation) { diff --git a/images/virtualization-artifact/pkg/controller/volumemigration/volumemigration_controller.go b/images/virtualization-artifact/pkg/controller/volumemigration/volumemigration_controller.go index bb0fa0656b..dc8b21455c 100644 --- a/images/virtualization-artifact/pkg/controller/volumemigration/volumemigration_controller.go +++ b/images/virtualization-artifact/pkg/controller/volumemigration/volumemigration_controller.go @@ -46,11 +46,10 @@ func SetupController( } client := mgr.GetClient() - backoffSvc := service.NewBackoffService(service.WithBaseDelay(5 * time.Second)) recorder := eventrecord.NewEventRecorderLogger(mgr, ControllerName) handlers := []Handler{ - handler.NewMigrationHandler(client, recorder, backoffSvc), + handler.NewMigrationHandler(client, recorder, service.WithBaseDelay(5*time.Second)), handler.NewCancelHandler(client), } r := NewReconciler(client, handlers) From 3fa2751cb47f0d465bf8b0ac9206b4c595e9fb32 Mon Sep 17 00:00:00 2001 From: Daniil Antoshin Date: Wed, 4 Mar 2026 17:38:46 +0200 Subject: [PATCH 3/3] refactor(core): remove baseDelay parameter from MigrationHandler Remove WithBaseDelay option from MigrationHandler constructor and update tests to use default backoff values (2/4/16 seconds instead of 5/10/40). Signed-off-by: Daniil Antoshin --- .../vm/internal/usb_device_attach_handler.go | 3 ++- .../vm/internal/usb_device_detach_handler.go | 8 +++---- .../internal/handler/migration_test.go | 23 +++++++++---------- .../volumemigration_controller.go | 3 +-- 4 files changed, 18 insertions(+), 19 deletions(-) diff --git a/images/virtualization-artifact/pkg/controller/vm/internal/usb_device_attach_handler.go b/images/virtualization-artifact/pkg/controller/vm/internal/usb_device_attach_handler.go index 07e47def2a..457a571ef4 100644 --- a/images/virtualization-artifact/pkg/controller/vm/internal/usb_device_attach_handler.go +++ b/images/virtualization-artifact/pkg/controller/vm/internal/usb_device_attach_handler.go @@ -165,7 +165,8 @@ func (h *USBDeviceAttachHandler) Handle(ctx context.Context, s state.VirtualMach requestName := h.getResourceClaimRequestName(deviceName) err := h.attachUSBDevice(ctx, vm, deviceName, templateName, requestName) if err != nil && !apierrors.IsAlreadyExists(err) && !strings.Contains(err.Error(), "already exists") { - return reconcile.Result{RequeueAfter: h.backoffSvc.RegisterFailureAndBackoff(vm)}, fmt.Errorf("failed to attach USB device %s: %w", deviceName, err) + log.Error("failed to attach USB device", "error", err, "usbDevice", deviceName) + return reconcile.Result{RequeueAfter: h.backoffSvc.RegisterFailureAndBackoff(vm)}, nil } nextStatusRefs = append(nextStatusRefs, h.buildDetachedStatus(existingStatus, deviceName, isReady)) diff --git a/images/virtualization-artifact/pkg/controller/vm/internal/usb_device_detach_handler.go b/images/virtualization-artifact/pkg/controller/vm/internal/usb_device_detach_handler.go index 063bc3033d..872329acf4 100644 --- a/images/virtualization-artifact/pkg/controller/vm/internal/usb_device_detach_handler.go +++ b/images/virtualization-artifact/pkg/controller/vm/internal/usb_device_detach_handler.go @@ -83,7 +83,7 @@ func (h *USBDeviceDetachHandler) Handle(ctx context.Context, s state.VirtualMach err := h.detachUSBDevice(ctx, vm, existingStatus.Name) if err != nil && !apierrors.IsNotFound(err) && !strings.Contains(err.Error(), "it does not exist") { log.Error("failed to detach USB device", "error", err, "usbDevice", existingStatus.Name) - return reconcile.Result{RequeueAfter: h.backoffSvc.RegisterFailureAndBackoff(vm)}, fmt.Errorf("failed to detach USB device %s: %w", existingStatus.Name, err) + return reconcile.Result{RequeueAfter: h.backoffSvc.RegisterFailureAndBackoff(vm)}, nil } } } @@ -99,7 +99,7 @@ func (h *USBDeviceDetachHandler) Handle(ctx context.Context, s state.VirtualMach err := h.detachUSBDevice(ctx, vm, usbDeviceRef.Name) if err != nil && !apierrors.IsNotFound(err) && !strings.Contains(err.Error(), "it does not exist") { log.Error("failed to detach USB device (device not found)", "error", err, "usbDevice", usbDeviceRef.Name) - return reconcile.Result{RequeueAfter: h.backoffSvc.RegisterFailureAndBackoff(vm)}, fmt.Errorf("failed to detach USB device %s (device not found): %w", usbDeviceRef.Name, err) + return reconcile.Result{RequeueAfter: h.backoffSvc.RegisterFailureAndBackoff(vm)}, nil } continue } @@ -108,7 +108,7 @@ func (h *USBDeviceDetachHandler) Handle(ctx context.Context, s state.VirtualMach err := h.detachUSBDevice(ctx, vm, usbDeviceRef.Name) if err != nil && !apierrors.IsNotFound(err) && !strings.Contains(err.Error(), "it does not exist") { log.Error("failed to detach USB device (device deleting)", "error", err, "usbDevice", usbDeviceRef.Name) - return reconcile.Result{RequeueAfter: h.backoffSvc.RegisterFailureAndBackoff(vm)}, fmt.Errorf("failed to detach USB device %s (device deleting): %w", usbDeviceRef.Name, err) + return reconcile.Result{RequeueAfter: h.backoffSvc.RegisterFailureAndBackoff(vm)}, nil } continue } @@ -117,7 +117,7 @@ func (h *USBDeviceDetachHandler) Handle(ctx context.Context, s state.VirtualMach err := h.detachUSBDevice(ctx, vm, usbDeviceRef.Name) if err != nil && !apierrors.IsNotFound(err) && !strings.Contains(err.Error(), "it does not exist") { log.Error("failed to detach USB device (absent on device)", "error", err, "usbDevice", usbDeviceRef.Name) - return reconcile.Result{RequeueAfter: h.backoffSvc.RegisterFailureAndBackoff(vm)}, fmt.Errorf("failed to detach USB device %s (device not ready): %w", usbDeviceRef.Name, err) + return reconcile.Result{RequeueAfter: h.backoffSvc.RegisterFailureAndBackoff(vm)}, nil } } } diff --git a/images/virtualization-artifact/pkg/controller/volumemigration/internal/handler/migration_test.go b/images/virtualization-artifact/pkg/controller/volumemigration/internal/handler/migration_test.go index 067b0f3e32..dd6d11d9fa 100644 --- a/images/virtualization-artifact/pkg/controller/volumemigration/internal/handler/migration_test.go +++ b/images/virtualization-artifact/pkg/controller/volumemigration/internal/handler/migration_test.go @@ -28,7 +28,6 @@ import ( vmbuilder "github.com/deckhouse/virtualization-controller/pkg/builder/vm" "github.com/deckhouse/virtualization-controller/pkg/common/testutil" - "github.com/deckhouse/virtualization-controller/pkg/controller/service" "github.com/deckhouse/virtualization-controller/pkg/eventrecord" "github.com/deckhouse/virtualization/api/core/v1alpha2" ) @@ -74,7 +73,7 @@ var _ = Describe("TestMigrationHandler", func() { vm := newVM() fakeClient = setupEnvironment(vd, vm) - h := NewMigrationHandler(fakeClient, newEventRecorder(), service.WithBaseDelay(5*time.Second)) + h := NewMigrationHandler(fakeClient, newEventRecorder()) result, err := h.Handle(ctx, vd) Expect(err).NotTo(HaveOccurred()) @@ -92,7 +91,7 @@ var _ = Describe("TestMigrationHandler", func() { vm := newVM() fakeClient = setupEnvironment(vd, vm) - h := NewMigrationHandler(fakeClient, newEventRecorder(), service.WithBaseDelay(5*time.Second)) + h := NewMigrationHandler(fakeClient, newEventRecorder()) result, err := h.Handle(ctx, vd) Expect(err).NotTo(HaveOccurred()) @@ -115,7 +114,7 @@ var _ = Describe("TestMigrationHandler", func() { Expect(reason).To(Equal(v1alpha2.ReasonVolumeMigrationCannotBeProcessed)) } - h := NewMigrationHandler(fakeClient, eventRecorder, service.WithBaseDelay(5*time.Second)) + h := NewMigrationHandler(fakeClient, eventRecorder) result, err := h.Handle(ctx, vd) Expect(err).NotTo(HaveOccurred()) @@ -133,7 +132,7 @@ var _ = Describe("TestMigrationHandler", func() { vm := newVM() fakeClient = setupEnvironment(vd, vm) - h := NewMigrationHandler(fakeClient, newEventRecorder(), service.WithBaseDelay(5*time.Second)) + h := NewMigrationHandler(fakeClient, newEventRecorder()) result, err := h.Handle(ctx, vd) Expect(err).NotTo(HaveOccurred()) @@ -152,7 +151,7 @@ var _ = Describe("TestMigrationHandler", func() { vmop := newVMOP("volume-migration", v1alpha2.VMOPPhaseInProgress) fakeClient = setupEnvironment(vd, vm, vmop) - h := NewMigrationHandler(fakeClient, newEventRecorder(), service.WithBaseDelay(5*time.Second)) + h := NewMigrationHandler(fakeClient, newEventRecorder()) result, err := h.Handle(ctx, vd) Expect(err).NotTo(HaveOccurred()) @@ -182,13 +181,13 @@ var _ = Describe("TestMigrationHandler", func() { Expect(messageFmt).To(ContainSubstring("VMOP will be created after the backoff")) } - h := NewMigrationHandler(fakeClient, eventRecorder, service.WithBaseDelay(5*time.Second)) + h := NewMigrationHandler(fakeClient, eventRecorder) result, err := h.Handle(ctx, vd) Expect(err).NotTo(HaveOccurred()) //nolint:staticcheck // check requeue is not used Expect(result.Requeue).To(BeFalse()) - Expect(result.RequeueAfter).To(Equal(5 * time.Second)) + Expect(result.RequeueAfter).To(Equal(2 * time.Second)) // Check that no new VMOP was created vmopList := &v1alpha2.VirtualMachineOperationList{} @@ -214,13 +213,13 @@ var _ = Describe("TestMigrationHandler", func() { Expect(messageFmt).To(ContainSubstring("VMOP will be created after the backoff")) } - h := NewMigrationHandler(fakeClient, eventRecorder, service.WithBaseDelay(5*time.Second)) + h := NewMigrationHandler(fakeClient, eventRecorder) result, err := h.Handle(ctx, vd) Expect(err).NotTo(HaveOccurred()) //nolint:staticcheck // check requeue is not used Expect(result.Requeue).To(BeFalse()) - Expect(result.RequeueAfter).To(Equal(10 * time.Second)) + Expect(result.RequeueAfter).To(Equal(4 * time.Second)) // Check that no new VMOP was created vmopList := &v1alpha2.VirtualMachineOperationList{} @@ -240,7 +239,7 @@ var _ = Describe("TestMigrationHandler", func() { BeforeEach(func() { firstTime = metav1.Now() secondTime = metav1.NewTime(firstTime.Add(time.Second)) - handler = NewMigrationHandler(fakeClient, newEventRecorder(), service.WithBaseDelay(5*time.Second)) + handler = NewMigrationHandler(fakeClient, newEventRecorder()) }) withCreationTime := func(time metav1.Time, vmops ...*v1alpha2.VirtualMachineOperation) { @@ -273,7 +272,7 @@ var _ = Describe("TestMigrationHandler", func() { } withCreationTime(secondTime, vmops...) backoff := handler.calculateBackoff(vmops, firstTime) - Expect(backoff).To(Equal(40 * time.Second)) + Expect(backoff).To(Equal(16 * time.Second)) }) It("should cap backoff at maximum delay", func() { diff --git a/images/virtualization-artifact/pkg/controller/volumemigration/volumemigration_controller.go b/images/virtualization-artifact/pkg/controller/volumemigration/volumemigration_controller.go index dc8b21455c..30e08e9ebe 100644 --- a/images/virtualization-artifact/pkg/controller/volumemigration/volumemigration_controller.go +++ b/images/virtualization-artifact/pkg/controller/volumemigration/volumemigration_controller.go @@ -25,7 +25,6 @@ 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/volumemigration/internal/handler" "github.com/deckhouse/virtualization-controller/pkg/eventrecord" "github.com/deckhouse/virtualization-controller/pkg/featuregates" @@ -49,7 +48,7 @@ func SetupController( recorder := eventrecord.NewEventRecorderLogger(mgr, ControllerName) handlers := []Handler{ - handler.NewMigrationHandler(client, recorder, service.WithBaseDelay(5*time.Second)), + handler.NewMigrationHandler(client, recorder), handler.NewCancelHandler(client), } r := NewReconciler(client, handlers)