support future leases, calculate effective begin/end/duration lease times #168
Conversation
lease's endTime was incorrectly using beginTime. This is only for the leases that effectively ended already, so not really used. But still should be fixed...
|
Important Review skippedReview was skipped due to path filters ⛔ Files ignored due to path filters (6)
CodeRabbit blocks several paths by default. You can override this behavior by explicitly including those paths in the path filters. For example, including You can disable this status message by setting the WalkthroughAdds BeginTime and EndTime to LeaseSpec, makes Duration nullable, introduces ReconcileLeaseTimeFields and protobuf emission changes, fixes GetLease/RequestLease handling, forbids BeginTime changes after start, enhances reconciler for scheduled starts/expirations and requeueing, and expands controller tests for scheduled and edge cases. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor Client
participant ClientSvc as Client Service
participant API as K8s API
participant Helper as ReconcileLeaseTimeFields
Client->>ClientSvc: UpdateLease(desired Spec: Begin/End/Duration?)
ClientSvc->>API: GET Lease
API-->>ClientSvc: Lease (Spec, Status)
alt Lease already started & BeginTime changed
ClientSvc-->>Client: error (cannot update BeginTime: lease has already started)
else
ClientSvc->>Helper: Reconcile(spec.BeginTime, spec.EndTime, spec.Duration)
Helper-->>ClientSvc: ok / error
alt ok
ClientSvc->>API: PATCH Lease (updated Spec fields if provided)
API-->>ClientSvc: Updated Lease
ClientSvc-->>Client: success
else
ClientSvc-->>Client: error (inconsistent times)
end
end
sequenceDiagram
autonumber
participant Controller as LeaseReconciler
participant API as K8s API
participant Clock as Now()
Controller->>API: Fetch Lease
alt Spec.BeginTime exists && Spec.BeginTime > Now && no ExporterRef
Controller->>Clock: now
Controller-->>Controller: Requeue after (Spec.BeginTime - now)
else
opt Status.BeginTime nil
Controller->>API: Set Status.BeginTime, mark Ready
end
Controller->>Clock: now
alt Spec.EndTime set
Controller-->>Controller: expiration = Spec.EndTime
else Spec.BeginTime & Spec.Duration set
Controller-->>Controller: expiration = Spec.BeginTime + Spec.Duration
else Spec.Duration set
Controller-->>Controller: expiration = Status.BeginTime + Spec.Duration
end
alt expiration > now
Controller-->>Controller: RequeueAfter = expiration - now
else
Controller->>API: Expire lease (mark ended)
end
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
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 |
michalskrivanek
force-pushed
the
endtime
branch
from
74fa578 to
e48ccb6
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (3)
api/v1alpha1/lease_types.go (1)
34-37: Add validation and clarify precedence for BeginTime/EndTime vs Duration.
- Validate invariants (e.g., EndTime >= BeginTime; and EndTime should not be before BeginTime+Duration if both provided, or define precedence/min logic).
- Consider kubebuilder validation or an admission webhook, and document mutability (e.g., BeginTime immutable after start).
internal/controller/lease_controller.go (2)
144-156: Honor Spec.EndTime as an upper bound for expiration.If EndTime is requested, expire no later than that.
- expiration := beginTime.Add(lease.Spec.Duration.Duration) + expiration := beginTime.Add(lease.Spec.Duration.Duration) + if lease.Spec.EndTime != nil && lease.Spec.EndTime.Time.Before(expiration) { + expiration = lease.Spec.EndTime.Time + }Ensure intended precedence between Duration and EndTime matches API expectations.
192-206: Set Pending("Scheduled") while waiting for future BeginTime.Improves visibility to clients that the lease is scheduled.
- result.RequeueAfter = waitDuration + lease.SetStatusPending("Scheduled", "Lease scheduled to begin at %s", lease.Spec.BeginTime.Time.Format(time.RFC3339)) + result.RequeueAfter = waitDuration
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
api/v1alpha1/lease_helpers.go(3 hunks)api/v1alpha1/lease_types.go(1 hunks)api/v1alpha1/zz_generated.deepcopy.go(1 hunks)internal/controller/lease_controller.go(3 hunks)internal/service/client/v1/client_service.go(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
internal/controller/lease_controller.go (1)
api/v1alpha1/lease_types.go (1)
Lease(76-82)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (6)
- GitHub Check: e2e-tests (ubuntu-24.04-arm)
- GitHub Check: e2e-tests (ubuntu-24.04)
- GitHub Check: e2e-tests-28d6b1cc3b49ab9ae176918ab9709a2e2522c97e
- GitHub Check: lint-go
- GitHub Check: tests
- GitHub Check: deploy-kind
🔇 Additional comments (3)
api/v1alpha1/zz_generated.deepcopy.go (1)
500-507: DeepCopy for BeginTime/EndTime looks correct.Nil-guarded pointer DeepCopy matches existing patterns. No issues.
api/v1alpha1/lease_helpers.go (1)
76-83: ToProtobuf time mapping looks good.Emits planned Begin/End from Spec and effective times/duration from Status with a sensible fallback to now. LGTM.
Also applies to: 84-94
internal/controller/lease_controller.go (1)
85-88: Rename to reconcileStatusBeginEndTimes is clear.Call-site update looks correct.
michalskrivanek
changed the title
michalskrivanek
force-pushed
the
endtime
branch
from
e48ccb6 to
805318f
Compare
|
michalskrivanek
force-pushed
the
endtime
branch
from
805318f to
f9e6c97
Compare
can you make this table part of the comments, or perhaps we should use it for the jumpstarter.dev documentation somewhere? :) It was hard for me to think about some of those combinations. |
michalskrivanek
force-pushed
the
endtime
branch
from
9e05ef3 to
913d59c
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
internal/controller/lease_controller_test.go(6 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
internal/controller/lease_controller_test.go (1)
api/v1alpha1/lease_helpers.go (2)
LeaseFromProtobuf(47-86)ReconcileLeaseTimeFields(26-45)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (6)
- GitHub Check: e2e-tests (ubuntu-24.04-arm)
- GitHub Check: e2e-tests-28d6b1cc3b49ab9ae176918ab9709a2e2522c97e
- GitHub Check: tests
- GitHub Check: e2e-tests (ubuntu-24.04)
- GitHub Check: lint-go
- GitHub Check: deploy-kind
michalskrivanek
force-pushed
the
endtime
branch
4 times, most recently
from
e7c838f to
7b67d4f
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
api/v1alpha1/lease_types.go (1)
31-31: Consider making Duration optional in the API surfaceIf Duration can be omitted (per comments), prefer a pointer with omitempty to avoid serializing 0 durations and to reflect optionality.
Example:
- Duration metav1.Duration `json:"duration"` + Duration *metav1.Duration `json:"duration,omitempty"`This will require adapting call sites and ToProtobuf to handle nil safely.
🧹 Nitpick comments (4)
api/v1alpha1/lease_types.go (1)
36-41: BeginTime/EndTime mutability guarantees need enforcement across entry pointsDocs say BeginTime is immutable after start and EndTime can be updated. Service layer enforces some of this; direct CR updates currently bypass it. Consider a validating webhook to enforce the same constraints when the CRD is used directly.
internal/controller/lease_controller.go (1)
197-211: Surface “scheduled” state to usersWhile requeueing until future BeginTime, consider setting Pending with a “Scheduled” reason to improve observability for clients (e.g., SetStatusPending("Scheduled", ...)).
api/v1alpha1/lease_helpers.go (2)
101-107: Only emit Duration when it’s specified (> 0)ToProtobuf always sets Duration, producing a 0s duration when it’s unspecified (e.g., EndTime‑only). Prefer omitting it to signal “unspecified”.
Apply:
- lease := cpb.Lease{ - Name: fmt.Sprintf("namespaces/%s/leases/%s", l.Namespace, l.Name), - Selector: metav1.FormatLabelSelector(&l.Spec.Selector), - Duration: durationpb.New(l.Spec.Duration.Duration), - Client: ptr.To(fmt.Sprintf("namespaces/%s/clients/%s", l.Namespace, l.Spec.ClientRef.Name)), - Conditions: conditions, - } + lease := cpb.Lease{ + Name: fmt.Sprintf("namespaces/%s/leases/%s", l.Namespace, l.Name), + Selector: metav1.FormatLabelSelector(&l.Spec.Selector), + Client: ptr.To(fmt.Sprintf("namespaces/%s/clients/%s", l.Namespace, l.Spec.ClientRef.Name)), + Conditions: conditions, + } + if l.Spec.Duration.Duration > 0 { + lease.Duration = durationpb.New(l.Spec.Duration.Duration) + }
117-127: Inject clock for testability (optional)EffectiveDuration uses time.Now(). Consider injecting a clock (e.g., controller‑runtime clock) to make tests deterministic and enable time travel without sleeps.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
api/v1alpha1/lease_helpers.go(3 hunks)api/v1alpha1/lease_types.go(1 hunks)api/v1alpha1/zz_generated.deepcopy.go(1 hunks)internal/controller/lease_controller.go(3 hunks)internal/controller/lease_controller_test.go(6 hunks)internal/service/client/v1/client_service.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- internal/service/client/v1/client_service.go
- api/v1alpha1/zz_generated.deepcopy.go
🧰 Additional context used
🧬 Code graph analysis (3)
internal/controller/lease_controller.go (1)
api/v1alpha1/lease_types.go (1)
Lease(80-86)
api/v1alpha1/lease_helpers.go (1)
api/v1alpha1/lease_types.go (2)
Lease(80-86)LeaseSpec(25-42)
internal/controller/lease_controller_test.go (3)
internal/service/helpers.go (1)
MatchLabels(3-13)api/v1alpha1/lease_types.go (1)
Lease(80-86)api/v1alpha1/lease_helpers.go (2)
LeaseFromProtobuf(44-83)ReconcileLeaseTimeFields(25-42)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (6)
- GitHub Check: e2e-tests (ubuntu-24.04)
- GitHub Check: e2e-tests-28d6b1cc3b49ab9ae176918ab9709a2e2522c97e
- GitHub Check: e2e-tests (ubuntu-24.04-arm)
- GitHub Check: lint-go
- GitHub Check: deploy-kind
- GitHub Check: tests
🔇 Additional comments (4)
api/v1alpha1/lease_types.go (1)
28-31: Duration doc vs validation mismatch (EndTime‑only)Comment says Duration can be omitted if EndTime is specified (incl. “EndTime − EffectiveBeginTime”). However, ReconcileLeaseTimeFields (api/v1alpha1/lease_helpers.go Lines 23–42) rejects Duration <= 0 unless both BeginTime and EndTime are provided. gRPC path will reject EndTime‑only, but controller tests accept it via direct K8s create. Please align semantics (either allow EndTime‑only in validation, or update docs/tests).
internal/controller/lease_controller.go (1)
145-154: Correct expiration precedence (EndTime > Begin+Duration > Status.Begin+Duration)This addresses the earlier bug and aligns with expected semantics. LGTM.
internal/controller/lease_controller_test.go (1)
805-840: EndTime‑only test conflicts with validation helpersThis test allows EndTime with Duration=0 and no BeginTime. ReconcileLeaseTimeFields (used by gRPC/service) rejects that (requires positive duration unless both Begin+End provided). Please align semantics across CR and gRPC (allow EndTime‑only in validator, or mark this path invalid and adjust the test once a webhook enforces validation).
api/v1alpha1/lease_helpers.go (1)
23-42: Clarify and reconcile timing semanticsFunction enforces Duration > 0 unless both BeginTime and EndTime are provided (then it derives Duration). This makes EndTime‑only invalid via gRPC, while controller/tests accept it via direct CR. Decide and unify:
- If EndTime‑only should be valid: relax the final check when EndTime != nil (allow Duration==0 to mean “until EndTime”).
- If it must be invalid: update API docs/tests to reflect that Duration is required unless both Begin and End are set.
michalskrivanek
force-pushed
the
endtime
branch
from
7b67d4f to
250bd4f
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
internal/service/controller_service.go (1)
612-621: Nil-deref risk: req.Duration may be nilCalling AsDuration() on a nil req.Duration will panic. Guard it.
Apply this diff:
- Spec: jumpstarterdevv1alpha1.LeaseSpec{ - ClientRef: corev1.LocalObjectReference{ - Name: client.Name, - }, - Duration: &metav1.Duration{Duration: req.Duration.AsDuration()}, - Selector: metav1.LabelSelector{ - MatchLabels: matchLabels, - MatchExpressions: matchExpressions, - }, - }, + Spec: jumpstarterdevv1alpha1.LeaseSpec{ + ClientRef: corev1.LocalObjectReference{ + Name: client.Name, + }, + Selector: metav1.LabelSelector{ + MatchLabels: matchLabels, + MatchExpressions: matchExpressions, + }, + }, } + if req.Duration != nil { + lease.Spec.Duration = &metav1.Duration{Duration: req.Duration.AsDuration()} + }
🧹 Nitpick comments (5)
api/v1alpha1/lease_types.go (1)
28-41: API shape LGTM; add validation/immutability guardsConsider enforcing invariants via validation:
- Reject zero/negative duration; ensure EndTime > BeginTime when both set.
- Make BeginTime immutable after start; allow EndTime edits for extend/shorten.
Implement via admission webhook or CRD XValidation where feasible.internal/controller/lease_controller.go (1)
145-154: Defensive guard for unset expirationIf no EndTime and no Duration, expiration stays zero and the lease would end immediately. Add a zero check to avoid accidental expiry.
- if expiration.Before(now) { + if expiration.IsZero() { + return nil + } + if expiration.Before(now) { lease.Expire(ctx) return nil } result.RequeueAfter = expiration.Sub(now) return nilAlso applies to: 160-162
internal/controller/lease_controller_test.go (1)
757-766: Slightly increase timeouts or avoid Truncate to reduce flakesUsing Truncate(time.Second) plus ~1s timeouts is tight under CI jitter. Consider bumping the Eventually timeout (e.g., +500ms) or avoid truncation where not required.
api/v1alpha1/lease_helpers.go (2)
23-43: Consider adding the 6-case matrix as function documentation.The function logic is sound, but as mentioned in past review comments, adding the 6-case matrix from the PR description would improve maintainability and help future developers understand the expected behavior for different input combinations.
Example documentation structure:
// ReconcileLeaseTimeFields calculates missing time fields and validates consistency // between BeginTime, EndTime, and Duration. Modifies pointers in place. +// +// Handles six cases: +// 1. Duration only (no begin/end): immediate start, runs for duration +// 2. EndTime only: invalid - cannot infer duration without BeginTime or explicit Duration +// 3. BeginTime + Duration: scheduled start at BeginTime, runs for duration +// 4. BeginTime + EndTime: scheduled window, duration computed from times +// 5. EndTime + Duration: scheduled end, BeginTime computed as EndTime - Duration +// 6. BeginTime + EndTime + Duration: validates all three are consistent func ReconcileLeaseTimeFields(beginTime, endTime **metav1.Time, duration **metav1.Duration) error {
38-41: Consider clarifying the error message for nil duration.The error message "duration must be positive" is technically correct but might be clearer if it distinguishes between missing and non-positive duration values.
- if *duration == nil || (*duration).Duration <= 0 { - return fmt.Errorf("duration must be positive") + if *duration == nil { + return fmt.Errorf("duration is required") + } + if (*duration).Duration <= 0 { + return fmt.Errorf("duration must be positive, got %v", (*duration).Duration) }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
api/v1alpha1/lease_helpers.go(3 hunks)api/v1alpha1/lease_types.go(1 hunks)api/v1alpha1/zz_generated.deepcopy.go(1 hunks)internal/controller/lease_controller.go(4 hunks)internal/controller/lease_controller_test.go(9 hunks)internal/service/client/v1/client_service.go(1 hunks)internal/service/controller_service.go(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- internal/service/client/v1/client_service.go
🧰 Additional context used
🧬 Code graph analysis (4)
internal/controller/lease_controller.go (1)
api/v1alpha1/lease_types.go (1)
Lease(80-86)
internal/service/controller_service.go (2)
internal/protocol/jumpstarter/v1/kubernetes.pb.go (6)
Time(140-146)Time(159-159)Time(174-176)LabelSelector(87-93)LabelSelector(106-106)LabelSelector(121-123)internal/protocol/jumpstarter/v1/jumpstarter.pb.go (3)
GetLeaseResponse(1168-1178)GetLeaseResponse(1191-1191)GetLeaseResponse(1206-1208)
internal/controller/lease_controller_test.go (2)
api/v1alpha1/lease_types.go (1)
Lease(80-86)api/v1alpha1/lease_helpers.go (2)
LeaseFromProtobuf(45-84)ReconcileLeaseTimeFields(25-43)
api/v1alpha1/lease_helpers.go (1)
api/v1alpha1/lease_types.go (2)
Lease(80-86)LeaseSpec(25-42)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (6)
- GitHub Check: e2e-tests (ubuntu-24.04-arm)
- GitHub Check: e2e-tests-28d6b1cc3b49ab9ae176918ab9709a2e2522c97e
- GitHub Check: e2e-tests (ubuntu-24.04)
- GitHub Check: deploy-kind
- GitHub Check: lint-go
- GitHub Check: tests
🔇 Additional comments (10)
api/v1alpha1/zz_generated.deepcopy.go (1)
498-511: DeepCopy for pointer fields is correctNil-safe copies for Duration, BeginTime, EndTime look good and avoid aliasing.
internal/service/controller_service.go (2)
536-537: Correct EndTime source fixedUsing lease.Status.EndTime for endTime is the right fix.
566-576: Response construction improvement is cleanBuilding resp and setting Duration only when non‑nil is correct.
internal/controller/lease_controller.go (3)
169-179: Setting Status.BeginTime on acquisition is correctUpdates begin time once exporter is assigned and marks Ready. Good.
199-211: Scheduled lease gating is correctRequeues until Spec.BeginTime, preventing premature exporter assignment. Good.
361-368: Nil-safe policy duration checkGuarding both MaximumDuration and Spec.Duration prevents nil deref and false comparisons. LGTM.
internal/controller/lease_controller_test.go (1)
112-131: Replaced fixed sleeps with Eventually—niceThis makes tests faster and less flaky. Good use of polling and shorter durations.
api/v1alpha1/lease_helpers.go (3)
26-32: Verify case 4 behavior matches intended design.The PR description states case 4 (BeginTime + EndTime without Duration) should be "invalid (duration computed as 0)". However, the current implementation computes the duration as
EndTime - BeginTime(Line 28), which for the example "2pm to 5pm" would be 3 hours (positive), passing validation at Line 39.Please confirm whether:
- Case 4 should be accepted (current behavior), or
- The code should reject providing both BeginTime and EndTime without an explicit Duration, or
- The PR matrix describes pre-fix behavior and is now outdated
55-82: LGTM! Properly parses and reconciles time fields.The refactored logic correctly:
- Parses BeginTime, EndTime, and Duration from protobuf when present
- Calls
ReconcileLeaseTimeFieldsto ensure internal consistency and derive missing fields- Assigns the reconciled values to
Lease.Spec- Propagates reconciliation errors appropriately
The earlier issue with missing EndTime parsing (noted in past review comments) has been addressed.
102-130: LGTM! Well-designed distinction between requested and actual times.The protobuf serialization correctly distinguishes between:
- Spec times (Lines 108-118): Requested/planned BeginTime, EndTime, and Duration
- Effective times (Lines 121-130): Actual Status.BeginTime, Status.EndTime, and computed EffectiveDuration
This dual representation allows API consumers to understand both the original request and what actually occurred. The computation of EffectiveDuration (Line 129) correctly handles:
- Active leases: uses
time.Now()to show current elapsed time- Completed leases: uses
Status.EndTimeto show final durationThe bug mentioned in the PR description (endTime incorrectly using beginTime) has been properly fixed at Line 125.
michalskrivanek
force-pushed
the
endtime
branch
from
250bd4f to
35b8498
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
internal/service/client/v1/client_service.go (1)
222-234: Fix BeginTime update check and avoid unintended clears; gate updates by request field presence
- Pointer comparison (Line 223) is wrong; different pointers with same timestamp will be treated as changes.
- Omitting fields in the request currently clears Spec fields (Lines 227-229), which can break reconciliation (e.g., nil Duration).
- Update BeginTime/EndTime/Duration only when explicitly present in the request; compare times, not pointers.
Apply this diff in this block:
- // BeginTime can only be updated before lease starts - if jlease.Status.ExporterRef != nil && desired.Spec.BeginTime != jlease.Spec.BeginTime { - return nil, fmt.Errorf("cannot update BeginTime: lease has already started") - } - - jlease.Spec.BeginTime = desired.Spec.BeginTime - jlease.Spec.Duration = desired.Spec.Duration - jlease.Spec.EndTime = desired.Spec.EndTime + // BeginTime can only be updated before lease starts; only if explicitly provided + if req.Lease.BeginTime != nil { + if jlease.Status.ExporterRef != nil { + // value-compare to avoid pointer inequality false positives + if jlease.Spec.BeginTime == nil || !jlease.Spec.BeginTime.Equal(desired.Spec.BeginTime) { + return nil, fmt.Errorf("cannot update BeginTime: lease has already started") + } + } + jlease.Spec.BeginTime = desired.Spec.BeginTime + } + // Update Duration only if provided; preserve existing otherwise + if req.Lease.Duration != nil { + jlease.Spec.Duration = desired.Spec.Duration + } + // Update EndTime only if provided; preserve existing otherwise + if req.Lease.EndTime != nil { + jlease.Spec.EndTime = desired.Spec.EndTime + }This prevents accidental clears and enforces BeginTime immutability after start without blocking no-op updates.
🧹 Nitpick comments (4)
internal/controller/lease_controller.go (2)
144-154: Defensive: handle “no expiration computed” explicitlyIf neither Spec.EndTime nor any Duration is set, expiration stays zero and the lease will be expired immediately. If you want to tolerate such CRs (e.g., misconfigurations outside gRPC), guard it:
- if expiration.Before(now) { + if expiration.IsZero() { + // No expiration computed; do not end or requeue + return nil + } + if expiration.Before(now) { lease.Expire(ctx) return nil } result.RequeueAfter = expiration.Sub(now) return nilOptional, since service-level validation normally prevents this state.
Also applies to: 160-162
361-368: Enforce MaximumDuration when BeginTime+EndTime are provided (Duration nil)Current check only applies when Spec.Duration is set. If the request uses BeginTime+EndTime with nil Duration, MaximumDuration is skipped. Consider deriving requested duration:
- if p.MaximumDuration != nil && lease.Spec.Duration != nil { - if lease.Spec.Duration.Duration > p.MaximumDuration.Duration { + // Determine requested duration: explicit Duration or EndTime-BeginTime + var reqDur *time.Duration + if lease.Spec.Duration != nil { + d := lease.Spec.Duration.Duration + reqDur = &d + } else if lease.Spec.BeginTime != nil && lease.Spec.EndTime != nil { + d := lease.Spec.EndTime.Sub(lease.Spec.BeginTime.Time) + if d > 0 { + reqDur = &d + } + } + if p.MaximumDuration != nil && reqDur != nil { + if *reqDur > p.MaximumDuration.Duration { // TODO: we probably should keep this on the list of approved exporters // but mark as excessive duration so we can report it on the status // of lease if no other options exist continue } }api/v1alpha1/lease_helpers.go (2)
54-74: Tight reconciliation logic; consider richer conflict/error messages.Logic is solid and matches the documented cases. Optionally improve debuggability by including values in the conflict error and by being explicit when end <= begin.
Apply this diff to enhance errors:
- calculated := (*endTime).Sub((*beginTime).Time) - if *duration != nil && (*duration).Duration > 0 && (*duration).Duration != calculated { - return fmt.Errorf("duration conflicts with begin_time and end_time") - } + calculated := (*endTime).Sub((*beginTime).Time) + if calculated <= 0 { + return fmt.Errorf("end_time must be after begin_time (begin=%s, end=%s)", (*beginTime).Time.Format(time.RFC3339), (*endTime).Time.Format(time.RFC3339)) + } + if *duration != nil && (*duration).Duration > 0 && (*duration).Duration != calculated { + return fmt.Errorf("duration %v conflicts with begin_time=%s and end_time=%s", + (*duration).Duration, + (*beginTime).Time.Format(time.RFC3339), + (*endTime).Time.Format(time.RFC3339)) + }
151-161: Clamp EffectiveDuration to non-negative to resist clock skew.If controller time is slightly behind Status.BeginTime (NTP skew), endTime.Sub(begin) can be negative. Safer to clamp at zero.
Apply this diff:
- // Final effective duration or current duration while active - lease.EffectiveDuration = durationpb.New(endTime.Sub(l.Status.BeginTime.Time)) + // Final effective duration or current duration while active; clamp to >= 0 for clock skew safety + if dur := endTime.Sub(l.Status.BeginTime.Time); dur > 0 { + lease.EffectiveDuration = durationpb.New(dur) + } else { + lease.EffectiveDuration = durationpb.New(0) + }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
api/v1alpha1/lease_helpers.go(3 hunks)api/v1alpha1/lease_types.go(1 hunks)api/v1alpha1/zz_generated.deepcopy.go(1 hunks)internal/controller/lease_controller.go(4 hunks)internal/controller/lease_controller_test.go(9 hunks)internal/service/client/v1/client_service.go(1 hunks)internal/service/controller_service.go(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- api/v1alpha1/zz_generated.deepcopy.go
🧰 Additional context used
🧬 Code graph analysis (5)
internal/controller/lease_controller.go (1)
api/v1alpha1/lease_types.go (1)
Lease(80-86)
internal/service/controller_service.go (2)
internal/protocol/jumpstarter/v1/kubernetes.pb.go (6)
Time(140-146)Time(159-159)Time(174-176)LabelSelector(87-93)LabelSelector(106-106)LabelSelector(121-123)internal/protocol/jumpstarter/v1/jumpstarter.pb.go (3)
GetLeaseResponse(1168-1178)GetLeaseResponse(1191-1191)GetLeaseResponse(1206-1208)
internal/service/client/v1/client_service.go (1)
api/v1alpha1/lease_helpers.go (1)
ReconcileLeaseTimeFields(53-74)
api/v1alpha1/lease_helpers.go (1)
api/v1alpha1/lease_types.go (2)
Lease(80-86)LeaseSpec(25-42)
internal/controller/lease_controller_test.go (1)
api/v1alpha1/lease_helpers.go (2)
LeaseFromProtobuf(76-115)ReconcileLeaseTimeFields(53-74)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (6)
- GitHub Check: deploy-kind
- GitHub Check: e2e-tests-28d6b1cc3b49ab9ae176918ab9709a2e2522c97e
- GitHub Check: e2e-tests (ubuntu-24.04-arm)
- GitHub Check: lint-go
- GitHub Check: e2e-tests (ubuntu-24.04)
- GitHub Check: tests
🔇 Additional comments (8)
internal/service/controller_service.go (1)
535-537: LGTM: EndTime fix and conditional Duration are correct
- Correctly populates EndTime from Status.EndTime.
- Duration is included only when set, matching nullable spec.
Also applies to: 566-576
api/v1alpha1/lease_types.go (1)
28-31: Docs/semantics look good—clarify EndTime-only acceptance across pathsThe nullable Duration and added BeginTime/EndTime align with behavior. One clarification: gRPC path validates Duration or Begin+End, while the controller tolerates EndTime-only (immediate until EndTime). Consider documenting whether EndTime-only is supported at the API level or only via direct CRD usage, for consistency.
Also applies to: 36-41
internal/controller/lease_controller.go (1)
197-211: LGTM: proper gating for future BeginTimeRequeues until requested BeginTime arrives, avoiding premature exporter assignment. Good.
internal/controller/lease_controller_test.go (1)
125-131: Tests: good move to Eventually and thorough scheduling coverage
- Replacing fixed sleeps with Eventually improves reliability and speed.
- Scenarios for BeginTime/EndTime/Duration interactions are well covered.
Also applies to: 737-766, 842-874
api/v1alpha1/lease_helpers.go (4)
23-53: Great addition: clear spec matrix in code.The embedded 6-case matrix makes the logic easy to follow and aligns with the PR discussion.
86-101: Protobuf → API parsing and reconciliation look correct.Begin/End/Duration are parsed, then reconciled to ensure consistency. This enables the supported patterns and blocks invalid ones as intended.
133-141: Good: Duration emitted only when specified/derived in Spec.Name, Selector, Client, Conditions, and Spec.Duration serialization look correct.
143-150: Spec times serialization LGTM.BeginTime/EndTime are emitted only when present in Spec, keeping Spec.EndTime meaningful when explicitly set.
…ease requests This effectively implements scheduled leases. - set EffectiveBeginTime once the lease is aquired, optionally wait with acquisition for after the BeginTime passes. - populate the EffectiveDuration before lease is ended so that clients see the duration so far (equivalent to: now - EffectiveBeginTime). - add BeginTime initialization to CreateLease request. - allow updating BeginTime by UpdateLease before the lease is acquired - lease can be created with acombination of BeginTime/EndTime/Duration, where missing BeginTime means "immediately". When querying leases that are pending or active the clients can see: BeginTime - if requested in CreateLease EffectiveBeginTime - the time lease become active. Either immediately or at(after) the requested BeginTime Duration - as requested EffectiveDuration - calculated, could be affected by subsequent lease extenstions as well as it could be shorten by acquisition happening later than requested in BeginTime
validation and behavior tests for Lease creation with BeginTime,EndTime,Duration
michalskrivanek
force-pushed
the
endtime
branch
from
35b8498 to
a5e3403
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
api/v1alpha1/lease_helpers.go (1)
53-74: Time-field reconciliation is consistent with the 6-case matrixRules for Begin/End/Duration cover all combinations, including validation for conflicts and positivity. LGTM.
🧹 Nitpick comments (5)
internal/service/controller_service.go (1)
566-576: Populate Duration conditionally — consider deriving when absentCurrent logic sets Duration only if Spec.Duration is set. If users specify BeginTime+EndTime (with nil Duration), consider also deriving Duration = EndTime−BeginTime for completeness in the response.
api/v1alpha1/lease_types.go (1)
28-41: API shape is good; consider CRD validations/immutability hints
- Duration as pointer with omitempty is right.
- Consider adding validation to reject zero/negative durations at the CRD level (+webhook or kubebuilder markers).
- If BeginTime must be immutable after start, a validation webhook check would help enforce it.
internal/controller/lease_controller.go (1)
144-162: Expiration logic is sound; consider early end when EndTime already past before startCurrently, if Spec.EndTime is in the past and the lease hasn’t started, we still acquire and then immediately expire. Consider ending immediately without acquiring to avoid transient exporter assignment:
+ // End immediately if EndTime already in the past and we haven't started + if lease.Spec.EndTime != nil && lease.Status.BeginTime == nil && + (lease.Spec.BeginTime == nil || !lease.Spec.BeginTime.After(now)) && + lease.Spec.EndTime.Time.Before(now) { + lease.Expire(ctx) + return nil + }This reduces churn and avoids momentary exporter flips.
internal/controller/lease_controller_test.go (1)
1101-1131: Good switch to Eventually; pad tight timeouts to reduce CI flakesSome waits (e.g., 1200ms for 1s events) are tight. Consider bumping timeouts to 1.5–2s in those cases to reduce flakiness on slow CI.
Also applies to: 1535-1548, 1650-1655
api/v1alpha1/lease_helpers.go (1)
133-149: ToProtobuf: includes requested and effective timesEmitting Spec times and effective Status-derived times is useful. After fixing the clamp above, this is solid.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
api/v1alpha1/lease_helpers.go(3 hunks)api/v1alpha1/lease_types.go(1 hunks)api/v1alpha1/zz_generated.deepcopy.go(1 hunks)internal/controller/lease_controller.go(4 hunks)internal/controller/lease_controller_test.go(9 hunks)internal/service/client/v1/client_service.go(1 hunks)internal/service/controller_service.go(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- internal/service/client/v1/client_service.go
🧰 Additional context used
🧬 Code graph analysis (4)
internal/controller/lease_controller_test.go (1)
api/v1alpha1/lease_helpers.go (2)
LeaseFromProtobuf(76-115)ReconcileLeaseTimeFields(53-74)
api/v1alpha1/lease_helpers.go (1)
api/v1alpha1/lease_types.go (2)
Lease(80-86)LeaseSpec(25-42)
internal/controller/lease_controller.go (1)
api/v1alpha1/lease_types.go (1)
Lease(80-86)
internal/service/controller_service.go (2)
internal/protocol/jumpstarter/v1/kubernetes.pb.go (6)
Time(140-146)Time(159-159)Time(174-176)LabelSelector(87-93)LabelSelector(106-106)LabelSelector(121-123)internal/protocol/jumpstarter/v1/jumpstarter.pb.go (3)
GetLeaseResponse(1168-1178)GetLeaseResponse(1191-1191)GetLeaseResponse(1206-1208)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (6)
- GitHub Check: tests
- GitHub Check: e2e-tests-28d6b1cc3b49ab9ae176918ab9709a2e2522c97e
- GitHub Check: lint-go
- GitHub Check: e2e-tests (ubuntu-24.04)
- GitHub Check: e2e-tests (ubuntu-24.04-arm)
- GitHub Check: deploy-kind
🔇 Additional comments (7)
api/v1alpha1/zz_generated.deepcopy.go (1)
498-512: DeepCopy for pointer fields looks correctNil-guarded allocations for Duration/BeginTime/EndTime are proper and consistent with controller-gen patterns. LGTM.
internal/service/controller_service.go (2)
536-537: Bugfix: EndTime now sourced from Status.EndTimeUsing lease.Status.EndTime.Time (instead of BeginTime) is the correct fix. LGTM.
621-623: RequestLease: Duration set only when providedThis change aligns with making Duration optional and avoids forcing zero durations. LGTM.
internal/controller/lease_controller.go (3)
169-179: Begin time stamping on first acquisitionSetting Status.BeginTime when ExporterRef appears and marking Ready is correct and minimal. LGTM.
198-211: Scheduled leases: defer acquisition until BeginTimeGating exporter assignment on future BeginTime and using RequeueAfter is correct and prevents premature allocation. LGTM.
363-369: Policy MaximumDuration guard handles all patternsComputing requestedDuration from either Spec.Duration or EndTime−BeginTime avoids nil derefs and enforces caps. LGTM.
api/v1alpha1/lease_helpers.go (1)
86-101: LeaseFromProtobuf: good—ensures consistency before persistingParsing all three fields and calling ReconcileLeaseTimeFields avoids invalid specs, and sets Begin/End/Duration coherently. LGTM.
Also applies to: 108-114
michalskrivanek
force-pushed
the
endtime
branch
from
5721fda to
cacdf9e
Compare
| _ = reconcileLease(ctx, lease2) | ||
| updatedLease = getLease(ctx, lease2Name) | ||
| return updatedLease.Status.ExporterRef != nil | ||
| }).WithTimeout(2500 * time.Millisecond).WithPolling(50 * time.Millisecond).Should(BeTrue()) |
mangelajo
left a comment
mangelajo
left a comment
There was a problem hiding this comment.
some of the tests belong to client_service tests, but we can reorg that later.
It looks great
2 participants
lease's endTime was incorrectly using beginTime. This is only for the leases that effectively ended already, so not really used. But still should be fixed...
Summary by CodeRabbit
New Features
Bug Fixes
Tests