Use approval workflow for instance resize operation #16044

sseago · 2017-09-26T16:35:11Z

Depends on UI PR ManageIQ/manageiq-ui-classic#2241

gmcculloug · 2017-09-28T17:02:00Z

@sseago Is there a UI you can add to the description?

Is this reconfigure for Cloud? If so we may want to consider how to use the existing workflow instead of creating a new, and likely confusing, duplication of the reconfigure logic.

sseago · 2017-09-28T17:13:55Z

@gmcculloug There's no new UI here. This is the openstack "resize" operation which is exposed in the UI for openstack-related VMs as "reconfigure". The functionality already exists as "resize" in the models -- it's distinct from reconfigure since the inputs and validation (and really everything) is different. Basically for OpenStack instances, resize takes a new flavor and the underlying OpenStack API call accepts an instance ID and a new flavor ID. The difference is instead of the UI action calling the model itself, we create a VMResizeRequest object which in turn calls resize on the model. Trying to rewrite the existing reconfigure (both model and request workflow) to use the same API would be even more complex and confusing and, in any case, seems as if it would be out of scope for the task of "do what we do now but with approval workflow" since it would be a fairly substantial rework of "what we do now" for both resize and reconfigure.

The development task here is basically "use approval rather than making a direct model call", and the existing VMReconfigureRequest has nothing in it that's relevant to the resize operation, since in the models resize and reconfigure are completely different operations.

tzumainn · 2017-10-04T12:57:34Z

@gmcculloug Hi! Can you let us know if what scott says makes sense? It made sense to me when he and I first looked at the code, but I don't know if I'm missing something obvious.

gmcculloug · 2017-10-04T14:33:37Z

@sseago Where is the "do what we do now but with approval workflow" comment coming from? Is there a reference to an RFE or enhancement request you can link here?

Today is is support approval workflow, but what about quota which is part of the provisioning/reconfigure workflows? Here we are allowing a user to consume more quota without any checking.

I totally agree that it will be a substantial rework of the existing reconfigure logic but this does not feel like we are not addressing the full issue.

tzumainn · 2017-10-04T14:45:17Z

@gmcculloug I can provide some background: we exposed a resize operation for OpenStack instances a while ago, and PM later asked why there wasn't an approval workflow for it. This is an effort to remediate that.

If there's a better way to fix this, please let us know!

gmcculloug · 2017-10-04T15:03:51Z

First, it would help if we could determine if they think quota is important here.

tzumainn · 2017-10-04T15:04:54Z

Fair enough! Are there other issues we should talk with PM about? Just want to make sure everything is covered in one swoop!

gmcculloug · 2017-10-04T16:18:02Z

I guess the only other thing I would ask is if the interaction with automate, outside of approval, is important. Right now the task would not be processed through automate, which is probably ok.

gmcculloug · 2017-10-10T17:03:59Z

I think this helps answer my question about quota: https://bugzilla.redhat.com/show_bug.cgi?id=1500152

Quota can be implemented with the current setup but I still think we should look into refactoring/sub-classing the current VmReconfigure models to handle this instead of adding ambiguous VmResize ones just for cloud.

cc @tinaafitz

tzumainn · 2017-10-10T17:09:47Z

@gmcculloug haha, yeah, it looks like something we should hopefully be able to fix here. Any guidance would be greatly appreciated if there's another direction you think we should go!

gmcculloug

@sseago Added my suggestions on changing to use a class more inline with the existing VmReconfigure request/task. I think we should avoid using Resize in most places and I pointed many of them out, but not all.

There is also outstanding work in the automate model to enable auto-approval logic which is how I would suggest shipping this feature out-of-the-box.
By doing that users will not have to manually approve request, but allows for an admin to enable/disable auto-approval logic. We can discuss when this is a little further along.

gmcculloug · 2017-10-11T18:29:48Z

app/models/vm_resize_request.rb

@@ -0,0 +1,26 @@
+class VmResizeRequest < MiqRequest


My suggestion is to use "VmCloudReconfigure" for this work. So this class name would be VmCloudReconfigureRequest and the task would be VmCloudReconfigureTask.

gmcculloug · 2017-10-11T18:30:54Z

app/models/vm_resize_request.rb

+class VmResizeRequest < MiqRequest
+  TASK_DESCRIPTION  = 'VM Resize'.freeze
+  SOURCE_CLASS_NAME = 'Vm'.freeze
+  ACTIVE_STATES     = %w(resized) + base_class::ACTIVE_STATES


ACTIVE_STATES should be reconfigured

gmcculloug · 2017-10-11T18:34:56Z

app/models/manageiq/providers/cloud_manager/vm.rb

@@ -107,7 +107,7 @@ def cpu_percent_available?
    true
  end

-  def resize_queue(userid, new_flavor)
+  def resize_queue(userid, new_flavor_id)


The new request_task is being queued to run with the proper role and zone so this method is no longer needed by the reconfigure request/task logic. The task should call the resize method directly.

I am not sure if there are any other callers of this method, if not it can be removed.

This can be removed. I had already wondered whether I could remove this, but I was holding off until I got feedback on whether that was the right thing to do. I'll pull this method out along with the class renaming for the next iteration.

gmcculloug · 2017-10-11T18:39:48Z

app/models/vm_resize_request.rb

+  validate  :must_have_user
+
+  def self.make_request(request, values, requester, auto_approve = false)
+    values[:request_type] = :vm_resize


:request_type should be vm_cloud_reconfigure. This will be used in automate when processing the request events.

gmcculloug · 2017-10-11T18:41:26Z

