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

[Jason Guiditta]

On Tue, 2008-07-15 at 22:45 -0400, Scott Seago wrote:
> 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.

ACK, I was able to test this, and it appears to do everything intended
(once I removed all the 'widget' calls, since the svg plugin does not
work for me).

-j
> 
> And some comments on your comments below...
> >   
> 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.

Ah, thanks, that makes much more sense now.
>  
> 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 should have been more specific here - I really didn't mean to move
_all_ of these 'somewhere', just the ones that there were a lot of and
everything had the same params for the hash.  I agree though, since we
use these status messages for visual output, we should indeed combine
any refactoring here to account for i18n.