Open
Conversation
umswmayj
requested review from
PhilippMatthes,
SoWieMarkus,
auhlig,
juliusclausnitzer and
mblos
as code owners
📝 WalkthroughWalkthroughIntroduces a new Changes
Sequence DiagramsequenceDiagram
participant Client as HTTP Client
participant Handler as HTTPAPI Handler
participant K8s as Kubernetes API<br/>(ProjectQuota)
participant Metrics as Prometheus<br/>Metrics
Client->>Handler: PUT /commitments/v1/projects/{id}/quota
Handler->>Handler: Validate request method & config
Handler->>Handler: Extract project UUID & decode quota
Handler->>Handler: Validate UUID & check for overflow
alt Error Path
Handler->>Handler: Generate error response
Handler->>Metrics: Record error status code
Handler-->>Client: 400/405/500 Error
else Success Path
Handler->>K8s: Create or Update ProjectQuota CRD
K8s-->>Handler: CRD persisted
Handler->>Metrics: Record 204 status & duration
Handler-->>Client: 204 No Content
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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. Review rate limit: 0/1 reviews remaining, refill in 60 minutes.Comment |
Contributor
Test Coverage ReportTest Coverage 📊: 69.2% |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (5)
internal/scheduling/reservations/commitments/usage.go (1)
474-500: ⚡ Quick winAdd a focused unit test for RAM quota initialization contract.
Please add a test that asserts: fixed-ratio group → RAM per-AZ quota is non-null (
-1default), variable-ratio group → RAM per-AZ quota is null, including the fallback path for AZs seen only in aggregated usage.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/scheduling/reservations/commitments/usage.go` around lines 474 - 500, Add a unit test that verifies RAM per-AZ quota initialization for fixed- vs variable-ratio flavor groups: create two FlavorGroup inputs (one that makes FlavorGroupAcceptsCommitments return true and one false), call the usage-building logic that produces ramPerAZ (exercise the code paths that iterate allAZs and the fallback path where az is present only in usageByFlavorGroupAZ), and assert that for the fixed-ratio group each liquid.AZResourceUsageReport in ramPerAZ has Quota set to Some(-1) while for the variable-ratio group Quota is nil; include an AZ that appears only in usageByFlavorGroupAZ to validate the fallback branch that constructs a report and applies the same quota rule.internal/scheduling/reservations/commitments/config.go (1)
94-95: 💤 Low valueUpdate comment to include
EnableQuotaAPI.The comment lists the boolean fields that don't get defaults applied but doesn't include the newly added
EnableQuotaAPI.Suggested fix
- // Note: EnableChangeCommitmentsAPI, EnableReportUsageAPI, EnableReportCapacityAPI + // Note: EnableChangeCommitmentsAPI, EnableReportUsageAPI, EnableReportCapacityAPI, EnableQuotaAPI // are booleans where false is a valid value, so we don't apply defaults for them🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/scheduling/reservations/commitments/config.go` around lines 94 - 95, Update the comment that enumerates boolean fields which should not receive defaults to also include the new EnableQuotaAPI flag; locate the comment near the booleans (mentions EnableChangeCommitmentsAPI, EnableReportUsageAPI, EnableReportCapacityAPI) in config.go and add EnableQuotaAPI to the list so the documentation matches the code.internal/scheduling/reservations/commitments/api/quota_monitor.go (1)
29-33: ⚡ Quick winAdd 503 to pre-initialized status codes.
The handler returns
503 Service Unavailablewhen the quota API is disabled (seequota.goline 52), but this status code isn't pre-initialized. For consistency with the stated purpose of ensuring metrics appear before first request, add "503" to the list.Suggested fix
// Pre-initialize common status codes so they appear in Prometheus before the first request - for _, statusCode := range []string{"204", "400", "405", "500"} { + for _, statusCode := range []string{"204", "400", "405", "500", "503"} { m.requestCounter.WithLabelValues(statusCode) m.requestDuration.WithLabelValues(statusCode) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/scheduling/reservations/commitments/api/quota_monitor.go` around lines 29 - 33, The metrics pre-initialization loop omits the "503" status code so m.requestCounter and m.requestDuration won't be created before the first 503 response; update the slice of status codes in the loop that pre-initializes labels (the []string{"204","400","405","500"} used with m.requestCounter.WithLabelValues and m.requestDuration.WithLabelValues) to include "503" so the 503 Service Unavailable metric is present from startup.internal/scheduling/reservations/commitments/api/handler.go (1)
63-66: 💤 Low valueConsider adding
quotaAPIEnabledto the log message for consistency.The Init log message includes enablement status for other APIs but omits
quotaAPIEnabled. This could cause confusion when debugging configuration issues.Suggested fix
log.Info("commitments API initialized", "changeCommitmentsEnabled", api.config.EnableChangeCommitmentsAPI, "reportUsageEnabled", api.config.EnableReportUsageAPI, - "reportCapacityEnabled", api.config.EnableReportCapacityAPI) + "reportCapacityEnabled", api.config.EnableReportCapacityAPI, + "quotaAPIEnabled", api.config.EnableQuotaAPI)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/scheduling/reservations/commitments/api/handler.go` around lines 63 - 66, The Init log in handler.go that calls log.Info("commitments API initialized", "changeCommitmentsEnabled", api.config.EnableChangeCommitmentsAPI, "reportUsageEnabled", api.config.EnableReportUsageAPI, "reportCapacityEnabled", api.config.EnableReportCapacityAPI) is missing the quota flag; update that log.Info invocation to also include "quotaAPIEnabled", api.config.EnableQuotaAPI so the quotas enablement is logged consistently with the other API feature flags.internal/scheduling/reservations/commitments/api/quota_test.go (1)
352-354: Consider usingtestlib.Ptrinstead of the customboolPtrhelper.Replace the custom pointer helper with
testlib.Ptr, which is the standard pattern for pointer values in test cases.Suggested fix
Add the import:
+ testlib "github.com/cobaltcore-dev/cortex/pkg/testing"Then remove the helper and update its usage:
-func boolPtr(b bool) *bool { - return &b -}- enableQuota: boolPtr(false), + enableQuota: testlib.Ptr(false),🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/scheduling/reservations/commitments/api/quota_test.go` around lines 352 - 354, Remove the custom helper function boolPtr and replace all its usages with the standard testlib.Ptr helper; specifically, add the testlib import to the test file, delete the boolPtr(b bool) *bool function, and change calls like boolPtr(true) / boolPtr(false) to testlib.Ptr(true) / testlib.Ptr(false) so tests use the shared pointer helper instead of a local helper.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@internal/scheduling/reservations/commitments/api/handler.go`:
- Around line 63-66: The Init log in handler.go that calls log.Info("commitments
API initialized", "changeCommitmentsEnabled",
api.config.EnableChangeCommitmentsAPI, "reportUsageEnabled",
api.config.EnableReportUsageAPI, "reportCapacityEnabled",
api.config.EnableReportCapacityAPI) is missing the quota flag; update that
log.Info invocation to also include "quotaAPIEnabled", api.config.EnableQuotaAPI
so the quotas enablement is logged consistently with the other API feature
flags.
In `@internal/scheduling/reservations/commitments/api/quota_monitor.go`:
- Around line 29-33: The metrics pre-initialization loop omits the "503" status
code so m.requestCounter and m.requestDuration won't be created before the first
503 response; update the slice of status codes in the loop that pre-initializes
labels (the []string{"204","400","405","500"} used with
m.requestCounter.WithLabelValues and m.requestDuration.WithLabelValues) to
include "503" so the 503 Service Unavailable metric is present from startup.
In `@internal/scheduling/reservations/commitments/api/quota_test.go`:
- Around line 352-354: Remove the custom helper function boolPtr and replace all
its usages with the standard testlib.Ptr helper; specifically, add the testlib
import to the test file, delete the boolPtr(b bool) *bool function, and change
calls like boolPtr(true) / boolPtr(false) to testlib.Ptr(true) /
testlib.Ptr(false) so tests use the shared pointer helper instead of a local
helper.
In `@internal/scheduling/reservations/commitments/config.go`:
- Around line 94-95: Update the comment that enumerates boolean fields which
should not receive defaults to also include the new EnableQuotaAPI flag; locate
the comment near the booleans (mentions EnableChangeCommitmentsAPI,
EnableReportUsageAPI, EnableReportCapacityAPI) in config.go and add
EnableQuotaAPI to the list so the documentation matches the code.
In `@internal/scheduling/reservations/commitments/usage.go`:
- Around line 474-500: Add a unit test that verifies RAM per-AZ quota
initialization for fixed- vs variable-ratio flavor groups: create two
FlavorGroup inputs (one that makes FlavorGroupAcceptsCommitments return true and
one false), call the usage-building logic that produces ramPerAZ (exercise the
code paths that iterate allAZs and the fallback path where az is present only in
usageByFlavorGroupAZ), and assert that for the fixed-ratio group each
liquid.AZResourceUsageReport in ramPerAZ has Quota set to Some(-1) while for the
variable-ratio group Quota is nil; include an AZ that appears only in
usageByFlavorGroupAZ to validate the fallback branch that constructs a report
and applies the same quota rule.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 8cf4a854-2548-4c45-a1a6-71ba95ad9515
📒 Files selected for processing (11)
api/v1alpha1/project_quota_types.goapi/v1alpha1/zz_generated.deepcopy.gohelm/library/cortex/files/crds/cortex.cloud_projectquotas.yamlinternal/scheduling/reservations/commitments/api/handler.gointernal/scheduling/reservations/commitments/api/info.gointernal/scheduling/reservations/commitments/api/info_test.gointernal/scheduling/reservations/commitments/api/quota.gointernal/scheduling/reservations/commitments/api/quota_monitor.gointernal/scheduling/reservations/commitments/api/quota_test.gointernal/scheduling/reservations/commitments/config.gointernal/scheduling/reservations/commitments/usage.go
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
1 participant
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.
No description provided.