[Ovirt-devel] [PATCH] added VM-level migration support to the WUI. Still no back end bits here, though.

[Scott Seago]

Jason Guiditta wrote:
> Scott, could you give me a quick scenario to test against this so I can
> see it does what you meant it to?  Comments based just on the code
> inline.
>
>   
OK. so there are several things to look for.

First, there's the question of the migrate link itself. In the VM 
details pane, you should get a migrate action link if:
1) you have permission to migrate this VM; and
2) the VM can be migrated.

For 1) -- you've got to have the administrator role on the HW pool that 
contains this VM's Virtual Machine pool. So if you're ovirtadmin, you 
should see the link. If you create a new user, give that user admin or 
user permissions on the VM pool only, the migrate link won't show up.

For 2) the VM must have a "pending state" of running -- i.e. take the VM 
state and "apply" all pending tasks, and if the result is "running" then 
the VM can be migrated. So a stopped VM with no pending tasks can't be 
migrated. A running VM with no pending tasks can. A stopped VM with a 
pending (not yet started) "start_vm" task can be migrated too.

Now once you get the migrate link, you should get a dialog form upon 
clicking it. You should see a flexigrid with all hosts in the hardware 
pool (minus the host that the VM is currently running on if the VM 
currently has a host listed. it won't have a host if, for example, it's 
not yet running but there's a pending start task. If you click "migrate" 
without choosing a host, then there should be a migrate_vm task inserted 
for the proper VM and user -- the 'args' field will be empty. If you 
click in the row for a host, then that host should show as selected, and 
clicking "migrate" creates a migrate task with that host's ID in the 
'args' field.

Two points for the form action though:
1) the "selected host" part of the dialog probably needs styling work 
from Tim -- I was just going for basic functionality here.
2) as we don't currently have a "task list" UI, you'll have to test this 
by looking at the tasks table directly in the database.

And a final point -- if you have taskomatic running, you'll probably see 
that all the tasks fail -- that's because Taskomatic doesn't yet know 
about migration.

And some comments on your comments below...
> On Tue, 2008-07-15 at 11:28 -0400, Scott Seago wrote:
>   
>> diff --git a/wui/src/app/controllers/host_controller.rb b/wui/src/app/controllers/host_controller.rb
>> index 20571d7..4e4375a 100644
>> --- a/wui/src/app/controllers/host_controller.rb
>> +++ b/wui/src/app/controllers/host_controller.rb
>> @@ -44,6 +44,17 @@ class HostController < ApplicationController
>>      render :layout => 'selection'    
>>    end
>>  
>> +  def quick_summary
>> +    pre_show
>> +    set_perms(@perm_obj)
>> +    unless @can_view
>> +      flash[:notice] = 'You do not have permission to view this host: redirecting to top level'
>> +      #perm errors for ajax should be done differently
>>     
> Considering it is almost all ajax now, we should figure out what
> 'differently' is asap
>   
Agreed. this handles stuff "just like the rest of it" -- i.e. it's 
broken in the same way. When we switched to the ajax design, we 
explicitly avoided fixing permissions/redirection across the board for 
schedule reasons. So this needs to be added to one of the upcoming 
sprint task lists (probably not the current one though!)
>> diff --git a/wui/src/app/models/vm.rb b/wui/src/app/models/vm.rb
>> index 617512e..b607886 100644
>> --- a/wui/src/app/models/vm.rb
>> +++ b/wui/src/app/models/vm.rb
>> @@ -22,7 +22,7 @@ require 'util/ovirt'
>>  class Vm < ActiveRecord::Base
>>    belongs_to :vm_resource_pool
>>    belongs_to :host
>> -  has_many :tasks, :class_name => "VmTask", :dependent => :destroy, :order => "id DESC" do
>> +  has_many :tasks, :class_name => "VmTask", :dependent => :destroy, :order => "id ASC" do
>>      def queued
>>        find(:all, :conditions=>{:state=>Task::STATE_QUEUED})
>>      end
>> @@ -58,6 +58,9 @@ class Vm < ActiveRecord::Base
>>    STATE_SAVING         = "saving"
>>    STATE_SAVED          = "saved"
>>    STATE_RESTORING      = "restoring"
>> +
>> +  STATE_MIGRATING      = "migrating"
>> +
>>    STATE_CREATE_FAILED  = "create_failed"
>>    STATE_INVALID        = "invalid"
>>  
>> @@ -68,7 +71,8 @@ class Vm < ActiveRecord::Base
>>                            STATE_SUSPENDING,
>>                            STATE_RESUMING,
>>                            STATE_SAVING,
>> -                          STATE_RESTORING]
>> +                          STATE_RESTORING,
>> +                          STATE_MIGRATING]
>>  
>>    EFFECTIVE_STATE = {  STATE_PENDING       => STATE_PENDING,
>>                         STATE_UNREACHABLE   => STATE_UNREACHABLE,
>> @@ -83,9 +87,15 @@ class Vm < ActiveRecord::Base
>>                         STATE_SAVING        => STATE_SAVED,
>>                         STATE_SAVED         => STATE_SAVED,
>>                         STATE_RESTORING     => STATE_RUNNING,
>> +                       STATE_MIGRATING     => STATE_RUNNING,
>>                         STATE_CREATE_FAILED => STATE_CREATE_FAILED}
>>    TASK_STATE_TRANSITIONS = []
>>  
>> +  def get_hardware_pool
>> +    pool = vm_resource_pool
>> +    pool = pool.get_hardware_pool if pool
>> +    pool
>> +  end
>>    def storage_volume_ids
>>      storage_volumes.collect {|x| x.id }
>>    end
>> @@ -126,9 +136,9 @@ class Vm < ActiveRecord::Base
>>      RUNNING_STATES.include?(get_pending_state)
>>    end
>>  
>> -  def get_action_list
>> +  def get_action_list(user=nil)
>>     
> You do this in several places below, and I must be misunderstanding -
> what is the purpose of having this user var if it is always nil?
>
>   
Mostly an issue of backwards compatibility. Up to now, we didn't pass a 
user into this method -- the intent is to get a list of valid actions 
for a VM given its state. With the migration patch, we also care _who_ 
will be doing the actions -- i.e. different actions can have different 
permission requirements. But if a user isn't passed in, the method still 
returns the list of possible actions (i.e. "somebody can do this") which 
may still be a valid use case on the admin or reporting side. i.e. "give 
me a list of stoppable VMs, etc.

