[Ovirt-devel] [PATCH] added ability to reprovision vm via edit vm form

[Scott Seago]

Mohammed Morsi wrote:
> Perry N. Myers wrote:
>   
>> Scott Seago wrote:
>>     
>>> Mohammed Morsi wrote:
>>>       
>>>> ---
>>>>  wui/src/app/controllers/vm_controller.rb |   29
>>>> +++++++++++++++++------------
>>>>  wui/src/app/views/vm/_form.rhtml         |    2 +-
>>>>  2 files changed, 18 insertions(+), 13 deletions(-)
>>>>
>>>> diff --git a/wui/src/app/controllers/vm_controller.rb
>>>> b/wui/src/app/controllers/vm_controller.rb
>>>> index e55ec28..6d32427 100644
>>>> --- a/wui/src/app/controllers/vm_controller.rb
>>>> +++ b/wui/src/app/controllers/vm_controller.rb
>>>> @@ -267,6 +271,7 @@ class VmController < ApplicationController
>>>>      @perm_obj = @vm.vm_resource_pool
>>>>      @redir_obj = @vm
>>>>      @current_pool_id=@perm_obj.id
>>>> +    _setup_provisioning
>>>>    end
>>>>    def pre_vm_action
>>>>      pre_edit
>>>> diff --git a/wui/src/app/views/vm/_form.rhtml
>>>> b/wui/src/app/views/vm/_form.rhtml
>>>> index 60ed003..0dfe940 100644
>>>> --- a/wui/src/app/views/vm/_form.rhtml
>>>> +++ b/wui/src/app/views/vm/_form.rhtml
>>>> @@ -8,7 +8,7 @@
>>>>  <%= hidden_field_tag 'hardware_pool_id', @hardware_pool.id if
>>>> @hardware_pool %>
>>>>  
>>>>      <%= text_field_with_label "Name:", "vm", "description",
>>>> {:style=>"width:250px;"}  %>
>>>> -    <%= select_with_label "Operating System:", 'vm',
>>>> 'provisioning_and_boot_settings', @provisioning_options,
>>>> :style=>"width:250px;"  if create %>
>>>> +    <%= select_with_label "Operating System:", 'vm',
>>>> 'provisioning_and_boot_settings', @provisioning_options,
>>>> :style=>"width:250px;" %>
>>>>      <div class="clear_row" style="height:15px;"></div>
>>>>  
>>>>      <div class="form_heading">Resources</div>
>>>>   
>>>>         
>>> The UI needs to make it clear that changing the provisioning bits on
>>> a running VM will potentially wipe the contents of the current VM and
>>> reprovision from scratch. In addition, if the VM is currently
>>> running, should we provide the user an easy way to force an immediate
>>> reboot (so the provisioning can take effect). Currently we have a
>>> 'start VM now' button for creating VMs -- maybe for edit, we should
>>> provide the option to start/restart VM now.
>>>       
>> This is not always the case... This new UI for changing the
>> provisioning could also be used to 'boot a rescue ISO' or something
>> like that.  In which case it's not going to blow away your existing
>> disk.  A rescue ISO in cobbler looks the same as an F9 install ISO...
>>
>>     
Right -- it's definitely not always the case -- but it is one of the 
common cases.
>>> Also, we need to be careful in how we handle the boot device and
>>> cobbler profile fields. Currently cobbler profile is only set on
>>> create -- so the above won't actually register a new cobbler system
>>> (and what of the old cobbler system -- do we need to un-register it?
>>> There's also the automatic update of the boot device on successful
>>> boot -- I don't think the above will cause any problems with that
>>> though.
>>>       
> Correct me if I'm wrong but upon submitting the edit vm form, vm.save!
> is invoked after which if you look at the database you will see the
> 'boot device' is updated and the 'provisioning' field is set to the
> correct cobbler profile. I'm not sure why we would have to unregister it
> (as opposed to just overwriting what profile is used), is there some
> reason why we should? (perhaps another system can't use the cobbler
> profile until its unregistered?)
>
>   
I"m not sure if cobbler systems can be edited like that. Either way we'd 
have to update the cobbler system -- either by deleting and recreating 
it, or by editing it. My main point was that the cobbler system is only 
currently created/modified at create_vm time. The issue here is that if 
we don't tell cobbler that this system needs a new profile, it will 
still use the old one.
>   
>> Maybe we should treat editing the provisioning information as a create
>> call.  i.e. if you change the VM in this way it removes the exising vm
>> and creates a new one.  Would that work?
>>     
> I'm not a huge fan of deleting / recreating as we have the need of
> history (for graphing and what not) and this might throw a monkey wrench
> into how we track that, eg we'd have to retain the old vm to lookup
> information about it and what not.
>
>   
Well we could still use a 'create' call even if the db record isn't 
dropped. I think it would be cleaner to create a 'reprovision' task 
instead -- this task would do the same thing as the 'create_vm' task, 
except it would be a valid action on a stopped VM. So for a running vm, 
we'd want to:
1) stop VM
2) reprovision VM
3) start VM.

For a stopped VM, all we'd need to do is issue the reprovision_vm call 
-- and when the VM is next started, it would get the new OS/rescue/etc.
>   
>> If unregistration of a system is necessary we should make that happen
>> in the create call in taskomatic for ALL creates.  i.e. whenever
>> create is called it should first try to delete the system record in
>> cobbler (and fail silently if that record does not exist) and then
>> create a new one.
>>
>> That will handle the reprovisioning case.
>>
>> Perry
>>     
>
> Just to make everything perfectly clear you are looking for the
> following additional requirements to be satisfied is addition to whats
> already there
> 1. On editing a vm, if a different boot device / provisioning method is
> selected, display a confirmation dialog that this _might _ overwrite the
> current vm if the user is not careful. Clicking 'ok' should cause the
> app to proceed as normal, clicking cancel should take the user back to
> the edit vm form.
>   
Something like this -- I'm not even sure we need a confirmation dialog 
-- it may be sufficient for the form itself to make this clear in the 
text surrounding the provisioning section.
> 2. When editing a vm, if it is stopped, provide the 'start now'
> checkbox, and if it is running provide a 'restart now' checkbox.
> Clicking either will register the appropriated event in the tasks table
> for task-o-matic to pick up.
>   
Yes, I think so. Right now the 'start now' checkbox is only shown for 
creating VMs. For editing VMs it's more complex though, so I"m not sure 
exactly what we want, as there are 4 different scenarios:

1) VM stopped, provisioning unchanged:
2) VM running, provisioning unchanged
3) VM stopped, reprovisioning
4) VM running, reprovisioning

There are also the other VM states (suspended, etc) which we wouldn't 
want to do anything with.
so I think for the edit case we'd want to show a 'restart' checkbox if 
it's running, and show a 'start' checkbox if it's stopped.

Upon save, we issue tasks as appropriate. If 'restart' is selected, we 
create both 'stop' and 'start' tasks -- if 'start' is selected, only the 
'start' task. And the fun part: if we're reprovisioning, we need the 
'reprovision' task included as well -- and this has to be submitted 
'before' any start tasks.

> 3. If neccessary (pending questions above) perform additional steps to
> unregister / register cobbler profile for edited vm (besides just
> writing to the 'boot device' and 'provisioning' fields of the vm table,
> what else needs to be done for this? a new task in the tasks table?)
>
>   
Cobbler needs to be updated so that the cobbler system matches what we 
want installed. Currently this is only done for 'create_vm' actions -- 
so the new 'reprovision_vm' task suggested above would be the 
appropriate place to do this.
>   -Mo
>   

Scott