[Ovirt-devel] [PATCH server] missing ovirt wui validations (round 1)
Scott Seago
sseago at redhat.com
Tue Dec 9 06:13:29 UTC 2008
Mohammed Morsi wrote:
> note this is a review only patch, as I haven't tested it yet
> (or even run it) so there will most likely be syntax and
> other errors
>
>
It looks good overall, but a few comments inline...
> Attached is additional validations, added to the rails model
> and controller layers verifying:
> bondings: arp related fields
> boot_types: label uniqueness and proto inclusion
> cpus: various fields are set and min values
> help_sections: presence and uniqueness of fields
> hosts: presence and format of various fields
> ip_addresses: fk existance / integrity
> nics: mac address format
> permissions: presence / inclusion of various fields
> pools: fields requied for tree present
> quotas: minimum values
> smart_pool_tags: tagged_id / type fk present
> storage_pools: type, state field inclusion, min values
> tasks: various fields present and consistensy maintained
> usages: label, other fields present, unique
> vms: uuid of correct format, min values, various fields present
>
> can only move host and storage to / from pools when you have
> sufficient permissions and target entities don't have associated
> vms
>
> can only destroy storage volumes / pools if there are no attached vms
> ---
>
> diff --git a/src/app/controllers/hardware_controller.rb b/src/app/controllers/hardware_controller.rb
> index 4dda736..a329f90 100644
> --- a/src/app/controllers/hardware_controller.rb
> +++ b/src/app/controllers/hardware_controller.rb
> @@ -285,6 +285,23 @@ class HardwareController < PoolController
> end
>
> def move_hosts
> + # if vm's present on host, fail
> + resource_ids_str = params[:resource_ids]
> + resource_ids_str.split(",").each {|x|
> + if Host.find(x).vms > 0
> + render :json => { :success => false,
> + :alert => "Cannot move hosts currently associated w/ vms" }
> + return
> + end
> + }
> +
>
To be consistent with the other multi-object actions, it might be
worthwhile to go ahead and move those VMs that don't ahve hosts and just
report which ones weren't moved. For a first pass though it may be
acceptable to abort the action if any in-use hosts are selected, but at
the least the error msg should identify which hosts have VMs -- and
identify all VMs which fail rather than just the first.
> + # if user doesn't have modify permission on both source and destination
> + unless @pool.can_modify(@user) and Pool.find(params[:target_pool_id]).can_modify(@user)
> + render :json => { :success => false,
> + :alert => "Cannot move hosts without admin permissions on both pools" }
> + return
> + end
> +
> edit_items(Host, :move_hosts, params[:target_pool_id], :move)
> end
>
> @@ -293,6 +310,25 @@ class HardwareController < PoolController
> end
>
> def move_storage
> + # if vm's present on host, fail
> + resource_ids_str = params[:resource_ids]
> + resource_ids_str.split(",").each {|x|
> + StoragePool.find(x).volumes.each { |v|
> + if v.vms.size > 0
> + render :json => { :success => false,
> + :alert => "Cannot move storage currently associated w/ vms" }
> + return
> + end
> + }
> + }
>
As with hosts, we should consider allowing partial success here, and
whether or not we allow partial success we should report which storage
pools caused the failure.
> +
> + # if user doesn't have modify permission on both source and destination
> + unless @pool.can_modify(@user) and Pool.find(params[:target_pool_id]).can_modify(@user)
> + render :json => { :success => false,
> + :alert => "Cannot move storage without admin permissions on both pools" }
> + return
> + end
> +
> edit_items(StoragePool, :move_storage, params[:target_pool_id], :move)
> end
>
Similar permissions/usage checks should be made for add_hosts and
add_storage -- those are essentially the same action as move_hosts and
move_storage, but the difference is the direction of moving. move_xxx
moves hosts _from_ here _to_ there, and add_xxx moves hosts _from_ there
_to_ here.
To avoid having four very similar blocks of code, though, we should
probablhy add the above checks to edit_items rather than the 4 actions
that call it. In edit_items you've already got
resource_ids_str/resource_ids available. The StoragePool/Host class
you're calling 'find' on is passed in as item_class. The one difficulty
here would be the "movable" check -- since it's different for hosts and
storage pools. But this should probably be abstracted out anyway. You
could add a "movable?" method to host.rb and storage_pool.rb that
returned vms.empty? for host. For storage_pool the result is a bit more
complex, since you've also got lvm to consider, but essentially
storage_pool.movable? should call movable? for each storage volume and
return false if any fail. storage_volume.movable? would have to check
vms.empty? as well as whether it had an associated lvm pool -- in the
latter case it would have to call movable? on the lvm pool.
>
> diff --git a/src/app/controllers/storage_controller.rb b/src/app/controllers/storage_controller.rb
> index 148d1be..804ba75 100644
> --- a/src/app/controllers/storage_controller.rb
> +++ b/src/app/controllers/storage_controller.rb
> @@ -310,6 +310,14 @@ class StorageController < ApplicationController
> end
>
> def destroy
> + @storage_pool.volumes.each { |volume|
> + if volume.vms.size > 0
> + format.json { render :json => { :object => "storage_pool",
> + :success => false, :alert => "Cannot delete storage with associated vms" } }
> + return
> + end
> + }
> +
>
Similar issue here as with moving storage -- we need to consider LVM as
well -- but the above approach will also help you here.
> pool = @storage_pool.hardware_pool
> if @storage_pool.destroy
> alert="Storage Pool was successfully deleted."
>
> diff --git a/src/app/models/host.rb b/src/app/models/host.rb
> index 640782d..5328500 100644
> --- a/src/app/models/host.rb
> +++ b/src/app/models/host.rb
> @@ -51,6 +51,27 @@ class Host < ActiveRecord::Base
> [ :search_users, 'U', "search_users" ] ],
> :eager_load => :smart_pools
>
> + validates_presence_of :hardware_pool_id,
> + :message => 'A hardware pool id must be specified.'
> +
> + validates_presence_of :uuid,
> + :message => 'A uuid must be specified.'
> +
> + validates_format_of :uuid,
> + :with => %r([a-f0-9]{8}-[a-f0-9]{4}-[a-f0-9]{4}-[a-f0-9]{4}-[a-f0-9]{12})
> +
>
uuid format check above won't work for hosts, as we're currently using
the hostname for Host uuids
> + # validate hostname doesn't contain invalid chars?
> +
> + # validates presence, format, or inclusion of arch?
> +
>
Yes we need these too.
> + validates_inclusion_of :hypervisor_type,
> + :in => HYPERVISOR_TYPES
> +
> + validates_inclusion_of :state,
> + :in => STATES + Task::COMPLETED_STATES + TASK::WORKING_STATES
>
> KVM_HYPERVISOR_TYPE = "KVM"
> HYPERVISOR_TYPES = [KVM_HYPERVISOR_TYPE]
> diff --git a/src/app/models/pool.rb b/src/app/models/pool.rb
> index 7034e79..5cfc947 100644
> --- a/src/app/models/pool.rb
> +++ b/src/app/models/pool.rb
> @@ -52,6 +52,13 @@ class Pool < ActiveRecord::Base
> validates_presence_of :name
> validates_uniqueness_of :name, :scope => :parent_id
>
> + validates_presence_of :parent_id
> + validates_presence_of :lft
> + validates_presence_of :rgt
> +
> + validates_inclusion_of :type,
> + :in => %w( DirectoryPool HardwarePool VmResourcePool SmartPool )
> +
> # overloading this method such that we can use permissions.admins to get all the admins for an object
> has_many :permissions, :dependent => :destroy, :order => "id ASC" do
> def super_admins
> diff --git a/src/app/models/storage_volume.rb b/src/app/models/storage_volume.rb
> index 39b72d5..0c05378 100644
> --- a/src/app/models/storage_volume.rb
> +++ b/src/app/models/storage_volume.rb
> @@ -32,6 +32,21 @@ class StorageVolume < ActiveRecord::Base
> end
> end
>
> + validates_presence_of :storage_pool_id
> +
> + validates_inclustion_of :state,
> + :in => [ STATE_PENDING_SETUP, STATE_PENDING_DELETION, STATE_AVAILABLE]
> +
> + validates_numericality_of :size,
> + :greater_than_or_equal_to => 0,
> + :unless => Proc.new { |storage_volume| storage_volume.nil? }
> +
> + validates_inclusion_of :type,
> + :in => %w( IscsiStorageVolume LvmStorageVolume NfsStorageVolume )
> +
> + # FIXME need to validate lun, filename, lv_name, lv_owner_perms,
> + # lv_group_perms, lv_mode_perms (which depends on what subclasses?)
> +
>
Yes these should be validated in their respective subclasses:
lun: iscsi
filename: nfs
lv_*: lvm
> STATE_PENDING_SETUP = "pending_setup"
> STATE_PENDING_DELETION = "pending_deletion"
> STATE_AVAILABLE = "available"
> diff --git a/src/app/models/task.rb b/src/app/models/task.rb
> index f231c18..bf5fbbd 100644
> --- a/src/app/models/task.rb
> +++ b/src/app/models/task.rb
> @@ -45,6 +45,16 @@ class Task < ActiveRecord::Base
> COMPLETED_STATES = [STATE_FINISHED, STATE_FAILED, STATE_CANCELED]
> WORKING_STATES = [STATE_QUEUED, STATE_RUNNING, STATE_PAUSED]
>
> + validates_inclusion_of :type,
> + :in => %w( HostTask StorageTask StorageVolumeTask VmTask )
> +
> + validates_inclustion_of :state,
> + :in => COMPLETED_STATES + WORKING_STATES
> +
> + # FIXME validate action depending on type / subclass
> + # validate task_target_id, task_type_id, arg, message
> + # depending on subclass, action, state
> +
>
> def cancel
> self[:state] = STATE_CANCELED
> save!
> @@ -71,4 +81,10 @@ class Task < ActiveRecord::Base
> ""
> end
>
> + def validate
> + errors.add("id", "Parent must be specified") if hardware_pool_id.nil? and vm_resource_pool_id.nil?
>
hardware_pool_id and vm_resource_pool_id are specified in
after_initialize, so we probably don't need to explicitly validate it.
> + errors.add("time_ended", "Tasks ends before its started") unless time_ended > time_started
> + errors.add("time_started", "Tasks starts before its created") unless time_started > time_created
> + end
> +
> e
> diff --git a/src/app/models/vm.rb b/src/app/models/vm.rb
> index 227f343..6038ac6 100644
> --- a/src/app/models/vm.rb
> +++ b/src/app/models/vm.rb
> @@ -34,7 +34,35 @@ class Vm < ActiveRecord::Base
>
> validates_presence_of :uuid, :description, :num_vcpus_allocated,
> :boot_device, :memory_allocated_in_mb,
> - :memory_allocated, :vnic_mac_addr
> + :memory_allocated, :vnic_mac_addr,
> + :vm_resource_pool_id
> +
> + validates_format_of :uuid,
> + :with => %r([a-f0-9]{8}-[a-f0-9]{4}-[a-f0-9]{4}-[a-f0-9]{4}-[a-f0-9]{12})
> +
> + validates_inclusion_of :state,
> + :in => RUNNING_STATES + DESTROYABLE_STATES
> +
>
RUNNING + DESTROYABLE misses a couple states. The closest thing we have
to a full state list is EFFECTIVE_STATE.keys, although that hash is
missing STATE_INVALID. Try adding STATE_INVALID => STATE_INVALID to
EFFECTIVE_STATES.
> + validates_numericality_of :needs_restart,
> + :greater_than_or_equal_to => 0,
> + :less_than_or_equal_to => 1,
> + :unless => Proc.new{ |vm| vm.needs_restart.nil? }
> +
> + validates_numericality_of :memory_used,
> + :greater_than_or_equal_to => 0,
> + :unless => Proc.new{ |vm| vm.memory_used.nil? }
> +
> + validates_numericality_of :vnc_port,
> + :greater_than_or_equal_to => 0,
> + :unless => Proc.new{ |vm| vm.vnc_port.nil? }
> +
> + validates_numericality_of :num_vcpus_allocated,
> + :greater_than_or_equal_to => 0,
> + :unless => Proc.new{ |vm| vm.num_vcpus_allocated.nil? }
> +
> + validates_numericality_of :memory_allocated,
> + :greater_than_or_equal_to => 0,
> + :unless => Proc.new{ |vm| vm.memory_allocated.nil? }
>
> acts_as_xapian :texts => [ :uuid, :description, :vnic_mac_addr, :state ],
> :terms => [ [ :search_users, 'U', "search_users" ] ],
>
Scott
More information about the ovirt-devel
mailing list