From 7e2883d688911204fcbc721d0164c5aa1fea071e Mon Sep 17 00:00:00 2001 From: Nishad Mathur Date: Wed, 17 Jun 2026 13:41:10 -0700 Subject: [PATCH 1/5] Add create, attach, and list endpoints to dynamic disks API 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 --- src/bosh-director/lib/bosh/director.rb | 2 + .../controllers/dynamic_disks_controller.rb | 50 +++++ .../jobs/dynamic_disks/attach_dynamic_disk.rb | 45 ++++ .../jobs/dynamic_disks/create_dynamic_disk.rb | 58 +++++ .../spec/support/test_identity_provider.rb | 1 + .../dynamic_disks_controller_spec.rb | 207 ++++++++++++++++++ .../dynamic_disks/attach_dynamic_disk_spec.rb | 138 ++++++++++++ .../dynamic_disks/create_dynamic_disk_spec.rb | 146 ++++++++++++ 8 files changed, 647 insertions(+) create mode 100644 src/bosh-director/lib/bosh/director/jobs/dynamic_disks/attach_dynamic_disk.rb create mode 100644 src/bosh-director/lib/bosh/director/jobs/dynamic_disks/create_dynamic_disk.rb create mode 100644 src/bosh-director/spec/unit/bosh/director/jobs/dynamic_disks/attach_dynamic_disk_spec.rb create mode 100644 src/bosh-director/spec/unit/bosh/director/jobs/dynamic_disks/create_dynamic_disk_spec.rb diff --git a/src/bosh-director/lib/bosh/director.rb b/src/bosh-director/lib/bosh/director.rb index 9a1cbdb8681..5b1e44bc1ca 100644 --- a/src/bosh-director/lib/bosh/director.rb +++ b/src/bosh-director/lib/bosh/director.rb @@ -237,6 +237,8 @@ module Director require 'bosh/director/jobs/helpers' require 'bosh/director/jobs/db_job' require 'bosh/director/jobs/orphan_disk' +require 'bosh/director/jobs/dynamic_disks/create_dynamic_disk' +require 'bosh/director/jobs/dynamic_disks/attach_dynamic_disk' require 'bosh/director/jobs/dynamic_disks/provide_dynamic_disk' require 'bosh/director/jobs/dynamic_disks/detach_dynamic_disk' require 'bosh/director/jobs/dynamic_disks/delete_dynamic_disk' diff --git a/src/bosh-director/lib/bosh/director/api/controllers/dynamic_disks_controller.rb b/src/bosh-director/lib/bosh/director/api/controllers/dynamic_disks_controller.rb index 2f9288af9ec..1353d892fba 100644 --- a/src/bosh-director/lib/bosh/director/api/controllers/dynamic_disks_controller.rb +++ b/src/bosh-director/lib/bosh/director/api/controllers/dynamic_disks_controller.rb @@ -5,6 +5,56 @@ module Api::Controllers class DynamicDisksController < BaseController include ValidationHelper + 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 + + post '/', scope: :create_dynamic_disks, consumes: :json do + request_hash = JSON.parse(request.body.read) + + instance_id = safe_property(request_hash, 'instance_id', class: String, min_length: 1) + disk_name = safe_property(request_hash, 'disk_name', class: String, min_length: 1) + disk_pool_name = safe_property(request_hash, 'disk_pool_name', class: String, min_length: 1) + disk_size = safe_property(request_hash, 'disk_size', class: Integer, min: 1) + metadata = safe_property(request_hash, 'metadata', class: Hash, optional: true) + + task = JobQueue.new.enqueue( + current_user, + Jobs::DynamicDisks::CreateDynamicDisk, + 'create dynamic disk', + [instance_id, disk_name, disk_pool_name, disk_size, metadata], + ) + + redirect "/tasks/#{task.id}" + end + + post '/:disk_name/attach', scope: :attach_dynamic_disks, consumes: :json do + disk_name = safe_property(params, 'disk_name', class: String, min_length: 1) + + request_hash = JSON.parse(request.body.read) + instance_id = safe_property(request_hash, 'instance_id', class: String, min_length: 1) + + task = JobQueue.new.enqueue( + current_user, + Jobs::DynamicDisks::AttachDynamicDisk, + 'attach dynamic disk', + [disk_name, instance_id], + ) + + redirect "/tasks/#{task.id}" + end + post '/provide', scope: :provide_dynamic_disks, consumes: :json do request_hash = JSON.parse(request.body.read) diff --git a/src/bosh-director/lib/bosh/director/jobs/dynamic_disks/attach_dynamic_disk.rb b/src/bosh-director/lib/bosh/director/jobs/dynamic_disks/attach_dynamic_disk.rb new file mode 100644 index 00000000000..52c8dcc6fba --- /dev/null +++ b/src/bosh-director/lib/bosh/director/jobs/dynamic_disks/attach_dynamic_disk.rb @@ -0,0 +1,45 @@ +module Bosh::Director + module Jobs::DynamicDisks + class AttachDynamicDisk < Jobs::BaseJob + include Jobs::Helpers::DynamicDiskHelpers + include LockHelper + + @queue = :dynamic_disks + + def self.job_type + :attach_dynamic_disk + end + + def initialize(disk_name, instance_id) + @disk_name = disk_name + @instance_id = instance_id + end + + def perform + disk_model = Models::DynamicDisk.find(name: @disk_name) + raise "disk `#{@disk_name}` not found" if disk_model.nil? + + instance = Models::Instance.find(uuid: @instance_id) + raise "instance `#{@instance_id}` not found" if instance.nil? + + vm = instance.active_vm + raise "no active vm found for instance `#{@instance_id}`" if vm.nil? + + unless disk_model.vm.nil? + return "disk `#{@disk_name}` is already attached to vm `#{disk_model.vm.cid}`" if disk_model.vm.id == vm.id + + raise "disk `#{@disk_name}` is already attached to a different vm `#{disk_model.vm.cid}`" + end + + cloud = Bosh::Director::CloudFactory.create.get(disk_model.cpi, vm.stemcell_api_version) + disk_hint = with_vm_lock(vm.cid, timeout: VM_LOCK_TIMEOUT) { cloud.attach_disk(vm.cid, disk_model.disk_cid) } + disk_model.update(vm_id: vm.id, availability_zone: vm.instance.availability_zone, disk_hint: disk_hint) + + agent_client = AgentClient.with_agent_id(vm.agent_id, instance.name) + agent_client.add_dynamic_disk(disk_model.disk_cid, disk_model.disk_hint) + + "attached disk `#{@disk_name}` to vm `#{vm.cid}` in deployment `#{instance.deployment.name}`" + end + end + end +end diff --git a/src/bosh-director/lib/bosh/director/jobs/dynamic_disks/create_dynamic_disk.rb b/src/bosh-director/lib/bosh/director/jobs/dynamic_disks/create_dynamic_disk.rb new file mode 100644 index 00000000000..1c614d33f39 --- /dev/null +++ b/src/bosh-director/lib/bosh/director/jobs/dynamic_disks/create_dynamic_disk.rb @@ -0,0 +1,58 @@ +module Bosh::Director + module Jobs::DynamicDisks + class CreateDynamicDisk < Jobs::BaseJob + include Jobs::Helpers::DynamicDiskHelpers + + @queue = :dynamic_disks + + def self.job_type + :create_dynamic_disk + end + + def initialize(instance_id, disk_name, disk_pool_name, disk_size, metadata) + @instance_id = instance_id + @disk_name = disk_name + @disk_pool_name = disk_pool_name + @disk_size = disk_size + @metadata = metadata + end + + def perform + instance = Models::Instance.find(uuid: @instance_id) + raise "instance `#{@instance_id}` not found" if instance.nil? + + vm = instance.active_vm + raise "no active vm found for instance `#{@instance_id}`" if vm.nil? + + 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) + 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 + + if !@metadata.nil? && disk_model.metadata != @metadata + MetadataUpdater.build.update_dynamic_disk_metadata(cloud, disk_model, @metadata) + disk_model.update(metadata: @metadata) + end + + disk_info = { disk_cid: disk_model.disk_cid } + + task_result.write(JSON.generate(disk_info)) + task_result.write("\n") + + "created disk `#{@disk_name}` in deployment `#{instance.deployment.name}`" + end + end + end +end diff --git a/src/bosh-director/spec/support/test_identity_provider.rb b/src/bosh-director/spec/support/test_identity_provider.rb index b4681e7c6e8..4d467fc8174 100644 --- a/src/bosh-director/spec/support/test_identity_provider.rb +++ b/src/bosh-director/spec/support/test_identity_provider.rb @@ -44,6 +44,7 @@ def user_scopes 'dynamic-disks-attacher' => ['bosh.dynamic-disks.attach'], 'dynamic-disks-detacher' => ['bosh.dynamic-disks.detach'], 'dynamic-disks-deleter' => ['bosh.dynamic-disks.delete'], + 'dynamic-disks-lister' => ['bosh.dynamic-disks.list'], 'outsider' => ['uaa.admin'], } end diff --git a/src/bosh-director/spec/unit/bosh/director/api/controllers/dynamic_disks_controller_spec.rb b/src/bosh-director/spec/unit/bosh/director/api/controllers/dynamic_disks_controller_spec.rb index a33f017519d..a0376694126 100644 --- a/src/bosh-director/spec/unit/bosh/director/api/controllers/dynamic_disks_controller_spec.rb +++ b/src/bosh-director/spec/unit/bosh/director/api/controllers/dynamic_disks_controller_spec.rb @@ -213,6 +213,213 @@ module Api end end + describe 'GET', '/' do + context 'when user is reader' do + before { basic_authorize('reader', 'reader') } + + it 'forbids access' do + expect(get('/').status).to eq(401) + end + end + + context 'when user has bosh.dynamic-disks.list scope' do + before { basic_authorize('dynamic-disks-lister', 'dynamic-disks-lister') } + + context 'when there are no dynamic disks' do + it 'returns an empty list' do + get '/' + expect(last_response.status).to eq(200) + expect(JSON.parse(last_response.body)).to eq([]) + end + end + + context 'when dynamic disks exist' do + let!(:deployment) { FactoryBot.create(:models_deployment, name: 'my-deployment') } + let!(:disk1) do + FactoryBot.create(:models_dynamic_disk, + name: 'disk-a', + disk_cid: 'cid-a', + deployment: deployment, + size: 1024, + ) + end + let!(:disk2) do + FactoryBot.create(:models_dynamic_disk, + name: 'disk-b', + disk_cid: 'cid-b', + deployment: deployment, + size: 2048, + ) + end + + it 'returns all dynamic disks' do + get '/' + expect(last_response.status).to eq(200) + body = JSON.parse(last_response.body) + expect(body.size).to eq(2) + expect(body).to include(hash_including('name' => 'disk-a', 'disk_cid' => 'cid-a', 'deployment' => 'my-deployment', 'size' => 1024)) + expect(body).to include(hash_including('name' => 'disk-b', 'disk_cid' => 'cid-b', 'deployment' => 'my-deployment', 'size' => 2048)) + end + end + end + + context 'when user has admin permissions' do + before { authorize 'admin', 'admin' } + + it 'returns a list of dynamic disks' do + get '/' + expect(last_response.status).to eq(200) + expect(JSON.parse(last_response.body)).to eq([]) + end + end + end + + describe 'POST', '/' do + let(:content) do + JSON.generate({ + 'instance_id' => instance_id, + 'disk_pool_name' => disk_pool_name, + 'disk_name' => disk_name, + 'disk_size' => disk_size, + 'metadata' => metadata, + }) + end + + context 'when user is reader' do + before { basic_authorize('reader', 'reader') } + + it 'forbids access' do + expect(post('/', content, { 'CONTENT_TYPE' => 'application/json' }).status).to eq(401) + end + end + + context 'when user has bosh.dynamic-disks.create scope' do + before { basic_authorize('dynamic-disks-creator', 'dynamic-disks-creator') } + + it 'enqueues a CreateDynamicDisk task' do + expect_any_instance_of(Bosh::Director::JobQueue).to receive(:enqueue).with( + 'dynamic-disks-creator', + Jobs::DynamicDisks::CreateDynamicDisk, + 'create dynamic disk', + [instance_id, disk_name, disk_pool_name, disk_size, metadata], + ).and_call_original + + post '/', content, { 'CONTENT_TYPE' => 'application/json' } + + expect_redirect_to_queued_task(last_response) + end + end + + context 'when user has only bosh.dynamic-disks.attach scope (not create)' do + before { basic_authorize('dynamic-disks-attacher', 'dynamic-disks-attacher') } + + it 'forbids access' do + expect(post('/', content, { 'CONTENT_TYPE' => 'application/json' }).status).to eq(401) + end + end + + context 'when user has admin permissions' do + before { authorize 'admin', 'admin' } + + it 'enqueues a CreateDynamicDisk task' do + expect_any_instance_of(Bosh::Director::JobQueue).to receive(:enqueue).with( + 'admin', + Jobs::DynamicDisks::CreateDynamicDisk, + 'create dynamic disk', + [instance_id, disk_name, disk_pool_name, disk_size, metadata], + ).and_call_original + + post '/', content, { 'CONTENT_TYPE' => 'application/json' } + + expect_redirect_to_queued_task(last_response) + end + + 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 + end + + describe 'POST', '/:disk_name/attach' do + let(:content) { JSON.generate({ 'instance_id' => instance_id }) } + + context 'when user is reader' do + before { basic_authorize('reader', 'reader') } + + it 'forbids access' do + expect(post('/disk_name/attach', content, { 'CONTENT_TYPE' => 'application/json' }).status).to eq(401) + end + end + + context 'when user has bosh.dynamic-disks.attach scope' do + before { basic_authorize('dynamic-disks-attacher', 'dynamic-disks-attacher') } + + it 'enqueues an AttachDynamicDisk task' do + expect_any_instance_of(Bosh::Director::JobQueue).to receive(:enqueue).with( + 'dynamic-disks-attacher', + Jobs::DynamicDisks::AttachDynamicDisk, + 'attach dynamic disk', + ['disk_name', instance_id], + ).and_call_original + + post '/disk_name/attach', content, { 'CONTENT_TYPE' => 'application/json' } + + expect_redirect_to_queued_task(last_response) + end + end + + context 'when user has only bosh.dynamic-disks.create scope (not attach)' do + before { basic_authorize('dynamic-disks-creator', 'dynamic-disks-creator') } + + it 'forbids access' do + expect(post('/disk_name/attach', content, { 'CONTENT_TYPE' => 'application/json' }).status).to eq(401) + end + end + + context 'when user has admin permissions' do + before { authorize 'admin', 'admin' } + + it 'enqueues an AttachDynamicDisk task' do + expect_any_instance_of(Bosh::Director::JobQueue).to receive(:enqueue).with( + 'admin', + Jobs::DynamicDisks::AttachDynamicDisk, + 'attach dynamic disk', + ['disk_name', instance_id], + ).and_call_original + + post '/disk_name/attach', content, { 'CONTENT_TYPE' => 'application/json' } + + expect_redirect_to_queued_task(last_response) + end + + context 'when instance_id is missing from the body' do + let(:content) { JSON.generate({}) } + + it 'raises an error' do + post '/disk_name/attach', content, { 'CONTENT_TYPE' => 'application/json' } + expect(last_response.status).to eq(400) + expect(JSON.parse(last_response.body)['description']).to include("instance_id") + end + end + end + end + describe 'DELETE', '/:disk_name' do context 'when user is reader' do before { basic_authorize('reader', 'reader') } diff --git a/src/bosh-director/spec/unit/bosh/director/jobs/dynamic_disks/attach_dynamic_disk_spec.rb b/src/bosh-director/spec/unit/bosh/director/jobs/dynamic_disks/attach_dynamic_disk_spec.rb new file mode 100644 index 00000000000..d1daeb74ef4 --- /dev/null +++ b/src/bosh-director/spec/unit/bosh/director/jobs/dynamic_disks/attach_dynamic_disk_spec.rb @@ -0,0 +1,138 @@ +require 'spec_helper' + +module Bosh::Director + describe Jobs::DynamicDisks::AttachDynamicDisk do + let(:disk_name) { 'fake-disk-name' } + let(:disk_cid) { 'fake-disk-cid' } + let(:disk_hint) { { 'id' => 'fake-disk-id' } } + + let(:agent_client) { instance_double(AgentClient) } + let(:cloud) { instance_double(Bosh::Clouds::ExternalCpi) } + + let(:instance) { FactoryBot.create(:models_instance) } + let!(:vm) { FactoryBot.create(:models_vm, instance: instance, active: true) } + + let(:attach_dynamic_disk_job) { Jobs::DynamicDisks::AttachDynamicDisk.new(disk_name, instance.uuid) } + let(:cloud_factory) { instance_double(CloudFactory, get: cloud) } + + before do + allow(Config).to receive(:name).and_return('fake-director-name') + allow(Config).to receive(:cloud_options).and_return('provider' => { 'path' => '/path/to/default/cpi' }) + allow(Config).to receive(:preferred_cpi_api_version).and_return(2) + allow(CloudFactory).to receive(:create).and_return(cloud_factory) + allow(AgentClient).to receive(:with_agent_id).with(vm.agent_id, instance.name).and_return(agent_client) + end + + describe 'DJ job class expectations' do + let(:job_type) { :attach_dynamic_disk } + let(:queue) { :dynamic_disks } + it_behaves_like 'a DelayedJob job' + end + + describe '#perform' do + context 'when disk exists and is not attached' do + let!(:disk) do + FactoryBot.create( + :models_dynamic_disk, + name: disk_name, + disk_cid: disk_cid, + deployment: instance.deployment, + ) + end + + it 'attaches the disk to the VM and notifies the agent' do + expect(attach_dynamic_disk_job).to receive(:with_vm_lock).with(vm.cid, timeout: Jobs::Helpers::DynamicDiskHelpers::VM_LOCK_TIMEOUT).and_yield + expect(cloud).to receive(:attach_disk).with(vm.cid, disk_cid).and_return(disk_hint) + expect(agent_client).to receive(:add_dynamic_disk).with(disk_cid, disk_hint) + + result = attach_dynamic_disk_job.perform + + expect(result).to eq("attached disk `#{disk_name}` to vm `#{vm.cid}` in deployment `#{instance.deployment.name}`") + + disk.reload + expect(disk.vm).to eq(vm) + expect(disk.availability_zone).to eq(vm.instance.availability_zone) + end + end + + context 'when disk is already attached to the same VM' do + let!(:disk) do + FactoryBot.create( + :models_dynamic_disk, + name: disk_name, + disk_cid: disk_cid, + deployment: instance.deployment, + vm: vm, + ) + end + + it 'returns a success message without re-attaching (idempotent)' do + expect(cloud).not_to receive(:attach_disk) + expect(agent_client).not_to receive(:add_dynamic_disk) + + result = attach_dynamic_disk_job.perform + + expect(result).to eq("disk `#{disk_name}` is already attached to vm `#{vm.cid}`") + end + end + + context 'when disk is already attached to a different VM' do + let(:other_instance) { FactoryBot.create(:models_instance) } + let!(:other_vm) { FactoryBot.create(:models_vm, instance: other_instance, active: true) } + let!(:disk) do + FactoryBot.create( + :models_dynamic_disk, + name: disk_name, + disk_cid: disk_cid, + deployment: instance.deployment, + vm: other_vm, + ) + end + + it 'raises an error' do + expect { attach_dynamic_disk_job.perform }.to raise_error( + "disk `#{disk_name}` is already attached to a different vm `#{other_vm.cid}`", + ) + end + end + + context 'when disk does not exist' do + it 'raises an error' do + expect { attach_dynamic_disk_job.perform }.to raise_error("disk `#{disk_name}` not found") + end + end + + context 'when instance cannot be found' do + let!(:disk) do + FactoryBot.create( + :models_dynamic_disk, + name: disk_name, + disk_cid: disk_cid, + deployment: instance.deployment, + ) + end + let(:attach_dynamic_disk_job) { Jobs::DynamicDisks::AttachDynamicDisk.new(disk_name, 'unknown-instance-id') } + + it 'raises an error' do + expect { attach_dynamic_disk_job.perform }.to raise_error("instance `unknown-instance-id` not found") + end + end + + context 'when there is no active VM for the instance' do + let(:vm) { FactoryBot.create(:models_vm, instance: instance, active: false) } + let!(:disk) do + FactoryBot.create( + :models_dynamic_disk, + name: disk_name, + disk_cid: disk_cid, + deployment: instance.deployment, + ) + end + + it 'raises an error' do + expect { attach_dynamic_disk_job.perform }.to raise_error("no active vm found for instance `#{instance.uuid}`") + end + end + end + end +end diff --git a/src/bosh-director/spec/unit/bosh/director/jobs/dynamic_disks/create_dynamic_disk_spec.rb b/src/bosh-director/spec/unit/bosh/director/jobs/dynamic_disks/create_dynamic_disk_spec.rb new file mode 100644 index 00000000000..1e7be924890 --- /dev/null +++ b/src/bosh-director/spec/unit/bosh/director/jobs/dynamic_disks/create_dynamic_disk_spec.rb @@ -0,0 +1,146 @@ +require 'spec_helper' + +module Bosh::Director + describe Jobs::DynamicDisks::CreateDynamicDisk do + let(:disk_name) { 'fake-disk-name' } + let(:disk_cid) { 'fake-disk-cid' } + let(:disk_pool_name) { 'fake-disk-pool-name' } + let(:disk_cloud_properties) { { 'fake-disk-cloud-property-key' => 'fake-disk-cloud-property-value' } } + let(:disk_size) { 1000 } + let(:metadata) { { 'fake-key' => 'fake-value' } } + + let(:cloud) { instance_double(Bosh::Clouds::ExternalCpi) } + let(:instance) { FactoryBot.create(:models_instance) } + let!(:vm) { FactoryBot.create(:models_vm, instance: instance, active: true) } + + let(:task_result) { TaskDBWriter.new(:result_output, task.id) } + let(:task) { FactoryBot.create(:models_task, id: 42, username: 'user') } + + def parsed_task_result + JSON.parse(Models::Task.first(id: 42).result_output) + end + + let(:create_dynamic_disk_job) { Jobs::DynamicDisks::CreateDynamicDisk.new(instance.uuid, disk_name, disk_pool_name, disk_size, metadata) } + + let!(:cloud_config) do + FactoryBot.create(:models_config_cloud, content: YAML.dump( + SharedSupport::DeploymentManifestHelper.simple_cloud_config.merge( + 'disk_types' => [ + { 'name' => disk_pool_name, + 'disk_size' => 1024, + 'cloud_properties' => disk_cloud_properties }, + ], + ), + )) + end + let(:cloud_factory) { instance_double(CloudFactory, get: cloud) } + + before do + allow(Config).to receive(:name).and_return('fake-director-name') + allow(Config).to receive(:cloud_options).and_return('provider' => { 'path' => '/path/to/default/cpi' }) + allow(Config).to receive(:preferred_cpi_api_version).and_return(2) + allow(Config).to receive(:result).and_return(task_result) + allow(CloudFactory).to receive(:create).and_return(cloud_factory) + allow(cloud).to receive(:respond_to?).with(:set_disk_metadata).and_return(false) + allow(create_dynamic_disk_job).to receive(:task_id).and_return(task.id) + end + + describe 'DJ job class expectations' do + let(:job_type) { :create_dynamic_disk } + let(:queue) { :dynamic_disks } + it_behaves_like 'a DelayedJob job' + end + + describe '#perform' do + context 'when disk does not exist' do + it 'creates the disk without attaching it to a VM' do + expected_cloud_properties = disk_cloud_properties.merge('name' => disk_name) + expect(cloud).to receive(:create_disk).with(disk_size, expected_cloud_properties, vm.cid).and_return(disk_cid) + + result = create_dynamic_disk_job.perform + + expect(result).to eq("created disk `#{disk_name}` in deployment `#{instance.deployment.name}`") + expect(parsed_task_result).to eq({ 'disk_cid' => disk_cid }) + + model = Models::DynamicDisk.where(disk_cid: disk_cid).first + expect(model).not_to be_nil + expect(model.name).to eq(disk_name) + expect(model.size).to eq(disk_size) + expect(model.deployment_id).to eq(instance.deployment.id) + expect(model.disk_pool_name).to eq(disk_pool_name) + expect(model.cpi).to eq(vm.cpi) + expect(model.vm).to be_nil + end + end + + context 'when disk already exists in the database' do + let!(:disk) do + FactoryBot.create( + :models_dynamic_disk, + name: disk_name, + disk_cid: disk_cid, + deployment: vm.instance.deployment, + disk_pool_name: disk_pool_name, + ) + end + + it 'returns the existing disk without calling create_disk' do + expect(cloud).not_to receive(:create_disk) + + result = create_dynamic_disk_job.perform + + expect(result).to eq("created disk `#{disk_name}` in deployment `#{instance.deployment.name}`") + expect(parsed_task_result).to eq({ 'disk_cid' => disk_cid }) + end + end + + context 'when metadata is provided and CPI supports set_disk_metadata' do + it 'sets disk metadata after creation' do + expected_cloud_properties = disk_cloud_properties.merge('name' => disk_name) + expect(cloud).to receive(:create_disk).with(disk_size, expected_cloud_properties, vm.cid).and_return(disk_cid) + expect(cloud).to receive(:respond_to?).with(:set_disk_metadata).and_return(true) + expect(cloud).to receive(:set_disk_metadata).with( + disk_cid, + hash_including('fake-key' => 'fake-value'), + ) + + create_dynamic_disk_job.perform + end + end + + context 'when disk_pool_name cannot be found in cloud config' do + let!(:cloud_config) do + FactoryBot.create(:models_config_cloud, content: YAML.dump( + SharedSupport::DeploymentManifestHelper.simple_cloud_config.merge( + 'disk_types' => [ + { 'name' => 'different-disk-type', + 'disk_size' => 1024, + 'cloud_properties' => {} }, + ], + ), + )) + end + + it 'raises an error' do + expect { create_dynamic_disk_job.perform }.to raise_error("Could not find disk pool by name `#{disk_pool_name}`") + end + end + + context 'when instance cannot be found' do + let(:create_dynamic_disk_job) { Jobs::DynamicDisks::CreateDynamicDisk.new('unknown-instance-id', disk_name, disk_pool_name, disk_size, metadata) } + + it 'raises an error' do + expect { create_dynamic_disk_job.perform }.to raise_error("instance `unknown-instance-id` not found") + end + end + + context 'when there is no active VM for the instance' do + let(:vm) { FactoryBot.create(:models_vm, instance: instance, active: false) } + + it 'raises an error' do + expect { create_dynamic_disk_job.perform }.to raise_error("no active vm found for instance `#{instance.uuid}`") + end + end + end + end +end From 597e39e794a43b9c237ee03ccef0f7b8fb5dbed6 Mon Sep 17 00:00:00 2001 From: Clay Kauzlaric Date: Tue, 23 Jun 2026 14:34:16 -0400 Subject: [PATCH 2/5] fix disk querying performance Models::DynamicDisk.all.map will cause N+1 queries when accessing disk.deployment and disk.vm.instance for each row. Eager-load the associated records so listing scales with number of disks. ai-assisted=yes [TNZ-109499] Add CLI commands for Dynamic Disks - Phase 2 (list, create, standalone attach) Signed-off-by: Nishad Mathur --- .../bosh/director/api/controllers/dynamic_disks_controller.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/bosh-director/lib/bosh/director/api/controllers/dynamic_disks_controller.rb b/src/bosh-director/lib/bosh/director/api/controllers/dynamic_disks_controller.rb index 1353d892fba..f2c06da500a 100644 --- a/src/bosh-director/lib/bosh/director/api/controllers/dynamic_disks_controller.rb +++ b/src/bosh-director/lib/bosh/director/api/controllers/dynamic_disks_controller.rb @@ -6,7 +6,7 @@ class DynamicDisksController < BaseController include ValidationHelper get '/', scope: :list_dynamic_disks do - disks = Models::DynamicDisk.all.map do |disk| + disks = Models::DynamicDisk.eager(:deployment, vm: :instance).all.map do |disk| { name: disk.name, disk_cid: disk.disk_cid, From 875c2399e2aa8cd474ce1582943785732c76e081 Mon Sep 17 00:00:00 2001 From: Clay Kauzlaric Date: Tue, 23 Jun 2026 16:52:52 -0400 Subject: [PATCH 3/5] refactor: create_dynamic_disk errors if disk already exists also allows attach_dynamic_disk to accept and set metadata, since ProvideDynamicDisk technically can override metadata if a disk already exists ai-assisted=yes [TNZ-109499] Add CLI commands for Dynamic Disks - Phase 2 (list, create, standalone attach) Signed-off-by: Nishad Mathur --- .../controllers/dynamic_disks_controller.rb | 3 +- .../jobs/dynamic_disks/attach_dynamic_disk.rb | 8 +++- .../jobs/dynamic_disks/create_dynamic_disk.rb | 26 ++++++------- .../dynamic_disks_controller_spec.rb | 23 ++++++++++-- .../dynamic_disks/attach_dynamic_disk_spec.rb | 37 ++++++++++++++++++- .../dynamic_disks/create_dynamic_disk_spec.rb | 7 +--- 6 files changed, 80 insertions(+), 24 deletions(-) diff --git a/src/bosh-director/lib/bosh/director/api/controllers/dynamic_disks_controller.rb b/src/bosh-director/lib/bosh/director/api/controllers/dynamic_disks_controller.rb index f2c06da500a..01d0ccaeeba 100644 --- a/src/bosh-director/lib/bosh/director/api/controllers/dynamic_disks_controller.rb +++ b/src/bosh-director/lib/bosh/director/api/controllers/dynamic_disks_controller.rb @@ -44,12 +44,13 @@ class DynamicDisksController < BaseController request_hash = JSON.parse(request.body.read) instance_id = safe_property(request_hash, 'instance_id', class: String, min_length: 1) + metadata = safe_property(request_hash, 'metadata', class: Hash, optional: true) task = JobQueue.new.enqueue( current_user, Jobs::DynamicDisks::AttachDynamicDisk, 'attach dynamic disk', - [disk_name, instance_id], + [disk_name, instance_id, metadata], ) redirect "/tasks/#{task.id}" diff --git a/src/bosh-director/lib/bosh/director/jobs/dynamic_disks/attach_dynamic_disk.rb b/src/bosh-director/lib/bosh/director/jobs/dynamic_disks/attach_dynamic_disk.rb index 52c8dcc6fba..4b92db2452e 100644 --- a/src/bosh-director/lib/bosh/director/jobs/dynamic_disks/attach_dynamic_disk.rb +++ b/src/bosh-director/lib/bosh/director/jobs/dynamic_disks/attach_dynamic_disk.rb @@ -10,9 +10,10 @@ def self.job_type :attach_dynamic_disk end - def initialize(disk_name, instance_id) + def initialize(disk_name, instance_id, metadata = nil) @disk_name = disk_name @instance_id = instance_id + @metadata = metadata end def perform @@ -35,6 +36,11 @@ def perform disk_hint = with_vm_lock(vm.cid, timeout: VM_LOCK_TIMEOUT) { cloud.attach_disk(vm.cid, disk_model.disk_cid) } disk_model.update(vm_id: vm.id, availability_zone: vm.instance.availability_zone, disk_hint: disk_hint) + if !@metadata.nil? && disk_model.metadata != @metadata + MetadataUpdater.build.update_dynamic_disk_metadata(cloud, disk_model, @metadata) + disk_model.update(metadata: @metadata) + end + agent_client = AgentClient.with_agent_id(vm.agent_id, instance.name) agent_client.add_dynamic_disk(disk_model.disk_cid, disk_model.disk_hint) diff --git a/src/bosh-director/lib/bosh/director/jobs/dynamic_disks/create_dynamic_disk.rb b/src/bosh-director/lib/bosh/director/jobs/dynamic_disks/create_dynamic_disk.rb index 1c614d33f39..a3452ef0dc3 100644 --- a/src/bosh-director/lib/bosh/director/jobs/dynamic_disks/create_dynamic_disk.rb +++ b/src/bosh-director/lib/bosh/director/jobs/dynamic_disks/create_dynamic_disk.rb @@ -18,6 +18,9 @@ def initialize(instance_id, disk_name, disk_pool_name, disk_size, metadata) end def perform + disk_model = Models::DynamicDisk.find(name: @disk_name) + raise "disk `#{@disk_name}` already exists" unless disk_model.nil? + instance = Models::Instance.find(uuid: @instance_id) raise "instance `#{@instance_id}` not found" if instance.nil? @@ -27,19 +30,16 @@ def perform 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) - 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 + 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, + ) if !@metadata.nil? && disk_model.metadata != @metadata MetadataUpdater.build.update_dynamic_disk_metadata(cloud, disk_model, @metadata) diff --git a/src/bosh-director/spec/unit/bosh/director/api/controllers/dynamic_disks_controller_spec.rb b/src/bosh-director/spec/unit/bosh/director/api/controllers/dynamic_disks_controller_spec.rb index a0376694126..af38f1d6313 100644 --- a/src/bosh-director/spec/unit/bosh/director/api/controllers/dynamic_disks_controller_spec.rb +++ b/src/bosh-director/spec/unit/bosh/director/api/controllers/dynamic_disks_controller_spec.rb @@ -357,7 +357,7 @@ module Api end describe 'POST', '/:disk_name/attach' do - let(:content) { JSON.generate({ 'instance_id' => instance_id }) } + let(:content) { JSON.generate({ 'instance_id' => instance_id, 'metadata' => metadata }) } context 'when user is reader' do before { basic_authorize('reader', 'reader') } @@ -375,7 +375,7 @@ module Api 'dynamic-disks-attacher', Jobs::DynamicDisks::AttachDynamicDisk, 'attach dynamic disk', - ['disk_name', instance_id], + ['disk_name', instance_id, metadata], ).and_call_original post '/disk_name/attach', content, { 'CONTENT_TYPE' => 'application/json' } @@ -400,7 +400,7 @@ module Api 'admin', Jobs::DynamicDisks::AttachDynamicDisk, 'attach dynamic disk', - ['disk_name', instance_id], + ['disk_name', instance_id, metadata], ).and_call_original post '/disk_name/attach', content, { 'CONTENT_TYPE' => 'application/json' } @@ -408,6 +408,23 @@ module Api expect_redirect_to_queued_task(last_response) end + context 'when metadata is omitted from the body' do + let(:content) { JSON.generate({ 'instance_id' => instance_id }) } + + it 'enqueues an AttachDynamicDisk task with nil metadata' do + expect_any_instance_of(Bosh::Director::JobQueue).to receive(:enqueue).with( + 'admin', + Jobs::DynamicDisks::AttachDynamicDisk, + 'attach dynamic disk', + ['disk_name', instance_id, nil], + ).and_call_original + + post '/disk_name/attach', content, { 'CONTENT_TYPE' => 'application/json' } + + expect_redirect_to_queued_task(last_response) + end + end + context 'when instance_id is missing from the body' do let(:content) { JSON.generate({}) } diff --git a/src/bosh-director/spec/unit/bosh/director/jobs/dynamic_disks/attach_dynamic_disk_spec.rb b/src/bosh-director/spec/unit/bosh/director/jobs/dynamic_disks/attach_dynamic_disk_spec.rb index d1daeb74ef4..96bd0830e66 100644 --- a/src/bosh-director/spec/unit/bosh/director/jobs/dynamic_disks/attach_dynamic_disk_spec.rb +++ b/src/bosh-director/spec/unit/bosh/director/jobs/dynamic_disks/attach_dynamic_disk_spec.rb @@ -5,6 +5,7 @@ module Bosh::Director let(:disk_name) { 'fake-disk-name' } let(:disk_cid) { 'fake-disk-cid' } let(:disk_hint) { { 'id' => 'fake-disk-id' } } + let(:metadata) { nil } let(:agent_client) { instance_double(AgentClient) } let(:cloud) { instance_double(Bosh::Clouds::ExternalCpi) } @@ -12,7 +13,7 @@ module Bosh::Director let(:instance) { FactoryBot.create(:models_instance) } let!(:vm) { FactoryBot.create(:models_vm, instance: instance, active: true) } - let(:attach_dynamic_disk_job) { Jobs::DynamicDisks::AttachDynamicDisk.new(disk_name, instance.uuid) } + let(:attach_dynamic_disk_job) { Jobs::DynamicDisks::AttachDynamicDisk.new(disk_name, instance.uuid, metadata) } let(:cloud_factory) { instance_double(CloudFactory, get: cloud) } before do @@ -21,6 +22,7 @@ module Bosh::Director allow(Config).to receive(:preferred_cpi_api_version).and_return(2) allow(CloudFactory).to receive(:create).and_return(cloud_factory) allow(AgentClient).to receive(:with_agent_id).with(vm.agent_id, instance.name).and_return(agent_client) + allow(cloud).to receive(:respond_to?).with(:set_disk_metadata).and_return(false) end describe 'DJ job class expectations' do @@ -53,6 +55,39 @@ module Bosh::Director expect(disk.vm).to eq(vm) expect(disk.availability_zone).to eq(vm.instance.availability_zone) end + + context 'when metadata is provided and CPI supports set_disk_metadata' do + let(:metadata) { { 'fake-key' => 'fake-value' } } + + it 'sets disk metadata after attaching' do + expect(attach_dynamic_disk_job).to receive(:with_vm_lock).with(vm.cid, timeout: Jobs::Helpers::DynamicDiskHelpers::VM_LOCK_TIMEOUT).and_yield + expect(cloud).to receive(:attach_disk).with(vm.cid, disk_cid).and_return(disk_hint) + expect(cloud).to receive(:respond_to?).with(:set_disk_metadata).and_return(true) + expect(cloud).to receive(:set_disk_metadata).with( + disk_cid, + hash_including('fake-key' => 'fake-value'), + ) + expect(agent_client).to receive(:add_dynamic_disk).with(disk_cid, disk_hint) + + attach_dynamic_disk_job.perform + + disk.reload + expect(disk.metadata).to eq(metadata) + end + end + + context 'when metadata is nil' do + let(:metadata) { nil } + + it 'does not call set_disk_metadata' do + expect(attach_dynamic_disk_job).to receive(:with_vm_lock).with(vm.cid, timeout: Jobs::Helpers::DynamicDiskHelpers::VM_LOCK_TIMEOUT).and_yield + expect(cloud).to receive(:attach_disk).with(vm.cid, disk_cid).and_return(disk_hint) + expect(cloud).not_to receive(:set_disk_metadata) + expect(agent_client).to receive(:add_dynamic_disk).with(disk_cid, disk_hint) + + attach_dynamic_disk_job.perform + end + end end context 'when disk is already attached to the same VM' do diff --git a/src/bosh-director/spec/unit/bosh/director/jobs/dynamic_disks/create_dynamic_disk_spec.rb b/src/bosh-director/spec/unit/bosh/director/jobs/dynamic_disks/create_dynamic_disk_spec.rb index 1e7be924890..e1cd7484da4 100644 --- a/src/bosh-director/spec/unit/bosh/director/jobs/dynamic_disks/create_dynamic_disk_spec.rb +++ b/src/bosh-director/spec/unit/bosh/director/jobs/dynamic_disks/create_dynamic_disk_spec.rb @@ -84,13 +84,10 @@ def parsed_task_result ) end - it 'returns the existing disk without calling create_disk' do + it 'raises an error without calling create_disk' do expect(cloud).not_to receive(:create_disk) - result = create_dynamic_disk_job.perform - - expect(result).to eq("created disk `#{disk_name}` in deployment `#{instance.deployment.name}`") - expect(parsed_task_result).to eq({ 'disk_cid' => disk_cid }) + expect { create_dynamic_disk_job.perform }.to raise_error("disk `#{disk_name}` already exists") end end From 68e787277d6bb799b56d7ebc3cc1734c02128236 Mon Sep 17 00:00:00 2001 From: Nishad Mathur Date: Mon, 22 Jun 2026 17:05:38 -0700 Subject: [PATCH 4/5] Improve dynamic disk API: list fields, AZ-based create, AZ assertion on attach - GET /dynamic_disks now returns disk_pool_name and metadata fields - POST /dynamic_disks (create): replace instance_id with deployment_name+az; finds an active VM in the requested AZ to use as CPI hint for placement; stores AZ in availability_zone column at creation time - POST /dynamic_disks/:name/attach: assert disk AZ matches target VM AZ, skip check when disk AZ is nil (disks created before AZ tracking) - Refactor find_disk_cloud_properties helper to accept deployment or instance - 76 unit specs, all passing [TNZ-109499] Co-authored-by: Cursor --- .../controllers/dynamic_disks_controller.rb | 15 +++-- .../jobs/dynamic_disks/attach_dynamic_disk.rb | 6 ++ .../jobs/dynamic_disks/create_dynamic_disk.rb | 32 +++++++--- .../jobs/helpers/dynamic_disk_helpers.rb | 7 ++- .../dynamic_disks_controller_spec.rb | 63 +++++++++++++++---- .../dynamic_disks/attach_dynamic_disk_spec.rb | 63 +++++++++++++++++++ .../dynamic_disks/create_dynamic_disk_spec.rb | 52 ++++++++++----- 7 files changed, 194 insertions(+), 44 deletions(-) diff --git a/src/bosh-director/lib/bosh/director/api/controllers/dynamic_disks_controller.rb b/src/bosh-director/lib/bosh/director/api/controllers/dynamic_disks_controller.rb index 01d0ccaeeba..1c8c621fecc 100644 --- a/src/bosh-director/lib/bosh/director/api/controllers/dynamic_disks_controller.rb +++ b/src/bosh-director/lib/bosh/director/api/controllers/dynamic_disks_controller.rb @@ -14,7 +14,9 @@ class DynamicDisksController < BaseController instance: disk.vm&.instance&.name, availability_zone: disk.availability_zone, size: disk.size, + disk_pool_name: disk.disk_pool_name, cpi: disk.cpi, + metadata: disk.metadata, } end json_encode(disks) @@ -23,17 +25,18 @@ class DynamicDisksController < BaseController post '/', scope: :create_dynamic_disks, consumes: :json do request_hash = JSON.parse(request.body.read) - instance_id = safe_property(request_hash, 'instance_id', class: String, min_length: 1) - disk_name = safe_property(request_hash, 'disk_name', class: String, min_length: 1) - disk_pool_name = safe_property(request_hash, 'disk_pool_name', class: String, min_length: 1) - disk_size = safe_property(request_hash, 'disk_size', class: Integer, min: 1) - metadata = safe_property(request_hash, 'metadata', class: Hash, optional: true) + deployment_name = safe_property(request_hash, 'deployment_name', class: String, min_length: 1) + az = safe_property(request_hash, 'az', class: String, min_length: 1) + disk_name = safe_property(request_hash, 'disk_name', class: String, min_length: 1) + disk_pool_name = safe_property(request_hash, 'disk_pool_name', class: String, min_length: 1) + disk_size = safe_property(request_hash, 'disk_size', class: Integer, min: 1) + metadata = safe_property(request_hash, 'metadata', class: Hash, optional: true) task = JobQueue.new.enqueue( current_user, Jobs::DynamicDisks::CreateDynamicDisk, 'create dynamic disk', - [instance_id, disk_name, disk_pool_name, disk_size, metadata], + [deployment_name, az, disk_name, disk_pool_name, disk_size, metadata], ) redirect "/tasks/#{task.id}" diff --git a/src/bosh-director/lib/bosh/director/jobs/dynamic_disks/attach_dynamic_disk.rb b/src/bosh-director/lib/bosh/director/jobs/dynamic_disks/attach_dynamic_disk.rb index 4b92db2452e..e7c3fd1f842 100644 --- a/src/bosh-director/lib/bosh/director/jobs/dynamic_disks/attach_dynamic_disk.rb +++ b/src/bosh-director/lib/bosh/director/jobs/dynamic_disks/attach_dynamic_disk.rb @@ -32,6 +32,12 @@ def perform raise "disk `#{@disk_name}` is already attached to a different vm `#{disk_model.vm.cid}`" end + disk_az = disk_model.availability_zone + vm_az = vm.instance.availability_zone + unless disk_az.nil? || vm_az.nil? || disk_az == vm_az + raise "disk `#{@disk_name}` is in AZ `#{disk_az}` but instance `#{@instance_id}` is in AZ `#{vm_az}`" + end + cloud = Bosh::Director::CloudFactory.create.get(disk_model.cpi, vm.stemcell_api_version) disk_hint = with_vm_lock(vm.cid, timeout: VM_LOCK_TIMEOUT) { cloud.attach_disk(vm.cid, disk_model.disk_cid) } disk_model.update(vm_id: vm.id, availability_zone: vm.instance.availability_zone, disk_hint: disk_hint) diff --git a/src/bosh-director/lib/bosh/director/jobs/dynamic_disks/create_dynamic_disk.rb b/src/bosh-director/lib/bosh/director/jobs/dynamic_disks/create_dynamic_disk.rb index a3452ef0dc3..84e64ae9c29 100644 --- a/src/bosh-director/lib/bosh/director/jobs/dynamic_disks/create_dynamic_disk.rb +++ b/src/bosh-director/lib/bosh/director/jobs/dynamic_disks/create_dynamic_disk.rb @@ -9,8 +9,9 @@ def self.job_type :create_dynamic_disk end - def initialize(instance_id, disk_name, disk_pool_name, disk_size, metadata) - @instance_id = instance_id + def initialize(deployment_name, az, disk_name, disk_pool_name, disk_size, metadata) + @deployment_name = deployment_name + @az = az @disk_name = disk_name @disk_pool_name = disk_pool_name @disk_size = disk_size @@ -21,24 +22,28 @@ def perform disk_model = Models::DynamicDisk.find(name: @disk_name) raise "disk `#{@disk_name}` already exists" unless disk_model.nil? - instance = Models::Instance.find(uuid: @instance_id) - raise "instance `#{@instance_id}` not found" if instance.nil? + deployment = Models::Deployment.find(name: @deployment_name) + raise "deployment `#{@deployment_name}` not found" if deployment.nil? - vm = instance.active_vm - raise "no active vm found for instance `#{@instance_id}`" if vm.nil? + # Find an active VM in the requested AZ to use as the hint for create_disk. + # The IaaS uses the VM's location to determine which AZ/datastore to place the disk in. + vm = find_active_vm_in_az(deployment, @az) + raise "no active VM found in deployment `#{@deployment_name}` in AZ `#{@az}`" if vm.nil? - cloud_properties = find_disk_cloud_properties(instance, @disk_pool_name) cloud = Bosh::Director::CloudFactory.create.get(vm.cpi) + cloud_properties = find_disk_cloud_properties(deployment, @disk_pool_name) 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, + deployment_id: deployment.id, size: @disk_size, disk_pool_name: @disk_pool_name, cpi: vm.cpi, + availability_zone: @az, ) if !@metadata.nil? && disk_model.metadata != @metadata @@ -51,7 +56,16 @@ def perform task_result.write(JSON.generate(disk_info)) task_result.write("\n") - "created disk `#{@disk_name}` in deployment `#{instance.deployment.name}`" + "created disk `#{@disk_name}` in deployment `#{deployment.name}` in AZ `#{@az}`" + end + + private + + def find_active_vm_in_az(deployment, az) + deployment.instances + .select { |i| i.availability_zone == az } + .flat_map { |i| i.vms.select(&:active) } + .first end end end diff --git a/src/bosh-director/lib/bosh/director/jobs/helpers/dynamic_disk_helpers.rb b/src/bosh-director/lib/bosh/director/jobs/helpers/dynamic_disk_helpers.rb index 2e46f7736e1..eeedb4ef49e 100644 --- a/src/bosh-director/lib/bosh/director/jobs/helpers/dynamic_disk_helpers.rb +++ b/src/bosh-director/lib/bosh/director/jobs/helpers/dynamic_disk_helpers.rb @@ -3,8 +3,9 @@ module Jobs::Helpers module DynamicDiskHelpers VM_LOCK_TIMEOUT = 60 - def find_disk_cloud_properties(instance, disk_pool_name) - teams = instance.deployment.teams + def find_disk_cloud_properties(deployment_or_instance, disk_pool_name) + deployment = deployment_or_instance.is_a?(Models::Deployment) ? deployment_or_instance : deployment_or_instance.deployment + teams = deployment.teams configs = Models::Config.latest_set_for_teams('cloud', *teams) raise 'No cloud configs provided' if configs.empty? @@ -16,4 +17,4 @@ def find_disk_cloud_properties(instance, disk_pool_name) end end end -end \ No newline at end of file +end diff --git a/src/bosh-director/spec/unit/bosh/director/api/controllers/dynamic_disks_controller_spec.rb b/src/bosh-director/spec/unit/bosh/director/api/controllers/dynamic_disks_controller_spec.rb index af38f1d6313..e76fe1459f2 100644 --- a/src/bosh-director/spec/unit/bosh/director/api/controllers/dynamic_disks_controller_spec.rb +++ b/src/bosh-director/spec/unit/bosh/director/api/controllers/dynamic_disks_controller_spec.rb @@ -16,6 +16,8 @@ module Api before { App.new(config) } let(:instance_id) { 'fake-instance-id' } + let(:deployment_name) { 'fake-deployment' } + let(:az) { 'z1' } let(:disk_pool_name) { 'fake_disk_pool_name' } let(:disk_name) { 'fake_disk_name' } let(:disk_size) { 1000 } @@ -241,6 +243,8 @@ module Api disk_cid: 'cid-a', deployment: deployment, size: 1024, + disk_pool_name: 'large', + metadata: { 'env' => 'prod' }, ) end let!(:disk2) do @@ -249,16 +253,30 @@ module Api disk_cid: 'cid-b', deployment: deployment, size: 2048, + disk_pool_name: 'small', ) end - it 'returns all dynamic disks' do + it 'returns all dynamic disks including disk_pool_name and metadata' do get '/' expect(last_response.status).to eq(200) body = JSON.parse(last_response.body) expect(body.size).to eq(2) - expect(body).to include(hash_including('name' => 'disk-a', 'disk_cid' => 'cid-a', 'deployment' => 'my-deployment', 'size' => 1024)) - expect(body).to include(hash_including('name' => 'disk-b', 'disk_cid' => 'cid-b', 'deployment' => 'my-deployment', 'size' => 2048)) + expect(body).to include(hash_including( + 'name' => 'disk-a', + 'disk_cid' => 'cid-a', + 'deployment' => 'my-deployment', + 'size' => 1024, + 'disk_pool_name' => 'large', + 'metadata' => { 'env' => 'prod' }, + )) + expect(body).to include(hash_including( + 'name' => 'disk-b', + 'disk_cid' => 'cid-b', + 'deployment' => 'my-deployment', + 'size' => 2048, + 'disk_pool_name' => 'small', + )) end end end @@ -277,11 +295,12 @@ module Api describe 'POST', '/' do let(:content) do JSON.generate({ - 'instance_id' => instance_id, - 'disk_pool_name' => disk_pool_name, - 'disk_name' => disk_name, - 'disk_size' => disk_size, - 'metadata' => metadata, + 'deployment_name' => deployment_name, + 'az' => az, + 'disk_pool_name' => disk_pool_name, + 'disk_name' => disk_name, + 'disk_size' => disk_size, + 'metadata' => metadata, }) end @@ -301,7 +320,7 @@ module Api 'dynamic-disks-creator', Jobs::DynamicDisks::CreateDynamicDisk, 'create dynamic disk', - [instance_id, disk_name, disk_pool_name, disk_size, metadata], + [deployment_name, az, disk_name, disk_pool_name, disk_size, metadata], ).and_call_original post '/', content, { 'CONTENT_TYPE' => 'application/json' } @@ -326,7 +345,7 @@ module Api 'admin', Jobs::DynamicDisks::CreateDynamicDisk, 'create dynamic disk', - [instance_id, disk_name, disk_pool_name, disk_size, metadata], + [deployment_name, az, disk_name, disk_pool_name, disk_size, metadata], ).and_call_original post '/', content, { 'CONTENT_TYPE' => 'application/json' } @@ -334,13 +353,33 @@ module Api expect_redirect_to_queued_task(last_response) end + context 'when deployment_name is missing' do + let(:deployment_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('deployment_name') + end + end + + context 'when az is missing' do + let(:az) { 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('az') + end + end + 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") + expect(JSON.parse(last_response.body)['description']).to include('disk_name') end end @@ -350,7 +389,7 @@ module Api 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") + expect(JSON.parse(last_response.body)['description']).to include('disk_size') end end end diff --git a/src/bosh-director/spec/unit/bosh/director/jobs/dynamic_disks/attach_dynamic_disk_spec.rb b/src/bosh-director/spec/unit/bosh/director/jobs/dynamic_disks/attach_dynamic_disk_spec.rb index 96bd0830e66..a08910d2a5c 100644 --- a/src/bosh-director/spec/unit/bosh/director/jobs/dynamic_disks/attach_dynamic_disk_spec.rb +++ b/src/bosh-director/spec/unit/bosh/director/jobs/dynamic_disks/attach_dynamic_disk_spec.rb @@ -131,6 +131,69 @@ module Bosh::Director end end + context 'AZ mismatch between disk and target VM' do + let!(:disk) do + FactoryBot.create( + :models_dynamic_disk, + name: disk_name, + disk_cid: disk_cid, + deployment: instance.deployment, + availability_zone: 'z1', + ) + end + + before do + allow(instance).to receive(:availability_zone).and_return('z2') + instance.update(availability_zone: 'z2') + end + + it 'raises an error when disk AZ does not match VM AZ' do + expect { attach_dynamic_disk_job.perform }.to raise_error( + /disk `#{disk_name}` is in AZ `z1` but instance `#{instance.uuid}` is in AZ `z2`/, + ) + end + end + + context 'AZ match between disk and target VM' do + let!(:disk) do + FactoryBot.create( + :models_dynamic_disk, + name: disk_name, + disk_cid: disk_cid, + deployment: instance.deployment, + availability_zone: instance.availability_zone, + ) + end + + it 'attaches successfully when AZs match' do + expect(attach_dynamic_disk_job).to receive(:with_vm_lock).with(vm.cid, timeout: Jobs::Helpers::DynamicDiskHelpers::VM_LOCK_TIMEOUT).and_yield + expect(cloud).to receive(:attach_disk).with(vm.cid, disk_cid).and_return(disk_hint) + expect(agent_client).to receive(:add_dynamic_disk).with(disk_cid, disk_hint) + + expect { attach_dynamic_disk_job.perform }.not_to raise_error + end + end + + context 'when disk has no AZ set (created before AZ tracking)' do + let!(:disk) do + FactoryBot.create( + :models_dynamic_disk, + name: disk_name, + disk_cid: disk_cid, + deployment: instance.deployment, + availability_zone: nil, + ) + end + + it 'skips AZ check and attaches successfully' do + expect(attach_dynamic_disk_job).to receive(:with_vm_lock).with(vm.cid, timeout: Jobs::Helpers::DynamicDiskHelpers::VM_LOCK_TIMEOUT).and_yield + expect(cloud).to receive(:attach_disk).with(vm.cid, disk_cid).and_return(disk_hint) + expect(agent_client).to receive(:add_dynamic_disk).with(disk_cid, disk_hint) + + expect { attach_dynamic_disk_job.perform }.not_to raise_error + end + end + context 'when disk does not exist' do it 'raises an error' do expect { attach_dynamic_disk_job.perform }.to raise_error("disk `#{disk_name}` not found") diff --git a/src/bosh-director/spec/unit/bosh/director/jobs/dynamic_disks/create_dynamic_disk_spec.rb b/src/bosh-director/spec/unit/bosh/director/jobs/dynamic_disks/create_dynamic_disk_spec.rb index e1cd7484da4..cf2f6ac17a2 100644 --- a/src/bosh-director/spec/unit/bosh/director/jobs/dynamic_disks/create_dynamic_disk_spec.rb +++ b/src/bosh-director/spec/unit/bosh/director/jobs/dynamic_disks/create_dynamic_disk_spec.rb @@ -8,9 +8,11 @@ module Bosh::Director let(:disk_cloud_properties) { { 'fake-disk-cloud-property-key' => 'fake-disk-cloud-property-value' } } let(:disk_size) { 1000 } let(:metadata) { { 'fake-key' => 'fake-value' } } + let(:az) { 'z1' } let(:cloud) { instance_double(Bosh::Clouds::ExternalCpi) } - let(:instance) { FactoryBot.create(:models_instance) } + let(:deployment) { FactoryBot.create(:models_deployment) } + let(:instance) { FactoryBot.create(:models_instance, deployment: deployment, availability_zone: az) } let!(:vm) { FactoryBot.create(:models_vm, instance: instance, active: true) } let(:task_result) { TaskDBWriter.new(:result_output, task.id) } @@ -20,7 +22,9 @@ def parsed_task_result JSON.parse(Models::Task.first(id: 42).result_output) end - let(:create_dynamic_disk_job) { Jobs::DynamicDisks::CreateDynamicDisk.new(instance.uuid, disk_name, disk_pool_name, disk_size, metadata) } + let(:create_dynamic_disk_job) do + Jobs::DynamicDisks::CreateDynamicDisk.new(deployment.name, az, disk_name, disk_pool_name, disk_size, metadata) + end let!(:cloud_config) do FactoryBot.create(:models_config_cloud, content: YAML.dump( @@ -53,22 +57,23 @@ def parsed_task_result describe '#perform' do context 'when disk does not exist' do - it 'creates the disk without attaching it to a VM' do + it 'creates the disk using a VM from the requested AZ as hint and stores the AZ' do expected_cloud_properties = disk_cloud_properties.merge('name' => disk_name) expect(cloud).to receive(:create_disk).with(disk_size, expected_cloud_properties, vm.cid).and_return(disk_cid) result = create_dynamic_disk_job.perform - expect(result).to eq("created disk `#{disk_name}` in deployment `#{instance.deployment.name}`") + expect(result).to eq("created disk `#{disk_name}` in deployment `#{deployment.name}` in AZ `#{az}`") expect(parsed_task_result).to eq({ 'disk_cid' => disk_cid }) model = Models::DynamicDisk.where(disk_cid: disk_cid).first expect(model).not_to be_nil expect(model.name).to eq(disk_name) expect(model.size).to eq(disk_size) - expect(model.deployment_id).to eq(instance.deployment.id) + expect(model.deployment_id).to eq(deployment.id) expect(model.disk_pool_name).to eq(disk_pool_name) expect(model.cpi).to eq(vm.cpi) + expect(model.availability_zone).to eq(az) expect(model.vm).to be_nil end end @@ -79,8 +84,9 @@ def parsed_task_result :models_dynamic_disk, name: disk_name, disk_cid: disk_cid, - deployment: vm.instance.deployment, + deployment: deployment, disk_pool_name: disk_pool_name, + availability_zone: az, ) end @@ -118,24 +124,42 @@ def parsed_task_result )) end - it 'raises an error' do - expect { create_dynamic_disk_job.perform }.to raise_error("Could not find disk pool by name `#{disk_pool_name}`") + it 'raises an error before calling create_disk' do + expect(cloud).not_to receive(:create_disk) + expect { create_dynamic_disk_job.perform }.to raise_error(/Could not find disk pool by name/) end end - context 'when instance cannot be found' do - let(:create_dynamic_disk_job) { Jobs::DynamicDisks::CreateDynamicDisk.new('unknown-instance-id', disk_name, disk_pool_name, disk_size, metadata) } + context 'when deployment cannot be found' do + let(:create_dynamic_disk_job) do + Jobs::DynamicDisks::CreateDynamicDisk.new('unknown-deployment', az, disk_name, disk_pool_name, disk_size, metadata) + end it 'raises an error' do - expect { create_dynamic_disk_job.perform }.to raise_error("instance `unknown-instance-id` not found") + expect { create_dynamic_disk_job.perform }.to raise_error('deployment `unknown-deployment` not found') end end - context 'when there is no active VM for the instance' do - let(:vm) { FactoryBot.create(:models_vm, instance: instance, active: false) } + context 'when no active VM exists in the requested AZ' do + let!(:vm) { FactoryBot.create(:models_vm, instance: instance, active: false) } it 'raises an error' do - expect { create_dynamic_disk_job.perform }.to raise_error("no active vm found for instance `#{instance.uuid}`") + expect { create_dynamic_disk_job.perform }.to raise_error( + "no active VM found in deployment `#{deployment.name}` in AZ `#{az}`", + ) + end + end + + context 'when VM exists in a different AZ' do + let(:other_az) { 'z2' } + let(:create_dynamic_disk_job) do + Jobs::DynamicDisks::CreateDynamicDisk.new(deployment.name, other_az, disk_name, disk_pool_name, disk_size, metadata) + end + + it 'raises an error when no VM found in requested AZ' do + expect { create_dynamic_disk_job.perform }.to raise_error( + "no active VM found in deployment `#{deployment.name}` in AZ `#{other_az}`", + ) end end end From bb78550bac0613f18b147ca7b83178b85e5a158c Mon Sep 17 00:00:00 2001 From: Nishad Mathur Date: Tue, 23 Jun 2026 18:47:51 -0700 Subject: [PATCH 5/5] Address PR review feedback on dynamic disk create/attach jobs - Avoid mutating the cloud_properties hash returned by find_disk_cloud_properties; use merge instead - Clean up orphaned IaaS disk with cloud.delete_disk if the DB write fails after cloud.create_disk - Fix attach idempotency: remove early return on same-VM so agent reconciliation (add_dynamic_disk) always runs on retry; guard the agent call when disk_hint is nil - Expand POST / controller validation tests to match the coverage of POST /provide ai-assisted=yes [TNZ-109499] Add CLI commands for Dynamic Disks - Phase 2 (list, create, standalone attach) Co-authored-by: Cursor --- .../jobs/dynamic_disks/attach_dynamic_disk.rb | 16 +++-- .../jobs/dynamic_disks/create_dynamic_disk.rb | 25 ++++---- .../dynamic_disks_controller_spec.rb | 60 +++++++++++++++++++ .../dynamic_disks/attach_dynamic_disk_spec.rb | 7 ++- .../dynamic_disks/create_dynamic_disk_spec.rb | 10 ++++ 5 files changed, 100 insertions(+), 18 deletions(-) diff --git a/src/bosh-director/lib/bosh/director/jobs/dynamic_disks/attach_dynamic_disk.rb b/src/bosh-director/lib/bosh/director/jobs/dynamic_disks/attach_dynamic_disk.rb index e7c3fd1f842..7795ca27252 100644 --- a/src/bosh-director/lib/bosh/director/jobs/dynamic_disks/attach_dynamic_disk.rb +++ b/src/bosh-director/lib/bosh/director/jobs/dynamic_disks/attach_dynamic_disk.rb @@ -26,10 +26,13 @@ def perform vm = instance.active_vm raise "no active vm found for instance `#{@instance_id}`" if vm.nil? + already_attached_same_vm = false unless disk_model.vm.nil? - return "disk `#{@disk_name}` is already attached to vm `#{disk_model.vm.cid}`" if disk_model.vm.id == vm.id - - raise "disk `#{@disk_name}` is already attached to a different vm `#{disk_model.vm.cid}`" + if disk_model.vm.id == vm.id + already_attached_same_vm = true + else + raise "disk `#{@disk_name}` is already attached to a different vm `#{disk_model.vm.cid}`" + end end disk_az = disk_model.availability_zone @@ -39,8 +42,11 @@ def perform end cloud = Bosh::Director::CloudFactory.create.get(disk_model.cpi, vm.stemcell_api_version) - disk_hint = with_vm_lock(vm.cid, timeout: VM_LOCK_TIMEOUT) { cloud.attach_disk(vm.cid, disk_model.disk_cid) } - disk_model.update(vm_id: vm.id, availability_zone: vm.instance.availability_zone, disk_hint: disk_hint) + + unless already_attached_same_vm + disk_hint = with_vm_lock(vm.cid, timeout: VM_LOCK_TIMEOUT) { cloud.attach_disk(vm.cid, disk_model.disk_cid) } + disk_model.update(vm_id: vm.id, availability_zone: vm.instance.availability_zone, disk_hint: disk_hint) + end if !@metadata.nil? && disk_model.metadata != @metadata MetadataUpdater.build.update_dynamic_disk_metadata(cloud, disk_model, @metadata) diff --git a/src/bosh-director/lib/bosh/director/jobs/dynamic_disks/create_dynamic_disk.rb b/src/bosh-director/lib/bosh/director/jobs/dynamic_disks/create_dynamic_disk.rb index 84e64ae9c29..7de0a2ba140 100644 --- a/src/bosh-director/lib/bosh/director/jobs/dynamic_disks/create_dynamic_disk.rb +++ b/src/bosh-director/lib/bosh/director/jobs/dynamic_disks/create_dynamic_disk.rb @@ -32,19 +32,24 @@ def perform cloud = Bosh::Director::CloudFactory.create.get(vm.cpi) - cloud_properties = find_disk_cloud_properties(deployment, @disk_pool_name) + cloud_properties = find_disk_cloud_properties(deployment, @disk_pool_name).clone 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: deployment.id, - size: @disk_size, - disk_pool_name: @disk_pool_name, - cpi: vm.cpi, - availability_zone: @az, - ) + begin + disk_model = Models::DynamicDisk.create( + name: @disk_name, + disk_cid: disk_cid, + deployment_id: deployment.id, + size: @disk_size, + disk_pool_name: @disk_pool_name, + cpi: vm.cpi, + availability_zone: @az, + ) + rescue + cloud.delete_disk(disk_cid) + raise + end if !@metadata.nil? && disk_model.metadata != @metadata MetadataUpdater.build.update_dynamic_disk_metadata(cloud, disk_model, @metadata) diff --git a/src/bosh-director/spec/unit/bosh/director/api/controllers/dynamic_disks_controller_spec.rb b/src/bosh-director/spec/unit/bosh/director/api/controllers/dynamic_disks_controller_spec.rb index e76fe1459f2..f308d4eff74 100644 --- a/src/bosh-director/spec/unit/bosh/director/api/controllers/dynamic_disks_controller_spec.rb +++ b/src/bosh-director/spec/unit/bosh/director/api/controllers/dynamic_disks_controller_spec.rb @@ -363,6 +363,16 @@ module Api end end + context 'when deployment_name is empty' do + let(:deployment_name) { '' } + + 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('deployment_name') + end + end + context 'when az is missing' do let(:az) { nil } @@ -373,6 +383,16 @@ module Api end end + context 'when az is empty' do + let(:az) { '' } + + 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('az') + end + end + context 'when disk_name is nil' do let(:disk_name) { nil } @@ -383,6 +403,46 @@ module Api end end + context 'when disk_name is empty' do + let(:disk_name) { '' } + + 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_pool_name is nil' do + let(:disk_pool_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_pool_name') + end + end + + context 'when disk_pool_name is empty' do + let(:disk_pool_name) { '' } + + 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_pool_name') + end + end + + context 'when disk_size is nil' do + let(:disk_size) { 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_size') + end + end + context 'when disk_size is 0' do let(:disk_size) { 0 } diff --git a/src/bosh-director/spec/unit/bosh/director/jobs/dynamic_disks/attach_dynamic_disk_spec.rb b/src/bosh-director/spec/unit/bosh/director/jobs/dynamic_disks/attach_dynamic_disk_spec.rb index a08910d2a5c..e3d921f2731 100644 --- a/src/bosh-director/spec/unit/bosh/director/jobs/dynamic_disks/attach_dynamic_disk_spec.rb +++ b/src/bosh-director/spec/unit/bosh/director/jobs/dynamic_disks/attach_dynamic_disk_spec.rb @@ -98,16 +98,17 @@ module Bosh::Director disk_cid: disk_cid, deployment: instance.deployment, vm: vm, + disk_hint: disk_hint, ) end - it 'returns a success message without re-attaching (idempotent)' do + it 'skips CPI attach but still notifies the agent (idempotent retry)' do expect(cloud).not_to receive(:attach_disk) - expect(agent_client).not_to receive(:add_dynamic_disk) + expect(agent_client).to receive(:add_dynamic_disk).with(disk_cid, disk_hint) result = attach_dynamic_disk_job.perform - expect(result).to eq("disk `#{disk_name}` is already attached to vm `#{vm.cid}`") + expect(result).to eq("attached disk `#{disk_name}` to vm `#{vm.cid}` in deployment `#{instance.deployment.name}`") end end diff --git a/src/bosh-director/spec/unit/bosh/director/jobs/dynamic_disks/create_dynamic_disk_spec.rb b/src/bosh-director/spec/unit/bosh/director/jobs/dynamic_disks/create_dynamic_disk_spec.rb index cf2f6ac17a2..3bb5c38b0cf 100644 --- a/src/bosh-director/spec/unit/bosh/director/jobs/dynamic_disks/create_dynamic_disk_spec.rb +++ b/src/bosh-director/spec/unit/bosh/director/jobs/dynamic_disks/create_dynamic_disk_spec.rb @@ -76,6 +76,16 @@ def parsed_task_result expect(model.availability_zone).to eq(az) expect(model.vm).to be_nil end + + context 'when DB write fails after IaaS disk creation' do + it 'deletes the IaaS disk and re-raises' do + expect(cloud).to receive(:create_disk).and_return(disk_cid) + allow(Models::DynamicDisk).to receive(:create).and_raise(Sequel::Error, 'DB failure') + expect(cloud).to receive(:delete_disk).with(disk_cid) + + expect { create_dynamic_disk_job.perform }.to raise_error(Sequel::Error, 'DB failure') + end + end end context 'when disk already exists in the database' do