Scott Seago
2008-Nov-06 20:24 UTC
[Ovirt-devel] [PATCH] add 'state' field to storage pools and volumes
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'
--
1.5.5.1
Chris Lalancette
2008-Nov-07 07:27 UTC
[Ovirt-devel] [PATCH] add 'state' field to storage pools and volumes
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. >This all looks reasonable, and is very much inline with what I thought it would be. At least for an initial cut, the states: STATE_PENDING_SETUP = "pending_setup" STATE_PENDING_DELETION = "pending_deletion" STATE_AVAILABLE = "available" should be sufficient to close the races we've thought of so far. We can always add more later. Oh, I'll be pushing my taskomatic LVM code today (with a few of the changes sseago suggested in his review). So then we'll be able the taskomatic side of this change. ACK -- Chris Lalancette
Jason Guiditta
2008-Nov-11 16:25 UTC
[Ovirt-devel] [PATCH] add 'state' field to storage pools and volumes
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