Add toggle for nova input host safety measure#616
Merged
PhilippMatthes merged 1 commit intomainfrom Mar 24, 2026
Merged
Conversation
PhilippMatthes
requested review from
SoWieMarkus,
auhlig,
juliusclausnitzer,
mblos and
umswmayj
as code owners
📝 WalkthroughWalkthroughThis PR introduces a config-driven feature flag for Nova scheduler's host filtering behavior. It adds an Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
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 docstrings
🧪 Generate unit tests (beta)
Comment |
Contributor
Test Coverage ReportTest Coverage 📊: 68.0% |
SoWieMarkus
approved these changes
Mar 24, 2026
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
internal/scheduling/nova/external_scheduler_api_test.go (1)
279-279: Consider adding a test case forNovaLimitHostsToRequest=true.The HTTP handler tests use
HTTPAPIConfig{}(zero value), which meansNovaLimitHostsToRequestdefaults tofalseand the filtering path is not exercised in integration. WhileTestLimitHostsToRequestthoroughly tests the filtering function itself, adding a test case withNovaLimitHostsToRequest: truewould verify the conditional integration inNovaExternalScheduler.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/scheduling/nova/external_scheduler_api_test.go` at line 279, Add an integration test that constructs the HTTP API with NovaLimitHostsToRequest enabled and exercises the handler path that triggers the host-filtering logic: create an API instance using NewAPI(HTTPAPIConfig{NovaLimitHostsToRequest: true}, delegate) (cast to *httpAPI) and send the same request used in existing handler tests, then assert that the response reflects the filtered host list; this will verify the conditional branch in NovaExternalScheduler integrates with the LimitHostsToRequest behavior already covered by TestLimitHostsToRequest.
🤖 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/nova/external_scheduler_api_test.go`:
- Line 279: Add an integration test that constructs the HTTP API with
NovaLimitHostsToRequest enabled and exercises the handler path that triggers the
host-filtering logic: create an API instance using
NewAPI(HTTPAPIConfig{NovaLimitHostsToRequest: true}, delegate) (cast to
*httpAPI) and send the same request used in existing handler tests, then assert
that the response reflects the filtered host list; this will verify the
conditional branch in NovaExternalScheduler integrates with the
LimitHostsToRequest behavior already covered by TestLimitHostsToRequest.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 9b8e8d65-d47f-475e-9063-ed2338779a6c
📒 Files selected for processing (4)
cmd/main.gohelm/bundles/cortex-nova/values.yamlinternal/scheduling/nova/external_scheduler_api.gointernal/scheduling/nova/external_scheduler_api_test.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
2 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.
With this change we can now toggle the
limitHostsToRequestfunctionality inside the external scheduler api for nova. This is needed because we will completely ignore nova's input soon, and nova will handle keyerrors for unknown proposed hosts appropriately. Thus, before we will completely remove this safety measure from the code, we add this toggle to turn it off.