Repository navigation
Return Released addresses to the pool after a class's releaseDelay - #11
Timofei Larkin (lllamnyp) wants to merge 1 commit into
Conversation
IPAddressClass gains spec.releaseDelay. Under Retain, the core clears the claimRef of a Released address once the delay has passed since its release, and the address becomes Available. An address annotated ipaddress.local.sdn.cozystack.io/held: "true" is exempt. The release is dated by a Bound condition on IPAddress, which the core now sets together with every phase it owns. AnnotationTrue names the value boolean annotations of the group take. Assisted-By: LLM Signed-off-by: Timofei Larkin <lllamnyp@gmail.com>
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the
|
IvanHunters
left a comment
There was a problem hiding this comment.
Review: return Released addresses to the pool after a class's releaseDelay
Conforms to the design. releaseAfterDelay (internal/controller/ipaddress_controller.go:193) acts only on a Released address that is not held, only for a class whose releaseDelay is set and positive, measures from the Bound/Released condition's lastTransitionTime, and requeues with a strict remaining > 0 so there is no negative or zero RequeueAfter. The clock is injectable. Tests pass.
[NIT] held pauses the action, not the clock
The held check returns early without touching the condition, so releaseDelay keeps elapsing while an address is quarantined. Removing held from an address that has sat quarantined longer than releaseDelay returns it to the pool on the next reconcile, with no cooling-off after the quarantine is lifted. This may be intended (lifting the hold means you want the address back), but it is not obvious and is not documented; a line in the operator guide would help.
Minor notes
markPhasesets the condition'slastTransitionTimefrommeta.SetStatusCondition(realtime.Now()), not the injectabler.Now, so the clock injection is partial; harmless in production but worth knowing.- Nothing enforces that
releaseDelayis meaningful only underRetain; underDeletethe field is silently inert. A CEL note or validation would match the "Retain only" wording. - See the cross-PR note on #10: a Retain class's
releaseDelayreturns addresses toAvailablerather than deleting them, which blocks the class-deletion finalizer. Worth documenting together.
LGTM.
Implements the release delay of the
ip-address-claimsproposal (Failure and edge cases: "A class may setreleaseDelay: after it elapses the controller clearsclaimRefand the address returns toAvailable. An address marked held by an annotation is exempt"). A review of cozystack/cozystack#3382 noted thatReleasedaddresses under the defaultRetainaccumulate, attributed to no one, until an admin clears them. Stacked on #10.Changes
IPAddressClass.spec.releaseDelay, ametav1.Duration. It is optional and applies underRetainonly. The CRD and deepcopy are regenerated.ipaddress.local.sdn.cozystack.io/held: "true"on anIPAddressexempts it from the delay.AnnotationTruenames the value that turns on the group's boolean annotations.Boundcondition onIPAddress. The core sets it together with each phase it owns:TrueforBound, andFalsewith reasonAvailable,ReleasedorPendingotherwise. ItslastTransitionTimedates the release. Both the address controller and the claim finalizer set phases through one helper,markPhase.releaseDelayhas passed since an address was released, the address controller clears itsclaimRefand the address becomesAvailableon the next pass.Releasedaddresses, so changing the delay takes effect. The delay is read from the class, not copied onto the address.docs/design.md§5 gains step 7 and the state diagram's new edge; the README lifecycle is updated.Behaviour to know
Releasedbefore this version has noBoundcondition. Its delay counts from the first pass that sets the condition, so upgrading does not immediately reissue old releases.ConflictandLostremain driver-owned and are not touched.Not in this PR
The
Associatedcondition from the proposal. It is driver-owned, alongsidestatus.associatedTo.go test ./internal/controller/andgolangci-lint runpass.