Remove CPI v1 testing#2757
Open
aramprice wants to merge 11 commits into
Open
Conversation
in all but one ci task this was set to `false` already.
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughThe PR updates CPI API handling to use version 2 defaults, changes external CPI wrapper response handling, and adds Suggested reviewers
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✨ 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. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 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.
Inline comments:
In `@src/bosh-director/lib/clouds/external_cpi_response_wrapper.rb`:
- Around line 38-50: Reject CPI API v1 in the wrapper because `create_vm` and
`attach_disk` now assume v2-style responses and no longer adapt v1 behavior.
Update `check_cpi_api_support` (and any related version gating in
`ExternalCpiResponseWrapper`) so versions below 2 are treated as unsupported, or
otherwise restore the v1 response handling in `create_vm` and `attach_disk` to
match the director/CPI contract.
- Around line 47-50: Remove the redundant attach_disk delegator from
ExternalCpiResponseWrapper so the class only keeps the generic attach_disk
method already defined earlier; this duplicate definition is what triggers
Lint/DuplicateMethods. Update the attach_disk handling in
ExternalCpiResponseWrapper by deleting the stale method body that wraps
`@cpi.attach_disk` and returns cpi_response, and keep the single shared
implementation used for other CPI response wrappers.
In
`@src/bosh-director/spec/unit/bosh/director/deployment_plan/stages/update_stage_functional_spec.rb`:
- Line 74: The functional spec for UpdateStage is now using CPI API v2, but the
wrapper stub in the affected examples still returns the old single-value
create_vm result, so it no longer validates the v2 response shape. Update the
stubs in the UpdateStage spec to return the v2 create_vm tuple from the wrapper
double, and make sure the expectations in the examples that cover network
settings still exercise the returned value through the relevant helper methods
in UpdateStage.
In
`@src/bosh-director/spec/unit/bosh/director/deployment_plan/steps/create_vm_step_spec.rb`:
- Line 15: The create VM specs are still using the old 3-item wrapper shape even
though these contexts opt into CPI API v2. Update the `create_vm_response` in
`create_vm_step_spec` and the deep-copy example responses to use the v2 two-item
`create_vm` response shape, and make sure the examples tied to `CreateVmStep`
and `cpi_api_version` reflect the new API contract consistently.
In `@src/tasks/spec.rake`:
- Around line 88-90: The multitask prerequisites are duplicating the repo-root
unit suite because `spec:unit:release` and `spec:unit:integration_support` both
resolve to the same `rspec`/`parallel_rspec` execution path. Update the
`parallel` task in `spec.rake` so it references a distinct `integration_support`
task chain instead of reusing `spec:unit:release:parallel`, and make the
corresponding adjustment in the related
`spec:unit:release`/`spec:unit:integration_support` task definitions to ensure
each prerequisite runs a different suite.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 2b06a7ec-b33e-4f07-9bdb-608037c6e0e3
📒 Files selected for processing (18)
ci/pipeline.ymlci/tasks/test-rake-task.ymlsrc/bosh-director/lib/clouds/external_cpi_response_wrapper.rbsrc/bosh-director/spec/unit/bosh/director/az_cloud_factory_spec.rbsrc/bosh-director/spec/unit/bosh/director/cloud_factory_spec.rbsrc/bosh-director/spec/unit/bosh/director/deployment_plan/compilation_instance_pool_spec.rbsrc/bosh-director/spec/unit/bosh/director/deployment_plan/stages/package_compile_stage_spec.rbsrc/bosh-director/spec/unit/bosh/director/deployment_plan/stages/update_stage_functional_spec.rbsrc/bosh-director/spec/unit/bosh/director/deployment_plan/steps/create_vm_step_spec.rbsrc/bosh-director/spec/unit/bosh/director/problem_handlers/missing_vm_spec.rbsrc/bosh-director/spec/unit/bosh/director/problem_handlers/unresponsive_agent_spec.rbsrc/bosh-director/spec/unit/clouds/external_cpi_response_wrapper_spec.rbsrc/bosh-director/spec/unit/clouds/external_cpi_spec.rbsrc/spec/integration/cpi_versions/director_cpi_v1_spec.rbsrc/spec/integration/unmanaged_persistent_disks_spec.rbsrc/spec/integration_support/bin/dummy_cpisrc/spec/integration_support/sandbox.rbsrc/tasks/spec.rake
💤 Files with no reviewable changes (4)
- src/spec/integration/cpi_versions/director_cpi_v1_spec.rb
- src/bosh-director/spec/unit/clouds/external_cpi_response_wrapper_spec.rb
- ci/pipeline.yml
- src/spec/integration/unmanaged_persistent_disks_spec.rb
aramprice
force-pushed
the
remove-cpi-v1-testing
branch
from
d82b9c1 to
8b363e5
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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.
Inline comments:
In
`@src/bosh-director/spec/unit/bosh/director/deployment_plan/stages/package_compile_stage_spec.rb`:
- Line 145: The package compile stage spec is still exercising the CPI v1 path
because only Config.preferred_cpi_api_version is changed and cloud.info remains
unstubbed. Update the setup around CloudFactory in package_compile_stage_spec to
stub cloud.info with a v2-compatible response so the factory actually selects
API version 2, and then adjust the create_vm expectations/stubs to use the v2
contract rather than the bare-CID v1 form.
In `@src/spec/integration_support/bin/dummy_cpi`:
- Line 66: The dummy CPI launcher now always instantiates DummyV2, which ignores
any configured API version and can silently run tests against the wrong
contract. Update the bin/dummy_cpi entrypoint to either route non-2 versions
through the existing version-specific CPI selection logic or explicitly fail
fast when the requested API version is not 2; use the version value from the
generated cpi_config/context path so IntegrationSupport::Sandbox overrides are
enforced consistently.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 384fbf29-1edd-4fd6-9650-c2e116a20538
📒 Files selected for processing (18)
ci/pipeline.ymlci/tasks/test-rake-task.ymlsrc/bosh-director/lib/clouds/external_cpi_response_wrapper.rbsrc/bosh-director/spec/unit/bosh/director/az_cloud_factory_spec.rbsrc/bosh-director/spec/unit/bosh/director/cloud_factory_spec.rbsrc/bosh-director/spec/unit/bosh/director/deployment_plan/compilation_instance_pool_spec.rbsrc/bosh-director/spec/unit/bosh/director/deployment_plan/stages/package_compile_stage_spec.rbsrc/bosh-director/spec/unit/bosh/director/deployment_plan/stages/update_stage_functional_spec.rbsrc/bosh-director/spec/unit/bosh/director/deployment_plan/steps/create_vm_step_spec.rbsrc/bosh-director/spec/unit/bosh/director/problem_handlers/missing_vm_spec.rbsrc/bosh-director/spec/unit/bosh/director/problem_handlers/unresponsive_agent_spec.rbsrc/bosh-director/spec/unit/clouds/external_cpi_response_wrapper_spec.rbsrc/bosh-director/spec/unit/clouds/external_cpi_spec.rbsrc/spec/integration/cpi_versions/director_cpi_v1_spec.rbsrc/spec/integration/unmanaged_persistent_disks_spec.rbsrc/spec/integration_support/bin/dummy_cpisrc/spec/integration_support/sandbox.rbsrc/tasks/spec.rake
💤 Files with no reviewable changes (4)
- src/spec/integration/cpi_versions/director_cpi_v1_spec.rb
- src/spec/integration/unmanaged_persistent_disks_spec.rb
- src/bosh-director/spec/unit/clouds/external_cpi_response_wrapper_spec.rb
- ci/pipeline.yml
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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.
Inline comments:
In `@src/spec/integration_support/dummy_cpi.rb`:
- Around line 381-384: Version sorting in the stemcell collection is effectively
doing nothing because the hashes built in `dummy_cpi.rb` use string keys from
YAML, while the final `sort` block in `latest_stemcell` reads `a[:version]` and
`b[:version]`. Update the sort logic to use the actual version key on those
result hashes (or normalize keys before sorting) so the list is ordered
numerically by version rather than by filesystem order. Refer to the
`latest_stemcell` collection pipeline and its final `sort` call when making the
change.
- Around line 141-150: The delete_vm flow in DummyCPI currently derives
agent_pid with vm_cid.to_i, which can turn malformed CIDs into 0 and
accidentally signal the current process group. Update delete_vm to resolve the
VM via `@vm_repo` first, and only call Process.kill in DummyCPI::delete_vm when a
matching record exists and its PID is valid. Keep the existing validation and
rescue behavior, but add a guard so invalid or missing VM CIDs never reach the
kill path.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: e6c88b5b-aeb5-4bd8-a7e2-63622bd4d49f
📒 Files selected for processing (7)
src/spec/integration_support/bin/dummy_cpisrc/spec/integration_support/clouds/dummy.rbsrc/spec/integration_support/clouds/dummy_v2.rbsrc/spec/integration_support/dummy_cpi.rbsrc/spec/integration_support/sandbox.rbsrc/spec/integration_support/spec/dummy_cpi_spec.rbsrc/tasks/spec.rake
💤 Files with no reviewable changes (2)
- src/spec/integration_support/clouds/dummy_v2.rb
- src/spec/integration_support/clouds/dummy.rb
aramprice
requested review from
a team,
Ivaylogi98 and
ystros
and removed request for
a team
Phase 1: Simplify ExternalCpiResponseWrapper by removing v1 branches. - create_vm: no longer wraps a bare string result in an array; v2+ CPIs always return [vm_cid, network_settings] directly - attach_disk: no longer returns nil for v1; always validates and returns the disk hint - create_stemcell: condense conditional to a single expression Phase 2: dummy_cpi now unconditionally uses DummyV2. The case statement that dispatched on the requested api_version is removed; v1 was the only other branch and it is no longer supported. Phase 3: Remove all v1 test stubs and fixtures. - Delete spec/integration/cpi_versions/director_cpi_v1_spec.rb entirely - Remove 'when CPI is v1' context from unmanaged_persistent_disks_spec.rb - Change sandbox default dummy_cpi_api_version from 1 to 2 - Remove 'when cpi_version is 1' describe block from external_cpi_response_wrapper_spec.rb - Update preferred_cpi_api_version and cpi_api_version stubs from 1 to 2 in: create_vm_step_spec, cloud_factory_spec, az_cloud_factory_spec, package_compile_stage_spec, unresponsive_agent_spec, update_stage_functional_spec, compilation_instance_pool_spec Fix pre-existing bug in external_cpi_spec.rb: replace undefined asset_path() helper call with an explicit File.expand_path relative to __dir__, pointing at spec/assets/bin/dummy_cpi. After removing the v1 ExternalCpiResponseWrapper branch that wrapped a bare string vm_cid in an array, these specs were failing because their cloud.create_vm mocks still returned a plain string or nil, which the step code then tried to index into as if it were an array. Update the mocks to return [vm_cid, network_settings] tuples as a v2 CPI would, keeping the behaviour consistent with the new wrapper code.
Now that CPI v1 is no longer tested, the Dummy/DummyV2 inheritance
hierarchy is unnecessary. Combine them into a single top-level
DummyCpi class with v2 behaviour baked in:
- create_vm returns [vm_cid, {}]
- attach_disk returns the attachment file path
- info always returns {api_version: 2, stemcell_formats: ...}
- initialize no longer accepts an api_version argument
File changes:
- clouds/dummy.rb + clouds/dummy_v2.rb -> dummy_cpi.rb (merged)
- spec/clouds/dummy_spec.rb -> spec/dummy_cpi_spec.rb (merged)
- bin/dummy_cpi: require + class name updated
- sandbox.rb: require + class name updated, api_version arg removed
- Remove duplicate attach_disk delegator from ExternalCpiResponseWrapper (the generic one-liner shadowed the explicit implementation) - Add minimum CPI API version check (>= 2) to check_cpi_api_support - Update cloud_factory.rb to default to API version 2 (not 1) when a CPI does not report a version from info; v1 is no longer supported - Fix all create_vm mock return values to use the v2 two-element tuple [vm_cid, network_settings] in package_compile_stage_spec, update_stage_functional_spec, create_vm_step_spec, and update_stemcell_spec - Stub cloud.info to return api_version: 2 in package_compile_stage_spec and missing_vm_spec so CloudFactory resolves to the correct version - Update cloud_factory_spec to reflect the new default-to-2 behaviour - Update update_stemcell_spec ExternalCpiResponseWrapper.new calls to use api_version 2 - Fix DummyCpi#all_stemcells sort to use string key 'version' instead of symbol :version (YAML loads string-keyed hashes) - Guard DummyCpi#delete_vm against killing PID 0 for malformed vm_cids - Add api_version guard to bin/dummy_cpi that raises for non-v2 requests
aramprice
force-pushed
the
remove-cpi-v1-testing
branch
from
8a63e7f to
781e6d4
Compare
aramprice
commented
Jun 27, 2026
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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.
Inline comments:
In `@src/spec/integration_support/dummy_cpi.rb`:
- Line 390: The version sort in `latest_stemcell` inside `dummy_cpi.rb` is using
`to_i`, which collapses dotted versions like `1.9` and `1.10` to the same key.
Update the sorting logic to compare the full version string in a way that
preserves dotted semver-style ordering, so `latest_stemcell` no longer depends
on directory order. Use the existing `latest_stemcell`/`sort` path as the place
to adjust the version comparison.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 6c7ed8e7-f246-41d0-9908-574d5f2499b7
📒 Files selected for processing (10)
src/bosh-director/lib/bosh/director/cloud_factory.rbsrc/bosh-director/lib/clouds/external_cpi_response_wrapper.rbsrc/bosh-director/spec/unit/bosh/director/cloud_factory_spec.rbsrc/bosh-director/spec/unit/bosh/director/deployment_plan/stages/package_compile_stage_spec.rbsrc/bosh-director/spec/unit/bosh/director/deployment_plan/stages/update_stage_functional_spec.rbsrc/bosh-director/spec/unit/bosh/director/deployment_plan/steps/create_vm_step_spec.rbsrc/bosh-director/spec/unit/bosh/director/jobs/update_stemcell_spec.rbsrc/bosh-director/spec/unit/bosh/director/problem_handlers/missing_vm_spec.rbsrc/spec/integration_support/bin/dummy_cpisrc/spec/integration_support/dummy_cpi.rb
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (1)
src/bosh-director/spec/unit/bosh/director/deployment_plan/steps/create_vm_step_spec.rb (1)
25-25: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winFinish the v2
create_vmtuple migration in this spec.
create_vm_responsenow uses the 2-item v2 shape, but this file still returns the legacy 3-item form at Line 195 and Lines 568-575. That leaves coverage for the removed wrapper contract in place and can hide regressions inCreateVmStepresponse handling.Suggested fix
- allow(cloud_wrapper).to receive(:create_vm).and_return(['', {}, {}]) + allow(cloud_wrapper).to receive(:create_vm).and_return(['', {}]) ... - [env_id, {}, {}] + [env_id, {}] ... - [env_id, {}, {}] + [env_id, {}]Also applies to: 520-520
🤖 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 `@src/bosh-director/spec/unit/bosh/director/deployment_plan/steps/create_vm_step_spec.rb` at line 25, Finish the v2 create_vm tuple migration in create_vm_step_spec by replacing the remaining legacy 3-item create_vm_response shapes with the 2-item v2 form used elsewhere in the file. Update the affected examples in the CreateVmStep spec so they all return the same [cid, result] tuple shape, removing any wrapper-style response expectations that still reference the old contract.
🤖 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.
Inline comments:
In `@src/spec/integration_support/dummy_cpi.rb`:
- Around line 623-637: The delete-vm wait helpers in DummyCpi spin indefinitely
and need a bounded wait. Update wait_for_delete_vms and
wait_for_unpause_delete_vms to use a timeout when polling the marker files, and
raise or fail with a clear message if the expected file state never changes.
Keep the behavior tied to the existing `@cpi_commands` path markers and the
`@logger` debug calls so the timeout is enforced in both the pause and unpause
flows.
- Around line 358-363: The cleanup helper in kill_process still converts the PID
with agent_pid.to_i, so invalid VM identifiers can become 0 and be signaled
during Sandbox#stop and Sandbox#do_reset. Update kill_process in dummy_cpi to
validate the PID before calling Process.kill, and skip or safely ignore any
non-numeric/zero value so cleanup never signals PID 0 or the current process
group.
In `@src/tasks/spec.rake`:
- Around line 88-94: The unit-test aggregators are scheduling the
integration_support suite twice because `component_dir_names` already yields the
`spec:unit:src` task, which runs the `src/spec` tree including
`spec/integration_support/spec/`. Update the `multitask parallel` and `task
unit` definitions in `spec.rake` to remove the explicit
`spec:unit:integration_support` prerequisite and rely on the component-derived
tasks instead, keeping `spec:unit:release` as the only explicit prerequisite
there.
---
Duplicate comments:
In
`@src/bosh-director/spec/unit/bosh/director/deployment_plan/steps/create_vm_step_spec.rb`:
- Line 25: Finish the v2 create_vm tuple migration in create_vm_step_spec by
replacing the remaining legacy 3-item create_vm_response shapes with the 2-item
v2 form used elsewhere in the file. Update the affected examples in the
CreateVmStep spec so they all return the same [cid, result] tuple shape,
removing any wrapper-style response expectations that still reference the old
contract.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: ec83d7d9-b6ba-4cfe-8582-10d62afe03cf
📒 Files selected for processing (22)
src/bosh-director/lib/bosh/director/cloud_factory.rbsrc/bosh-director/lib/clouds/external_cpi_response_wrapper.rbsrc/bosh-director/spec/unit/bosh/director/az_cloud_factory_spec.rbsrc/bosh-director/spec/unit/bosh/director/cloud_factory_spec.rbsrc/bosh-director/spec/unit/bosh/director/deployment_plan/compilation_instance_pool_spec.rbsrc/bosh-director/spec/unit/bosh/director/deployment_plan/stages/package_compile_stage_spec.rbsrc/bosh-director/spec/unit/bosh/director/deployment_plan/stages/update_stage_functional_spec.rbsrc/bosh-director/spec/unit/bosh/director/deployment_plan/steps/create_vm_step_spec.rbsrc/bosh-director/spec/unit/bosh/director/jobs/update_stemcell_spec.rbsrc/bosh-director/spec/unit/bosh/director/problem_handlers/missing_vm_spec.rbsrc/bosh-director/spec/unit/bosh/director/problem_handlers/unresponsive_agent_spec.rbsrc/bosh-director/spec/unit/clouds/external_cpi_response_wrapper_spec.rbsrc/bosh-director/spec/unit/clouds/external_cpi_spec.rbsrc/spec/integration/cpi_versions/director_cpi_v1_spec.rbsrc/spec/integration/unmanaged_persistent_disks_spec.rbsrc/spec/integration_support/bin/dummy_cpisrc/spec/integration_support/clouds/dummy.rbsrc/spec/integration_support/clouds/dummy_v2.rbsrc/spec/integration_support/dummy_cpi.rbsrc/spec/integration_support/sandbox.rbsrc/spec/integration_support/spec/dummy_cpi_spec.rbsrc/tasks/spec.rake
💤 Files with no reviewable changes (5)
- src/spec/integration/unmanaged_persistent_disks_spec.rb
- src/spec/integration_support/clouds/dummy_v2.rb
- src/bosh-director/spec/unit/clouds/external_cpi_response_wrapper_spec.rb
- src/spec/integration/cpi_versions/director_cpi_v1_spec.rb
- src/spec/integration_support/clouds/dummy.rb
cf-foundation-community-automation
Bot
moved this to Inbox
in Foundational Infrastructure Working Group
github-project-automation
Bot
moved this from Inbox
to Waiting for Changes | Open for Contribution
in Foundational Infrastructure Working Group
- Sort stemcells by dotted semantic version (split on '.', map to_i) so that versions like 1.9 sort before 1.10 correctly - Guard kill_process against PID 0: parse with Integer(..., 10) and return early for non-positive results; rescue ArgumentError for non-numeric filenames under running_vms - Add a 300-second timeout to CommandTransport#wait_for_delete_vms and #wait_for_unpause_delete_vms to prevent CI hangs when a test pauses delete_vms but fails before unpausing
github-project-automation
Bot
moved this from Waiting for Changes | Open for Contribution
to Pending Merge | Prioritized
in Foundational Infrastructure Working Group
With CPI v2, create_vm returns [vm_cid, {}] instead of just vm_cid.
Update the test helpers to correctly extract the vm_cid string:
- cid_and_agent: extract first element of array response so vm_cid
variables hold strings, not [vm_cid, {}] arrays, allowing reference
hash lookups to work for set_vm_metadata, attach_disk, detach_disk,
delete_vm checks
- old_vm_id: same treatment for the 'response' field of the old VM's
create_vm invocation in the update scenario
- create_vm_sequence: replace be(vm_cid) with a satisfy block that
compares the first element of the array response to the expected
vm_cid string, since be/equal? would fail on a new Array object
Member
Author
|
@coderabbitai review |
✅ Action performedReview finished.
|
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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.
Inline comments:
In `@src/bosh-director/lib/bosh/director/cloud_factory.rb`:
- Around line 61-63: The cloud version detection in CloudFactory is treating
missing or failing cloud.info responses as api_version 2, which can incorrectly
route legacy or broken CPIs into v2-only paths. Update the version parsing logic
around the info_response fetch/rescue in CloudFactory to fail closed instead of
defaulting to 2 when api_version is absent or an error occurs. Keep the existing
wrapper guard behavior intact so unknown CPI versions do not get silently
upgraded, and ensure callers like create_vm cannot proceed with a spoofed v2
version.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: db8f7369-e381-4588-a6e3-4e877246e64c
📒 Files selected for processing (24)
src/bosh-director/lib/bosh/director/cloud_factory.rbsrc/bosh-director/lib/clouds/external_cpi_response_wrapper.rbsrc/bosh-director/spec/unit/bosh/director/az_cloud_factory_spec.rbsrc/bosh-director/spec/unit/bosh/director/cloud_factory_spec.rbsrc/bosh-director/spec/unit/bosh/director/deployment_plan/compilation_instance_pool_spec.rbsrc/bosh-director/spec/unit/bosh/director/deployment_plan/stages/package_compile_stage_spec.rbsrc/bosh-director/spec/unit/bosh/director/deployment_plan/stages/update_stage_functional_spec.rbsrc/bosh-director/spec/unit/bosh/director/deployment_plan/steps/create_vm_step_spec.rbsrc/bosh-director/spec/unit/bosh/director/jobs/update_stemcell_spec.rbsrc/bosh-director/spec/unit/bosh/director/problem_handlers/missing_vm_spec.rbsrc/bosh-director/spec/unit/bosh/director/problem_handlers/unresponsive_agent_spec.rbsrc/bosh-director/spec/unit/clouds/external_cpi_response_wrapper_spec.rbsrc/bosh-director/spec/unit/clouds/external_cpi_spec.rbsrc/spec/integration/cpi_and_agent_spec.rbsrc/spec/integration/cpi_versions/director_cpi_v1_spec.rbsrc/spec/integration/unmanaged_persistent_disks_spec.rbsrc/spec/integration_support/bin/dummy_cpisrc/spec/integration_support/clouds/dummy.rbsrc/spec/integration_support/clouds/dummy_v2.rbsrc/spec/integration_support/dummy_cpi.rbsrc/spec/integration_support/invocations_helper.rbsrc/spec/integration_support/sandbox.rbsrc/spec/integration_support/spec/dummy_cpi_spec.rbsrc/tasks/spec.rake
💤 Files with no reviewable changes (5)
- src/spec/integration/unmanaged_persistent_disks_spec.rb
- src/spec/integration/cpi_versions/director_cpi_v1_spec.rb
- src/bosh-director/spec/unit/clouds/external_cpi_response_wrapper_spec.rb
- src/spec/integration_support/clouds/dummy_v2.rb
- src/spec/integration_support/clouds/dummy.rb
github-project-automation
Bot
moved this from Pending Merge | Prioritized
to Waiting for Changes | Open for Contribution
in Foundational Infrastructure Working Group
Two fixes for v2 CPI compatibility:
- cpi_and_agent_spec: replace satisfy{} block with eq([vm_cid, {}]) in
create_vm_sequence. The satisfy matcher wraps a Proc which Marshal.dump
(used by DeepCopy in the custom be_sequence_of_calls matcher) cannot
serialise, causing a TypeError. Using eq([vm_cid, {}]) is both
marshalable and precisely documents the v2 create_vm response contract
(DummyCpi always returns [vm_cid, {}]).
- vm_delete_spec: unpack the v2 create_vm response tuple with
`id, _ = cpi.create_vm(...)` so that the returned vm_cid is a String,
not the full [vm_cid, {}] array. Passing the array to has_vm() fails
schema validation which expects a String for vm_cid.
…t shapes
cloud_factory.rb: fail with NotSupported if cloud.info raises or its response
does not include api_version. Previously, both conditions defaulted silently
to version 2; a legacy v1 CPI that never implemented info would then be treated
as v2, which corrupts the create_vm response flow. Now those CPIs get an
explicit, actionable error.
Spec updates that follow from the behaviour change:
- cloud_factory_spec: update 'CPIs that do not report a version' to expect
NotSupported; fix all info stubs that returned {} to return {'api_version'=>2}
- az_cloud_factory_spec: same {} → {'api_version'=>2} fix in before blocks
- update_stage_functional_spec: add ExternalCpi stub so cloud.info can return
a valid api_version instead of relying on exception swallowing
- update_stemcell_spec: 'cpi does not have stemcell formats' now uses a v2 info
response; 'cpi does not support stemcell formats' renamed to 'cpi does not
implement info' and now expects NotSupported to be raised
- compilation_instance_pool_spec, unresponsive_agent_spec: fix info stub
return value
Remaining v2 shape fixes:
- create_vm_step_spec: replace lingering 3-item [cid, {}, {}] with 2-item
[cid, {}] in the 'sets vm on given report' and deep-copy examples
Member
Author
|
@coderabbitai review |
✅ Action performedReview finished.
|
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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.
Inline comments:
In
`@src/bosh-director/spec/unit/bosh/director/deployment_plan/steps/create_vm_step_spec.rb`:
- Line 195: The `CreateVmStep` specs are stubbing `cloud_wrapper.create_vm` with
empty or fake CID values, so they no longer validate the persisted VM CID
contract. Update the affected `create_vm` stubs in `create_vm_step_spec.rb` to
return a real non-empty VM CID string instead of `''` or `env_id`, and keep the
existing return shape so the examples still exercise `CreateVmStep`’s use of
`create_vm_obj[0]`.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 23ed0dcc-fc25-42d0-931a-90efb38abb3f
📒 Files selected for processing (10)
src/bosh-director/lib/bosh/director/cloud_factory.rbsrc/bosh-director/spec/unit/bosh/director/az_cloud_factory_spec.rbsrc/bosh-director/spec/unit/bosh/director/cloud_factory_spec.rbsrc/bosh-director/spec/unit/bosh/director/deployment_plan/compilation_instance_pool_spec.rbsrc/bosh-director/spec/unit/bosh/director/deployment_plan/stages/update_stage_functional_spec.rbsrc/bosh-director/spec/unit/bosh/director/deployment_plan/steps/create_vm_step_spec.rbsrc/bosh-director/spec/unit/bosh/director/jobs/update_stemcell_spec.rbsrc/bosh-director/spec/unit/bosh/director/problem_handlers/unresponsive_agent_spec.rbsrc/spec/integration/cpi_and_agent_spec.rbsrc/spec/integration/vm_delete_spec.rb
All cloud.info stubs that only returned stemcell_formats were missing api_version, which now causes cloud_factory.rb to raise NotSupported when fetching the api_version key from the response. Add 'api_version' => 2 to every info stub in update_stemcell_spec so the tests exercise a realistic v2 CPI response and the cloud_factory fail-closed guard is satisfied.
In v2 CPI, attach_disk returns a disk hint that must be passed to the agent via add_persistent_disk before set_disk_metadata is called. Update attach_disk_sequence to reflect this ordering.
- Use non-empty VM CID strings in create_vm_step_spec stubs instead of empty string or object_id values - Add guard in DummyCpi#delete_vm to prevent signaling PID 0 which would kill the entire process group
github-project-automation
Bot
moved this from Waiting for Changes | Open for Contribution
to Pending Merge | Prioritized
in Foundational Infrastructure Working Group
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
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.
CPI v1 has been deprecated for a long time, we should stop testing it.
=> https://bosh.ci.cloudfoundry.org/builds/365415162 ⏳