[Ovirt-devel] [PATCH server] First attempt at refactoring the vm_controller using the service layer.

[Scott Seago]

David Lutterkort wrote:
> On Mon, 2009-04-20 at 15:13 -0700, Ian Main wrote:
>   
>>> +  def svc_create(vm_hash, start_now)
>>> +    # from before_filter
>>> +    vm_hash[:state] = Vm::STATE_PENDING
>>> +    @vm = Vm.new(params[:vm])
>>> +    @perm_obj = @vm.vm_resource_pool
>>> +    @current_pool_id=@perm_obj.id
>>> +    authorized!(Privilege::MODIFY)
>>> +
>>> +    alert = "VM was successfully created."
>>> +    Vm.transaction do
>>> +      @vm.save!
>>> +      vm_provision
>>> +      @task = VmTask.new({ :user        => @user,
>>> +                           :task_target => @vm,
>>> +                           :action      => VmTask::ACTION_CREATE_VM,
>>> +                           :state       => Task::STATE_QUEUED})
>>> +      @task.save!
>>> +      if start_now
>>> +        if @vm.get_action_list.include?(VmTask::ACTION_START_VM)
>>> +          @task = VmTask.new({ :user        => @user,
>>> +                               :task_target => @vm,
>>> +                               :action      => VmTask::ACTION_START_VM,
>>> +                               :state       => Task::STATE_QUEUED})
>>> +          @task.save!
>>>       
>> I can see this is a bit odd.. if we're calling this from QMF then
>> there's going to be issues here..  In the current API I don't have
>> create VM returning a task, which is kind of beside the point, but if
>> it did then we would have to have a QMF object of this task figured
>> out when the command returned (there are other APIs that do return
>> tasks like start_vm).  I'm not entirely sure how that would work using
>> this API right now.. we'd have to somehow figure out which task it was
>> that was put in the DB.. I wonder if there is a better way to approach
>> this?
>>     
>
> It depends on which task the QMF API would return, but in general the
> last task created by this method would be available in the QMF agent as
> @task.
>
> If you needed something else, we could change the above code to remember
> it somewhere else - these are all internal API's, and we can change
> things around as we go along. I expect that once we start implementing
> the QMF agent, we'll discover lots of places where things need to be
> changed to be more convenient for QMF purposes.
>
>   
>> I'm not going to argue this hard here because I know this is a first cut
>> and just a refactoring but I do think there should be no tasks
>> associated with creating a VM definition for the API.  I think we've
>> talked about this before.. not a big deal, just wanted to bring it up
>> for future reference :).
>>     
>
> Yeah, I think we should see if we can kill the CREATE_VM task
> altogether, but as you said, it's a little besides the point, since it's
> orthogonal to the refactoring happening right now.
>
> David
>   
Yes, the create_vm task is definitely an issue to discuss. It's 
completely orthogonal to the refactoring and the QMF API, but it's still 
something we need to nail down. At the moment the create_vm task doesn't 
really do anything, and we don't even have a modify_vm task. At one 
point the cobbler system creation was in the create_vm task, as the 
original goal was to isolate the WUI from making external calls as much 
as possible. We ended up having to pull it back into the WUI when we 
added cobbler functionality into the modify VM form, but we had 
discussed eventually pulling out all cobbler system 
creation/modification bits _back_ into the create_vm task as well as a 
not-yet-created modify_vm task. At the time clalance and I were thinking 
that pulling it into taskomatic would be a cleaner way of handling this 
code. The idea was to avoid doing anything from the WUI that could 
potentially block.

Is there any reason why doing this in taskomatic would be a bad thing?

Scott