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

David Lutterkort lutter at redhat.com
Mon Apr 20 22:37:34 UTC 2009


On Mon, 2009-04-20 at 15:22 -0700, Ian Main wrote:
> On Mon, 20 Apr 2009 15:13:13 +0000
> Scott Seago <sseago at redhat.com> 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!
> > +          alert += " VM Start action queued."
> > +        else
> > +          alert += " Resources are not available to start VM now."
> > +        end
> > +      end
> > +    end
> > +    return alert
> > +  end
> 
> 
> I also just realized that we return the newly created VM definition in
> the QMF API too.. so again we'd have to somehow figure out which new VM
> was created when making this call..  I think it would be good to return
> the VM ID?  Isn't this some kind of odd design where you have all these
> side effects from calling a single method?

That's the strange beauty of using modules here .. you can access the
newly created VM as @vm in whatever class includes that module - it
works well for the Rails code, so I'd hope it will also work for the QMF
agent; if it starts to get too confusing, we can think about changing
either the return type, e.g. to [alert, @vm], or document these
sideeffects more clearly.

> > +  def svc_update(vm_id, vm_hash, start_now, restart_now)
> > +    # from before_filter
> > +    @vm = Vm.find(vm_id)
> > +    @perm_obj = @vm.vm_resource_pool
> > +    @current_pool_id=@perm_obj.id
> > +    authorized!(Privilege::MODIFY)
> > +
> > +    #needs restart if certain fields are changed
> > +    # (since those will only take effect the next startup)
> > +    needs_restart = false
> > +    unless @vm.get_pending_state == Vm::STATE_STOPPED
> > +      Vm::NEEDS_RESTART_FIELDS.each do |field|
> > +        unless @vm[field].to_s == vm_hash[field]
> > +          needs_restart = true
> > +          break
> > +        end
> > +      end
> > +      current_storage_ids = @vm.storage_volume_ids.sort
> > +      new_storage_ids = vm_hash[:storage_volume_ids]
> > +      new_storage_ids = [] unless new_storage_ids
> > +      new_storage_ids = new_storage_ids.sort.collect {|x| x.to_i }
> > +      needs_restart = true unless current_storage_ids == new_storage_ids
> > +    end
> > +    vm_hash[:needs_restart] = 1 if needs_restart
> > +
> > +
> > +    alert = "VM was successfully updated."
> > +    Vm.transaction do
> > +      @vm.update_attributes!(vm_hash)
> > +      vm_provision
> > +      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!
> > +          alert += " VM Start action queued."
> > +        else
> > +          alert += " Resources are not available to start VM now."
> > +        end
> > +      elsif restart_now
> > +        if @vm.get_action_list.include?(VmTask::ACTION_SHUTDOWN_VM)
> > +          @task = VmTask.new({ :user        => @user,
> > +                               :task_target => @vm,
> > +                               :action      => VmTask::ACTION_SHUTDOWN_VM,
> > +                               :state       => Task::STATE_QUEUED})
> > +          @task.save!
> > +          @task = VmTask.new({ :user    => @user,
> > +                               :task_target => @vm,
> > +                               :action  => VmTask::ACTION_START_VM,
> > +                               :state   => Task::STATE_QUEUED})
> > +          @task.save!
> > +          alert += " VM Restart action queued."
> > +        else
> > +          alert += " Restart action was not available."
> > +        end
> > +      end
> > +    end
> > +    return alert
> > +  end
> 
> Is this something we want in the API?  It seems a little odd to me from
> a usage standpoint for the QMF implementation.  It will require that we
> map the info from the active record into a hash, then pass it to this
> method with the property changed, and then deal with side effects from
> tasks starting?

Not sure I understand - do you mean that you'd like to have a more
direct way to start/stop/etc. a VM for QMF ? I agree that that should be
the case, and we should have such methods in the service layer - we
definitely will need further changes to accomodate QMF.

For now the focus is more on separating presentation logic from core
business logic - restructuring the business logic to better fit QMF will
be the next step.

> > +  def svc_cancel_queued_tasks(vm_id)
> > +    @vm = Vm.find(vm_id)
> > +    @perm_obj = @vm.vm_resource_pool
> > +    @current_pool_id=@perm_obj.id
> > +    authorized!(Privilege::MODIFY)
> > +
> > +    Task.transaction do
> > +      @vm.tasks.queued.each { |task| task.cancel}
> > +    end
> > +    return "Queued tasks were successfully canceled."
> > +  end
> 
> So I'm guessing things like this we would just ignore?  I'm thinking we
> may end up using only a portion of the calls here..  In QMF I would
> think you would just do a search of open tasks where the target is the
> VM in question and then call cancel on them.

Sure .. we'd only use the above from the QMF agent if we needed to
support a 'cancel all tasks' API call.

David





More information about the ovirt-devel mailing list