Conversation
cf-foundation-community-automation
bot
moved this to Inbox
in Foundational Infrastructure Working Group
github-project-automation
bot
moved this from Inbox
to Pending Merge | Prioritized
in Foundational Infrastructure Working Group
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughAdds a new DiskManager flow to update detached persistent disks via CPI during VM recreation, integrates it into RecreateHandler and UpdateProcedure, consolidates DiskManager model updates and CID handling, and adds comprehensive unit tests for the new behaviors and edge cases. Changes
Sequence DiagramsequenceDiagram
participant UP as UpdateProcedure
participant RH as RecreateHandler
participant DM as DiskManager
participant CPI as Cloud (CPI)
participant DB as Database
UP->>RH: new(..., disk_manager)
UP->>RH: perform()
RH->>RH: detach_disks()
alt disk_manager present
RH->>DM: update_detached_disks(instance_plan)
alt enable_cpi_update_disk && persistent_disk_changed?
DM->>DB: load detached persistent disk models
DM->>CPI: update_disk(disk_cid, size, cloud_properties)
CPI-->>DM: disk_cid (new / same / nil)
alt disk_cid changed
DM->>DB: update model(size, cloud_properties, disk_cid)
else
DM->>DB: update model(size, cloud_properties)
end
else
DM->>DM: no-op (flag/unchanged)
end
else
RH->>RH: skip disk CPI updates
end
RH->>RH: delete_vm() / create_vm()
RH->>RH: attach_disks()
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 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 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: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/bosh-director/lib/bosh/director/disk_manager.rb`:
- Around line 85-90: The update path is passing new_disk.cloud_properties
directly to the CPI (see update_disk call), causing unresolved ((...))
placeholders; fix update_disk_cpi to interpolate new_disk.cloud_properties the
same way the create path does before calling cloud.update_disk. Locate
update_disk_cpi (and the update_disk call that uses new_disk.cloud_properties),
apply the same interpolation helper used by create_disk to produce resolved
cloud_properties, and pass the resolved properties to cloud.update_disk and to
the updates hash.
- Around line 309-312: The stale-state guard in DiskManager that returns early
when old_disk_model.size == new_disk.size && old_disk_model.cloud_properties ==
new_disk.cloud_properties skips CPI updates when only variable-interpolated
values changed; change the check to use the variable-set-aware comparison
already produced for this flow (use changed_disk_pairs or the resolved
cloud_properties for old vs new) rather than comparing raw
new_disk.cloud_properties, so that interpolation-only changes trigger the CPI
update for disk_cid (referencing old_disk_model, new_disk, changed_disk_pairs,
and disk_cid in your patch).
- Around line 327-335: The attach happens before persisting a new disk CID,
causing the VM to be reattached with a stale CID; move or apply the updates
(i.e., persist new_disk_cid and any other changes) to old_disk_model before
calling attach_disk, or explicitly set old_disk_model.disk_cid to new_disk_cid
prior to attach_disk(old_disk_model, instance_plan.tags); update the logic
around new_disk_cid, attach_disk, and old_disk_model.update to ensure the model
reflects the returned CID at attach time, and extend the new-CID spec to assert
the attach step uses the returned CID.
🪄 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: 04b9488c-58f8-48c7-997d-1e59f987c776
📒 Files selected for processing (5)
src/bosh-director/lib/bosh/director/disk_manager.rbsrc/bosh-director/lib/bosh/director/instance_updater/recreate_handler.rbsrc/bosh-director/lib/bosh/director/instance_updater/update_procedure.rbsrc/bosh-director/spec/unit/bosh/director/disk_manager_spec.rbsrc/bosh-director/spec/unit/bosh/director/instance_updater/update_procedure_spec.rb
github-project-automation
bot
moved this from Pending Merge | Prioritized
to Waiting for Changes | Open for Contribution
in Foundational Infrastructure Working Group
Both update_detached_disks and update_disk_cpi were passing raw cloud_properties to the CPI, which could contain unresolved variable placeholders like ((...)). The create_disk path already resolves these via variables_interpolator.interpolate_with_versioning; do the same for the update paths.
… used in attach_disk
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/bosh-director/spec/unit/bosh/director/disk_manager_spec.rb`:
- Around line 906-926: Add a unit test for the defensive branch where a disk has
no active VM: in the spec for disk_manager.update_detached_disks, create a
context "when there is no active VM for the disk", remove the active VM from
instance_model (e.g. call instance_model.active_vm.destroy and reload), then
call disk_manager.update_detached_disks(instance_plan) and assert that
per_spec_logger receives a warn matching /No active VM for disk/,
cloud.update_disk is not called, and the Models::PersistentDisk record (disk_cid
'disk123') still has the original size (e.g. 2048) to ensure the update is
skipped and state unchanged.
🪄 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: cef0edd0-50d9-4cd9-9fe7-38ffcca4d6a1
📒 Files selected for processing (2)
src/bosh-director/lib/bosh/director/disk_manager.rbsrc/bosh-director/spec/unit/bosh/director/disk_manager_spec.rb
github-project-automation
bot
moved this from Waiting for Changes | Open for Contribution
to Pending Merge | Prioritized
in Foundational Infrastructure Working Group
|
@neddp perhaps, I am missing some context here, but as far as I can see we had the same or at least a similar requirement for Azure some time ago and added native disk update support via bosh parameter |
|
Hi @s4heid, This builds on top of your initial implementation, but the existing Azure functionality works for in-place updates only and was not returning anything. In this particular GCP scenario, an in-place is not possible. (Similar discussion was started for Azure as well - cloudfoundry/bosh-azure-cpi-release#699) This PR, along with the GCP CPI changes, do a disk snapshot and then a recreate with a different type. The CPI Please correct me if I'm wrong. |
|
Got it - this makes sense to me. |
4 participants
What is this change about?
This PR adds the ability to update persistent disk properties (size, type, cloud_properties) via the CPI while disks are already in a detached state during VM recreation. This avoids a redundant detach→update→attach cycle when the disk is already detached as part of the recreation flow.
Changes:
New
update_detached_disksmethod inDiskManager: Called byRecreateHandlerafter disks are detached but before the old VM is deleted. Attempts a CPI-levelupdate_diskwhile the disk is already detached. If the CPI doesn't supportupdate_disk, it gracefully falls back - the subsequentupdate_persistent_diskcall handles disk changes via the standard copy path after recreation.Pass
disk_managerintoRecreateHandler: Previously,RecreateHandleronly handled VM lifecycle (detach disks → delete VM → create VM → attach disks) using stateless Step classes. It had no access toDiskManagerbecause it didn't need it. Now it receivesdisk_manageras an optional keyword argument to enable the new detached-disk update path.Handle new disk CID from
update_diskreturn value: The CPI'supdate_diskmay return a new disk CID (e.g., GCP replaces the disk via snapshot when changing disk types). Bothupdate_detached_disksandupdate_disk_cpinow capture and persist the returned CID. Previously the return value was discarded.Staleness guard in
update_disk_cpi: Afterupdate_detached_diskssucceeds and updates the DB model, the subsequentupdate_persistent_diskcall would redundantly invoke the CPI again (especially whenrecreate_persistent_disksis set). A new guard skips the CPI call when the disk model already matches the desired size and cloud_properties.Consolidate DB updates: Merged separate
update(size:)andupdate(cloud_properties:)calls into a singleupdate(updates)call, including the optionaldisk_cidwhen it changes. Reduces DB roundtrips from 2-3 to 1.Nil guard for
active_vm: Added a defensive check inupdate_detached_disksin case the instance has no active VM during recreation.Please provide contextual information.
This is part of the GCP migration from N2 to N4 machine types we are working on. Since the disk_type must be changed, and in-place change is not supported, a CPI change was also added:
cloudfoundry/bosh-google-cpi-release#382
What tests have you run against this PR?
How should this change be described in bosh release notes?
When
enable_cpi_update_diskis enabled, the Director now updates persistent disk properties (size, type) via the CPI during VM recreation while the disk is already detached, avoiding a redundant detach-update-attach cycle. The CPI'supdate_diskmay return a new disk CID (e.g., when the disk is replaced), which is now correctly persisted. If the CPI does not supportupdate_disk, the Director falls back to the standard disk copy path transparently.Does this PR introduce a breaking change?
No. All changes are behind the existing
enable_cpi_update_diskfeature flag (default:false). TheRecreateHandlerconstructor change uses a keyword argument with a default ofnil, so existing callers are unaffected. When the feature is disabled, behavior is identical to before this PR.Claude was used for the code.