Enable dynamic cloud volume modifications #177

gberginc · 2017-03-13T22:18:54Z

Recently Amazon introduced a capability to modify existing cloud volumes
on the fly. Depending on the volume type size, volume type and IOPS can
be modified without detaching the volume first.

This patch enhances the existing update_volume method that previously
allowed changing only the name of the volume by using the new method
#modify_volume from the aws-sdk gem. The patch also adds a set of
tests verifying different possible options passed to the
update_volume.

Depends on #176 (requires newer aws-sdk gem).

@miq-bot add_label enhancement

durandom · 2017-03-15T14:17:15Z

manageiq-providers-amazon.gemspec

@@ -13,7 +13,7 @@ Gem::Specification.new do |s|

  s.files = Dir["{app,config.lib}/**/*"]

-  s.add_dependency("aws-sdk", ["~>2.6.14"])
+  s.add_dependency("aws-sdk", ["~>2.7.8"])


can you put this in a separate PR - for backporting reasons...

oh, it's already in #176 👍

miq-bot · 2017-03-15T16:37:32Z

This pull request is not mergeable. Please rebase and repush.

gberginc · 2017-03-15T16:48:57Z

Since #176 was just merged, I rebased it and removed the gem version bump from this one.

durandom · 2017-03-16T08:36:45Z

spec/models/manageiq/providers/amazon/storage_manager/ebs/cloud_volume_spec.rb

+        it "is allowed when :name is provided" do
+          # Name is set via AWS tags so stub the create_tags.
+          expect_any_instance_of(Aws::EC2::Volume).to receive(:create_tags).with(
+            :tags => [{:key => "Name", :value => "new_name"}]


I think you can also stub this. add the api calls to with_aws_stubbed below

Possible, but only to some degree. I wanted to ensure that specific arguments are used as well as check whether the call is made or not (if name is given, it must be called, if it's not given, it should not be called).

What I could do is ignore input parameters to create_tags and then use an exception in the stub to ensure it doesn't get called. I'll have a look.

durandom · 2017-03-16T08:38:00Z

app/models/manageiq/providers/amazon/storage_manager/ebs/cloud_volume.rb

-      with_provider_object do |volume|
-        volume.create_tags(:tags => [{:key => "Name", :value => options[:name]}])
-      end
+    res = nil


does any caller really care about the return value?

I am not completely sure. Perhaps I just misunderstood what's needed.

durandom · 2017-03-16T08:38:59Z

app/models/manageiq/providers/amazon/storage_manager/ebs/cloud_volume.rb

+    modify_opts = {}
+    if volume_type != 'standard'
+      modify_opts[:volume_type] = options[:volume_type] if options[:volume_type] && options[:volume_type] != 'standard'
+      modify_opts[:size]        = options[:size] if options[:size] && options[:size] != (size / 1_073_741_824)


Is this something like 1.gigabyte ?

true, alternative check would be ... && options[:size].gigabyte == size. Database stores the size in bytes, but the :size in options must be in gigabytes.

miq-bot · 2017-03-16T20:37:58Z

Checked commit gberginc@d586e19 with ruby 2.2.6, rubocop 0.47.1, and haml-lint 0.20.0
3 files checked, 0 offenses detected
Everything looks good. ⭐

gberginc · 2017-03-16T20:38:11Z

@durandom I've updated the change:

use stubs for all tests: in these stubs I have explicitly set exceptions for the calls for which I'd like to ensure they are not called (for example, when :name is not given, it should not call create_tags, so I set the stubbed response to failed to ensure exception is thrown in case the call is made)
use gigabyte instead of that ugly number
ignore return value of update_volume - it really doesn't seem like it's used somewhere

durandom

@gberginc awesome second iteration 👏

besides the options question great

durandom · 2017-03-17T14:16:11Z

app/models/manageiq/providers/amazon/storage_manager/ebs/cloud_volume.rb

+    modify_opts = {}
+    if volume_type != 'standard'
+      modify_opts[:volume_type] = options[:volume_type] if options[:volume_type] && options[:volume_type] != 'standard'
+      modify_opts[:size]        = options[:size] if options[:size] && options[:size].gigabytes != size


where does options[:size] come from? Maybe do Integer(options[:size]) ?

@durandom Thanks for spotting this. So far value of options[:size] came from the UI where I tested only with valid values, however it could also come from API or automate which means that it could in fact be invalid. I added two more tests to ensure that valid and invalid strings work properly with the updated function.

Recently Amazon introduced a capability to modify existing cloud volumes on the fly. Depending on the volume type size, volume type and IOPS can be modified without detaching the volume first. This patch enhances the existing `update_volume` method that previously allowed changing only the name of the volume by using the new method `#modify_volume` from the aws-sdk gem. The patch also adds a set of tests verifying different possible options passed to the `update_volume`.

Ladas · 2017-03-20T08:48:36Z

looks great, very nice specs 👍

miq-bot added the enhancement label Mar 13, 2017

gberginc force-pushed the enable_modify_volume branch 2 times, most recently from c6e19e7 to c79c93d Compare March 14, 2017 00:18

gberginc mentioned this pull request Mar 14, 2017

Allow advanced modifications of Amazon cloud volumes ManageIQ/manageiq-ui-classic#676

Merged

gberginc force-pushed the enable_modify_volume branch from c79c93d to 2611e7b Compare March 14, 2017 09:48

blomquisg requested a review from durandom March 15, 2017 14:14

durandom reviewed Mar 15, 2017

View reviewed changes

miq-bot added the unmergeable label Mar 15, 2017

gberginc force-pushed the enable_modify_volume branch from 2611e7b to 88cf776 Compare March 15, 2017 16:48

miq-bot removed the unmergeable label Mar 15, 2017

durandom suggested changes Mar 16, 2017

View reviewed changes

gberginc force-pushed the enable_modify_volume branch 2 times, most recently from 28aa126 to d586e19 Compare March 16, 2017 20:33

durandom approved these changes Mar 17, 2017

View reviewed changes

gberginc force-pushed the enable_modify_volume branch from d586e19 to fe5147c Compare March 20, 2017 06:11

Ladas added this to the Sprint 57 Ending Mar 27, 2017 milestone Mar 20, 2017

Ladas self-assigned this Mar 20, 2017

Ladas merged commit 11b9393 into ManageIQ:master Mar 20, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Enable dynamic cloud volume modifications #177

Enable dynamic cloud volume modifications #177

gberginc commented Mar 13, 2017

durandom Mar 15, 2017

durandom Mar 15, 2017

miq-bot commented Mar 15, 2017

gberginc commented Mar 15, 2017

durandom Mar 16, 2017

gberginc Mar 16, 2017

durandom Mar 16, 2017

gberginc Mar 16, 2017

durandom Mar 16, 2017

gberginc Mar 16, 2017

miq-bot commented Mar 16, 2017

gberginc commented Mar 16, 2017

durandom left a comment

durandom Mar 17, 2017

gberginc Mar 20, 2017

Ladas commented Mar 20, 2017

Enable dynamic cloud volume modifications #177

Enable dynamic cloud volume modifications #177

Conversation

gberginc commented Mar 13, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

miq-bot commented Mar 15, 2017

gberginc commented Mar 15, 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

miq-bot commented Mar 16, 2017

gberginc commented Mar 16, 2017

durandom left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Ladas commented Mar 20, 2017