app/models/vm_resize_task.rb

@@ -0,0 +1,45 @@
+class VmResizeTask < MiqRequestTask


Class name changes to VmCloudReconfigureTask

gmcculloug · 2017-10-11T18:43:00Z

app/models/vm_resize_task.rb

+
+  def do_request
+    _log.info("Resizing VM #{vm.id}:#{vm.name} to flavor #{options[:flavor_name]}")
+    vm.resize_queue(miq_request.userid, options[:flavor_id])


As mentioned before, the task will be running in the zone and role so you do not need to queue here.

gmcculloug · 2017-10-11T18:43:19Z

app/models/vm_resize_task.rb

+    vm.resize_queue(miq_request.userid, options[:flavor_id])
+
+    if AUTOMATE_DRIVES
+      update_and_notify_parent(:state => 'resized', :message => "Finished #{request_class::TASK_DESCRIPTION}")


:state => 'reconfigured'

sseago · 2017-10-11T19:15:06Z

@gmcculloug I think I must be missing the big picture here. You're proposing that we use the existing Reconfigure-related classes instead of the existing Resize-related ones (and any additional ones needed to complete the functionality). However, the actual underlying actions aren't at all the same. They do different things. They act on different elements and associations. Quota checking won't be the same either. Unless I'm missing something, trying to shoehorn resize into reconfigure will be significantly more complex and more confusing, since what happens in the action itself isn't the same. Reconfigure actually modifies explicit size-related attributes like RAM, CPUs, disk, etc. -- resize does none of that. Maybe some examples of how you imagine the functionality being merged might help me understand what's being proposed here.

gmcculloug · 2017-10-11T20:40:26Z

Sorry, taking a step back from what I said earlier. I am ok with deriving the new request/task models from MiqRequest/MiqRequestTask models, I am not going to push to sub-class them at the moment.

In the long run this still might be something we refactor as most of the request/task logic you include here is directly copied from the VmReconfigureRequest and VmReconfigureTask. Thinking it would end up looking something like this:

                MiqRequest
                    |
               VmReconfigure
               /           \
VmInfraReconfigure       VmCloudReconfigure

(Where VmInfraReconfigure replaces the current VmReconfigure model with shared logic for both Infra and Cloud.)

I do not agree with the statement that the "resize" logic is different then what reconfigure is doing. They are both trying to change the hardware configuration of the resource. Looking at the AWS documentation for cloud flavors it is clear that changing an instance flavor impacts the RAM, CPU and disk of the instance. (https://aws.amazon.com/ec2/instance-types)

(The resize method is exposed as "Reconfigure" for cloud already: https://github.com/ManageIQ/manageiq-ui-classic/blob/master/app/controllers/mixins/actions/vm_actions/resize.rb#L32)

As for quota, I would suggest that the automate team takes on that work as that feature is currently intertwined in automate methods which we are hoping to extract soon.

sseago · 2017-10-12T12:12:32Z

@gmcculloug I think we're probably focusing on different aspects of this when saying that the two actions are similar vs. different. The high-level logical idea of what's going on is similar, but the implementation as to what actually needs to happen on the back end (and how it's validated) is different. I'd agree with your long-term suggestion that, rather than try to use the existing VmReconfigure code for resize as well that instead the existing VmReconfigure be moved to VmInfraReconfigure and it can share a common base class with resize (we can call it VmCloudReconfigure). It may be that once the quota impl is done it will be clearer how much logic we can pull into the common base classes.

I'll go back into this PR and figure out what can be renamed here. I'm thinking that I can just go ahead and call the MiqRequest subclasses VmCloudReconfigure instead of VmResize, but without messing with trying to create a new common subclass now. Also, I'll probably leave the existing model-side "resize" functionality as-is, since that's not really being changed by this PR.

gmcculloug · 2017-10-12T15:06:15Z

app/models/vm_resize_task.rb

+
+  def do_request
+    _log.info("Resizing VM #{vm.id}:#{vm.name} to flavor #{options[:flavor_name]}")
+    vm.resize_queue(miq_request.userid, options[:flavor_id])


Please store the options[:flavor_id] as options[:instance_type] so it matches how the data is stored from the provisioning dialogs.

Example:
https://github.com/ManageIQ/manageiq/blob/master/product/dialogs/miq_dialogs/miq_provision_amazon_dialogs_template.yaml#L290

This will assist in quota validation which shares common logic with provisioning.

Ahh. I somehow missed this one. Working on it...

sseago · 2017-10-12T15:23:34Z

@gmcculloug I've made the suggested changes (changing "resize" to "cloud reconfigure", removing resize_queue, etc). See my comment on the associated UI PR about auto-approval, though.

gmcculloug · 2017-10-12T16:09:09Z

@sseago I do not see any approval comment on the UI associated PR. Do you have a link?

gmcculloug · 2017-10-12T17:21:26Z

Also this comment got lost: #16044 (comment)

Please store the options[:flavor_id] as options[:instance_type] so it matches how the data is stored from the provisioning dialogs.

Example:
https://github.com/ManageIQ/manageiq/blob/master/product/dialogs/miq_dialogs/miq_provision_amazon_dialogs_template.yaml#L290

This will assist in quota validation which shares common logic with provisioning.

sseago · 2017-10-12T18:40:06Z

@gmcculloug Ahh my comments were hidden because I accidentally clicked "review" instead of "comment" -- I think it's visible now: ManageIQ/manageiq-ui-classic#2241 (comment)

gmcculloug · 2017-10-16T19:34:14Z