Select datastore by its association with the provider #15245

masayag · 2017-05-29T13:18:10Z

Datastores on the same setup may have the same name.
However, there is an assumption that datastores names on the same
provider are unique. The selection of datastore by its name should be
narrow to the provider on which the vm is provisioned.

https://bugzilla.redhat.com/show_bug.cgi?id=1427498

masayag · 2017-05-29T13:19:38Z

@gmcculloug please review.

masayag · 2017-05-29T13:22:39Z

Bug 1427498 - VM provision via restapi fail, if the chosen data store name exist more than once in CFME

agrare · 2017-05-30T13:29:19Z

app/models/vm_or_template/operations/configuration.rb

@@ -74,7 +74,7 @@ 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])
+ datastore = ext_management_system.storages.find_by(:name => options[:datastore])


Can we use host.writable_storages here? It could be possible to pick a datastore that exists but the VM's host doesn't have access to.

@agrare hosts aren't available in this context. How would you suggest to do so ?

Also, the decision on which host to schedule the vm depends on auto-placement or not, in case of provision this is not in the context of this code.

@masayag You would need to access hosts from the ext_management_system and collect the writable_storages.

Maybe something like this:

storages = ext_management_system.hosts.collect(&:writable_storages).uniq.compact datastore = storages.detect { |storage| storage.name == options[:datastore] }

@gmcculloug fixed.

gmcculloug · 2017-06-06T20:21:53Z

app/models/vm_or_template/operations/configuration.rb

- raise _("Data Store does not exist, unable to add disk") unless datastore
+ datastore = ext_management_system.hosts.detect do |h|
+ h.writable_storages.find_by(:name => options[:datastore])
+ end.uniq.compact.first


This is not going to do what you want. The detect method will return a single Host object or nil and it looks like you are expecting a Storage object array (I think) since you are calling uniq.compact.first.

Which brings us to my second point: Test!

@gmcculloug ofcourse - i thought i squashed a local change to the commit which did exactly that. The fix should have been similar to https://github.com/ManageIQ/manageiq-providers-ovirt/pull/42/files#diff-17198062ae8d3e8788a4f5db1eb17ccdR59 - I'll add a test as well.

masayag

@gmcculloug the funny (well...not that funny) thing about it that I caught that mistake by the test I added to the other PR https://github.com/ManageIQ/manageiq-providers-ovirt/pull/42/files#diff-ea68074c0f05c6eccff224929cd39fceR188 which confirms your second comment about test necessity.

masayag · 2017-06-07T04:12:33Z

app/models/vm_or_template/operations/configuration.rb

- raise _("Data Store does not exist, unable to add disk") unless datastore
+ datastore = ext_management_system.hosts.detect do |h|
+ h.writable_storages.find_by(:name => options[:datastore])
+ end.uniq.compact.first


@gmcculloug ofcourse - i thought i squashed a local change to the commit which did exactly that. The fix should have been similar to https://github.com/ManageIQ/manageiq-providers-ovirt/pull/42/files#diff-17198062ae8d3e8788a4f5db1eb17ccdR59 - I'll add a test as well.

masayag · 2017-06-07T10:55:01Z

@gmcculloug added tests.

masayag · 2017-06-07T17:22:03Z

@agrare @gmcculloug I think this incorporates all of the comments.

verified the following scenarios:

add disk via vm reconfigure
add disk via an automation script (restapi)
vm provision via UI
vm provision via restapi where disks were specified as part of the request.

gmcculloug · 2017-06-07T21:24:11Z

app/models/vm_or_template/operations/configuration.rb

+ datastore = ext_management_system.hosts.collect do |h|
+ h.writable_storages.find_by(:name => options[:datastore])
+ end.uniq.compact.first
+ raise _("Data Store does not exist or cannot be accessed, unable to add disk") unless datastore


We should use Datastore to match usage throughout the rest of the product.
https://github.com/ManageIQ/manageiq/blob/master/locale/en/manageiq.po#L801

Only place it differs is with the previous error string from the method: https://github.com/ManageIQ/manageiq/blob/master/locale/en/manageiq.po#L7491

@gmcculloug fixed. I assume the translation itself isn't a concern of this PR ?

@mzazrivec Can you confirm there is nothing extra we need to do here?

Yep, wrt. translations nothing else is needed to be done here.

