Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Allow to receive datastore by name or by id #15101

Closed
wants to merge 1 commit into from
Closed

Allow to receive datastore by name or by id #15101

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 6 additions & 0 deletions app/models/storage.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -412,6 +412,12 @@ def total_unmanaged_vms
.each_with_object(Hash.new(0)) { |(storage_id, count), h| h[storage_id] = count.to_i }
end

# searches for a storage by its ID or name, while the ID gets precedence over the name if both provided
def self.search_by_id_or_name(storage_id, storage_name)
return Storage.find_by(:id => storage_id) if storage_id
return Storage.find_by(:name => storage_name) if storage_name
end

def count_of_vmdk_disk_files
self.class.count_of_vmdk_disk_files[id]
end
Expand Down
4 changes: 2 additions & 2 deletions app/models/vm_or_template/operations/configuration.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -73,8 +73,8 @@ def disconnect_floppies

def raw_add_disk(disk_name, disk_size_mb, options = {})
raise _("VM has no EMS, unable to add disk") unless ext_management_system
if options[:datastore]
datastore = Storage.find_by(:name => options[:datastore])
if options[:datastore] || options[:datastore_id]
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @masayag - renaming :datastore to datastore_name would offer more clarity, as : datastore_id does

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@bronaghs, @abellotti wouldn't it be considered as breaking the API ?

In [1] the datastore is exposed as diskscsi##.datastore=<datastore name>, therefore changing it to datastore_name will cause any existing users/customer scripts not to work.

I can add diskscsi##.datastore_name in addition, with the same semantics as the diskscsi##.datastore, and later on to deprecate the latter, but I'm not familiar with the API deprecation policy.

[1] https://access.redhat.com/documentation/en-US/Red_Hat_CloudForms/3.2/html/Red_Hat_CloudForms_SOAP_API/Defining_new_disks_during_provisioning.html

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I will defer to @abellotti on using datastore_name in addition.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@gmcculloug could you review ?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we skip all this logic and just look for the datastore based on the ext_management_system handle to avoid this issue?

datastore = ext_management_system.storages.find_by(:name => options[:datastore])

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@gmcculloug that's good idea. I'll update the PR to address it.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

hmm, I thought the root cause is having a datastore with the same name twice under the the same ems. If this is the case, then scoping it to ems does not help

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed by #15245, closing the current one.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@durandom the use case in which datastore appears twice for the same RHV provider happened when the provider was first added to manageiq, then removed and added again.

The previous datastore remained on cmdb, however, its association with any provider was removed. Therefore the code above will allow to select only a datastore which is associated with a datastore related to the provider in which the vm is about to be added.

However, @agrare raised an issue on ManageIQ/manageiq-providers-ovirt#42 where VMware might have several datastores with the same name.

datastore = Storage.search_by_id_or_name(options[:datastore_id], options[:datastore])
raise _("Data Store does not exist, unable to add disk") unless datastore
end

Expand Down
59 changes: 59 additions & 0 deletions spec/models/storage_spec.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -542,4 +542,63 @@
expect(storage.tenant_identity.current_tenant).to eq(Tenant.root_tenant)
end
end

context "#search_by_id_or_name" do
let(:storage_name) { "test_storage" }
let(:storage_id) { 123 }

context "when storage exists" do
let(:storage) { FactoryGirl.create(:storage, :name => storage_name, :id => storage_id) }

it "searches for a storage by specifying only the ID" do
allow(Storage).to receive(:find_by).with(:id => storage_id).and_return(storage)

expect(Storage).not_to receive(:find_by).with(:name => storage_name)
expect(Storage.search_by_id_or_name(storage_id, nil)).to eq(storage)
end

it "searches for a storage by specifying only the name" do
allow(Storage).to receive(:find_by).with(:name => storage_name).and_return(storage)

expect(Storage).not_to receive(:find_by).with(:id => storage_id)
expect(Storage.search_by_id_or_name(nil, storage_name)).to eq(storage)
end

it "searches for a storage both by its ID and name" do
allow(Storage).to receive(:find_by).with(:id => storage_id).and_return(storage)

expect(Storage).not_to receive(:find_by).with(:name => storage_name)
expect(Storage.search_by_id_or_name(storage_id, storage_name)).to eq(storage)
end
end

context "when storage does not exist" do
it "searches for a storage by specifying only the ID" do
allow(Storage).to receive(:find_by).with(:id => storage_id).and_return(nil)

expect(Storage).not_to receive(:find_by).with(:name => storage_name)
expect(Storage.search_by_id_or_name(storage_id, nil)).to eq(nil)
end

it "searches for a storage by specifying only the name" do
allow(Storage).to receive(:find_by).with(:name => storage_name).and_return(nil)

expect(Storage).not_to receive(:find_by).with(:id => storage_id)
expect(Storage.search_by_id_or_name(nil, storage_name)).to eq(nil)
end

it "searches for a storage both by its ID and name" do
allow(Storage).to receive(:find_by).with(:id => storage_id).and_return(nil)

expect(Storage).not_to receive(:find_by).with(:name => storage_name)
expect(Storage.search_by_id_or_name(storage_id, storage_name)).to eq(nil)
end

it "searches for a storage without specifying ID or name" do
expect(Storage).not_to receive(:find_by).with(:id => storage_id)
expect(Storage).not_to receive(:find_by).with(:name => storage_name)
expect(Storage.search_by_id_or_name(nil, nil)).to eq(nil)
end
end
end
end