Cloud Volume creation fixes #4908

martinpovolny · 2018-11-13T14:27:53Z

Fixes:

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

CloudVolume create: Need to turn off the spinner in both if/else branches.
Fix a crash when there is a CloudManager that does not have a CloudVolume. Such as Azure.
Fix the storage manager is not being loaded when creating a new Volume (and therefor emstype (and possibly other) parameters where missing) . Therefor the case when adding a new volume w/o selecting a storage manager did not work.

Part of the problems seem to be coming from 771e97f .

Populate volume types for EBS. Fixing:
https://bugzilla.redhat.com/show_bug.cgi?id=1644306

I have issues with understanding what are all the possible situations here and what is the expected behavior. Review and more testing is very welcome. Ping @mansam, @miha-plesko, @gberginc

…lume. Such as Azure.

martinpovolny · 2018-11-16T12:52:11Z

ping @AlexanderZagaynov

AlexanderZagaynov · 2018-11-16T15:48:40Z

Looks like it works now (with your PR).

miha-plesko · 2018-11-19T13:25:43Z

app/assets/javascripts/controllers/cloud_volume/cloud_volume_form_controller.js

@@ -270,6 +277,8 @@ ManageIQ.angular.app.controller('cloudVolumeFormController', ['miqService', 'API
    vm.supportsCinderVolumeTypes = data.supports_cinder_volume_types;
    if (vm.supportsCinderVolumeTypes) {
      vm.volumeTypes = data.parent_manager.cloud_volume_types;
+    } else if (vm.cloudVolumeModel.emstype === 'ManageIQ::Providers::Amazon::StorageManager::Ebs') {
+      loadEBSVolumeTypes();


Do we need to call it here as well? I think we call it inside setForm() already which is called regardless where we arrived to the page from.

The vm.storageManagerChanged is called from https://github.com/ManageIQ/manageiq-ui-classic/blob/master/app/views/cloud_volume/_common_new_edit.html.haml#L8. And from, there I need to populate the Volume types when the provider type is selected.

Oh I see 👍 You actually moved it to better place, we should probably delete it from setForm function eventually (not sure if in this PR that will be backported)

miha-plesko · 2018-11-19T13:30:59Z

For some reason I'm getting this error when arriving to the page via AWS volumes:

But it seems to be working when arriving from all volumes page.

martinpovolny · 2018-11-19T13:37:01Z

@miha-plesko : Please, provide more detailed steps for the error that you found. I'd love to reproduce what you got.

To be honest I am pretty lost in this form. It seems to be doing multiple things from multiple contexts and I don't know all the situations then need to be covered. I welcome any help here.

miha-plesko · 2018-11-19T13:43:30Z

Ahh, I know what it is. There is also variant c) to access the page - not through EBS manager, but through EC2 provider itself.

Comput > Cloud > Providers > "my aws" > Cloud Volumes

On this page it's also possible to click "Add new volume" but here storage_manager_id querystring parameter actually holds EC2 id instead EBS manager id BUUUM. We should somehow fix Rails to set correct ID here...

martinpovolny · 2018-11-19T13:48:16Z

Let's make a list of all the stuff that is expected to work with cloudVolumeFormController controller:

app/views/cloud_volume/backup_select.html.haml
app/views/cloud_volume/new.html.haml
app/views/cloud_volume/edit.html.haml
app/views/cloud_volume/snapshot_new.html.haml
app/views/cloud_volume/backup_new.html.haml
app/views/cloud_volume/detach.html.haml
app/views/cloud_volume/attach.html.haml

Creating a volume should work

from the screen that lists all the volumes (with the StorageManager drop-down working)
from a nested list screen under a particular StorageManager (with the StorageManager drop-down disabled).
from a nested list under a particular Cloud Provider (with the StorageManager drop-down disabled).

Providers:

It should work for Amazon EBS (with a hardcoded list of volume types).
It should work for OpenShift Cider (with a list of volume types coming from the parent manager of the StorageManager)

martinpovolny · 2018-11-19T13:50:01Z

Ahh, I know what it is. There is also variant c) to access the page - not through EBS manager, but through EC2 provider itself.

Oh how I love code that does a million things from a dozen contexts that is greatly documented and test-covered :-D

miha-plesko · 2018-11-19T13:56:04Z

I know the feeling @martinpovolny , was working on this controller in Gaprindashvilli blocker-only phase... @himdel said the same thing, "we need to split into 7 controllers". But are we really going to, assuming we're in Hammer blocker phase this time? 😂

miha-plesko · 2018-11-19T14:28:20Z

@martinpovolny it should fix the problem if we replace this line with

@storage_manager = begin
  if (manager = find_record_with_rbac(ExtManagementSystem, params[:storage_manager_id]))
    # Handle situation when CloudProvider ID is given instead StorageManager
    manager unless manager.respond_to?(:storage_managers)
    manager.storage_managers.detect { |m| m.supports_block_storage? }
  end
end

martinpovolny · 2018-11-19T18:18:50Z

I have added one more commit that does not fix things, but removes a creating of an empty CloudVolume instance. Should that cause a problem, please try testing w/o the last commit.

Thx!

miha-plesko · 2018-11-19T20:40:11Z

app/controllers/ems_common.rb

+    return nil unless manager
+    return manager.id unless manager.respond_to?(:storage_managers)
+    manager.storage_managers.detect { |m| m.supports_block_storage? }&.id
+  end


I like this. Maybe rename function into
def block_storage_manager_id
because at some point we'll probably need
def object_storage_manager_id
as well. Or even better, make it accept additional parameter to support both storage manager types:

def storage_manager_id(provider_id, criteria: :supports_block_storage?) ... manager.storage_managers.detect { |m| m.send(criteria) }&.id end

I'll test your latest changes tomorrow when I come to the office.

miha-plesko

@martinpovolny the last commit causes haml crash in _common_new_edit.haml if I go to:
Storage > BlockStorage > Volumes > "Add a new Cloud Volume"
We need to modify the HAML as well, please see the comment.

miha-plesko · 2018-11-20T08:04:06Z

app/controllers/cloud_volume_controller.rb

@@ -185,7 +185,6 @@ def new
    assert_privileges("cloud_volume_new")
    assert_privileges("cloud_tenant_show_list")

-    @volume = CloudVolume.new


Please also modify haml:

manageiq-ui-classic/app/views/cloud_volume/_common_new_edit.html.haml

Line 10 in 9ad557b

"disabled" => !@storage_manager.nil? || !@volume.id.nil?,

Here ^ it crashes with undefined method 'id' for nil:NilClass. We should probably test if @volume itself is nil, not try to access its id.

Right. I fixed it and added a simple spec.

…lay an empty form.

miha-plesko

LGTM. Tested and it works - NEW volume form gets properly populated no matter where I access it from 👍. I'd still rename the function into def block_storage_manager_id() (see comment from before), but as you wish.

BTW while testing I realized EDIT requires "storage manager show" permission (besides "volume edit") but that's not a problem to solve here. Not sure if it's really a bug even, because you shouldn't edit volume without accessing the manager options anyway, but I got the red bubble on UI so... :)

martinpovolny · 2018-11-21T08:42:27Z

@miha-plesko : Renaming the method as you suggested. However not adding the parameters as you also suggested. It's a good suggestion, but one that is not (yet) relevant. And a good practice is not to do things that are not needed. Not add parameters that are not needed.
I am also looking at adding some more tests.

Thank you for your cooperation and testing!

martinpovolny · 2018-11-22T12:53:01Z

@AlexanderZagaynov : good to merge?

AlexanderZagaynov · 2018-11-22T15:59:25Z

@martinpovolny yes, looks good, let's do that :)

miq-bot · 2018-11-22T16:22:56Z

Checked commits martinpovolny/manageiq-ui-classic@b5d15d8~...b8731b6 with ruby 2.3.3, rubocop 0.52.1, haml-lint 0.20.0, and yamllint 1.10.0
8 files checked, 1 offense detected

**

💣 💥 🔥 🚒 - Linter/Haml - Linter::Haml STDERR:

warning: parser/current is loading parser/ruby23, which recognizes
warning: 2.3.6-compliant syntax, but you are running 2.3.3.
warning: please see https://github.com/whitequark/parser#compatibility-with-ruby-mri.

martinpovolny · 2018-11-26T12:22:06Z

ping @mzazrivec, @himdel, @h-kataria ?

himdel · 2018-11-26T12:59:37Z

LGTM, code looks good and both BZs seem fixed 👍

Cloud Volume creation fixes (cherry picked from commit c7af6b6) Fixes https://bugzilla.redhat.com/show_bug.cgi?id=1644310

simaishi · 2018-11-26T22:32:27Z

Hammer backport details:

$ git log -1
commit ceb56d29494ad33e96b4b2d17b952dfc855e01eb
Author: Martin Hradil <himdel@seznam.cz>
Date:   Mon Nov 26 13:59:40 2018 +0100

    Merge pull request #4908 from martinpovolny/cloud_volume_buttons
    
    Cloud Volume creation fixes
    
    (cherry picked from commit c7af6b63e77ba40a36cf5661f35c44c5767dfc55)
    
    Fixes https://bugzilla.redhat.com/show_bug.cgi?id=1644310

simaishi · 2018-12-13T17:06:43Z

@martinpovolny @himdel can this be gaprindashvili/yes?

himdel · 2018-12-14T12:45:35Z

I think so, yes. #4549 is in gaprindashvili

himdel · 2018-12-14T12:48:34Z

.. But, looks like it would depend on backporting #4536 first (which fails to backport without #4345, but that one also has conflicts because it fixes some files which werent in gaprindashvili).

EDIT: and also #4605, which depends on #3927

himdel · 2018-12-14T13:04:00Z

This seeems to take all the relevant changes

git cherry-pick -x -m1 70b2475 # #3927
git cherry-pick -x -m1 760c7d6 # #4605

git cherry-pick -x -m1 46260de # #4345
rm app/assets/javascripts/controllers/ems_physical_infra_dashboard/server_health_pie_chart_controller.js app/assets/javascripts/components/cloud_subnet/cloud-subnet-form.js
git add app/assets/javascripts/controllers/ems_physical_infra_dashboard/server_health_pie_chart_controller.js app/assets/javascripts/components/cloud_subnet/cloud-subnet-form.js
git cherry-pick --continue

git cherry-pick -x -m1 7c4afa8 # #4536

git cherry-pick -x -m1 c7af6b6 # #4908
sed -i -e 202d -e 169,192d app/controllers/ems_common.rb
git add app/controllers/ems_common.rb
git cherry-pick --continue

not sure how to proceed :)