Datastores on the same setup may have the same name. However, there is an assumption that datastores names on the same provider are unique. The selection of datastore by its name should be narrow to the provider on which the vm is provisioned. https://bugzilla.redhat.com/show_bug.cgi?id=1427498

miq-bot · 2017-06-08T04:17:53Z

Checked commit masayag@e9c07df with ruby 2.2.6, rubocop 0.47.1, and haml-lint 0.20.0
2 files checked, 0 offenses detected
Everything looks fine. 🍰

Select datastore by its association with the provider (cherry picked from commit ec5d106) https://bugzilla.redhat.com/show_bug.cgi?id=1462774

simaishi · 2017-06-19T13:57:14Z

Fine backport details:

$ git log -1
commit 44b7dfe9631e4a72a3234da5ce858e4266ab72bb
Author: Greg McCullough <gmccullo@redhat.com>
Date:   Thu Jun 8 10:51:54 2017 -0400

    Merge pull request #15245 from masayag/find_datastore_within_provider
    
    Select datastore by its association with the provider
    (cherry picked from commit ec5d10654285cab2f2e5461d098330cee0333048)
    
    https://bugzilla.redhat.com/show_bug.cgi?id=1462774

masayag mentioned this pull request May 29, 2017

Allow to receive datastore by name or by id #15101

Closed

masayag mentioned this pull request May 29, 2017

Select datastore by its association with the provider ManageIQ/manageiq-providers-ovirt#42

Merged

agrare reviewed May 30, 2017

View reviewed changes

chessbyte assigned agrare May 31, 2017

masayag force-pushed the find_datastore_within_provider branch from 5cb561d to af7d916 Compare June 6, 2017 20:05

gmcculloug reviewed Jun 6, 2017

View reviewed changes

masayag force-pushed the find_datastore_within_provider branch from af7d916 to 620d610 Compare June 7, 2017 04:11

masayag commented Jun 7, 2017

View reviewed changes

masayag force-pushed the find_datastore_within_provider branch from 620d610 to 24b0e25 Compare June 7, 2017 10:53

gmcculloug reviewed Jun 7, 2017

View reviewed changes

masayag force-pushed the find_datastore_within_provider branch from 24b0e25 to e9c07df Compare June 8, 2017 04:15

gmcculloug merged commit ec5d106 into ManageIQ:master Jun 8, 2017

gmcculloug added this to the Sprint 63 Ending Jun 19, 2017 milestone Jun 8, 2017

gmcculloug added bug fine/yes labels Jun 8, 2017

masayag mentioned this pull request Jun 9, 2017

Align datastore naming ManageIQ/manageiq-providers-ovirt#48

Merged

simaishi pushed a commit that referenced this pull request Jun 19, 2017

Merge pull request #15245 from masayag/find_datastore_within_provider

44b7dfe

Select datastore by its association with the provider (cherry picked from commit ec5d106) https://bugzilla.redhat.com/show_bug.cgi?id=1462774

simaishi added fine/backported and removed fine/yes labels Jun 19, 2017

agrare added the lifecycle/provisioning label Jun 19, 2017

masayag deleted the find_datastore_within_provider branch June 27, 2018 20:30

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Select datastore by its association with the provider #15245

Select datastore by its association with the provider #15245

masayag commented May 29, 2017

masayag commented May 29, 2017

masayag commented May 29, 2017

agrare May 30, 2017

masayag Jun 5, 2017

gmcculloug Jun 5, 2017

masayag Jun 6, 2017

gmcculloug Jun 6, 2017

masayag Jun 7, 2017

masayag left a comment

masayag Jun 7, 2017

masayag commented Jun 7, 2017

masayag commented Jun 7, 2017

gmcculloug Jun 7, 2017 •

edited

Loading

masayag Jun 8, 2017

gmcculloug Jun 8, 2017

mzazrivec Jun 8, 2017

miq-bot commented Jun 8, 2017

simaishi commented Jun 19, 2017

Select datastore by its association with the provider #15245

Select datastore by its association with the provider #15245

Conversation

masayag commented May 29, 2017

masayag commented May 29, 2017

masayag commented May 29, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

masayag left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

masayag commented Jun 7, 2017

masayag commented Jun 7, 2017

gmcculloug Jun 7, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

miq-bot commented Jun 8, 2017

simaishi commented Jun 19, 2017

gmcculloug Jun 7, 2017 •

edited

Loading