From af695e80ff9ac96361c7846ab9dd02f72c2f336e Mon Sep 17 00:00:00 2001 From: Nishad Mathur Date: Thu, 21 Aug 2025 21:16:41 +0000 Subject: [PATCH 1/8] Add dynamic disks provide request handler * Add database migration for dynamic_disks table * Handle NATS request to provide dynamic disk * Attach disk via CPI, save disk info in the database Co-authored-by: Maria Shaldybin --- jobs/nats/templates/nats.cfg.erb | 2 +- spec/nats_templates_spec.rb | 2 +- src/bosh-director/bin/bosh-director | 2 + .../20250623223600_add_dynamic_disks.rb | 57 +++ src/bosh-director/lib/bosh/director.rb | 8 + .../lib/bosh/director/api_nats/api.rb | 78 +++++ .../api_nats/dynamic_disk_controller.rb | 92 +++++ .../jobs/dynamic_disks/attach_dynamic_disk.rb | 80 +++++ .../jobs/dynamic_disks/create_dynamic_disk.rb | 85 +++++ .../jobs/dynamic_disks/delete_dynamic_disk.rb | 74 ++++ .../jobs/dynamic_disks/detach_dynamic_disk.rb | 71 ++++ .../dynamic_disks/provide_dynamic_disk.rb | 79 +++++ .../lib/bosh/director/metadata_updater.rb | 11 + src/bosh-director/lib/bosh/director/models.rb | 1 + .../lib/bosh/director/models/dynamic_disk.rb | 23 ++ .../lib/bosh/director/tasks/db.rake | 2 +- .../lib/bosh/director/validation_helper.rb | 16 + src/bosh-director/spec/factories.rb | 7 + .../api_nats/dynamic_disk_controller_spec.rb | 330 ++++++++++++++++++ .../20250623223600_add_dynamic_disks_spec.rb | 26 ++ .../verify_migration_digest_spec.rb | 4 + .../provide_dynamic_disk_spec.rb | 198 +++++++++++ .../bosh/director/validation_helper_spec.rb | 32 ++ .../lib/nats_sync/nats_auth_config.rb | 5 +- 24 files changed, 1280 insertions(+), 5 deletions(-) create mode 100644 src/bosh-director/db/migrations/20250623223600_add_dynamic_disks.rb create mode 100644 src/bosh-director/lib/bosh/director/api_nats/api.rb create mode 100644 src/bosh-director/lib/bosh/director/api_nats/dynamic_disk_controller.rb 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/lib/bosh/director/jobs/dynamic_disks/delete_dynamic_disk.rb create mode 100644 src/bosh-director/lib/bosh/director/jobs/dynamic_disks/detach_dynamic_disk.rb create mode 100644 src/bosh-director/lib/bosh/director/jobs/dynamic_disks/provide_dynamic_disk.rb create mode 100644 src/bosh-director/lib/bosh/director/models/dynamic_disk.rb create mode 100644 src/bosh-director/spec/unit/bosh/director/api_nats/dynamic_disk_controller_spec.rb create mode 100644 src/bosh-director/spec/unit/bosh/director/db/migrations/20250623223600_add_dynamic_disks_spec.rb create mode 100644 src/bosh-director/spec/unit/bosh/director/jobs/dynamic_disks/provide_dynamic_disk_spec.rb diff --git a/jobs/nats/templates/nats.cfg.erb b/jobs/nats/templates/nats.cfg.erb index e9d715c7478..a1f6477854a 100644 --- a/jobs/nats/templates/nats.cfg.erb +++ b/jobs/nats/templates/nats.cfg.erb @@ -12,7 +12,7 @@ authorization { { user: "C=USA, O=Cloud Foundry, CN=default.director.bosh-internal" permissions: { - publish: [ "agent.*", "hm.director.alert" ] + publish: [ "agent.*", "agent.inbox.>", "hm.director.alert" ] subscribe: [ "director.>" ] } }, diff --git a/spec/nats_templates_spec.rb b/spec/nats_templates_spec.rb index b447de8f6aa..1280f4b10b3 100644 --- a/spec/nats_templates_spec.rb +++ b/spec/nats_templates_spec.rb @@ -90,7 +90,7 @@ user: "C=USA, O=Cloud Foundry, CN=default.director.bosh-internal" permissions: { publish: [ "agent.*", "hm.director.alert" ] - subscribe: [ "director.>" ] + subscribe: [ "director.>", "director.agent.disk.*" ] } }, { diff --git a/src/bosh-director/bin/bosh-director b/src/bosh-director/bin/bosh-director index 536a1bc16e4..60dfcd8a2b3 100755 --- a/src/bosh-director/bin/bosh-director +++ b/src/bosh-director/bin/bosh-director @@ -37,6 +37,8 @@ rack_app = Puma::Rack::Builder.app do route_configuration.controllers.each do |route, controller| map(route) { run controller } end + + Bosh::Director::DynamicDiskManager.new(Bosh::Director::Config.logger).setup_events end puma_configuration = Puma::Configuration.new do |user_config| diff --git a/src/bosh-director/db/migrations/20250623223600_add_dynamic_disks.rb b/src/bosh-director/db/migrations/20250623223600_add_dynamic_disks.rb new file mode 100644 index 00000000000..d64c82e219a --- /dev/null +++ b/src/bosh-director/db/migrations/20250623223600_add_dynamic_disks.rb @@ -0,0 +1,57 @@ +Sequel.migration do + up do + case adapter_scheme + when :postgres + create_table(:dynamic_disks) do + primary_key :id + foreign_key :deployment_id, :deployments, :null => false, :key => [:id] + foreign_key :vm_id, :vms, :key => [:id], :on_delete => :set_null + column :disk_cid, 'varchar(255)', :null => false + column :disk_hint_json, 'varchar(255)' + column :name, 'varchar(255)', :null => false + column :disk_pool_name, 'varchar(255)', :null => false + column :cpi, 'varchar(255)', :default => '' + column :size, 'integer', :null => false + column :metadata_json, 'text' + column :availability_zone, 'varchar(255)' + index [:name], :unique => true + end + when :mysql2 + create_table(:dynamic_disks) do + primary_key :id + foreign_key :deployment_id, :deployments, :null => false, :key => [:id] + foreign_key :vm_id, :vms, :key => [:id], :on_delete => :set_null + column :disk_cid, 'varchar(255)', :null => false + column :disk_hint_json, 'varchar(255)' + column :name, 'varchar(255)', :null => false + column :disk_pool_name, 'varchar(255)', :null => false + column :cpi, 'varchar(255)', :default => '' + column :size, 'integer', :null => false + column :metadata_json, 'longtext' + column :availability_zone, 'varchar(255)' + index [:name], :unique => true + end + when :sqlite + create_table(:dynamic_disks) do + primary_key :id + foreign_key :deployment_id, :deployments, :null => false, :key => [:id] + foreign_key :vm_id, :vms, :key => [:id], :on_delete => :set_null + column :disk_cid, 'varchar(255)', :null => false + column :disk_hint_json, 'varchar(255)' + column :name, 'varchar(255)', :null => false + column :disk_pool_name, 'varchar(255)', :null => false + column :cpi, 'varchar(255)', :default => '' + column :size, 'integer', :null => false + column :metadata_json, 'TEXT' + column :availability_zone, 'varchar(255)' + index [:name], :unique => true + end + else + raise "Unknown adapter_scheme: #{adapter_scheme}" + end + end + + down do + delete_table(:dynamic_disks) + end +end diff --git a/src/bosh-director/lib/bosh/director.rb b/src/bosh-director/lib/bosh/director.rb index 8696bb2d36a..48127252807 100644 --- a/src/bosh-director/lib/bosh/director.rb +++ b/src/bosh-director/lib/bosh/director.rb @@ -229,6 +229,11 @@ 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' require 'bosh/director/models/helpers/model_helper' @@ -279,6 +284,9 @@ module Bosh::Director require 'bosh/director/api/controllers/deployed_variables_controller' require 'bosh/director/api/route_configuration' +require 'bosh/director/api_nats/api' +require 'bosh/director/api_nats/dynamic_disk_controller' + require 'bosh/director/step_executor' require 'bosh/director/metrics_collector' require 'bosh/director/strip_deployments_middleware_collector' diff --git a/src/bosh-director/lib/bosh/director/api_nats/api.rb b/src/bosh-director/lib/bosh/director/api_nats/api.rb new file mode 100644 index 00000000000..67f5acb2b06 --- /dev/null +++ b/src/bosh-director/lib/bosh/director/api_nats/api.rb @@ -0,0 +1,78 @@ +module Bosh::Director + module ApiNats + class Api + extend ValidationHelper + + USERNAME = "bosh-agent".freeze + + def initialize(logger, nats_rpc) + @nats_rpc = nats_rpc + @logger = logger + end + + def setup_events + disk_controller = DynamicDiskController.new(@nats_rpc, @logger) + + @nats_rpc.nats.subscribe('director.agent.disk.create.*') do |payload, reply, _subject| + payload = parse_payload(payload) + disk_controller.handle_create_disk_request(reply, payload) + rescue => e + @nats_rpc.send_message(reply, { "error" => e.message }) + raise + end + + @nats_rpc.nats.subscribe('director.agent.disk.attach.*') do |payload, reply, subject| + payload = parse_payload(payload) + disk_controller.handle_attach_disk_request(parse_agent_id(subject), reply, payload) + rescue => e + @nats_rpc.send_message(reply, { "error" => e.message }) + raise + end + + @nats_rpc.nats.subscribe('director.agent.disk.provide.*') do |payload, reply, subject| + payload = parse_payload(payload) + disk_controller.handle_provide_disk_request(parse_agent_id(subject), reply, payload) + rescue => e + @nats_rpc.send_message(reply, { "error" => e.message }) + raise + end + + @nats_rpc.nats.subscribe('director.agent.disk.detach.*') do |payload, reply, subject| + payload = parse_payload(payload) + disk_controller.handle_detach_disk_request(parse_agent_id(subject), reply, payload) + rescue => e + @nats_rpc.send_message(reply, { "error" => e.message }) + raise + end + + @nats_rpc.nats.subscribe('director.agent.disk.delete.*') do |payload, reply, _subject| + payload = parse_payload(payload) + disk_controller.handle_delete_disk_request(reply, payload) + rescue => e + @nats_rpc.send_message(reply, { "error" => e.message }) + raise + end + end + + private + + def parse_agent_id(subject) + # subject: director.agent.disk.provide.agent_id + agent_id = subject.split('.', 5).last + raise 'Subject must include agent_id' if agent_id.empty? + return agent_id + end + + def parse_payload(payload) + case payload + when String + return JSON.parse(payload) + when Hash + return payload + else + raise "Payload must be a JSON string or hash" + end + end + end + end +end \ No newline at end of file diff --git a/src/bosh-director/lib/bosh/director/api_nats/dynamic_disk_controller.rb b/src/bosh-director/lib/bosh/director/api_nats/dynamic_disk_controller.rb new file mode 100644 index 00000000000..877d905e53f --- /dev/null +++ b/src/bosh-director/lib/bosh/director/api_nats/dynamic_disk_controller.rb @@ -0,0 +1,92 @@ +module Bosh::Director + module ApiNats + class DynamicDiskController + include ValidationHelper + + USERNAME = "bosh-agent".freeze + + def initialize(logger, nats_rpc) + @nats_rpc = nats_rpc + @logger = logger + end + + def handle_create_disk_request(reply, payload) + disk_name = safe_property(payload, "disk_name", class: String, min_length: 1) + disk_pool_name = safe_property(payload, "disk_pool_name", class: String, min_length: 1) + disk_size = safe_property(payload, "disk_size", class: Integer, min: 1) + metadata = safe_property(payload, "metadata", class: Hash, optional: true) + + JobQueue.new.enqueue( + USERNAME, + Jobs::DynamicDisk::CreateDynamicDisk, + 'create dynamic disk', + [reply, disk_name, disk_pool_name, disk_size, metadata] + ) + rescue => e + @nats_rpc.send_message(reply, { "error" => e.message }) + raise + end + + def handle_attach_disk_request(agent_id, reply, payload) + disk_name = safe_property(payload, "disk_name", class: String, min_length: 1) + + JobQueue.new.enqueue( + USERNAME, + Jobs::DynamicDisk::AttachDynamicDisk, + 'attach dynamic disk', + [agent_id, reply, disk_name] + ) + rescue => e + @nats_rpc.send_message(reply, { 'error' => e.message }) + raise + end + + def handle_provide_disk_request(agent_id, reply, payload) + disk_name = safe_property(payload, "disk_name", class: String, min_length: 1) + disk_pool_name = safe_property(payload, "disk_pool_name", class: String, min_length: 1) + disk_size = safe_property(payload, "disk_size", class: Integer, min: 1) + metadata = safe_property(payload, "metadata", class: Hash, optional: true) + + JobQueue.new.enqueue( + USERNAME, + Jobs::DynamicDisk::ProvideDynamicDisk, + 'provide dynamic disk', + [agent_id, reply, disk_name, disk_pool_name, disk_size, metadata] + ) + rescue => e + @nats_rpc.send_message(reply, { "error" => e.message }) + raise + end + + def handle_detach_disk_request(agent_id, reply, payload) + disk_name = safe_property(payload, "disk_name", class: String, min_length: 1) + + JobQueue.new.enqueue( + USERNAME, + Jobs::DynamicDisk::DetachDynamicDisk, + 'detach dynamic disk', + [agent_id, reply, disk_name] + ) + rescue => e + @nats_rpc.send_message(reply, { "error" => e.message }) + raise + end + + def handle_delete_disk_request(reply, payload) + disk_name = safe_property(payload, "disk_name", class: String, min_length: 1) + + JobQueue.new.enqueue( + USERNAME, + Jobs::DynamicDisk::DeleteDynamicDisk, + 'delete dynamic disk', + [reply, disk_name] + ) + rescue => e + # TODO is this right? Not sure what the best practice is here in rb + # Also is there a generic decorator like pattern for this? + @nats_rpc.send_message(reply, { "error" => e.message }) + raise + end + end + end +end \ No newline at end of file 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..d52583b01b6 --- /dev/null +++ b/src/bosh-director/lib/bosh/director/jobs/dynamic_disks/attach_dynamic_disk.rb @@ -0,0 +1,80 @@ +module Bosh::Director + module Jobs::DynamicDisks + class AttachDynamicDisk < Jobs::BaseJob + @queue = :normal + + def self.job_type + :provide_dynamic_disk + end + + def initialize(agent_id, reply, disk_name) + super() + @agent_id = agent_id + @reply = reply + @disk_name = disk_name + end + + def perform + # validate_message(@payload) + + # cloud_properties = find_disk_cloud_properties(@payload['disk_pool_name']) + + # cloud = Bosh::Director::CloudFactory.create.get(nil) + # unless cloud.has_disk(@payload['disk_name']) + # raise "Could not find disk #{@payload['disk_name']}" + # end + + # # TODO See if we should use the MetadataUpdater abstraction? It seems like overkill. + # if @payload['metadata'] != nil && cloud.respond_to?(:set_disk_metadata) + # # TODO implement this + # # metadata_updater_cloud = cloud_factory.get(@disk.cpi) + # # MetadataUpdater.build.update_dynamic_disk_metadata(metadata_updater_cloud, @disk, @tags) + # cloud.set_disk_metadata(disk_name, @payload['metadata']) + # end + + # # TODO record which vm the disk is attached to in the DB + # vm_cid = Models::Vm.find(agent_id: @agent_id).cid + # disk_hint = cloud.attach_disk(vm_cid, disk_name) + + # response = { + # 'error' => nil, + # 'disk_name' => disk_name, + # 'disk_hint' => disk_hint, + # } + # nats_client.send_message(@reply, response) + + # "attached disk '#{disk_name}' to '#{vm_cid}' in deployment '#{@payload['deployment']}'" + # rescue => e + # nats_client.send_message(@reply, { 'error' => e.message }) + # raise e + end + + private + + def nats_client + Config.nats_rpc + end + + def find_disk_cloud_properties(disk_pool_name) + configs = Models::Config.latest_set('cloud') + raise 'No cloud configs provided' if configs.empty? + + consolidated_configs = Bosh::Director::CloudConfig::CloudConfigsConsolidator.new(configs) + cloud_config_disk_type = DeploymentPlan::CloudManifestParser.new(logger).parse(consolidated_configs.raw_manifest).disk_type(disk_pool_name) + raise "Could not find disk pool by name `#{disk_pool_name}`" if cloud_config_disk_type.nil? + + cloud_config_disk_type.cloud_properties + end + + def validate_message(payload) + if payload['deployment'].nil? || payload['deployment'].empty? + raise 'Invalid request: `deployment` must be provided' + elsif payload['disk_name'].nil? || payload['disk_name'].empty? + raise 'Invalid request: `disk_name` must be provided' + elsif @reply.nil? || @reply.empty? + raise 'Invalid request: `disk_pool_name` must be provided' + end + end + end + end +end \ No newline at end of file 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..7d13259eb19 --- /dev/null +++ b/src/bosh-director/lib/bosh/director/jobs/dynamic_disks/create_dynamic_disk.rb @@ -0,0 +1,85 @@ +module Bosh::Director + module Jobs::DynamicDisks + class CreateDynamicDisk < Jobs::BaseJob + @queue = :normal + + def self.job_type + :create_dynamic_disk + end + + def initialize(agent_id, reply, disk_name, disk_pool_name, disk_size, metadata) + super() + @agent_id = agent_id + @reply = reply + + @disk_name = disk_name + @disk_pool_name = disk_pool_name + @disk_size = disk_size + @metadata = metadata + end + + def perform + # validate_message(@payload) + + # cloud_properties = find_disk_cloud_properties(@payload['disk_pool_name']) + + # cloud = Bosh::Director::CloudFactory.create.get(nil) + # if cloud.has_disk(@payload['disk_name']) + # raise "disk '#{}'" + # # TODO: save in database + # end + + # # TODO this still needed? + # if @payload['metadata'] != nil && cloud.respond_to?(:set_disk_metadata) + # cloud.set_disk_metadata(disk_name, @payload['metadata']) + # end + + # disk_name = cloud.create_disk(@payload['disk_size'], cloud_properties, nil) + # # TODO save disk name to db + + # response = { + # 'error' => nil, + # 'disk_name' => disk_name, + # 'disk_hint' => disk_hint, + # } + # nats_rpc.send_message(@reply, response) + + # "created disk '#{disk_name}' in deployment '#{@payload['deployment']}'" + # rescue => e + # nats_rpc.send_message(@reply, { 'error' => e.message }) + # raise e + end + + private + + def nats_client + Config.nats_rpc + end + + def find_disk_cloud_properties(disk_pool_name) + configs = Models::Config.latest_set('cloud') + raise 'No cloud configs provided' if configs.empty? + + consolidated_configs = Bosh::Director::CloudConfig::CloudConfigsConsolidator.new(configs) + cloud_config_disk_type = DeploymentPlan::CloudManifestParser.new(logger).parse(consolidated_configs.raw_manifest).disk_type(disk_pool_name) + raise "Could not find disk pool by name `#{disk_pool_name}`" if cloud_config_disk_type.nil? + + cloud_config_disk_type.cloud_properties + end + + def validate_message(payload) + if payload['deployment'].nil? || payload['deployment'].empty? + raise 'Invalid request: `deployment` must be provided' + elsif payload['disk_name'].nil? || payload['disk_name'].empty? + raise 'Invalid request: `disk_name` must be provided' + elsif payload['disk_size'].nil? || payload['disk_size'] == 0 + raise 'Invalid request: `disk_size` must be provided' + elsif payload['disk_pool_name'].nil? || payload['disk_pool_name'].empty? + raise 'Invalid request: `disk_pool_name` must be provided' + elsif @reply.nil? || @reply.empty? + raise 'Invalid request: `disk_pool_name` must be provided' + end + end + end + end +end \ No newline at end of file diff --git a/src/bosh-director/lib/bosh/director/jobs/dynamic_disks/delete_dynamic_disk.rb b/src/bosh-director/lib/bosh/director/jobs/dynamic_disks/delete_dynamic_disk.rb new file mode 100644 index 00000000000..29b46f06e3d --- /dev/null +++ b/src/bosh-director/lib/bosh/director/jobs/dynamic_disks/delete_dynamic_disk.rb @@ -0,0 +1,74 @@ +module Bosh::Director + module Jobs::DynamicDisks + class DeleteDynamicDisk < Jobs::BaseJob + @queue = :normal + + def self.job_type + :provide_dynamic_disk + end + + def initialize(nats_rpc, reply, payload) + super() + @nats_rpc = nats_rpc + @reply = reply + @payload = payload + end + + def perform + # validate_message(@payload) + + # cloud_properties = find_disk_cloud_properties(@payload['disk_pool_name']) + + # cloud = Bosh::Director::CloudFactory.create.get(nil) + # unless cloud.has_disk(@payload['disk_name']) + # # TODO raise or exit early + # raise "TODO" + # end + + # # TODO what to do when a disk is still attached? Maybe add a force param? + # # TODO Map name => cid + # cloud.delete_disk(disk_name) + + # response = { + # 'error' => nil, + # } + # @nats_rpc.send_message(@reply, response) + + # "deleted disk '#{disk_name}' in deployment '#{@payload['deployment']}'" + # rescue => e + # @nats_rpc.send_message(@reply, { 'error' => e.message }) + # raise e + end + + private + def nats_client + Config.nats_rpc + end + + def find_disk_cloud_properties(disk_pool_name) + configs = Models::Config.latest_set('cloud') + raise 'No cloud configs provided' if configs.empty? + + consolidated_configs = Bosh::Director::CloudConfig::CloudConfigsConsolidator.new(configs) + cloud_config_disk_type = DeploymentPlan::CloudManifestParser.new(logger).parse(consolidated_configs.raw_manifest).disk_type(disk_pool_name) + raise "Could not find disk pool by name `#{disk_pool_name}`" if cloud_config_disk_type.nil? + + cloud_config_disk_type.cloud_properties + end + + def validate_message(payload) + if payload['deployment'].nil? || payload['deployment'].empty? + raise 'Invalid request: `deployment` must be provided' + elsif payload['disk_name'].nil? || payload['disk_name'].empty? + raise 'Invalid request: `disk_name` must be provided' + elsif payload['disk_size'].nil? || payload['disk_size'] == 0 + raise 'Invalid request: `disk_size` must be provided' + elsif payload['disk_pool_name'].nil? || payload['disk_pool_name'].empty? + raise 'Invalid request: `disk_pool_name` must be provided' + elsif @reply.nil? || @reply.empty? + raise 'Invalid request: `disk_pool_name` must be provided' + end + end + end + end +end \ No newline at end of file diff --git a/src/bosh-director/lib/bosh/director/jobs/dynamic_disks/detach_dynamic_disk.rb b/src/bosh-director/lib/bosh/director/jobs/dynamic_disks/detach_dynamic_disk.rb new file mode 100644 index 00000000000..ebaa98322af --- /dev/null +++ b/src/bosh-director/lib/bosh/director/jobs/dynamic_disks/detach_dynamic_disk.rb @@ -0,0 +1,71 @@ +module Bosh::Director + module Jobs::DynamicDisks + class DetachDynamicDisk < Jobs::BaseJob + @queue = :normal + + def self.job_type + :provide_dynamic_disk + end + + def initialize(agent_id, reply, payload) + super() + @agent_id = agent_id + @reply = reply + @payload = payload + end + + def perform + # validate_message(@payload) + + # cloud = Bosh::Director::CloudFactory.create.get(nil) + # unless cloud.has_disk(@payload['disk_name']) + # raise "Could not find disk #{@payload['disk_name']}" + # end + + # # TODO find disk cid; this may need us to start saving disk state in the db + # vm_cid = Models::Vm.find(agent_id: @agent_id).cid + # cloud.detach_disk(vm_cid, @disk.disk_cid) + + # response = { + # 'error' => nil, + # } + # nats_rpc.send_message(@reply, response) + + # "detached disk '#{disk_name}' from '#{vm_cid}' in deployment '#{@payload['deployment']}'" + # rescue => e + # nats_rpc.send_message(@reply, { 'error' => e.message }) + # raise e + end + + private + def nats_client + Config.nats_rpc + end + + def find_disk_cloud_properties(disk_pool_name) + configs = Models::Config.latest_set('cloud') + raise 'No cloud configs provided' if configs.empty? + + consolidated_configs = Bosh::Director::CloudConfig::CloudConfigsConsolidator.new(configs) + cloud_config_disk_type = DeploymentPlan::CloudManifestParser.new(logger).parse(consolidated_configs.raw_manifest).disk_type(disk_pool_name) + raise "Could not find disk pool by name `#{disk_pool_name}`" if cloud_config_disk_type.nil? + + cloud_config_disk_type.cloud_properties + end + + def validate_message(payload) + if payload['deployment'].nil? || payload['deployment'].empty? + raise 'Invalid request: `deployment` must be provided' + elsif payload['disk_name'].nil? || payload['disk_name'].empty? + raise 'Invalid request: `disk_name` must be provided' + elsif payload['disk_size'].nil? || payload['disk_size'] == 0 + raise 'Invalid request: `disk_size` must be provided' + elsif payload['disk_pool_name'].nil? || payload['disk_pool_name'].empty? + raise 'Invalid request: `disk_pool_name` must be provided' + elsif @reply.nil? || @reply.empty? + raise 'Invalid request: `disk_pool_name` must be provided' + end + end + end + end +end \ No newline at end of file diff --git a/src/bosh-director/lib/bosh/director/jobs/dynamic_disks/provide_dynamic_disk.rb b/src/bosh-director/lib/bosh/director/jobs/dynamic_disks/provide_dynamic_disk.rb new file mode 100644 index 00000000000..e659ea85062 --- /dev/null +++ b/src/bosh-director/lib/bosh/director/jobs/dynamic_disks/provide_dynamic_disk.rb @@ -0,0 +1,79 @@ +module Bosh::Director + module Jobs + module DynamicDisks + class ProvideDynamicDisk < BaseJob + @queue = :normal + + def self.job_type + :provide_dynamic_disk + end + + def initialize(agent_id, reply, disk_name, disk_pool_name, disk_size, metadata) + super() + @agent_id = agent_id + @reply = reply + + @disk_name = disk_name + @disk_pool_name = disk_pool_name + @disk_size = disk_size + @metadata = metadata + end + + def perform + vm = Models::Vm.find(agent_id: @agent_id) + raise "vm for agent `#{@agent_id}` not found" unless vm + cloud_properties = find_disk_cloud_properties(vm.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, vm.cid) + disk_model = Models::DynamicDisk.create( + name: @disk_name, + disk_cid: disk_cid, + deployment_id: vm.instance.deployment.id, + size: @disk_size, + disk_pool_name: @disk_pool_name, + metadata: @metadata, + ) + end + + disk_hint = cloud.attach_disk(vm.cid, disk_model.disk_cid) + if @metadata != nil + MetadataUpdater.build.update_dynamic_disk_metadata(cloud, disk_model, @metadata) + end + + response = { + 'error' => nil, + 'disk_name' => @disk_name, + 'disk_hint' => disk_hint, + } + nats_rpc.send_message(@reply, response) + + "attached disk '#{@disk_name}' to '#{vm.cid}' in deployment '#{vm.instance.deployment.name}'" + rescue => e + nats_rpc.send_message(@reply, { 'error' => e.message }) + raise e + end + + private + + def nats_rpc + Config.nats_rpc + end + + def find_disk_cloud_properties(instance, disk_pool_name) + teams = instance.deployment.teams + configs = Models::Config.latest_set_for_teams('cloud', *teams) + raise 'No cloud configs provided' if configs.empty? + + consolidated_configs = Bosh::Director::CloudConfig::CloudConfigsConsolidator.new(configs) + cloud_config_disk_type = DeploymentPlan::CloudManifestParser.new(logger).parse(consolidated_configs.raw_manifest).disk_type(disk_pool_name) + raise "Could not find disk pool by name `#{disk_pool_name}`" if cloud_config_disk_type.nil? + + cloud_config_disk_type.cloud_properties + end + end + end + end +end \ No newline at end of file diff --git a/src/bosh-director/lib/bosh/director/metadata_updater.rb b/src/bosh-director/lib/bosh/director/metadata_updater.rb index 7b294be8a94..46207c675d6 100644 --- a/src/bosh-director/lib/bosh/director/metadata_updater.rb +++ b/src/bosh-director/lib/bosh/director/metadata_updater.rb @@ -42,5 +42,16 @@ def update_disk_metadata(cloud, disk, metadata) rescue Bosh::Clouds::NotImplemented => e @logger.debug(e.inspect) end + + def update_dynamic_disk_metadata(cloud, disk, metadata) + if cloud.respond_to?(:set_disk_metadata) + metadata = metadata.merge(@director_metadata) + metadata['deployment'] = disk.deployment.name + + cloud.set_disk_metadata(disk.disk_cid, metadata) + end + rescue Bosh::Clouds::NotImplemented => e + @logger.debug(e.inspect) + end end end diff --git a/src/bosh-director/lib/bosh/director/models.rb b/src/bosh-director/lib/bosh/director/models.rb index 97c392328dc..74e8d2b87a2 100644 --- a/src/bosh-director/lib/bosh/director/models.rb +++ b/src/bosh-director/lib/bosh/director/models.rb @@ -31,6 +31,7 @@ require 'bosh/director/models/orphaned_vm' require 'bosh/director/models/package' require 'bosh/director/models/persistent_disk' +require 'bosh/director/models/dynamic_disk' require 'bosh/director/models/release' require 'bosh/director/models/release_version' require 'bosh/director/models/rendered_templates_archive' diff --git a/src/bosh-director/lib/bosh/director/models/dynamic_disk.rb b/src/bosh-director/lib/bosh/director/models/dynamic_disk.rb new file mode 100644 index 00000000000..51670f6e562 --- /dev/null +++ b/src/bosh-director/lib/bosh/director/models/dynamic_disk.rb @@ -0,0 +1,23 @@ +module Bosh::Director::Models + class DynamicDisk < Sequel::Model(Bosh::Director::Config.db) + many_to_one :deployment + + def validate + validates_presence [:name, :disk_cid] + validates_unique [:name, :disk_cid] + end + + def metadata + result = self.metadata_json + result ? JSON.parse(result) : {} + end + + def metadata=(metadata) + self.metadata_json = JSON.generate(metadata) + end + + def to_s + "#{self.name}/#{self.disk_cid}" + end + end +end diff --git a/src/bosh-director/lib/bosh/director/tasks/db.rake b/src/bosh-director/lib/bosh/director/tasks/db.rake index 6f99afe9757..1d7f243e717 100644 --- a/src/bosh-director/lib/bosh/director/tasks/db.rake +++ b/src/bosh-director/lib/bosh/director/tasks/db.rake @@ -6,7 +6,7 @@ namespace :db do timestamp = Time.new.getutc.strftime('%Y%m%d%H%M%S') new_migration_path = "bosh-director/db/migrations/#{timestamp}_#{name}.rb" - new_migration_spec_path = "bosh-director/spec/unit/db/migrations/#{timestamp}_#{name}_spec.rb" + new_migration_spec_path = "bosh-director/spec/unit/bosh/director/db/migrations/#{timestamp}_#{name}_spec.rb" puts "Creating #{new_migration_spec_path}" File.write new_migration_spec_path, < options[:max_length] + raise ValidationViolatedMax, validation_message.invalid_max_length(options, property, result) + end + result end end @@ -59,6 +67,14 @@ def invalid_max(options, property, result) def invalid_min(options, property, result) "'#{property}' value (#{result.inspect}) should be greater than #{options[:min].inspect}" end + + def invalid_max_length(options, property, result) + "'#{property}' length (#{result.length.inspect}) should be less than #{options[:max_length].inspect}" + end + + def invalid_min_length(options, property, result) + "'#{property}' length (#{result.length.inspect}) should be greater than #{options[:min_length].inspect}" + end end class DefaultPropertyValidationMessage diff --git a/src/bosh-director/spec/factories.rb b/src/bosh-director/spec/factories.rb index 4cb0101ab5d..c8548946fd5 100644 --- a/src/bosh-director/spec/factories.rb +++ b/src/bosh-director/spec/factories.rb @@ -289,6 +289,13 @@ association :instance, factory: :models_instance, strategy: :create end + factory :models_dynamic_disk, class: Bosh::Director::Models::DynamicDisk do + sequence(:name) { |i| "dynamic-disk-name-#{i}" } + sequence(:disk_cid) { |i| "dynamic-disk-cid-#{i}" } + sequence(:size) { |i| 1024 + i } + association :deployment, factory: :models_deployment, strategy: :create + end + factory :models_release, class: Bosh::Director::Models::Release do sequence(:name) { |i| "release-#{i}" } end diff --git a/src/bosh-director/spec/unit/bosh/director/api_nats/dynamic_disk_controller_spec.rb b/src/bosh-director/spec/unit/bosh/director/api_nats/dynamic_disk_controller_spec.rb new file mode 100644 index 00000000000..d0e4f2561e9 --- /dev/null +++ b/src/bosh-director/spec/unit/bosh/director/api_nats/dynamic_disk_controller_spec.rb @@ -0,0 +1,330 @@ +require 'spec_helper' + +module Bosh::Director + module ApiNats + describe DynamicDiskController do + subject(:controller) { DynamicDiskController.new(per_spec_logger, nats_rpc) } + let(:nats_rpc) { instance_double('Bosh::Director::NatsRpc') } + let(:task) { instance_double('Bosh::Director::Models::Task', id: 1) } + let(:job_queue) { instance_double('Bosh::Director::JobQueue', enqueue: task) } + + before { allow(JobQueue).to receive(:new).and_return(job_queue) } + + let(:agent_id) { 'fake_agent_id' } + let(:reply) { 'inbox.fake' } + let(:disk_pool_name) { 'fake_disk_pool_name' } + let(:disk_name) { 'fake_disk_name' } + let(:disk_size) { 1000 } + let(:metadata) { { 'some-key' => 'some-value' } } + + describe 'handle_create_disk_request' do + let(:payload) do + { + 'disk_pool_name' => disk_pool_name, + 'disk_name' => disk_name, + 'disk_size' => disk_size, + 'metadata' => metadata + }.compact + end + + it 'enqueues a CreateDynamicDisk task' do + expect(job_queue).to receive(:enqueue).with( + 'bosh-agent', + Jobs::DynamicDisk::CreateDynamicDisk, + 'create dynamic disk', + [reply, disk_name, disk_pool_name, disk_size, metadata], + ).and_return(task) + + controller.handle_create_disk_request(reply, payload) + end + + context 'payload is invalid' do + context 'disk_pool_name is nil' do + let(:disk_pool_name) { nil } + + it 'raises an error' do + expect(nats_rpc).to receive(:send_message).with(reply, hash_including({ 'error' => a_string_matching("Required property 'disk_pool_name'") })) + expect { + controller.handle_create_disk_request(reply, payload) + }.to raise_error(ValidationMissingField) + end + end + + context 'disk_pool_name is empty' do + let(:disk_pool_name) { "" } + + it 'raises an error' do + expect(nats_rpc).to receive(:send_message).with(reply, hash_including({ 'error' => a_string_matching("'disk_pool_name' length") })) + expect { + controller.handle_create_disk_request(reply, payload) + }.to raise_error(ValidationViolatedMin) + end + end + + context 'disk_name is nil' do + let(:disk_name) { nil } + + it 'raises an error' do + expect(nats_rpc).to receive(:send_message).with(reply, hash_including({ 'error' => a_string_matching("Required property 'disk_name'") })) + expect { + controller.handle_create_disk_request(reply, payload) + }.to raise_error(ValidationMissingField) + end + end + + context 'disk_name is empty' do + let(:disk_name) { "" } + + it 'raises an error' do + expect(nats_rpc).to receive(:send_message).with(reply, hash_including({ 'error' => a_string_matching("'disk_name' length") })) + expect { + controller.handle_create_disk_request(reply, payload) + }.to raise_error(ValidationViolatedMin) + end + end + + context 'disk_size is empty' do + let(:disk_size) { nil } + + it 'raises an error' do + expect(nats_rpc).to receive(:send_message).with(reply, hash_including({ 'error' => a_string_matching("Required property 'disk_size'") })) + expect { + controller.handle_create_disk_request(reply, payload) + }.to raise_error(ValidationMissingField) + end + end + + context 'disk_size is 0' do + let(:disk_size) { 0 } + + it 'raises an error' do + expect(nats_rpc).to receive(:send_message).with(reply, hash_including({ 'error' => a_string_matching("'disk_size' value") })) + expect { + controller.handle_create_disk_request(reply, payload) + }.to raise_error(ValidationViolatedMin) + end + end + end + end + + describe 'handle_attach_disk_request' do + let(:payload) do + { + 'disk_name' => disk_name, + }.compact + end + + it 'enqueues a AttachDynamicDisk task' do + expect(job_queue).to receive(:enqueue).with( + 'bosh-agent', + Jobs::DynamicDisk::AttachDynamicDisk, + 'attach dynamic disk', + [agent_id, reply, disk_name], + ).and_return(task) + + controller.handle_attach_disk_request(agent_id, reply, payload) + end + + context 'payload is invalid' do + context 'disk_name is nil' do + let(:disk_name) { nil } + + it 'raises an error' do + expect(nats_rpc).to receive(:send_message).with(reply, hash_including({ 'error' => a_string_matching("Required property 'disk_name'") })) + expect { + controller.handle_attach_disk_request(agent_id, reply, payload) + }.to raise_error(ValidationMissingField) + end + end + + context 'disk_name is empty' do + let(:disk_name) { "" } + + it 'raises an error' do + expect(nats_rpc).to receive(:send_message).with(reply, hash_including({ 'error' => a_string_matching("'disk_name' length") })) + expect { + controller.handle_attach_disk_request(agent_id, reply, payload) + }.to raise_error(ValidationViolatedMin) + end + end + end + end + + describe 'handle_provide_disk_request' do + let(:payload) do + { + 'disk_pool_name' => disk_pool_name, + 'disk_name' => disk_name, + 'disk_size' => disk_size, + 'metadata' => metadata + }.compact + end + + it 'enqueues a ProvideDynamicDisk task' do + expect(job_queue).to receive(:enqueue).with( + 'bosh-agent', + Jobs::DynamicDisk::ProvideDynamicDisk, + 'provide dynamic disk', + [agent_id, reply, disk_name, disk_pool_name, disk_size, metadata], + ).and_return(task) + + controller.handle_provide_disk_request(agent_id, reply, payload) + end + + context 'payload is invalid' do + context 'disk_pool_name is nil' do + let(:disk_pool_name) { nil } + + it 'raises an error' do + expect(nats_rpc).to receive(:send_message).with(reply, hash_including({ 'error' => a_string_matching("Required property 'disk_pool_name'") })) + expect { + controller.handle_provide_disk_request(agent_id, reply, payload) + }.to raise_error(ValidationMissingField) + end + end + + context 'disk_pool_name is empty' do + let(:disk_pool_name) { "" } + + it 'raises an error' do + expect(nats_rpc).to receive(:send_message).with(reply, hash_including({ 'error' => a_string_matching("'disk_pool_name' length") })) + expect { + controller.handle_provide_disk_request(agent_id, reply, payload) + }.to raise_error(ValidationViolatedMin) + end + end + + context 'disk_name is nil' do + let(:disk_name) { nil } + + it 'raises an error' do + expect(nats_rpc).to receive(:send_message).with(reply, hash_including({ 'error' => a_string_matching("Required property 'disk_name'") })) + expect { + controller.handle_provide_disk_request(agent_id, reply, payload) + }.to raise_error(ValidationMissingField) + end + end + + context 'disk_name is empty' do + let(:disk_name) { "" } + + it 'raises an error' do + expect(nats_rpc).to receive(:send_message).with(reply, hash_including({ 'error' => a_string_matching("'disk_name' length") })) + expect { + controller.handle_provide_disk_request(agent_id, reply, payload) + }.to raise_error(ValidationViolatedMin) + end + end + + context 'disk_size is empty' do + let(:disk_size) { nil } + + it 'raises an error' do + expect(nats_rpc).to receive(:send_message).with(reply, hash_including({ 'error' => a_string_matching("Required property 'disk_size'") })) + expect { + controller.handle_provide_disk_request(agent_id, reply, payload) + }.to raise_error(ValidationMissingField) + end + end + + context 'disk_size is 0' do + let(:disk_size) { 0 } + + it 'raises an error' do + expect(nats_rpc).to receive(:send_message).with(reply, hash_including({ 'error' => a_string_matching("'disk_size' value") })) + expect { + controller.handle_provide_disk_request(agent_id, reply, payload) + }.to raise_error(ValidationViolatedMin) + end + end + end + end + + describe 'handle_detach_disk_request' do + let(:payload) do + { + 'disk_name' => disk_name, + }.compact + end + + it 'enqueues a DetachDynamicDisk task' do + expect(job_queue).to receive(:enqueue).with( + 'bosh-agent', + Jobs::DynamicDisk::DetachDynamicDisk, + 'detach dynamic disk', + [agent_id, reply, disk_name], + ).and_return(task) + + controller.handle_detach_disk_request(agent_id, reply, payload) + end + + context 'payload is invalid' do + context 'disk_name is nil' do + let(:disk_name) { nil } + + it 'raises an error' do + expect(nats_rpc).to receive(:send_message).with(reply, hash_including({ 'error' => a_string_matching("Required property 'disk_name'") })) + expect { + controller.handle_detach_disk_request(agent_id, reply, payload) + }.to raise_error(ValidationMissingField) + end + end + + context 'disk_name is empty' do + let(:disk_name) { "" } + + it 'raises an error' do + expect(nats_rpc).to receive(:send_message).with(reply, hash_including({ 'error' => a_string_matching("'disk_name' length") })) + expect { + controller.handle_detach_disk_request(agent_id, reply, payload) + }.to raise_error(ValidationViolatedMin) + end + end + end + end + + describe 'handle_delete_disk_request' do + let(:payload) do + { + 'disk_name' => disk_name, + }.compact + end + + it 'enqueues a DeleteDynamicDisk task' do + expect(job_queue).to receive(:enqueue).with( + 'bosh-agent', + Jobs::DynamicDisk::DeleteDynamicDisk, + 'delete dynamic disk', + [reply, disk_name], + ).and_return(task) + + controller.handle_delete_disk_request(reply, payload) + end + + context 'payload is invalid' do + context 'disk_name is nil' do + let(:disk_name) { nil } + + it 'raises an error' do + expect(nats_rpc).to receive(:send_message).with(reply, hash_including({ 'error' => a_string_matching("Required property 'disk_name'") })) + expect { + controller.handle_delete_disk_request(reply, payload) + }.to raise_error(ValidationMissingField) + end + end + + context 'disk_name is empty' do + let(:disk_name) { "" } + + it 'raises an error' do + expect(nats_rpc).to receive(:send_message).with(reply, hash_including({ 'error' => a_string_matching("'disk_name' length") })) + expect { + controller.handle_delete_disk_request(reply, payload) + }.to raise_error(ValidationViolatedMin) + end + end + end + end + end + end +end \ No newline at end of file diff --git a/src/bosh-director/spec/unit/bosh/director/db/migrations/20250623223600_add_dynamic_disks_spec.rb b/src/bosh-director/spec/unit/bosh/director/db/migrations/20250623223600_add_dynamic_disks_spec.rb new file mode 100644 index 00000000000..e1c4ef5b48d --- /dev/null +++ b/src/bosh-director/spec/unit/bosh/director/db/migrations/20250623223600_add_dynamic_disks_spec.rb @@ -0,0 +1,26 @@ +require 'db_spec_helper' + +module Bosh::Director + describe '20250623223600_add_dynamic_disks.rb' do + let(:db) { DBSpecHelper.db } + + before { DBSpecHelper.migrate_all_before(subject) } + + it 'creates dynamic_disks table' do + expect(db.table_exists?(:dynamic_disks)).to be_falsy + + DBSpecHelper.migrate(subject) + + expect(db.table_exists?(:dynamic_disks)).to be_truthy + expect(db[:dynamic_disks].columns).to include( + :id, + :deployment_id, + :disk_cid, + :name, + :disk_pool_name, + :size, + :metadata_json + ) + end + end +end diff --git a/src/bosh-director/spec/unit/bosh/director/db/migrations/verify_migration_digest_spec.rb b/src/bosh-director/spec/unit/bosh/director/db/migrations/verify_migration_digest_spec.rb index 3c2ae7a9ef6..d356c2aca30 100644 --- a/src/bosh-director/spec/unit/bosh/director/db/migrations/verify_migration_digest_spec.rb +++ b/src/bosh-director/spec/unit/bosh/director/db/migrations/verify_migration_digest_spec.rb @@ -24,6 +24,10 @@ module Bosh::Director filename: '20250618102610_migrate_ip_address_representation_from_integer_to_cidr_notation.rb', sha1: '83f219a46b0943b5e27355cb3792e639c7507707', }, + { + filename: '20250623223600_add_dynamic_disks.rb', + sha1: '25e2bb3567fbc1c853b078920989699bca95277d', + }, ] end diff --git a/src/bosh-director/spec/unit/bosh/director/jobs/dynamic_disks/provide_dynamic_disk_spec.rb b/src/bosh-director/spec/unit/bosh/director/jobs/dynamic_disks/provide_dynamic_disk_spec.rb new file mode 100644 index 00000000000..3f41bee5968 --- /dev/null +++ b/src/bosh-director/spec/unit/bosh/director/jobs/dynamic_disks/provide_dynamic_disk_spec.rb @@ -0,0 +1,198 @@ +require 'spec_helper' + +module Bosh::Director + describe Jobs::DynamicDisks::ProvideDynamicDisk do + let(:agent_id) { 'fake-agent-id' } + let(:reply) { 'inbox.fake' } + 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(:disk_hint) { 'fake-disk-hint' } + + let(:nats_rpc) { instance_double(Bosh::Director::NatsRpc) } + let(:cloud) { instance_double(Bosh::Clouds::ExternalCpi) } + let(:provide_dynamic_disk_job) { Jobs::DynamicDisks::ProvideDynamicDisk.new(agent_id, reply, disk_name, disk_pool_name, disk_size, metadata) } + let!(:vm) { FactoryBot.create(:models_vm, agent_id: agent_id, cid: 'fake-vm-cid') } + 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(Bosh::Director::CloudFactory, get: cloud) } + + before do + allow(Config).to receive(:nats_rpc).and_return(nats_rpc) + allow(Bosh::Director::Config).to receive(:name).and_return('fake-director-name') + allow(Bosh::Director::Config).to receive(:cloud_options).and_return('provider' => { 'path' => '/path/to/default/cpi' }) + allow(Bosh::Director::Config).to receive(:preferred_cpi_api_version).and_return(2) + allow(CloudFactory).to receive(:create).and_return(cloud_factory) + allow(cloud).to receive(:respond_to?).with(:set_disk_metadata).and_return(false) + end + + describe '#perform' do + context 'when disk exists' do + let!(:disk) do + FactoryBot.create( + :models_dynamic_disk, + name: disk_name, + disk_cid: disk_cid, + deployment: vm.instance.deployment, + ) + end + + it 'attaches the disk to VM' do + expect(cloud).to receive(:attach_disk).with('fake-vm-cid', disk_cid).and_return(disk_hint) + expect(nats_rpc).to receive(:send_message).with(reply, { + 'error' => nil, + 'disk_name' => disk_name, + 'disk_hint' => disk_hint, + }) + expect(provide_dynamic_disk_job.perform).to eq("attached disk '#{disk_name}' to '#{vm.cid}' in deployment '#{vm.instance.deployment.name}'") + end + end + + context 'when disk does not exist' do + it 'creates the disk and attaches it to VM' do + expect(cloud).to receive(:create_disk).with(disk_size, disk_cloud_properties, vm.cid).and_return(disk_cid) + expect(cloud).to receive(:respond_to?).with(:set_disk_metadata).and_return(false) + expect(cloud).to receive(:attach_disk).with('fake-vm-cid', disk_cid).and_return(disk_hint) + + expect(nats_rpc).to receive(:send_message).with(reply, { + 'error' => nil, + 'disk_name' => disk_name, + 'disk_hint' => disk_hint, + }) + expect(provide_dynamic_disk_job.perform).to eq("attached disk '#{disk_name}' to '#{vm.cid}' in deployment '#{vm.instance.deployment.name}'") + + model = Models::DynamicDisk.where(disk_cid: 'fake-disk-cid').first + expect(model.name).to eq(disk_name) + expect(model.size).to eq(disk_size) + expect(model.deployment_id).to eq(vm.instance.deployment.id) + expect(model.disk_pool_name).to eq(disk_pool_name) + expect(model.metadata).to eq(metadata) + end + end + + context 'when disk exists in database but not in the cloud' do + let!(:disk) do + FactoryBot.create( + :models_dynamic_disk, + name: disk_name, + disk_cid: disk_cid, + deployment: vm.instance.deployment, + ) + end + + it 'returns an error from attach_disk call' do + expect(cloud).to receive(:attach_disk).with('fake-vm-cid', disk_cid).and_raise('Disk not found') + + expect(nats_rpc).to receive(:send_message).with(reply, { + 'error' => "Disk not found" + }) + expect { provide_dynamic_disk_job.perform }.to raise_error("Disk not found") + end + end + + context 'when disk type can not 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 'responds with error' do + expect(nats_rpc).to receive(:send_message).with(reply, { + 'error' => "Could not find disk pool by name `fake-disk-pool-name`" + }) + expect { provide_dynamic_disk_job.perform }.to raise_error("Could not find disk pool by name `fake-disk-pool-name`") + end + end + + context 'when cpi supports set_disk_metadata' do + it 'sets disk metadata' do + expect(cloud).to receive(:create_disk).with(disk_size, disk_cloud_properties, vm.cid).and_return('fake-disk-cid') + expect(cloud).to receive(:respond_to?).with(:set_disk_metadata).and_return(true) + expect(cloud).to receive(:set_disk_metadata).with( + 'fake-disk-cid', + { + "deployment"=> vm.instance.deployment.name, + "director"=>'fake-director-name', + "fake-key"=>"fake-value" + } + ) + expect(cloud).to receive(:attach_disk).with('fake-vm-cid', 'fake-disk-cid').and_return(disk_hint) + + expect(nats_rpc).to receive(:send_message).with(reply, { + 'error' => nil, + 'disk_name' => disk_name, + 'disk_hint' => disk_hint, + }) + expect(provide_dynamic_disk_job.perform).to eq("attached disk '#{disk_name}' to '#{vm.cid}' in deployment '#{vm.instance.deployment.name}'") + end + end + + context 'when teams are set on the deployment' do + let(:team_1) { FactoryBot.create(:models_team, name: 'team-1') } + let(:team_2) { FactoryBot.create(:models_team, name: 'team-2') } + let(:other_team) { FactoryBot.create(:models_team, name: 'other_team') } + let(:other_disk_cloud_properties) { { 'other-disk-cloud-properties' => {} } } + let!(:latest_other_cloud_config) do + FactoryBot.create(:models_config_cloud, team_id: other_team.id, content: YAML.dump( + SharedSupport::DeploymentManifestHelper.simple_cloud_config.merge( + 'disk_types' => [ + { 'name' => disk_pool_name, + 'disk_size' => 1024, + 'cloud_properties' => other_disk_cloud_properties, + } + ] + ) + )) + end + + before do + vm.instance.deployment.update(teams: [team_1, team_2]) + cloud_config.update(team_id: team_1.id) + end + + it 'gets the disk cloud properties from the latest cloud config for those teams' do + expect(cloud).to receive(:create_disk).with(disk_size, disk_cloud_properties, vm.cid).and_return('fake-disk-cid') + expect(cloud).to receive(:attach_disk).with('fake-vm-cid', 'fake-disk-cid').and_return(disk_hint) + expect(nats_rpc).to receive(:send_message).with(reply, { + 'error' => nil, + 'disk_name' => disk_name, + 'disk_hint' => disk_hint, + }) + expect(provide_dynamic_disk_job.perform).to eq("attached disk '#{disk_name}' to '#{vm.cid}' in deployment '#{vm.instance.deployment.name}'") + end + end + + context 'when VM cannot be found' do + let!(:vm) { FactoryBot.create(:models_vm, agent_id: 'different-agent-id', cid: 'fake-vm-cid') } + + it 'responds with error' do + expect(nats_rpc).to receive(:send_message).with(reply, { + 'error' => "vm for agent `fake-agent-id` not found" + }) + expect { provide_dynamic_disk_job.perform }.to raise_error("vm for agent `fake-agent-id` not found") + end + end + end + end +end diff --git a/src/bosh-director/spec/unit/bosh/director/validation_helper_spec.rb b/src/bosh-director/spec/unit/bosh/director/validation_helper_spec.rb index af9ceed94fb..a38c9975780 100644 --- a/src/bosh-director/spec/unit/bosh/director/validation_helper_spec.rb +++ b/src/bosh-director/spec/unit/bosh/director/validation_helper_spec.rb @@ -179,6 +179,38 @@ def self.it_raises_unknown_object_error(hash, options) end end + describe 'when length is constrained' do + it 'should pass if strings do not have constraints' do + expect(obj.safe_property({ 'test' => '' }, 'test', class: String)).to eql('') + end + + it 'should pass if strings pass min constraints' do + expect(obj.safe_property({ 'test' => 'abc' }, 'test', min_length: 2)).to eql('abc') + end + + it 'should pass if strings pass max constraints' do + expect(obj.safe_property({ 'test' => 'abc' }, 'test', max_length: 4)).to eql('abc') + end + + it 'should fail if strings do not pass min constraints' do + expect do + expect(obj.safe_property({ 'test' => 'abc' }, 'test', min_length: 4)).to eql('abc') + end.to raise_error( + Bosh::Director::ValidationViolatedMin, + "'test' length (3) should be greater than 4", + ) + end + + it 'should fail if strings do not pass max constraints' do + expect do + expect(obj.safe_property({ 'test' => 'abc' }, 'test', max_length: 2)).to eql('abc') + end.to raise_error( + Bosh::Director::ValidationViolatedMax, + "'test' length (3) should be less than 2", + ) + end + end + describe 'when numeric constraints' do it 'should pass if numbers do not have constraints' do expect(obj.safe_property({ 'test' => 1 }, 'test', class: Numeric)).to eql(1) diff --git a/src/bosh-nats-sync/lib/nats_sync/nats_auth_config.rb b/src/bosh-nats-sync/lib/nats_sync/nats_auth_config.rb index 6f6007c6c2d..25586ce1eb9 100644 --- a/src/bosh-nats-sync/lib/nats_sync/nats_auth_config.rb +++ b/src/bosh-nats-sync/lib/nats_sync/nats_auth_config.rb @@ -12,7 +12,7 @@ def director_user { 'user' => @director_subject, 'permissions' => { - 'publish' => %w[agent.* hm.director.alert], + 'publish' => %w[agent.* agent.inbox.> hm.director.alert], 'subscribe' => ['director.>'], }, } @@ -37,8 +37,9 @@ def agent_user(agent_id, cn) "hm.agent.alert.#{agent_id}", "hm.agent.shutdown.#{agent_id}", "director.*.#{agent_id}.*", + "director.agent.disk.*.#{agent_id}", ], - "subscribe": ["agent.#{agent_id}"], + "subscribe": ["agent.#{agent_id}", "agent.inbox.#{agent_id}.>"], }, } end From 11250e8355b1f554f18185d24478cbf081d18c17 Mon Sep 17 00:00:00 2001 From: Maria Shaldybin Date: Tue, 24 Jun 2025 22:02:31 +0000 Subject: [PATCH 2/8] Add dynamic disks delete request handler * Handle NATS API request to delete dynamic disk * Send CPI request to delete disk * Remove disk from dynamic_disks table Signed-off-by: Nishad Mathur Co-authored-by: Nishad Mathur --- src/bosh-director/lib/bosh/director.rb | 2 - .../jobs/dynamic_disks/attach_dynamic_disk.rb | 80 ------------ .../jobs/dynamic_disks/create_dynamic_disk.rb | 85 ------------- .../jobs/dynamic_disks/delete_dynamic_disk.rb | 76 ++++-------- .../dynamic_disks/provide_dynamic_disk.rb | 114 ++++++++---------- .../lib/bosh/director/jobs/helpers.rb | 1 + .../jobs/helpers/dynamic_disk_helpers.rb | 21 ++++ .../20250623223600_add_dynamic_disks_spec.rb | 5 +- .../dynamic_disks/delete_dynamic_disk_spec.rb | 94 +++++++++++++++ .../provide_dynamic_disk_spec.rb | 14 ++- 10 files changed, 199 insertions(+), 293 deletions(-) delete mode 100644 src/bosh-director/lib/bosh/director/jobs/dynamic_disks/attach_dynamic_disk.rb delete mode 100644 src/bosh-director/lib/bosh/director/jobs/dynamic_disks/create_dynamic_disk.rb create mode 100644 src/bosh-director/lib/bosh/director/jobs/helpers/dynamic_disk_helpers.rb create mode 100644 src/bosh-director/spec/unit/bosh/director/jobs/dynamic_disks/delete_dynamic_disk_spec.rb diff --git a/src/bosh-director/lib/bosh/director.rb b/src/bosh-director/lib/bosh/director.rb index 48127252807..1add579580b 100644 --- a/src/bosh-director/lib/bosh/director.rb +++ b/src/bosh-director/lib/bosh/director.rb @@ -229,8 +229,6 @@ 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/jobs/dynamic_disks/attach_dynamic_disk.rb b/src/bosh-director/lib/bosh/director/jobs/dynamic_disks/attach_dynamic_disk.rb deleted file mode 100644 index d52583b01b6..00000000000 --- a/src/bosh-director/lib/bosh/director/jobs/dynamic_disks/attach_dynamic_disk.rb +++ /dev/null @@ -1,80 +0,0 @@ -module Bosh::Director - module Jobs::DynamicDisks - class AttachDynamicDisk < Jobs::BaseJob - @queue = :normal - - def self.job_type - :provide_dynamic_disk - end - - def initialize(agent_id, reply, disk_name) - super() - @agent_id = agent_id - @reply = reply - @disk_name = disk_name - end - - def perform - # validate_message(@payload) - - # cloud_properties = find_disk_cloud_properties(@payload['disk_pool_name']) - - # cloud = Bosh::Director::CloudFactory.create.get(nil) - # unless cloud.has_disk(@payload['disk_name']) - # raise "Could not find disk #{@payload['disk_name']}" - # end - - # # TODO See if we should use the MetadataUpdater abstraction? It seems like overkill. - # if @payload['metadata'] != nil && cloud.respond_to?(:set_disk_metadata) - # # TODO implement this - # # metadata_updater_cloud = cloud_factory.get(@disk.cpi) - # # MetadataUpdater.build.update_dynamic_disk_metadata(metadata_updater_cloud, @disk, @tags) - # cloud.set_disk_metadata(disk_name, @payload['metadata']) - # end - - # # TODO record which vm the disk is attached to in the DB - # vm_cid = Models::Vm.find(agent_id: @agent_id).cid - # disk_hint = cloud.attach_disk(vm_cid, disk_name) - - # response = { - # 'error' => nil, - # 'disk_name' => disk_name, - # 'disk_hint' => disk_hint, - # } - # nats_client.send_message(@reply, response) - - # "attached disk '#{disk_name}' to '#{vm_cid}' in deployment '#{@payload['deployment']}'" - # rescue => e - # nats_client.send_message(@reply, { 'error' => e.message }) - # raise e - end - - private - - def nats_client - Config.nats_rpc - end - - def find_disk_cloud_properties(disk_pool_name) - configs = Models::Config.latest_set('cloud') - raise 'No cloud configs provided' if configs.empty? - - consolidated_configs = Bosh::Director::CloudConfig::CloudConfigsConsolidator.new(configs) - cloud_config_disk_type = DeploymentPlan::CloudManifestParser.new(logger).parse(consolidated_configs.raw_manifest).disk_type(disk_pool_name) - raise "Could not find disk pool by name `#{disk_pool_name}`" if cloud_config_disk_type.nil? - - cloud_config_disk_type.cloud_properties - end - - def validate_message(payload) - if payload['deployment'].nil? || payload['deployment'].empty? - raise 'Invalid request: `deployment` must be provided' - elsif payload['disk_name'].nil? || payload['disk_name'].empty? - raise 'Invalid request: `disk_name` must be provided' - elsif @reply.nil? || @reply.empty? - raise 'Invalid request: `disk_pool_name` must be provided' - end - end - end - end -end \ No newline at end of file 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 deleted file mode 100644 index 7d13259eb19..00000000000 --- a/src/bosh-director/lib/bosh/director/jobs/dynamic_disks/create_dynamic_disk.rb +++ /dev/null @@ -1,85 +0,0 @@ -module Bosh::Director - module Jobs::DynamicDisks - class CreateDynamicDisk < Jobs::BaseJob - @queue = :normal - - def self.job_type - :create_dynamic_disk - end - - def initialize(agent_id, reply, disk_name, disk_pool_name, disk_size, metadata) - super() - @agent_id = agent_id - @reply = reply - - @disk_name = disk_name - @disk_pool_name = disk_pool_name - @disk_size = disk_size - @metadata = metadata - end - - def perform - # validate_message(@payload) - - # cloud_properties = find_disk_cloud_properties(@payload['disk_pool_name']) - - # cloud = Bosh::Director::CloudFactory.create.get(nil) - # if cloud.has_disk(@payload['disk_name']) - # raise "disk '#{}'" - # # TODO: save in database - # end - - # # TODO this still needed? - # if @payload['metadata'] != nil && cloud.respond_to?(:set_disk_metadata) - # cloud.set_disk_metadata(disk_name, @payload['metadata']) - # end - - # disk_name = cloud.create_disk(@payload['disk_size'], cloud_properties, nil) - # # TODO save disk name to db - - # response = { - # 'error' => nil, - # 'disk_name' => disk_name, - # 'disk_hint' => disk_hint, - # } - # nats_rpc.send_message(@reply, response) - - # "created disk '#{disk_name}' in deployment '#{@payload['deployment']}'" - # rescue => e - # nats_rpc.send_message(@reply, { 'error' => e.message }) - # raise e - end - - private - - def nats_client - Config.nats_rpc - end - - def find_disk_cloud_properties(disk_pool_name) - configs = Models::Config.latest_set('cloud') - raise 'No cloud configs provided' if configs.empty? - - consolidated_configs = Bosh::Director::CloudConfig::CloudConfigsConsolidator.new(configs) - cloud_config_disk_type = DeploymentPlan::CloudManifestParser.new(logger).parse(consolidated_configs.raw_manifest).disk_type(disk_pool_name) - raise "Could not find disk pool by name `#{disk_pool_name}`" if cloud_config_disk_type.nil? - - cloud_config_disk_type.cloud_properties - end - - def validate_message(payload) - if payload['deployment'].nil? || payload['deployment'].empty? - raise 'Invalid request: `deployment` must be provided' - elsif payload['disk_name'].nil? || payload['disk_name'].empty? - raise 'Invalid request: `disk_name` must be provided' - elsif payload['disk_size'].nil? || payload['disk_size'] == 0 - raise 'Invalid request: `disk_size` must be provided' - elsif payload['disk_pool_name'].nil? || payload['disk_pool_name'].empty? - raise 'Invalid request: `disk_pool_name` must be provided' - elsif @reply.nil? || @reply.empty? - raise 'Invalid request: `disk_pool_name` must be provided' - end - end - end - end -end \ No newline at end of file diff --git a/src/bosh-director/lib/bosh/director/jobs/dynamic_disks/delete_dynamic_disk.rb b/src/bosh-director/lib/bosh/director/jobs/dynamic_disks/delete_dynamic_disk.rb index 29b46f06e3d..73dafde3e90 100644 --- a/src/bosh-director/lib/bosh/director/jobs/dynamic_disks/delete_dynamic_disk.rb +++ b/src/bosh-director/lib/bosh/director/jobs/dynamic_disks/delete_dynamic_disk.rb @@ -1,73 +1,39 @@ module Bosh::Director module Jobs::DynamicDisks class DeleteDynamicDisk < Jobs::BaseJob + include Jobs::Helpers::DynamicDiskHelpers + @queue = :normal def self.job_type - :provide_dynamic_disk + :delet_dynamic_disk end - def initialize(nats_rpc, reply, payload) + def initialize(reply, disk_name) super() - @nats_rpc = nats_rpc @reply = reply - @payload = payload + @disk_name = disk_name end def perform - # validate_message(@payload) - - # cloud_properties = find_disk_cloud_properties(@payload['disk_pool_name']) - - # cloud = Bosh::Director::CloudFactory.create.get(nil) - # unless cloud.has_disk(@payload['disk_name']) - # # TODO raise or exit early - # raise "TODO" - # end - - # # TODO what to do when a disk is still attached? Maybe add a force param? - # # TODO Map name => cid - # cloud.delete_disk(disk_name) - - # response = { - # 'error' => nil, - # } - # @nats_rpc.send_message(@reply, response) - - # "deleted disk '#{disk_name}' in deployment '#{@payload['deployment']}'" - # rescue => e - # @nats_rpc.send_message(@reply, { 'error' => e.message }) - # raise e - end - - private - def nats_client - Config.nats_rpc - end - - def find_disk_cloud_properties(disk_pool_name) - configs = Models::Config.latest_set('cloud') - raise 'No cloud configs provided' if configs.empty? - - consolidated_configs = Bosh::Director::CloudConfig::CloudConfigsConsolidator.new(configs) - cloud_config_disk_type = DeploymentPlan::CloudManifestParser.new(logger).parse(consolidated_configs.raw_manifest).disk_type(disk_pool_name) - raise "Could not find disk pool by name `#{disk_pool_name}`" if cloud_config_disk_type.nil? + disk_model = Models::DynamicDisk.find(name: @disk_name) + if disk_model != nil + cloud = Bosh::Director::CloudFactory.create.get(disk_model.cpi) + if cloud.has_disk(disk_model.disk_cid) + cloud.delete_disk(disk_model.disk_cid) + end + Models::DynamicDisk.where(id: disk_model.id).delete + end - cloud_config_disk_type.cloud_properties - end + response = { + 'error' => nil, + } + nats_rpc.send_message(@reply, response) - def validate_message(payload) - if payload['deployment'].nil? || payload['deployment'].empty? - raise 'Invalid request: `deployment` must be provided' - elsif payload['disk_name'].nil? || payload['disk_name'].empty? - raise 'Invalid request: `disk_name` must be provided' - elsif payload['disk_size'].nil? || payload['disk_size'] == 0 - raise 'Invalid request: `disk_size` must be provided' - elsif payload['disk_pool_name'].nil? || payload['disk_pool_name'].empty? - raise 'Invalid request: `disk_pool_name` must be provided' - elsif @reply.nil? || @reply.empty? - raise 'Invalid request: `disk_pool_name` must be provided' - end + "deleted disk '#{@disk_name}'" + rescue => e + nats_rpc.send_message(@reply, { 'error' => e.message }) + raise e end end end diff --git a/src/bosh-director/lib/bosh/director/jobs/dynamic_disks/provide_dynamic_disk.rb b/src/bosh-director/lib/bosh/director/jobs/dynamic_disks/provide_dynamic_disk.rb index e659ea85062..4d144125089 100644 --- a/src/bosh-director/lib/bosh/director/jobs/dynamic_disks/provide_dynamic_disk.rb +++ b/src/bosh-director/lib/bosh/director/jobs/dynamic_disks/provide_dynamic_disk.rb @@ -1,78 +1,62 @@ module Bosh::Director - module Jobs - module DynamicDisks - class ProvideDynamicDisk < BaseJob - @queue = :normal + module Jobs::DynamicDisks + class ProvideDynamicDisk < Jobs::BaseJob + include Jobs::Helpers::DynamicDiskHelpers - def self.job_type - :provide_dynamic_disk - end - - def initialize(agent_id, reply, disk_name, disk_pool_name, disk_size, metadata) - super() - @agent_id = agent_id - @reply = reply - - @disk_name = disk_name - @disk_pool_name = disk_pool_name - @disk_size = disk_size - @metadata = metadata - end - - def perform - vm = Models::Vm.find(agent_id: @agent_id) - raise "vm for agent `#{@agent_id}` not found" unless vm - cloud_properties = find_disk_cloud_properties(vm.instance, @disk_pool_name) - cloud = Bosh::Director::CloudFactory.create.get(vm.cpi) - disk_model = Models::DynamicDisk.find(name: @disk_name) + @queue = :normal - if disk_model == nil - 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: vm.instance.deployment.id, - size: @disk_size, - disk_pool_name: @disk_pool_name, - metadata: @metadata, - ) - end + def self.job_type + :provide_dynamic_disk + end - disk_hint = cloud.attach_disk(vm.cid, disk_model.disk_cid) - if @metadata != nil - MetadataUpdater.build.update_dynamic_disk_metadata(cloud, disk_model, @metadata) - end + def initialize(agent_id, reply, disk_name, disk_pool_name, disk_size, metadata) + super() + @agent_id = agent_id + @reply = reply - response = { - 'error' => nil, - 'disk_name' => @disk_name, - 'disk_hint' => disk_hint, - } - nats_rpc.send_message(@reply, response) + @disk_name = disk_name + @disk_pool_name = disk_pool_name + @disk_size = disk_size + @metadata = metadata + end - "attached disk '#{@disk_name}' to '#{vm.cid}' in deployment '#{vm.instance.deployment.name}'" - rescue => e - nats_rpc.send_message(@reply, { 'error' => e.message }) - raise e + def perform + vm = Models::Vm.find(agent_id: @agent_id) + raise "vm for agent `#{@agent_id}` not found" unless vm + cloud_properties = find_disk_cloud_properties(vm.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, vm.cid) + disk_model = Models::DynamicDisk.create( + name: @disk_name, + disk_cid: disk_cid, + deployment_id: vm.instance.deployment.id, + availability_zone: vm.instance.availability_zone, + size: @disk_size, + disk_pool_name: @disk_pool_name, + cpi: vm.cpi, + metadata: @metadata, + ) end - private - - def nats_rpc - Config.nats_rpc + disk_hint = cloud.attach_disk(vm.cid, disk_model.disk_cid) + if @metadata != nil + MetadataUpdater.build.update_dynamic_disk_metadata(cloud, disk_model, @metadata) end - def find_disk_cloud_properties(instance, disk_pool_name) - teams = instance.deployment.teams - configs = Models::Config.latest_set_for_teams('cloud', *teams) - raise 'No cloud configs provided' if configs.empty? - - consolidated_configs = Bosh::Director::CloudConfig::CloudConfigsConsolidator.new(configs) - cloud_config_disk_type = DeploymentPlan::CloudManifestParser.new(logger).parse(consolidated_configs.raw_manifest).disk_type(disk_pool_name) - raise "Could not find disk pool by name `#{disk_pool_name}`" if cloud_config_disk_type.nil? - - cloud_config_disk_type.cloud_properties - end + response = { + 'error' => nil, + 'disk_name' => @disk_name, + 'disk_hint' => disk_hint, + } + nats_rpc.send_message(@reply, response) + + "attached disk '#{@disk_name}' to '#{vm.cid}' in deployment '#{vm.instance.deployment.name}'" + rescue => e + nats_rpc.send_message(@reply, { 'error' => e.message }) + raise e end end end diff --git a/src/bosh-director/lib/bosh/director/jobs/helpers.rb b/src/bosh-director/lib/bosh/director/jobs/helpers.rb index 744faeb028c..3402db7e263 100644 --- a/src/bosh-director/lib/bosh/director/jobs/helpers.rb +++ b/src/bosh-director/lib/bosh/director/jobs/helpers.rb @@ -8,3 +8,4 @@ require 'bosh/director/jobs/helpers/release_version_deleter' require 'bosh/director/jobs/helpers/release_deleter' require 'bosh/director/jobs/helpers/name_version_release_deleter' +require 'bosh/director/jobs/helpers/dynamic_disk_helpers' 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 new file mode 100644 index 00000000000..78ca7421b36 --- /dev/null +++ b/src/bosh-director/lib/bosh/director/jobs/helpers/dynamic_disk_helpers.rb @@ -0,0 +1,21 @@ +module Bosh::Director + module Jobs::Helpers + module DynamicDiskHelpers + def find_disk_cloud_properties(instance, disk_pool_name) + teams = instance.deployment.teams + configs = Models::Config.latest_set_for_teams('cloud', *teams) + raise 'No cloud configs provided' if configs.empty? + + consolidated_configs = Bosh::Director::CloudConfig::CloudConfigsConsolidator.new(configs) + cloud_config_disk_type = DeploymentPlan::CloudManifestParser.new(logger).parse(consolidated_configs.raw_manifest).disk_type(disk_pool_name) + raise "Could not find disk pool by name `#{disk_pool_name}`" if cloud_config_disk_type.nil? + + cloud_config_disk_type.cloud_properties + end + + def nats_rpc + Config.nats_rpc + end + end + end +end \ No newline at end of file diff --git a/src/bosh-director/spec/unit/bosh/director/db/migrations/20250623223600_add_dynamic_disks_spec.rb b/src/bosh-director/spec/unit/bosh/director/db/migrations/20250623223600_add_dynamic_disks_spec.rb index e1c4ef5b48d..c7e19513afa 100644 --- a/src/bosh-director/spec/unit/bosh/director/db/migrations/20250623223600_add_dynamic_disks_spec.rb +++ b/src/bosh-director/spec/unit/bosh/director/db/migrations/20250623223600_add_dynamic_disks_spec.rb @@ -15,11 +15,14 @@ module Bosh::Director expect(db[:dynamic_disks].columns).to include( :id, :deployment_id, + :vm_id, :disk_cid, :name, :disk_pool_name, + :cpi, :size, - :metadata_json + :metadata_json, + :availability_zone ) end end diff --git a/src/bosh-director/spec/unit/bosh/director/jobs/dynamic_disks/delete_dynamic_disk_spec.rb b/src/bosh-director/spec/unit/bosh/director/jobs/dynamic_disks/delete_dynamic_disk_spec.rb new file mode 100644 index 00000000000..ecc40192a6d --- /dev/null +++ b/src/bosh-director/spec/unit/bosh/director/jobs/dynamic_disks/delete_dynamic_disk_spec.rb @@ -0,0 +1,94 @@ +require 'spec_helper' + +module Bosh::Director + describe Jobs::DynamicDisks::DeleteDynamicDisk do + let(:agent_id) { 'fake-agent-id' } + let(:reply) { 'inbox.fake' } + let(:disk_name) { 'fake-disk-name' } + let(:disk_cid) { 'fake-disk-cid' } + let(:disk_pool_name) { 'fake-disk-pool-name' } + + let(:nats_rpc) { instance_double(Bosh::Director::NatsRpc) } + let(:cloud) { instance_double(Bosh::Clouds::ExternalCpi) } + let(:delete_dynamic_disk_job) { Jobs::DynamicDisks::DeleteDynamicDisk.new(reply, disk_name) } + let!(:vm) { FactoryBot.create(:models_vm, agent_id: agent_id, cid: 'fake-vm-cid') } + let(:cloud_factory) { instance_double(Bosh::Director::CloudFactory, get: cloud) } + + before do + allow(Config).to receive(:nats_rpc).and_return(nats_rpc) + allow(Bosh::Director::Config).to receive(:name).and_return('fake-director-name') + allow(Bosh::Director::Config).to receive(:cloud_options).and_return('provider' => { 'path' => '/path/to/default/cpi' }) + allow(Bosh::Director::Config).to receive(:preferred_cpi_api_version).and_return(2) + allow(CloudFactory).to receive(:create).and_return(cloud_factory) + allow(cloud).to receive(:has_disk).and_return(false) + end + + describe '#perform' do + context 'when disk exists in database but not in the cloud' 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 'deletes the disk' do + expect(nats_rpc).to receive(:send_message).with(reply, { + 'error' => nil + }) + expect(delete_dynamic_disk_job.perform).to eq("deleted disk '#{disk_name}'") + expect(Models::DynamicDisk.where(disk_cid: disk_cid).count).to eq(0) + end + end + + context 'when disk exists in database and in the cloud' 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 + + before do + allow(cloud).to receive(:has_disk).and_return(true) + end + + it 'deletes the disk' do + expect(cloud).to receive(:delete_disk).with(disk_cid) + expect(nats_rpc).to receive(:send_message).with(reply, { + 'error' => nil + }) + expect(delete_dynamic_disk_job.perform).to eq("deleted disk '#{disk_name}'") + expect(Models::DynamicDisk.where(disk_cid: disk_cid).count).to eq(0) + end + + context 'when deleting disk returns an error' do + it 'returns an error from delete_disk call' do + expect(cloud).to receive(:delete_disk).with(disk_cid).and_raise('some-error') + + expect(nats_rpc).to receive(:send_message).with(reply, { + 'error' => "some-error" + }) + expect { delete_dynamic_disk_job.perform }.to raise_error("some-error") + end + end + end + + context 'when disk does not exist' do + it 'does not delete disk and returns no error' do + expect(cloud).not_to receive(:delete_disk).with(disk_cid) + expect(nats_rpc).to receive(:send_message).with(reply, { + 'error' => nil + }) + expect(delete_dynamic_disk_job.perform).to eq("deleted disk '#{disk_name}'") + end + end + end + end +end diff --git a/src/bosh-director/spec/unit/bosh/director/jobs/dynamic_disks/provide_dynamic_disk_spec.rb b/src/bosh-director/spec/unit/bosh/director/jobs/dynamic_disks/provide_dynamic_disk_spec.rb index 3f41bee5968..f91e846deba 100644 --- a/src/bosh-director/spec/unit/bosh/director/jobs/dynamic_disks/provide_dynamic_disk_spec.rb +++ b/src/bosh-director/spec/unit/bosh/director/jobs/dynamic_disks/provide_dynamic_disk_spec.rb @@ -43,10 +43,11 @@ module Bosh::Director context 'when disk exists' do let!(:disk) do FactoryBot.create( - :models_dynamic_disk, + :models_dynamic_disk, name: disk_name, disk_cid: disk_cid, deployment: vm.instance.deployment, + disk_pool_name: disk_pool_name, ) end @@ -79,6 +80,8 @@ module Bosh::Director expect(model.size).to eq(disk_size) expect(model.deployment_id).to eq(vm.instance.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(vm.instance.availability_zone) expect(model.metadata).to eq(metadata) end end @@ -90,7 +93,8 @@ module Bosh::Director name: disk_name, disk_cid: disk_cid, deployment: vm.instance.deployment, - ) + disk_pool_name: disk_pool_name, + ) end it 'returns an error from attach_disk call' do @@ -132,9 +136,9 @@ module Bosh::Director expect(cloud).to receive(:set_disk_metadata).with( 'fake-disk-cid', { - "deployment"=> vm.instance.deployment.name, - "director"=>'fake-director-name', - "fake-key"=>"fake-value" + "deployment" => vm.instance.deployment.name, + "director" => 'fake-director-name', + "fake-key" => "fake-value" } ) expect(cloud).to receive(:attach_disk).with('fake-vm-cid', 'fake-disk-cid').and_return(disk_hint) From 4732fb848d65af0ce59f2359807710706dc4e11d Mon Sep 17 00:00:00 2001 From: Nishad Mathur Date: Thu, 21 Aug 2025 21:21:47 +0000 Subject: [PATCH 3/8] Add dynamic disks detach request handler * Handle NATS request to detach dynamic disk * Send CPI request to detach disk * Update disk in the database Signed-off-by: Maria Shaldybin Co-authored-by: Maria Shaldybin --- src/bosh-director/bin/bosh-director | 2 +- src/bosh-director/lib/bosh/director.rb | 2 +- .../lib/bosh/director/api_nats/api.rb | 78 ---------- .../lib/bosh/director/api_nats/api_nats.rb | 55 +++++++ .../api_nats/dynamic_disk_controller.rb | 52 +------ .../jobs/dynamic_disks/delete_dynamic_disk.rb | 14 +- .../jobs/dynamic_disks/detach_dynamic_disk.rb | 75 ++++----- .../dynamic_disks/provide_dynamic_disk.rb | 14 +- .../lib/bosh/director/models/dynamic_disk.rb | 1 + .../api_nats/dynamic_disk_controller_spec.rb | 147 +----------------- .../dynamic_disks/delete_dynamic_disk_spec.rb | 10 +- .../dynamic_disks/detach_dynamic_disk_spec.rb | 114 ++++++++++++++ .../provide_dynamic_disk_spec.rb | 32 ++-- 13 files changed, 253 insertions(+), 343 deletions(-) delete mode 100644 src/bosh-director/lib/bosh/director/api_nats/api.rb create mode 100644 src/bosh-director/lib/bosh/director/api_nats/api_nats.rb create mode 100644 src/bosh-director/spec/unit/bosh/director/jobs/dynamic_disks/detach_dynamic_disk_spec.rb diff --git a/src/bosh-director/bin/bosh-director b/src/bosh-director/bin/bosh-director index 60dfcd8a2b3..d5c3378ccf9 100755 --- a/src/bosh-director/bin/bosh-director +++ b/src/bosh-director/bin/bosh-director @@ -38,7 +38,7 @@ rack_app = Puma::Rack::Builder.app do map(route) { run controller } end - Bosh::Director::DynamicDiskManager.new(Bosh::Director::Config.logger).setup_events + Bosh::Director::ApiNats.setup_events end puma_configuration = Puma::Configuration.new do |user_config| diff --git a/src/bosh-director/lib/bosh/director.rb b/src/bosh-director/lib/bosh/director.rb index 1add579580b..2c1aac58258 100644 --- a/src/bosh-director/lib/bosh/director.rb +++ b/src/bosh-director/lib/bosh/director.rb @@ -282,7 +282,7 @@ module Bosh::Director require 'bosh/director/api/controllers/deployed_variables_controller' require 'bosh/director/api/route_configuration' -require 'bosh/director/api_nats/api' +require 'bosh/director/api_nats/api_nats' require 'bosh/director/api_nats/dynamic_disk_controller' require 'bosh/director/step_executor' diff --git a/src/bosh-director/lib/bosh/director/api_nats/api.rb b/src/bosh-director/lib/bosh/director/api_nats/api.rb deleted file mode 100644 index 67f5acb2b06..00000000000 --- a/src/bosh-director/lib/bosh/director/api_nats/api.rb +++ /dev/null @@ -1,78 +0,0 @@ -module Bosh::Director - module ApiNats - class Api - extend ValidationHelper - - USERNAME = "bosh-agent".freeze - - def initialize(logger, nats_rpc) - @nats_rpc = nats_rpc - @logger = logger - end - - def setup_events - disk_controller = DynamicDiskController.new(@nats_rpc, @logger) - - @nats_rpc.nats.subscribe('director.agent.disk.create.*') do |payload, reply, _subject| - payload = parse_payload(payload) - disk_controller.handle_create_disk_request(reply, payload) - rescue => e - @nats_rpc.send_message(reply, { "error" => e.message }) - raise - end - - @nats_rpc.nats.subscribe('director.agent.disk.attach.*') do |payload, reply, subject| - payload = parse_payload(payload) - disk_controller.handle_attach_disk_request(parse_agent_id(subject), reply, payload) - rescue => e - @nats_rpc.send_message(reply, { "error" => e.message }) - raise - end - - @nats_rpc.nats.subscribe('director.agent.disk.provide.*') do |payload, reply, subject| - payload = parse_payload(payload) - disk_controller.handle_provide_disk_request(parse_agent_id(subject), reply, payload) - rescue => e - @nats_rpc.send_message(reply, { "error" => e.message }) - raise - end - - @nats_rpc.nats.subscribe('director.agent.disk.detach.*') do |payload, reply, subject| - payload = parse_payload(payload) - disk_controller.handle_detach_disk_request(parse_agent_id(subject), reply, payload) - rescue => e - @nats_rpc.send_message(reply, { "error" => e.message }) - raise - end - - @nats_rpc.nats.subscribe('director.agent.disk.delete.*') do |payload, reply, _subject| - payload = parse_payload(payload) - disk_controller.handle_delete_disk_request(reply, payload) - rescue => e - @nats_rpc.send_message(reply, { "error" => e.message }) - raise - end - end - - private - - def parse_agent_id(subject) - # subject: director.agent.disk.provide.agent_id - agent_id = subject.split('.', 5).last - raise 'Subject must include agent_id' if agent_id.empty? - return agent_id - end - - def parse_payload(payload) - case payload - when String - return JSON.parse(payload) - when Hash - return payload - else - raise "Payload must be a JSON string or hash" - end - end - end - end -end \ No newline at end of file diff --git a/src/bosh-director/lib/bosh/director/api_nats/api_nats.rb b/src/bosh-director/lib/bosh/director/api_nats/api_nats.rb new file mode 100644 index 00000000000..beb06c1e327 --- /dev/null +++ b/src/bosh-director/lib/bosh/director/api_nats/api_nats.rb @@ -0,0 +1,55 @@ +module Bosh::Director + module ApiNats + extend ValidationHelper + + USERNAME = "bosh-agent".freeze + + def setup_events + disk_controller = DynamicDiskController.new + + Config.nats_rpc.nats.subscribe('director.agent.disk.provide.*') do |payload, reply, subject| + payload = parse_payload(payload) + disk_controller.handle_provide_disk_request(parse_agent_id(subject), reply, payload) + rescue => e + Config.nats_rpc.send_message(reply, { "error" => e.message }) + raise + end + + Config.nats_rpc.nats.subscribe('director.agent.disk.detach.*') do |payload, reply, subject| + payload = parse_payload(payload) + disk_controller.handle_detach_disk_request(parse_agent_id(subject), reply, payload) + rescue => e + Config.nats_rpc.send_message(reply, { "error" => e.message }) + raise + end + + Config.nats_rpc.nats.subscribe('director.agent.disk.delete.*') do |payload, reply, _subject| + payload = parse_payload(payload) + disk_controller.handle_delete_disk_request(reply, payload) + rescue => e + Config.nats_rpc.send_message(reply, { "error" => e.message }) + raise + end + end + + private + + def parse_agent_id(subject) + # subject: director.agent.disk.provide.agent_id + agent_id = subject.split('.', 5).last + raise 'Subject must include agent_id' if agent_id.empty? + return agent_id + end + + def parse_payload(payload) + case payload + when String + return JSON.parse(payload) + when Hash + return payload + else + raise "Payload must be a JSON string or hash" + end + end + end +end \ No newline at end of file diff --git a/src/bosh-director/lib/bosh/director/api_nats/dynamic_disk_controller.rb b/src/bosh-director/lib/bosh/director/api_nats/dynamic_disk_controller.rb index 877d905e53f..4911f68b123 100644 --- a/src/bosh-director/lib/bosh/director/api_nats/dynamic_disk_controller.rb +++ b/src/bosh-director/lib/bosh/director/api_nats/dynamic_disk_controller.rb @@ -5,42 +5,6 @@ class DynamicDiskController USERNAME = "bosh-agent".freeze - def initialize(logger, nats_rpc) - @nats_rpc = nats_rpc - @logger = logger - end - - def handle_create_disk_request(reply, payload) - disk_name = safe_property(payload, "disk_name", class: String, min_length: 1) - disk_pool_name = safe_property(payload, "disk_pool_name", class: String, min_length: 1) - disk_size = safe_property(payload, "disk_size", class: Integer, min: 1) - metadata = safe_property(payload, "metadata", class: Hash, optional: true) - - JobQueue.new.enqueue( - USERNAME, - Jobs::DynamicDisk::CreateDynamicDisk, - 'create dynamic disk', - [reply, disk_name, disk_pool_name, disk_size, metadata] - ) - rescue => e - @nats_rpc.send_message(reply, { "error" => e.message }) - raise - end - - def handle_attach_disk_request(agent_id, reply, payload) - disk_name = safe_property(payload, "disk_name", class: String, min_length: 1) - - JobQueue.new.enqueue( - USERNAME, - Jobs::DynamicDisk::AttachDynamicDisk, - 'attach dynamic disk', - [agent_id, reply, disk_name] - ) - rescue => e - @nats_rpc.send_message(reply, { 'error' => e.message }) - raise - end - def handle_provide_disk_request(agent_id, reply, payload) disk_name = safe_property(payload, "disk_name", class: String, min_length: 1) disk_pool_name = safe_property(payload, "disk_pool_name", class: String, min_length: 1) @@ -49,12 +13,12 @@ def handle_provide_disk_request(agent_id, reply, payload) JobQueue.new.enqueue( USERNAME, - Jobs::DynamicDisk::ProvideDynamicDisk, + Jobs::DynamicDisks::ProvideDynamicDisk, 'provide dynamic disk', [agent_id, reply, disk_name, disk_pool_name, disk_size, metadata] ) rescue => e - @nats_rpc.send_message(reply, { "error" => e.message }) + Config.nats_rpc.send_message(reply, { "error" => e.message }) raise end @@ -63,12 +27,12 @@ def handle_detach_disk_request(agent_id, reply, payload) JobQueue.new.enqueue( USERNAME, - Jobs::DynamicDisk::DetachDynamicDisk, + Jobs::DynamicDisks::DetachDynamicDisk, 'detach dynamic disk', - [agent_id, reply, disk_name] + [reply, disk_name] ) rescue => e - @nats_rpc.send_message(reply, { "error" => e.message }) + Config.nats_rpc.send_message(reply, { "error" => e.message }) raise end @@ -77,14 +41,12 @@ def handle_delete_disk_request(reply, payload) JobQueue.new.enqueue( USERNAME, - Jobs::DynamicDisk::DeleteDynamicDisk, + Jobs::DynamicDisks::DeleteDynamicDisk, 'delete dynamic disk', [reply, disk_name] ) rescue => e - # TODO is this right? Not sure what the best practice is here in rb - # Also is there a generic decorator like pattern for this? - @nats_rpc.send_message(reply, { "error" => e.message }) + Config.nats_rpc.send_message(reply, { "error" => e.message }) raise end end diff --git a/src/bosh-director/lib/bosh/director/jobs/dynamic_disks/delete_dynamic_disk.rb b/src/bosh-director/lib/bosh/director/jobs/dynamic_disks/delete_dynamic_disk.rb index 73dafde3e90..cace5a8a0bb 100644 --- a/src/bosh-director/lib/bosh/director/jobs/dynamic_disks/delete_dynamic_disk.rb +++ b/src/bosh-director/lib/bosh/director/jobs/dynamic_disks/delete_dynamic_disk.rb @@ -6,7 +6,7 @@ class DeleteDynamicDisk < Jobs::BaseJob @queue = :normal def self.job_type - :delet_dynamic_disk + :delete_dynamic_disk end def initialize(reply, disk_name) @@ -17,7 +17,7 @@ def initialize(reply, disk_name) def perform disk_model = Models::DynamicDisk.find(name: @disk_name) - if disk_model != nil + unless disk_model.nil? cloud = Bosh::Director::CloudFactory.create.get(disk_model.cpi) if cloud.has_disk(disk_model.disk_cid) cloud.delete_disk(disk_model.disk_cid) @@ -25,12 +25,14 @@ def perform Models::DynamicDisk.where(id: disk_model.id).delete end - response = { - 'error' => nil, - } + response = { 'error' => nil } nats_rpc.send_message(@reply, response) - "deleted disk '#{@disk_name}'" + if disk_model.nil? + "disk with name `#{@disk_name}` was already deleted" + else + "deleted disk `#{disk_model.disk_cid}`" + end rescue => e nats_rpc.send_message(@reply, { 'error' => e.message }) raise e diff --git a/src/bosh-director/lib/bosh/director/jobs/dynamic_disks/detach_dynamic_disk.rb b/src/bosh-director/lib/bosh/director/jobs/dynamic_disks/detach_dynamic_disk.rb index ebaa98322af..c3a0966dde1 100644 --- a/src/bosh-director/lib/bosh/director/jobs/dynamic_disks/detach_dynamic_disk.rb +++ b/src/bosh-director/lib/bosh/director/jobs/dynamic_disks/detach_dynamic_disk.rb @@ -1,70 +1,47 @@ module Bosh::Director module Jobs::DynamicDisks class DetachDynamicDisk < Jobs::BaseJob + include Jobs::Helpers::DynamicDiskHelpers + @queue = :normal def self.job_type - :provide_dynamic_disk + :detach_dynamic_disk end - def initialize(agent_id, reply, payload) + def initialize(reply, disk_name) super() - @agent_id = agent_id @reply = reply - @payload = payload + @disk_name = disk_name end def perform - # validate_message(@payload) - - # cloud = Bosh::Director::CloudFactory.create.get(nil) - # unless cloud.has_disk(@payload['disk_name']) - # raise "Could not find disk #{@payload['disk_name']}" - # end - - # # TODO find disk cid; this may need us to start saving disk state in the db - # vm_cid = Models::Vm.find(agent_id: @agent_id).cid - # cloud.detach_disk(vm_cid, @disk.disk_cid) - - # response = { - # 'error' => nil, - # } - # nats_rpc.send_message(@reply, response) - - # "detached disk '#{disk_name}' from '#{vm_cid}' in deployment '#{@payload['deployment']}'" - # rescue => e - # nats_rpc.send_message(@reply, { 'error' => e.message }) - # raise e - end - - private - def nats_client - Config.nats_rpc - end + disk_model = Models::DynamicDisk.find(name: @disk_name) + raise "disk `#{@disk_name}` can not be found in the database" if disk_model.nil? - def find_disk_cloud_properties(disk_pool_name) - configs = Models::Config.latest_set('cloud') - raise 'No cloud configs provided' if configs.empty? + cloud = Bosh::Director::CloudFactory.create.get(disk_model.cpi) - consolidated_configs = Bosh::Director::CloudConfig::CloudConfigsConsolidator.new(configs) - cloud_config_disk_type = DeploymentPlan::CloudManifestParser.new(logger).parse(consolidated_configs.raw_manifest).disk_type(disk_pool_name) - raise "Could not find disk pool by name `#{disk_pool_name}`" if cloud_config_disk_type.nil? + raise "disk `#{@disk_name}` can not be found in the cloud" unless cloud.has_disk(disk_model.disk_cid) - cloud_config_disk_type.cloud_properties - end + vm = disk_model.vm + unless vm.nil? + cloud.detach_disk(disk_model.vm.cid, disk_model.disk_cid) + disk_model.update(vm_id: nil) + end - def validate_message(payload) - if payload['deployment'].nil? || payload['deployment'].empty? - raise 'Invalid request: `deployment` must be provided' - elsif payload['disk_name'].nil? || payload['disk_name'].empty? - raise 'Invalid request: `disk_name` must be provided' - elsif payload['disk_size'].nil? || payload['disk_size'] == 0 - raise 'Invalid request: `disk_size` must be provided' - elsif payload['disk_pool_name'].nil? || payload['disk_pool_name'].empty? - raise 'Invalid request: `disk_pool_name` must be provided' - elsif @reply.nil? || @reply.empty? - raise 'Invalid request: `disk_pool_name` must be provided' + nats_rpc.send_message(@reply, { 'error' => nil }) + if vm.nil? + "disk `#{disk_model.disk_cid}` was already detached" + else + "detached disk `#{disk_model.disk_cid}` from vm `#{vm.cid}`" end + rescue Bosh::Clouds::DiskNotAttached + disk_model.update(vm_id: nil) + nats_rpc.send_message(@reply, { 'error' => nil }) + "disk `#{disk_model.disk_cid}` was already detached" + rescue => e + nats_rpc.send_message(@reply, { 'error' => e.message }) + raise e end end end diff --git a/src/bosh-director/lib/bosh/director/jobs/dynamic_disks/provide_dynamic_disk.rb b/src/bosh-director/lib/bosh/director/jobs/dynamic_disks/provide_dynamic_disk.rb index 4d144125089..64a07f91eca 100644 --- a/src/bosh-director/lib/bosh/director/jobs/dynamic_disks/provide_dynamic_disk.rb +++ b/src/bosh-director/lib/bosh/director/jobs/dynamic_disks/provide_dynamic_disk.rb @@ -22,18 +22,18 @@ def initialize(agent_id, reply, disk_name, disk_pool_name, disk_size, metadata) def perform vm = Models::Vm.find(agent_id: @agent_id) - raise "vm for agent `#{@agent_id}` not found" unless vm + raise "vm for agent `#{@agent_id}` not found" if vm.nil? + cloud_properties = find_disk_cloud_properties(vm.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_model = Models::DynamicDisk.find(name: @disk_name) + if disk_model.nil? 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: vm.instance.deployment.id, - availability_zone: vm.instance.availability_zone, size: @disk_size, disk_pool_name: @disk_pool_name, cpi: vm.cpi, @@ -42,7 +42,9 @@ def perform end disk_hint = cloud.attach_disk(vm.cid, disk_model.disk_cid) - if @metadata != nil + disk_model.update(vm_id: vm.id, availability_zone: vm.instance.availability_zone) + + unless @metadata.nil? MetadataUpdater.build.update_dynamic_disk_metadata(cloud, disk_model, @metadata) end @@ -53,7 +55,7 @@ def perform } nats_rpc.send_message(@reply, response) - "attached disk '#{@disk_name}' to '#{vm.cid}' in deployment '#{vm.instance.deployment.name}'" + "attached disk `#{@disk_name}` to `#{vm.cid}` in deployment `#{vm.instance.deployment.name}`" rescue => e nats_rpc.send_message(@reply, { 'error' => e.message }) raise e diff --git a/src/bosh-director/lib/bosh/director/models/dynamic_disk.rb b/src/bosh-director/lib/bosh/director/models/dynamic_disk.rb index 51670f6e562..aeccc779f6e 100644 --- a/src/bosh-director/lib/bosh/director/models/dynamic_disk.rb +++ b/src/bosh-director/lib/bosh/director/models/dynamic_disk.rb @@ -1,6 +1,7 @@ module Bosh::Director::Models class DynamicDisk < Sequel::Model(Bosh::Director::Config.db) many_to_one :deployment + many_to_one :vm def validate validates_presence [:name, :disk_cid] diff --git a/src/bosh-director/spec/unit/bosh/director/api_nats/dynamic_disk_controller_spec.rb b/src/bosh-director/spec/unit/bosh/director/api_nats/dynamic_disk_controller_spec.rb index d0e4f2561e9..a35fd8fc086 100644 --- a/src/bosh-director/spec/unit/bosh/director/api_nats/dynamic_disk_controller_spec.rb +++ b/src/bosh-director/spec/unit/bosh/director/api_nats/dynamic_disk_controller_spec.rb @@ -3,13 +3,10 @@ module Bosh::Director module ApiNats describe DynamicDiskController do - subject(:controller) { DynamicDiskController.new(per_spec_logger, nats_rpc) } + subject(:controller) { DynamicDiskController.new } let(:nats_rpc) { instance_double('Bosh::Director::NatsRpc') } let(:task) { instance_double('Bosh::Director::Models::Task', id: 1) } let(:job_queue) { instance_double('Bosh::Director::JobQueue', enqueue: task) } - - before { allow(JobQueue).to receive(:new).and_return(job_queue) } - let(:agent_id) { 'fake_agent_id' } let(:reply) { 'inbox.fake' } let(:disk_pool_name) { 'fake_disk_pool_name' } @@ -17,137 +14,9 @@ module ApiNats let(:disk_size) { 1000 } let(:metadata) { { 'some-key' => 'some-value' } } - describe 'handle_create_disk_request' do - let(:payload) do - { - 'disk_pool_name' => disk_pool_name, - 'disk_name' => disk_name, - 'disk_size' => disk_size, - 'metadata' => metadata - }.compact - end - - it 'enqueues a CreateDynamicDisk task' do - expect(job_queue).to receive(:enqueue).with( - 'bosh-agent', - Jobs::DynamicDisk::CreateDynamicDisk, - 'create dynamic disk', - [reply, disk_name, disk_pool_name, disk_size, metadata], - ).and_return(task) - - controller.handle_create_disk_request(reply, payload) - end - - context 'payload is invalid' do - context 'disk_pool_name is nil' do - let(:disk_pool_name) { nil } - - it 'raises an error' do - expect(nats_rpc).to receive(:send_message).with(reply, hash_including({ 'error' => a_string_matching("Required property 'disk_pool_name'") })) - expect { - controller.handle_create_disk_request(reply, payload) - }.to raise_error(ValidationMissingField) - end - end - - context 'disk_pool_name is empty' do - let(:disk_pool_name) { "" } - - it 'raises an error' do - expect(nats_rpc).to receive(:send_message).with(reply, hash_including({ 'error' => a_string_matching("'disk_pool_name' length") })) - expect { - controller.handle_create_disk_request(reply, payload) - }.to raise_error(ValidationViolatedMin) - end - end - - context 'disk_name is nil' do - let(:disk_name) { nil } - - it 'raises an error' do - expect(nats_rpc).to receive(:send_message).with(reply, hash_including({ 'error' => a_string_matching("Required property 'disk_name'") })) - expect { - controller.handle_create_disk_request(reply, payload) - }.to raise_error(ValidationMissingField) - end - end - - context 'disk_name is empty' do - let(:disk_name) { "" } - - it 'raises an error' do - expect(nats_rpc).to receive(:send_message).with(reply, hash_including({ 'error' => a_string_matching("'disk_name' length") })) - expect { - controller.handle_create_disk_request(reply, payload) - }.to raise_error(ValidationViolatedMin) - end - end - - context 'disk_size is empty' do - let(:disk_size) { nil } - - it 'raises an error' do - expect(nats_rpc).to receive(:send_message).with(reply, hash_including({ 'error' => a_string_matching("Required property 'disk_size'") })) - expect { - controller.handle_create_disk_request(reply, payload) - }.to raise_error(ValidationMissingField) - end - end - - context 'disk_size is 0' do - let(:disk_size) { 0 } - - it 'raises an error' do - expect(nats_rpc).to receive(:send_message).with(reply, hash_including({ 'error' => a_string_matching("'disk_size' value") })) - expect { - controller.handle_create_disk_request(reply, payload) - }.to raise_error(ValidationViolatedMin) - end - end - end - end - - describe 'handle_attach_disk_request' do - let(:payload) do - { - 'disk_name' => disk_name, - }.compact - end - - it 'enqueues a AttachDynamicDisk task' do - expect(job_queue).to receive(:enqueue).with( - 'bosh-agent', - Jobs::DynamicDisk::AttachDynamicDisk, - 'attach dynamic disk', - [agent_id, reply, disk_name], - ).and_return(task) - - controller.handle_attach_disk_request(agent_id, reply, payload) - end - - context 'payload is invalid' do - context 'disk_name is nil' do - let(:disk_name) { nil } - - it 'raises an error' do - expect(nats_rpc).to receive(:send_message).with(reply, hash_including({ 'error' => a_string_matching("Required property 'disk_name'") })) - expect { - controller.handle_attach_disk_request(agent_id, reply, payload) - }.to raise_error(ValidationMissingField) - end - end - - context 'disk_name is empty' do - let(:disk_name) { "" } - - it 'raises an error' do - expect(nats_rpc).to receive(:send_message).with(reply, hash_including({ 'error' => a_string_matching("'disk_name' length") })) - expect { - controller.handle_attach_disk_request(agent_id, reply, payload) - }.to raise_error(ValidationViolatedMin) - end - end - end + before do + allow(Config).to receive(:nats_rpc).and_return(nats_rpc) + allow(JobQueue).to receive(:new).and_return(job_queue) end describe 'handle_provide_disk_request' do @@ -163,7 +32,7 @@ module ApiNats it 'enqueues a ProvideDynamicDisk task' do expect(job_queue).to receive(:enqueue).with( 'bosh-agent', - Jobs::DynamicDisk::ProvideDynamicDisk, + Jobs::DynamicDisks::ProvideDynamicDisk, 'provide dynamic disk', [agent_id, reply, disk_name, disk_pool_name, disk_size, metadata], ).and_return(task) @@ -250,9 +119,9 @@ module ApiNats it 'enqueues a DetachDynamicDisk task' do expect(job_queue).to receive(:enqueue).with( 'bosh-agent', - Jobs::DynamicDisk::DetachDynamicDisk, + Jobs::DynamicDisks::DetachDynamicDisk, 'detach dynamic disk', - [agent_id, reply, disk_name], + [reply, disk_name], ).and_return(task) controller.handle_detach_disk_request(agent_id, reply, payload) @@ -293,7 +162,7 @@ module ApiNats it 'enqueues a DeleteDynamicDisk task' do expect(job_queue).to receive(:enqueue).with( 'bosh-agent', - Jobs::DynamicDisk::DeleteDynamicDisk, + Jobs::DynamicDisks::DeleteDynamicDisk, 'delete dynamic disk', [reply, disk_name], ).and_return(task) diff --git a/src/bosh-director/spec/unit/bosh/director/jobs/dynamic_disks/delete_dynamic_disk_spec.rb b/src/bosh-director/spec/unit/bosh/director/jobs/dynamic_disks/delete_dynamic_disk_spec.rb index ecc40192a6d..9ad19c344ed 100644 --- a/src/bosh-director/spec/unit/bosh/director/jobs/dynamic_disks/delete_dynamic_disk_spec.rb +++ b/src/bosh-director/spec/unit/bosh/director/jobs/dynamic_disks/delete_dynamic_disk_spec.rb @@ -39,7 +39,7 @@ module Bosh::Director expect(nats_rpc).to receive(:send_message).with(reply, { 'error' => nil }) - expect(delete_dynamic_disk_job.perform).to eq("deleted disk '#{disk_name}'") + expect(delete_dynamic_disk_job.perform).to eq("deleted disk `#{disk_cid}`") expect(Models::DynamicDisk.where(disk_cid: disk_cid).count).to eq(0) end end @@ -64,7 +64,7 @@ module Bosh::Director expect(nats_rpc).to receive(:send_message).with(reply, { 'error' => nil }) - expect(delete_dynamic_disk_job.perform).to eq("deleted disk '#{disk_name}'") + expect(delete_dynamic_disk_job.perform).to eq("deleted disk `#{disk_cid}`") expect(Models::DynamicDisk.where(disk_cid: disk_cid).count).to eq(0) end @@ -73,9 +73,9 @@ module Bosh::Director expect(cloud).to receive(:delete_disk).with(disk_cid).and_raise('some-error') expect(nats_rpc).to receive(:send_message).with(reply, { - 'error' => "some-error" + 'error' => 'some-error' }) - expect { delete_dynamic_disk_job.perform }.to raise_error("some-error") + expect { delete_dynamic_disk_job.perform }.to raise_error('some-error') end end end @@ -86,7 +86,7 @@ module Bosh::Director expect(nats_rpc).to receive(:send_message).with(reply, { 'error' => nil }) - expect(delete_dynamic_disk_job.perform).to eq("deleted disk '#{disk_name}'") + expect(delete_dynamic_disk_job.perform).to eq("disk with name `#{disk_name}` was already deleted") end end end diff --git a/src/bosh-director/spec/unit/bosh/director/jobs/dynamic_disks/detach_dynamic_disk_spec.rb b/src/bosh-director/spec/unit/bosh/director/jobs/dynamic_disks/detach_dynamic_disk_spec.rb new file mode 100644 index 00000000000..607180fc457 --- /dev/null +++ b/src/bosh-director/spec/unit/bosh/director/jobs/dynamic_disks/detach_dynamic_disk_spec.rb @@ -0,0 +1,114 @@ +require 'spec_helper' + +module Bosh::Director + describe Jobs::DynamicDisks::DetachDynamicDisk do + let(:agent_id) { 'fake-agent-id' } + let(:reply) { 'inbox.fake' } + let(:disk_name) { 'fake-disk-name' } + let(:disk_cid) { 'fake-disk-cid' } + let(:disk_pool_name) { 'fake-disk-pool-name' } + + let(:nats_rpc) { instance_double(Bosh::Director::NatsRpc) } + let(:cloud) { instance_double(Bosh::Clouds::ExternalCpi) } + let(:detach_dynamic_disk_job) { Jobs::DynamicDisks::DetachDynamicDisk.new(reply, disk_name) } + let!(:vm) { FactoryBot.create(:models_vm, agent_id: agent_id, cid: 'fake-vm-cid') } + let(:cloud_factory) { instance_double(Bosh::Director::CloudFactory, get: cloud) } + + before do + allow(Config).to receive(:nats_rpc).and_return(nats_rpc) + allow(Bosh::Director::Config).to receive(:name).and_return('fake-director-name') + allow(Bosh::Director::Config).to receive(:cloud_options).and_return('provider' => { 'path' => '/path/to/default/cpi' }) + allow(Bosh::Director::Config).to receive(:preferred_cpi_api_version).and_return(2) + allow(CloudFactory).to receive(:create).and_return(cloud_factory) + allow(cloud).to receive(:has_disk).and_return(true) + end + + describe '#perform' do + context 'when disk exists in database' do + context 'when disk has no vm assigned' 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 'does not detach the disk' do + expect(nats_rpc).to receive(:send_message).with(reply, { + 'error' => nil + }) + expect(detach_dynamic_disk_job.perform).to eq("disk `#{disk_cid}` was already detached") + end + end + + context 'when disk has vm assigned' do + let!(:disk) do + FactoryBot.create( + :models_dynamic_disk, + name: disk_name, + disk_cid: disk_cid, + deployment: vm.instance.deployment, + vm: vm, + disk_pool_name: disk_pool_name + ) + end + + it 'detaches the disk' do + expect(cloud).to receive(:detach_disk).with(vm.cid, disk_cid) + expect(nats_rpc).to receive(:send_message).with(reply, { + 'error' => nil + }) + expect(detach_dynamic_disk_job.perform).to eq("detached disk `#{disk_cid}` from vm `#{vm.cid}`") + expect(Models::DynamicDisk.find(disk_cid: disk_cid).vm_id).to be_nil + end + + context 'when disk is already detached' do + it 'returns an error' do + expect(cloud).to receive(:detach_disk).with(vm.cid, disk_cid).and_raise(Bosh::Clouds::DiskNotAttached.new(false)) + expect(nats_rpc).to receive(:send_message).with(reply, { + 'error' => nil + }) + expect(detach_dynamic_disk_job.perform).to eq("disk `#{disk_cid}` was already detached") + expect(Models::DynamicDisk.find(disk_cid: disk_cid).vm_id).to be_nil + end + end + end + end + + context 'when disk does not exist in database' do + it 'returns an error' do + expect(nats_rpc).to receive(:send_message).with(reply, { + 'error' => "disk `#{disk_name}` can not be found in the database" + }) + expect { detach_dynamic_disk_job.perform }.to raise_error("disk `#{disk_name}` can not be found in the database") + end + end + + context 'when disk exists in database but not in the cloud' 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 + + before do + allow(cloud).to receive(:has_disk).and_return(false) + end + + it 'returns an error' do + expect(nats_rpc).to receive(:send_message).with(reply, { + 'error' => "disk `#{disk_name}` can not be found in the cloud" + }) + expect { detach_dynamic_disk_job.perform }.to raise_error("disk `#{disk_name}` can not be found in the cloud") + end + end + end + end +end diff --git a/src/bosh-director/spec/unit/bosh/director/jobs/dynamic_disks/provide_dynamic_disk_spec.rb b/src/bosh-director/spec/unit/bosh/director/jobs/dynamic_disks/provide_dynamic_disk_spec.rb index f91e846deba..fa0dd335daf 100644 --- a/src/bosh-director/spec/unit/bosh/director/jobs/dynamic_disks/provide_dynamic_disk_spec.rb +++ b/src/bosh-director/spec/unit/bosh/director/jobs/dynamic_disks/provide_dynamic_disk_spec.rb @@ -51,14 +51,19 @@ module Bosh::Director ) end - it 'attaches the disk to VM' do - expect(cloud).to receive(:attach_disk).with('fake-vm-cid', disk_cid).and_return(disk_hint) + it 'attaches the disk to VM and updates disk vm and availability zone' do + expect(cloud).not_to receive(:create_disk) + expect(cloud).to receive(:attach_disk).with(vm.cid, disk_cid).and_return(disk_hint) expect(nats_rpc).to receive(:send_message).with(reply, { 'error' => nil, 'disk_name' => disk_name, 'disk_hint' => disk_hint, }) - expect(provide_dynamic_disk_job.perform).to eq("attached disk '#{disk_name}' to '#{vm.cid}' in deployment '#{vm.instance.deployment.name}'") + expect(provide_dynamic_disk_job.perform).to eq("attached disk `#{disk_name}` to `#{vm.cid}` in deployment `#{vm.instance.deployment.name}`") + + model = Models::DynamicDisk.where(disk_cid: disk_cid).first + expect(model.vm).to eq(vm) + expect(model.availability_zone).to eq(vm.instance.availability_zone) end end @@ -66,21 +71,22 @@ module Bosh::Director it 'creates the disk and attaches it to VM' do expect(cloud).to receive(:create_disk).with(disk_size, disk_cloud_properties, vm.cid).and_return(disk_cid) expect(cloud).to receive(:respond_to?).with(:set_disk_metadata).and_return(false) - expect(cloud).to receive(:attach_disk).with('fake-vm-cid', disk_cid).and_return(disk_hint) + expect(cloud).to receive(:attach_disk).with(vm.cid, disk_cid).and_return(disk_hint) expect(nats_rpc).to receive(:send_message).with(reply, { 'error' => nil, 'disk_name' => disk_name, 'disk_hint' => disk_hint, }) - expect(provide_dynamic_disk_job.perform).to eq("attached disk '#{disk_name}' to '#{vm.cid}' in deployment '#{vm.instance.deployment.name}'") + expect(provide_dynamic_disk_job.perform).to eq("attached disk `#{disk_name}` to `#{vm.cid}` in deployment `#{vm.instance.deployment.name}`") - model = Models::DynamicDisk.where(disk_cid: 'fake-disk-cid').first + model = Models::DynamicDisk.where(disk_cid: disk_cid).first expect(model.name).to eq(disk_name) expect(model.size).to eq(disk_size) expect(model.deployment_id).to eq(vm.instance.deployment.id) expect(model.disk_pool_name).to eq(disk_pool_name) expect(model.cpi).to eq(vm.cpi) + expect(model.vm).to eq(vm) expect(model.availability_zone).to eq(vm.instance.availability_zone) expect(model.metadata).to eq(metadata) end @@ -98,12 +104,12 @@ module Bosh::Director end it 'returns an error from attach_disk call' do - expect(cloud).to receive(:attach_disk).with('fake-vm-cid', disk_cid).and_raise('Disk not found') + expect(cloud).to receive(:attach_disk).with(vm.cid, disk_cid).and_raise('some-error') expect(nats_rpc).to receive(:send_message).with(reply, { - 'error' => "Disk not found" + 'error' => 'some-error' }) - expect { provide_dynamic_disk_job.perform }.to raise_error("Disk not found") + expect { provide_dynamic_disk_job.perform }.to raise_error('some-error') end end @@ -148,7 +154,7 @@ module Bosh::Director 'disk_name' => disk_name, 'disk_hint' => disk_hint, }) - expect(provide_dynamic_disk_job.perform).to eq("attached disk '#{disk_name}' to '#{vm.cid}' in deployment '#{vm.instance.deployment.name}'") + expect(provide_dynamic_disk_job.perform).to eq("attached disk `#{disk_name}` to `#{vm.cid}` in deployment `#{vm.instance.deployment.name}`") end end @@ -176,14 +182,14 @@ module Bosh::Director end it 'gets the disk cloud properties from the latest cloud config for those teams' do - expect(cloud).to receive(:create_disk).with(disk_size, disk_cloud_properties, vm.cid).and_return('fake-disk-cid') - expect(cloud).to receive(:attach_disk).with('fake-vm-cid', 'fake-disk-cid').and_return(disk_hint) + expect(cloud).to receive(:create_disk).with(disk_size, disk_cloud_properties, vm.cid).and_return(disk_cid) + expect(cloud).to receive(:attach_disk).with(vm.cid, disk_cid).and_return(disk_hint) expect(nats_rpc).to receive(:send_message).with(reply, { 'error' => nil, 'disk_name' => disk_name, 'disk_hint' => disk_hint, }) - expect(provide_dynamic_disk_job.perform).to eq("attached disk '#{disk_name}' to '#{vm.cid}' in deployment '#{vm.instance.deployment.name}'") + expect(provide_dynamic_disk_job.perform).to eq("attached disk `#{disk_name}` to `#{vm.cid}` in deployment `#{vm.instance.deployment.name}`") end end From 41713bff11f4b04fa71ea66d0b8100049353af2c Mon Sep 17 00:00:00 2001 From: Nishad Mathur Date: Thu, 21 Aug 2025 21:22:41 +0000 Subject: [PATCH 4/8] Delete dynamic diks when deleting deploymennt Signed-off-by: Maria Shaldybin Co-authored-by: Maria Shaldybin --- src/bosh-director/lib/bosh/director.rb | 1 + .../lib/bosh/director/deployment_deleter.rb | 7 +- .../bosh/director/deployment_plan/steps.rb | 2 + .../steps/delete_dynamic_disk_step.rb | 22 ++++++ .../steps/detach_dynamic_disk_step.rb | 26 +++++++ .../steps/detach_instance_disks_step.rb | 4 ++ .../lib/bosh/director/disk_deleter.rb | 50 ++++++++++++++ .../lib/bosh/director/disk_manager.rb | 4 ++ .../bosh/director/jobs/delete_deployment.rb | 3 +- .../jobs/dynamic_disks/delete_dynamic_disk.rb | 24 +++---- .../jobs/dynamic_disks/detach_dynamic_disk.rb | 24 +++---- .../lib/bosh/director/models/deployment.rb | 1 + .../lib/bosh/director/models/vm.rb | 1 + src/bosh-director/spec/factories.rb | 2 + .../bosh/director/deployment_deleter_spec.rb | 21 ++++-- .../steps/delete_dynamic_disk_step_spec.rb | 66 ++++++++++++++++++ .../steps/detach_dynamic_disk_step_spec.rb | 69 +++++++++++++++++++ .../steps/detach_instance_disks_step_spec.rb | 23 ++++++- .../unit/bosh/director/disk_deleter_spec.rb | 43 ++++++++++++ .../unit/bosh/director/disk_manager_spec.rb | 16 +++++ .../dynamic_disks/detach_dynamic_disk_spec.rb | 23 ------- 21 files changed, 370 insertions(+), 62 deletions(-) create mode 100644 src/bosh-director/lib/bosh/director/deployment_plan/steps/delete_dynamic_disk_step.rb create mode 100644 src/bosh-director/lib/bosh/director/deployment_plan/steps/detach_dynamic_disk_step.rb create mode 100644 src/bosh-director/lib/bosh/director/disk_deleter.rb create mode 100644 src/bosh-director/spec/unit/bosh/director/deployment_plan/steps/delete_dynamic_disk_step_spec.rb create mode 100644 src/bosh-director/spec/unit/bosh/director/deployment_plan/steps/detach_dynamic_disk_step_spec.rb create mode 100644 src/bosh-director/spec/unit/bosh/director/disk_deleter_spec.rb diff --git a/src/bosh-director/lib/bosh/director.rb b/src/bosh-director/lib/bosh/director.rb index 2c1aac58258..8d80b241c4d 100644 --- a/src/bosh-director/lib/bosh/director.rb +++ b/src/bosh-director/lib/bosh/director.rb @@ -128,6 +128,7 @@ module Director require 'bosh/director/instance_updater/state_applier' require 'bosh/director/instance_updater/update_procedure' require 'bosh/director/disk_manager' +require 'bosh/director/disk_deleter' require 'bosh/director/orphan_disk_manager' require 'bosh/director/orphan_network_manager' require 'bosh/director/stopper' diff --git a/src/bosh-director/lib/bosh/director/deployment_deleter.rb b/src/bosh-director/lib/bosh/director/deployment_deleter.rb index cfe7e203071..011f14355d5 100644 --- a/src/bosh-director/lib/bosh/director/deployment_deleter.rb +++ b/src/bosh-director/lib/bosh/director/deployment_deleter.rb @@ -8,7 +8,7 @@ def initialize(event_log, logger, max_threads) @variables_interpolator = ConfigServer::VariablesInterpolator.new end - def delete(deployment_model, instance_deleter, vm_deleter) + def delete(deployment_model, instance_deleter, vm_deleter, disk_deleter) instance_plans = deployment_model.instances.map do |instance_model| DeploymentPlan::InstancePlan.new( existing_instance: instance_model, @@ -41,6 +41,11 @@ def delete(deployment_model, instance_deleter, vm_deleter) end end + event_log_stage.advance_and_track('Deleting dynamic disks') do + @logger.info('Deleting dynamic disks') + disk_deleter.delete_dynamic_disks(deployment_model, event_log_stage, max_threads: @max_threads) + end + if Config.network_lifecycle_enabled? deployment_model.networks.each do |network| with_network_lock(network.name) do diff --git a/src/bosh-director/lib/bosh/director/deployment_plan/steps.rb b/src/bosh-director/lib/bosh/director/deployment_plan/steps.rb index 0c2e802d33a..d55d43279ac 100644 --- a/src/bosh-director/lib/bosh/director/deployment_plan/steps.rb +++ b/src/bosh-director/lib/bosh/director/deployment_plan/steps.rb @@ -12,7 +12,9 @@ module Steps require 'bosh/director/deployment_plan/steps/commit_instance_network_settings_step' require 'bosh/director/deployment_plan/steps/delete_vm_step' require 'bosh/director/deployment_plan/steps/detach_disk_step' +require 'bosh/director/deployment_plan/steps/detach_dynamic_disk_step' require 'bosh/director/deployment_plan/steps/detach_instance_disks_step' +require 'bosh/director/deployment_plan/steps/delete_dynamic_disk_step' require 'bosh/director/deployment_plan/steps/elect_active_vm_step' require 'bosh/director/deployment_plan/steps/mount_disk_step' require 'bosh/director/deployment_plan/steps/mount_instance_disks_step' diff --git a/src/bosh-director/lib/bosh/director/deployment_plan/steps/delete_dynamic_disk_step.rb b/src/bosh-director/lib/bosh/director/deployment_plan/steps/delete_dynamic_disk_step.rb new file mode 100644 index 00000000000..7f8c9fd0783 --- /dev/null +++ b/src/bosh-director/lib/bosh/director/deployment_plan/steps/delete_dynamic_disk_step.rb @@ -0,0 +1,22 @@ +module Bosh::Director + module DeploymentPlan + module Steps + class DeleteDynamicDiskStep + def initialize(disk) + @disk = disk + @logger = Config.logger + end + + def perform(_report) + return if @disk.nil? + + @logger.info("Deleting dynamic disk '#{@disk.disk_cid}'") + cloud = Bosh::Director::CloudFactory.create.get(@disk.cpi) + cloud.delete_disk(@disk.disk_cid) if cloud.has_disk(@disk.disk_cid) + + @disk.destroy + end + end + end + end +end diff --git a/src/bosh-director/lib/bosh/director/deployment_plan/steps/detach_dynamic_disk_step.rb b/src/bosh-director/lib/bosh/director/deployment_plan/steps/detach_dynamic_disk_step.rb new file mode 100644 index 00000000000..c68f5d0d652 --- /dev/null +++ b/src/bosh-director/lib/bosh/director/deployment_plan/steps/detach_dynamic_disk_step.rb @@ -0,0 +1,26 @@ +module Bosh::Director + module DeploymentPlan + module Steps + class DetachDynamicDiskStep + def initialize(disk) + @disk = disk + @logger = Config.logger + end + + def perform(_report) + return if @disk.nil? || @disk.vm.nil? + + cloud = CloudFactory.create.get(@disk.cpi, @disk.vm.stemcell_api_version) + @logger.info("Detaching dynamic disk #{@disk.disk_cid}") + + cloud.detach_disk(@disk.vm.cid, @disk.disk_cid) + @disk.update(vm_id: nil) + rescue Bosh::Clouds::DiskNotAttached + raise CloudDiskNotAttached, + "'#{@disk.vm.cid}' VM should have dynamic disk " \ + "'#{@disk.disk_cid}' attached but it doesn't (according to CPI)" + end + end + end + end +end diff --git a/src/bosh-director/lib/bosh/director/deployment_plan/steps/detach_instance_disks_step.rb b/src/bosh-director/lib/bosh/director/deployment_plan/steps/detach_instance_disks_step.rb index 5add89a373c..23d469c3624 100644 --- a/src/bosh-director/lib/bosh/director/deployment_plan/steps/detach_instance_disks_step.rb +++ b/src/bosh-director/lib/bosh/director/deployment_plan/steps/detach_instance_disks_step.rb @@ -13,6 +13,10 @@ def perform(report) @instance_model.persistent_disks.each do |disk| DetachDiskStep.new(disk).perform(report) end + + @instance_model.active_vm.dynamic_disks.each do |disk| + DetachDynamicDiskStep.new(disk).perform(report) + end end end end diff --git a/src/bosh-director/lib/bosh/director/disk_deleter.rb b/src/bosh-director/lib/bosh/director/disk_deleter.rb new file mode 100644 index 00000000000..39497ae2d8b --- /dev/null +++ b/src/bosh-director/lib/bosh/director/disk_deleter.rb @@ -0,0 +1,50 @@ +module Bosh::Director + # Coordinates the safe deletion of an instance and all associates resources. + class DiskDeleter + def initialize(logger, disk_manager, options = {}) + @disk_manager = disk_manager + force = options.fetch(:force, false) + @error_ignorer = ErrorIgnorer.new(force, @logger) + end + + def delete_dynamic_disks(deployment_model, event_log_stage, options = {}) + max_threads = options[:max_threads] || Config.max_threads + ThreadPool.new(:max_threads => max_threads).wrap do |pool| + deployment_model.dynamic_disks.each do |dynamic_disk| + pool.process { delete_dynamic_disk(dynamic_disk, event_log_stage) } + end + end + end + + private + + def delete_dynamic_disk(dynamic_disk, event_log_stage) + parent_id = add_event(dynamic_disk.deployment.name, dynamic_disk.name) + event_log_stage.advance_and_track(dynamic_disk.to_s) do + @error_ignorer.with_force_check do + @disk_manager.delete_dynamic_disk(dynamic_disk) + end + + dynamic_disk.destroy + end + rescue Exception => e + raise e + ensure + add_event(deployment_name, dynamic_disk.name, parent_id, e) if parent_id + end + + def add_event(deployment_name, disk_name, parent_id = nil, error = nil) + event = Config.current_job.event_manager.create_event( + parent_id: parent_id, + user: Config.current_job.username, + action: 'delete', + object_type: 'disk', + object_name: disk_name, + task: Config.current_job.task_id, + deployment: deployment_name, + error: error, + ) + event.id + end + end +end diff --git a/src/bosh-director/lib/bosh/director/disk_manager.rb b/src/bosh-director/lib/bosh/director/disk_manager.rb index e82693719e1..7efb6d1e303 100644 --- a/src/bosh-director/lib/bosh/director/disk_manager.rb +++ b/src/bosh-director/lib/bosh/director/disk_manager.rb @@ -65,6 +65,10 @@ def delete_persistent_disks(instance_model) end end + def delete_dynamic_disk(disk) + DeploymentPlan::Steps::DeleteDynamicDiskStep.new(disk).perform(step_report) + end + def attach_disk(disk, tags) report = step_report DeploymentPlan::Steps::AttachDiskStep.new(disk, tags).perform(report) diff --git a/src/bosh-director/lib/bosh/director/jobs/delete_deployment.rb b/src/bosh-director/lib/bosh/director/jobs/delete_deployment.rb index df113996a7b..ef1e946f1a4 100644 --- a/src/bosh-director/lib/bosh/director/jobs/delete_deployment.rb +++ b/src/bosh-director/lib/bosh/director/jobs/delete_deployment.rb @@ -30,10 +30,11 @@ def perform force: @force, stop_intent: :delete_deployment, ) + disk_deleter = DiskDeleter.new(logger, disk_manager, force: @force) deployment_deleter = DeploymentDeleter.new(Config.event_log, logger, Config.max_threads) vm_deleter = Bosh::Director::VmDeleter.new(logger, @force, Config.enable_virtual_delete_vms) - deployment_deleter.delete(deployment_model, instance_deleter, vm_deleter) + deployment_deleter.delete(deployment_model, instance_deleter, vm_deleter, disk_deleter) add_event(parent_id) "/deployments/#{@deployment_name}" diff --git a/src/bosh-director/lib/bosh/director/jobs/dynamic_disks/delete_dynamic_disk.rb b/src/bosh-director/lib/bosh/director/jobs/dynamic_disks/delete_dynamic_disk.rb index cace5a8a0bb..02990e57369 100644 --- a/src/bosh-director/lib/bosh/director/jobs/dynamic_disks/delete_dynamic_disk.rb +++ b/src/bosh-director/lib/bosh/director/jobs/dynamic_disks/delete_dynamic_disk.rb @@ -17,25 +17,19 @@ def initialize(reply, disk_name) def perform disk_model = Models::DynamicDisk.find(name: @disk_name) - unless disk_model.nil? - cloud = Bosh::Director::CloudFactory.create.get(disk_model.cpi) - if cloud.has_disk(disk_model.disk_cid) - cloud.delete_disk(disk_model.disk_cid) - end - Models::DynamicDisk.where(id: disk_model.id).delete - end + return "disk with name `#{@disk_name}` was already deleted" if disk_model.nil? - response = { 'error' => nil } - nats_rpc.send_message(@reply, response) + cloud = Bosh::Director::CloudFactory.create.get(disk_model.cpi) + disk_cid = disk_model.disk_cid - if disk_model.nil? - "disk with name `#{@disk_name}` was already deleted" - else - "deleted disk `#{disk_model.disk_cid}`" - end + cloud.delete_disk(disk_cid) if cloud.has_disk(disk_cid) + disk_model.destroy + + "deleted disk `#{disk_cid}`" rescue => e - nats_rpc.send_message(@reply, { 'error' => e.message }) raise e + ensure + nats_rpc.send_message(@reply, { 'error' => e&.message }) end end end diff --git a/src/bosh-director/lib/bosh/director/jobs/dynamic_disks/detach_dynamic_disk.rb b/src/bosh-director/lib/bosh/director/jobs/dynamic_disks/detach_dynamic_disk.rb index c3a0966dde1..e3139899c9c 100644 --- a/src/bosh-director/lib/bosh/director/jobs/dynamic_disks/detach_dynamic_disk.rb +++ b/src/bosh-director/lib/bosh/director/jobs/dynamic_disks/detach_dynamic_disk.rb @@ -19,29 +19,21 @@ def perform disk_model = Models::DynamicDisk.find(name: @disk_name) raise "disk `#{@disk_name}` can not be found in the database" if disk_model.nil? - cloud = Bosh::Director::CloudFactory.create.get(disk_model.cpi) + return "disk `#{disk_model.disk_cid}` was already detached" if disk_model.vm.nil? - raise "disk `#{@disk_name}` can not be found in the cloud" unless cloud.has_disk(disk_model.disk_cid) - - vm = disk_model.vm - unless vm.nil? - cloud.detach_disk(disk_model.vm.cid, disk_model.disk_cid) - disk_model.update(vm_id: nil) - end + cloud = Bosh::Director::CloudFactory.create.get(disk_model.cpi, disk_model.vm.stemcell_api_version) + vm_cid = disk_model.vm.cid + cloud.detach_disk(disk_model.vm.cid, disk_model.disk_cid) + disk_model.update(vm_id: nil) - nats_rpc.send_message(@reply, { 'error' => nil }) - if vm.nil? - "disk `#{disk_model.disk_cid}` was already detached" - else - "detached disk `#{disk_model.disk_cid}` from vm `#{vm.cid}`" - end + "detached disk `#{disk_model.disk_cid}` from vm `#{vm_cid}`" rescue Bosh::Clouds::DiskNotAttached disk_model.update(vm_id: nil) - nats_rpc.send_message(@reply, { 'error' => nil }) "disk `#{disk_model.disk_cid}` was already detached" rescue => e - nats_rpc.send_message(@reply, { 'error' => e.message }) raise e + ensure + nats_rpc.send_message(@reply, { 'error' => e&.message }) end end end diff --git a/src/bosh-director/lib/bosh/director/models/deployment.rb b/src/bosh-director/lib/bosh/director/models/deployment.rb index e14881994a3..09a2b64fe54 100644 --- a/src/bosh-director/lib/bosh/director/models/deployment.rb +++ b/src/bosh-director/lib/bosh/director/models/deployment.rb @@ -12,6 +12,7 @@ def self.join_table_block(type) many_to_many :release_versions one_to_many :job_instances, :class => 'Bosh::Director::Models::Instance' one_to_many :instances + one_to_many :dynamic_disks one_to_many :properties, :class => "Bosh::Director::Models::DeploymentProperty" one_to_many :problems, :class => "Bosh::Director::Models::DeploymentProblem" one_to_many :link_consumers, :class => 'Bosh::Director::Models::Links::LinkConsumer' diff --git a/src/bosh-director/lib/bosh/director/models/vm.rb b/src/bosh-director/lib/bosh/director/models/vm.rb index b6790f57830..a5c1c0dffa2 100644 --- a/src/bosh-director/lib/bosh/director/models/vm.rb +++ b/src/bosh-director/lib/bosh/director/models/vm.rb @@ -2,6 +2,7 @@ module Bosh::Director::Models class Vm < Sequel::Model(Bosh::Director::Config.db) many_to_one :instance one_to_many :ip_addresses + one_to_many :dynamic_disks def before_destroy ip_addresses_dataset.each do |ip_address| diff --git a/src/bosh-director/spec/factories.rb b/src/bosh-director/spec/factories.rb index c8548946fd5..81353482d45 100644 --- a/src/bosh-director/spec/factories.rb +++ b/src/bosh-director/spec/factories.rb @@ -292,6 +292,8 @@ factory :models_dynamic_disk, class: Bosh::Director::Models::DynamicDisk do sequence(:name) { |i| "dynamic-disk-name-#{i}" } sequence(:disk_cid) { |i| "dynamic-disk-cid-#{i}" } + sequence(:disk_pool_name) { |i| "dynamic-disk-pool-name-#{i}" } + sequence(:cpi) { |i| "dynamic-disk-cpi-#{i}" } sequence(:size) { |i| 1024 + i } association :deployment, factory: :models_deployment, strategy: :create end diff --git a/src/bosh-director/spec/unit/bosh/director/deployment_deleter_spec.rb b/src/bosh-director/spec/unit/bosh/director/deployment_deleter_spec.rb index c7575c25f84..44dd9941aba 100644 --- a/src/bosh-director/spec/unit/bosh/director/deployment_deleter_spec.rb +++ b/src/bosh-director/spec/unit/bosh/director/deployment_deleter_spec.rb @@ -9,6 +9,7 @@ module Bosh::Director let(:blobstore) { instance_double(Bosh::Director::Blobstore::Client) } let(:instance_deleter) { instance_double(InstanceDeleter) } let(:vm_deleter) { instance_double(VmDeleter) } + let(:disk_deleter) { instance_double(DiskDeleter) } let(:dns_enabled) { false } let(:task) { FactoryBot.create(:models_task, id: 42) } let(:task_writer) { Bosh::Director::TaskDBWriter.new(:event_output, task.id) } @@ -31,6 +32,7 @@ module Bosh::Director deployment_model.add_property(FactoryBot.create(:models_deployment_property)) allow(instance_deleter).to receive(:delete_instance_plans) + allow(disk_deleter).to receive(:delete_dynamic_disks) allow(deployment_model).to receive(:destroy) end @@ -41,29 +43,38 @@ module Bosh::Director expect(options).to eq(max_threads: 3) end - deleter.delete(deployment_model, instance_deleter, vm_deleter) + deleter.delete(deployment_model, instance_deleter, vm_deleter, disk_deleter) end it 'removes all stemcells' do expect(deployment_stemcell.deployments).to include(deployment_model) - deleter.delete(deployment_model, instance_deleter, vm_deleter) + deleter.delete(deployment_model, instance_deleter, vm_deleter, disk_deleter) expect(deployment_stemcell.reload.deployments).to be_empty end it 'removes all releases' do expect(deployment_release_version.deployments).to include(deployment_model) - deleter.delete(deployment_model, instance_deleter, vm_deleter) + deleter.delete(deployment_model, instance_deleter, vm_deleter, disk_deleter) expect(deployment_release_version.reload.deployments).to be_empty end it 'deletes all properties' do - deleter.delete(deployment_model, instance_deleter, vm_deleter) + deleter.delete(deployment_model, instance_deleter, vm_deleter, disk_deleter) expect(Models::DeploymentProperty.all.size).to eq(0) end + it 'deletes all dynamic disks' do + expect(disk_deleter).to receive(:delete_dynamic_disks).with( + deployment_model, + instance_of(EventLog::Stage), + {max_threads: 3} + ) + deleter.delete(deployment_model, instance_deleter, vm_deleter, disk_deleter) + end + it 'destroys deployment model' do expect(deployment_model).to receive(:destroy) - deleter.delete(deployment_model, instance_deleter, vm_deleter) + deleter.delete(deployment_model, instance_deleter, vm_deleter, disk_deleter) end end end diff --git a/src/bosh-director/spec/unit/bosh/director/deployment_plan/steps/delete_dynamic_disk_step_spec.rb b/src/bosh-director/spec/unit/bosh/director/deployment_plan/steps/delete_dynamic_disk_step_spec.rb new file mode 100644 index 00000000000..c020812f6a3 --- /dev/null +++ b/src/bosh-director/spec/unit/bosh/director/deployment_plan/steps/delete_dynamic_disk_step_spec.rb @@ -0,0 +1,66 @@ +require 'spec_helper' + +module Bosh::Director + module DeploymentPlan + module Steps + describe DeleteDynamicDiskStep do + subject(:step) { DeleteDynamicDiskStep.new(disk) } + + let!(:disk) { FactoryBot.create(:models_dynamic_disk, name: 'disk-name') } + let(:cloud_factory) { instance_double(CloudFactory) } + let(:cloud) { instance_double(Bosh::Clouds::ExternalCpi) } + let(:report) { Stages::Report.new } + + before do + allow(CloudFactory).to receive(:create).and_return(cloud_factory) + allow(cloud_factory).to receive(:get).with(disk&.cpi).once.and_return(cloud) + allow(cloud).to receive(:delete_disk) + allow(cloud).to receive(:has_disk).and_return(true) + end + + it 'calls out to cpi associated with disk to delete disk' do + expect(cloud_factory).to receive(:get).with(disk&.cpi).once.and_return(cloud) + expect(cloud).to receive(:delete_disk).with(disk.disk_cid) + + step.perform(report) + end + + it 'deletes dynamic disk from the database' do + step.perform(report) + + expect(Models::DynamicDisk.find(disk_cid: disk.disk_cid)).to be_nil + end + + context 'when disk does not exist' do + before do + allow(cloud).to receive(:has_disk).and_return(false) + end + + it 'does not perform any cloud actions' do + expect(cloud).to_not receive(:delete_disk) + + step.perform(report) + + expect(Models::DynamicDisk.find(disk_cid: disk.disk_cid)).to be_nil + end + end + + it 'logs notification of deleting' do + expect(per_spec_logger).to receive(:info).with("Deleting dynamic disk '#{disk.disk_cid}'") + + step.perform(report) + end + + context 'when given nil disk' do + let(:disk) { nil } + + it 'does not perform any cloud actions' do + expect(cloud).to_not receive(:delete_disk) + + step.perform(report) + end + end + end + end + end +end diff --git a/src/bosh-director/spec/unit/bosh/director/deployment_plan/steps/detach_dynamic_disk_step_spec.rb b/src/bosh-director/spec/unit/bosh/director/deployment_plan/steps/detach_dynamic_disk_step_spec.rb new file mode 100644 index 00000000000..4a7a1d7e274 --- /dev/null +++ b/src/bosh-director/spec/unit/bosh/director/deployment_plan/steps/detach_dynamic_disk_step_spec.rb @@ -0,0 +1,69 @@ +require 'spec_helper' + +module Bosh::Director + module DeploymentPlan + module Steps + describe DetachDynamicDiskStep do + subject(:step) { DetachDynamicDiskStep.new(disk) } + + let!(:vm) { FactoryBot.create(:models_vm, stemcell_api_version: 25) } + let!(:disk) { FactoryBot.create(:models_dynamic_disk, vm: vm, name: 'disk-name') } + let(:cloud_factory) { instance_double(CloudFactory) } + let(:cloud) { instance_double(Bosh::Clouds::ExternalCpi) } + let(:report) { Stages::Report.new } + + before do + allow(CloudFactory).to receive(:create).and_return(cloud_factory) + allow(cloud_factory).to receive(:get).with(disk&.cpi, 25).once.and_return(cloud) + allow(cloud).to receive(:detach_disk) + end + + it 'calls out to cpi associated with disk to detach disk' do + expect(cloud_factory).to receive(:get).with(disk&.cpi, 25).once.and_return(cloud) + expect(cloud).to receive(:detach_disk).with(vm.cid, disk.disk_cid) + + step.perform(report) + end + + it 'clears vm_cid from disk' do + step.perform(report) + expect(Models::DynamicDisk.find(disk_cid: disk.disk_cid).vm_id).to be_nil + end + + it 'logs notification of detaching' do + expect(per_spec_logger).to receive(:info).with("Detaching dynamic disk #{disk.disk_cid}") + + step.perform(report) + end + + context 'when the CPI reports that a disk is not attached' do + before do + allow(cloud).to receive( :detach_disk) + .with(vm.cid, disk.disk_cid) + .and_raise(Bosh::Clouds::DiskNotAttached.new('foo')) + end + + it 'raises a CloudDiskNotAttached error' do + expect { + step.perform(report) + }.to raise_error( + CloudDiskNotAttached, + "'#{vm.cid}' VM should have dynamic disk '#{disk.disk_cid}' attached " \ + "but it doesn't (according to CPI)", + ) + end + end + + context 'when given nil disk' do + let(:disk) { nil } + + it 'does not perform any cloud actions' do + expect(cloud).to_not receive(:detach_disk) + + step.perform(report) + end + end + end + end + end +end diff --git a/src/bosh-director/spec/unit/bosh/director/deployment_plan/steps/detach_instance_disks_step_spec.rb b/src/bosh-director/spec/unit/bosh/director/deployment_plan/steps/detach_instance_disks_step_spec.rb index 0829bd564ae..6e409be3720 100644 --- a/src/bosh-director/spec/unit/bosh/director/deployment_plan/steps/detach_instance_disks_step_spec.rb +++ b/src/bosh-director/spec/unit/bosh/director/deployment_plan/steps/detach_instance_disks_step_spec.rb @@ -10,21 +10,29 @@ module Steps let!(:vm) { FactoryBot.create(:models_vm, instance: instance, active: true, cpi: 'vm-cpi') } let!(:disk1) { FactoryBot.create(:models_persistent_disk, instance: instance, name: '') } let!(:disk2) { FactoryBot.create(:models_persistent_disk, instance: instance, name: 'unmanaged') } + let!(:dynamic_disk_1) { FactoryBot.create(:models_dynamic_disk, vm: vm) } + let!(:dynamic_disk_2) { FactoryBot.create(:models_dynamic_disk, vm: vm) } let(:detach_disk_1) { instance_double(DetachDiskStep) } let(:detach_disk_2) { instance_double(DetachDiskStep) } + let(:detach_dynamic_disk_1) { instance_double(DetachDynamicDiskStep) } + let(:detach_dynamic_disk_2) { instance_double(DetachDynamicDiskStep) } let(:report) { Stages::Report.new } before do allow(DetachDiskStep).to receive(:new).with(disk1).and_return(detach_disk_1) allow(DetachDiskStep).to receive(:new).with(disk2).and_return(detach_disk_2) + allow(DetachDynamicDiskStep).to receive(:new).with(dynamic_disk_1).and_return(detach_dynamic_disk_1) + allow(DetachDynamicDiskStep).to receive(:new).with(dynamic_disk_2).and_return(detach_dynamic_disk_2) allow(detach_disk_1).to receive(:perform).with(report).once allow(detach_disk_2).to receive(:perform).with(report).once + allow(detach_dynamic_disk_1).to receive(:perform).with(report).once + allow(detach_dynamic_disk_2).to receive(:perform).with(report).once end - it 'calls out to vms cpi to detach all attached disks' do + it 'calls out to vms cpi to detach all attached persistent disks' do expect(DetachDiskStep).to receive(:new).with(disk1) expect(DetachDiskStep).to receive(:new).with(disk2) expect(detach_disk_1).to receive(:perform).with(report).once @@ -33,6 +41,15 @@ module Steps step.perform(report) end + it 'calls out to vms cpi to detach all attached dynamic disks' do + expect(DetachDynamicDiskStep).to receive(:new).with(dynamic_disk_1) + expect(DetachDynamicDiskStep).to receive(:new).with(dynamic_disk_2) + expect(detach_dynamic_disk_1).to receive(:perform).with(report).once + expect(detach_dynamic_disk_2).to receive(:perform).with(report).once + + step.perform(report) + end + context 'when the instance does not have an active vm' do before do vm.update(active: false) @@ -43,6 +60,10 @@ module Steps expect(DetachDiskStep).not_to receive(:new) expect(detach_disk_1).not_to receive(:perform).with(report) expect(detach_disk_2).not_to receive(:perform).with(report) + expect(DetachDynamicDiskStep).not_to receive(:new) + expect(DetachDynamicDiskStep).not_to receive(:new) + expect(detach_dynamic_disk_1).not_to receive(:perform).with(report) + expect(detach_dynamic_disk_2).not_to receive(:perform).with(report) step.perform(report) end diff --git a/src/bosh-director/spec/unit/bosh/director/disk_deleter_spec.rb b/src/bosh-director/spec/unit/bosh/director/disk_deleter_spec.rb new file mode 100644 index 00000000000..395b8419cef --- /dev/null +++ b/src/bosh-director/spec/unit/bosh/director/disk_deleter_spec.rb @@ -0,0 +1,43 @@ +require 'spec_helper' + +module Bosh::Director + describe DiskDeleter do + subject(:deleter) { described_class.new(per_spec_logger, disk_manager, {}) } + let(:disk_manager) { DiskManager.new(per_spec_logger) } + let(:options) { {} } + let(:event_log) { Bosh::Director::EventLog::Log.new(task_writer) } + let(:event_log_stage) { instance_double('Bosh::Director::EventLog::Stage') } + let!(:deployment_model) { FactoryBot.create(:models_deployment, name: 'fake-deployment') } + let(:delete_job) { Jobs::DeleteDeployment.new('test_deployment', {}) } + let(:task) { FactoryBot.create(:models_task, id: 42, username: 'user') } + + before do + allow(event_log_stage).to receive(:advance_and_track).and_yield + allow(delete_job).to receive(:task_id).and_return(task.id) + allow(Config).to receive(:current_job).and_return(delete_job) + end + + describe '#delete_dynamic_disks' do + let(:five_disks) { + 5.times.map do | index | + FactoryBot.create(:models_dynamic_disk, deployment: deployment_model) + end + } + + it 'should delete disks with the config max threads option' do + allow(Config).to receive(:max_threads).and_return(5) + pool = double('pool') + expect(ThreadPool).to receive(:new).with(max_threads: 5).and_return(pool) + expect(pool).to receive(:wrap).and_yield(pool) + expect(pool).to receive(:process).exactly(5).times.and_yield + + + 5.times do | index| + expect(disk_manager).to receive(:delete_dynamic_disk).with(five_disks[index]) + end + + deleter.delete_dynamic_disks(deployment_model, event_log_stage) + end + end + end +end diff --git a/src/bosh-director/spec/unit/bosh/director/disk_manager_spec.rb b/src/bosh-director/spec/unit/bosh/director/disk_manager_spec.rb index fb8a7e38ce7..e8c925fbf8b 100644 --- a/src/bosh-director/spec/unit/bosh/director/disk_manager_spec.rb +++ b/src/bosh-director/spec/unit/bosh/director/disk_manager_spec.rb @@ -778,6 +778,22 @@ module Bosh::Director end end + describe '#delete_dynamic_disk' do + let!(:dynamic_disk) { FactoryBot.create(:models_dynamic_disk, vm: instance_model.active_vm) } + before do + allow(cloud_factory).to receive(:get).with(dynamic_disk.cpi).and_return(cloud) + allow(cloud).to receive(:has_disk).and_return(true) + end + + it 'deletes disks for instance' do + expect(Models::DynamicDisk.all.size).to eq(1) + allow(cloud).to receive(:delete_disk).with(dynamic_disk.disk_cid) + + disk_manager.delete_dynamic_disk(dynamic_disk) + expect(Models::DynamicDisk.all.size).to eq(0) + end + end + describe '#attach_disks_if_needed' do let(:cloud_for_set_disk_metadata) { instance_double(Bosh::Clouds::ExternalCpi) } diff --git a/src/bosh-director/spec/unit/bosh/director/jobs/dynamic_disks/detach_dynamic_disk_spec.rb b/src/bosh-director/spec/unit/bosh/director/jobs/dynamic_disks/detach_dynamic_disk_spec.rb index 607180fc457..4a9767adc4a 100644 --- a/src/bosh-director/spec/unit/bosh/director/jobs/dynamic_disks/detach_dynamic_disk_spec.rb +++ b/src/bosh-director/spec/unit/bosh/director/jobs/dynamic_disks/detach_dynamic_disk_spec.rb @@ -86,29 +86,6 @@ module Bosh::Director expect { detach_dynamic_disk_job.perform }.to raise_error("disk `#{disk_name}` can not be found in the database") end end - - context 'when disk exists in database but not in the cloud' 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 - - before do - allow(cloud).to receive(:has_disk).and_return(false) - end - - it 'returns an error' do - expect(nats_rpc).to receive(:send_message).with(reply, { - 'error' => "disk `#{disk_name}` can not be found in the cloud" - }) - expect { detach_dynamic_disk_job.perform }.to raise_error("disk `#{disk_name}` can not be found in the cloud") - end - end end end end From cc39fa8b5aff318fcecd9a3de902dfa893f95d09 Mon Sep 17 00:00:00 2001 From: Maria Shaldybin Date: Thu, 21 Aug 2025 21:24:06 +0000 Subject: [PATCH 5/8] Switch dynamic disks API to HTTP The following endpoints were added: /dynamic_disks/provide /dynamic_disks/detach /dynamic_disks/delete Remove all NATS dynamic disks related code --- jobs/nats/templates/nats.cfg.erb | 2 +- spec/nats_templates_spec.rb | 2 +- src/bosh-director/bin/bosh-director | 2 - src/bosh-director/lib/bosh/director.rb | 4 +- .../lib/bosh/director/agent_client.rb | 8 + .../controllers/dynamic_disks_controller.rb | 54 +++++ .../bosh/director/api/route_configuration.rb | 1 + .../lib/bosh/director/api_nats/api_nats.rb | 55 ----- .../api_nats/dynamic_disk_controller.rb | 54 ----- .../steps/detach_dynamic_disk_step.rb | 4 + .../lib/bosh/director/disk_deleter.rb | 1 - .../jobs/dynamic_disks/delete_dynamic_disk.rb | 8 +- .../jobs/dynamic_disks/detach_dynamic_disk.rb | 14 +- .../dynamic_disks/provide_dynamic_disk.rb | 29 +-- .../jobs/helpers/dynamic_disk_helpers.rb | 4 - .../bosh/director/permission_authorizer.rb | 2 + .../unit/bosh/director/agent_client_spec.rb | 26 +++ .../dynamic_disks_controller_spec.rb | 203 ++++++++++++++++++ .../api_nats/dynamic_disk_controller_spec.rb | 199 ----------------- .../steps/detach_dynamic_disk_step_spec.rb | 8 +- .../dynamic_disks/delete_dynamic_disk_spec.rb | 21 +- .../dynamic_disks/detach_dynamic_disk_spec.rb | 29 +-- .../provide_dynamic_disk_spec.rb | 72 +++---- .../lib/nats_sync/nats_auth_config.rb | 5 +- 24 files changed, 372 insertions(+), 435 deletions(-) create mode 100644 src/bosh-director/lib/bosh/director/api/controllers/dynamic_disks_controller.rb delete mode 100644 src/bosh-director/lib/bosh/director/api_nats/api_nats.rb delete mode 100644 src/bosh-director/lib/bosh/director/api_nats/dynamic_disk_controller.rb create mode 100644 src/bosh-director/spec/unit/bosh/director/api/controllers/dynamic_disks_controller_spec.rb delete mode 100644 src/bosh-director/spec/unit/bosh/director/api_nats/dynamic_disk_controller_spec.rb diff --git a/jobs/nats/templates/nats.cfg.erb b/jobs/nats/templates/nats.cfg.erb index a1f6477854a..e9d715c7478 100644 --- a/jobs/nats/templates/nats.cfg.erb +++ b/jobs/nats/templates/nats.cfg.erb @@ -12,7 +12,7 @@ authorization { { user: "C=USA, O=Cloud Foundry, CN=default.director.bosh-internal" permissions: { - publish: [ "agent.*", "agent.inbox.>", "hm.director.alert" ] + publish: [ "agent.*", "hm.director.alert" ] subscribe: [ "director.>" ] } }, diff --git a/spec/nats_templates_spec.rb b/spec/nats_templates_spec.rb index 1280f4b10b3..b447de8f6aa 100644 --- a/spec/nats_templates_spec.rb +++ b/spec/nats_templates_spec.rb @@ -90,7 +90,7 @@ user: "C=USA, O=Cloud Foundry, CN=default.director.bosh-internal" permissions: { publish: [ "agent.*", "hm.director.alert" ] - subscribe: [ "director.>", "director.agent.disk.*" ] + subscribe: [ "director.>" ] } }, { diff --git a/src/bosh-director/bin/bosh-director b/src/bosh-director/bin/bosh-director index d5c3378ccf9..536a1bc16e4 100755 --- a/src/bosh-director/bin/bosh-director +++ b/src/bosh-director/bin/bosh-director @@ -37,8 +37,6 @@ rack_app = Puma::Rack::Builder.app do route_configuration.controllers.each do |route, controller| map(route) { run controller } end - - Bosh::Director::ApiNats.setup_events end puma_configuration = Puma::Configuration.new do |user_config| diff --git a/src/bosh-director/lib/bosh/director.rb b/src/bosh-director/lib/bosh/director.rb index 8d80b241c4d..f38efe42f7d 100644 --- a/src/bosh-director/lib/bosh/director.rb +++ b/src/bosh-director/lib/bosh/director.rb @@ -255,6 +255,7 @@ module Bosh::Director require 'bosh/director/api/controllers/deployments_controller' require 'bosh/director/api/controllers/disks_controller' require 'bosh/director/api/controllers/director_controller' +require 'bosh/director/api/controllers/dynamic_disks_controller' require 'bosh/director/api/controllers/networks_controller' require 'bosh/director/api/controllers/orphan_disks_controller' require 'bosh/director/api/controllers/orphaned_vms_controller' @@ -283,9 +284,6 @@ module Bosh::Director require 'bosh/director/api/controllers/deployed_variables_controller' require 'bosh/director/api/route_configuration' -require 'bosh/director/api_nats/api_nats' -require 'bosh/director/api_nats/dynamic_disk_controller' - require 'bosh/director/step_executor' require 'bosh/director/metrics_collector' require 'bosh/director/strip_deployments_middleware_collector' diff --git a/src/bosh-director/lib/bosh/director/agent_client.rb b/src/bosh-director/lib/bosh/director/agent_client.rb index c71d1fa72d6..8cbcaba5297 100644 --- a/src/bosh-director/lib/bosh/director/agent_client.rb +++ b/src/bosh-director/lib/bosh/director/agent_client.rb @@ -115,6 +115,14 @@ def remove_persistent_disk(*args) safe_send_message(:remove_persistent_disk, *args) end + def add_dynamic_disk(*args) + safe_send_message(:add_dynamic_disk, *args) + end + + def remove_dynamic_disk(*args) + safe_send_message(:remove_dynamic_disk, *args) + end + def shutdown fire_and_forget(:shutdown) end 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 new file mode 100644 index 00000000000..f8000daf915 --- /dev/null +++ b/src/bosh-director/lib/bosh/director/api/controllers/dynamic_disks_controller.rb @@ -0,0 +1,54 @@ +require 'bosh/director/api/controllers/base_controller' + +module Bosh::Director + module Api::Controllers + class DynamicDisksController < BaseController + include ValidationHelper + + post '/provide', 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::ProvideDynamicDisk, + 'provide dynamic disk', + [instance_id, disk_name, disk_pool_name, disk_size, metadata] + ) + + redirect "/tasks/#{task.id}" + end + + post '/:disk_name/detach', scope: :manage_dynamic_disks do + disk_name = safe_property(params, 'disk_name', class: String, min_length: 1) + + task = JobQueue.new.enqueue( + current_user, + Jobs::DynamicDisks::DetachDynamicDisk, + 'detach dynamic disk', + [disk_name] + ) + + redirect "/tasks/#{task.id}" + end + + delete '/:disk_name', scope: :manage_dynamic_disks do + disk_name = safe_property(params, 'disk_name', class: String, min_length: 1) + + task = JobQueue.new.enqueue( + current_user, + Jobs::DynamicDisks::DeleteDynamicDisk, + 'delete dynamic disk', + [disk_name] + ) + + redirect "/tasks/#{task.id}" + end + end + end +end diff --git a/src/bosh-director/lib/bosh/director/api/route_configuration.rb b/src/bosh-director/lib/bosh/director/api/route_configuration.rb index 10098a00215..b93e3c7389c 100644 --- a/src/bosh-director/lib/bosh/director/api/route_configuration.rb +++ b/src/bosh-director/lib/bosh/director/api/route_configuration.rb @@ -18,6 +18,7 @@ def controllers controllers['/deployment_configs'] = Bosh::Director::Api::Controllers::DeploymentConfigsController.new(@config) controllers['/disks'] = Bosh::Director::Api::Controllers::DisksController.new(@config) controllers['/director'] = Bosh::Director::Api::Controllers::DirectorController.new(@config) + controllers['/dynamic_disks'] = Bosh::Director::Api::Controllers::DynamicDisksController.new(@config) controllers['/networks'] = Bosh::Director::Api::Controllers::NetworksController.new(@config) controllers['/orphan_disks'] = Bosh::Director::Api::Controllers::OrphanDisksController.new(@config) controllers['/orphaned_vms'] = Bosh::Director::Api::Controllers::OrphanedVmsController.new(@config) diff --git a/src/bosh-director/lib/bosh/director/api_nats/api_nats.rb b/src/bosh-director/lib/bosh/director/api_nats/api_nats.rb deleted file mode 100644 index beb06c1e327..00000000000 --- a/src/bosh-director/lib/bosh/director/api_nats/api_nats.rb +++ /dev/null @@ -1,55 +0,0 @@ -module Bosh::Director - module ApiNats - extend ValidationHelper - - USERNAME = "bosh-agent".freeze - - def setup_events - disk_controller = DynamicDiskController.new - - Config.nats_rpc.nats.subscribe('director.agent.disk.provide.*') do |payload, reply, subject| - payload = parse_payload(payload) - disk_controller.handle_provide_disk_request(parse_agent_id(subject), reply, payload) - rescue => e - Config.nats_rpc.send_message(reply, { "error" => e.message }) - raise - end - - Config.nats_rpc.nats.subscribe('director.agent.disk.detach.*') do |payload, reply, subject| - payload = parse_payload(payload) - disk_controller.handle_detach_disk_request(parse_agent_id(subject), reply, payload) - rescue => e - Config.nats_rpc.send_message(reply, { "error" => e.message }) - raise - end - - Config.nats_rpc.nats.subscribe('director.agent.disk.delete.*') do |payload, reply, _subject| - payload = parse_payload(payload) - disk_controller.handle_delete_disk_request(reply, payload) - rescue => e - Config.nats_rpc.send_message(reply, { "error" => e.message }) - raise - end - end - - private - - def parse_agent_id(subject) - # subject: director.agent.disk.provide.agent_id - agent_id = subject.split('.', 5).last - raise 'Subject must include agent_id' if agent_id.empty? - return agent_id - end - - def parse_payload(payload) - case payload - when String - return JSON.parse(payload) - when Hash - return payload - else - raise "Payload must be a JSON string or hash" - end - end - end -end \ No newline at end of file diff --git a/src/bosh-director/lib/bosh/director/api_nats/dynamic_disk_controller.rb b/src/bosh-director/lib/bosh/director/api_nats/dynamic_disk_controller.rb deleted file mode 100644 index 4911f68b123..00000000000 --- a/src/bosh-director/lib/bosh/director/api_nats/dynamic_disk_controller.rb +++ /dev/null @@ -1,54 +0,0 @@ -module Bosh::Director - module ApiNats - class DynamicDiskController - include ValidationHelper - - USERNAME = "bosh-agent".freeze - - def handle_provide_disk_request(agent_id, reply, payload) - disk_name = safe_property(payload, "disk_name", class: String, min_length: 1) - disk_pool_name = safe_property(payload, "disk_pool_name", class: String, min_length: 1) - disk_size = safe_property(payload, "disk_size", class: Integer, min: 1) - metadata = safe_property(payload, "metadata", class: Hash, optional: true) - - JobQueue.new.enqueue( - USERNAME, - Jobs::DynamicDisks::ProvideDynamicDisk, - 'provide dynamic disk', - [agent_id, reply, disk_name, disk_pool_name, disk_size, metadata] - ) - rescue => e - Config.nats_rpc.send_message(reply, { "error" => e.message }) - raise - end - - def handle_detach_disk_request(agent_id, reply, payload) - disk_name = safe_property(payload, "disk_name", class: String, min_length: 1) - - JobQueue.new.enqueue( - USERNAME, - Jobs::DynamicDisks::DetachDynamicDisk, - 'detach dynamic disk', - [reply, disk_name] - ) - rescue => e - Config.nats_rpc.send_message(reply, { "error" => e.message }) - raise - end - - def handle_delete_disk_request(reply, payload) - disk_name = safe_property(payload, "disk_name", class: String, min_length: 1) - - JobQueue.new.enqueue( - USERNAME, - Jobs::DynamicDisks::DeleteDynamicDisk, - 'delete dynamic disk', - [reply, disk_name] - ) - rescue => e - Config.nats_rpc.send_message(reply, { "error" => e.message }) - raise - end - end - end -end \ No newline at end of file diff --git a/src/bosh-director/lib/bosh/director/deployment_plan/steps/detach_dynamic_disk_step.rb b/src/bosh-director/lib/bosh/director/deployment_plan/steps/detach_dynamic_disk_step.rb index c68f5d0d652..d03ffcc4eb4 100644 --- a/src/bosh-director/lib/bosh/director/deployment_plan/steps/detach_dynamic_disk_step.rb +++ b/src/bosh-director/lib/bosh/director/deployment_plan/steps/detach_dynamic_disk_step.rb @@ -13,6 +13,10 @@ def perform(_report) cloud = CloudFactory.create.get(@disk.cpi, @disk.vm.stemcell_api_version) @logger.info("Detaching dynamic disk #{@disk.disk_cid}") + instance_name = @disk.vm.instance.nil? ? nil : @disk.vm.instance.name + agent_client = AgentClient.with_agent_id(@disk.vm.agent_id, instance_name) + agent_client.remove_dynamic_disk(@disk.disk_cid) + cloud.detach_disk(@disk.vm.cid, @disk.disk_cid) @disk.update(vm_id: nil) rescue Bosh::Clouds::DiskNotAttached diff --git a/src/bosh-director/lib/bosh/director/disk_deleter.rb b/src/bosh-director/lib/bosh/director/disk_deleter.rb index 39497ae2d8b..8d5c3422bd4 100644 --- a/src/bosh-director/lib/bosh/director/disk_deleter.rb +++ b/src/bosh-director/lib/bosh/director/disk_deleter.rb @@ -1,5 +1,4 @@ module Bosh::Director - # Coordinates the safe deletion of an instance and all associates resources. class DiskDeleter def initialize(logger, disk_manager, options = {}) @disk_manager = disk_manager diff --git a/src/bosh-director/lib/bosh/director/jobs/dynamic_disks/delete_dynamic_disk.rb b/src/bosh-director/lib/bosh/director/jobs/dynamic_disks/delete_dynamic_disk.rb index 02990e57369..17dc3a61b33 100644 --- a/src/bosh-director/lib/bosh/director/jobs/dynamic_disks/delete_dynamic_disk.rb +++ b/src/bosh-director/lib/bosh/director/jobs/dynamic_disks/delete_dynamic_disk.rb @@ -9,9 +9,7 @@ def self.job_type :delete_dynamic_disk end - def initialize(reply, disk_name) - super() - @reply = reply + def initialize(disk_name) @disk_name = disk_name end @@ -26,10 +24,6 @@ def perform disk_model.destroy "deleted disk `#{disk_cid}`" - rescue => e - raise e - ensure - nats_rpc.send_message(@reply, { 'error' => e&.message }) end end end diff --git a/src/bosh-director/lib/bosh/director/jobs/dynamic_disks/detach_dynamic_disk.rb b/src/bosh-director/lib/bosh/director/jobs/dynamic_disks/detach_dynamic_disk.rb index e3139899c9c..ec5b3bbdbd7 100644 --- a/src/bosh-director/lib/bosh/director/jobs/dynamic_disks/detach_dynamic_disk.rb +++ b/src/bosh-director/lib/bosh/director/jobs/dynamic_disks/detach_dynamic_disk.rb @@ -9,9 +9,7 @@ def self.job_type :detach_dynamic_disk end - def initialize(reply, disk_name) - super() - @reply = reply + def initialize( disk_name) @disk_name = disk_name end @@ -23,17 +21,19 @@ def perform cloud = Bosh::Director::CloudFactory.create.get(disk_model.cpi, disk_model.vm.stemcell_api_version) vm_cid = disk_model.vm.cid + + instance_name = disk_model.vm.instance.nil? ? nil : disk_model.vm.instance.name + agent_client = AgentClient.with_agent_id(disk_model.vm.agent_id, instance_name) + agent_client.remove_dynamic_disk(disk_model.disk_cid) + cloud.detach_disk(disk_model.vm.cid, disk_model.disk_cid) + disk_model.update(vm_id: nil) "detached disk `#{disk_model.disk_cid}` from vm `#{vm_cid}`" rescue Bosh::Clouds::DiskNotAttached disk_model.update(vm_id: nil) "disk `#{disk_model.disk_cid}` was already detached" - rescue => e - raise e - ensure - nats_rpc.send_message(@reply, { 'error' => e&.message }) end end end diff --git a/src/bosh-director/lib/bosh/director/jobs/dynamic_disks/provide_dynamic_disk.rb b/src/bosh-director/lib/bosh/director/jobs/dynamic_disks/provide_dynamic_disk.rb index 64a07f91eca..9784dc1d596 100644 --- a/src/bosh-director/lib/bosh/director/jobs/dynamic_disks/provide_dynamic_disk.rb +++ b/src/bosh-director/lib/bosh/director/jobs/dynamic_disks/provide_dynamic_disk.rb @@ -9,11 +9,8 @@ def self.job_type :provide_dynamic_disk end - def initialize(agent_id, reply, disk_name, disk_pool_name, disk_size, metadata) - super() - @agent_id = agent_id - @reply = reply - + 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 @@ -21,10 +18,13 @@ def initialize(agent_id, reply, disk_name, disk_pool_name, disk_size, metadata) end def perform - vm = Models::Vm.find(agent_id: @agent_id) - raise "vm for agent `#{@agent_id}` not found" if vm.nil? + instance = Models::Instance.find(id: @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(vm.instance, @disk_pool_name) + 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) @@ -33,7 +33,7 @@ def perform disk_model = Models::DynamicDisk.create( name: @disk_name, disk_cid: disk_cid, - deployment_id: vm.instance.deployment.id, + deployment_id: instance.deployment.id, size: @disk_size, disk_pool_name: @disk_pool_name, cpi: vm.cpi, @@ -48,17 +48,10 @@ def perform MetadataUpdater.build.update_dynamic_disk_metadata(cloud, disk_model, @metadata) end - response = { - 'error' => nil, - 'disk_name' => @disk_name, - 'disk_hint' => disk_hint, - } - nats_rpc.send_message(@reply, response) + agent_client = AgentClient.with_agent_id(vm.agent_id, instance.name) + agent_client.add_dynamic_disk(disk_model.disk_cid, disk_hint) "attached disk `#{@disk_name}` to `#{vm.cid}` in deployment `#{vm.instance.deployment.name}`" - rescue => e - nats_rpc.send_message(@reply, { 'error' => e.message }) - raise e 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 78ca7421b36..200bb386fc7 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 @@ -12,10 +12,6 @@ def find_disk_cloud_properties(instance, disk_pool_name) cloud_config_disk_type.cloud_properties end - - def nats_rpc - Config.nats_rpc - end end end end \ No newline at end of file diff --git a/src/bosh-director/lib/bosh/director/permission_authorizer.rb b/src/bosh-director/lib/bosh/director/permission_authorizer.rb index 9de640a130b..f9fe578d217 100644 --- a/src/bosh-director/lib/bosh/director/permission_authorizer.rb +++ b/src/bosh-director/lib/bosh/director/permission_authorizer.rb @@ -88,6 +88,8 @@ def get_director_scopes(expected_scope, permission, user_scopes) expected_scope << bosh_team_admin_scopes(user_scopes) when :read, :upload_releases, :upload_stemcells expected_scope << director_permissions[permission] + when :manage_dynamic_disks + expected_scope << bosh_team_admin_scopes(user_scopes) else raise ArgumentError, "Unexpected permission for director: #{permission}" end diff --git a/src/bosh-director/spec/unit/bosh/director/agent_client_spec.rb b/src/bosh-director/spec/unit/bosh/director/agent_client_spec.rb index 411c93cc961..e22c01a04aa 100644 --- a/src/bosh-director/spec/unit/bosh/director/agent_client_spec.rb +++ b/src/bosh-director/spec/unit/bosh/director/agent_client_spec.rb @@ -136,6 +136,8 @@ def self.it_acts_as_message_with_timeout(message_name) it_acts_as_asynchronous_message :start it_acts_as_asynchronous_message :add_persistent_disk it_acts_as_asynchronous_message :remove_persistent_disk + it_acts_as_asynchronous_message :add_dynamic_disk + it_acts_as_asynchronous_message :remove_dynamic_disk end describe 'update_settings' do @@ -288,6 +290,30 @@ def self.it_acts_as_message_with_timeout(message_name) end end + describe 'add_dynamic_disk' do + it 'logs a warning if the message is not handled' do + allow(client).to receive(:handle_method).and_raise(RpcRemoteException, 'unknown message add_dynamic_disk') + + expect(Config.logger).to receive(:warn).with( + "Ignoring add_dynamic_disk 'unknown message' error from the agent: "\ + '#', + ) + expect { client.add_dynamic_disk('diskCID', 'hint') }.to_not raise_error + end + end + + describe 'remove_dynamic_disk' do + it 'logs a warning if the message is not handled' do + allow(client).to receive(:handle_method).and_raise(RpcRemoteException, 'unknown message remove_dynamic_disk') + + expect(Config.logger).to receive(:warn).with( + "Ignoring remove_dynamic_disk 'unknown message' error from the agent: "\ + '#', + ) + expect { client.remove_dynamic_disk('diskCID') }.to_not raise_error + end + end + context 'task can time out' do it_acts_as_message_with_timeout :stop 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 new file mode 100644 index 00000000000..6ec777337c5 --- /dev/null +++ b/src/bosh-director/spec/unit/bosh/director/api/controllers/dynamic_disks_controller_spec.rb @@ -0,0 +1,203 @@ +require 'spec_helper' +require 'rack/test' + +module Bosh::Director + module Api + describe Controllers::DynamicDisksController do + include Rack::Test::Methods + + subject(:app) { linted_rack_app(described_class.new(config)) } + let(:config) do + config = Config.load_hash(SpecHelper.director_config_hash) + identity_provider = Support::TestIdentityProvider.new(config.get_uuid_provider) + allow(config).to receive(:identity_provider).and_return(identity_provider) + config + end + before { App.new(config) } + + let(:instance_id) { 'fake-instance-id' } + let(:disk_pool_name) { 'fake_disk_pool_name' } + let(:disk_name) { 'fake_disk_name' } + let(:disk_size) { 1000 } + let(:metadata) { { 'some-key' => 'some-value' } } + + describe 'POST', '/provide' 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('/provide', content, {'CONTENT_TYPE' => 'application/json'}).status).to eq(401) + end + end + + context 'user has admin permissions' do + before { authorize 'admin', 'admin' } + + it 'enqueues a ProvideDynamicDisk task' do + expect_any_instance_of(Bosh::Director::JobQueue).to receive(:enqueue).with( + 'admin', + Jobs::DynamicDisks::ProvideDynamicDisk, + 'provide dynamic disk', + [instance_id, disk_name, disk_pool_name, disk_size, metadata], + ).and_call_original + + post '/provide', content, { 'CONTENT_TYPE' => 'application/json' } + + expect_redirect_to_queued_task(last_response) + end + + context 'content is invalid' do + context 'disk_pool_name is nil' do + let(:disk_pool_name) { nil } + + it 'raises an error' do + post '/provide', content, { 'CONTENT_TYPE' => 'application/json' } + + expect(last_response.status).to eq(400) + expect(JSON.parse(last_response.body)).to eq( + 'code' => 40000, + 'description' => "Property 'disk_pool_name' value (nil) did not match the required type 'String'", + ) + end + end + + context 'disk_pool_name is empty' do + let(:disk_pool_name) { '' } + + it 'raises an error' do + post '/provide', content, { 'CONTENT_TYPE' => 'application/json' } + + expect(last_response.status).to eq(400) + expect(JSON.parse(last_response.body)).to eq( + 'code' => 40002, + 'description' => "'disk_pool_name' length (0) should be greater than 1", + ) + end + end + + context 'disk_name is nil' do + let(:disk_name) { nil } + + it 'raises an error' do + post '/provide', content, { 'CONTENT_TYPE' => 'application/json' } + + expect(last_response.status).to eq(400) + expect(JSON.parse(last_response.body)).to eq( + 'code' => 40000, + 'description' => "Property 'disk_name' value (nil) did not match the required type 'String'", + ) + end + end + + context 'disk_name is empty' do + let(:disk_name) { '' } + + it 'raises an error' do + post '/provide', content, { 'CONTENT_TYPE' => 'application/json' } + + expect(last_response.status).to eq(400) + expect(JSON.parse(last_response.body)).to eq( + 'code' => 40002, + 'description' => "'disk_name' length (0) should be greater than 1", + ) + end + end + + context 'disk_size is empty' do + let(:disk_size) { nil } + + it 'raises an error' do + post '/provide', content, { 'CONTENT_TYPE' => 'application/json' } + + expect(last_response.status).to eq(400) + expect(JSON.parse(last_response.body)).to eq( + 'code' => 40000, + 'description' => "Property 'disk_size' value (nil) did not match the required type 'Integer'", + ) + end + end + + context 'disk_size is 0' do + let(:disk_size) { 0 } + + it 'raises an error' do + post '/provide', content, { 'CONTENT_TYPE' => 'application/json' } + + expect(last_response.status).to eq(400) + expect(JSON.parse(last_response.body)).to eq( + 'code' => 40002, + 'description' => "'disk_size' value (0) should be greater than 1", + ) + end + end + end + end + end + + describe 'POST', '/:disk_name/detach' do + context 'when user is reader' do + before { basic_authorize('reader', 'reader') } + + it 'forbids access' do + expect(post('/disk_name/detach').status).to eq(401) + end + end + + context 'user has admin permissions' do + before { authorize 'admin', 'admin' } + + it 'enqueues a ProvideDynamicDisk task' do + expect_any_instance_of(Bosh::Director::JobQueue).to receive(:enqueue).with( + 'admin', + Jobs::DynamicDisks::DetachDynamicDisk, + 'detach dynamic disk', + ['disk_name'], + ).and_call_original + + post '/disk_name/detach' + + expect_redirect_to_queued_task(last_response) + end + end + end + + describe 'DELETE', '/:disk_name' do + context 'when user is reader' do + before { basic_authorize('reader', 'reader') } + + it 'forbids access' do + expect(delete('/disk_name').status).to eq(401) + end + end + + context 'user has admin permissions' do + before { authorize 'admin', 'admin' } + + it 'enqueues a ProvideDynamicDisk task' do + expect_any_instance_of(Bosh::Director::JobQueue).to receive(:enqueue).with( + 'admin', + Jobs::DynamicDisks::DeleteDynamicDisk, + 'delete dynamic disk', + ['disk_name'], + ).and_call_original + + delete '/disk_name' + + expect_redirect_to_queued_task(last_response) + end + + end + end + end + end +end \ No newline at end of file diff --git a/src/bosh-director/spec/unit/bosh/director/api_nats/dynamic_disk_controller_spec.rb b/src/bosh-director/spec/unit/bosh/director/api_nats/dynamic_disk_controller_spec.rb deleted file mode 100644 index a35fd8fc086..00000000000 --- a/src/bosh-director/spec/unit/bosh/director/api_nats/dynamic_disk_controller_spec.rb +++ /dev/null @@ -1,199 +0,0 @@ -require 'spec_helper' - -module Bosh::Director - module ApiNats - describe DynamicDiskController do - subject(:controller) { DynamicDiskController.new } - let(:nats_rpc) { instance_double('Bosh::Director::NatsRpc') } - let(:task) { instance_double('Bosh::Director::Models::Task', id: 1) } - let(:job_queue) { instance_double('Bosh::Director::JobQueue', enqueue: task) } - let(:agent_id) { 'fake_agent_id' } - let(:reply) { 'inbox.fake' } - let(:disk_pool_name) { 'fake_disk_pool_name' } - let(:disk_name) { 'fake_disk_name' } - let(:disk_size) { 1000 } - let(:metadata) { { 'some-key' => 'some-value' } } - - before do - allow(Config).to receive(:nats_rpc).and_return(nats_rpc) - allow(JobQueue).to receive(:new).and_return(job_queue) - end - - describe 'handle_provide_disk_request' do - let(:payload) do - { - 'disk_pool_name' => disk_pool_name, - 'disk_name' => disk_name, - 'disk_size' => disk_size, - 'metadata' => metadata - }.compact - end - - it 'enqueues a ProvideDynamicDisk task' do - expect(job_queue).to receive(:enqueue).with( - 'bosh-agent', - Jobs::DynamicDisks::ProvideDynamicDisk, - 'provide dynamic disk', - [agent_id, reply, disk_name, disk_pool_name, disk_size, metadata], - ).and_return(task) - - controller.handle_provide_disk_request(agent_id, reply, payload) - end - - context 'payload is invalid' do - context 'disk_pool_name is nil' do - let(:disk_pool_name) { nil } - - it 'raises an error' do - expect(nats_rpc).to receive(:send_message).with(reply, hash_including({ 'error' => a_string_matching("Required property 'disk_pool_name'") })) - expect { - controller.handle_provide_disk_request(agent_id, reply, payload) - }.to raise_error(ValidationMissingField) - end - end - - context 'disk_pool_name is empty' do - let(:disk_pool_name) { "" } - - it 'raises an error' do - expect(nats_rpc).to receive(:send_message).with(reply, hash_including({ 'error' => a_string_matching("'disk_pool_name' length") })) - expect { - controller.handle_provide_disk_request(agent_id, reply, payload) - }.to raise_error(ValidationViolatedMin) - end - end - - context 'disk_name is nil' do - let(:disk_name) { nil } - - it 'raises an error' do - expect(nats_rpc).to receive(:send_message).with(reply, hash_including({ 'error' => a_string_matching("Required property 'disk_name'") })) - expect { - controller.handle_provide_disk_request(agent_id, reply, payload) - }.to raise_error(ValidationMissingField) - end - end - - context 'disk_name is empty' do - let(:disk_name) { "" } - - it 'raises an error' do - expect(nats_rpc).to receive(:send_message).with(reply, hash_including({ 'error' => a_string_matching("'disk_name' length") })) - expect { - controller.handle_provide_disk_request(agent_id, reply, payload) - }.to raise_error(ValidationViolatedMin) - end - end - - context 'disk_size is empty' do - let(:disk_size) { nil } - - it 'raises an error' do - expect(nats_rpc).to receive(:send_message).with(reply, hash_including({ 'error' => a_string_matching("Required property 'disk_size'") })) - expect { - controller.handle_provide_disk_request(agent_id, reply, payload) - }.to raise_error(ValidationMissingField) - end - end - - context 'disk_size is 0' do - let(:disk_size) { 0 } - - it 'raises an error' do - expect(nats_rpc).to receive(:send_message).with(reply, hash_including({ 'error' => a_string_matching("'disk_size' value") })) - expect { - controller.handle_provide_disk_request(agent_id, reply, payload) - }.to raise_error(ValidationViolatedMin) - end - end - end - end - - describe 'handle_detach_disk_request' do - let(:payload) do - { - 'disk_name' => disk_name, - }.compact - end - - it 'enqueues a DetachDynamicDisk task' do - expect(job_queue).to receive(:enqueue).with( - 'bosh-agent', - Jobs::DynamicDisks::DetachDynamicDisk, - 'detach dynamic disk', - [reply, disk_name], - ).and_return(task) - - controller.handle_detach_disk_request(agent_id, reply, payload) - end - - context 'payload is invalid' do - context 'disk_name is nil' do - let(:disk_name) { nil } - - it 'raises an error' do - expect(nats_rpc).to receive(:send_message).with(reply, hash_including({ 'error' => a_string_matching("Required property 'disk_name'") })) - expect { - controller.handle_detach_disk_request(agent_id, reply, payload) - }.to raise_error(ValidationMissingField) - end - end - - context 'disk_name is empty' do - let(:disk_name) { "" } - - it 'raises an error' do - expect(nats_rpc).to receive(:send_message).with(reply, hash_including({ 'error' => a_string_matching("'disk_name' length") })) - expect { - controller.handle_detach_disk_request(agent_id, reply, payload) - }.to raise_error(ValidationViolatedMin) - end - end - end - end - - describe 'handle_delete_disk_request' do - let(:payload) do - { - 'disk_name' => disk_name, - }.compact - end - - it 'enqueues a DeleteDynamicDisk task' do - expect(job_queue).to receive(:enqueue).with( - 'bosh-agent', - Jobs::DynamicDisks::DeleteDynamicDisk, - 'delete dynamic disk', - [reply, disk_name], - ).and_return(task) - - controller.handle_delete_disk_request(reply, payload) - end - - context 'payload is invalid' do - context 'disk_name is nil' do - let(:disk_name) { nil } - - it 'raises an error' do - expect(nats_rpc).to receive(:send_message).with(reply, hash_including({ 'error' => a_string_matching("Required property 'disk_name'") })) - expect { - controller.handle_delete_disk_request(reply, payload) - }.to raise_error(ValidationMissingField) - end - end - - context 'disk_name is empty' do - let(:disk_name) { "" } - - it 'raises an error' do - expect(nats_rpc).to receive(:send_message).with(reply, hash_including({ 'error' => a_string_matching("'disk_name' length") })) - expect { - controller.handle_delete_disk_request(reply, payload) - }.to raise_error(ValidationViolatedMin) - end - end - end - end - end - end -end \ No newline at end of file diff --git a/src/bosh-director/spec/unit/bosh/director/deployment_plan/steps/detach_dynamic_disk_step_spec.rb b/src/bosh-director/spec/unit/bosh/director/deployment_plan/steps/detach_dynamic_disk_step_spec.rb index 4a7a1d7e274..a995e20b4de 100644 --- a/src/bosh-director/spec/unit/bosh/director/deployment_plan/steps/detach_dynamic_disk_step_spec.rb +++ b/src/bosh-director/spec/unit/bosh/director/deployment_plan/steps/detach_dynamic_disk_step_spec.rb @@ -6,20 +6,25 @@ module Steps describe DetachDynamicDiskStep do subject(:step) { DetachDynamicDiskStep.new(disk) } - let!(:vm) { FactoryBot.create(:models_vm, stemcell_api_version: 25) } + let!(:vm) { FactoryBot.create(:models_vm, instance: instance, stemcell_api_version: 25) } + let(:instance) { FactoryBot.create(:models_instance) } let!(:disk) { FactoryBot.create(:models_dynamic_disk, vm: vm, name: 'disk-name') } let(:cloud_factory) { instance_double(CloudFactory) } let(:cloud) { instance_double(Bosh::Clouds::ExternalCpi) } let(:report) { Stages::Report.new } + let(:agent_client) { instance_double(AgentClient) } before do allow(CloudFactory).to receive(:create).and_return(cloud_factory) allow(cloud_factory).to receive(:get).with(disk&.cpi, 25).once.and_return(cloud) allow(cloud).to receive(:detach_disk) + allow(agent_client).to receive(:remove_dynamic_disk) + allow(AgentClient).to receive(:with_agent_id).with(vm.agent_id, instance.name).and_return(agent_client) end it 'calls out to cpi associated with disk to detach disk' do expect(cloud_factory).to receive(:get).with(disk&.cpi, 25).once.and_return(cloud) + expect(agent_client).to receive(:remove_dynamic_disk).with(disk.disk_cid) expect(cloud).to receive(:detach_disk).with(vm.cid, disk.disk_cid) step.perform(report) @@ -58,6 +63,7 @@ module Steps let(:disk) { nil } it 'does not perform any cloud actions' do + expect(agent_client).to_not receive(:remove_dynamic_disk) expect(cloud).to_not receive(:detach_disk) step.perform(report) diff --git a/src/bosh-director/spec/unit/bosh/director/jobs/dynamic_disks/delete_dynamic_disk_spec.rb b/src/bosh-director/spec/unit/bosh/director/jobs/dynamic_disks/delete_dynamic_disk_spec.rb index 9ad19c344ed..51a91d086b9 100644 --- a/src/bosh-director/spec/unit/bosh/director/jobs/dynamic_disks/delete_dynamic_disk_spec.rb +++ b/src/bosh-director/spec/unit/bosh/director/jobs/dynamic_disks/delete_dynamic_disk_spec.rb @@ -2,20 +2,16 @@ module Bosh::Director describe Jobs::DynamicDisks::DeleteDynamicDisk do - let(:agent_id) { 'fake-agent-id' } - let(:reply) { 'inbox.fake' } let(:disk_name) { 'fake-disk-name' } let(:disk_cid) { 'fake-disk-cid' } let(:disk_pool_name) { 'fake-disk-pool-name' } - let(:nats_rpc) { instance_double(Bosh::Director::NatsRpc) } let(:cloud) { instance_double(Bosh::Clouds::ExternalCpi) } - let(:delete_dynamic_disk_job) { Jobs::DynamicDisks::DeleteDynamicDisk.new(reply, disk_name) } - let!(:vm) { FactoryBot.create(:models_vm, agent_id: agent_id, cid: 'fake-vm-cid') } + let(:delete_dynamic_disk_job) { Jobs::DynamicDisks::DeleteDynamicDisk.new(disk_name) } + let!(:vm) { FactoryBot.create(:models_vm) } let(:cloud_factory) { instance_double(Bosh::Director::CloudFactory, get: cloud) } before do - allow(Config).to receive(:nats_rpc).and_return(nats_rpc) allow(Bosh::Director::Config).to receive(:name).and_return('fake-director-name') allow(Bosh::Director::Config).to receive(:cloud_options).and_return('provider' => { 'path' => '/path/to/default/cpi' }) allow(Bosh::Director::Config).to receive(:preferred_cpi_api_version).and_return(2) @@ -36,9 +32,6 @@ module Bosh::Director end it 'deletes the disk' do - expect(nats_rpc).to receive(:send_message).with(reply, { - 'error' => nil - }) expect(delete_dynamic_disk_job.perform).to eq("deleted disk `#{disk_cid}`") expect(Models::DynamicDisk.where(disk_cid: disk_cid).count).to eq(0) end @@ -61,9 +54,6 @@ module Bosh::Director it 'deletes the disk' do expect(cloud).to receive(:delete_disk).with(disk_cid) - expect(nats_rpc).to receive(:send_message).with(reply, { - 'error' => nil - }) expect(delete_dynamic_disk_job.perform).to eq("deleted disk `#{disk_cid}`") expect(Models::DynamicDisk.where(disk_cid: disk_cid).count).to eq(0) end @@ -71,10 +61,6 @@ module Bosh::Director context 'when deleting disk returns an error' do it 'returns an error from delete_disk call' do expect(cloud).to receive(:delete_disk).with(disk_cid).and_raise('some-error') - - expect(nats_rpc).to receive(:send_message).with(reply, { - 'error' => 'some-error' - }) expect { delete_dynamic_disk_job.perform }.to raise_error('some-error') end end @@ -83,9 +69,6 @@ module Bosh::Director context 'when disk does not exist' do it 'does not delete disk and returns no error' do expect(cloud).not_to receive(:delete_disk).with(disk_cid) - expect(nats_rpc).to receive(:send_message).with(reply, { - 'error' => nil - }) expect(delete_dynamic_disk_job.perform).to eq("disk with name `#{disk_name}` was already deleted") end end diff --git a/src/bosh-director/spec/unit/bosh/director/jobs/dynamic_disks/detach_dynamic_disk_spec.rb b/src/bosh-director/spec/unit/bosh/director/jobs/dynamic_disks/detach_dynamic_disk_spec.rb index 4a9767adc4a..86fac84ad56 100644 --- a/src/bosh-director/spec/unit/bosh/director/jobs/dynamic_disks/detach_dynamic_disk_spec.rb +++ b/src/bosh-director/spec/unit/bosh/director/jobs/dynamic_disks/detach_dynamic_disk_spec.rb @@ -2,25 +2,24 @@ module Bosh::Director describe Jobs::DynamicDisks::DetachDynamicDisk do - let(:agent_id) { 'fake-agent-id' } - let(:reply) { 'inbox.fake' } let(:disk_name) { 'fake-disk-name' } let(:disk_cid) { 'fake-disk-cid' } let(:disk_pool_name) { 'fake-disk-pool-name' } - let(:nats_rpc) { instance_double(Bosh::Director::NatsRpc) } let(:cloud) { instance_double(Bosh::Clouds::ExternalCpi) } - let(:detach_dynamic_disk_job) { Jobs::DynamicDisks::DetachDynamicDisk.new(reply, disk_name) } - let!(:vm) { FactoryBot.create(:models_vm, agent_id: agent_id, cid: 'fake-vm-cid') } + let(:agent_client) { instance_double(AgentClient) } + let(:detach_dynamic_disk_job) { Jobs::DynamicDisks::DetachDynamicDisk.new(disk_name) } + let!(:vm) { FactoryBot.create(:models_vm, instance: instance) } + let(:instance) { FactoryBot.create(:models_instance) } let(:cloud_factory) { instance_double(Bosh::Director::CloudFactory, get: cloud) } before do - allow(Config).to receive(:nats_rpc).and_return(nats_rpc) allow(Bosh::Director::Config).to receive(:name).and_return('fake-director-name') allow(Bosh::Director::Config).to receive(:cloud_options).and_return('provider' => { 'path' => '/path/to/default/cpi' }) allow(Bosh::Director::Config).to receive(:preferred_cpi_api_version).and_return(2) allow(CloudFactory).to receive(:create).and_return(cloud_factory) allow(cloud).to receive(:has_disk).and_return(true) + allow(AgentClient).to receive(:with_agent_id).with(vm.agent_id, instance.name).and_return(agent_client) end describe '#perform' do @@ -37,9 +36,6 @@ module Bosh::Director end it 'does not detach the disk' do - expect(nats_rpc).to receive(:send_message).with(reply, { - 'error' => nil - }) expect(detach_dynamic_disk_job.perform).to eq("disk `#{disk_cid}` was already detached") end end @@ -58,19 +54,15 @@ module Bosh::Director it 'detaches the disk' do expect(cloud).to receive(:detach_disk).with(vm.cid, disk_cid) - expect(nats_rpc).to receive(:send_message).with(reply, { - 'error' => nil - }) + expect(agent_client).to receive(:remove_dynamic_disk).with(disk_cid) expect(detach_dynamic_disk_job.perform).to eq("detached disk `#{disk_cid}` from vm `#{vm.cid}`") expect(Models::DynamicDisk.find(disk_cid: disk_cid).vm_id).to be_nil end context 'when disk is already detached' do - it 'returns an error' do + it 'does not return an error' do expect(cloud).to receive(:detach_disk).with(vm.cid, disk_cid).and_raise(Bosh::Clouds::DiskNotAttached.new(false)) - expect(nats_rpc).to receive(:send_message).with(reply, { - 'error' => nil - }) + expect(agent_client).to receive(:remove_dynamic_disk).with(disk_cid) expect(detach_dynamic_disk_job.perform).to eq("disk `#{disk_cid}` was already detached") expect(Models::DynamicDisk.find(disk_cid: disk_cid).vm_id).to be_nil end @@ -79,10 +71,7 @@ module Bosh::Director end context 'when disk does not exist in database' do - it 'returns an error' do - expect(nats_rpc).to receive(:send_message).with(reply, { - 'error' => "disk `#{disk_name}` can not be found in the database" - }) + it 'raises an error' do expect { detach_dynamic_disk_job.perform }.to raise_error("disk `#{disk_name}` can not be found in the database") end end diff --git a/src/bosh-director/spec/unit/bosh/director/jobs/dynamic_disks/provide_dynamic_disk_spec.rb b/src/bosh-director/spec/unit/bosh/director/jobs/dynamic_disks/provide_dynamic_disk_spec.rb index fa0dd335daf..d05c2ad71f1 100644 --- a/src/bosh-director/spec/unit/bosh/director/jobs/dynamic_disks/provide_dynamic_disk_spec.rb +++ b/src/bosh-director/spec/unit/bosh/director/jobs/dynamic_disks/provide_dynamic_disk_spec.rb @@ -2,8 +2,6 @@ module Bosh::Director describe Jobs::DynamicDisks::ProvideDynamicDisk do - let(:agent_id) { 'fake-agent-id' } - let(:reply) { 'inbox.fake' } let(:disk_name) { 'fake-disk-name' } let(:disk_cid) { 'fake-disk-cid' } let(:disk_pool_name) { 'fake-disk-pool-name' } @@ -12,10 +10,14 @@ module Bosh::Director let(:metadata) { { 'fake-key' => 'fake-value' } } let(:disk_hint) { 'fake-disk-hint' } - let(:nats_rpc) { instance_double(Bosh::Director::NatsRpc) } + let(:agent_client) { instance_double(AgentClient) } let(:cloud) { instance_double(Bosh::Clouds::ExternalCpi) } - let(:provide_dynamic_disk_job) { Jobs::DynamicDisks::ProvideDynamicDisk.new(agent_id, reply, disk_name, disk_pool_name, disk_size, metadata) } - let!(:vm) { FactoryBot.create(:models_vm, agent_id: agent_id, cid: 'fake-vm-cid') } + + let(:instance) { FactoryBot.create(:models_instance) } + let!(:vm) { FactoryBot.create(:models_vm, instance: instance, active: true) } + + let(:provide_dynamic_disk_job) { Jobs::DynamicDisks::ProvideDynamicDisk.new(instance.id, 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( @@ -31,12 +33,12 @@ module Bosh::Director let(:cloud_factory) { instance_double(Bosh::Director::CloudFactory, get: cloud) } before do - allow(Config).to receive(:nats_rpc).and_return(nats_rpc) allow(Bosh::Director::Config).to receive(:name).and_return('fake-director-name') allow(Bosh::Director::Config).to receive(:cloud_options).and_return('provider' => { 'path' => '/path/to/default/cpi' }) allow(Bosh::Director::Config).to receive(:preferred_cpi_api_version).and_return(2) allow(CloudFactory).to receive(:create).and_return(cloud_factory) allow(cloud).to receive(:respond_to?).with(:set_disk_metadata).and_return(false) + allow(AgentClient).to receive(:with_agent_id).with(vm.agent_id, instance.name).and_return(agent_client) end describe '#perform' do @@ -54,11 +56,7 @@ module Bosh::Director it 'attaches the disk to VM and updates disk vm and availability zone' do expect(cloud).not_to receive(:create_disk) expect(cloud).to receive(:attach_disk).with(vm.cid, disk_cid).and_return(disk_hint) - expect(nats_rpc).to receive(:send_message).with(reply, { - 'error' => nil, - 'disk_name' => disk_name, - 'disk_hint' => disk_hint, - }) + expect(agent_client).to receive(:add_dynamic_disk).with(disk_cid, disk_hint) expect(provide_dynamic_disk_job.perform).to eq("attached disk `#{disk_name}` to `#{vm.cid}` in deployment `#{vm.instance.deployment.name}`") model = Models::DynamicDisk.where(disk_cid: disk_cid).first @@ -73,11 +71,7 @@ module Bosh::Director expect(cloud).to receive(:respond_to?).with(:set_disk_metadata).and_return(false) expect(cloud).to receive(:attach_disk).with(vm.cid, disk_cid).and_return(disk_hint) - expect(nats_rpc).to receive(:send_message).with(reply, { - 'error' => nil, - 'disk_name' => disk_name, - 'disk_hint' => disk_hint, - }) + expect(agent_client).to receive(:add_dynamic_disk).with(disk_cid, disk_hint) expect(provide_dynamic_disk_job.perform).to eq("attached disk `#{disk_name}` to `#{vm.cid}` in deployment `#{vm.instance.deployment.name}`") model = Models::DynamicDisk.where(disk_cid: disk_cid).first @@ -105,10 +99,6 @@ module Bosh::Director it 'returns an error from attach_disk call' do expect(cloud).to receive(:attach_disk).with(vm.cid, disk_cid).and_raise('some-error') - - expect(nats_rpc).to receive(:send_message).with(reply, { - 'error' => 'some-error' - }) expect { provide_dynamic_disk_job.perform }.to raise_error('some-error') end end @@ -128,9 +118,6 @@ module Bosh::Director end it 'responds with error' do - expect(nats_rpc).to receive(:send_message).with(reply, { - 'error' => "Could not find disk pool by name `fake-disk-pool-name`" - }) expect { provide_dynamic_disk_job.perform }.to raise_error("Could not find disk pool by name `fake-disk-pool-name`") end end @@ -147,13 +134,9 @@ module Bosh::Director "fake-key" => "fake-value" } ) - expect(cloud).to receive(:attach_disk).with('fake-vm-cid', 'fake-disk-cid').and_return(disk_hint) + expect(cloud).to receive(:attach_disk).with(vm.cid, disk_cid).and_return(disk_hint) - expect(nats_rpc).to receive(:send_message).with(reply, { - 'error' => nil, - 'disk_name' => disk_name, - 'disk_hint' => disk_hint, - }) + expect(agent_client).to receive(:add_dynamic_disk).with(disk_cid, disk_hint) expect(provide_dynamic_disk_job.perform).to eq("attached disk `#{disk_name}` to `#{vm.cid}` in deployment `#{vm.instance.deployment.name}`") end end @@ -184,23 +167,32 @@ module Bosh::Director it 'gets the disk cloud properties from the latest cloud config for those teams' do expect(cloud).to receive(:create_disk).with(disk_size, disk_cloud_properties, vm.cid).and_return(disk_cid) expect(cloud).to receive(:attach_disk).with(vm.cid, disk_cid).and_return(disk_hint) - expect(nats_rpc).to receive(:send_message).with(reply, { - 'error' => nil, - 'disk_name' => disk_name, - 'disk_hint' => disk_hint, - }) + expect(agent_client).to receive(:add_dynamic_disk).with(disk_cid, disk_hint) expect(provide_dynamic_disk_job.perform).to eq("attached disk `#{disk_name}` to `#{vm.cid}` in deployment `#{vm.instance.deployment.name}`") end end - context 'when VM cannot be found' do - let!(:vm) { FactoryBot.create(:models_vm, agent_id: 'different-agent-id', cid: 'fake-vm-cid') } + context 'when instance cannot be found' do + let(:provide_dynamic_disk_job) { Jobs::DynamicDisks::ProvideDynamicDisk.new('unknown-instance-id', disk_name, disk_pool_name, disk_size, metadata) } + + it 'responds with error' do + expect { provide_dynamic_disk_job.perform }.to raise_error("instance `unknown-instance-id` not found") + end + end + + context 'when active vm for instance cannot be found' do + let(:vm) { FactoryBot.create(:models_vm, instance: instance, active: false) } + + it 'responds with error' do + expect { provide_dynamic_disk_job.perform }.to raise_error("no active vm found for instance `#{instance.id}`") + end + end + + context 'when instance has no vms' do + let(:vm) { FactoryBot.create(:models_vm) } it 'responds with error' do - expect(nats_rpc).to receive(:send_message).with(reply, { - 'error' => "vm for agent `fake-agent-id` not found" - }) - expect { provide_dynamic_disk_job.perform }.to raise_error("vm for agent `fake-agent-id` not found") + expect { provide_dynamic_disk_job.perform }.to raise_error("no active vm found for instance `#{instance.id}`") end end end diff --git a/src/bosh-nats-sync/lib/nats_sync/nats_auth_config.rb b/src/bosh-nats-sync/lib/nats_sync/nats_auth_config.rb index 25586ce1eb9..6f6007c6c2d 100644 --- a/src/bosh-nats-sync/lib/nats_sync/nats_auth_config.rb +++ b/src/bosh-nats-sync/lib/nats_sync/nats_auth_config.rb @@ -12,7 +12,7 @@ def director_user { 'user' => @director_subject, 'permissions' => { - 'publish' => %w[agent.* agent.inbox.> hm.director.alert], + 'publish' => %w[agent.* hm.director.alert], 'subscribe' => ['director.>'], }, } @@ -37,9 +37,8 @@ def agent_user(agent_id, cn) "hm.agent.alert.#{agent_id}", "hm.agent.shutdown.#{agent_id}", "director.*.#{agent_id}.*", - "director.agent.disk.*.#{agent_id}", ], - "subscribe": ["agent.#{agent_id}", "agent.inbox.#{agent_id}.>"], + "subscribe": ["agent.#{agent_id}"], }, } end From d3e69987a8a054bced0a8dfb0125fec1997a0145 Mon Sep 17 00:00:00 2001 From: Maria Shaldybin Date: Thu, 21 Aug 2025 20:48:17 +0000 Subject: [PATCH 6/8] Add dynamic disks scopes * bosh.dynamic_disks.update allows to create, attach and detach dynamic disks * bosh.dynamic_disks.delete allows to delete dynamic disks * bosh director admin scopes allow update and delete dynamic disks --- .../controllers/dynamic_disks_controller.rb | 6 +- .../jobs/dynamic_disks/detach_dynamic_disk.rb | 2 +- .../dynamic_disks/provide_dynamic_disk.rb | 28 ++++++-- .../lib/bosh/director/models/dynamic_disk.rb | 9 +++ .../bosh/director/permission_authorizer.rb | 6 +- .../spec/support/test_identity_provider.rb | 2 + .../dynamic_disks_controller_spec.rb | 69 +++++++++++++++---- .../dynamic_disks/detach_dynamic_disk_spec.rb | 4 +- .../provide_dynamic_disk_spec.rb | 37 ++++++---- .../director/permission_authorizer_spec.rb | 54 +++++++++++++++ 10 files changed, 176 insertions(+), 41 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 f8000daf915..bb940ccda8b 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,7 +5,7 @@ module Api::Controllers class DynamicDisksController < BaseController include ValidationHelper - post '/provide', consumes: :json do + post '/provide', scope: :update_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) @@ -24,7 +24,7 @@ class DynamicDisksController < BaseController redirect "/tasks/#{task.id}" end - post '/:disk_name/detach', scope: :manage_dynamic_disks do + post '/:disk_name/detach', scope: :update_dynamic_disks do disk_name = safe_property(params, 'disk_name', class: String, min_length: 1) task = JobQueue.new.enqueue( @@ -37,7 +37,7 @@ class DynamicDisksController < BaseController redirect "/tasks/#{task.id}" end - delete '/:disk_name', scope: :manage_dynamic_disks do + delete '/:disk_name', scope: :delete_dynamic_disks do disk_name = safe_property(params, 'disk_name', class: String, min_length: 1) task = JobQueue.new.enqueue( diff --git a/src/bosh-director/lib/bosh/director/jobs/dynamic_disks/detach_dynamic_disk.rb b/src/bosh-director/lib/bosh/director/jobs/dynamic_disks/detach_dynamic_disk.rb index ec5b3bbdbd7..b93f30e98c2 100644 --- a/src/bosh-director/lib/bosh/director/jobs/dynamic_disks/detach_dynamic_disk.rb +++ b/src/bosh-director/lib/bosh/director/jobs/dynamic_disks/detach_dynamic_disk.rb @@ -28,7 +28,7 @@ def perform cloud.detach_disk(disk_model.vm.cid, disk_model.disk_cid) - disk_model.update(vm_id: nil) + disk_model.update(vm_id: nil, disk_hint: '') "detached disk `#{disk_model.disk_cid}` from vm `#{vm_cid}`" rescue Bosh::Clouds::DiskNotAttached diff --git a/src/bosh-director/lib/bosh/director/jobs/dynamic_disks/provide_dynamic_disk.rb b/src/bosh-director/lib/bosh/director/jobs/dynamic_disks/provide_dynamic_disk.rb index 9784dc1d596..5e37804d386 100644 --- a/src/bosh-director/lib/bosh/director/jobs/dynamic_disks/provide_dynamic_disk.rb +++ b/src/bosh-director/lib/bosh/director/jobs/dynamic_disks/provide_dynamic_disk.rb @@ -18,7 +18,7 @@ def initialize(instance_id, disk_name, disk_pool_name, disk_size, metadata) end def perform - instance = Models::Instance.find(id: @instance_id) + instance = Models::Instance.find(uuid: @instance_id) raise "instance `#{@instance_id}` not found" if instance.nil? vm = instance.active_vm @@ -29,6 +29,7 @@ def perform 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, @@ -37,19 +38,32 @@ def perform size: @disk_size, disk_pool_name: @disk_pool_name, cpi: vm.cpi, - metadata: @metadata, ) end - disk_hint = cloud.attach_disk(vm.cid, disk_model.disk_cid) - disk_model.update(vm_id: vm.id, availability_zone: vm.instance.availability_zone) + if disk_model.vm.nil? + disk_hint = 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) + else + if disk_model.vm.id != vm.id + raise "disk is attached to a different vm `#{disk_model.vm.cid}`" + end + end - unless @metadata.nil? + if !@metadata.nil? && disk_model.metadata != @metadata MetadataUpdater.build.update_dynamic_disk_metadata(cloud, disk_model, @metadata) + disk_model.update(metadata: @metadata) + end + + unless disk_model.disk_hint.nil? + agent_client = AgentClient.with_agent_id(vm.agent_id, instance.name) + agent_client.add_dynamic_disk(disk_model.disk_cid, disk_model.disk_hint) end - agent_client = AgentClient.with_agent_id(vm.agent_id, instance.name) - agent_client.add_dynamic_disk(disk_model.disk_cid, disk_hint) + disk_info = { "disk_cid": disk_model.disk_cid } + + task_result.write(JSON.generate(disk_info)) + task_result.write("\n") "attached disk `#{@disk_name}` to `#{vm.cid}` in deployment `#{vm.instance.deployment.name}`" end diff --git a/src/bosh-director/lib/bosh/director/models/dynamic_disk.rb b/src/bosh-director/lib/bosh/director/models/dynamic_disk.rb index aeccc779f6e..7a1d63b34f3 100644 --- a/src/bosh-director/lib/bosh/director/models/dynamic_disk.rb +++ b/src/bosh-director/lib/bosh/director/models/dynamic_disk.rb @@ -8,6 +8,15 @@ def validate validates_unique [:name, :disk_cid] end + def disk_hint + result = disk_hint_json + result ? JSON.parse(result) : nil + end + + def disk_hint=(disk_hint) + self.disk_hint_json = JSON.generate(disk_hint) + end + def metadata result = self.metadata_json result ? JSON.parse(result) : {} diff --git a/src/bosh-director/lib/bosh/director/permission_authorizer.rb b/src/bosh-director/lib/bosh/director/permission_authorizer.rb index f9fe578d217..ebe28986944 100644 --- a/src/bosh-director/lib/bosh/director/permission_authorizer.rb +++ b/src/bosh-director/lib/bosh/director/permission_authorizer.rb @@ -86,10 +86,8 @@ def get_director_scopes(expected_scope, permission, user_scopes) expected_scope << bosh_team_admin_scopes(user_scopes) when :update_configs expected_scope << bosh_team_admin_scopes(user_scopes) - when :read, :upload_releases, :upload_stemcells + when :read, :upload_releases, :upload_stemcells, :update_dynamic_disks, :delete_dynamic_disks expected_scope << director_permissions[permission] - when :manage_dynamic_disks - expected_scope << bosh_team_admin_scopes(user_scopes) else raise ArgumentError, "Unexpected permission for director: #{permission}" end @@ -113,6 +111,8 @@ def director_permissions admin: ['bosh.admin', "bosh.#{@uuid_provider.uuid}.admin"], upload_stemcells: ['bosh.stemcells.upload'], upload_releases: ['bosh.releases.upload'], + update_dynamic_disks: ['bosh.dynamic_disks.update'], + delete_dynamic_disks: ['bosh.dynamic_disks.delete'], } end diff --git a/src/bosh-director/spec/support/test_identity_provider.rb b/src/bosh-director/spec/support/test_identity_provider.rb index fd4820edb22..5abf3db1bdb 100644 --- a/src/bosh-director/spec/support/test_identity_provider.rb +++ b/src/bosh-director/spec/support/test_identity_provider.rb @@ -39,6 +39,8 @@ def user_scopes 'director-reader' => ["bosh.#{@uuid_provider.uuid}.read"], 'dev-team-member' => ['bosh.teams.dev.admin'], 'dev-team-read-member' => ['bosh.teams.dev.read'], + 'dynamic-disks-updater' => ['bosh.dynamic_disks.update'], + 'dynamic-disks-deleter' => ['bosh.dynamic_disks.delete'], '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 6ec777337c5..bcefb35926a 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 @@ -24,19 +24,19 @@ module Api describe 'POST', '/provide' 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 - }) + '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('/provide', content, {'CONTENT_TYPE' => 'application/json'}).status).to eq(401) + expect(post('/provide', content, { 'CONTENT_TYPE' => 'application/json' }).status).to eq(401) end end @@ -81,7 +81,7 @@ module Api expect(JSON.parse(last_response.body)).to eq( 'code' => 40002, 'description' => "'disk_pool_name' length (0) should be greater than 1", - ) + ) end end @@ -95,7 +95,7 @@ module Api expect(JSON.parse(last_response.body)).to eq( 'code' => 40000, 'description' => "Property 'disk_name' value (nil) did not match the required type 'String'", - ) + ) end end @@ -123,7 +123,7 @@ module Api expect(JSON.parse(last_response.body)).to eq( 'code' => 40000, 'description' => "Property 'disk_size' value (nil) did not match the required type 'Integer'", - ) + ) end end @@ -137,7 +137,7 @@ module Api expect(JSON.parse(last_response.body)).to eq( 'code' => 40002, 'description' => "'disk_size' value (0) should be greater than 1", - ) + ) end end end @@ -153,6 +153,23 @@ module Api end end + context 'when user has bosh.dynamic_disks.update scope' do + before { basic_authorize('dynamic-disks-updater', 'dynamic-disks-updater') } + + it 'enqueues a ProvideDynamicDisk task' do + expect_any_instance_of(Bosh::Director::JobQueue).to receive(:enqueue).with( + 'dynamic-disks-updater', + Jobs::DynamicDisks::DetachDynamicDisk, + 'detach dynamic disk', + ['disk_name'], + ).and_call_original + + post '/disk_name/detach' + + expect_redirect_to_queued_task(last_response) + end + end + context 'user has admin permissions' do before { authorize 'admin', 'admin' } @@ -162,7 +179,7 @@ module Api Jobs::DynamicDisks::DetachDynamicDisk, 'detach dynamic disk', ['disk_name'], - ).and_call_original + ).and_call_original post '/disk_name/detach' @@ -180,6 +197,31 @@ module Api end end + context 'when user has bosh.dynamic_disks.update scope' do + before { basic_authorize('dynamic-disks-updater', 'dynamic-disks-updater') } + + it 'forbids access' do + expect(delete('/disk_name').status).to eq(401) + end + end + + context 'when user has bosh.dynamic_disks.delete scope' do + before { authorize 'dynamic-disks-deleter', 'dynamic-disks-deleter' } + + it 'enqueues a ProvideDynamicDisk task' do + expect_any_instance_of(Bosh::Director::JobQueue).to receive(:enqueue).with( + 'dynamic-disks-deleter', + Jobs::DynamicDisks::DeleteDynamicDisk, + 'delete dynamic disk', + ['disk_name'], + ).and_call_original + + delete '/disk_name' + + expect_redirect_to_queued_task(last_response) + end + end + context 'user has admin permissions' do before { authorize 'admin', 'admin' } @@ -189,13 +231,12 @@ module Api Jobs::DynamicDisks::DeleteDynamicDisk, 'delete dynamic disk', ['disk_name'], - ).and_call_original + ).and_call_original delete '/disk_name' expect_redirect_to_queued_task(last_response) end - end end end diff --git a/src/bosh-director/spec/unit/bosh/director/jobs/dynamic_disks/detach_dynamic_disk_spec.rb b/src/bosh-director/spec/unit/bosh/director/jobs/dynamic_disks/detach_dynamic_disk_spec.rb index 86fac84ad56..28ccfdf47b0 100644 --- a/src/bosh-director/spec/unit/bosh/director/jobs/dynamic_disks/detach_dynamic_disk_spec.rb +++ b/src/bosh-director/spec/unit/bosh/director/jobs/dynamic_disks/detach_dynamic_disk_spec.rb @@ -56,7 +56,9 @@ module Bosh::Director expect(cloud).to receive(:detach_disk).with(vm.cid, disk_cid) expect(agent_client).to receive(:remove_dynamic_disk).with(disk_cid) expect(detach_dynamic_disk_job.perform).to eq("detached disk `#{disk_cid}` from vm `#{vm.cid}`") - expect(Models::DynamicDisk.find(disk_cid: disk_cid).vm_id).to be_nil + updated_disk_model = Models::DynamicDisk.find(disk_cid: disk_cid) + expect(updated_disk_model.vm_id).to be_nil + expect(updated_disk_model.disk_hint).to be_empty end context 'when disk is already detached' do diff --git a/src/bosh-director/spec/unit/bosh/director/jobs/dynamic_disks/provide_dynamic_disk_spec.rb b/src/bosh-director/spec/unit/bosh/director/jobs/dynamic_disks/provide_dynamic_disk_spec.rb index d05c2ad71f1..5cce4270e65 100644 --- a/src/bosh-director/spec/unit/bosh/director/jobs/dynamic_disks/provide_dynamic_disk_spec.rb +++ b/src/bosh-director/spec/unit/bosh/director/jobs/dynamic_disks/provide_dynamic_disk_spec.rb @@ -8,7 +8,7 @@ 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(:disk_hint) { 'fake-disk-hint' } + let(:disk_hint) { { "id" => "fake-disk-id" } } let(:agent_client) { instance_double(AgentClient) } let(:cloud) { instance_double(Bosh::Clouds::ExternalCpi) } @@ -16,7 +16,14 @@ module Bosh::Director let(:instance) { FactoryBot.create(:models_instance) } let!(:vm) { FactoryBot.create(:models_vm, instance: instance, active: true) } - let(:provide_dynamic_disk_job) { Jobs::DynamicDisks::ProvideDynamicDisk.new(instance.id, disk_name, disk_pool_name, disk_size, metadata) } + 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(:provide_dynamic_disk_job) { Jobs::DynamicDisks::ProvideDynamicDisk.new(instance.uuid, disk_name, disk_pool_name, disk_size, metadata) } let!(:cloud_config) do FactoryBot.create(:models_config_cloud, content: YAML.dump( @@ -30,15 +37,17 @@ module Bosh::Director ) )) end - let(:cloud_factory) { instance_double(Bosh::Director::CloudFactory, get: cloud) } + let(:cloud_factory) { instance_double(CloudFactory, get: cloud) } before do - allow(Bosh::Director::Config).to receive(:name).and_return('fake-director-name') - allow(Bosh::Director::Config).to receive(:cloud_options).and_return('provider' => { 'path' => '/path/to/default/cpi' }) - allow(Bosh::Director::Config).to receive(:preferred_cpi_api_version).and_return(2) + 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(AgentClient).to receive(:with_agent_id).with(vm.agent_id, instance.name).and_return(agent_client) + allow(provide_dynamic_disk_job).to receive(:task_id).and_return(task.id) end describe '#perform' do @@ -58,6 +67,7 @@ module Bosh::Director 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(provide_dynamic_disk_job.perform).to eq("attached disk `#{disk_name}` to `#{vm.cid}` in deployment `#{vm.instance.deployment.name}`") + expect(parsed_task_result).to eq({'disk_cid' => disk_cid}) model = Models::DynamicDisk.where(disk_cid: disk_cid).first expect(model.vm).to eq(vm) @@ -67,12 +77,13 @@ module Bosh::Director context 'when disk does not exist' do it 'creates the disk and attaches it to VM' do - expect(cloud).to receive(:create_disk).with(disk_size, disk_cloud_properties, vm.cid).and_return(disk_cid) - expect(cloud).to receive(:respond_to?).with(:set_disk_metadata).and_return(false) + expected_cloud_properties = disk_cloud_properties.merge('name' => 'fake-disk-name') + expect(cloud).to receive(:create_disk).with(disk_size, expected_cloud_properties, vm.cid).and_return(disk_cid) 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(provide_dynamic_disk_job.perform).to eq("attached disk `#{disk_name}` to `#{vm.cid}` in deployment `#{vm.instance.deployment.name}`") + expect(parsed_task_result).to eq({'disk_cid' => disk_cid}) model = Models::DynamicDisk.where(disk_cid: disk_cid).first expect(model.name).to eq(disk_name) @@ -124,7 +135,8 @@ module Bosh::Director context 'when cpi supports set_disk_metadata' do it 'sets disk metadata' do - expect(cloud).to receive(:create_disk).with(disk_size, disk_cloud_properties, vm.cid).and_return('fake-disk-cid') + expected_cloud_properties = disk_cloud_properties.merge('name' => 'fake-disk-name') + expect(cloud).to receive(:create_disk).with(disk_size, expected_cloud_properties, vm.cid).and_return('fake-disk-cid') expect(cloud).to receive(:respond_to?).with(:set_disk_metadata).and_return(true) expect(cloud).to receive(:set_disk_metadata).with( 'fake-disk-cid', @@ -165,7 +177,8 @@ module Bosh::Director end it 'gets the disk cloud properties from the latest cloud config for those teams' do - expect(cloud).to receive(:create_disk).with(disk_size, disk_cloud_properties, vm.cid).and_return(disk_cid) + expected_cloud_properties = disk_cloud_properties.merge('name' => 'fake-disk-name') + expect(cloud).to receive(:create_disk).with(disk_size, expected_cloud_properties, vm.cid).and_return(disk_cid) 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(provide_dynamic_disk_job.perform).to eq("attached disk `#{disk_name}` to `#{vm.cid}` in deployment `#{vm.instance.deployment.name}`") @@ -184,7 +197,7 @@ module Bosh::Director let(:vm) { FactoryBot.create(:models_vm, instance: instance, active: false) } it 'responds with error' do - expect { provide_dynamic_disk_job.perform }.to raise_error("no active vm found for instance `#{instance.id}`") + expect { provide_dynamic_disk_job.perform }.to raise_error("no active vm found for instance `#{instance.uuid}`") end end @@ -192,7 +205,7 @@ module Bosh::Director let(:vm) { FactoryBot.create(:models_vm) } it 'responds with error' do - expect { provide_dynamic_disk_job.perform }.to raise_error("no active vm found for instance `#{instance.id}`") + expect { provide_dynamic_disk_job.perform }.to raise_error("no active vm found for instance `#{instance.uuid}`") end end end diff --git a/src/bosh-director/spec/unit/bosh/director/permission_authorizer_spec.rb b/src/bosh-director/spec/unit/bosh/director/permission_authorizer_spec.rb index 3f38db7aa8d..7518100a54a 100644 --- a/src/bosh-director/spec/unit/bosh/director/permission_authorizer_spec.rb +++ b/src/bosh-director/spec/unit/bosh/director/permission_authorizer_spec.rb @@ -328,6 +328,60 @@ module Bosh::Director it_behaves_like :admin_read_team_admin_scopes end + describe 'checking update_dynamic_disks rights' do + let(:acl_right) { :update_dynamic_disks } + it 'allows bosh.dynamic_disks.update scope' do + expect(subject.is_granted?(acl_subject, acl_right, ['bosh.dynamic_disks.update'])).to eq(true) + end + + it 'allows bosh.admin scope' do + expect(subject.is_granted?(acl_subject, acl_right, ['bosh.admin'])).to eq(true) + end + + it 'allows bosh.X.admin scope' do + expect(subject.is_granted?(acl_subject, acl_right, ['bosh.fake-director-uuid.admin'])).to eq(true) + end + + it 'denies others' do + expect(subject.is_granted?(acl_subject, acl_right, [ + 'bosh.unexpected-uuid.admin', + 'bosh.read', + 'bosh.teams.anything.admin', + 'bosh.teams.anything.read', + 'bosh.fake-director-uuid.read', + 'bosh.unexpected-uuid.read', + 'bosh.teams.security.unexpected', + ])).to eq(false) + end + end + + describe 'checking delete_dynamic_disks rights' do + let(:acl_right) { :delete_dynamic_disks } + it 'allows bosh.dynamic_disks.delete scope' do + expect(subject.is_granted?(acl_subject, acl_right, ['bosh.dynamic_disks.delete'])).to eq(true) + end + + it 'allows bosh.admin scope' do + expect(subject.is_granted?(acl_subject, acl_right, ['bosh.admin'])).to eq(true) + end + + it 'allows bosh.X.admin scope' do + expect(subject.is_granted?(acl_subject, acl_right, ['bosh.fake-director-uuid.admin'])).to eq(true) + end + + it 'denies others' do + expect(subject.is_granted?(acl_subject, acl_right, [ + 'bosh.unexpected-uuid.admin', + 'bosh.read', + 'bosh.teams.anything.admin', + 'bosh.teams.anything.read', + 'bosh.fake-director-uuid.read', + 'bosh.unexpected-uuid.read', + 'bosh.teams.security.unexpected', + ])).to eq(false) + end + end + describe 'checking for invalid rights' do let(:acl_right) { :what_I_fancy } From d9ea1e364262373f9919587fc763fd8a74119e34 Mon Sep 17 00:00:00 2001 From: Maria Shaldybin Date: Mon, 17 Nov 2025 21:25:16 +0000 Subject: [PATCH 7/8] Add VM lock for VM operations --- .../lib/bosh/director/cloudcheck_helper.rb | 4 +++- .../deployment_plan/steps/attach_disk_step.rb | 4 +++- .../deployment_plan/steps/delete_vm_step.rb | 4 +++- .../deployment_plan/steps/detach_disk_step.rb | 4 +++- .../jobs/dynamic_disks/detach_dynamic_disk.rb | 3 ++- .../jobs/dynamic_disks/provide_dynamic_disk.rb | 3 ++- .../jobs/helpers/dynamic_disk_helpers.rb | 2 ++ .../lib/bosh/director/lock_helper.rb | 6 ++++++ .../director/problem_handlers/inactive_disk.rb | 2 +- .../director/problem_handlers/missing_disk.rb | 2 +- .../problem_handlers/mount_info_mismatch.rb | 2 +- .../lib/bosh/director/vm_deleter.rb | 4 +++- .../unit/bosh/director/cloudcheck_helper_spec.rb | 3 ++- .../steps/attach_disk_step_spec.rb | 6 +++++- .../deployment_plan/steps/delete_vm_step_spec.rb | 16 ++++++++++------ .../steps/detach_disk_step_spec.rb | 6 +++++- .../unit/bosh/director/instance_deleter_spec.rb | 4 ++-- .../unit/bosh/director/jobs/delete_vm_spec.rb | 2 +- .../dynamic_disks/detach_dynamic_disk_spec.rb | 2 ++ .../dynamic_disks/provide_dynamic_disk_spec.rb | 5 +++++ .../spec/unit/bosh/director/lock_helper_spec.rb | 14 ++++++++++++++ .../problem_handlers/inactive_disk_spec.rb | 3 ++- .../problem_handlers/missing_disk_spec.rb | 8 +++++++- .../problem_handlers/mount_info_mismatch_spec.rb | 5 +++++ .../spec/unit/bosh/director/vm_deleter_spec.rb | 5 ++++- 25 files changed, 94 insertions(+), 25 deletions(-) diff --git a/src/bosh-director/lib/bosh/director/cloudcheck_helper.rb b/src/bosh-director/lib/bosh/director/cloudcheck_helper.rb index 617d8ddb25d..fbb4ea32979 100644 --- a/src/bosh-director/lib/bosh/director/cloudcheck_helper.rb +++ b/src/bosh-director/lib/bosh/director/cloudcheck_helper.rb @@ -1,5 +1,7 @@ module Bosh::Director module CloudcheckHelper + include LockHelper + # Helper functions that come in handy for # cloudcheck: # 1. VM/agent interactions @@ -10,7 +12,7 @@ def reboot_vm(instance) vm = instance.active_vm cloud = CloudFactory.create.get(vm.cpi) - cloud.reboot_vm(vm.cid) + with_vm_lock(vm.cid) { cloud.reboot_vm(vm.cid) } begin agent_client(instance.agent_id, instance.name).wait_until_ready diff --git a/src/bosh-director/lib/bosh/director/deployment_plan/steps/attach_disk_step.rb b/src/bosh-director/lib/bosh/director/deployment_plan/steps/attach_disk_step.rb index 6d46222523a..87ec4ec22ec 100644 --- a/src/bosh-director/lib/bosh/director/deployment_plan/steps/attach_disk_step.rb +++ b/src/bosh-director/lib/bosh/director/deployment_plan/steps/attach_disk_step.rb @@ -2,6 +2,8 @@ module Bosh::Director module DeploymentPlan module Steps class AttachDiskStep + include LockHelper + def initialize(disk, tags) @disk = disk @logger = Config.logger @@ -17,7 +19,7 @@ def perform(report) cloud_factory = CloudFactory.create attach_disk_cloud = cloud_factory.get(@disk.cpi, instance_active_vm.stemcell_api_version) @logger.info("Attaching disk #{@disk.disk_cid}") - disk_hint = attach_disk_cloud.attach_disk(@disk.instance.vm_cid, @disk.disk_cid) + disk_hint = with_vm_lock(@disk.instance.vm_cid) { attach_disk_cloud.attach_disk(@disk.instance.vm_cid, @disk.disk_cid) } if disk_hint agent_client(@disk.instance).wait_until_ready diff --git a/src/bosh-director/lib/bosh/director/deployment_plan/steps/delete_vm_step.rb b/src/bosh-director/lib/bosh/director/deployment_plan/steps/delete_vm_step.rb index a1186c9a694..9857baae966 100644 --- a/src/bosh-director/lib/bosh/director/deployment_plan/steps/delete_vm_step.rb +++ b/src/bosh-director/lib/bosh/director/deployment_plan/steps/delete_vm_step.rb @@ -2,6 +2,8 @@ module Bosh::Director module DeploymentPlan module Steps class DeleteVmStep + include LockHelper + def initialize(store_event = true, force = false, enable_virtual_delete_vm = false) @store_event = store_event @logger = Config.logger @@ -24,7 +26,7 @@ def perform(report) cloud = CloudFactory.create.get(vm.cpi, vm.stemcell_api_version) begin - cloud.delete_vm(vm_cid) unless @enable_virtual_delete_vm + with_vm_lock(vm_cid) { cloud.delete_vm(vm_cid) } unless @enable_virtual_delete_vm rescue Bosh::Clouds::VMNotFound @logger.warn("VM '#{vm_cid}' might have already been deleted from the cloud") end diff --git a/src/bosh-director/lib/bosh/director/deployment_plan/steps/detach_disk_step.rb b/src/bosh-director/lib/bosh/director/deployment_plan/steps/detach_disk_step.rb index ff27bd63b12..be855c09c77 100644 --- a/src/bosh-director/lib/bosh/director/deployment_plan/steps/detach_disk_step.rb +++ b/src/bosh-director/lib/bosh/director/deployment_plan/steps/detach_disk_step.rb @@ -2,6 +2,8 @@ module Bosh::Director module DeploymentPlan module Steps class DetachDiskStep + include LockHelper + def initialize(disk) @disk = disk @logger = Config.logger @@ -18,7 +20,7 @@ def perform(_report) cloud_factory = CloudFactory.create cloud = cloud_factory.get(@disk.cpi, instance_active_vm.stemcell_api_version) @logger.info("Detaching disk #{@disk.disk_cid}") - cloud.detach_disk(@disk.instance.vm_cid, @disk.disk_cid) + with_vm_lock(@disk.instance.vm_cid) { cloud.detach_disk(@disk.instance.vm_cid, @disk.disk_cid) } rescue Bosh::Clouds::DiskNotAttached if @disk.active raise CloudDiskNotAttached, diff --git a/src/bosh-director/lib/bosh/director/jobs/dynamic_disks/detach_dynamic_disk.rb b/src/bosh-director/lib/bosh/director/jobs/dynamic_disks/detach_dynamic_disk.rb index b93f30e98c2..2f96fce17df 100644 --- a/src/bosh-director/lib/bosh/director/jobs/dynamic_disks/detach_dynamic_disk.rb +++ b/src/bosh-director/lib/bosh/director/jobs/dynamic_disks/detach_dynamic_disk.rb @@ -2,6 +2,7 @@ module Bosh::Director module Jobs::DynamicDisks class DetachDynamicDisk < Jobs::BaseJob include Jobs::Helpers::DynamicDiskHelpers + include LockHelper @queue = :normal @@ -26,7 +27,7 @@ def perform agent_client = AgentClient.with_agent_id(disk_model.vm.agent_id, instance_name) agent_client.remove_dynamic_disk(disk_model.disk_cid) - cloud.detach_disk(disk_model.vm.cid, disk_model.disk_cid) + with_vm_lock(vm_cid, timeout: VM_LOCK_TIMEOUT) { cloud.detach_disk(vm_cid, disk_model.disk_cid) } disk_model.update(vm_id: nil, disk_hint: '') diff --git a/src/bosh-director/lib/bosh/director/jobs/dynamic_disks/provide_dynamic_disk.rb b/src/bosh-director/lib/bosh/director/jobs/dynamic_disks/provide_dynamic_disk.rb index 5e37804d386..53c0191c149 100644 --- a/src/bosh-director/lib/bosh/director/jobs/dynamic_disks/provide_dynamic_disk.rb +++ b/src/bosh-director/lib/bosh/director/jobs/dynamic_disks/provide_dynamic_disk.rb @@ -2,6 +2,7 @@ module Bosh::Director module Jobs::DynamicDisks class ProvideDynamicDisk < Jobs::BaseJob include Jobs::Helpers::DynamicDiskHelpers + include LockHelper @queue = :normal @@ -42,7 +43,7 @@ def perform end if disk_model.vm.nil? - disk_hint = cloud.attach_disk(vm.cid, disk_model.disk_cid) + 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) else if disk_model.vm.id != vm.id 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 200bb386fc7..2e46f7736e1 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 @@ -1,6 +1,8 @@ module Bosh::Director module Jobs::Helpers module DynamicDiskHelpers + VM_LOCK_TIMEOUT = 60 + def find_disk_cloud_properties(instance, disk_pool_name) teams = instance.deployment.teams configs = Models::Config.latest_set_for_teams('cloud', *teams) diff --git a/src/bosh-director/lib/bosh/director/lock_helper.rb b/src/bosh-director/lib/bosh/director/lock_helper.rb index a3237a7c4e3..4a471414332 100644 --- a/src/bosh-director/lib/bosh/director/lock_helper.rb +++ b/src/bosh-director/lib/bosh/director/lock_helper.rb @@ -16,6 +16,12 @@ def locked_deployments Models::Lock.where { Sequel.like(:name, 'lock:deployment:%') & (expired_at > Time.now) }.all end + def with_vm_lock(vm_cid, opts = {}) + timeout = opts[:timeout] || 10 + Config.logger.info("Acquiring VM lock on #{vm_cid}") + Lock.new("lock:vm:#{vm_cid}", timeout: timeout).lock { yield } + end + def with_network_lock(name, opts = {}) timeout = opts[:timeout] || 10 Config.logger.info("Acquiring network lock on #{name}") diff --git a/src/bosh-director/lib/bosh/director/problem_handlers/inactive_disk.rb b/src/bosh-director/lib/bosh/director/problem_handlers/inactive_disk.rb index e3ef9a99e11..66031369798 100644 --- a/src/bosh-director/lib/bosh/director/problem_handlers/inactive_disk.rb +++ b/src/bosh-director/lib/bosh/director/problem_handlers/inactive_disk.rb @@ -81,7 +81,7 @@ def delete_disk if active_vm begin cloud = CloudFactory.create.get(active_vm.cpi, active_vm.stemcell_api_version) - cloud.detach_disk(active_vm.cid, @disk.disk_cid) + with_vm_lock(active_vm.cid) { cloud.detach_disk(active_vm.cid, @disk.disk_cid) } rescue => e # We are going to delete this disk anyway # and we know it's not in use, so we can ignore diff --git a/src/bosh-director/lib/bosh/director/problem_handlers/missing_disk.rb b/src/bosh-director/lib/bosh/director/problem_handlers/missing_disk.rb index e49f12636a2..c433093acc6 100644 --- a/src/bosh-director/lib/bosh/director/problem_handlers/missing_disk.rb +++ b/src/bosh-director/lib/bosh/director/problem_handlers/missing_disk.rb @@ -75,7 +75,7 @@ def delete_disk_reference begin @logger.debug('Sending cpi request: detach_disk') - cloud.detach_disk(@instance.vm_cid, @disk.disk_cid) + with_vm_lock(@instance.vm_cid) { cloud.detach_disk(@instance.vm_cid, @disk.disk_cid) } rescue Bosh::Clouds::DiskNotAttached, Bosh::Clouds::DiskNotFound end end diff --git a/src/bosh-director/lib/bosh/director/problem_handlers/mount_info_mismatch.rb b/src/bosh-director/lib/bosh/director/problem_handlers/mount_info_mismatch.rb index 943d87cb4a7..2205a4e0c00 100644 --- a/src/bosh-director/lib/bosh/director/problem_handlers/mount_info_mismatch.rb +++ b/src/bosh-director/lib/bosh/director/problem_handlers/mount_info_mismatch.rb @@ -67,7 +67,7 @@ def instance_group def reattach_disk(reboot = false) az_cloud_factory = AZCloudFactory.create_with_latest_configs(@instance.deployment) cloud_for_attach_disk = az_cloud_factory.get_for_az(@instance.availability_zone, @vm_stemcell_api_version) - disk_hint = cloud_for_attach_disk.attach_disk(@vm_cid, @disk_cid) + disk_hint = with_vm_lock(@vm_cid) { cloud_for_attach_disk.attach_disk(@vm_cid, @disk_cid) } cloud_for_update_metadata = az_cloud_factory.get_for_az(@instance.availability_zone) MetadataUpdater.build.update_disk_metadata(cloud_for_update_metadata, @disk, @disk.instance.deployment.tags) diff --git a/src/bosh-director/lib/bosh/director/vm_deleter.rb b/src/bosh-director/lib/bosh/director/vm_deleter.rb index 2cdd7a31fe6..7b114a2f505 100644 --- a/src/bosh-director/lib/bosh/director/vm_deleter.rb +++ b/src/bosh-director/lib/bosh/director/vm_deleter.rb @@ -1,5 +1,7 @@ module Bosh::Director class VmDeleter + include LockHelper + def initialize(logger, force = false, enable_virtual_delete_vm = false) @logger = logger @error_ignorer = ErrorIgnorer.new(force, @logger) @@ -20,7 +22,7 @@ def delete_vm_by_cid(cid, stemcell_api_version, cpi_name = nil) @logger.info('Deleting VM') @error_ignorer.with_force_check do cloud_factory = CloudFactory.create - cloud_factory.get(cpi_name, stemcell_api_version).delete_vm(cid) unless @enable_virtual_delete_vm + with_vm_lock(cid) { cloud_factory.get(cpi_name, stemcell_api_version).delete_vm(cid) } unless @enable_virtual_delete_vm end end diff --git a/src/bosh-director/spec/unit/bosh/director/cloudcheck_helper_spec.rb b/src/bosh-director/spec/unit/bosh/director/cloudcheck_helper_spec.rb index a606e901afe..edbd57123f4 100644 --- a/src/bosh-director/spec/unit/bosh/director/cloudcheck_helper_spec.rb +++ b/src/bosh-director/spec/unit/bosh/director/cloudcheck_helper_spec.rb @@ -119,7 +119,8 @@ def fake_job_context expect(cloud_factory).to receive(:get).with(instance.active_vm.cpi).and_return(cloud) end - it 'reboots the vm on success' do + it 'reboots the vm on success with vm lock' do + expect(test_problem_handler).to receive(:with_vm_lock).with(vm.cid).and_yield allow(agent_client).to receive(:wait_until_ready) test_problem_handler.reboot_vm(instance) end diff --git a/src/bosh-director/spec/unit/bosh/director/deployment_plan/steps/attach_disk_step_spec.rb b/src/bosh-director/spec/unit/bosh/director/deployment_plan/steps/attach_disk_step_spec.rb index 33aaa040682..65da4cad93e 100644 --- a/src/bosh-director/spec/unit/bosh/director/deployment_plan/steps/attach_disk_step_spec.rb +++ b/src/bosh-director/spec/unit/bosh/director/deployment_plan/steps/attach_disk_step_spec.rb @@ -19,6 +19,7 @@ module Steps let(:meta_updater) { instance_double(MetadataUpdater, update_disk_metadata: nil) } let(:report) { instance_double(Stages::Report).as_null_object } let(:agent_client) { instance_double(AgentClient).as_null_object } + let(:job) { instance_double(Bosh::Director::Jobs::BaseJob, username: 'user', task_id: 42) } before do allow(CloudFactory).to receive(:create).and_return(cloud_factory) @@ -27,9 +28,11 @@ module Steps allow(cloud).to receive(:attach_disk) allow(MetadataUpdater).to receive(:build).and_return(meta_updater) allow(AgentClient).to receive(:with_agent_id).and_return(agent_client) + allow(Config).to receive(:current_job).and_return(job) end - it 'calls out to cpi associated with disk to attach disk' do + it 'calls out to cpi associated with disk to attach disk with vm lock' do + expect(step).to receive(:with_vm_lock).with(vm.cid).and_yield expect(cloud_factory).to receive(:get).with(disk&.cpi, stemcell_api_version).once.and_return(cloud) expect(cloud).to receive(:attach_disk).with(vm.cid, disk.disk_cid) @@ -68,6 +71,7 @@ module Steps end it 'logs notification of attaching' do + expect(per_spec_logger).to receive(:info).with("Acquiring VM lock on #{vm.cid}") expect(per_spec_logger).to receive(:info).with("Attaching disk #{disk.disk_cid}") step.perform(report) diff --git a/src/bosh-director/spec/unit/bosh/director/deployment_plan/steps/delete_vm_step_spec.rb b/src/bosh-director/spec/unit/bosh/director/deployment_plan/steps/delete_vm_step_spec.rb index 826022186bb..f152cf7c595 100644 --- a/src/bosh-director/spec/unit/bosh/director/deployment_plan/steps/delete_vm_step_spec.rb +++ b/src/bosh-director/spec/unit/bosh/director/deployment_plan/steps/delete_vm_step_spec.rb @@ -58,11 +58,12 @@ module Steps it 'deletes the instances vm and stores an event' do expect(job).to receive(:event_manager).twice.and_return(event_manager) - expect(job).to receive(:username).twice.and_return('fake-username') - expect(job).to receive(:task_id).twice.and_return('fake-task-id') + expect(job).to receive(:username).exactly(4).times.and_return('fake-username') + expect(job).to receive(:task_id).exactly(3).times.and_return('fake-task-id') expect(per_spec_logger).to receive(:info).with('Deleting VM') - expect(Config).to receive(:current_job).and_return(job).exactly(6).times + expect(per_spec_logger).to receive(:info).with('Acquiring VM lock on vm-cid') + expect(Config).to receive(:current_job).and_return(job).exactly(9).times expect(Models::LocalDnsRecord.all).to eq([local_dns_record]) expect(cloud).to receive(:delete_vm).with('vm-cid') @@ -100,6 +101,7 @@ module Steps context 'when vm has already been deleted from the IaaS' do it 'should log a warning' do expect(per_spec_logger).to receive(:info).with('Deleting VM') + expect(per_spec_logger).to receive(:info).with('Acquiring VM lock on vm-cid') expect(per_spec_logger).to receive(:warn) .with("VM '#{vm_model.cid}' might have already been deleted from the cloud") expect(cloud).to receive(:delete_vm).with(vm_model.cid).and_raise Bosh::Clouds::VMNotFound @@ -131,11 +133,12 @@ module Steps it 'creates requests a cloud instance with that stemcell api version' do expect(cloud_factory).to receive(:get).with('cpi1', 25).and_return(cloud) expect(job).to receive(:event_manager).twice.and_return(event_manager) - expect(job).to receive(:username).twice.and_return('fake-username') - expect(job).to receive(:task_id).twice.and_return('fake-task-id') + expect(job).to receive(:username).exactly(4).times.and_return('fake-username') + expect(job).to receive(:task_id).exactly(3).times.and_return('fake-task-id') expect(per_spec_logger).to receive(:info).with('Deleting VM') - expect(Config).to receive(:current_job).and_return(job).exactly(6).times + expect(per_spec_logger).to receive(:info).with('Acquiring VM lock on vm-cid') + expect(Config).to receive(:current_job).and_return(job).exactly(9).times expect(Models::LocalDnsRecord.all).to eq([local_dns_record]) expect(cloud).to receive(:delete_vm).with('vm-cid') @@ -146,6 +149,7 @@ module Steps context 'when trying to delete VM mutiple times' do it 'deletes the instances vm and stores an event' do expect(per_spec_logger).to receive(:info).with('Deleting VM').twice + expect(per_spec_logger).to receive(:info).with('Acquiring VM lock on vm-cid').twice expect(Models::LocalDnsRecord.all).to eq([local_dns_record]) expect(cloud).to receive(:delete_vm).with('vm-cid').twice diff --git a/src/bosh-director/spec/unit/bosh/director/deployment_plan/steps/detach_disk_step_spec.rb b/src/bosh-director/spec/unit/bosh/director/deployment_plan/steps/detach_disk_step_spec.rb index 10d50d43e12..a36d3ea7135 100644 --- a/src/bosh-director/spec/unit/bosh/director/deployment_plan/steps/detach_disk_step_spec.rb +++ b/src/bosh-director/spec/unit/bosh/director/deployment_plan/steps/detach_disk_step_spec.rb @@ -15,12 +15,14 @@ module Steps let(:agent_client) do instance_double(AgentClient, list_disk: [disk&.disk_cid], remove_persistent_disk: nil) end + let(:job) { instance_double(Bosh::Director::Jobs::BaseJob, username: 'user', task_id: 42) } before do allow(AgentClient).to receive(:with_agent_id).with(vm.agent_id, vm.instance.name).and_return(agent_client) allow(CloudFactory).to receive(:create).and_return(cloud_factory) allow(cloud_factory).to receive(:get).with(disk&.cpi, 25).once.and_return(cloud) allow(cloud).to receive(:detach_disk) + allow(Config).to receive(:current_job).and_return(job) end it 'sends remove_persistent_disk method to agent' do @@ -29,8 +31,9 @@ module Steps step.perform(report) end - it 'calls out to cpi associated with disk to detach disk' do + it 'calls out to cpi associated with disk to detach disk with vm lock' do expect(cloud_factory).to receive(:get).with(disk&.cpi, 25).once.and_return(cloud) + expect(step).to receive(:with_vm_lock).with(vm.cid).and_yield expect(cloud).to receive(:detach_disk).with(vm.cid, disk.disk_cid) step.perform(report) @@ -38,6 +41,7 @@ module Steps it 'logs notification of detaching' do expect(per_spec_logger).to receive(:info).with("Detaching disk #{disk.disk_cid}") + expect(per_spec_logger).to receive(:info).with("Acquiring VM lock on #{vm.cid}") step.perform(report) end diff --git a/src/bosh-director/spec/unit/bosh/director/instance_deleter_spec.rb b/src/bosh-director/spec/unit/bosh/director/instance_deleter_spec.rb index ee87dfee2c4..2c0cb8b5295 100644 --- a/src/bosh-director/spec/unit/bosh/director/instance_deleter_spec.rb +++ b/src/bosh-director/spec/unit/bosh/director/instance_deleter_spec.rb @@ -120,7 +120,7 @@ module Bosh::Director expect do deleter.delete_instance_plans([instance_plan], event_log_stage) - end.to change { Bosh::Director::Models::Event.count }.from(0).to(8) + end.to change { Bosh::Director::Models::Event.count }.from(0).to(10) event1 = Bosh::Director::Models::Event.order(:id).first expect(event1.user).to eq(task.username) @@ -204,7 +204,7 @@ module Bosh::Director expect do deleter.delete_instance_plans([instance_plan], event_log_stage) - end.to change { Bosh::Director::Models::Event.count }.from(0).to(8) + end.to change { Bosh::Director::Models::Event.count }.from(0).to(10) end end diff --git a/src/bosh-director/spec/unit/bosh/director/jobs/delete_vm_spec.rb b/src/bosh-director/spec/unit/bosh/director/jobs/delete_vm_spec.rb index f0aa7596def..a56b507d388 100644 --- a/src/bosh-director/spec/unit/bosh/director/jobs/delete_vm_spec.rb +++ b/src/bosh-director/spec/unit/bosh/director/jobs/delete_vm_spec.rb @@ -76,7 +76,7 @@ module Bosh::Director expect(cloud).to receive(:delete_vm).with(vm_cid) expect do job.perform - end.to change { Bosh::Director::Models::Event.count }.by(2) + end.to change { Bosh::Director::Models::Event.count }.by(4) event1 = Bosh::Director::Models::Event.order_by(:id).first expect(event1.user).to eq(task.username) diff --git a/src/bosh-director/spec/unit/bosh/director/jobs/dynamic_disks/detach_dynamic_disk_spec.rb b/src/bosh-director/spec/unit/bosh/director/jobs/dynamic_disks/detach_dynamic_disk_spec.rb index 28ccfdf47b0..fab99df5bd6 100644 --- a/src/bosh-director/spec/unit/bosh/director/jobs/dynamic_disks/detach_dynamic_disk_spec.rb +++ b/src/bosh-director/spec/unit/bosh/director/jobs/dynamic_disks/detach_dynamic_disk_spec.rb @@ -53,6 +53,7 @@ module Bosh::Director end it 'detaches the disk' do + expect(detach_dynamic_disk_job).to receive(:with_vm_lock).with(vm.cid, timeout: 60).and_yield expect(cloud).to receive(:detach_disk).with(vm.cid, disk_cid) expect(agent_client).to receive(:remove_dynamic_disk).with(disk_cid) expect(detach_dynamic_disk_job.perform).to eq("detached disk `#{disk_cid}` from vm `#{vm.cid}`") @@ -63,6 +64,7 @@ module Bosh::Director context 'when disk is already detached' do it 'does not return an error' do + expect(detach_dynamic_disk_job).to receive(:with_vm_lock).with(vm.cid, timeout: 60).and_yield expect(cloud).to receive(:detach_disk).with(vm.cid, disk_cid).and_raise(Bosh::Clouds::DiskNotAttached.new(false)) expect(agent_client).to receive(:remove_dynamic_disk).with(disk_cid) expect(detach_dynamic_disk_job.perform).to eq("disk `#{disk_cid}` was already detached") diff --git a/src/bosh-director/spec/unit/bosh/director/jobs/dynamic_disks/provide_dynamic_disk_spec.rb b/src/bosh-director/spec/unit/bosh/director/jobs/dynamic_disks/provide_dynamic_disk_spec.rb index 5cce4270e65..6853a338c66 100644 --- a/src/bosh-director/spec/unit/bosh/director/jobs/dynamic_disks/provide_dynamic_disk_spec.rb +++ b/src/bosh-director/spec/unit/bosh/director/jobs/dynamic_disks/provide_dynamic_disk_spec.rb @@ -64,6 +64,7 @@ def parsed_task_result it 'attaches the disk to VM and updates disk vm and availability zone' do expect(cloud).not_to receive(:create_disk) + expect(provide_dynamic_disk_job).to receive(:with_vm_lock).with(vm.cid, timeout: 60).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(provide_dynamic_disk_job.perform).to eq("attached disk `#{disk_name}` to `#{vm.cid}` in deployment `#{vm.instance.deployment.name}`") @@ -79,6 +80,7 @@ def parsed_task_result it 'creates the disk and attaches it to VM' do expected_cloud_properties = disk_cloud_properties.merge('name' => 'fake-disk-name') expect(cloud).to receive(:create_disk).with(disk_size, expected_cloud_properties, vm.cid).and_return(disk_cid) + expect(provide_dynamic_disk_job).to receive(:with_vm_lock).with(vm.cid, timeout: 60).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) @@ -109,6 +111,7 @@ def parsed_task_result end it 'returns an error from attach_disk call' do + expect(provide_dynamic_disk_job).to receive(:with_vm_lock).with(vm.cid, timeout: 60).and_yield expect(cloud).to receive(:attach_disk).with(vm.cid, disk_cid).and_raise('some-error') expect { provide_dynamic_disk_job.perform }.to raise_error('some-error') end @@ -146,6 +149,7 @@ def parsed_task_result "fake-key" => "fake-value" } ) + expect(provide_dynamic_disk_job).to receive(:with_vm_lock).with(vm.cid, timeout: 60).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) @@ -179,6 +183,7 @@ def parsed_task_result it 'gets the disk cloud properties from the latest cloud config for those teams' do expected_cloud_properties = disk_cloud_properties.merge('name' => 'fake-disk-name') expect(cloud).to receive(:create_disk).with(disk_size, expected_cloud_properties, vm.cid).and_return(disk_cid) + expect(provide_dynamic_disk_job).to receive(:with_vm_lock).with(vm.cid, timeout: 60).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(provide_dynamic_disk_job.perform).to eq("attached disk `#{disk_name}` to `#{vm.cid}` in deployment `#{vm.instance.deployment.name}`") diff --git a/src/bosh-director/spec/unit/bosh/director/lock_helper_spec.rb b/src/bosh-director/spec/unit/bosh/director/lock_helper_spec.rb index cf885c506e2..50e8494130c 100644 --- a/src/bosh-director/spec/unit/bosh/director/lock_helper_spec.rb +++ b/src/bosh-director/spec/unit/bosh/director/lock_helper_spec.rb @@ -124,6 +124,20 @@ class TestClass end end + describe :with_vm_lock do + it 'creates a lock for the given vm cid' do + lock = double(:lock) + allow(Lock).to receive(:new).with('lock:vm:bar', timeout: 5).and_return(lock) + expect(lock).to receive(:lock).and_yield + + called = false + @test_instance.with_vm_lock('bar', timeout: 5) do + called = true + end + expect(called).to be(true) + end + end + describe :with_release_lock do it 'creates a lock for the given name' do lock = double(:lock) diff --git a/src/bosh-director/spec/unit/bosh/director/problem_handlers/inactive_disk_spec.rb b/src/bosh-director/spec/unit/bosh/director/problem_handlers/inactive_disk_spec.rb index 48f459c3bca..e8da45cd59d 100644 --- a/src/bosh-director/spec/unit/bosh/director/problem_handlers/inactive_disk_spec.rb +++ b/src/bosh-director/spec/unit/bosh/director/problem_handlers/inactive_disk_spec.rb @@ -108,8 +108,9 @@ def make_handler(disk_id, data = {}) }.to raise_error(Bosh::Director::ProblemHandlerError, 'Disk is currently in use') end - it 'detaches disk from VM and deletes it and its snapshots from DB (if instance has VM)' do + it 'detaches disk from VM with VM lock and deletes it and its snapshots from DB (if instance has VM)' do expect(@agent).to receive(:list_disk).and_return(['other-disk']) + expect(@handler).to receive(:with_vm_lock).with(@instance.vm_cid).and_yield expect(cloud).to receive(:detach_disk).with(@instance.vm_cid, 'disk-cid') expect(cloud_factory).to receive(:get).with(@instance.active_vm.cpi, 25).and_return(cloud) @handler.apply_resolution(:delete_disk) diff --git a/src/bosh-director/spec/unit/bosh/director/problem_handlers/missing_disk_spec.rb b/src/bosh-director/spec/unit/bosh/director/problem_handlers/missing_disk_spec.rb index 1193c426319..cb8dfff3211 100644 --- a/src/bosh-director/spec/unit/bosh/director/problem_handlers/missing_disk_spec.rb +++ b/src/bosh-director/spec/unit/bosh/director/problem_handlers/missing_disk_spec.rb @@ -92,12 +92,13 @@ def self.it_ignores_cloud_disk_errors it_ignores_cloud_disk_errors - it 'deactivates, unmounts, detaches, deletes snapshots, deletes disk reference' do + it 'deactivates, unmounts, detaches with VM lock, deletes snapshots, deletes disk reference' do expect(agent_client).to receive(:unmount_disk) do db_disk = Bosh::Director::Models::PersistentDisk[disk.id] expect(db_disk.active).to be(false) end.ordered + expect(handler).to receive(:with_vm_lock).with('vm-cid').and_yield.ordered expect(cloud).to receive(:detach_disk).with('vm-cid', 'disk-cid').ordered handler.apply_resolution(:delete_disk_reference) @@ -115,6 +116,7 @@ def self.it_ignores_cloud_disk_errors end it 'detaches disk, deletes snapshots, deletes disk reference' do + expect(handler).to receive(:with_vm_lock).with('vm-cid').and_yield.ordered expect(cloud).to receive(:detach_disk).with('vm-cid', 'disk-cid').ordered handler.apply_resolution(:delete_disk_reference) @@ -132,6 +134,7 @@ def self.it_ignores_cloud_disk_errors end it 'raises the error' do + expect(handler).to_not receive(:with_vm_lock).with('vm-cid').and_yield.ordered expect(cloud).to_not receive(:detach_disk).with('vm-cid', 'disk-cid').ordered expect(cloud).to_not receive(:delete_snapshot).with('snapshot-cid').ordered @@ -155,6 +158,7 @@ def self.it_ignores_cloud_disk_errors it 'deactivates, detaches, deletes snapshots, deletes disk reference' do expect(agent_client).to_not receive(:unmount_disk) + expect(handler).to receive(:with_vm_lock).with('vm-cid').and_yield.ordered expect(cloud).to receive(:detach_disk).with('vm-cid', 'disk-cid') do db_disk = Bosh::Director::Models::PersistentDisk[disk.id] expect(db_disk.active).to be(false) @@ -177,6 +181,7 @@ def self.it_ignores_cloud_disk_errors it 'detaches disk, deletes snapshots, deletes disk reference' do expect(agent_client).to_not receive(:unmount_disk) + expect(handler).to receive(:with_vm_lock).with('vm-cid').and_yield.ordered expect(cloud).to receive(:detach_disk).ordered handler.apply_resolution(:delete_disk_reference) @@ -197,6 +202,7 @@ def self.it_ignores_cloud_disk_errors it 'deletes snapshots, deletes disk reference' do expect(agent_client).to_not receive(:unmount_disk) + expect(handler).to_not receive(:with_vm_lock) expect(cloud).to_not receive(:detach_disk) handler.apply_resolution(:delete_disk_reference) diff --git a/src/bosh-director/spec/unit/bosh/director/problem_handlers/mount_info_mismatch_spec.rb b/src/bosh-director/spec/unit/bosh/director/problem_handlers/mount_info_mismatch_spec.rb index 6d139c33f6b..77028b42fd1 100644 --- a/src/bosh-director/spec/unit/bosh/director/problem_handlers/mount_info_mismatch_spec.rb +++ b/src/bosh-director/spec/unit/bosh/director/problem_handlers/mount_info_mismatch_spec.rb @@ -11,6 +11,7 @@ def make_handler(disk_id, data = {}) let(:manifest) do { 'tags' => { 'mytag' => 'myvalue' } } end + let(:current_job) { instance_double(Bosh::Director::Jobs::CloudCheck::ApplyResolutions, username: 'user', task_id: 42) } before(:each) do @agent = double('agent') @@ -37,6 +38,7 @@ def make_handler(disk_id, data = {}) allow(Bosh::Director::AZCloudFactory).to receive(:create_from_deployment).and_return(az_cloud_factory) allow(Bosh::Director::CloudFactory).to receive(:create).and_return(base_cloud_factory) allow(az_cloud_factory).to receive(:get_for_az).with('az1', 25).and_return(cloud) + allow(Bosh::Director::Config).to receive(:current_job).and_return(current_job) end it 'registers under inactive_disk type' do @@ -96,6 +98,7 @@ def make_handler(disk_id, data = {}) end it 'attaches disk and reboots the vm' do + expect(@handler).to receive(:with_vm_lock).twice.with(@instance.vm_cid).and_yield expect(cloud).to receive(:attach_disk).with(@instance.vm_cid, @disk.disk_cid) expect(cloud).to receive(:reboot_vm).with(@instance.vm_cid) expect(cloud_for_update_metadata).to_not receive(:attach_disk) @@ -107,6 +110,7 @@ def make_handler(disk_id, data = {}) end it 'sets disk metadata with deployment information' do + expect(@handler).to receive(:with_vm_lock).twice.with(@instance.vm_cid).and_yield expect(cloud).to receive(:attach_disk).with(@instance.vm_cid, @disk.disk_cid) expect(cloud).to receive(:reboot_vm).with(@instance.vm_cid) expect(cloud_for_update_metadata).to_not receive(:attach_disk) @@ -144,6 +148,7 @@ def make_handler(disk_id, data = {}) end it 'attaches disk and reboots the vm' do + expect(@handler).to receive(:with_vm_lock).twice.with(@instance.vm_cid).and_yield expect(cloud).to receive(:attach_disk).with(@instance.vm_cid, @disk.disk_cid).and_return(disk_hint) expect(cloud).to receive(:reboot_vm).with(@instance.vm_cid) expect(cloud_for_update_metadata).to_not receive(:attach_disk) diff --git a/src/bosh-director/spec/unit/bosh/director/vm_deleter_spec.rb b/src/bosh-director/spec/unit/bosh/director/vm_deleter_spec.rb index 1bcec0c5208..84564bfe0a9 100644 --- a/src/bosh-director/spec/unit/bosh/director/vm_deleter_spec.rb +++ b/src/bosh-director/spec/unit/bosh/director/vm_deleter_spec.rb @@ -8,6 +8,7 @@ module Director let(:cloud) { instance_double(Bosh::Clouds::ExternalCpi) } let(:cloud_factory) { instance_double(CloudFactory) } let(:deployment) { FactoryBot.create(:models_deployment, name: 'deployment_name') } + let(:current_job) { instance_double(Bosh::Director::Jobs::CloudCheck::ApplyResolutions, username: 'user', task_id: 42) } let(:vm_model) { FactoryBot.create(:models_vm, cid: 'vm-cid', instance_id: instance_model.id, cpi: 'cpi1') } let(:instance_model) do FactoryBot.create(:models_instance, @@ -30,6 +31,7 @@ module Director allow(CloudFactory).to receive(:create).and_return(cloud_factory) allow(cloud_factory).to receive(:get).with(nil, nil).and_return(cloud) + allow(Bosh::Director::Config).to receive(:current_job).and_return(current_job) end describe '#delete_for_instance' do @@ -95,8 +97,9 @@ module Director allow(cloud_factory).to receive(:get).with('cpi1', 25).and_return(cloud) end - it 'calls delete_vm' do + it 'calls delete_vm with vm lock' do expect(per_spec_logger).to receive(:info).with('Deleting VM') + expect(per_spec_logger).to receive(:info).with("Acquiring VM lock on vm-cid") expect(cloud).to receive(:delete_vm).with(vm_model.cid) subject.delete_vm_by_cid(vm_model.cid, 25, 'cpi1') end From b5402645c96e32ac8051150a0b78d57b4ecfb46a Mon Sep 17 00:00:00 2001 From: Maria Shaldybin Date: Fri, 12 Dec 2025 22:22:04 +0000 Subject: [PATCH 8/8] Add dedicated dynamic_disks workers * Change dynamic disks jobs to be in dynamic_disks queue * Add dynamic_disks_workers property to director * When dynamic_disks_workers is greater than 0, start separate dynamic_disks_worker process * Update drain to drain dynamic_disks workers --- jobs/director/monit | 13 +++- jobs/director/spec | 3 + jobs/director/templates/drain | 15 ++++- jobs/director/templates/ps_utils.sh | 2 +- jobs/director/templates/worker_ctl.erb | 16 ++--- spec/director_templates_spec.rb | 65 +++++++++++++++++-- spec/support/ps_utils_tests.sh | 8 +-- src/bosh-director/bin/bosh-director-worker | 10 +-- .../jobs/dynamic_disks/delete_dynamic_disk.rb | 2 +- .../jobs/dynamic_disks/detach_dynamic_disk.rb | 2 +- .../dynamic_disks/provide_dynamic_disk.rb | 2 +- src/bosh-director/lib/bosh/director/worker.rb | 6 +- .../dynamic_disks/delete_dynamic_disk_spec.rb | 6 ++ .../dynamic_disks/detach_dynamic_disk_spec.rb | 6 ++ .../provide_dynamic_disk_spec.rb | 6 ++ .../spec/unit/bosh/director/worker_spec.rb | 4 +- 16 files changed, 128 insertions(+), 38 deletions(-) diff --git a/jobs/director/monit b/jobs/director/monit index f21140259b8..d418410cc6c 100644 --- a/jobs/director/monit +++ b/jobs/director/monit @@ -7,8 +7,17 @@ check process director <% (1..(p('director.workers'))).each do |index| %> check process worker_<%= index %> with pidfile /var/vcap/sys/run/director/worker_<%= index %>.pid - start program "/var/vcap/jobs/director/bin/worker_ctl start <%= index %>" - stop program "/var/vcap/jobs/director/bin/worker_ctl stop <%= index %>" + start program "/var/vcap/jobs/director/bin/worker_ctl start worker_<%= index %>" + stop program "/var/vcap/jobs/director/bin/worker_ctl stop worker_<%= index %>" + group vcap + depends on director +<% end %> + +<% (1..(p('director.dynamic_disks_workers'))).each do |index| %> +check process dynamic_disks_worker_<%= index %> + with pidfile /var/vcap/sys/run/director/dynamic_disks_worker_<%= index %>.pid + start program "/var/vcap/jobs/director/bin/worker_ctl start dynamic_disks_worker_<%= index %> dynamic_disks" + stop program "/var/vcap/jobs/director/bin/worker_ctl stop dynamic_disks_worker_<%= index %> dynamic_disks" group vcap depends on director <% end %> diff --git a/jobs/director/spec b/jobs/director/spec index a7c88883521..32a39767ed6 100644 --- a/jobs/director/spec +++ b/jobs/director/spec @@ -75,6 +75,9 @@ properties: director.workers: description: Number of director workers default: 3 + director.dynamic_disks_workers: + description: Number of director workers dedicated for dynamic disks jobs + default: 0 director.enable_dedicated_status_worker: description: "Separate worker for 'bosh vms' and 'bosh ssh'" default: false diff --git a/jobs/director/templates/drain b/jobs/director/templates/drain index 8965f88989d..0ea66e40d70 100755 --- a/jobs/director/templates/drain +++ b/jobs/director/templates/drain @@ -18,7 +18,7 @@ mkdir -p $LOG_DIR export BOSH_DIRECTOR_LOG_FILE=$LOG_DIR/drain.workers.stdout.log stop_worker() { - pidfile=/var/vcap/sys/run/director/worker_$1.pid + pidfile=/var/vcap/sys/run/director/$1.pid if [ -f "$pidfile" ] ; then pid="$( cat "$pidfile" )" @@ -33,16 +33,25 @@ stop_dedicated_worker () { stop_worker "$@"; } <% (1..p('director.workers')).each do |index| %> <% if index > dedicated_workers %> - stop_worker <%= index %> + stop_worker worker_<%= index %> <% end %> <% end %> +<% (1..p('director.dynamic_disks_workers')).each do |index| %> + stop_worker dynamic_disks_worker_<%= index %> +<% end %> + <% if dedicated_workers > 0 %> /var/vcap/packages/director/bin/bosh-director-drain-workers -c /var/vcap/jobs/director/config/director.yml --queue normal \ 2>>$LOG_DIR/drain.stderr.log + <% if p('director.dynamic_disks_workers') > 0 %> + /var/vcap/packages/director/bin/bosh-director-drain-workers -c /var/vcap/jobs/director/config/director.yml --queue dynamic_disks \ + 2>>$LOG_DIR/drain.stderr.log + <% end %> + <% (1..dedicated_workers).each do |index| %> - stop_dedicated_worker <%= index %> + stop_dedicated_worker worker_<%= index %> <% end %> <% end %> diff --git a/jobs/director/templates/ps_utils.sh b/jobs/director/templates/ps_utils.sh index d59d08cd26f..fb588e81bed 100755 --- a/jobs/director/templates/ps_utils.sh +++ b/jobs/director/templates/ps_utils.sh @@ -22,7 +22,7 @@ function kill_process { function list_child_processes { ps -eo pid,command | grep bosh-director-worker | - grep -- "-i $1" | + grep -- "-n $1" | awk '{print $1}' | grep -v ^$1$ } diff --git a/jobs/director/templates/worker_ctl.erb b/jobs/director/templates/worker_ctl.erb index 15675695ffb..31367e005ba 100755 --- a/jobs/director/templates/worker_ctl.erb +++ b/jobs/director/templates/worker_ctl.erb @@ -1,10 +1,10 @@ #!/bin/bash -INDEX=$2 +NAME=$2 RUN_DIR=/var/vcap/sys/run/director LOG_DIR=/var/vcap/sys/log/director -PIDFILE=$RUN_DIR/worker_$INDEX.pid +PIDFILE=${RUN_DIR}/${NAME}.pid RUNAS=vcap # Postgres @@ -25,9 +25,9 @@ export LANG=en_US.UTF-8 export TMPDIR=/var/vcap/data/director/tmp -export QUEUE="normal,urgent" +export QUEUE="${3:-normal,urgent}" <% if (p('director.enable_dedicated_status_worker')) && (p('director.workers') > 1) %> -if [ $INDEX -eq 1 ]; then +if [ "${NAME}" = "worker_1" ]; then export QUEUE="urgent" fi <% end %> @@ -49,15 +49,15 @@ case $1 in exec chpst -u $RUNAS:$RUNAS \ /var/vcap/packages/director/bin/bosh-director-worker \ - -c /var/vcap/jobs/director/config/director.yml -i $INDEX \ - >>$LOG_DIR/worker_$INDEX.stdout.log \ - 2>>$LOG_DIR/worker_$INDEX.stderr.log + -c /var/vcap/jobs/director/config/director.yml -n $NAME \ + >>$LOG_DIR/${NAME}.stdout.log \ + 2>>$LOG_DIR/${NAME}.stderr.log ;; stop) PID=$(head -1 $PIDFILE) kill_process $PID # prevent the parent from fork()ing new children - for CHILD in $(list_child_processes $INDEX); do + for CHILD in $(list_child_processes $NAME); do kill_process $CHILD done rm -f $PIDFILE diff --git a/spec/director_templates_spec.rb b/spec/director_templates_spec.rb index 9534c7b3731..d69774e548b 100644 --- a/spec/director_templates_spec.rb +++ b/spec/director_templates_spec.rb @@ -271,25 +271,52 @@ let(:rendered_template) { template.render(properties) } let(:enable_dedicated_status_worker) { false } + let(:dynamic_disks_workers) { 0 } let(:properties) do properties = default_properties.dup properties['director']['enable_dedicated_status_worker'] = enable_dedicated_status_worker + properties['director']['dynamic_disks_workers'] = dynamic_disks_workers properties end - it 'renders' do - expect(rendered_template).to match(/.+stop_worker 1.+stop_worker 2.+stop_worker 3/m) - expect(rendered_template).to include('bosh-director-drain-workers') + it 'renders to drain all jobs' do + expect(rendered_template).to match(/.+stop_worker worker_1.+stop_worker worker_2.+stop_worker worker_3/m) + expect(rendered_template).to include('bosh-director-drain-workers -c /var/vcap/jobs/director/config/director.yml') + expect(rendered_template).to_not include('--queue normal') + expect(rendered_template).to_not include('--queue dynamic_disks') + end + + context 'dynamic disks workers' do + let(:dynamic_disks_workers) { 2 } + + it 'renders to drain all jobs' do + expect(rendered_template).to match(/.+stop_worker worker_2.+stop_worker worker_3.+.+stop_worker dynamic_disks_worker_1.+stop_worker dynamic_disks_worker_2/m) + + expect(rendered_template).to include('bosh-director-drain-workers -c /var/vcap/jobs/director/config/director.yml') + expect(rendered_template).to_not include('--queue normal') + expect(rendered_template).to_not include('--queue dynamic_disks') + end end context 'dedicated status workers' do let(:enable_dedicated_status_worker) { true } - it 'renders' do - expect(rendered_template).to match(/.+stop_worker 2.+stop_worker 3.+stop_dedicated_worker 1/m) + it 'renders to drain normal jobs and then the rest' do + expect(rendered_template).to match(/.+stop_worker worker_2.+stop_worker worker_3.+stop_dedicated_worker worker_1/m) + + expect(rendered_template).to include('bosh-director-drain-workers -c /var/vcap/jobs/director/config/director.yml --queue normal') + expect(rendered_template).to_not include('bosh-director-drain-workers -c /var/vcap/jobs/director/config/director.yml --queue dynamic_disks') + end + + context 'dynamic disks workers' do + let(:dynamic_disks_workers) { 2 } - expect(rendered_template).to include('--queue normal') - expect(rendered_template).to include('bosh-director-drain-workers') + it 'renders to drain normal jobs, then dynamic_disks jobs and then the rest' do + expect(rendered_template).to match(/.+stop_worker worker_2.+stop_worker worker_3.+.+stop_worker dynamic_disks_worker_1.+stop_worker dynamic_disks_worker_2.+stop_dedicated_worker worker_1/m) + + expect(rendered_template).to include('bosh-director-drain-workers -c /var/vcap/jobs/director/config/director.yml --queue normal') + expect(rendered_template).to include('bosh-director-drain-workers -c /var/vcap/jobs/director/config/director.yml --queue dynamic_disks') + end end end end @@ -438,6 +465,30 @@ end end end + + describe 'worker_ctl' do + let(:template) { job.template('bin/worker_ctl') } + let(:rendered_template) { template.render(properties) } + + let(:enable_dedicated_status_worker) { false } + let(:properties) do + properties = default_properties.dup + properties['director']['enable_dedicated_status_worker'] = enable_dedicated_status_worker + properties + end + + it 'renders' do + expect(rendered_template).to include('export QUEUE="${3:-normal,urgent}"') + end + + context 'dedicated status workers' do + let(:enable_dedicated_status_worker) { true } + + it 'renders' do + expect(rendered_template).to include('export QUEUE="urgent"') + end + end + end end end diff --git a/spec/support/ps_utils_tests.sh b/spec/support/ps_utils_tests.sh index b51dcbc7797..dbf4af90d43 100755 --- a/spec/support/ps_utils_tests.sh +++ b/spec/support/ps_utils_tests.sh @@ -39,14 +39,14 @@ function test_kill_process { function test_list_child_processes { function ps { cat <