simaishi · 2018-12-14T13:07:50Z

@himdel Oh my...... thanks for finding out all those dependencies and extra steps!!! Let me check with mgmt team with this info, this might be more than what we'd like in G branch. Thanks again 😄

martinpovolny · 2018-12-19T09:07:06Z

Thank you for taking care of this, @himdel !

@simaishi : might be better to create a separate PR for Gaprindashvili (and test it!) following the steps @himdel provided rather than just trying to backport this.

simaishi · 2018-12-19T14:45:34Z

@martinpovolny Please hold off working on PRs for Gaprindashvili. We might not take this into G release.

martinpovolny added 2 commits November 13, 2018 15:24

Fix a crash when there is a CloudManager that does not have a CloudVo…

b5d15d8

…lume. Such as Azure.

CloudVolume create: Need to turn off the spinner in this branch too.

e07dc00

martinpovolny requested a review from alexander-demicev November 13, 2018 14:28

martinpovolny added bug compute/cloud storage hammer/yes labels Nov 13, 2018

martinpovolny changed the title ~~Fixes:~~ Cloud Volume creation fixes Nov 13, 2018

CloudVolumes: use "any?", test rather than rescue.

75e0157

martinpovolny force-pushed the cloud_volume_buttons branch from 4cdf368 to 75e0157 Compare November 14, 2018 11:53

