router: make the kvcache GC configurable and stoppable - #1634
Conversation
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
LiZhenCheng9527
left a comment
LiZhenCheng9527
left a comment
There was a problem hiding this comment.
I don't think these need to be placed in the CRD.
Will there be situations where different KV caches require different GC configurations?
|
@LiZhenCheng9527 the reason they are config args rather than a test-only hook: the e2e planned in #1328 runs the router as deployed, and the configmap is the only seam a test outside the process can reach to shrink the freshness window to seconds. a unit test can call gcStaleFields directly, an e2e cannot. if the surface still feels wide, gcScanSize is the one i would fold back into a const, the two durations are what the e2e needs |
Signed-off-by: aeron-gh <agab0323@gmail.com>
Signed-off-by: aeron-gh <agab0323@gmail.com>
e3b330c to
ecef11a
Compare
| // GCInterval overrides how often the ownership GC runs (default 1h). | ||
| // Any duration time.ParseDuration accepts, for example "30m". | ||
| GCInterval string `yaml:"gcInterval,omitempty"` | ||
| // GCFieldFreshDuration overrides how long an ownership field survives without | ||
| // a refresh before GC removes it (default 24h), same format as GCInterval. | ||
| // The default matches the runtime's mapping key expiry; lowering it only | ||
| // shortens how long ownership written by a departed pod lingers, it does not | ||
| // affect scoring, which filters owners by container start time. | ||
| GCFieldFreshDuration string `yaml:"gcFieldFreshDuration,omitempty"` | ||
| // GCScanSize overrides the SCAN COUNT hint per round (default 100). | ||
| GCScanSize int64 `yaml:"gcScanSize,omitempty"` |
There was a problem hiding this comment.
I don't think it's necessary to write such complicated comments
There was a problem hiding this comment.
done, all one liners now.
same habit as the other pr, working on it :)
|
/kind enhancement |
volcano-sh-bot
added
kind/enhancement
Signed-off-by: aeron-gh <agab0323@gmail.com>
/kind feature
What this PR does / why we need it:
Makes the kvcache-aware ownership GC configurable, stoppable, and audible, the three things #1580 asks for so its boundary behavior can be exercised by a test rather than waited out for an hour.
gcInterval,gcFieldFreshDurationandgcScanSizejoin the existingKVCacheAwareArgs, following theblockSizeToHashpattern. The two durations are strings parsed withtime.ParseDuration("30m"), nottime.Durationfields: plugin args go throughsigs.k8s.io/yaml, which bridges YAML to JSON, andtime.Durationdecodes there only as an integer of nanoseconds. Each rejected value logs a warning naming the field, because the surrounding unmarshal drops every argument silently when one of them is malformed.runGCnow selects on a stop channel, andStop()closes it (idempotent viasync.Once). The scheduler framework has no teardown hook today, so nothing in the router calls this yet; what it buys now is that a plugin built in a test can be torn down between cases instead of leaving an hourly ticker goroutine behind.klog.V(4)toklog.Warningf, so a router at default verbosity says something when GC stops making progress.A zero-valued
KVCacheAwarekeeps the previous defaults: the three values are read through small helpers that fall back to the existing constants, so the many tests that build the plugin as a struct literal behave exactly as before.kvCacheFieldFreshDurationis still 24h by default, matching the runtime's mapping key expiry documented inkv_cache_manager.py; it is only read by the GC cutoff, never by scoring, which filters owners by container start time.Which issue(s) this PR fixes:
Fixes #1580
Notes for reviewers:
gcStaleFieldsfunction (for router: kvcache GC scans one batch per hourly tick, a full pass takes days at modest keyspace sizes #1552). The two are independent in intent and touch different lines, but whichever lands first, I will rebase the other.NewKVCacheAware(the argument names have to survive the YAML to JSON bridge), a zero-value guard, GC honouring a short configured window against miniredis, andStop()being idempotent and actually ending the loop.Does this PR introduce a user-facing change?: