Add create, attach, and list endpoints to dynamic disks API#2743
Draft
Alphasite wants to merge 1 commit into
Draft
Add create, attach, and list endpoints to dynamic disks API#2743Alphasite wants to merge 1 commit into
Alphasite wants to merge 1 commit into
Conversation
Adds three new Director API endpoints to complement the existing provide/detach/delete endpoints: - GET /dynamic_disks list all dynamic disks (bosh.dynamic-disks.list) - POST /dynamic_disks create disk without attaching (bosh.dynamic-disks.create) - POST /dynamic_disks/:name/attach attach existing disk to a VM (bosh.dynamic-disks.attach) Implements CreateDynamicDisk and AttachDynamicDisk background jobs, both on the :dynamic_disks worker queue. All operations are idempotent. 70 unit specs, all passing. [TNZ-109499] Co-authored-by: Cursor <cursoragent@cursor.com>
WalkthroughTwo new background job classes are introduced under Suggested reviewers
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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 |
Contributor
There was a problem hiding this comment.
Pull request overview
This PR extends the BOSH Director dynamic disks API with endpoints to list disks, create a disk (without attaching), and attach an existing disk to a VM, to support the next phase of CLI commands.
Changes:
- Add
GET /dynamic_disksto list dynamic disks (new scope:bosh.dynamic-disks.list). - Add
POST /dynamic_disksto create a dynamic disk in the IaaS without attaching it. - Add
POST /dynamic_disks/:name/attachto attach an existing named disk to a VM via a background job.
Reviewed changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| src/bosh-director/lib/bosh/director/api/controllers/dynamic_disks_controller.rb | Adds list/create/attach endpoints and enqueues new background jobs |
| src/bosh-director/lib/bosh/director/jobs/dynamic_disks/create_dynamic_disk.rb | New job to create a dynamic disk without attaching |
| src/bosh-director/lib/bosh/director/jobs/dynamic_disks/attach_dynamic_disk.rb | New job to attach an existing dynamic disk to an instance’s active VM |
| src/bosh-director/lib/bosh/director.rb | Requires the newly added job classes |
| src/bosh-director/spec/support/test_identity_provider.rb | Adds a scoped test user for the list endpoint |
| src/bosh-director/spec/unit/bosh/director/api/controllers/dynamic_disks_controller_spec.rb | Adds controller coverage for the new endpoints and scope enforcement |
| src/bosh-director/spec/unit/bosh/director/jobs/dynamic_disks/create_dynamic_disk_spec.rb | Adds unit tests for the create job |
| src/bosh-director/spec/unit/bosh/director/jobs/dynamic_disks/attach_dynamic_disk_spec.rb | Adds unit tests for the attach job |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| include ValidationHelper | ||
|
|
||
| get '/', scope: :list_dynamic_disks do | ||
| disks = Models::DynamicDisk.all.map do |disk| |
Comment on lines
+27
to
+33
| cloud_properties = find_disk_cloud_properties(instance, @disk_pool_name) | ||
| cloud = Bosh::Director::CloudFactory.create.get(vm.cpi) | ||
|
|
||
| disk_model = Models::DynamicDisk.find(name: @disk_name) | ||
| if disk_model.nil? | ||
| cloud_properties['name'] = @disk_name | ||
| disk_cid = cloud.create_disk(@disk_size, cloud_properties, vm.cid) |
![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 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/api/controllers/dynamic_disks_controller.rb`:
- Around line 8-21: The get '/' route handler in the DynamicDisksController has
an N+1 query issue because it retrieves all dynamic disks with
Models::DynamicDisk.all and then accesses the associated objects
(disk.deployment and disk.vm.instance) within the map block, causing separate
database queries for each disk. Fix this by using Sequel's eager loading method
on the Models::DynamicDisk query to pre-load the deployment and vm associations
along with the nested instance association before performing the map operation.
In
`@src/bosh-director/lib/bosh/director/jobs/dynamic_disks/create_dynamic_disk.rb`:
- Around line 30-42: The code directly mutates the cloud_properties hash by
assigning cloud_properties['name'] = `@disk_name` on line 32, which could cause
unintended side effects if this hash is shared or cached. Instead of modifying
the original hash in place, create a new hash by merging the cloud_properties
with a new hash containing the name key, then pass this merged hash to the
cloud.create_disk call. This preserves the original cloud_properties hash and
prevents potential side effects from mutations.
In
`@src/bosh-director/spec/unit/bosh/director/api/controllers/dynamic_disks_controller_spec.rb`:
- Around line 337-356: Expand the validation test coverage for the POST '/'
endpoint to match the comprehensiveness of the existing POST '/provide' endpoint
tests. In addition to the current test contexts for disk_name being nil and
disk_size being 0, add more test cases covering nil values for other fields,
empty string validations, and boundary value tests for disk_size and other
numeric properties. Reference the validation test patterns used in the POST
'/provide' endpoint (lines 84-168) to ensure consistency in what scenarios are
being tested, while leveraging the same safe_property validation helper that
both endpoints use.
🪄 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: fb65caa2-5ab2-45be-a214-c92ffe8705bb
📒 Files selected for processing (8)
src/bosh-director/lib/bosh/director.rbsrc/bosh-director/lib/bosh/director/api/controllers/dynamic_disks_controller.rbsrc/bosh-director/lib/bosh/director/jobs/dynamic_disks/attach_dynamic_disk.rbsrc/bosh-director/lib/bosh/director/jobs/dynamic_disks/create_dynamic_disk.rbsrc/bosh-director/spec/support/test_identity_provider.rbsrc/bosh-director/spec/unit/bosh/director/api/controllers/dynamic_disks_controller_spec.rbsrc/bosh-director/spec/unit/bosh/director/jobs/dynamic_disks/attach_dynamic_disk_spec.rbsrc/bosh-director/spec/unit/bosh/director/jobs/dynamic_disks/create_dynamic_disk_spec.rb
Comment on lines
+8
to
+21
| get '/', scope: :list_dynamic_disks do | ||
| disks = Models::DynamicDisk.all.map do |disk| | ||
| { | ||
| name: disk.name, | ||
| disk_cid: disk.disk_cid, | ||
| deployment: disk.deployment&.name, | ||
| instance: disk.vm&.instance&.name, | ||
| availability_zone: disk.availability_zone, | ||
| size: disk.size, | ||
| cpi: disk.cpi, | ||
| } | ||
| end | ||
| json_encode(disks) | ||
| end |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial | ⚡ Quick win
Consider eager loading associations to prevent N+1 queries.
The current implementation accesses disk.deployment and disk.vm.instance inside the map block, which may trigger separate queries for each disk. With many dynamic disks, this could degrade performance.
Consider using Sequel's eager loading:
⚡ Suggested optimization
- get '/', scope: :list_dynamic_disks do
- disks = Models::DynamicDisk.all.map do |disk|
+ get '/', scope: :list_dynamic_disks do
+ disks = Models::DynamicDisk.eager(:deployment, vm: :instance).all.map do |disk|
{🤖 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/lib/bosh/director/api/controllers/dynamic_disks_controller.rb`
around lines 8 - 21, The get '/' route handler in the DynamicDisksController has
an N+1 query issue because it retrieves all dynamic disks with
Models::DynamicDisk.all and then accesses the associated objects
(disk.deployment and disk.vm.instance) within the map block, causing separate
database queries for each disk. Fix this by using Sequel's eager loading method
on the Models::DynamicDisk query to pre-load the deployment and vm associations
along with the nested instance association before performing the map operation.
Comment on lines
+30
to
+42
| disk_model = Models::DynamicDisk.find(name: @disk_name) | ||
| if disk_model.nil? | ||
| cloud_properties['name'] = @disk_name | ||
| disk_cid = cloud.create_disk(@disk_size, cloud_properties, vm.cid) | ||
| disk_model = Models::DynamicDisk.create( | ||
| name: @disk_name, | ||
| disk_cid: disk_cid, | ||
| deployment_id: instance.deployment.id, | ||
| size: @disk_size, | ||
| disk_pool_name: @disk_pool_name, | ||
| cpi: vm.cpi, | ||
| ) | ||
| end |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial | 💤 Low value
Consider avoiding mutation of cloud_properties hash.
Line 32 modifies the cloud_properties hash returned by find_disk_cloud_properties. If that helper returns a shared or cached hash, this mutation could have unintended side effects.
♻️ Defensive alternative using merge
- cloud_properties = find_disk_cloud_properties(instance, `@disk_pool_name`)
- cloud = Bosh::Director::CloudFactory.create.get(vm.cpi)
-
- disk_model = Models::DynamicDisk.find(name: `@disk_name`)
- if disk_model.nil?
- cloud_properties['name'] = `@disk_name`
- disk_cid = cloud.create_disk(`@disk_size`, cloud_properties, vm.cid)
+ cloud_properties = find_disk_cloud_properties(instance, `@disk_pool_name`)
+ cloud = Bosh::Director::CloudFactory.create.get(vm.cpi)
+
+ disk_model = Models::DynamicDisk.find(name: `@disk_name`)
+ if disk_model.nil?
+ disk_cid = cloud.create_disk(`@disk_size`, cloud_properties.merge('name' => `@disk_name`), vm.cid)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| disk_model = Models::DynamicDisk.find(name: @disk_name) | |
| if disk_model.nil? | |
| cloud_properties['name'] = @disk_name | |
| disk_cid = cloud.create_disk(@disk_size, cloud_properties, vm.cid) | |
| disk_model = Models::DynamicDisk.create( | |
| name: @disk_name, | |
| disk_cid: disk_cid, | |
| deployment_id: instance.deployment.id, | |
| size: @disk_size, | |
| disk_pool_name: @disk_pool_name, | |
| cpi: vm.cpi, | |
| ) | |
| end | |
| disk_model = Models::DynamicDisk.find(name: `@disk_name`) | |
| if disk_model.nil? | |
| disk_cid = cloud.create_disk(`@disk_size`, cloud_properties.merge('name' => `@disk_name`), vm.cid) | |
| disk_model = Models::DynamicDisk.create( | |
| name: `@disk_name`, | |
| disk_cid: disk_cid, | |
| deployment_id: instance.deployment.id, | |
| size: `@disk_size`, | |
| disk_pool_name: `@disk_pool_name`, | |
| cpi: vm.cpi, | |
| ) | |
| end |
🤖 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/lib/bosh/director/jobs/dynamic_disks/create_dynamic_disk.rb`
around lines 30 - 42, The code directly mutates the cloud_properties hash by
assigning cloud_properties['name'] = `@disk_name` on line 32, which could cause
unintended side effects if this hash is shared or cached. Instead of modifying
the original hash in place, create a new hash by merging the cloud_properties
with a new hash containing the name key, then pass this merged hash to the
cloud.create_disk call. This preserves the original cloud_properties hash and
prevents potential side effects from mutations.
Comment on lines
+337
to
+356
| context 'when disk_name is nil' do | ||
| let(:disk_name) { nil } | ||
|
|
||
| it 'raises an error' do | ||
| post '/', content, { 'CONTENT_TYPE' => 'application/json' } | ||
| expect(last_response.status).to eq(400) | ||
| expect(JSON.parse(last_response.body)['description']).to include("disk_name") | ||
| end | ||
| end | ||
|
|
||
| context 'when disk_size is 0' do | ||
| let(:disk_size) { 0 } | ||
|
|
||
| it 'raises an error' do | ||
| post '/', content, { 'CONTENT_TYPE' => 'application/json' } | ||
| expect(last_response.status).to eq(400) | ||
| expect(JSON.parse(last_response.body)['description']).to include("disk_size") | ||
| end | ||
| end | ||
| end |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial | 💤 Low value
Consider expanding validation test coverage for consistency.
The POST '/' endpoint validation tests only cover disk_name=nil and disk_size=0, whereas the existing POST '/provide' endpoint (lines 84-168) includes more comprehensive validation tests (nil values, empty strings, boundary values for multiple fields).
While both endpoints use the same safe_property validation helper (so the underlying validation is covered), expanding the test cases here would improve consistency and make the test suite more robust.
🤖 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/api/controllers/dynamic_disks_controller_spec.rb`
around lines 337 - 356, Expand the validation test coverage for the POST '/'
endpoint to match the comprehensiveness of the existing POST '/provide' endpoint
tests. In addition to the current test contexts for disk_name being nil and
disk_size being 0, add more test cases covering nil values for other fields,
empty string validations, and boundary value tests for disk_size and other
numeric properties. Reference the validation test patterns used in the POST
'/provide' endpoint (lines 84-168) to ensure consistency in what scenarios are
being tested, while leveraging the same safe_property validation helper that
both endpoints use.
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
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.
Summary
Extends the BOSH Director's dynamic disk API with three new endpoints to support Phase 2 CLI commands (TNZ-109499):
GET /dynamic_disks— list all dynamic disks (bosh.adminorbosh.dynamic-disks.listscope)POST /dynamic_disks— create a disk in the IaaS without attaching it (bosh.dynamic-disks.createscope)POST /dynamic_disks/:name/attach— attach an existing named disk to a VM (bosh.dynamic-disks.attachscope)Changes
New files
src/bosh-director/lib/bosh/director/jobs/dynamic_disks/create_dynamic_disk.rb— background job that creates a disk via CPI without attachingsrc/bosh-director/lib/bosh/director/jobs/dynamic_disks/attach_dynamic_disk.rb— background job that attaches a named disk to a VM (idempotent)src/bosh-director/spec/unit/bosh/director/jobs/dynamic_disks/create_dynamic_disk_spec.rbsrc/bosh-director/spec/unit/bosh/director/jobs/dynamic_disks/attach_dynamic_disk_spec.rbModified files
src/bosh-director/lib/bosh/director/api/controllers/dynamic_disks_controller.rb— added 3 new routessrc/bosh-director/lib/bosh/director.rb— required new job classessrc/bosh-director/spec/support/test_identity_provider.rb— addeddynamic-disks-listeranddynamic-disks-attachertest userssrc/bosh-director/spec/unit/bosh/director/api/controllers/dynamic_disks_controller_spec.rb— full coverage for all 6 endpointsTest plan
bundle exec rspec spec/unit/bosh/director/api/controllers/dynamic_disks_controller_spec.rb spec/unit/bosh/director/jobs/dynamic_disks/— 70 examples, 0 failuresRelated: TNZ-109499, TNZ-99509