martinpovolny changed the title ~~Cloud Volume creation fixes~~ [WIP] Cloud Volume creation fixes Nov 16, 2018

miq-bot added the wip label Nov 16, 2018

martinpovolny requested review from andyvesel and removed request for alexander-demicev and andyvesel November 16, 2018 12:49

martinpovolny added 2 commits November 19, 2018 13:32

Populate storageManager information when adding a new volume.

3623f17

Populate volume types for EBS.

ea52035

martinpovolny force-pushed the cloud_volume_buttons branch from 54c339c to ea52035 Compare November 19, 2018 12:33

martinpovolny changed the title ~~[WIP] Cloud Volume creation fixes~~ Cloud Volume creation fixes Nov 19, 2018

miq-bot removed the wip label Nov 19, 2018

miha-plesko reviewed Nov 19, 2018

View reviewed changes

miha-plesko suggested changes Nov 20, 2018

View reviewed changes

martinpovolny added 2 commits November 20, 2018 17:36

Creating a cloud volume: Do not create a CloudVolume instance to disp…

0c73939

…lay an empty form.

CloudVolume: a simple spec for new volume.

14cdc9e

martinpovolny force-pushed the cloud_volume_buttons branch from 4a3c9a3 to 14cdc9e Compare November 20, 2018 16:36

miha-plesko approved these changes Nov 21, 2018

View reviewed changes

