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

[Ian Main]

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?

> +  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?

> +  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.

> +
> +  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..

> +  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