Support base snapshot for EBS cloud volume provisioning #1324

sasoc · 2017-05-10T08:59:22Z

Add option to create new cloud volume from snapshot for AWS EBS Storage Maneger.

gberginc

Please, review the comments. Also attach a screenshot showing creation of Amazon cloud volume (attach it into the PR body).

gberginc · 2017-05-10T09:04:03Z

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

@@ -113,7 +113,7 @@ ManageIQ.angular.app.controller('cloudVolumeFormController', ['miqService', 'API

  vm.storageManagerChanged = function(id) {
    miqService.sparkleOn();
-    API.get('/api/providers/' + id + '?attributes=type,parent_manager.availability_zones,parent_manager.cloud_tenants')
+    API.get('/api/providers/' + id + '?attributes=type,parent_manager.availability_zones,parent_manager.cloud_volume_snapshots,parent_manager.cloud_tenants')


Add the change to the end of the API call.

gberginc · 2017-05-10T09:04:58Z

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

@@ -213,6 +213,7 @@ ManageIQ.angular.app.controller('cloudVolumeFormController', ['miqService', 'API
    // model attribute with AWS.
    vm.cloudVolumeModel.aws_volume_type = data.volume_type;
    vm.cloudVolumeModel.aws_availability_zone_id = data.availability_zone.ems_ref;
+    //vm.cloudVolumeModel.aws_cloud_volume_snapshot_id = data.cloud_volume_snapshot_id;


The snapshot id should be set here so that, when editing the volume, the snapshot will be displayed. The ID should be returned by the current API call.

gberginc · 2017-05-10T09:07:20Z

app/views/cloud_volume/_common_new_edit.html.haml

+            "data-live-search" => "true",
+            "pf-select"   => true}
+      %option{"value" => "", "enabled" => ""}
+        = "#{_('No snapshot')}"


You can change this to = _('No snapshot') since you are not rendering any other chars, like < and > in other cases.

gberginc · 2017-05-10T09:07:51Z

app/views/cloud_volume/_common_new_edit.html.haml

+            "ng-options"  => "vs.ems_ref as vs.name for vs in vm.cloudVolumeSnapshotChoices",
+            "ng-disabled" => "!vm.newRecord",
+            :checkchange  => true,
+            "data-live-search" => "true",


Align all => to the widest key name.

gberginc · 2017-05-10T09:09:33Z

app/views/cloud_volume/_common_new_edit.html.haml

@@ -49,6 +49,20 @@
    %span.help-block{"ng-show" => "angularForm.aws_availability_zone_id.$error.required"}
      = _("Required")

+.form-group{"ng-if"    => "vm.cloudVolumeModel.emstype == 'ManageIQ::Providers::Amazon::StorageManager::Ebs'"}


Move this just in front of "Encryption" at the bottom.

gberginc · 2017-05-10T09:11:38Z

@miq-bot add_label storage,enhancement

gberginc · 2017-05-11T11:28:33Z

app/controllers/cloud_volume_controller.rb

@@ -693,6 +693,7 @@ def form_params_create
      # Only set IOPS if io1 (provisioned IOPS) and IOPS available
      options[:iops] = params[:aws_iops] if options[:volume_type] == 'io1' && params[:aws_iops]
      options[:availability_zone] = params[:aws_availability_zone_id] if params[:aws_availability_zone_id]
+      options[:snapshot_id] = params[:aws_cloud_volume_snapshot_id] if params[:aws_cloud_volume_snapshot_id]


Now that we are using base_snapshot properly from the API, I suggest you rename this to aws_base_snapshot

gberginc · 2017-05-22T09:34:54Z

app/views/cloud_volume/_common_new_edit.html.haml

@@ -110,6 +110,20 @@
    %span.help-block{"ng-show" => "vm.cloudVolumeModel.aws_volume_type == 'io1' && angularForm.aws_iops.$error.required"}
      = _("Required")

+.form-group{"ng-if"    => "vm.cloudVolumeModel.emstype == 'ManageIQ::Providers::Amazon::StorageManager::Ebs'"}


Remove unnecessary spaces between "ng-if" and =>.

gberginc · 2017-05-22T09:35:19Z

app/views/cloud_volume/_common_new_edit.html.haml

+            "ng-options"       => "vs.ems_ref as vs.name for vs in vm.baseSnapshotChoices",
+            "ng-disabled"      => "!vm.newRecord",
+            :checkchange       => true,
+            "data-live-search" => "true",


Does it work with true (instead of "true")?

It doesn't work with true instead of "true".

gberginc · 2017-05-22T10:04:02Z

app/views/cloud_volume/_common_new_edit.html.haml

@@ -112,6 +112,20 @@

 .form-group{"ng-if" => "vm.cloudVolumeModel.emstype == 'ManageIQ::Providers::Amazon::StorageManager::Ebs'"}
  %label.col-md-2.control-label
+    = _('Cloud Snapshot')


Sorry, but I've missed this one before: please replace this with Base Snapshot.

gberginc · 2017-05-22T10:31:02Z

Thanks @sasoc!

@bronaghs LGTM, but a review from you or the UI team (Aparna?) would be appreciated.

@miq-bot remove_label wip

bronaghs · 2017-05-25T17:47:29Z

@sasoc -
I see this screen prompts the user for an IOPs and an Encrypted value. There are currently no columns for these values in the DB, that depends on ManageIQ/manageiq#14704 being merged. Am I missing something?

bronaghs · 2017-05-25T17:47:55Z

@miq-bot assign @AparnaKarve

gberginc · 2017-05-25T18:05:28Z

@bronaghs the UI shows these two properties because it allows users to specify them when creating new volumes. The referenced PR from the core will allow us to show these properties while editing cloud volume.

Both IOPS and Encrypted fields are already merged in the UI repo. @sasoc just added the base snapshot field.

AparnaKarve · 2017-05-25T19:53:07Z

I got this error because I selected Yes for Encryption.

Just wanted to know if it is possible to determine beforehand if Encryption=true is not supported for a given snapshot?

gberginc · 2017-05-25T19:59:41Z

No, unfortunatelly not until we update the models :(. I wanted to make this change already, but since the PR on the master has not been accepted, I waited as I wasn't sure if I am doing it correctly.

AparnaKarve · 2017-05-25T20:03:42Z

@gberginc Regarding the CC error Cyclomatic complexity for form_params_create is too high. [12/11] , I was wondering if we could break down the form_params_create method into smaller sections? Something to this effect?

when "ManageIQ::Providers::StorageManager::CinderManager"
  options = cinder_manager_options
when "ManageIQ::Providers::Amazon::StorageManager::Ebs"
  options = aws_ebs_options

gberginc · 2017-05-25T20:41:48Z

@AparnaKarve this would make sense. We'll check what would be the best way because some options are common. Prehaps we extend the options with provider specific ones not to duplicate the common logic.

sasoc · 2017-05-31T12:16:57Z

@AparnaKarve I've broken form_params_create into smaller sections as you suggested. Can you please look into it?

Add option to create new cloud volume from snapshot for AWS EBS Storage Maneger.

miq-bot · 2017-05-31T15:24:01Z

Checked commit sasoc@07e3b5e with ruby 2.2.6, rubocop 0.47.1, and haml-lint 0.20.0
1 file checked, 0 offenses detected
Everything looks fine. 🍰

AparnaKarve · 2017-05-31T18:53:02Z

@sasoc That looks good, and it has also addressed the cyclomatic complexity CC error. Thanks.

@gberginc About the error in #1324 (comment), do you think we can address it once the model changes are in?

gberginc · 2017-05-31T19:18:19Z

@AparnaKarve that error message will require model changes. We still have just two PRs for the changes in the cloud volume. @sasoc fixed the tests, but unfortunately, they still haven't been merged.

Would you propose that we simply add the PRs for the snapshots as well?

AparnaKarve · 2017-05-31T20:09:28Z

@gberginc Can you post the PR links here, the one with the model changes?

gberginc · 2017-05-31T20:21:55Z

@AparnaKarve

PR in core: ManageIQ/manageiq#14704
PR in Amazon provider: ManageIQ/manageiq-providers-amazon#211

Both are just for the cloud volumes, but the idea for snapshots will be the same: update the model and extend the inventory.

AparnaKarve · 2017-05-31T20:41:05Z

@gberginc I think we should get this in since the Cloud Volume creation from a snapshot does work.

Let's revisit #1324 (comment) once the necessary model PRs are merged in.

AparnaKarve · 2017-05-31T22:47:41Z

@dclarizio I think we can go ahead and merge this.

If we address the issue described in #1324 (comment), it will improve the user experience, and is not a bug as such.
Also, it would require some model changes before we can consider a fix for it.

miq-bot added the wip label May 10, 2017

gberginc suggested changes May 10, 2017

View reviewed changes

miq-bot added enhancement storage labels May 10, 2017

sasoc force-pushed the add_snapshot_to_new_cloud_volume branch from 03e3552 to 88f1dd3 Compare May 11, 2017 09:41

gberginc suggested changes May 11, 2017

View reviewed changes

sasoc force-pushed the add_snapshot_to_new_cloud_volume branch from 88f1dd3 to 18face2 Compare May 17, 2017 09:32

chessbyte assigned bronaghs May 19, 2017

sasoc force-pushed the add_snapshot_to_new_cloud_volume branch from 18face2 to 2b1d1e0 Compare May 22, 2017 09:22

gberginc suggested changes May 22, 2017

View reviewed changes

sasoc force-pushed the add_snapshot_to_new_cloud_volume branch from 2b1d1e0 to fe5aa2b Compare May 22, 2017 09:50

gberginc reviewed May 22, 2017

View reviewed changes

sasoc force-pushed the add_snapshot_to_new_cloud_volume branch from fe5aa2b to 48f5015 Compare May 22, 2017 10:07

miq-bot changed the title ~~[WIP] Support base snapshot for EBS cloud volume provisioning~~ Support base snapshot for EBS cloud volume provisioning May 22, 2017

miq-bot removed the wip label May 22, 2017

miq-bot assigned AparnaKarve and unassigned bronaghs May 25, 2017

dclarizio added the fine/no label May 25, 2017

sasoc force-pushed the add_snapshot_to_new_cloud_volume branch 3 times, most recently from f09b950 to 7d576c0 Compare May 31, 2017 12:00

Support base snapshot for EBS cloud volume provisioning

07e3b5e

Add option to create new cloud volume from snapshot for AWS EBS Storage Maneger.

sasoc force-pushed the add_snapshot_to_new_cloud_volume branch from 7d576c0 to 07e3b5e Compare May 31, 2017 15:19

gberginc mentioned this pull request Jun 1, 2017

Add IOPS and encrypted columns to CloudVolume ManageIQ/manageiq#14704

Merged

dclarizio merged commit 4f15973 into ManageIQ:master Jun 1, 2017

dclarizio added this to the Sprint 62 Ending Jun 5, 2017 milestone Jun 1, 2017

sasoc mentioned this pull request Jul 5, 2017

Regulate encryption when creating new volume from snapshot #1651

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support base snapshot for EBS cloud volume provisioning #1324

Support base snapshot for EBS cloud volume provisioning #1324

sasoc commented May 10, 2017 •

edited

Loading

gberginc left a comment

gberginc May 10, 2017

gberginc May 10, 2017

gberginc May 10, 2017

gberginc May 10, 2017

gberginc May 10, 2017

gberginc commented May 10, 2017

gberginc May 11, 2017

gberginc May 22, 2017

gberginc May 22, 2017

sasoc May 22, 2017

gberginc May 22, 2017

gberginc May 22, 2017

sasoc May 22, 2017

gberginc commented May 22, 2017

bronaghs commented May 25, 2017

bronaghs commented May 25, 2017

gberginc commented May 25, 2017

AparnaKarve commented May 25, 2017

gberginc commented May 25, 2017

AparnaKarve commented May 25, 2017

gberginc commented May 25, 2017

sasoc commented May 31, 2017

miq-bot commented May 31, 2017

AparnaKarve commented May 31, 2017

gberginc commented May 31, 2017

AparnaKarve commented May 31, 2017

gberginc commented May 31, 2017

AparnaKarve commented May 31, 2017

AparnaKarve commented May 31, 2017

Support base snapshot for EBS cloud volume provisioning #1324

Support base snapshot for EBS cloud volume provisioning #1324

Conversation

sasoc commented May 10, 2017 • edited Loading

gberginc left a 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

Choose a reason for hiding this comment

gberginc commented May 10, 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

Choose a reason for hiding this comment

gberginc commented May 22, 2017

bronaghs commented May 25, 2017

bronaghs commented May 25, 2017

gberginc commented May 25, 2017

AparnaKarve commented May 25, 2017

gberginc commented May 25, 2017

AparnaKarve commented May 25, 2017

gberginc commented May 25, 2017

sasoc commented May 31, 2017

miq-bot commented May 31, 2017

AparnaKarve commented May 31, 2017

gberginc commented May 31, 2017

AparnaKarve commented May 31, 2017

gberginc commented May 31, 2017

AparnaKarve commented May 31, 2017

AparnaKarve commented May 31, 2017

sasoc commented May 10, 2017 •

edited

Loading