However, for the action list that we display in the VM details pane, we 
_do_ want to filter based on user permissions -- if I'm just a user in 
this VM pool, I can't migrate. If I'm an admin for this VM's Hardware 
Pool, I _can_ migrate. Above I'm just supplying the default value if 
get_action_list is called, like before, with no args. I do pass in the 
user in the vm_controller where the list is generated for the details pane.
>
>> diff --git a/wui/src/app/models/vm_task.rb b/wui/src/app/models/vm_task.rb
>> index aa12903..3f52478 100644
>> --- a/wui/src/app/models/vm_task.rb
>> +++ b/wui/src/app/models/vm_task.rb
>> @@ -33,59 +33,97 @@ class VmTask < Task
>>  
>>    ACTION_UPDATE_STATE_VM = "update_state_vm"
>>  
>> +  # for migrate VM action, args provides the optional target host
>> +  ACTION_MIGRATE_VM  = "migrate_vm"
>> +
>> +  PRIV_OBJECT_VM_POOL = "vm_resource_pool"
>> +  PRIV_OBJECT_HW_POOL = "get_hardware_pool"
>> +
>> +
>>     
>
> Perhaps this is less a comment on the patch and more on the way we are
> doing this.  It feels vaguely wrong to me to do it this way.  I see a
> lot of repeated attributes, which to me says it should be more
> encapsulated in some way.  Would it make more sense to put these
> 'contants' in the database?  Or even a fixture of some sort?
>
>   
Yeah, again I'm just being consistent here :-) -- but now is the time to 
raise more general architecture/implementation issues for the project, 
so we should probably start a list of "things to revisit in the current 
design/implementation" and prioritize those separately from the 
functionality-based patches. As for this particular case, I started 
doing this early on as an improvement over the proliferation of magic 
strings that was starting to happen. i.e. worst case scenario for this 
sort of list of random text strings is to inline the string literal 
everywhere. I was trying to consolidate all required string literals in 
one place per model class into a list (even if an unstructured one) of 
string constants in each model. Some of these might make sense in the db 
-- i.e. create tables for vm_actions, vm_states, task_states, etc.  
ACTION_MIGRATE_VM above is an example of this.  PRIV_OBJECT_VM_POOL on 
the other hand has a more limited use -- in this case it signifies what 
method to call on the vm model object to determine what pool to use for 
permissions checks -- and, as such is probably best left as-is.

Then comes the already-considered question of internationalization -- in 
which case, you might want to forget everything I said above and start 
over. But, seriously, we really can't figure out how we want to redesign 
our handling of string literals apart from the more general 
internationalization case, so ultimately this is probably just another 
part of that process.
> I had trouble applying this (manually dropped in the 4 lines), but it
> seems to have worked for steve, so hopefully it is just me
>
>   
I wonder if it's the usual dos/windows cr/lf junk creeping back in -- it 
was all over the jquery stuff when we first imported it and it really 
confuses git. Anyway if it applied for steve, then we're probably fine, 
and upon ACK I can just push my change from my repo.