From 4928b16e2846f5defdf7621144ef75cdf3f90783 Mon Sep 17 00:00:00 2001 From: Fabian Wiesel Date: Thu, 9 Jul 2026 09:49:06 +0200 Subject: [PATCH 1/2] Extract registerWithManager from SetupWithManager for testability Split SetupWithManager into two methods for controllers with OpenStack dependencies: registerWithManager handles only the controller-runtime registration, while SetupWithManager acquires OpenStack clients first then delegates to registerWithManager. This allows integration tests to inject fake OpenStack clients and call registerWithManager directly. onboarding_controller is handled in a separate commit because it has more moving parts (additional helpers, requeueInterval). --- internal/controller/aggregates_controller.go | 14 ++++++++++---- internal/controller/eviction_controller.go | 14 ++++++++++---- .../hypervisor_maintenance_controller.go | 16 +++++++++++----- internal/controller/offboarding_controller.go | 14 ++++++++++---- internal/controller/traits_controller.go | 14 ++++++++++---- 5 files changed, 51 insertions(+), 21 deletions(-) diff --git a/internal/controller/aggregates_controller.go b/internal/controller/aggregates_controller.go index 7490a4ab..30aa47e1 100644 --- a/internal/controller/aggregates_controller.go +++ b/internal/controller/aggregates_controller.go @@ -217,6 +217,15 @@ func slicesEqualUnordered(a, b []string) bool { return true } +// registerWithManager registers the controller with the Manager without acquiring OpenStack clients. +// This is useful for testing where clients are injected directly. +func (ac *AggregatesController) registerWithManager(mgr ctrl.Manager) error { + return ctrl.NewControllerManagedBy(mgr). + Named(AggregatesControllerName). + For(&kvmv1.Hypervisor{}, builder.WithPredicates(utils.LifecycleEnabledPredicate)). + Complete(ac) +} + // SetupWithManager sets up the controller with the Manager. func (ac *AggregatesController) SetupWithManager(mgr ctrl.Manager) error { ctx := context.Background() @@ -226,8 +235,5 @@ func (ac *AggregatesController) SetupWithManager(mgr ctrl.Manager) error { return err } - return ctrl.NewControllerManagedBy(mgr). - Named(AggregatesControllerName). - For(&kvmv1.Hypervisor{}, builder.WithPredicates(utils.LifecycleEnabledPredicate)). - Complete(ac) + return ac.registerWithManager(mgr) } diff --git a/internal/controller/eviction_controller.go b/internal/controller/eviction_controller.go index 4a766292..b0fa33c9 100644 --- a/internal/controller/eviction_controller.go +++ b/internal/controller/eviction_controller.go @@ -442,6 +442,15 @@ func (r *EvictionReconciler) coldMigrate(ctx context.Context, uuid string, evict return nil } +// registerWithManager registers the controller with the Manager without acquiring OpenStack clients. +// This is useful for testing where clients are injected directly. +func (r *EvictionReconciler) registerWithManager(mgr ctrl.Manager) error { + return ctrl.NewControllerManagedBy(mgr). + Named(EvictionControllerName). + For(&kvmv1.Eviction{}). + Complete(r) +} + // SetupWithManager sets up the controller with the Manager. func (r *EvictionReconciler) SetupWithManager(mgr ctrl.Manager) error { ctx := context.Background() @@ -452,8 +461,5 @@ func (r *EvictionReconciler) SetupWithManager(mgr ctrl.Manager) error { } r.computeClient.Microversion = "2.90" // Xena (or later) - return ctrl.NewControllerManagedBy(mgr). - Named(EvictionControllerName). - For(&kvmv1.Eviction{}). - Complete(r) + return r.registerWithManager(mgr) } diff --git a/internal/controller/hypervisor_maintenance_controller.go b/internal/controller/hypervisor_maintenance_controller.go index 2d77cb0b..5c824878 100644 --- a/internal/controller/hypervisor_maintenance_controller.go +++ b/internal/controller/hypervisor_maintenance_controller.go @@ -247,6 +247,16 @@ func (hec *HypervisorMaintenanceController) ensureEviction(ctx context.Context, } } +// registerWithManager registers the controller with the Manager without acquiring OpenStack clients. +// This is useful for testing where clients are injected directly. +func (hec *HypervisorMaintenanceController) registerWithManager(mgr ctrl.Manager) error { + return ctrl.NewControllerManagedBy(mgr). + Named(HypervisorMaintenanceControllerName). + For(&kvmv1.Hypervisor{}). + Owns(&kvmv1.Eviction{}). // trigger Reconcile whenever an Own-ed eviction is created/updated/deleted + Complete(hec) +} + // SetupWithManager sets up the controller with the Manager. func (hec *HypervisorMaintenanceController) SetupWithManager(mgr ctrl.Manager) error { ctx := context.Background() @@ -257,9 +267,5 @@ func (hec *HypervisorMaintenanceController) SetupWithManager(mgr ctrl.Manager) e } hec.computeClient.Microversion = "2.90" // Xena (or later) - return ctrl.NewControllerManagedBy(mgr). - Named(HypervisorMaintenanceControllerName). - For(&kvmv1.Hypervisor{}). - Owns(&kvmv1.Eviction{}). // trigger Reconcile whenever an Own-ed eviction is created/updated/deleted - Complete(hec) + return hec.registerWithManager(mgr) } diff --git a/internal/controller/offboarding_controller.go b/internal/controller/offboarding_controller.go index 39ee92b5..eee2cfe9 100644 --- a/internal/controller/offboarding_controller.go +++ b/internal/controller/offboarding_controller.go @@ -187,6 +187,15 @@ func (r *HypervisorOffboardingReconciler) markOffboarded(ctx context.Context, hv return nil } +// registerWithManager registers the controller with the Manager without acquiring OpenStack clients. +// This is useful for testing where clients are injected directly. +func (r *HypervisorOffboardingReconciler) registerWithManager(mgr ctrl.Manager) error { + return ctrl.NewControllerManagedBy(mgr). + Named(OffboardingControllerName). + For(&kvmv1.Hypervisor{}). + Complete(r) +} + // SetupWithManager sets up the controller with the Manager. func (r *HypervisorOffboardingReconciler) SetupWithManager(mgr ctrl.Manager) error { ctx := context.Background() @@ -204,8 +213,5 @@ func (r *HypervisorOffboardingReconciler) SetupWithManager(mgr ctrl.Manager) err } r.placementClient.Microversion = "1.39" // yoga, or later - return ctrl.NewControllerManagedBy(mgr). - Named(OffboardingControllerName). - For(&kvmv1.Hypervisor{}). - Complete(r) + return r.registerWithManager(mgr) } diff --git a/internal/controller/traits_controller.go b/internal/controller/traits_controller.go index 6da55b84..4e91e219 100644 --- a/internal/controller/traits_controller.go +++ b/internal/controller/traits_controller.go @@ -176,6 +176,15 @@ func getTraitCondition(err error, msg string) metav1.Condition { } } +// registerWithManager registers the controller with the Manager without acquiring OpenStack clients. +// This is useful for testing where clients are injected directly. +func (tc *TraitsController) registerWithManager(mgr ctrl.Manager) error { + return ctrl.NewControllerManagedBy(mgr). + Named(TraitsControllerName). + For(&kvmv1.Hypervisor{}). + Complete(tc) +} + // SetupWithManager sets up the controller with the Manager. func (tc *TraitsController) SetupWithManager(mgr ctrl.Manager) error { ctx := context.Background() @@ -186,8 +195,5 @@ func (tc *TraitsController) SetupWithManager(mgr ctrl.Manager) error { } tc.serviceClient.Microversion = "1.39" // yoga, or later - return ctrl.NewControllerManagedBy(mgr). - Named(TraitsControllerName). - For(&kvmv1.Hypervisor{}). - Complete(tc) + return tc.registerWithManager(mgr) } From e43ceab679cb6b9cf2e6078196edc303141d4b8b Mon Sep 17 00:00:00 2001 From: Fabian Wiesel Date: Thu, 9 Jul 2026 09:49:12 +0200 Subject: [PATCH 2/2] Make OnboardingController testable with injectable clients and requeue interval Extract registerWithManager from SetupWithManager and add a configurable requeueInterval field (defaults to 1m via getRequeueInterval). Integration tests set requeueInterval to 1s, reducing test runtime from minutes to seconds. --- internal/controller/onboarding_controller.go | 38 +++++++++++++------- 1 file changed, 26 insertions(+), 12 deletions(-) diff --git a/internal/controller/onboarding_controller.go b/internal/controller/onboarding_controller.go index 7e9fed71..1f52d0e7 100644 --- a/internal/controller/onboarding_controller.go +++ b/internal/controller/onboarding_controller.go @@ -71,6 +71,14 @@ type OnboardingController struct { testComputeClient *gophercloud.ServiceClient testImageClient *gophercloud.ServiceClient testNetworkClient *gophercloud.ServiceClient + requeueInterval time.Duration +} + +func (r *OnboardingController) getRequeueInterval() time.Duration { + if r.requeueInterval > 0 { + return r.requeueInterval + } + return defaultWaitTime } // +kubebuilder:rbac:groups=kvm.cloud.sap,resources=hypervisors,verbs=get;list;watch;patch @@ -101,7 +109,7 @@ func (r *OnboardingController) Reconcile(ctx context.Context, req ctrl.Request) if hv.Status.HypervisorID == "" || hv.Status.ServiceID == "" { if err := r.ensureNovaProperties(ctx, hv); err != nil { if errors.Is(err, errRequeue) { - return ctrl.Result{RequeueAfter: defaultWaitTime}, nil + return ctrl.Result{RequeueAfter: r.getRequeueInterval()}, nil } return ctrl.Result{}, err } @@ -239,7 +247,7 @@ func (r *OnboardingController) smokeTest(ctx context.Context, hv *kvmv1.Hypervis if err != nil { // should not happened log.Error(err, "failed to get test instance, instance vanished", "id", id) - return ctrl.Result{RequeueAfter: defaultWaitTime}, nil + return ctrl.Result{RequeueAfter: r.getRequeueInterval()}, nil } // Set condition back to testing @@ -260,7 +268,7 @@ func (r *OnboardingController) smokeTest(ctx context.Context, hv *kvmv1.Hypervis log.Error(err, "failed to delete test instance", "id", id) } - return ctrl.Result{RequeueAfter: defaultWaitTime}, nil + return ctrl.Result{RequeueAfter: r.getRequeueInterval()}, nil case "ACTIVE": consoleOutput, err := servers. ShowConsoleOutput(ctx, r.testComputeClient, server.ID, servers.ShowConsoleOutputOpts{Length: 11}). @@ -277,7 +285,7 @@ func (r *OnboardingController) smokeTest(ctx context.Context, hv *kvmv1.Hypervis }); err != nil { return ctrl.Result{}, err } - return ctrl.Result{RequeueAfter: defaultWaitTime}, nil + return ctrl.Result{RequeueAfter: r.getRequeueInterval()}, nil } if !strings.Contains(consoleOutput, server.Name) { @@ -299,7 +307,7 @@ func (r *OnboardingController) smokeTest(ctx context.Context, hv *kvmv1.Hypervis } } } - return ctrl.Result{RequeueAfter: defaultWaitTime}, nil + return ctrl.Result{RequeueAfter: r.getRequeueInterval()}, nil } if err = servers.Delete(ctx, r.testComputeClient, server.ID).ExtractErr(); err != nil { @@ -314,12 +322,12 @@ func (r *OnboardingController) smokeTest(ctx context.Context, hv *kvmv1.Hypervis }); err != nil { return ctrl.Result{}, err } - return ctrl.Result{RequeueAfter: defaultWaitTime}, nil + return ctrl.Result{RequeueAfter: r.getRequeueInterval()}, nil } return r.completeOnboarding(ctx, host, hv) default: - return ctrl.Result{RequeueAfter: defaultWaitTime}, nil + return ctrl.Result{RequeueAfter: r.getRequeueInterval()}, nil } } @@ -332,7 +340,7 @@ func (r *OnboardingController) completeOnboarding(ctx context.Context, host stri return ctrl.Result{}, nil } if !meta.IsStatusConditionTrue(hv.Status.Conditions, kvmv1.ConditionTypeTraitsUpdated) { - return ctrl.Result{RequeueAfter: defaultWaitTime}, nil + return ctrl.Result{RequeueAfter: r.getRequeueInterval()}, nil } // Wait for HypervisorInstanceHa controller to enable HA @@ -582,6 +590,15 @@ func (r *OnboardingController) findTestImage(ctx context.Context) (string, error return "", fmt.Errorf("couldn't find image with name %v", testImageName) } +// registerWithManager registers the controller with the Manager without acquiring OpenStack clients. +// This is useful for testing where clients are injected directly. +func (r *OnboardingController) registerWithManager(mgr ctrl.Manager) error { + return ctrl.NewControllerManagedBy(mgr). + Named(OnboardingControllerName). + For(&kvmv1.Hypervisor{}). + Complete(r) +} + // SetupWithManager sets up the controller with the Manager. func (r *OnboardingController) SetupWithManager(mgr ctrl.Manager) error { if r.TestFlavorID == "" { @@ -621,8 +638,5 @@ func (r *OnboardingController) SetupWithManager(mgr ctrl.Manager) error { r.Clock = clock.RealClock{} } - return ctrl.NewControllerManagedBy(mgr). - Named(OnboardingControllerName). - For(&kvmv1.Hypervisor{}). - Complete(r) + return r.registerWithManager(mgr) }