feat(api): Added support for automatic prefix selection by VPC ID#3456
feat(api): Added support for automatic prefix selection by VPC ID#3456bcavnvidia wants to merge 1 commit into
Conversation
|
Auto-sync is disabled for draft pull requests in this repository. Workflows must be run manually. Contributors can view more details about this message here. |
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Enterprise Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (2)
crates/api-core/src/tests/instance.rs (1)
110-115: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsolidate the triplicated
fixture_tenant_config()into a shared fixture. The identical helper now lives in three test modules; any future change to the fixture tenant contract must be applied in lockstep or the modules silently diverge. Extract a single definition into the existingcrates/api-core/src/tests/common/api_fixturesmodule (alongsidecreate_fixture_tenant) and import it in each site.
crates/api-core/src/tests/instance.rs#L110-L115: remove the local definition and import the shared helper.crates/api-core/src/tests/instance_config_update.rs#L46-L51: remove the local definition and import the shared helper.crates/api-core/src/tests/vpc_prefix.rs#L41-L46: remove the local definition and import the shared helper.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/api-core/src/tests/instance.rs` around lines 110 - 115, Consolidate the duplicated fixture_tenant_config helper by adding one shared definition to the existing api_fixtures module alongside create_fixture_tenant, then remove the local definitions and import the shared helper in crates/api-core/src/tests/instance.rs#L110-L115, crates/api-core/src/tests/instance_config_update.rs#L46-L51, and crates/api-core/src/tests/vpc_prefix.rs#L41-L46.crates/machine-controller/src/handler.rs (1)
6437-6443: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider centralising the generated-segment projection. The identical expression
interfaces.iter().filter_map(InstanceInterfaceConfig::generated_network_segment_id).collect_vec()now appears verbatim at three readiness/cleanup sites. Extracting a single helper (e.g.InstanceNetworkConfig::generated_network_segment_ids()or a free function over&[InstanceInterfaceConfig]) would keep the "which segments are generated VPC-prefix segments" contract in one place and guard against future divergence between readiness gating and cleanup.
crates/machine-controller/src/handler.rs#L6437-L6443: replace the inlinefilter_map(...).collect_vec()inInstanceState::WaitingForNetworkSegmentToBeReadywith the shared helper.crates/machine-controller/src/handler.rs#L7567-L7572: replace the identical expression inNetworkConfigUpdateState::WaitingForNetworkSegmentToBeReadywith the same helper.crates/machine-controller/src/handler.rs#L7929-L7932: replace the identical expression inrelease_network_segments_with_vpc_prefixwith the same helper.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/machine-controller/src/handler.rs` around lines 6437 - 6443, Centralize the generated network-segment projection in a shared helper, such as InstanceNetworkConfig::generated_network_segment_ids(), preserving the existing filter_map and collection behavior. Use that helper in crates/machine-controller/src/handler.rs:6437-6443 within InstanceState::WaitingForNetworkSegmentToBeReady, :7567-7572 within NetworkConfigUpdateState::WaitingForNetworkSegmentToBeReady, and :7929-7932 within release_network_segments_with_vpc_prefix, replacing each inline expression.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@crates/api-core/src/tests/instance.rs`:
- Around line 110-115: Consolidate the duplicated fixture_tenant_config helper
by adding one shared definition to the existing api_fixtures module alongside
create_fixture_tenant, then remove the local definitions and import the shared
helper in crates/api-core/src/tests/instance.rs#L110-L115,
crates/api-core/src/tests/instance_config_update.rs#L46-L51, and
crates/api-core/src/tests/vpc_prefix.rs#L41-L46.
In `@crates/machine-controller/src/handler.rs`:
- Around line 6437-6443: Centralize the generated network-segment projection in
a shared helper, such as InstanceNetworkConfig::generated_network_segment_ids(),
preserving the existing filter_map and collection behavior. Use that helper in
crates/machine-controller/src/handler.rs:6437-6443 within
InstanceState::WaitingForNetworkSegmentToBeReady, :7567-7572 within
NetworkConfigUpdateState::WaitingForNetworkSegmentToBeReady, and :7929-7932
within release_network_segments_with_vpc_prefix, replacing each inline
expression.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: cf68be2c-2460-4ede-bded-b2d185b58fe8
📒 Files selected for processing (18)
crates/admin-cli/src/instance/show/cmd.rscrates/agent/src/tests/full.rscrates/api-core/src/handlers/instance.rscrates/api-core/src/instance/mod.rscrates/api-core/src/network_segment/allocate.rscrates/api-core/src/tests/instance.rscrates/api-core/src/tests/instance_config_update.rscrates/api-core/src/tests/vpc_prefix.rscrates/api-db/src/instance_address.rscrates/api-db/src/instance_network_config.rscrates/api-db/src/vpc_prefix.rscrates/api-model/src/instance/config/network.rscrates/api-model/src/instance/status/network.rscrates/machine-controller/src/handler.rscrates/rpc/build.rscrates/rpc/proto/forge.protocrates/rpc/src/model/instance/config/network.rscrates/rpc/src/model/instance/status/network.rs
1 participant
This PR adds support for callers to auto-select VPC prefixes per interface by specifying only a VPC ID, which gets resolved to a VPC prefix with capacity. Some effort is made to avoid sparse allocation, but we're favoring simplicity and deadlock+serialization avoidance over strong avoidance of sparse-allocation for the moment. Not too hard to change later.
Related issues
#3110
Type of Change
Breaking Changes
Testing
Additional Notes
Partially addresses #3110
REST API work still needed.