Add resync-period flag for k8s native informers #4047
Conversation
This is to optinally help with cache inconsistencies Signed-off-by: Ricardo Aravena <ricardo.aravena@snowflake.com>
|
Welcome @sfc-gh-raravena! |
volcano-sh-bot
added
kind/bug
volcano-sh-bot
added
the
kind/documentation
|
[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.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
volcano-sh-bot
added
kind/feature
| @@ -32,6 +32,7 @@ import ( | |||
| const ( | |||
| defaultSchedulerName = "volcano" | |||
| defaultSchedulerPeriod = time.Second | |||
| defaultResyncPeriod = 5 * time.Minute | |||
There was a problem hiding this comment.
Can you give more details about why we need this resync perion, as I didn't see that the resync is set in upstream kubernetes components: )
There was a problem hiding this comment.
It is too small and will cause scheduler performance issue.
| @@ -633,7 +634,7 @@ func newSchedulerCache(config *rest.Config, schedulerNames []string, defaultQueu | |||
| } | |||
|
|
|||
| func (sc *SchedulerCache) addEventHandler() { | |||
| informerFactory := informers.NewSharedInformerFactory(sc.kubeClient, 0) | |||
| informerFactory := informers.NewSharedInformerFactory(sc.kubeClient, sc.resyncPeriod) | |||
| @@ -32,6 +32,7 @@ import ( | |||
| const ( | |||
| defaultSchedulerName = "volcano" | |||
| defaultSchedulerPeriod = time.Second | |||
| defaultResyncPeriod = 5 * time.Minute | |||
There was a problem hiding this comment.
It is too small and will cause scheduler performance issue.
Monokaix
removed
kind/bug
Monokaix
removed
kind/flake
There was a problem hiding this comment.
PR Overview
This PR adds a new resync-period flag for Kubernetes native informers to help mitigate cache inconsistencies.
- Adds a new flag and default value in options.go
- Passes the resync-period parameter through to the scheduler and cache constructors
- Updates the informer factory initialization in scheduler cache to use the new resync period
Reviewed Changes
| File | Description |
|---|---|
| cmd/scheduler/app/options/options.go | Adds a new command-line flag for resync period |
| pkg/scheduler/scheduler.go | Passes the resync period parameter to the cache constructor |
| pkg/scheduler/cache/cache.go | Updates the cache interface and informer factory initialization |
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
| @@ -542,7 +543,7 @@ func newDefaultAndRootQueue(vcClient vcclient.Interface, defaultQueue string) { | |||
| } | |||
| } | |||
|
|
|||
| func newSchedulerCache(config *rest.Config, schedulerNames []string, defaultQueue string, nodeSelectors []string, nodeWorkers uint32, ignoredProvisioners []string) *SchedulerCache { | |||
| func newSchedulerCache(config *rest.Config, schedulerNames []string, defaultQueue string, nodeSelectors []string, nodeWorkers uint32, ignoredProvisioners []string, resyncPeriod time.Duration) *SchedulerCache { | |||
There was a problem hiding this comment.
The new field 'resyncPeriod' in SchedulerCache is declared but not assigned within newSchedulerCache; ensure you set sc.resyncPeriod = resyncPeriod to properly propagate the flag value.
Copilot is powered by AI, so mistakes are possible. Review output carefully before use.
This is to optionally help with cache inconsistencies
What type of PR is this?
/kind feature
What this PR does / why we need it:
Which issue(s) this PR fixes:
Fixes # #4046
Special notes for your reviewer:
Does this PR introduce a user-facing change?