[Ovirt-devel] [PATCH] add 'state' field to storage pools and volumes
Jason Guiditta
jguiditt at redhat.com
Tue Nov 11 16:25:25 UTC 2008
On Thu, 2008-11-06 at 20:24 +0000, Scott Seago wrote:
> I've added a state field to StoragePool and StorageVolume. When initially created, pools and volumes are in the pending_setup state -- once taskomatic is done they're 'available'. Upon deletion, they go into pending_deletion until taskomatic is done.
>
> Taskomatic changes to take this into account have not yet been included, since clalance's LVM patch hasn't been pushed.
>
> Signed-off-by: Scott Seago <sseago at redhat.com>
> ---
> src/app/controllers/storage_controller.rb | 4 +-
> src/app/controllers/vm_controller.rb | 2 +-
> src/app/models/hardware_pool.rb | 17 +++++++++--
> src/app/models/storage_pool.rb | 33 ++++++++++++++++++-----
> src/app/models/storage_volume.rb | 33 ++++++++++++++++++-----
> src/app/views/storage/show.rhtml | 2 +
> src/app/views/storage/show_volume.rhtml | 2 +
> src/db/migrate/029_storage_state_field.rb | 42 +++++++++++++++++++++++++++++
> src/test/fixtures/storage_pools.yml | 10 +++++++
> src/test/fixtures/storage_volumes.yml | 6 ++++
> 10 files changed, 131 insertions(+), 20 deletions(-)
> create mode 100644 src/db/migrate/029_storage_state_field.rb
>
> diff --git a/src/app/controllers/storage_controller.rb b/src/app/controllers/storage_controller.rb
> index 75058cd..e3e5522 100644
> --- a/src/app/controllers/storage_controller.rb
> +++ b/src/app/controllers/storage_controller.rb
> @@ -442,9 +442,9 @@ class StorageController < ApplicationController
> vm_list = volume.vms.collect {|vm| vm.description}.join(", ")
> ["Storage Volume #{name} must be unattached from VMs (#{vm_list}) before deleting it.",
> false]
> - #FIXME: create delete volume task. include metadata in task
> else
> - #FIXME: need to set volume to 'unavailable' state once we have states
> + volume.state=StorageVolume::STATE_PENDING_DELETION
> + volume.save!
> @task = StorageVolumeTask.new({ :user => @user,
> :task_target => volume,
> :action => StorageVolumeTask::ACTION_DELETE_VOLUME,
> diff --git a/src/app/controllers/vm_controller.rb b/src/app/controllers/vm_controller.rb
> index ee956da..ff74a37 100644
> --- a/src/app/controllers/vm_controller.rb
> +++ b/src/app/controllers/vm_controller.rb
> @@ -81,7 +81,7 @@ class VmController < ApplicationController
> end
>
> def edit
> - @storage_tree = @vm.vm_resource_pool.get_hardware_pool.storage_tree(@vm).to_json
> + @storage_tree = @vm.vm_resource_pool.get_hardware_pool.storage_tree(:vm_to_include => @vm).to_json
> render :layout => 'popup'
> end
>
> diff --git a/src/app/models/hardware_pool.rb b/src/app/models/hardware_pool.rb
> index 6780329..d39d8e7 100644
> --- a/src/app/models/hardware_pool.rb
> +++ b/src/app/models/hardware_pool.rb
> @@ -101,10 +101,21 @@ class HardwarePool < Pool
> return {:total => total, :labels => labels}
> end
>
> - def storage_tree(vm_to_include=nil)
> + # params accepted:
> + # :vm_to_include - if specified, storage used by this VM is included in the tree
> + # :filter_unavailable - if true, don't include Storage not currently available
> + # :include_used - include all storage pools/volumes, even those in use
> + def storage_tree(params = {})
> + vm_to_include=params.fetch(:vm_to_include, nil)
> + filter_unavailable = params.fetch(:filter_unavailable, true)
> + include_used = params.fetch(:include_used, false)
> + conditions = "type != 'LvmStoragePool'"
> + if filter_unavailable
> + conditions = "(#{conditions}) and (storage_pools.state = '#{StoragePool::STATE_AVAILABLE}')"
> + end
> storage_pools.find(:all,
> - :conditions => "type != 'LvmStoragePool'").collect do |pool|
> - pool.storage_tree_element(vm_to_include)
> + :conditions => conditions).collect do |pool|
> + pool.storage_tree_element(params)
> end
> end
> end
> diff --git a/src/app/models/storage_pool.rb b/src/app/models/storage_pool.rb
> index 9c550b8..9c80f54 100644
> --- a/src/app/models/storage_pool.rb
> +++ b/src/app/models/storage_pool.rb
> @@ -50,7 +50,12 @@ class StoragePool < ActiveRecord::Base
> LVM => "Lvm" }
> STORAGE_TYPE_PICKLIST = STORAGE_TYPES.keys - [LVM]
>
> - def self.factory(type, params = nil)
> + STATE_PENDING_SETUP = "pending_setup"
> + STATE_PENDING_DELETION = "pending_deletion"
> + STATE_AVAILABLE = "available"
> +
> + def self.factory(type, params = {})
> + params[:state] = STATE_PENDING_SETUP unless params[:state]
> case type
> when ISCSI
> return IscsiStoragePool.new(params)
> @@ -82,7 +87,10 @@ class StoragePool < ActiveRecord::Base
> false
> end
>
> - def storage_tree_element(vm_to_include=nil)
> + def storage_tree_element(params = {})
> + vm_to_include=params.fetch(:vm_to_include, nil)
> + filter_unavailable = params.fetch(:filter_unavailable, true)
> + include_used = params.fetch(:include_used, false)
> return_hash = { :id => id,
> :type => self[:type],
> :text => display_name,
> @@ -90,14 +98,25 @@ class StoragePool < ActiveRecord::Base
> :available => false,
> :create_volume => user_subdividable,
> :selected => false}
> - condition = "vms.id is null"
> - if (vm_to_include and vm_to_include.id)
> - condition +=" or vms.id=#{vm_to_include.id}"
> + conditions = nil
> + unless include_used
> + conditions = "vms.id is null"
> + if (vm_to_include and vm_to_include.id)
> + conditions +=" or vms.id=#{vm_to_include.id}"
> + end
> + end
> + if filter_unavailable
> + availability_conditions = "storage_volumes.state = '#{StoragePool::STATE_AVAILABLE}'"
> + if conditions.nil?
> + conditions = availability_conditions
> + else
> + conditions ="(#{conditions}) and (#{availability_conditions})"
> + end
> end
> return_hash[:children] = storage_volumes.find(:all,
> :include => :vms,
> - :conditions => condition).collect do |volume|
> - volume.storage_tree_element(vm_to_include)
> + :conditions => conditions).collect do |volume|
> + volume.storage_tree_element(params)
> end
> return_hash
> end
> diff --git a/src/app/models/storage_volume.rb b/src/app/models/storage_volume.rb
> index 6f14c4d..56cdcef 100644
> --- a/src/app/models/storage_volume.rb
> +++ b/src/app/models/storage_volume.rb
> @@ -32,7 +32,12 @@ class StorageVolume < ActiveRecord::Base
> end
> end
>
> - def self.factory(type, params = nil)
> + STATE_PENDING_SETUP = "pending_setup"
> + STATE_PENDING_DELETION = "pending_deletion"
> + STATE_AVAILABLE = "available"
> +
> + def self.factory(type, params = {})
> + params[:state] = STATE_PENDING_SETUP unless params[:state]
> case type
> when StoragePool::ISCSI
> return IscsiStorageVolume.new(params)
> @@ -75,7 +80,10 @@ class StorageVolume < ActiveRecord::Base
> return false
> end
>
> - def storage_tree_element(vm_to_include=nil)
> + def storage_tree_element(params = {})
> + vm_to_include=params.fetch(:vm_to_include, nil)
> + filter_unavailable = params.fetch(:filter_unavailable, true)
> + include_used = params.fetch(:include_used, false)
> vm_ids = vms.collect {|vm| vm.id}
> return_hash = { :id => id,
> :type => self[:type],
> @@ -92,14 +100,25 @@ class StorageVolume < ActiveRecord::Base
> if return_hash[:available]
> return_hash[:available] = lvm_storage_pool.storage_volumes.full_vm_list.empty?
> end
> - condition = "vms.id is null"
> - if (vm_to_include and vm_to_include.id)
> - condition +=" or vms.id=#{vm_to_include.id}"
> + conditions = nil
> + unless include_used
> + conditions = "vms.id is null"
> + if (vm_to_include and vm_to_include.id)
> + conditions +=" or vms.id=#{vm_to_include.id}"
> + end
> + end
> + if filter_unavailable
> + availability_conditions = "storage_volumes.state = '#{StoragePool::STATE_AVAILABLE}'"
> + if conditions.nil?
> + conditions = availability_conditions
> + else
> + conditions ="(#{conditions}) and (#{availability_conditions})"
> + end
> end
> return_hash[:children] = lvm_storage_pool.storage_volumes.find(:all,
> :include => :vms,
> - :conditions => condition).collect do |volume|
> - volume.storage_tree_element(vm_to_include)
> + :conditions => conditions).collect do |volume|
> + volume.storage_tree_element(params)
> end
> else
> return_hash[:children] = []
> diff --git a/src/app/views/storage/show.rhtml b/src/app/views/storage/show.rhtml
> index a84dc62..123cf20 100644
> --- a/src/app/views/storage/show.rhtml
> +++ b/src/app/views/storage/show.rhtml
> @@ -26,6 +26,7 @@
> Export path:<br/>
> <% end %>
> Type:<br/>
> + State:<br/>
> </div>
> <div class="selection_value">
> <%=h @storage_pool.ip_addr %><br/>
> @@ -36,6 +37,7 @@
> <%=h @storage_pool.export_path %><br/>
> <% end %>
> <%=h @storage_pool.get_type_label %><br/>
> + <%=h @storage_pool.state %><br/>
> </div>
> <%- content_for :right do -%>
> <div class="selection_right_title">Volumes</div>
> diff --git a/src/app/views/storage/show_volume.rhtml b/src/app/views/storage/show_volume.rhtml
> index e39c10e..c1cb66a 100644
> --- a/src/app/views/storage/show_volume.rhtml
> +++ b/src/app/views/storage/show_volume.rhtml
> @@ -28,6 +28,7 @@
> Export path:<br/>
> <% end %>
> Type:<br/>
> + State:<br/>
> Path:<br/>
> <% if @storage_volume[:type] == "IscsiStorageVolume" %>
> LUN:<br/>
> @@ -45,6 +46,7 @@
> <%=h @storage_volume.storage_pool.export_path %><br/>
> <% end %>
> <%=h @storage_volume.storage_pool.get_type_label %><br/>
> + <%=h @storage_volume.state %><br/>
> <%=h @storage_volume.path %><br/>
> <% if @storage_volume[:type] == "IscsiStorageVolume" %>
> <%=h @storage_volume.lun %><br/>
> diff --git a/src/db/migrate/029_storage_state_field.rb b/src/db/migrate/029_storage_state_field.rb
> new file mode 100644
> index 0000000..d8f3ab1
> --- /dev/null
> +++ b/src/db/migrate/029_storage_state_field.rb
> @@ -0,0 +1,42 @@
> +#
> +# Copyright (C) 2008 Red Hat, Inc.
> +# Written by Scott Seago <sseago 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.
> +
> +class StorageStateField < ActiveRecord::Migration
> + def self.up
> + add_column :storage_pools, :state, :string
> + add_column :storage_volumes, :state, :string
> + begin
> + StoragePool.transaction do
> + StoragePool.find(:all).each do |pool|
> + pool.state = 'available'
> + pool.save!
> + end
> + StorageVolume.find(:all).each do |volume|
> + volume.state = 'available'
> + volume.save!
> + end
> + end
> + end
> + end
> +
> + def self.down
> + remove_column :storage_pools, :state
> + remove_column :storage_volumes, :state
> + end
> +end
> diff --git a/src/test/fixtures/storage_pools.yml b/src/test/fixtures/storage_pools.yml
> index ac042a0..d52bf51 100644
> --- a/src/test/fixtures/storage_pools.yml
> +++ b/src/test/fixtures/storage_pools.yml
> @@ -5,18 +5,21 @@ one:
> type: 'IscsiStoragePool'
> port: 53
> target: 'footarget'
> + state: 'available'
> two:
> id: 2
> hardware_pool_id: 4
> type: 'NfsStoragePool'
> ip_addr: '127.0.0.1'
> export_path: '/tmp/foopath'
> + state: 'available'
> three:
> id: 3
> hardware_pool_id: 4
> type: 'NfsStoragePool'
> ip_addr: '192.168.50.1'
> export_path: '/tmp/barpath'
> + state: 'available'
> four:
> id: 4
> hardware_pool_id: 9
> @@ -25,24 +28,28 @@ four:
> ip_addr: '192.168.50.1'
> target: 'rubarb'
> hardware_pool_id: 1
> + state: 'available'
> five:
> id: 5
> hardware_pool_id: 9
> type: 'NfsStoragePool'
> ip_addr: '192.168.50.2'
> export_path: '/tmp/thepath'
> + state: 'available'
> six:
> id: 6
> hardware_pool_id: 9
> type: 'NfsStoragePool'
> ip_addr: '192.168.50.3'
> export_path: '/tmp/anotherpath'
> + state: 'available'
> seven:
> id: 7
> hardware_pool_id: 9
> type: 'NfsStoragePool'
> ip_addr: '192.168.50.4'
> export_path: '/tmp/apath'
> + state: 'available'
> eight:
> id: 8
> hardware_pool_id: 9
> @@ -50,12 +57,14 @@ eight:
> type: 'IscsiStoragePool'
> port: 531
> target: 'bartarget'
> + state: 'available'
> nine:
> id: 9
> hardware_pool_id: 9
> type: 'NfsStoragePool'
> ip_addr: '1.2.3.4'
> export_path: '/tmp/somepath'
> + state: 'available'
> ten:
> id: 10
> hardware_pool_id: 9
> @@ -63,3 +72,4 @@ ten:
> ip_addr: '192.168.50.3'
> port: 539
> target: 'stayontarget'
> + state: 'available'
> diff --git a/src/test/fixtures/storage_volumes.yml b/src/test/fixtures/storage_volumes.yml
> index 1c49ace..4761d95 100644
> --- a/src/test/fixtures/storage_volumes.yml
> +++ b/src/test/fixtures/storage_volumes.yml
> @@ -6,6 +6,7 @@ one:
> path: '/tmp/foobar'
> storage_pool_id: 1
> type: 'IscsiStorageVolume'
> + state: 'available'
> two:
> id: 2
> size: '20485760'
> @@ -13,6 +14,7 @@ two:
> storage_pool_id: 2
> type: 'NfsStorageVolume'
> filename: 'secretstuff'
> + state: 'available'
> three:
> id: 3
> lun: 'abcd'
> @@ -20,21 +22,25 @@ three:
> path:
> storage_pool_id: 1
> type: 'IscsiStorageVolume'
> + state: 'available'
> four:
> id: 4
> lun: '49723e4d-32e6-469e-8019-913a3dc1bb5d'
> size: '01223542'
> storage_pool_id: 4
> type: 'IscsiStorageVolume'
> + state: 'available'
> five:
> id: 5
> size: '42424242'
> storage_pool_id: 2
> type: 'NfsStorageVolume'
> filename: 'foobar'
> + state: 'available'
> six:
> id: 6
> size: '007007'
> storage_pool_id: 6
> type: 'NfsStorageVolume'
> filename: 'barfoo'
> + state: 'available'
ACK, with, as usual, a little feedback. I tested this by manually
changing the state of storage volumes and pools. They updated
appropriately in both the detail areas (showing correct state) and the
storage tree for new vm creation (only appearing in the list when
state=available).
Now for the feedback. I am currently reworking to fixtures to fit rails
2.1 standards/features, so this could be before or after I finish that,
but I think we should really try to start writing unit tests for things
like this. There are some fairly complicated bits that could be pretty
easily misunderstood and broken by someone else trying to add
functionality to this code. Unit tests on each of these methods
asserting what should come back would easily allow another developer to
see quickly if they unintentionally broke something. Like I said, I am
fine with waiting on this one until my upcoming changes are in, but I
would like to see us get in the habit of writing more tests, especially
since there are so many moving pieces and possible points of failure.
-j
More information about the ovirt-devel
mailing list