fix: support Prometheus duration format for windowSize in SchedulingShard CR#1371
Merged
itsomri merged 2 commits intokai-scheduler:mainfrom Apr 3, 2026
Merged
Conversation
…ield Replace *metav1.Duration with *model.Duration (github.com/prometheus/common/model) for the UsageParams.WindowSize field. metav1.Duration uses Go's time.ParseDuration internally which does not support the 'w' (week) or 'd' (day) units. This caused the kai-operator to crash-loop when windowSize was set to '1w' in the SchedulingShard CR: failed to list *v1.SchedulingShard: time: unknown unit \"w\" in duration \"1w\" model.Duration supports the full Prometheus duration format (ms, s, m, h, d, w, y), allowing users to set windowSize using human-friendly values like '1w' or '7d' as documented. Signed-off-by: Homer <homer.su@fongcon.com.tw>
Contributor
|
Important Review skippedAuto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughThis PR migrates the Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
Collaborator
|
@coderabbitai review |
Contributor
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (7)
test/e2e/suites/timeaware/config.go (1)
6-19: Import organization does not follow coding guidelines.The
github.com/prometheus/common/modelimport (line 18) is placed after internal packages. It should be grouped with other external dependencies.♻️ Suggested import reordering
import ( "context" "time" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/utils/ptr" + "github.com/prometheus/common/model" kaiv1 "github.com/kai-scheduler/KAI-scheduler/pkg/apis/kai/v1" kaiprometheus "github.com/kai-scheduler/KAI-scheduler/pkg/apis/kai/v1/prometheus" usagedbapi "github.com/kai-scheduler/KAI-scheduler/pkg/scheduler/cache/usagedb/api" "github.com/kai-scheduler/KAI-scheduler/test/e2e/modules/configurations" testcontext "github.com/kai-scheduler/KAI-scheduler/test/e2e/modules/context" - "github.com/prometheus/common/model" )As per coding guidelines: "Organize imports in three groups separated by blank lines: standard library, external dependencies, and internal packages".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/e2e/suites/timeaware/config.go` around lines 6 - 19, The import ordering is wrong: move the external package import "github.com/prometheus/common/model" into the external dependencies group (with other non-standard imports) so imports are grouped as standard library, external dependencies, then internal packages; update the import block in test/e2e/suites/timeaware/config.go (the import section containing metav1, ptr, kaiv1, kaiprometheus, usagedbapi, configurations, testcontext) to place the prometheus model import alongside other external imports, separating groups with blank lines to match the project's import organization guidelines.pkg/scheduler/cache/usagedb/fake/fake_with_history.go (1)
6-16: Import organization does not follow coding guidelines.Internal packages (
github.com/kai-scheduler/KAI-scheduler/...) should be separated from external dependencies (k8s.io/...) with a blank line.♻️ Suggested import reordering
import ( "math" "sync" "time" - "github.com/kai-scheduler/KAI-scheduler/pkg/scheduler/api/common_info" - "github.com/kai-scheduler/KAI-scheduler/pkg/scheduler/api/queue_info" - "github.com/kai-scheduler/KAI-scheduler/pkg/scheduler/cache/usagedb/api" v1 "k8s.io/api/core/v1" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + + "github.com/kai-scheduler/KAI-scheduler/pkg/scheduler/api/common_info" + "github.com/kai-scheduler/KAI-scheduler/pkg/scheduler/api/queue_info" + "github.com/kai-scheduler/KAI-scheduler/pkg/scheduler/cache/usagedb/api" )As per coding guidelines: "Organize imports in three groups separated by blank lines: standard library, external dependencies, and internal packages".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/scheduler/cache/usagedb/fake/fake_with_history.go` around lines 6 - 16, The import block in fake_with_history.go is not grouped per project style: move standard library imports (math, sync, time) first, then a blank line, then external dependencies (v1 "k8s.io/api/core/v1", metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"), then a blank line, and finally internal packages (github.com/kai-scheduler/... common_info, queue_info, api); adjust the import order around the existing symbols (common_info, queue_info, api, v1, metav1) to enforce the three-group separation.pkg/scheduler/cache/usagedb/api/interface.go (1)
6-10: Import organization does not follow coding guidelines.Imports should be organized in three groups: standard library, external dependencies, then internal packages. Currently, the internal package is listed first.
♻️ Suggested import reordering
import ( + "github.com/prometheus/common/model" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "github.com/kai-scheduler/KAI-scheduler/pkg/scheduler/api/queue_info" - "github.com/prometheus/common/model" - metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" )As per coding guidelines: "Organize imports in three groups separated by blank lines: standard library, external dependencies, and internal packages".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/scheduler/cache/usagedb/api/interface.go` around lines 6 - 10, The import block in interface.go is misordered: move internal package "github.com/kai-scheduler/KAI-scheduler/pkg/scheduler/api/queue_info" into the internal group and place external dependencies (github.com/prometheus/common/model and k8s.io/apimachinery/pkg/apis/meta/v1) in the external group, separating groups with a blank line; update the import section around the existing import declaration so it follows the three-group convention (standard, external, internal) and run gofmt/goimports to ensure proper formatting.pkg/scheduler/cache/usagedb/prometheus/prometheus.go (1)
6-22: Import organization does not follow coding guidelines.Internal packages are mixed with external dependencies. They should be in separate groups.
♻️ Suggested import reordering
import ( "context" "fmt" "time" "github.com/aptible/supercronic/cronexpr" - commonconstants "github.com/kai-scheduler/KAI-scheduler/pkg/common/constants" - "github.com/kai-scheduler/KAI-scheduler/pkg/scheduler/api/common_info" - "github.com/kai-scheduler/KAI-scheduler/pkg/scheduler/api/queue_info" - "github.com/kai-scheduler/KAI-scheduler/pkg/scheduler/cache/usagedb/api" - "github.com/kai-scheduler/KAI-scheduler/pkg/scheduler/log" promapi "github.com/prometheus/client_golang/api" promv1 "github.com/prometheus/client_golang/api/prometheus/v1" "github.com/prometheus/common/model" v1 "k8s.io/api/core/v1" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + + commonconstants "github.com/kai-scheduler/KAI-scheduler/pkg/common/constants" + "github.com/kai-scheduler/KAI-scheduler/pkg/scheduler/api/common_info" + "github.com/kai-scheduler/KAI-scheduler/pkg/scheduler/api/queue_info" + "github.com/kai-scheduler/KAI-scheduler/pkg/scheduler/cache/usagedb/api" + "github.com/kai-scheduler/KAI-scheduler/pkg/scheduler/log" )As per coding guidelines: "Organize imports in three groups separated by blank lines: standard library, external dependencies, and internal packages".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/scheduler/cache/usagedb/prometheus/prometheus.go` around lines 6 - 22, The import block in prometheus.go mixes standard library, external, and internal packages; reorder the imports in three groups separated by blank lines: first standard library (context, fmt, time), then external third-party packages (github.com/aptible/supercronic/cronexpr, promapi, promv1, model, v1, metav1), and finally internal project packages (commonconstants, common_info, queue_info, api, log) so the import grouping follows the repository coding guidelines; update the import section in pkg/scheduler/cache/usagedb/prometheus/prometheus.go accordingly.pkg/scheduler/cache/usagedb/prometheus/prometheus_test.go (1)
6-17: Import organization does not follow coding guidelines.The internal package (
github.com/kai-scheduler/KAI-scheduler/...) should be in a separate group after external dependencies.♻️ Suggested import reordering
import ( "testing" "time" "github.com/aptible/supercronic/cronexpr" - "github.com/kai-scheduler/KAI-scheduler/pkg/scheduler/cache/usagedb/api" "github.com/prometheus/common/model" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/utils/ptr" + + "github.com/kai-scheduler/KAI-scheduler/pkg/scheduler/cache/usagedb/api" )As per coding guidelines: "Organize imports in three groups separated by blank lines: standard library, external dependencies, and internal packages".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/scheduler/cache/usagedb/prometheus/prometheus_test.go` around lines 6 - 17, The import block in prometheus_test.go is not grouped into standard library, external dependencies, and internal packages; reorder the imports so the first group contains standard packages (testing, time), the second group contains external modules (github.com/aptible/supercronic/cronexpr, github.com/prometheus/common/model, github.com/stretchr/testify/assert, github.com/stretchr/testify/require, metav1 "k8s.io/apimachinery/pkg/apis/meta/v1", "k8s.io/utils/ptr"), and the final group contains the internal package (github.com/kai-scheduler/KAI-scheduler/pkg/scheduler/cache/usagedb/api), separating each group with a single blank line to comply with the project's import organization guideline.pkg/operator/operands/scheduler/resources_test.go (1)
1166-1166: Prefer a non-nil assertion before pointer dereference in validation callback.Add
require.NotNil(t, result.UsageParams.WindowSize)before dereferencing to avoid panic-style failures.Suggested diff
validate: func(t *testing.T, result *usagedbapi.UsageDBConfig) { assert.NotNil(t, result) assert.NotNil(t, result.UsageParams) assert.Equal(t, 10*time.Minute, result.UsageParams.HalfLifePeriod.Duration) + require.NotNil(t, result.UsageParams.WindowSize) assert.Equal(t, 20*time.Minute, time.Duration(*result.UsageParams.WindowSize)) assert.Equal(t, "http://prometheus:9090", result.ConnectionString) },🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/operator/operands/scheduler/resources_test.go` at line 1166, The test dereferences result.UsageParams.WindowSize directly which can panic if nil; add a non-nil assertion before dereferencing by calling require.NotNil(t, result.UsageParams.WindowSize) (or the project's equivalent) immediately before the assert.Equal line that reads time.Duration(*result.UsageParams.WindowSize), so the test fails cleanly if WindowSize is nil; update the validation callback in resources_test.go where result and UsageParams.WindowSize are checked.pkg/scheduler/cache/usagedb/api/usage_params_test.go (1)
381-382: Add explicit nil guard forUsageParamsbefore dereference.This test can fail with a panic instead of a clear assertion if
usageParamsis unexpectedly absent; addrequire.NotNil(t, config.UsageParams)before accessingWindowSize.Suggested diff
require.NoError(t, err) + require.NotNil(t, config.UsageParams) require.NotNil(t, config.UsageParams.WindowSize) assert.Equal(t, tt.expected, *config.UsageParams.WindowSize)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/scheduler/cache/usagedb/api/usage_params_test.go` around lines 381 - 382, Add an explicit nil guard for the UsageParams struct in the test before dereferencing WindowSize: in the test in usage_params_test.go assert that config.UsageParams is not nil (use require.NotNil(t, config.UsageParams)) immediately before the existing require.NotNil(t, config.UsageParams.WindowSize) / assert.Equal(t, tt.expected, *config.UsageParams.WindowSize) lines so the test fails with a clear assertion rather than panicking when UsageParams is absent.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@pkg/operator/operands/scheduler/resources_test.go`:
- Line 1166: The test dereferences result.UsageParams.WindowSize directly which
can panic if nil; add a non-nil assertion before dereferencing by calling
require.NotNil(t, result.UsageParams.WindowSize) (or the project's equivalent)
immediately before the assert.Equal line that reads
time.Duration(*result.UsageParams.WindowSize), so the test fails cleanly if
WindowSize is nil; update the validation callback in resources_test.go where
result and UsageParams.WindowSize are checked.
In `@pkg/scheduler/cache/usagedb/api/interface.go`:
- Around line 6-10: The import block in interface.go is misordered: move
internal package
"github.com/kai-scheduler/KAI-scheduler/pkg/scheduler/api/queue_info" into the
internal group and place external dependencies
(github.com/prometheus/common/model and k8s.io/apimachinery/pkg/apis/meta/v1) in
the external group, separating groups with a blank line; update the import
section around the existing import declaration so it follows the three-group
convention (standard, external, internal) and run gofmt/goimports to ensure
proper formatting.
In `@pkg/scheduler/cache/usagedb/api/usage_params_test.go`:
- Around line 381-382: Add an explicit nil guard for the UsageParams struct in
the test before dereferencing WindowSize: in the test in usage_params_test.go
assert that config.UsageParams is not nil (use require.NotNil(t,
config.UsageParams)) immediately before the existing require.NotNil(t,
config.UsageParams.WindowSize) / assert.Equal(t, tt.expected,
*config.UsageParams.WindowSize) lines so the test fails with a clear assertion
rather than panicking when UsageParams is absent.
In `@pkg/scheduler/cache/usagedb/fake/fake_with_history.go`:
- Around line 6-16: The import block in fake_with_history.go is not grouped per
project style: move standard library imports (math, sync, time) first, then a
blank line, then external dependencies (v1 "k8s.io/api/core/v1", metav1
"k8s.io/apimachinery/pkg/apis/meta/v1"), then a blank line, and finally internal
packages (github.com/kai-scheduler/... common_info, queue_info, api); adjust the
import order around the existing symbols (common_info, queue_info, api, v1,
metav1) to enforce the three-group separation.
In `@pkg/scheduler/cache/usagedb/prometheus/prometheus_test.go`:
- Around line 6-17: The import block in prometheus_test.go is not grouped into
standard library, external dependencies, and internal packages; reorder the
imports so the first group contains standard packages (testing, time), the
second group contains external modules (github.com/aptible/supercronic/cronexpr,
github.com/prometheus/common/model, github.com/stretchr/testify/assert,
github.com/stretchr/testify/require, metav1
"k8s.io/apimachinery/pkg/apis/meta/v1", "k8s.io/utils/ptr"), and the final group
contains the internal package
(github.com/kai-scheduler/KAI-scheduler/pkg/scheduler/cache/usagedb/api),
separating each group with a single blank line to comply with the project's
import organization guideline.
In `@pkg/scheduler/cache/usagedb/prometheus/prometheus.go`:
- Around line 6-22: The import block in prometheus.go mixes standard library,
external, and internal packages; reorder the imports in three groups separated
by blank lines: first standard library (context, fmt, time), then external
third-party packages (github.com/aptible/supercronic/cronexpr, promapi, promv1,
model, v1, metav1), and finally internal project packages (commonconstants,
common_info, queue_info, api, log) so the import grouping follows the repository
coding guidelines; update the import section in
pkg/scheduler/cache/usagedb/prometheus/prometheus.go accordingly.
In `@test/e2e/suites/timeaware/config.go`:
- Around line 6-19: The import ordering is wrong: move the external package
import "github.com/prometheus/common/model" into the external dependencies group
(with other non-standard imports) so imports are grouped as standard library,
external dependencies, then internal packages; update the import block in
test/e2e/suites/timeaware/config.go (the import section containing metav1, ptr,
kaiv1, kaiprometheus, usagedbapi, configurations, testcontext) to place the
prometheus model import alongside other external imports, separating groups with
blank lines to match the project's import organization guidelines.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 9d2d0a55-f582-49b6-9581-ea5926777be5
📒 Files selected for processing (10)
CHANGELOG.mdpkg/env-tests/timeaware/timeaware.gopkg/operator/operands/scheduler/resources_test.gopkg/scheduler/cache/usagedb/api/defaults.gopkg/scheduler/cache/usagedb/api/interface.gopkg/scheduler/cache/usagedb/api/usage_params_test.gopkg/scheduler/cache/usagedb/fake/fake_with_history.gopkg/scheduler/cache/usagedb/prometheus/prometheus.gopkg/scheduler/cache/usagedb/prometheus/prometheus_test.gotest/e2e/suites/timeaware/config.go
📊 Performance Benchmark ResultsComparing PR (
|
enoodle
reviewed
Apr 1, 2026
Collaborator
|
@JheSue You have to run |
Replace *model.Duration with *monitoringv1.Duration for the UsageParams.WindowSize field. model.Duration is backed by int64, which caused the CRD schema to be generated as an integer instead of string. monitoringv1.Duration is a string-based type with kubebuilder validation that ensures the CRD schema is generated as string and enforces valid Prometheus duration format (ms, s, m, h, d, w, y) at the API server level. Signed-off-by: Homer <homer.su@fongcon.com.tw>
Contributor
Author
|
@enoodle Thanks! After running make generate manifests, I noticed that the CRD schema for windowSize was generated as an integer due to model.Duration. This breaks support for Prometheus duration strings, so I switched it to monitoringv1.Duration to ensure it’s handled as a validated string. |
Merging this branch will increase overall coverage
Coverage by fileChanged files (no unit tests)
Please note that the "Total", "Covered", and "Missed" counts above refer to code statements instead of lines of code. The value in brackets refers to the test coverage of that file in the old version of the code. Changed unit test files
|
Collaborator
|
This is great! Thanks for the contribution! |
itsomri
approved these changes
Apr 3, 2026
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
3 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.
Description
The
windowSizefield inSchedulingShardCR uses*metav1.Durationwhich internally calls Go'stime.ParseDuration. This does not support Prometheus duration units likew(week) ord(day), causing both the kai-operator and kai-scheduler to crash whenwindowSize: 1wis set as documented:Replace
*metav1.Durationwith*model.Durationfromgithub.com/prometheus/common/model, which supports the full Prometheus duration format (ms, s, m, h, d, w, y) via built-inUnmarshalJSON/UnmarshalYAML.Related Issues
Checklist
Breaking Changes
Additional Notes
Summary by CodeRabbit
Bug Fixes
windowSizefield in the SchedulingShard custom resource to accept Prometheus duration format strings (e.g.,1w,7d,168h) instead of only Kubernetes duration format, resolving operator crash-loop errors when using unsupported duration units.