[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