martinpovolny added 3 commits November 21, 2018 11:14

CloudVolumes: rename a method.

c86a095

CloudVolumeController: style fixes.

bd2d877

CloudVolume: style fixes.

b8731b6

himdel merged commit c7af6b6 into ManageIQ:master Nov 26, 2018

himdel self-assigned this Nov 26, 2018

himdel added this to the Sprint 100 Ending Dec 3, 2018 milestone Nov 26, 2018

simaishi pushed a commit that referenced this pull request Nov 26, 2018

Merge pull request #4908 from martinpovolny/cloud_volume_buttons

ceb56d2

Cloud Volume creation fixes (cherry picked from commit c7af6b6) Fixes https://bugzilla.redhat.com/show_bug.cgi?id=1644310

simaishi added hammer/backported and removed hammer/yes labels Nov 26, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Cloud Volume creation fixes #4908

Cloud Volume creation fixes #4908

martinpovolny commented Nov 13, 2018 •

edited

Loading

martinpovolny commented Nov 16, 2018

AlexanderZagaynov commented Nov 16, 2018 •

edited

Loading

miha-plesko Nov 19, 2018

martinpovolny Nov 19, 2018

miha-plesko Nov 19, 2018

miha-plesko commented Nov 19, 2018

martinpovolny commented Nov 19, 2018

miha-plesko commented Nov 19, 2018

martinpovolny commented Nov 19, 2018 •

edited

Loading

martinpovolny commented Nov 19, 2018

miha-plesko commented Nov 19, 2018

miha-plesko commented Nov 19, 2018

martinpovolny commented Nov 19, 2018

miha-plesko Nov 19, 2018

miha-plesko left a comment

miha-plesko Nov 20, 2018

martinpovolny Nov 20, 2018

miha-plesko left a comment

martinpovolny commented Nov 21, 2018

martinpovolny commented Nov 22, 2018

AlexanderZagaynov commented Nov 22, 2018

miq-bot commented Nov 22, 2018

martinpovolny commented Nov 26, 2018

himdel commented Nov 26, 2018

simaishi commented Nov 26, 2018

simaishi commented Dec 13, 2018

himdel commented Dec 14, 2018

himdel commented Dec 14, 2018 •

edited

Loading

himdel commented Dec 14, 2018

simaishi commented Dec 14, 2018

martinpovolny commented Dec 19, 2018

simaishi commented Dec 19, 2018

Cloud Volume creation fixes #4908

Cloud Volume creation fixes #4908

Conversation

martinpovolny commented Nov 13, 2018 • edited Loading

martinpovolny commented Nov 16, 2018

AlexanderZagaynov commented Nov 16, 2018 • edited Loading

miha-plesko Nov 19, 2018

Choose a reason for hiding this comment

martinpovolny Nov 19, 2018

Choose a reason for hiding this comment

miha-plesko Nov 19, 2018

Choose a reason for hiding this comment

miha-plesko commented Nov 19, 2018

martinpovolny commented Nov 19, 2018

miha-plesko commented Nov 19, 2018

martinpovolny commented Nov 19, 2018 • edited Loading

martinpovolny commented Nov 19, 2018

miha-plesko commented Nov 19, 2018

miha-plesko commented Nov 19, 2018

martinpovolny commented Nov 19, 2018

miha-plesko Nov 19, 2018

Choose a reason for hiding this comment

miha-plesko left a comment

Choose a reason for hiding this comment

miha-plesko Nov 20, 2018

Choose a reason for hiding this comment

martinpovolny Nov 20, 2018

Choose a reason for hiding this comment

miha-plesko left a comment

Choose a reason for hiding this comment

martinpovolny commented Nov 21, 2018

martinpovolny commented Nov 22, 2018

AlexanderZagaynov commented Nov 22, 2018

miq-bot commented Nov 22, 2018

martinpovolny commented Nov 26, 2018

himdel commented Nov 26, 2018

simaishi commented Nov 26, 2018

simaishi commented Dec 13, 2018

himdel commented Dec 14, 2018

himdel commented Dec 14, 2018 • edited Loading

himdel commented Dec 14, 2018

simaishi commented Dec 14, 2018

martinpovolny commented Dec 19, 2018

simaishi commented Dec 19, 2018

martinpovolny commented Nov 13, 2018 •

edited

Loading

AlexanderZagaynov commented Nov 16, 2018 •

edited

Loading

martinpovolny commented Nov 19, 2018 •

edited

Loading

himdel commented Dec 14, 2018 •

edited

Loading