Allow to receive datastore by name or by id #15101
Conversation
|
Another PR was proposed to ManageIQ/manageiq-providers-ovirt#37 in order to support the new parameter. |
| if options[:datastore] | ||
| datastore = Storage.find_by(:name => options[:datastore]) | ||
| if options[:datastore] || options[:datastore_id] | ||
| datastore = Storage.find(:name => options[:datastore_id]) if options[:datastore_id] |
There was a problem hiding this comment.
why the query on :name matching the datastore_id?
There was a problem hiding this comment.
This is a mistake. I pulled the same code which is common ovirt-provider and automation to Storage and covered with tests.
Datastores on the same setup may have the same name. In order to add a disk on a specific datastore, it is better to use the datastore id. https://bugzilla.redhat.com/show_bug.cgi?id=1450952
masayag
force-pushed
the
accept_datastore_id_when_adding_disk
branch
from
5dc5ba4
to
1e2a674
Compare
|
Checked commit masayag@1e2a674 with ruby 2.2.6, rubocop 0.47.1, and haml-lint 0.20.0 |
| @@ -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] | |||
There was a problem hiding this comment.
Hi @masayag - renaming :datastore to datastore_name would offer more clarity, as : datastore_id does
There was a problem hiding this comment.
@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.
There was a problem hiding this comment.
I will defer to @abellotti on using datastore_name in addition.
There was a problem hiding this comment.
@gmcculloug could you review ?
There was a problem hiding this comment.
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])
There was a problem hiding this comment.
@gmcculloug that's good idea. I'll update the PR to address it.
There was a problem hiding this comment.
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
There was a problem hiding this comment.
Fixed by #15245, closing the current one.
There was a problem hiding this comment.
@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.
Datastores on the same setup may have the same name. In order to add a
disk on a specific datastore, it is better to use the datastore id.
https://bugzilla.redhat.com/show_bug.cgi?id=1450952