Mohammed Morsi
2008-Dec-08 21:05 UTC
[Ovirt-devel] [PATCH server] missing ovirt wui validations (round 1)
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 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 --- src/app/controllers/hardware_controller.rb | 36 ++++++++++++++++++++++++++++ src/app/controllers/storage_controller.rb | 8 ++++++ src/app/models/bonding.rb | 11 ++++++++ src/app/models/boot_type.rb | 5 ++++ src/app/models/cpu.rb | 23 +++++++++++++++++ src/app/models/help_section.rb | 34 ++++++++++++++++++++++++++ src/app/models/host.rb | 21 ++++++++++++++++ src/app/models/ip_address.rb | 4 +++ src/app/models/nic.rb | 11 ++++++++ src/app/models/permission.rb | 7 +++++ src/app/models/pool.rb | 7 +++++ src/app/models/quota.rb | 22 +++++++++++++++++ src/app/models/smart_pool_tag.rb | 5 ++++ src/app/models/storage_pool.rb | 9 +++++++ src/app/models/storage_volume.rb | 15 +++++++++++ src/app/models/task.rb | 16 ++++++++++++ src/app/models/usage.rb | 6 ++++ src/app/models/vm.rb | 30 ++++++++++++++++++++++- 18 files changed, 269 insertions(+), 1 deletions(-) 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 + } + + # 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 + } + } + + # 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 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 + } + pool = @storage_pool.hardware_pool if @storage_pool.destroy alert="Storage Pool was successfully deleted." diff --git a/src/app/models/bonding.rb b/src/app/models/bonding.rb index c9af38c..fabdb67 100644 --- a/src/app/models/bonding.rb +++ b/src/app/models/bonding.rb @@ -30,6 +30,8 @@ # interface. They can be ignored if not used. # class Bonding < ActiveRecord::Base + + belongs_to :host belongs_to :bonding_type belongs_to :vlan @@ -58,6 +60,15 @@ class Bonding < ActiveRecord::Base validates_presence_of :vlan_id, :message => 'A vlan must be specified.' + # verify arp ping address to be ipv4 if set + validates_format_of :arp_ping_address, + :with => %r{^(\d{1,3}\.){3}\d{1,3}$}, + :unless => Proc.new { |bonding| bonding.arp_ping_address.nil? } + + validates_numericality_of :arp_interval, + :greater_than_or_equal_to => 0, + :unless => Proc.new { |bonding| bonding.arp_interval.nil? } + protected def validate if vlan.boot_type.proto == 'static' and ip_addresses.size == 0 diff --git a/src/app/models/boot_type.rb b/src/app/models/boot_type.rb index 8ffbe67..1f30e8a 100644 --- a/src/app/models/boot_type.rb +++ b/src/app/models/boot_type.rb @@ -18,4 +18,9 @@ # also available at http://www.gnu.org/copyleft/gpl.html. class BootType < ActiveRecord::Base + validates_presence_of :label + validates_uniqueness_of :label, + :message => 'Label must be unique' + validates_inclusion_of :proto, + :in => %w ( static, dhcp, bootp ) end diff --git a/src/app/models/cpu.rb b/src/app/models/cpu.rb index 1a7d3cf..714a8ed 100644 --- a/src/app/models/cpu.rb +++ b/src/app/models/cpu.rb @@ -21,4 +21,27 @@ # class Cpu < ActiveRecord::Base belongs_to :host + + validates_presence_of :host_id, + :message => 'A host must be specified.' + + validates_numericality_of :cpu_number, + :greater_than_or_equal_to => 0 + + validates_numericality_of :core_number, + :greater_than_or_equal_to => 0 + + validates_numericality_of :number_of_cores, + :greater_than_or_equal_to => 1 + + validates_numericality_of :cpuid_level, + :greater_than_or_equal_to => 0 + + validates_numericality_of :speed, + :greater_than => 0 + # also verify speed in MHz ? + + validates_presence_of :vendor + validates_presence_of :model + validates_presence_of :family end diff --git a/src/app/models/help_section.rb b/src/app/models/help_section.rb index a891383..1de1e5a 100644 --- a/src/app/models/help_section.rb +++ b/src/app/models/help_section.rb @@ -1,2 +1,36 @@ +# Copyright (C) 2008 Red Hat, Inc. +# Written by Mohammed Morsi <mmorsi at redhat.com> +# +# This program is free software; you can redistribute it and/or modify +# it under the terms of the GNU General Public License as published by +# the Free Software Foundation; version 2 of the License. +# +# This program is distributed in the hope that it will be useful, +# but WITHOUT ANY WARRANTY; without even the implied warranty of +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the +# GNU General Public License for more details. +# +# You should have received a copy of the GNU General Public License +# along with this program; if not, write to the Free Software +# Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, +# MA 02110-1301, USA. A copy of the GNU General Public License is +# also available at http://www.gnu.org/copyleft/gpl.html. +# +# +HelpSection+ defines a section of the web help document to be +# available for a specific controller / action +# class HelpSection < ActiveRecord::Base + + validates_uniqueness_of :action, + :scope => :controller + :message => 'Controller / Action must be unique' + + validates_presence_of :controller, + :message => 'A controller must be specified.' + + validates_presence_of :action, + :message => 'An action must be specified.' + + validates_presence_of :section, + :message => 'A section must be specified.' end 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}) + + validates_presence_of :hostname, + :message => 'A hostname must be specified.' + + # validate hostname doesn't contain invalid chars? + + # validates presence, format, or inclusion of arch? + + 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/ip_address.rb b/src/app/models/ip_address.rb index 5d2e6af..3f246b1 100644 --- a/src/app/models/ip_address.rb +++ b/src/app/models/ip_address.rb @@ -24,4 +24,8 @@ class IpAddress < ActiveRecord::Base belongs_to :network belongs_to :nic belongs_to :bonding + + def validate + errors.add("id", "ip must be associated with foreign entity") if network_id.nil? and nic_id.nil? and bonding_id.nil? + end end diff --git a/src/app/models/nic.rb b/src/app/models/nic.rb index 5649763..c9f970d 100644 --- a/src/app/models/nic.rb +++ b/src/app/models/nic.rb @@ -24,12 +24,23 @@ class Nic < ActiveRecord::Base has_and_belongs_to_many :bondings, :join_table => 'bondings_nics' + validates_presence_of :mac, + :message => 'A mac must be specified.' + + validates_format_of :mac, + :with => %r{^([0-9a-f]{2}([:-]|$)){6}$} + validates_presence_of :host_id, :message => 'A host must be specified.' validates_presence_of :physical_network_id, :message => 'A network must be specified.' + validates_numericality_of :bandwidth, + :greater_than_or_equal_to => 0, + + # validate 'bridge' or 'usage_type' attribute ? + protected def validate if physical_network.boot_type.proto == 'static' and ip_addresses.size == 0 diff --git a/src/app/models/permission.rb b/src/app/models/permission.rb index ece3da5..b3929ad 100644 --- a/src/app/models/permission.rb +++ b/src/app/models/permission.rb @@ -24,9 +24,12 @@ class Permission < ActiveRecord::Base has_many :child_permissions, :dependent => :destroy, :class_name => "Permission", :foreign_key => "inherited_from_id" + validates_presence_of :pool_id + validates_presence_of :uid validates_uniqueness_of :uid, :scope => [:pool_id, :inherited_from_id] + ROLE_SUPER_ADMIN = "Super Admin" ROLE_ADMIN = "Administrator" ROLE_USER = "User" @@ -44,6 +47,10 @@ class Permission < ActiveRecord::Base ROLE_USER => [PRIV_VIEW, PRIV_VM_CONTROL], ROLE_MONITOR => [PRIV_VIEW]} + + validates_inclusion_of :user_role, + :in => ROLES.keys + def self.invert_roles return_hash = {} ROLES.each do |role, privs| 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/quota.rb b/src/app/models/quota.rb index fc52177..2a9e0f0 100644 --- a/src/app/models/quota.rb +++ b/src/app/models/quota.rb @@ -22,6 +22,28 @@ require 'util/ovirt' class Quota < ActiveRecord::Base belongs_to :pool + validates_presence_of :pool_id + + + validates_numericality_of :total_vcpus, + :greater_than_or_equal_to => 0, + :unless => Proc.new{ |quota| quota.total_vcpus.nil? } + + validates_numericality_of :total_vmemory, + :greater_than_or_equal_to => 0, + :unless => Proc.new{ |quota| quota.total_vmemory.nil? } + + validates_numericality_of :total_vnics, + :greater_than_or_equal_to => 0, + :unless => Proc.new{ |quota| quota.total_vnics.nil? } + + validates_numericality_of :total_storage, + :greater_than_or_equal_to => 0, + :unless => Proc.new{ |quota| quota.total_storage.nil? } + + validates_numericality_of :total_vms, + :greater_than_or_equal_to => 0, + :unless => Proc.new{ |quota| quota.total_vms.nil? } def total_vmemory_in_mb kb_to_mb(total_vmemory) diff --git a/src/app/models/smart_pool_tag.rb b/src/app/models/smart_pool_tag.rb index e135ddf..00bb5a7 100644 --- a/src/app/models/smart_pool_tag.rb +++ b/src/app/models/smart_pool_tag.rb @@ -31,6 +31,11 @@ class SmartPoolTag < ActiveRecord::Base validates_uniqueness_of :smart_pool_id, :scope => [:tagged_id, :tagged_type] + validates_presence_of :tagged_id + + validates_inclusion_of :tagged_type, + :in => %w( Pool StoragePool Host Vm ) + def tagged_type=(sType) super(sType.to_s.classify.constantize.base_class.to_s) end diff --git a/src/app/models/storage_pool.rb b/src/app/models/storage_pool.rb index bab7031..cffb52f 100644 --- a/src/app/models/storage_pool.rb +++ b/src/app/models/storage_pool.rb @@ -39,6 +39,15 @@ class StoragePool < ActiveRecord::Base validates_presence_of :hardware_pool_id + validates_inclusion_of :type, + :in => %w( IscsiStoragePool LvmStoragePool NfsStoragePool ) + + validates_inclustion_of :state, + :in => [ STATE_PENDING_SETUP, STATE_PENDING_DELETION, STATE_AVAILABLE] + + validates_numericality_of :capacity, + :greater_than_or_equal_to => 0 + acts_as_xapian :texts => [ :ip_addr, :target, :export_path, :type ], :terms => [ [ :search_users, 'U', "search_users" ] ], :eager_load => :smart_pools 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?) + 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? + 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 + end diff --git a/src/app/models/usage.rb b/src/app/models/usage.rb index 353e8f4..59b0e48 100644 --- a/src/app/models/usage.rb +++ b/src/app/models/usage.rb @@ -18,4 +18,10 @@ class Usage < ActiveRecord::Base has_and_belongs_to_many :networks, :join_table => 'networks_usages' + + validates_presence_of :label + validates_presence_of :usage + + validates_uniqueness_of :usage + end 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 + + 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" ] ], -- 1.6.0.4
Scott Seago
2008-Dec-09 06:13 UTC
[Ovirt-devel] [PATCH server] missing ovirt wui validations (round 1)
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