[Ovirt-devel] [PATCH server] First attempt at refactoring the vm_controller using the service layer.
Scott Seago
sseago at redhat.com
Tue Apr 21 02:09:02 UTC 2009
Ian Main wrote:
> On Mon, 20 Apr 2009 15:13:13 +0000
> Scott Seago <sseago at redhat.com> wrote:
>
>
>> This code is based on David Lutterkort's proposal outlined in
>> https://www.redhat.com/archives/ovirt-devel/2009-April/msg00086.html
>>
>>
> [SNIP]
>
>
>> +
>> + 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?
>
>
Yeah as David said we are "returning" it by setting the @vm instance
variable. (and the @task variable too). I don't really see these so much
as side effects than as the primary effect of calling this method.
svc_create is responsible for all business logic associated with
creating a new VM -- which includes the VM description record in the DB
_and_ any required tasks. This includes the create task (if we end up
keeping it long-term) and the start task -- since from a usability point
of view we definitely want to give the user the chance to start the
newly-created VM as part of the single logical creation action.
>> + 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?
>
>
Most of this stuff is done as a hash since on the web side we already
get a hash from the form submission _and_ AR expects a hash to come in
to define the new VM. I'd expect the public QMF API wouldn't get a hash
-- we'd pass in explicit parameters, and the QMF implementation (the
class that includes the vm_service module) would take those params and
generate the hash to pass into svc_create, scv_update, etc. The task
bits are parallel to the 'new VM' case -- since some VM characteristics
require a restart to take effect, we want to give the user the
opportunity to trigger a restart as part of the update action. It may be
that for QMF we _don't_ want to include restart as an option for the
modify task -- that's OK -- we just wouldn't expose the start_now,
restart_now attributes in the API.
>> + def svc_destroy(vm_id)
>> + # from before_filter
>> + @vm = Vm.find(vm_id)
>> + @perm_obj = @vm.vm_resource_pool
>> + @current_pool_id=@perm_obj.id
>> + authorized!(Privilege::MODIFY)
>> +
>> + unless @vm.is_destroyable?
>> + raise ActionError.new("Virtual Machine must be stopped to delete it")
>> + end
>> + destroy_cobbler_system(@vm)
>> + @vm.destroy
>> + return "Virtual Machine wa ssuccessfully deleted."
>> + end
>> +
>> + def svc_vm_action(vm_id, vm_action, action_args)
>> + @vm = Vm.find(vm_id)
>> + @perm_obj = @vm.vm_resource_pool
>> + @current_pool_id=@perm_obj.id
>> + authorized!(Privilege::MODIFY)
>> + unless @vm.queue_action(@user, vm_action, action_args)
>> + raise ActionError.new("#{vm_action} is an invalid action.")
>> + end
>> + return "#{vm_action} was successfully queued."
>> + end
>> +
>> + 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 -- and if there was _no_ reason for a cancel_queued_tasks_for_vm
action in the API I could even pull this method back into the
controller. We don't need _all_ methods going through the service layer
-- only those that are useful for both WUI and QMF.
>> +
>> + def vm_provision
>> + if @vm.uses_cobbler?
>> + # spaces are invalid in the cobbler name
>> + name = @vm.uuid
>> + system = Cobbler::System.find_one(name)
>> + unless system
>> + system = Cobbler::System.new("name" => name,
>> + @vm.cobbler_type => @vm.cobbler_name)
>> + system.interfaces = [Cobbler::NetworkInterface.new(
>> + {'mac_address' => @vm.vnic_mac_addr})]
>> + system.save
>> + end
>> + end
>> + end
>>
>
> I'm not sure how these fit in either..
>
>
This is just an internal method called by svc_create and svc_update --
this is the bit that could be pulled into the start_vm (and
not-currently-created modify_vm) tasks in the future. It won't be part
of the API.
Scott
>> + def destroy_cobbler_system(vm)
>> + # Destroy the Cobbler system first if it's defined
>> + if vm.uses_cobbler?
>> + system = Cobbler::System.find_one(vm.cobbler_system_name)
>> + system.remove if system
>> + end
>> + end
>>
>
> Ian
>
> _______________________________________________
> Ovirt-devel mailing list
> Ovirt-devel at redhat.com
> https://www.redhat.com/mailman/listinfo/ovirt-devel
>
More information about the ovirt-devel
mailing list