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..258dabb10 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 @@ -207,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, @@ -245,60 +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) - } - // 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) @@ -557,6 +603,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})