diff --git a/cmd/main.go b/cmd/main.go index fecb7318..e0482b4f 100644 --- a/cmd/main.go +++ b/cmd/main.go @@ -101,20 +101,10 @@ func main() { flag.StringVar(&certificateIssuerName, "certificate-issuer-name", "nova-hypervisor-agents-ca-issuer", "Name of the certificate issuer.") - if certificateIssuerName == "" { - setupLog.Error(errors.New("certificate-issuer-name cannot be empty"), "invalid certificate issuer name") - os.Exit(1) - } - - if certificateNamespace == "" { - setupLog.Error(errors.New("certificate-namespace cannot be empty"), "invalid certificate namespace") - os.Exit(1) - } - opts := ctrlzap.Options{ Development: true, TimeEncoder: zapcore.ISO8601TimeEncoder, - Encoder: logger.NewSanitzeReconcileErrorEncoder(zap.NewDevelopmentEncoderConfig()), + Encoder: logger.NewSanitizeReconcileErrorEncoder(zap.NewDevelopmentEncoderConfig()), StacktraceLevel: zap.DPanicLevel, } @@ -122,9 +112,22 @@ func main() { flag.Parse() if version { + fmt.Printf("%s %s (%s/%s) %s\n", + bininfo.Component(), bininfo.VersionOr("devel"), gruntime.GOOS, gruntime.GOARCH, + bininfo.CommitOr("edge")) os.Exit(0) } + if certificateIssuerName == "" { + setupLog.Error(errors.New("certificate-issuer-name cannot be empty"), "invalid certificate issuer name") + os.Exit(1) + } + + if certificateNamespace == "" { + setupLog.Error(errors.New("certificate-namespace cannot be empty"), "invalid certificate namespace") + os.Exit(1) + } + ctrl.SetLogger(ctrlzap.New(ctrlzap.UseFlagOptions(&opts))) // if the enable-http2 flag is false (the default), http/2 should be disabled diff --git a/config/rbac/role.yaml b/config/rbac/role.yaml index 528d2cbd..27fd80cb 100644 --- a/config/rbac/role.yaml +++ b/config/rbac/role.yaml @@ -48,7 +48,6 @@ rules: resources: - evictions - hypervisors - - hypervisors/status verbs: - create - delete @@ -71,6 +70,16 @@ rules: - get - patch - update +- apiGroups: + - kvm.cloud.sap + resources: + - hypervisors/status + verbs: + - get + - list + - patch + - update + - watch - apiGroups: - policy resources: diff --git a/internal/controller/aggregates_controller.go b/internal/controller/aggregates_controller.go index 0072ce6c..7490a4ab 100644 --- a/internal/controller/aggregates_controller.go +++ b/internal/controller/aggregates_controller.go @@ -29,7 +29,6 @@ import ( ctrl "sigs.k8s.io/controller-runtime" "sigs.k8s.io/controller-runtime/pkg/builder" k8sclient "sigs.k8s.io/controller-runtime/pkg/client" - logger "sigs.k8s.io/controller-runtime/pkg/log" "github.com/gophercloud/gophercloud/v2" @@ -49,7 +48,7 @@ type AggregatesController struct { } // +kubebuilder:rbac:groups=kvm.cloud.sap,resources=hypervisors,verbs=get;list;watch -// +kubebuilder:rbac:groups=kvm.cloud.sap,resources=hypervisors/status,verbs=get;list;watch;create;update;patch;delete +// +kubebuilder:rbac:groups=kvm.cloud.sap,resources=hypervisors/status,verbs=get;update;patch func (ac *AggregatesController) Reconcile(ctx context.Context, req ctrl.Request) (ctrl.Result, error) { hv := &kvmv1.Hypervisor{} @@ -221,7 +220,6 @@ func slicesEqualUnordered(a, b []string) bool { // SetupWithManager sets up the controller with the Manager. func (ac *AggregatesController) SetupWithManager(mgr ctrl.Manager) error { ctx := context.Background() - _ = logger.FromContext(ctx) var err error if ac.computeClient, err = openstack.GetServiceClient(ctx, "compute", nil); err != nil { diff --git a/internal/controller/constants.go b/internal/controller/constants.go index 5151bd28..f4119dc6 100644 --- a/internal/controller/constants.go +++ b/internal/controller/constants.go @@ -19,8 +19,6 @@ package controller // This should contain constants shared between controllers const ( - labelEvictionRequired = "cloud.sap/hypervisor-eviction-required" - labelEvictionApproved = "cloud.sap/hypervisor-eviction-succeeded" - labelHypervisor = "nova.openstack.cloud.sap/virt-driver" - testAggregateName = "tenant_filter_tests" + labelHypervisor = "nova.openstack.cloud.sap/virt-driver" + testAggregateName = "tenant_filter_tests" ) diff --git a/internal/controller/eviction_controller.go b/internal/controller/eviction_controller.go index d2e38981..ae502fa1 100644 --- a/internal/controller/eviction_controller.go +++ b/internal/controller/eviction_controller.go @@ -32,12 +32,14 @@ import ( metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/runtime" "k8s.io/apimachinery/pkg/types" + "k8s.io/client-go/util/retry" ctrl "sigs.k8s.io/controller-runtime" "sigs.k8s.io/controller-runtime/pkg/client" logger "sigs.k8s.io/controller-runtime/pkg/log" kvmv1 "github.com/cobaltcore-dev/openstack-hypervisor-operator/api/v1" "github.com/cobaltcore-dev/openstack-hypervisor-operator/internal/openstack" + "github.com/cobaltcore-dev/openstack-hypervisor-operator/internal/utils" ) // EvictionReconciler reconciles a Eviction object @@ -116,7 +118,6 @@ func (r *EvictionReconciler) Reconcile(ctx context.Context, req ctrl.Request) (c statusCondition := meta.FindStatusCondition(eviction.Status.Conditions, kvmv1.ConditionTypeEvicting) if statusCondition == nil { - base := eviction.DeepCopy() meta.SetStatusCondition(&eviction.Status.Conditions, metav1.Condition{ Type: kvmv1.ConditionTypeEvicting, Status: metav1.ConditionTrue, @@ -124,7 +125,7 @@ func (r *EvictionReconciler) Reconcile(ctx context.Context, req ctrl.Request) (c Reason: kvmv1.ConditionReasonRunning, }) - return ctrl.Result{}, r.updateStatus(ctx, eviction, base) + return ctrl.Result{}, r.updateStatus(ctx, eviction) } switch statusCondition.Status { @@ -156,7 +157,6 @@ func (r *EvictionReconciler) handleRunning(ctx context.Context, eviction *kvmv1. return r.evictNext(ctx, eviction) } - base := eviction.DeepCopy() meta.SetStatusCondition(&eviction.Status.Conditions, metav1.Condition{ Type: kvmv1.ConditionTypeEvicting, Status: metav1.ConditionFalse, @@ -166,16 +166,30 @@ func (r *EvictionReconciler) handleRunning(ctx context.Context, eviction *kvmv1. eviction.Status.OutstandingRamMb = 0 logger.FromContext(ctx).Info("succeeded") - return ctrl.Result{}, r.updateStatus(ctx, eviction, base) + return ctrl.Result{}, r.updateStatus(ctx, eviction) } -func (r *EvictionReconciler) updateStatus(ctx context.Context, eviction, base *kvmv1.Eviction) error { - return r.Status().Patch(ctx, eviction, client.MergeFromWithOptions(base, - client.MergeFromWithOptimisticLock{}), client.FieldOwner(EvictionControllerName)) +func (r *EvictionReconciler) updateStatus(ctx context.Context, eviction *kvmv1.Eviction) error { + desiredStatus := eviction.Status.DeepCopy() + return retry.RetryOnConflict(utils.StatusPatchBackoff, func() error { + freshEviction := &kvmv1.Eviction{} + if err := r.Get(ctx, client.ObjectKeyFromObject(eviction), freshEviction); err != nil { + return err + } + freshBase := freshEviction.DeepCopy() + // Apply desired conditions and scalar fields onto the fresh status + for _, c := range desiredStatus.Conditions { + meta.SetStatusCondition(&freshEviction.Status.Conditions, c) + } + freshEviction.Status.OutstandingInstances = desiredStatus.OutstandingInstances + freshEviction.Status.OutstandingRamMb = desiredStatus.OutstandingRamMb + freshEviction.Status.HypervisorServiceId = desiredStatus.HypervisorServiceId + return r.Status().Patch(ctx, freshEviction, client.MergeFromWithOptions(freshBase, + client.MergeFromWithOptimisticLock{}), client.FieldOwner(EvictionControllerName)) + }) } func (r *EvictionReconciler) handlePreflight(ctx context.Context, eviction *kvmv1.Eviction, hv *kvmv1.Hypervisor) (ctrl.Result, error) { - base := eviction.DeepCopy() expectHypervisor := hv.Status.HypervisorID != "" && hv.Status.ServiceID != "" // The hypervisor has been registered // If the hypervisor should exist, then we need to ensure it is disabled before we start evicting @@ -187,7 +201,7 @@ func (r *EvictionReconciler) handlePreflight(ctx context.Context, eviction *kvmv Message: "hypervisor not disabled", Reason: kvmv1.ConditionReasonFailed, }) { - return ctrl.Result{}, r.updateStatus(ctx, eviction, base) + return ctrl.Result{}, r.updateStatus(ctx, eviction) } return ctrl.Result{RequeueAfter: defaultPollTime}, nil // Wait for hypervisor to be disabled } @@ -208,7 +222,7 @@ func (r *EvictionReconciler) handlePreflight(ctx context.Context, eviction *kvmv Message: fmt.Sprintf("failed to get hypervisor %v", err), Reason: kvmv1.ConditionReasonFailed, }) - return ctrl.Result{}, r.updateStatus(ctx, eviction, base) + return ctrl.Result{}, r.updateStatus(ctx, eviction) } else { // That is (likely) an eviction for a node that never registered // so we are good to go @@ -221,7 +235,7 @@ func (r *EvictionReconciler) handlePreflight(ctx context.Context, eviction *kvmv }) eviction.Status.OutstandingRamMb = 0 logger.FromContext(ctx).Info(msg) - return ctrl.Result{}, r.updateStatus(ctx, eviction, base) + return ctrl.Result{}, r.updateStatus(ctx, eviction) } } @@ -242,18 +256,15 @@ func (r *EvictionReconciler) handlePreflight(ctx context.Context, eviction *kvmv Message: "Preflight checks passed, hypervisor is disabled and ready for eviction", Reason: kvmv1.ConditionReasonSucceeded, }) - return ctrl.Result{}, r.updateStatus(ctx, eviction, base) + return ctrl.Result{}, r.updateStatus(ctx, eviction) } // Tries to handle the NotFound-error by updating the status -func (r *EvictionReconciler) handleNotFound(ctx context.Context, eviction, base *kvmv1.Eviction, err error) error { +func (r *EvictionReconciler) handleNotFound(ctx context.Context, eviction *kvmv1.Eviction, err error) error { if !gophercloud.ResponseCodeIs(err, http.StatusNotFound) { return err } logger.FromContext(ctx).Info("Instance is gone") - if base == nil { - base = eviction.DeepCopy() - } var uuid string eviction.Status.OutstandingInstances, uuid = popInstance(eviction.Status.OutstandingInstances) if uuid == "" { @@ -265,11 +276,10 @@ func (r *EvictionReconciler) handleNotFound(ctx context.Context, eviction, base Message: fmt.Sprintf("Instance %s is gone", uuid), Reason: kvmv1.ConditionReasonSucceeded, }) - return r.updateStatus(ctx, eviction, base) + return r.updateStatus(ctx, eviction) } func (r *EvictionReconciler) evictNext(ctx context.Context, eviction *kvmv1.Eviction) (ctrl.Result, error) { - base := eviction.DeepCopy() uuid := peekInstance(eviction.Status.OutstandingInstances) if uuid == "" { return ctrl.Result{}, nil @@ -281,7 +291,7 @@ func (r *EvictionReconciler) evictNext(ctx context.Context, eviction *kvmv1.Evic vm, err := res.Extract() if err != nil { - if err2 := r.handleNotFound(ctx, eviction, base, err); err2 != nil { + if err2 := r.handleNotFound(ctx, eviction, err); err2 != nil { return ctrl.Result{}, err2 } else { return ctrl.Result{RequeueAfter: shortRetryTime}, nil @@ -309,7 +319,7 @@ func (r *EvictionReconciler) evictNext(ctx context.Context, eviction *kvmv1.Evic }) return ctrl.Result{}, errors.Join(fmt.Errorf("error migrating instance %v", uuid), - r.updateStatus(ctx, eviction, base)) + r.updateStatus(ctx, eviction)) } currentHypervisor, _, _ := strings.Cut(vm.HypervisorHostname, ".") @@ -326,7 +336,7 @@ func (r *EvictionReconciler) evictNext(ctx context.Context, eviction *kvmv1.Evic // So, it is already off this one, do we need to verify it? if vm.Status == "VERIFY_RESIZE" { err := servers.ConfirmResize(ctx, r.computeClient, vm.ID).ExtractErr() - if err2 := r.handleNotFound(ctx, eviction, base, err); err2 != nil { + if err2 := r.handleNotFound(ctx, eviction, err); err2 != nil { // Retry confirm in next reconciliation return ctrl.Result{}, err2 } else { @@ -337,7 +347,7 @@ func (r *EvictionReconciler) evictNext(ctx context.Context, eviction *kvmv1.Evic // All done eviction.Status.OutstandingInstances, _ = popInstance(eviction.Status.OutstandingInstances) - return ctrl.Result{}, r.updateStatus(ctx, eviction, base) + return ctrl.Result{}, r.updateStatus(ctx, eviction) } if vm.TaskState == "deleting" { //nolint:gocritic @@ -350,35 +360,32 @@ func (r *EvictionReconciler) evictNext(ctx context.Context, eviction *kvmv1.Evic Message: fmt.Sprintf("Live migration of terminating instance %s skipped", vm.ID), Reason: kvmv1.ConditionReasonFailed, }) - if err2 := r.updateStatus(ctx, eviction, base); err2 != nil { - return ctrl.Result{}, fmt.Errorf("could update status due to %w", err2) + if err := r.updateStatus(ctx, eviction); err != nil { + return ctrl.Result{}, fmt.Errorf("could not update status due to %w", err) } + return ctrl.Result{RequeueAfter: defaultPollTime}, nil } else if vm.Status == "ACTIVE" || vm.PowerState == 1 { log.Info("trigger live-migration") - err := r.liveMigrate(ctx, vm.ID, eviction) - if err != nil { - if err2 := r.handleNotFound(ctx, eviction, base, err); err2 != nil { + if err := r.liveMigrate(ctx, vm.ID, eviction); err != nil { + if err2 := r.handleNotFound(ctx, eviction, err); err2 != nil { return ctrl.Result{}, err2 - } else { - return ctrl.Result{RequeueAfter: shortRetryTime}, nil } + return ctrl.Result{RequeueAfter: shortRetryTime}, nil } } else { log.Info("trigger cold-migration") - err := r.coldMigrate(ctx, vm.ID, eviction) - if err != nil { - if err2 := r.handleNotFound(ctx, eviction, base, err); err2 != nil { + if err := r.coldMigrate(ctx, vm.ID, eviction); err != nil { + if err2 := r.handleNotFound(ctx, eviction, err); err2 != nil { return ctrl.Result{}, err2 - } else { - return ctrl.Result{RequeueAfter: shortRetryTime}, nil } + return ctrl.Result{RequeueAfter: shortRetryTime}, nil } } // Triggered a migration, give it a generous time to start, so we do not // see the old state because the migration didn't start log.Info("poll") - return ctrl.Result{RequeueAfter: defaultPollTime}, err + return ctrl.Result{RequeueAfter: defaultPollTime}, nil } func (r *EvictionReconciler) liveMigrate(ctx context.Context, uuid string, eviction *kvmv1.Eviction) error { diff --git a/internal/controller/gardener_node_lifecycle_controller.go b/internal/controller/gardener_node_lifecycle_controller.go index a59ed9b2..9966651c 100644 --- a/internal/controller/gardener_node_lifecycle_controller.go +++ b/internal/controller/gardener_node_lifecycle_controller.go @@ -72,8 +72,12 @@ func (r *GardenerNodeLifecycleController) Reconcile(ctx context.Context, req ctr } hv := &kvmv1.Hypervisor{} - if err := r.Get(ctx, k8sclient.ObjectKey{Name: req.Name}, hv); k8sclient.IgnoreNotFound(err) != nil { - return ctrl.Result{}, err + if err := r.Get(ctx, k8sclient.ObjectKey{Name: req.Name}, hv); err != nil { + if k8sclient.IgnoreNotFound(err) != nil { + return ctrl.Result{}, err + } + // Hypervisor not found, nothing to do + return ctrl.Result{}, nil } if !hv.Spec.LifecycleEnabled { @@ -209,7 +213,7 @@ func (r *GardenerNodeLifecycleController) ensureSignallingDeployment(ctx context WithStartupProbe(corev1ac.Probe(). WithExec(corev1ac.ExecAction().WithCommand(command)). WithInitialDelaySeconds(0). - WithPeriodSeconds(0). + WithPeriodSeconds(1). WithFailureThreshold(1). WithSuccessThreshold(1)))))) diff --git a/internal/controller/hypervisor_controller.go b/internal/controller/hypervisor_controller.go index 41213ad2..138aa0b0 100644 --- a/internal/controller/hypervisor_controller.go +++ b/internal/controller/hypervisor_controller.go @@ -48,7 +48,7 @@ const ( HypervisorControllerName = "hypervisor" ) -var transferLabels = []string{ +var defaultTransferLabels = []string{ corev1.LabelHostname, "kubernetes.metal.cloud.sap/bb", "kubernetes.metal.cloud.sap/cluster", @@ -60,6 +60,8 @@ var transferLabels = []string{ corev1.LabelTopologyZone, } +var transferLabels = append([]string{}, defaultTransferLabels...) + type HypervisorController struct { k8sclient.Client Scheme *runtime.Scheme @@ -68,7 +70,7 @@ type HypervisorController struct { // +kubebuilder:rbac:groups="",resources=nodes,verbs=get;list;watch // +kubebuilder:rbac:groups="",resources=nodes/status,verbs=get // +kubebuilder:rbac:groups=kvm.cloud.sap,resources=hypervisors,verbs=get;list;watch;create;update;patch;delete -// +kubebuilder:rbac:groups=kvm.cloud.sap,resources=hypervisors/status,verbs=get;list;watch;create;update;patch;delete +// +kubebuilder:rbac:groups=kvm.cloud.sap,resources=hypervisors/status,verbs=get;update;patch func (hv *HypervisorController) Reconcile(ctx context.Context, req ctrl.Request) (ctrl.Result, error) { log := logger.FromContext(ctx).WithName(req.Name) @@ -195,8 +197,13 @@ func (hv *HypervisorController) SetupWithManager(mgr ctrl.Manager) error { return fmt.Errorf("failed to parse global label selector: %w", err) } + // Rebuild from immutable defaults to avoid accumulating state across repeated calls + transferLabels = append([]string{}, defaultTransferLabels...) for _, requirement := range requirements { - transferLabels = append(transferLabels, requirement.Key()) + key := requirement.Key() + if !slices.Contains(transferLabels, key) { + transferLabels = append(transferLabels, key) + } } } diff --git a/internal/controller/hypervisor_instance_ha_controller.go b/internal/controller/hypervisor_instance_ha_controller.go index 831f7134..9b622de5 100644 --- a/internal/controller/hypervisor_instance_ha_controller.go +++ b/internal/controller/hypervisor_instance_ha_controller.go @@ -153,9 +153,6 @@ func (r *HypervisorInstanceHaController) Reconcile(ctx context.Context, req ctrl // SetupWithManager sets up the controller with the Manager. func (r *HypervisorInstanceHaController) SetupWithManager(mgr ctrl.Manager) error { - ctx := context.Background() - _ = logger.FromContext(ctx) - return ctrl.NewControllerManagedBy(mgr). Named(HypervisorInstanceHaControllerName). For(&kvmv1.Hypervisor{}). // trigger the r.Reconcile whenever a hypervisor is created/updated/deleted. diff --git a/internal/controller/hypervisor_maintenance_controller.go b/internal/controller/hypervisor_maintenance_controller.go index 5a4e55f3..c35b54e9 100644 --- a/internal/controller/hypervisor_maintenance_controller.go +++ b/internal/controller/hypervisor_maintenance_controller.go @@ -53,7 +53,7 @@ type HypervisorMaintenanceController struct { } // +kubebuilder:rbac:groups=kvm.cloud.sap,resources=hypervisors,verbs=get;list;watch -// +kubebuilder:rbac:groups=kvm.cloud.sap,resources=hypervisors/status,verbs=get;list;watch;create;update;patch;delete +// +kubebuilder:rbac:groups=kvm.cloud.sap,resources=hypervisors/status,verbs=get;update;patch // +kubebuilder:rbac:groups=kvm.cloud.sap,resources=evictions,verbs=get;list;watch;create;update;patch;delete func (hec *HypervisorMaintenanceController) Reconcile(ctx context.Context, req ctrl.Request) (ctrl.Result, error) { hv := &kvmv1.Hypervisor{} @@ -248,7 +248,6 @@ func (hec *HypervisorMaintenanceController) ensureEviction(ctx context.Context, // SetupWithManager sets up the controller with the Manager. func (hec *HypervisorMaintenanceController) SetupWithManager(mgr ctrl.Manager) error { ctx := context.Background() - _ = logger.FromContext(ctx) var err error if hec.computeClient, err = openstack.GetServiceClient(ctx, "compute", nil); err != nil { diff --git a/internal/controller/hypervisor_taint_controller.go b/internal/controller/hypervisor_taint_controller.go index ad15da4a..7822b348 100644 --- a/internal/controller/hypervisor_taint_controller.go +++ b/internal/controller/hypervisor_taint_controller.go @@ -43,7 +43,7 @@ type HypervisorTaintController struct { } // +kubebuilder:rbac:groups=kvm.cloud.sap,resources=hypervisors,verbs=get;list;watch -// +kubebuilder:rbac:groups=kvm.cloud.sap,resources=hypervisors/status,verbs=get;list;watch;create;update;patch +// +kubebuilder:rbac:groups=kvm.cloud.sap,resources=hypervisors/status,verbs=get;update;patch func (r *HypervisorTaintController) Reconcile(ctx context.Context, req ctrl.Request) (ctrl.Result, error) { hypervisor := &kvmv1.Hypervisor{ diff --git a/internal/controller/node_certificate_controller.go b/internal/controller/node_certificate_controller.go index a822ef25..b8da375e 100644 --- a/internal/controller/node_certificate_controller.go +++ b/internal/controller/node_certificate_controller.go @@ -171,7 +171,7 @@ func (r *NodeCertificateController) Reconcile(ctx context.Context, req ctrl.Requ }) if err != nil { - return ctrl.Result{}, fmt.Errorf("could create certificate %w", err) + return ctrl.Result{}, fmt.Errorf("could not create certificate: %w", err) } return ctrl.Result{}, nil diff --git a/internal/controller/onboarding_controller.go b/internal/controller/onboarding_controller.go index 0e466e66..7e9fed71 100644 --- a/internal/controller/onboarding_controller.go +++ b/internal/controller/onboarding_controller.go @@ -415,7 +415,7 @@ func (r *OnboardingController) ensureNovaProperties(ctx context.Context, hv *kvm return errRequeue } - shortHypervisorAddress := strings.SplitN(hypervisorAddress, ".", 1)[0] + shortHypervisorAddress := strings.SplitN(hypervisorAddress, ".", 2)[0] hypervisorQuery := hypervisors.ListOpts{HypervisorHostnamePattern: &shortHypervisorAddress} hypervisorPages, err := hypervisors.List(r.computeClient, hypervisorQuery).AllPages(ctx) @@ -438,7 +438,7 @@ func (r *OnboardingController) ensureNovaProperties(ctx context.Context, hv *kvm var found = false var myHypervisor hypervisors.Hypervisor for _, h := range hs { - short := strings.SplitN(h.HypervisorHostname, ".", 1)[0] + short := strings.SplitN(h.HypervisorHostname, ".", 2)[0] if short == shortHypervisorAddress { myHypervisor = h found = true @@ -503,7 +503,7 @@ func (r *OnboardingController) createOrGetTestServer(ctx context.Context, zone, imageRef, err := r.findTestImage(ctx) if err != nil { - return nil, fmt.Errorf("could not list networks: %w", err) + return nil, fmt.Errorf("could not find test image: %w", err) } networkRef, err := r.findTestNetwork(ctx) diff --git a/internal/controller/traits_controller.go b/internal/controller/traits_controller.go index 11916361..6da55b84 100644 --- a/internal/controller/traits_controller.go +++ b/internal/controller/traits_controller.go @@ -29,7 +29,6 @@ import ( "k8s.io/apimachinery/pkg/runtime" ctrl "sigs.k8s.io/controller-runtime" k8sclient "sigs.k8s.io/controller-runtime/pkg/client" - logger "sigs.k8s.io/controller-runtime/pkg/log" "github.com/gophercloud/gophercloud/v2" "github.com/gophercloud/gophercloud/v2/openstack/placement/v1/resourceproviders" @@ -51,7 +50,7 @@ type TraitsController struct { } // +kubebuilder:rbac:groups=kvm.cloud.sap,resources=hypervisors,verbs=get;list;watch -// +kubebuilder:rbac:groups=kvm.cloud.sap,resources=hypervisors/status,verbs=get;list;watch;create;update;patch;delete +// +kubebuilder:rbac:groups=kvm.cloud.sap,resources=hypervisors/status,verbs=get;update;patch func (tc *TraitsController) Reconcile(ctx context.Context, req ctrl.Request) (ctrl.Result, error) { hv := &kvmv1.Hypervisor{} @@ -98,8 +97,8 @@ func (tc *TraitsController) Reconcile(ctx context.Context, req ctrl.Request) (ct return ctrl.Result{}, nil } - toAdd := Difference(customTraitsApplied, hv.Spec.CustomTraits) - toRemove := Difference(hv.Spec.CustomTraits, customTraitsApplied) + toAdd := utils.Difference(customTraitsApplied, hv.Spec.CustomTraits) + toRemove := utils.Difference(hv.Spec.CustomTraits, customTraitsApplied) // fetch current traits, to ensure we don't add duplicates current, err := resourceproviders.GetTraits(ctx, tc.serviceClient, hv.Status.HypervisorID).Extract() @@ -180,7 +179,6 @@ func getTraitCondition(err error, msg string) metav1.Condition { // SetupWithManager sets up the controller with the Manager. func (tc *TraitsController) SetupWithManager(mgr ctrl.Manager) error { ctx := context.Background() - _ = logger.FromContext(ctx) var err error if tc.serviceClient, err = openstack.GetServiceClient(ctx, "placement", nil); err != nil { diff --git a/internal/controller/utils.go b/internal/controller/utils.go index 84e42720..c1ca4b47 100644 --- a/internal/controller/utils.go +++ b/internal/controller/utils.go @@ -19,13 +19,13 @@ package controller import ( "bytes" - "errors" "fmt" "io" "net/http" "os" "slices" "strings" + "time" corev1 "k8s.io/api/core/v1" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" @@ -37,6 +37,9 @@ import ( func InstanceHaUrl(region, zone, hostname string) string { if haURL, found := os.LookupEnv("KVM_HA_SERVICE_URL"); found { + if !strings.HasSuffix(haURL, "/") { + haURL += "/" + } return haURL + "api/hypervisors/" + hostname } return fmt.Sprintf("https://kvm-ha-service-%v.%v.cloud.sap/api/hypervisors/%v", zone, region, hostname) @@ -58,8 +61,9 @@ func updateInstanceHA(hypervisor *kvmv1.Hypervisor, data string, acceptedCodes [ } url := InstanceHaUrl(region, zone, hostname) + client := &http.Client{Timeout: 30 * time.Second} // G107: Potential HTTP request made with variable url - resp, err := http.Post(url, "application/json", bytes.NewBuffer([]byte(data))) //nolint:gosec,bodyclose + resp, err := client.Post(url, "application/json", bytes.NewBuffer([]byte(data))) //nolint:bodyclose if err != nil { return fmt.Errorf("failed to send request to ha service due to %w", err) } @@ -80,11 +84,6 @@ func disableInstanceHA(hypervisor *kvmv1.Hypervisor) error { return updateInstanceHA(hypervisor, `{"enabled": false}`, []int{http.StatusOK, http.StatusNotFound}) } -// IsNodeConditionTrue returns true when the conditionType is present and set to `corev1.ConditionTrue` -func IsNodeConditionTrue(conditions []corev1.NodeCondition, conditionType corev1.NodeConditionType) bool { - return IsNodeConditionPresentAndEqual(conditions, conditionType, corev1.ConditionTrue) -} - // IsNodeConditionPresentAndEqual returns true when conditionType is present and equal to status. func IsNodeConditionPresentAndEqual(conditions []corev1.NodeCondition, conditionType corev1.NodeConditionType, status corev1.ConditionStatus) bool { for _, condition := range conditions { @@ -96,36 +95,16 @@ func IsNodeConditionPresentAndEqual(conditions []corev1.NodeCondition, condition } // FindNodeStatusCondition returns the condition of the given type if it exists. +// Note: Returns a pointer into the original slice element, not a copy. func FindNodeStatusCondition(conditions []corev1.NodeCondition, conditionType corev1.NodeConditionType) *corev1.NodeCondition { - for _, condition := range conditions { - if condition.Type == conditionType { - return &condition + for i := range conditions { + if conditions[i].Type == conditionType { + return &conditions[i] } } return nil } -func HasStatusCondition(conditions []metav1.Condition, conditionType string) bool { - for _, condition := range conditions { - if condition.Type == conditionType { - return true - } - } - return false -} - -// returns all elements in b not in a -func Difference[S ~[]E, E comparable](s1, s2 S) S { - diff := make(S, 0) - for item := range slices.Values(s2) { - if !slices.Contains(s1, item) { - diff = append(diff, item) - } - } - - return diff -} - // returns a OwnerReference for the given object and groupversionkind info as returned by apiutil.GVKForObject func OwnerReference(obj metav1.Object, gvk *schema.GroupVersionKind) *v1ac.OwnerReferenceApplyConfiguration { return v1ac.OwnerReference(). @@ -135,8 +114,6 @@ func OwnerReference(obj metav1.Object, gvk *schema.GroupVersionKind) *v1ac.Owner WithUID(obj.GetUID()) } -var ErrRetry = errors.New("ErrRetry") - // returns if any ManagedField of the object has been modified by kubectl func HasKubectlManagedFields(object *metav1.ObjectMeta) bool { for _, field := range object.GetManagedFields() { diff --git a/internal/controller/utils_test.go b/internal/controller/utils_test.go index 78ddabc2..709d5a10 100644 --- a/internal/controller/utils_test.go +++ b/internal/controller/utils_test.go @@ -20,6 +20,8 @@ package controller import ( . "github.com/onsi/ginkgo/v2" . "github.com/onsi/gomega" + + "github.com/cobaltcore-dev/openstack-hypervisor-operator/internal/utils" ) var _ = Describe("Utils", func() { @@ -31,7 +33,7 @@ var _ = Describe("Utils", func() { By("returning all elements in b not in a") expectedDifference := []string{"e", "f"} - difference := Difference(sliceA, sliceB) + difference := utils.Difference(sliceA, sliceB) Expect(difference).To(Equal(expectedDifference)) }) }) diff --git a/internal/logger/logger.go b/internal/logger/logger.go index 26e78b53..51371baf 100644 --- a/internal/logger/logger.go +++ b/internal/logger/logger.go @@ -22,16 +22,16 @@ import ( "go.uber.org/zap/zapcore" ) -func NewSanitzeReconcileErrorEncoder(cfg zapcore.EncoderConfig) zapcore.Encoder { - return &SanitzeReconcileErrorEncoder{zapcore.NewConsoleEncoder(cfg), cfg} +func NewSanitizeReconcileErrorEncoder(cfg zapcore.EncoderConfig) zapcore.Encoder { + return &SanitizeReconcileErrorEncoder{zapcore.NewConsoleEncoder(cfg), cfg} } -type SanitzeReconcileErrorEncoder struct { +type SanitizeReconcileErrorEncoder struct { zapcore.Encoder cfg zapcore.EncoderConfig } -func (e *SanitzeReconcileErrorEncoder) EncodeEntry(entry zapcore.Entry, fields []zapcore.Field) (*buffer.Buffer, error) { +func (e *SanitizeReconcileErrorEncoder) EncodeEntry(entry zapcore.Entry, fields []zapcore.Field) (*buffer.Buffer, error) { if entry.Message == "Reconcile error" { // Downgrade the log level to debug to avoid log spam entry.Level = zapcore.WarnLevel @@ -40,8 +40,9 @@ func (e *SanitzeReconcileErrorEncoder) EncodeEntry(entry zapcore.Entry, fields [ return e.Encoder.EncodeEntry(entry, fields) } -func (e *SanitzeReconcileErrorEncoder) Clone() zapcore.Encoder { - return &SanitzeReconcileErrorEncoder{ +func (e *SanitizeReconcileErrorEncoder) Clone() zapcore.Encoder { + return &SanitizeReconcileErrorEncoder{ Encoder: e.Encoder.Clone(), + cfg: e.cfg, } } diff --git a/internal/openstack/aggregates.go b/internal/openstack/aggregates.go index 079a26c8..1515f488 100644 --- a/internal/openstack/aggregates.go +++ b/internal/openstack/aggregates.go @@ -26,6 +26,7 @@ import ( "github.com/gophercloud/gophercloud/v2/openstack/compute/v2/aggregates" kvmv1 "github.com/cobaltcore-dev/openstack-hypervisor-operator/api/v1" + "github.com/cobaltcore-dev/openstack-hypervisor-operator/internal/utils" ) // ApplyAggregates ensures a host is in exactly the specified aggregates. @@ -75,8 +76,8 @@ func ApplyAggregates(ctx context.Context, serviceClient *gophercloud.ServiceClie } } - toAdd := difference(currentAggregates, desiredAggregates) - toRemove := difference(desiredAggregates, currentAggregates) + toAdd := utils.Difference(currentAggregates, desiredAggregates) + toRemove := utils.Difference(desiredAggregates, currentAggregates) // Verify all desired aggregates exist var missingAggregates []string @@ -135,14 +136,3 @@ func ApplyAggregates(ctx context.Context, serviceClient *gophercloud.ServiceClie return result, nil } - -// difference returns all elements in s2 that are not in s1 -func difference(s1, s2 []string) []string { - diff := make([]string, 0) - for _, item := range s2 { - if !slices.Contains(s1, item) { - diff = append(diff, item) - } - } - return diff -} diff --git a/internal/utils/slices.go b/internal/utils/slices.go new file mode 100644 index 00000000..8ab2997f --- /dev/null +++ b/internal/utils/slices.go @@ -0,0 +1,32 @@ +/* +SPDX-FileCopyrightText: Copyright 2024 SAP SE or an SAP affiliate company and cobaltcore-dev contributors +SPDX-License-Identifier: Apache-2.0 + +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 utils + +import "slices" + +// Difference returns all elements in s2 that are not in s1. +func Difference[S ~[]E, E comparable](s1, s2 S) S { + diff := make(S, 0) + for item := range slices.Values(s2) { + if !slices.Contains(s1, item) { + diff = append(diff, item) + } + } + + return diff +}