From f78c6a6f9f85814a327b82e8d8a98f1becb57351 Mon Sep 17 00:00:00 2001 From: Nick Cao Date: Thu, 17 Oct 2024 13:34:53 -0400 Subject: [PATCH 1/3] Factor out reconcileStatusExporterRef and reconcileStatusBeginEndTime --- internal/controller/lease_controller.go | 286 ++++++++++++------------ 1 file changed, 138 insertions(+), 148 deletions(-) diff --git a/internal/controller/lease_controller.go b/internal/controller/lease_controller.go index 4a8b738..0effcbd 100644 --- a/internal/controller/lease_controller.go +++ b/internal/controller/lease_controller.go @@ -26,6 +26,7 @@ import ( "k8s.io/apimachinery/pkg/api/meta" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/runtime" + "k8s.io/apimachinery/pkg/types" ctrl "sigs.k8s.io/controller-runtime" "sigs.k8s.io/controller-runtime/pkg/client" "sigs.k8s.io/controller-runtime/pkg/controller/controllerutil" @@ -146,182 +147,171 @@ func (r *LeaseReconciler) Reconcile(ctx context.Context, req ctrl.Request) (ctrl } } -func (r *LeaseReconciler) ReconcileNewLease( +func (r *LeaseReconciler) reconcileStatusBeginEndTime( ctx context.Context, - lease jumpstarterdevv1alpha1.Lease, -) (ctrl.Result, error) { - log := log.FromContext(ctx) - - selector, err := metav1.LabelSelectorAsSelector(&lease.Spec.Selector) - if err != nil { - log.Error(err, "Error creating selector for label selector") - return ctrl.Result{}, err - } + lease *jumpstarterdevv1alpha1.Lease, +) error { + logger := log.FromContext(ctx) - // List all Exporter matching selector - var matchedExporters jumpstarterdevv1alpha1.ExporterList - if err := r.List( - ctx, - &matchedExporters, - client.InNamespace(lease.Namespace), - client.MatchingLabelsSelector{Selector: selector}, - ); err != nil { - log.Error(err, "Error listing exporters") - return ctrl.Result{}, err + if lease.Status.BeginTime == nil && lease.Status.ExporterRef != nil { + logger.Info("reconcileStatusBeginEndTime: updating begin time", "lease", lease) + lease.Status.BeginTime = &metav1.Time{ + Time: time.Now(), + } } - onlineExporters := slices.DeleteFunc( - matchedExporters.Items, - func(exporter jumpstarterdevv1alpha1.Exporter) bool { - return !(true && - meta.IsStatusConditionTrue( - exporter.Status.Conditions, - string(jumpstarterdevv1alpha1.ExporterConditionTypeRegistered), - ) && - meta.IsStatusConditionTrue( - exporter.Status.Conditions, - string(jumpstarterdevv1alpha1.ExporterConditionTypeOnline), - )) - }, - ) - - // No Exporter available, lease unsatisfiable - if len(onlineExporters) == 0 { - lease.Status = jumpstarterdevv1alpha1.LeaseStatus{ - BeginTime: nil, - EndTime: nil, - ExporterRef: nil, - Ended: true, + if lease.Status.EndTime == nil && lease.Status.BeginTime != nil { + logger.Info("reconcileStatusBeginEndTime: updating end time", "lease", lease) + lease.Status.EndTime = &metav1.Time{ + Time: lease.Status.BeginTime.Add(lease.Spec.Duration.Duration), } + } - meta.SetStatusCondition(&lease.Status.Conditions, metav1.Condition{ - Type: string(jumpstarterdevv1alpha1.LeaseConditionTypeUnsatisfiable), - Status: metav1.ConditionTrue, - ObservedGeneration: lease.Generation, - LastTransitionTime: metav1.Time{ - Time: time.Now(), - }, - Reason: "NoExporter", - }) + return nil +} - if err := r.Status().Update(ctx, &lease); err != nil { - log.Error(err, "unable to update Lease status") - return ctrl.Result{}, client.IgnoreNotFound(err) +// Also manages LeaseConditionTypeUnsatisfiable and LeaseConditionTypePending +func (r *LeaseReconciler) reconcileStatusExporterRef( + ctx context.Context, + result *ctrl.Result, + lease *jumpstarterdevv1alpha1.Lease, +) error { + logger := log.FromContext(ctx) + + if lease.Status.ExporterRef == nil { + logger.Info("reconcileStatusExporterRef: looking for matching exporter", "lease", lease) + + selector, err := metav1.LabelSelectorAsSelector(&lease.Spec.Selector) + if err != nil { + logger.Error(err, "reconcileStatusExporterRef: failed to create selector from label selector", "lease", lease) + return err } - return ctrl.Result{}, nil - } + // List all Exporter matching selector + var matchingExporters jumpstarterdevv1alpha1.ExporterList + if err := r.List( + ctx, + &matchingExporters, + client.InNamespace(lease.Namespace), + client.MatchingLabelsSelector{Selector: selector}, + ); err != nil { + logger.Error(err, "reconcileStatusExporterRef: failed to list exporters matching selector", "lease", lease) + return err + } - var leases jumpstarterdevv1alpha1.LeaseList - err = r.List( - ctx, - &leases, - client.InNamespace(lease.Namespace), - MatchingActiveLeases(), - ) - if err != nil { - log.Error(err, "Error listing leases") - return ctrl.Result{}, err - } + // Filter out offline exporters + onlineExporters := slices.DeleteFunc( + matchingExporters.Items, + func(exporter jumpstarterdevv1alpha1.Exporter) bool { + return !(true && + meta.IsStatusConditionTrue( + exporter.Status.Conditions, + string(jumpstarterdevv1alpha1.ExporterConditionTypeRegistered), + ) && + meta.IsStatusConditionTrue( + exporter.Status.Conditions, + string(jumpstarterdevv1alpha1.ExporterConditionTypeOnline), + )) + }, + ) - // Find available exporter - for _, exporter := range onlineExporters { - taken := false - for _, existingLease := range leases.Items { - // if lease is active and is referencing an exporter - if !existingLease.Status.Ended && existingLease.Status.ExporterRef != nil { - // if lease is referencing this exporter - if existingLease.Status.ExporterRef.Name == exporter.Name { - taken = true - } - } - } - // Exporter taken by lease - if taken { - continue + // No matching exporter online, lease unsatisfiable + if len(onlineExporters) == 0 { + meta.SetStatusCondition(&lease.Status.Conditions, metav1.Condition{ + Type: string(jumpstarterdevv1alpha1.LeaseConditionTypeUnsatisfiable), + Status: metav1.ConditionTrue, + ObservedGeneration: lease.Generation, + LastTransitionTime: metav1.Time{ + Time: time.Now(), + }, + Reason: "NoExporter", + }) + return nil } - beginTime := time.Now() - - lease.Status = jumpstarterdevv1alpha1.LeaseStatus{ - BeginTime: &metav1.Time{ - Time: beginTime, - }, - EndTime: &metav1.Time{ - Time: beginTime.Add(lease.Spec.Duration.Duration), - }, - ExporterRef: &corev1.LocalObjectReference{ - Name: exporter.Name, - }, - Ended: false, + var leases jumpstarterdevv1alpha1.LeaseList + if err := r.List( + ctx, + &leases, + client.InNamespace(lease.Namespace), + MatchingActiveLeases(), + ); err != nil { + logger.Error(err, "reconcileStatusExporterRef: failed to list active leases", "lease", lease) + return err } - meta.SetStatusCondition(&lease.Status.Conditions, metav1.Condition{ - Type: string(jumpstarterdevv1alpha1.LeaseConditionTypeReady), - Status: metav1.ConditionTrue, - ObservedGeneration: lease.Generation, - LastTransitionTime: metav1.Time{ - Time: beginTime, - }, - Reason: "Acquired", + availableExporters := slices.DeleteFunc(onlineExporters, func(exporter jumpstarterdevv1alpha1.Exporter) bool { + for _, existingLease := range leases.Items { + // if the lease is referencing the current exporter + if existingLease.Status.ExporterRef != nil && existingLease.Status.ExporterRef.Name == exporter.Name { + return true + } + } + return false }) - if err := r.Status().Update(ctx, &lease); err != nil { - log.Error(err, "unable to update Lease status") - return ctrl.Result{}, client.IgnoreNotFound(err) + if len(availableExporters) == 0 { + meta.SetStatusCondition(&lease.Status.Conditions, metav1.Condition{ + Type: string(jumpstarterdevv1alpha1.LeaseConditionTypePending), + Status: metav1.ConditionTrue, + ObservedGeneration: lease.Generation, + LastTransitionTime: metav1.Time{ + Time: time.Now(), + }, + Reason: "NotAvailable", + }) + result.RequeueAfter = time.Second + return nil + } else { + lease.Status.ExporterRef = &corev1.LocalObjectReference{ + Name: availableExporters[0].Name, + } + return nil } + } - if err := controllerutil.SetControllerReference(&exporter, &lease, r.Scheme); err != nil { - log.Error(err, "unable to update Lease owner reference") - return ctrl.Result{}, err - } + return nil +} - if err := r.Update(ctx, &lease); err != nil { - log.Error(err, "unable to update Lease") - return ctrl.Result{}, err - } +func (r *LeaseReconciler) ReconcileNewLease( + ctx context.Context, + lease jumpstarterdevv1alpha1.Lease, +) (ctrl.Result, error) { + logger := log.FromContext(ctx) - // Requeue at EndTime - return ctrl.Result{ - RequeueAfter: time.Until(lease.Status.EndTime.Time), - }, nil + var result ctrl.Result + if err := r.reconcileStatusExporterRef(ctx, &result, &lease); err != nil { + return result, err } - lease.Status = jumpstarterdevv1alpha1.LeaseStatus{ - BeginTime: nil, - EndTime: nil, - ExporterRef: nil, - Ended: false, + if err := r.reconcileStatusBeginEndTime(ctx, &lease); err != nil { + return result, err } - meta.SetStatusCondition(&lease.Status.Conditions, metav1.Condition{ - Type: string(jumpstarterdevv1alpha1.LeaseConditionTypePending), - Status: metav1.ConditionTrue, - ObservedGeneration: lease.Generation, - LastTransitionTime: metav1.Time{ - Time: time.Now(), - }, - Reason: "NotAvailable", - }) - meta.SetStatusCondition(&lease.Status.Conditions, metav1.Condition{ - Type: string(jumpstarterdevv1alpha1.LeaseConditionTypeReady), - Status: metav1.ConditionFalse, - ObservedGeneration: lease.Generation, - LastTransitionTime: metav1.Time{ - Time: time.Now(), - }, - Reason: "Pending", - }) if err := r.Status().Update(ctx, &lease); err != nil { - log.Error(err, "unable to update Lease status") - return ctrl.Result{}, client.IgnoreNotFound(err) + logger.Error(err, "unable to update Lease status") + return result, err + } + + if lease.Status.ExporterRef != nil { + var exporter jumpstarterdevv1alpha1.Exporter + if err := r.Get(ctx, types.NamespacedName{ + Namespace: lease.Namespace, + Name: lease.Status.ExporterRef.Name, + }, &exporter); err != nil { + return result, err + } + if err := controllerutil.SetControllerReference(&exporter, &lease, r.Scheme); err != nil { + logger.Error(err, "unable to update Lease owner reference") + return result, err + } + if err := r.Update(ctx, &lease); err != nil { + logger.Error(err, "unable to update Lease status") + return result, err + } } - // No exporter available - // Try again later - return ctrl.Result{ - RequeueAfter: time.Second, - }, nil + return result, nil } // SetupWithManager sets up the controller with the Manager. From 1ff72d4dbbc75ba0e718610d731c8b768d5b4668 Mon Sep 17 00:00:00 2001 From: Nick Cao Date: Thu, 17 Oct 2024 14:16:23 -0400 Subject: [PATCH 2/3] Refactor the rest of lease reconciler --- internal/controller/lease_controller.go | 208 +++++++++---------- internal/controller/lease_controller_test.go | 5 - 2 files changed, 93 insertions(+), 120 deletions(-) diff --git a/internal/controller/lease_controller.go b/internal/controller/lease_controller.go index 0effcbd..81f7e5e 100644 --- a/internal/controller/lease_controller.go +++ b/internal/controller/lease_controller.go @@ -53,117 +53,136 @@ type LeaseReconciler struct { // For more details, check Reconcile and its Result here: // - https://pkg.go.dev/sigs.k8s.io/controller-runtime@v0.18.4/pkg/reconcile func (r *LeaseReconciler) Reconcile(ctx context.Context, req ctrl.Request) (ctrl.Result, error) { - log := log.FromContext(ctx) + logger := log.FromContext(ctx) var lease jumpstarterdevv1alpha1.Lease if err := r.Get(ctx, req.NamespacedName, &lease); err != nil { - log.Error(err, "unable to fetch Lease") + logger.Error(err, "Reconcile: unable to get lease", "lease", req.NamespacedName) return ctrl.Result{}, client.IgnoreNotFound(err) } - // lease already ended, ignoring - if lease.Status.Ended { - return ctrl.Result{}, nil + var result ctrl.Result + if err := r.reconcileStatusExporterRef(ctx, &result, &lease); err != nil { + return result, err } - // force release lease - if lease.Spec.Release { - log.Info("lease released early", "lease", lease.Name) - now := time.Now() + if err := r.reconcileStatusBeginTime(ctx, &lease); err != nil { + return result, err + } - if lease.Labels == nil { - lease.Labels = make(map[string]string) - } - lease.Labels[string(jumpstarterdevv1alpha1.LeaseLabelEnded)] = jumpstarterdevv1alpha1.LeaseLabelEndedValue + if err := r.reconcileStatusEnded(ctx, &result, &lease); err != nil { + return result, err + } - if err := r.Update(ctx, &lease); err != nil { - log.Error(err, "unable to update Lease labels") - return ctrl.Result{}, err - } + if err := r.Status().Update(ctx, &lease); err != nil { + return result, err + } - lease.Status.Ended = true - lease.Status.EndTime = &metav1.Time{ - Time: now, - } - meta.SetStatusCondition(&lease.Status.Conditions, metav1.Condition{ - Type: string(jumpstarterdevv1alpha1.LeaseConditionTypeReady), - Status: metav1.ConditionFalse, - ObservedGeneration: lease.Generation, - LastTransitionTime: metav1.Time{ - Time: now, - }, - Reason: "Released", - }) + if lease.Labels == nil { + lease.Labels = make(map[string]string) + } + if lease.Status.Ended { + lease.Labels[string(jumpstarterdevv1alpha1.LeaseLabelEnded)] = jumpstarterdevv1alpha1.LeaseLabelEndedValue + } - if err := r.Status().Update(ctx, &lease); err != nil { - log.Error(err, "unable to update Lease status") - return ctrl.Result{}, err + if lease.Status.ExporterRef != nil { + var exporter jumpstarterdevv1alpha1.Exporter + if err := r.Get(ctx, types.NamespacedName{ + Namespace: lease.Namespace, + Name: lease.Status.ExporterRef.Name, + }, &exporter); err != nil { + return result, err + } + if err := controllerutil.SetControllerReference(&exporter, &lease, r.Scheme); err != nil { + logger.Error(err, "Reconcile: failed to update lease controller reference", "lease", lease) + return result, err } + } - return ctrl.Result{}, nil + if err := r.Update(ctx, &lease); err != nil { + logger.Error(err, "Reconcile: failed to update lease metadata", "lease", lease) + return result, err } - // 1. newly created lease - if lease.Status.BeginTime == nil || lease.Status.EndTime == nil || lease.Status.ExporterRef == nil { - return r.ReconcileNewLease(ctx, lease) - } else { - // 2. expired lease - if time.Now().After(lease.Status.EndTime.Time) { - log.Info("lease expired", "lease", lease.Name) - if lease.Labels == nil { - lease.Labels = make(map[string]string) - } - lease.Labels[string(jumpstarterdevv1alpha1.LeaseLabelEnded)] = jumpstarterdevv1alpha1.LeaseLabelEndedValue + return result, nil +} - if err := r.Update(ctx, &lease); err != nil { - log.Error(err, "unable to update Lease labels") - return ctrl.Result{}, err - } +// also manages EndTime and LeaseConditionTypeReady +// nolint:unparam +func (r *LeaseReconciler) reconcileStatusEnded( + ctx context.Context, + result *ctrl.Result, + lease *jumpstarterdevv1alpha1.Lease, +) error { + logger := log.FromContext(ctx) - lease.Status.Ended = true + now := time.Now() + if !lease.Status.Ended { + if lease.Spec.Release { + logger.Info("reconcileStatusEndTime: force releasing lease", "lease", lease) meta.SetStatusCondition(&lease.Status.Conditions, metav1.Condition{ Type: string(jumpstarterdevv1alpha1.LeaseConditionTypeReady), Status: metav1.ConditionFalse, ObservedGeneration: lease.Generation, LastTransitionTime: metav1.Time{ - Time: time.Now(), + Time: now, }, - Reason: "Expired", + Reason: "Released", }) - - if err := r.Status().Update(ctx, &lease); err != nil { - log.Error(err, "unable to update Lease status") - return ctrl.Result{}, err + lease.Status.Ended = true + lease.Status.EndTime = &metav1.Time{ + Time: now, + } + return nil + } else if lease.Status.BeginTime != nil { + expiration := lease.Status.BeginTime.Add(lease.Spec.Duration.Duration) + if expiration.Before(now) { + logger.Info("reconcileStatusEndTime: lease expired", "lease", lease) + meta.SetStatusCondition(&lease.Status.Conditions, metav1.Condition{ + Type: string(jumpstarterdevv1alpha1.LeaseConditionTypeReady), + Status: metav1.ConditionFalse, + ObservedGeneration: lease.Generation, + LastTransitionTime: metav1.Time{ + Time: time.Now(), + }, + Reason: "Expired", + }) + lease.Status.Ended = true + lease.Status.EndTime = &metav1.Time{ + Time: now, + } + return nil + } else { + result.RequeueAfter = expiration.Sub(now) + return nil } - - return ctrl.Result{}, nil - } else { - // 3. acquired lease - // Requeue acquire lease on EndTime - return ctrl.Result{ - RequeueAfter: time.Until(lease.Status.EndTime.Time), - }, nil } } + + return nil } -func (r *LeaseReconciler) reconcileStatusBeginEndTime( +// nolint:unparam +func (r *LeaseReconciler) reconcileStatusBeginTime( ctx context.Context, lease *jumpstarterdevv1alpha1.Lease, ) error { logger := log.FromContext(ctx) + now := time.Now() if lease.Status.BeginTime == nil && lease.Status.ExporterRef != nil { - logger.Info("reconcileStatusBeginEndTime: updating begin time", "lease", lease) + logger.Info("reconcileStatusBeginTime: updating begin time", "lease", lease) + meta.SetStatusCondition(&lease.Status.Conditions, metav1.Condition{ + Type: string(jumpstarterdevv1alpha1.LeaseConditionTypeReady), + Status: metav1.ConditionTrue, + ObservedGeneration: lease.Generation, + LastTransitionTime: metav1.Time{ + Time: now, + }, + Reason: "Ready", + }) lease.Status.BeginTime = &metav1.Time{ - Time: time.Now(), - } - } - - if lease.Status.EndTime == nil && lease.Status.BeginTime != nil { - logger.Info("reconcileStatusBeginEndTime: updating end time", "lease", lease) - lease.Status.EndTime = &metav1.Time{ - Time: lease.Status.BeginTime.Add(lease.Spec.Duration.Duration), + Time: now, } } @@ -273,47 +292,6 @@ func (r *LeaseReconciler) reconcileStatusExporterRef( return nil } -func (r *LeaseReconciler) ReconcileNewLease( - ctx context.Context, - lease jumpstarterdevv1alpha1.Lease, -) (ctrl.Result, error) { - logger := log.FromContext(ctx) - - var result ctrl.Result - if err := r.reconcileStatusExporterRef(ctx, &result, &lease); err != nil { - return result, err - } - - if err := r.reconcileStatusBeginEndTime(ctx, &lease); err != nil { - return result, err - } - - if err := r.Status().Update(ctx, &lease); err != nil { - logger.Error(err, "unable to update Lease status") - return result, err - } - - if lease.Status.ExporterRef != nil { - var exporter jumpstarterdevv1alpha1.Exporter - if err := r.Get(ctx, types.NamespacedName{ - Namespace: lease.Namespace, - Name: lease.Status.ExporterRef.Name, - }, &exporter); err != nil { - return result, err - } - if err := controllerutil.SetControllerReference(&exporter, &lease, r.Scheme); err != nil { - logger.Error(err, "unable to update Lease owner reference") - return result, err - } - if err := r.Update(ctx, &lease); err != nil { - logger.Error(err, "unable to update Lease status") - return result, err - } - } - - return result, nil -} - // SetupWithManager sets up the controller with the Manager. func (r *LeaseReconciler) SetupWithManager(mgr ctrl.Manager) error { return ctrl.NewControllerManagedBy(mgr). diff --git a/internal/controller/lease_controller_test.go b/internal/controller/lease_controller_test.go index 18c9b88..73d9741 100644 --- a/internal/controller/lease_controller_test.go +++ b/internal/controller/lease_controller_test.go @@ -74,7 +74,6 @@ var _ = Describe("Lease Controller", func() { Expect(updatedLease.Status.ExporterRef).NotTo(BeNil()) Expect(updatedLease.Status.ExporterRef.Name).To(BeElementOf([]string{testExporter1DutA.Name, testExporter2DutA.Name})) Expect(updatedLease.Status.BeginTime).NotTo(BeNil()) - Expect(updatedLease.Status.EndTime).NotTo(BeNil()) updatedExporter := getExporter(ctx, updatedLease.Status.ExporterRef.Name) Expect(updatedExporter.Status.LeaseRef).NotTo(BeNil()) @@ -167,7 +166,6 @@ var _ = Describe("Lease Controller", func() { Expect(updatedLease.Status.ExporterRef).NotTo(BeNil()) Expect(updatedLease.Status.ExporterRef.Name).To(BeElementOf([]string{testExporter2DutA.Name})) Expect(updatedLease.Status.BeginTime).NotTo(BeNil()) - Expect(updatedLease.Status.EndTime).NotTo(BeNil()) updatedExporter := getExporter(ctx, updatedLease.Status.ExporterRef.Name) Expect(updatedExporter.Status.LeaseRef).NotTo(BeNil()) @@ -265,8 +263,6 @@ var _ = Describe("Lease Controller", func() { // we should consider adding a flag on the spec to do this, or look at the duration too updatedLease.Spec.Release = true - endTime := updatedLease.Status.EndTime - Expect(k8sClient.Update(ctx, updatedLease)).To(Succeed()) _ = reconcileLease(ctx, updatedLease) @@ -274,7 +270,6 @@ var _ = Describe("Lease Controller", func() { updatedLease = getLease(ctx, lease.Name) Expect(updatedLease.Status.ExporterRef).NotTo(BeNil()) Expect(updatedLease.Status.Ended).To(BeTrue()) - Expect(updatedLease.Status.EndTime).ToNot(Equal(endTime)) updatedExporter := getExporter(ctx, exporterName) Expect(updatedExporter.Status.LeaseRef).To(BeNil()) From f1d5622d9bd3e97e24f2377ad37d19a3aefb3b7c Mon Sep 17 00:00:00 2001 From: Nick Cao Date: Fri, 18 Oct 2024 08:44:18 -0400 Subject: [PATCH 3/3] Only log get error if not "not found" --- internal/controller/lease_controller.go | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/internal/controller/lease_controller.go b/internal/controller/lease_controller.go index 81f7e5e..aba7b85 100644 --- a/internal/controller/lease_controller.go +++ b/internal/controller/lease_controller.go @@ -23,6 +23,7 @@ import ( jumpstarterdevv1alpha1 "github.com/jumpstarter-dev/jumpstarter-controller/api/v1alpha1" corev1 "k8s.io/api/core/v1" + apierrors "k8s.io/apimachinery/pkg/api/errors" "k8s.io/apimachinery/pkg/api/meta" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/runtime" @@ -57,7 +58,9 @@ func (r *LeaseReconciler) Reconcile(ctx context.Context, req ctrl.Request) (ctrl var lease jumpstarterdevv1alpha1.Lease if err := r.Get(ctx, req.NamespacedName, &lease); err != nil { - logger.Error(err, "Reconcile: unable to get lease", "lease", req.NamespacedName) + if !apierrors.IsNotFound(err) { + logger.Error(err, "Reconcile: unable to get lease", "lease", req.NamespacedName) + } return ctrl.Result{}, client.IgnoreNotFound(err) }