fix(lease): let a pool provision for a lease that matches nothing yet - #1048
Open
kirkbrauer wants to merge 2 commits into
Open
fix(lease): let a pool provision for a lease that matches nothing yet#1048kirkbrauer wants to merge 2 commits into
kirkbrauer wants to merge 2 commits into
Conversation
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 <kirkebrauer@gmail.com>
kirkbrauer
added
bug
Contributor
|
Warning Review limit reachedNext included review available in 55 minutes. View limit detailsLimit details: You’ve used all 2 included reviews currently available. You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. Review configuration: ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (5)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
…rterRef 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 <kirkebrauer@gmail.com>
mangelajo
reviewed
Sep 1, 2026
| ctx context.Context, | ||
| result *ctrl.Result, | ||
| lease *jumpstarterdevv1alpha1.Lease, | ||
| selector labels.Selector, |
Member
There was a problem hiding this comment.
don't we have the selector in lease already?
mangelajo
reviewed
Sep 1, 2026
Comment on lines
334
to
340
| selector, err := lease.GetExporterSelector() | ||
| if err != nil { | ||
| return fmt.Errorf("reconcileStatusExporterRef: failed to get exporter selector: %w", err) | ||
| } else if selector.Empty() && lease.Spec.ExporterRef == nil { | ||
| lease.SetStatusInvalid("InvalidSelector", "The selector for the lease is empty, a selector is required") | ||
| return nil | ||
| } |
Member
There was a problem hiding this comment.
nit: you can move this block inside the function as well, and then no need to pass selector. It's not used anywhere later :)
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
An ExporterSet with
minReplicas: 0has no members until something asks for one, so a lease against it matches zero exporters. The lease controller called that unsatisfiable, andreconcileStatusEndedends an unsatisfiable lease in the same reconcile. The set's demand rule only counts leases that are pending and not ended, so it never saw the request — a pool that keeps nothing warm could never provision on demand, which is the case it exists for.The controller now asks whether an exporter set could provision a match before deciding. If one could, the lease becomes
Pending/Provisioningand is requeued, which is the state the set scales on. If none could — no pool matches the selector, or the only one that does is atmaxReplicas— the lease is unsatisfiable as before, so a typo in a selector still fails fast rather than hanging.This is the first thing the lease controller reads outside
jumpstarter.dev, so it needs the virtualtarget scheme registered and a read on exportersets in the Role the operator builds for it. Where that read is refused — a controller newer than its operator, or a cluster without the exporter set CRDs — the check reports "no pool" and behaviour is exactly what it was before.Three specs cover it. With the fix reverted, the one describing the bug fails and the other two (pool at ceiling, no matching pool) still pass.