From 9e3cf869873d8ba4024c7df2f88334bf0cd6e635 Mon Sep 17 00:00:00 2001 From: Kirk Brauer Date: Sun, 30 Aug 2026 17:23:48 -0400 Subject: [PATCH 1/2] fix(lease): let a pool provision for a lease that matches nothing yet MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit An ExporterSet that keeps nothing warm has no members until something asks for one. A lease against such a pool matches zero exporters, and the lease controller called that unsatisfiable — which reconcileStatusEnded then ends in the same reconcile. The set's demand rule counts pending, unended leases, so it never saw the request: a pool with minReplicas 0 could never provision on demand, which is the case it exists for. The lease controller now asks whether an exporter set could provision a match before deciding. If one could, the lease is Pending/Provisioning and requeued, which is the state the set scales on. If none could — no pool matches the selector, or the only one that does is at maxReplicas — nothing changes and the lease is still unsatisfiable, so a typo in a selector still fails fast instead of hanging. This is the first thing the lease controller reads outside jumpstarter.dev, so it needs the virtualtarget scheme and a read on exportersets, granted by the operator alongside the controller's other permissions. Where that read is refused — a controller newer than its operator, or a cluster with no exporter set CRDs — the check quietly reports "no pool" and the behaviour is exactly what it was before. Assisted-by: Claude Signed-off-by: Kirk Brauer --- controller/cmd/main.go | 5 + .../internal/controller/jumpstarter/rbac.go | 9 ++ .../internal/controller/lease_controller.go | 68 +++++++++++ .../controller/lease_controller_test.go | 113 ++++++++++++++++++ controller/internal/controller/suite_test.go | 6 + 5 files changed, 201 insertions(+) diff --git a/controller/cmd/main.go b/controller/cmd/main.go index 09c429eac..9ab9102a7 100644 --- a/controller/cmd/main.go +++ b/controller/cmd/main.go @@ -44,6 +44,7 @@ import ( "sigs.k8s.io/controller-runtime/pkg/webhook" jumpstarterdevv1alpha1 "github.com/jumpstarter-dev/jumpstarter/controller/api/v1alpha1" + virtualtargetv1alpha1 "github.com/jumpstarter-dev/jumpstarter/controller/api/virtualtarget/v1alpha1" "github.com/jumpstarter-dev/jumpstarter/controller/internal/authentication" "github.com/jumpstarter-dev/jumpstarter/controller/internal/authorization" "github.com/jumpstarter-dev/jumpstarter/controller/internal/config" @@ -102,6 +103,10 @@ func init() { utilruntime.Must(jumpstarterdevv1alpha1.AddToScheme(scheme)) + // Read-only, for the lease controller: whether a selector that matches no + // exporter today could be satisfied by a pool provisioning one. + utilruntime.Must(virtualtargetv1alpha1.AddToScheme(scheme)) + // +kubebuilder:scaffold:scheme apiserverinstall.Install(scheme) } diff --git a/controller/deploy/operator/internal/controller/jumpstarter/rbac.go b/controller/deploy/operator/internal/controller/jumpstarter/rbac.go index 989db4847..9a43e5e88 100644 --- a/controller/deploy/operator/internal/controller/jumpstarter/rbac.go +++ b/controller/deploy/operator/internal/controller/jumpstarter/rbac.go @@ -207,6 +207,15 @@ func (r *JumpstarterReconciler) createRole(jumpstarter *operatorv1alpha1.Jumpsta Resources: []string{"clients/finalizers", "exporters/finalizers", "leases/finalizers", "exporteraccesspolicies/finalizers"}, Verbs: []string{"update"}, }, + { + // Read-only: the lease controller asks whether a selector that + // matches no exporter today could be satisfied by a pool + // provisioning one, which decides between pending and + // unsatisfiable. It never writes exporter sets. + APIGroups: []string{"virtualtarget.jumpstarter.dev"}, + Resources: []string{"exportersets"}, + Verbs: []string{"get", "list", "watch"}, + }, { APIGroups: []string{""}, Resources: []string{"events"}, diff --git a/controller/internal/controller/lease_controller.go b/controller/internal/controller/lease_controller.go index 77acbbf49..bfa56ae2a 100755 --- a/controller/internal/controller/lease_controller.go +++ b/controller/internal/controller/lease_controller.go @@ -24,6 +24,7 @@ import ( "time" jumpstarterdevv1alpha1 "github.com/jumpstarter-dev/jumpstarter/controller/api/v1alpha1" + virtualtargetv1alpha1 "github.com/jumpstarter-dev/jumpstarter/controller/api/virtualtarget/v1alpha1" jmpmetrics "github.com/jumpstarter-dev/jumpstarter/controller/internal/metrics" corev1 "k8s.io/api/core/v1" k8serrors "k8s.io/apimachinery/pkg/api/errors" @@ -63,6 +64,7 @@ type ApprovedExporter struct { // +kubebuilder:rbac:groups=jumpstarter.dev,resources=leases,verbs=get;list;watch;create;update;patch;delete // +kubebuilder:rbac:groups=jumpstarter.dev,resources=leases/status,verbs=get;update;patch // +kubebuilder:rbac:groups=jumpstarter.dev,resources=leases/finalizers,verbs=update +// +kubebuilder:rbac:groups=virtualtarget.jumpstarter.dev,resources=exportersets,verbs=get;list;watch // Reconcile is part of the main kubernetes reconciliation loop which aims to // move the current state of the cluster closer to the desired state. // TODO(user): Modify the Reconcile function to compare the state specified by @@ -289,6 +291,27 @@ func (r *LeaseReconciler) reconcileStatusExporterRef( if err != nil { return fmt.Errorf("reconcileStatusExporterRef: failed to list matching exporters: %w", err) } + if len(listed.Items) == 0 { + // Nothing matches yet, which is the normal state of a pool that + // keeps nothing warm. An exporter set that could provision a + // match makes this lease pending rather than unsatisfiable — + // and pending is what the set counts as demand to scale up on. + // Calling it unsatisfiable here ends the lease in this same + // reconcile, so the set would never see it. + set, err := r.exporterSetCanProvision(ctx, lease, selector) + if err != nil { + return fmt.Errorf("reconcileStatusExporterRef: %w", err) + } + if set != "" { + lease.SetStatusPending( + "Provisioning", + "No exporter matches the selector yet; exporter set %s can provision one", + set, + ) + result.RequeueAfter = pendingRequeueAfter(lease) + return nil + } + } // Filter out disabled exporters from selector-based listing matchingExporters = filterOutDisabledExporters(listed.Items) if len(matchingExporters) == 0 && len(listed.Items) > 0 { @@ -557,6 +580,51 @@ func (r *LeaseReconciler) ListMatchingExporters(ctx context.Context, lease *jump return &matchingExporters, nil } +// exporterSetCanProvision reports the name of an exporter set that could +// provision an exporter matching the lease's selector, or "" if none can. +// +// It answers the question the lease controller cannot otherwise ask: is there +// nothing for this lease because nothing will ever match, or because the pool +// that would match keeps nothing warm? The two look identical from the exporter +// listing and mean opposite things to the caller. +// +// A set qualifies when the labels it stamps on its members satisfy the selector +// and it is not already at its ceiling. Whether it is actually willing to scale +// right now is the set's own decision, made in its reconciler; this only has to +// be right about whether waiting is worthwhile. +func (r *LeaseReconciler) exporterSetCanProvision( + ctx context.Context, + lease *jumpstarterdevv1alpha1.Lease, + selector labels.Selector, +) (string, error) { + var sets virtualtargetv1alpha1.ExporterSetList + if err := r.List(ctx, &sets, client.InNamespace(lease.Namespace)); err != nil { + if k8serrors.IsForbidden(err) || meta.IsNoMatchError(err) { + // A controller running against an operator that has not granted + // this read, or a cluster without the exporter set CRDs at all. + // Neither is transient, and neither is a reason to fail a lease: + // fall back to the behaviour from before pools existed. + log.FromContext(ctx).V(1).Info( + "cannot read exporter sets, treating the selector as unsatisfiable", + "lease", lease.Name, "reason", err.Error()) + return "", nil + } + return "", fmt.Errorf("exporterSetCanProvision: failed to list exporter sets: %w", err) + } + + for i := range sets.Items { + set := &sets.Items[i] + if !selector.Matches(labels.Set(set.Spec.Template.Metadata.Labels)) { + continue + } + if set.Spec.MaxReplicas > 0 && set.Status.Replicas >= set.Spec.MaxReplicas { + continue + } + return set.Name, nil + } + return "", nil +} + // ListActiveLeases returns a list of active leases in the namespace func (r *LeaseReconciler) ListActiveLeases(ctx context.Context, namespace string) (*jumpstarterdevv1alpha1.LeaseList, error) { var activeLeases jumpstarterdevv1alpha1.LeaseList diff --git a/controller/internal/controller/lease_controller_test.go b/controller/internal/controller/lease_controller_test.go index 26e3d6200..0c5d213bb 100755 --- a/controller/internal/controller/lease_controller_test.go +++ b/controller/internal/controller/lease_controller_test.go @@ -21,6 +21,7 @@ import ( "time" jumpstarterdevv1alpha1 "github.com/jumpstarter-dev/jumpstarter/controller/api/v1alpha1" + virtualtargetv1alpha1 "github.com/jumpstarter-dev/jumpstarter/controller/api/virtualtarget/v1alpha1" "github.com/jumpstarter-dev/jumpstarter/controller/internal/oidc" cpb "github.com/jumpstarter-dev/jumpstarter/controller/internal/protocol/jumpstarter/client/v1" . "github.com/onsi/ginkgo/v2" @@ -2654,3 +2655,115 @@ var _ = Describe("pendingRequeueAfter", func() { Entry("5m (capped)", 5*time.Minute, 30*time.Second), ) }) + +var _ = Describe("Lease against an exporter set with nothing warm", func() { + const setName = "cold-pool" + + newSet := func(maxReplicas, replicas int32) *virtualtargetv1alpha1.ExporterSet { + return &virtualtargetv1alpha1.ExporterSet{ + ObjectMeta: metav1.ObjectMeta{Name: setName, Namespace: "default"}, + Spec: virtualtargetv1alpha1.ExporterSetSpec{ + VirtualTargetClassName: "qemu", + MinReplicas: 0, + MaxReplicas: maxReplicas, + Selector: metav1.LabelSelector{MatchLabels: map[string]string{"pool": "cold"}}, + Template: virtualtargetv1alpha1.ExporterSetTemplate{ + Metadata: virtualtargetv1alpha1.EmbeddedObjectMeta{ + Labels: map[string]string{"pool": "cold"}, + }, + }, + }, + Status: virtualtargetv1alpha1.ExporterSetStatus{Replicas: replicas}, + } + } + + newLease := func(name string, matchLabels map[string]string) *jumpstarterdevv1alpha1.Lease { + return &jumpstarterdevv1alpha1.Lease{ + ObjectMeta: metav1.ObjectMeta{Name: name, Namespace: "default"}, + Spec: jumpstarterdevv1alpha1.LeaseSpec{ + ClientRef: corev1.LocalObjectReference{Name: testClient.Name}, + Selector: metav1.LabelSelector{MatchLabels: matchLabels}, + Duration: &metav1.Duration{Duration: 2 * time.Second}, + }, + } + } + + createSet := func(ctx context.Context, set *virtualtargetv1alpha1.ExporterSet) { + status := set.Status + Expect(k8sClient.Create(ctx, set)).To(Succeed()) + set.Status = status + Expect(k8sClient.Status().Update(ctx, set)).To(Succeed()) + } + + AfterEach(func() { + ctx := context.Background() + set := &virtualtargetv1alpha1.ExporterSet{ + ObjectMeta: metav1.ObjectMeta{Name: setName, Namespace: "default"}, + } + _ = k8sClient.Delete(ctx, set) + deleteLeases(ctx, "cold-lease", "cold-lease-max", "no-pool-lease") + }) + + When("a pool could provision a matching exporter", func() { + It("should stay pending rather than ending the lease", func() { + ctx := context.Background() + createSet(ctx, newSet(4, 0)) + + lease := newLease("cold-lease", map[string]string{"pool": "cold"}) + Expect(k8sClient.Create(ctx, lease)).To(Succeed()) + _ = reconcileLease(ctx, lease) + + updated := getLease(ctx, lease.Name) + // Ending it here is what stopped the set from ever seeing demand: + // countPendingLeases skips ended leases and requires Pending. + Expect(updated.Status.Ended).To(BeFalse()) + Expect(meta.IsStatusConditionTrue( + updated.Status.Conditions, + string(jumpstarterdevv1alpha1.LeaseConditionTypeUnsatisfiable), + )).To(BeFalse()) + + condition := meta.FindStatusCondition( + updated.Status.Conditions, + string(jumpstarterdevv1alpha1.LeaseConditionTypePending), + ) + Expect(condition).NotTo(BeNil()) + Expect(condition.Reason).To(Equal("Provisioning")) + Expect(condition.Message).To(ContainSubstring(setName)) + }) + }) + + When("the pool is already at its ceiling", func() { + It("should be unsatisfiable, since waiting would not help", func() { + ctx := context.Background() + createSet(ctx, newSet(2, 2)) + + lease := newLease("cold-lease-max", map[string]string{"pool": "cold"}) + Expect(k8sClient.Create(ctx, lease)).To(Succeed()) + _ = reconcileLease(ctx, lease) + + updated := getLease(ctx, lease.Name) + Expect(meta.IsStatusConditionTrue( + updated.Status.Conditions, + string(jumpstarterdevv1alpha1.LeaseConditionTypeUnsatisfiable), + )).To(BeTrue()) + }) + }) + + When("no pool matches the selector", func() { + It("should still fail fast rather than waiting forever", func() { + ctx := context.Background() + createSet(ctx, newSet(4, 0)) + + lease := newLease("no-pool-lease", map[string]string{"pool": "does-not-exist"}) + Expect(k8sClient.Create(ctx, lease)).To(Succeed()) + _ = reconcileLease(ctx, lease) + + updated := getLease(ctx, lease.Name) + Expect(meta.IsStatusConditionTrue( + updated.Status.Conditions, + string(jumpstarterdevv1alpha1.LeaseConditionTypeUnsatisfiable), + )).To(BeTrue()) + Expect(updated.Status.Ended).To(BeTrue()) + }) + }) +}) diff --git a/controller/internal/controller/suite_test.go b/controller/internal/controller/suite_test.go index 022f5e59e..851f7d026 100644 --- a/controller/internal/controller/suite_test.go +++ b/controller/internal/controller/suite_test.go @@ -39,6 +39,7 @@ import ( "sigs.k8s.io/controller-runtime/pkg/reconcile" jumpstarterdevv1alpha1 "github.com/jumpstarter-dev/jumpstarter/controller/api/v1alpha1" + virtualtargetv1alpha1 "github.com/jumpstarter-dev/jumpstarter/controller/api/virtualtarget/v1alpha1" "github.com/jumpstarter-dev/jumpstarter/controller/internal/oidc" // +kubebuilder:scaffold:imports ) @@ -84,6 +85,11 @@ var _ = BeforeSuite(func() { err = jumpstarterdevv1alpha1.AddToScheme(scheme.Scheme) Expect(err).NotTo(HaveOccurred()) + // The lease controller reads exporter sets to tell "nothing matches yet" + // from "nothing will ever match". + err = virtualtargetv1alpha1.AddToScheme(scheme.Scheme) + Expect(err).NotTo(HaveOccurred()) + // +kubebuilder:scaffold:scheme k8sClient, err = client.New(cfg, client.Options{Scheme: scheme.Scheme}) From d1ec3021cb408dd53950dd9a7a5e14681fadf111 Mon Sep 17 00:00:00 2001 From: Kirk Brauer Date: Sun, 30 Aug 2026 22:20:18 -0400 Subject: [PATCH 2/2] refactor(lease): extract exporter resolution from reconcileStatusExporterRef MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit reconcileStatusExporterRef was already at 29 of gocyclo's limit of 30, so asking it one more question about exporter sets put it over at 32. Move the block that resolves which exporters a lease could be given — the one it names, or everything its selector matches — into candidateExporters, which reports whether it has already settled the lease's status. The parent drops to 21 and reads as the sequence of decisions it is. No behaviour change; the same specs pass. Assisted-by: Claude Signed-off-by: Kirk Brauer --- .../internal/controller/lease_controller.go | 173 ++++++++++-------- 1 file changed, 98 insertions(+), 75 deletions(-) diff --git a/controller/internal/controller/lease_controller.go b/controller/internal/controller/lease_controller.go index bfa56ae2a..258dabb10 100755 --- a/controller/internal/controller/lease_controller.go +++ b/controller/internal/controller/lease_controller.go @@ -209,6 +209,98 @@ func (r *LeaseReconciler) reconcileStatusBeginEndTimes( return nil } +// candidateExporters resolves the exporters a lease could be given: the one it +// names, or every exporter its selector matches. +// +// It reports decided=true when it has already settled the lease's status and +// the caller should stop — the named exporter is missing, does not match, or is +// disabled; every match is disabled; or nothing matches yet and a pool can +// provide one. +func (r *LeaseReconciler) candidateExporters( + ctx context.Context, + result *ctrl.Result, + lease *jumpstarterdevv1alpha1.Lease, + selector labels.Selector, +) ([]jumpstarterdevv1alpha1.Exporter, bool, error) { + if lease.Spec.ExporterRef != nil { + var exporter jumpstarterdevv1alpha1.Exporter + if err := r.Get(ctx, types.NamespacedName{ + Namespace: lease.Namespace, + Name: lease.Spec.ExporterRef.Name, + }, &exporter); err != nil { + if k8serrors.IsNotFound(err) { + lease.SetStatusUnsatisfiable( + "ExporterNotFound", + "Requested exporter %s was not found", + lease.Spec.ExporterRef.Name, + ) + return nil, true, nil + } + return nil, false, fmt.Errorf("candidateExporters: failed to get requested exporter: %w", err) + } + if !selector.Empty() && !selector.Matches(labels.Set(exporter.Labels)) { + lease.SetStatusUnsatisfiable( + "SelectorMismatch", + "Requested exporter %s does not match selector %s", + exporter.Name, + metav1.FormatLabelSelector(&lease.Spec.Selector), + ) + return nil, true, nil + } + // Check if the explicitly requested exporter is disabled + if !exporter.IsEnabled() && !lease.Spec.AllowDisabled { + lease.SetStatusUnsatisfiable( + "ExporterDisabled", + "Requested exporter %s is disabled. "+ + "To lease a disabled exporter, set spec.allowDisabled: true on the Lease, "+ + "or use --allow-disabled with jmp create lease or jmp shell", + exporter.Name, + ) + return nil, true, nil + } + return []jumpstarterdevv1alpha1.Exporter{exporter}, false, nil + } + + // List all exporters matching selector + listed, err := r.ListMatchingExporters(ctx, lease, selector) + if err != nil { + return nil, false, fmt.Errorf("candidateExporters: failed to list matching exporters: %w", err) + } + if len(listed.Items) == 0 { + // Nothing matches yet, which is the normal state of a pool that + // keeps nothing warm. An exporter set that could provision a + // match makes this lease pending rather than unsatisfiable — + // and pending is what the set counts as demand to scale up on. + // Calling it unsatisfiable here ends the lease in this same + // reconcile, so the set would never see it. + set, err := r.exporterSetCanProvision(ctx, lease, selector) + if err != nil { + return nil, false, fmt.Errorf("candidateExporters: %w", err) + } + if set != "" { + lease.SetStatusPending( + "Provisioning", + "No exporter matches the selector yet; exporter set %s can provision one", + set, + ) + result.RequeueAfter = pendingRequeueAfter(lease) + return nil, true, nil + } + } + + // Filter out disabled exporters from selector-based listing + matchingExporters := filterOutDisabledExporters(listed.Items) + if len(matchingExporters) == 0 && len(listed.Items) > 0 { + lease.SetStatusUnsatisfiable( + "AllDisabled", + "All %d exporters matching the selector are disabled", + len(listed.Items), + ) + return nil, true, nil + } + return matchingExporters, false, nil +} + // Also manages LeaseConditionTypeUnsatisfiable and LeaseConditionTypePending func (r *LeaseReconciler) reconcileStatusExporterRef( ctx context.Context, @@ -247,81 +339,12 @@ func (r *LeaseReconciler) reconcileStatusExporterRef( return nil } - var matchingExporters []jumpstarterdevv1alpha1.Exporter - if lease.Spec.ExporterRef != nil { - var exporter jumpstarterdevv1alpha1.Exporter - if err := r.Get(ctx, types.NamespacedName{ - Namespace: lease.Namespace, - Name: lease.Spec.ExporterRef.Name, - }, &exporter); err != nil { - if k8serrors.IsNotFound(err) { - lease.SetStatusUnsatisfiable( - "ExporterNotFound", - "Requested exporter %s was not found", - lease.Spec.ExporterRef.Name, - ) - return nil - } - return fmt.Errorf("reconcileStatusExporterRef: failed to get requested exporter: %w", err) - } - if !selector.Empty() && !selector.Matches(labels.Set(exporter.Labels)) { - lease.SetStatusUnsatisfiable( - "SelectorMismatch", - "Requested exporter %s does not match selector %s", - exporter.Name, - metav1.FormatLabelSelector(&lease.Spec.Selector), - ) - return nil - } - // Check if the explicitly requested exporter is disabled - if !exporter.IsEnabled() && !lease.Spec.AllowDisabled { - lease.SetStatusUnsatisfiable( - "ExporterDisabled", - "Requested exporter %s is disabled. "+ - "To lease a disabled exporter, set spec.allowDisabled: true on the Lease, "+ - "or use --allow-disabled with jmp create lease or jmp shell", - exporter.Name, - ) - return nil - } - matchingExporters = []jumpstarterdevv1alpha1.Exporter{exporter} - } else { - // List all exporters matching selector - listed, err := r.ListMatchingExporters(ctx, lease, selector) - if err != nil { - return fmt.Errorf("reconcileStatusExporterRef: failed to list matching exporters: %w", err) - } - if len(listed.Items) == 0 { - // Nothing matches yet, which is the normal state of a pool that - // keeps nothing warm. An exporter set that could provision a - // match makes this lease pending rather than unsatisfiable — - // and pending is what the set counts as demand to scale up on. - // Calling it unsatisfiable here ends the lease in this same - // reconcile, so the set would never see it. - set, err := r.exporterSetCanProvision(ctx, lease, selector) - if err != nil { - return fmt.Errorf("reconcileStatusExporterRef: %w", err) - } - if set != "" { - lease.SetStatusPending( - "Provisioning", - "No exporter matches the selector yet; exporter set %s can provision one", - set, - ) - result.RequeueAfter = pendingRequeueAfter(lease) - return nil - } - } - // Filter out disabled exporters from selector-based listing - matchingExporters = filterOutDisabledExporters(listed.Items) - if len(matchingExporters) == 0 && len(listed.Items) > 0 { - lease.SetStatusUnsatisfiable( - "AllDisabled", - "All %d exporters matching the selector are disabled", - len(listed.Items), - ) - return nil - } + matchingExporters, decided, err := r.candidateExporters(ctx, result, lease, selector) + if err != nil { + return err + } + if decided { + return nil } approvedExporters, unmatchedDescriptions, err := r.attachMatchingPolicies(ctx, lease, matchingExporters)