Ovirt devel - Jul 2009 - [PATCH server] remove vm forward vnc and vm host history

since the introduction of ovirt-vnc-proxy and the updating
of ovirt-viewer to user it the forward vnc functionality is
no longer necessary / and incompatable w/ the current system.
---
 src/app/controllers/vm_controller.rb               |    2 -
 src/app/models/host.rb                             |    9 -
 src/app/models/vm.rb                               |   36 ----
 src/app/models/vm_host_history.rb                  |   22 ---
 src/app/views/vm/_form.rhtml                       |    3 -
 src/app/views/vm/show.rhtml                        |    4 -
 src/db-omatic/db_omatic.rb                         |   31 ----
 src/db-omatic/vnc.rb                               |  173 --------------------
 .../040_remove_vm_history_and_forward_vnc.rb       |   65 ++++++++
 src/test/fixtures/vms.yml                          |    2 -
 src/test/unit/vm_test.rb                           |   16 --
 11 files changed, 65 insertions(+), 298 deletions(-)
 delete mode 100644 src/app/models/vm_host_history.rb
 delete mode 100644 src/db-omatic/vnc.rb
 create mode 100644 src/db/migrate/040_remove_vm_history_and_forward_vnc.rb

diff --git a/src/app/controllers/vm_controller.rb
b/src/app/controllers/vm_controller.rb
index 197241d..11b68ed 100644
--- a/src/app/controllers/vm_controller.rb
+++ b/src/app/controllers/vm_controller.rb
@@ -61,7 +61,6 @@ class VmController < ApplicationController
   end
 
   def create
-    params[:vm][:forward_vnc] = params[:forward_vnc]
     alert = svc_create(params[:vm], params[:start_now])
     render :json => { :object => "vm", :success => true,
:alert => alert  }
   end
@@ -75,7 +74,6 @@ class VmController < ApplicationController
   end
 
   def update
-    params[:vm][:forward_vnc] = params[:forward_vnc]
     alert = svc_update(params[:id], params[:vm], params[:start_now],
                        params[:restart_now])
     render :json => { :object => "vm", :success => true,
:alert => alert  }
diff --git a/src/app/models/host.rb b/src/app/models/host.rb
index 588137b..4975205 100644
--- a/src/app/models/host.rb
+++ b/src/app/models/host.rb
@@ -54,15 +54,6 @@ class Host < ActiveRecord::Base
   has_many :smart_pool_tags, :as => :tagged, :dependent => :destroy
   has_many :smart_pools, :through => :smart_pool_tags
 
-  # reverse cronological collection of vm history
-  # each collection item contains vm that was running on host
-  # time started, and time ended (see VmHostHistory)
-  has_many :vm_host_histories,
-           :order => 'time_started DESC',
-           :dependent => :destroy
-
-  alias history vm_host_histories
-
   acts_as_xapian :texts => [ :hostname, :uuid, :hypervisor_type, :arch ],
                  :values => [ [ :created_at, 0, "created_at",
:date ],
                               [ :updated_at, 1, "updated_at", :date ]
],
diff --git a/src/app/models/vm.rb b/src/app/models/vm.rb
index 6d0f864..14ad14c 100644
--- a/src/app/models/vm.rb
+++ b/src/app/models/vm.rb
@@ -34,14 +34,6 @@ class Vm < ActiveRecord::Base
   has_many :smart_pool_tags, :as => :tagged, :dependent => :destroy
   has_many :smart_pools, :through => :smart_pool_tags
 
-  # reverse cronological collection of vm history
-  # each collection item contains host vm was running on,
-  # time started, and time ended (see VmHostHistory)
-  has_many :vm_host_histories,
-           :order => 'time_started DESC',
-           :dependent => :destroy
-
-
   has_many :vm_state_change_events,
            :order => 'created_at' do
     def previous_state_with_type(state_type)
@@ -59,22 +51,6 @@ class Vm < ActiveRecord::Base
   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})
 
-  FORWARD_VNC_PORT_START = 5901
-
-  validates_numericality_of :forward_vnc_port,
-    :message => 'must be >= ' + FORWARD_VNC_PORT_START.to_s,
-    :greater_than_or_equal_to => FORWARD_VNC_PORT_START,
-    :if => Proc.new { |vm| vm.forward_vnc &&
-                           !vm.forward_vnc_port.nil? &&
-                           vm.forward_vnc_port != 0 }
-
-  validates_uniqueness_of :forward_vnc_port,
-    :message => "is already in use",
-    :if => Proc.new { |vm| vm.forward_vnc &&
-                           !vm.forward_vnc_port.nil? &&
-                           vm.forward_vnc_port != 0 }
-
-
   validates_numericality_of :needs_restart,
      :greater_than_or_equal_to => 0,
      :less_than_or_equal_to => 1,
@@ -408,18 +384,6 @@ class Vm < ActiveRecord::Base
     super
   end
 
-  # find the first available vnc port
-  def self.available_forward_vnc_port
-    i = FORWARD_VNC_PORT_START
-    Vm.find(:all,
-            :conditions => "forward_vnc_port is not NULL",
-            :order => 'forward_vnc_port ASC' ).each{ |vm|
-               break if vm.forward_vnc_port > i
-               i = i + 1
-            }
-    return i
-  end
-
   def self.calc_uptime
     "vms.*, case when state='running' then
        (cast(total_uptime || ' sec' as interval) +
diff --git a/src/app/models/vm_host_history.rb
b/src/app/models/vm_host_history.rb
deleted file mode 100644
index bd61ddc..0000000
--- a/src/app/models/vm_host_history.rb
+++ /dev/null
@@ -1,22 +0,0 @@
-# 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.
-
-class VmHostHistory < ActiveRecord::Base
-   belongs_to :vm
-   belongs_to :host
-end
diff --git a/src/app/views/vm/_form.rhtml b/src/app/views/vm/_form.rhtml
index 034c3df..a08f2a4 100644
--- a/src/app/views/vm/_form.rhtml
+++ b/src/app/views/vm/_form.rhtml
@@ -51,9 +51,6 @@
     <div class="clear_row"></div>
     <div class="clear_row"></div>
 
-    <%= check_box_tag_with_label "Forward vm's vnc port
locally", "forward_vnc", 1, @vm.forward_vnc %>
-    <div class="clear_row"></div>
-
    <%= check_box_tag_with_label "Start VM Now? (pending current
resource availability)", "start_now", nil if create or @vm.state
== Vm::STATE_STOPPED %>
    <%= check_box_tag_with_label "Restart VM Now? (pending current
resource availability)", "restart_now", nil if @vm.state ==
Vm::STATE_RUNNING %>
 
diff --git a/src/app/views/vm/show.rhtml b/src/app/views/vm/show.rhtml
index ffe5055..0bdf1e9 100644
--- a/src/app/views/vm/show.rhtml
+++ b/src/app/views/vm/show.rhtml
@@ -101,7 +101,6 @@
     <div id="vms_selection_id"
style="display:none"><%= @vm.id %></div>
     <div class="selection_key">
         Uuid:<br/>
-        <%= @vm.forward_vnc ? "VNC uri:<br/>" : ""
%>
 	Num vcpus allocated:<br/>
 	Num vcpus used:<br/>
 	Memory allocated:<br/>
@@ -115,9 +114,6 @@
     </div>
     <div class="selection_value">
        <%=h @vm.uuid %><br/>
-       <%= url = request.url
-           url = request.url[0..(url.index('/', 8) - 1)] +
":" + @vm.forward_vnc_port.to_s
-           @vm.forward_vnc ? (url + "<br/>") : ""
%>
        <%=h @vm.num_vcpus_allocated %><br/>
        <%=h @vm.num_vcpus_used %><br/>
        <%=h @vm.memory_allocated_in_mb %> MB<br/>
diff --git a/src/db-omatic/db_omatic.rb b/src/db-omatic/db_omatic.rb
index 155ff5e..b469695 100755
--- a/src/db-omatic/db_omatic.rb
+++ b/src/db-omatic/db_omatic.rb
@@ -10,7 +10,6 @@ require 'dutils'
 require 'daemons'
 require 'optparse'
 require 'logger'
-require 'vnc'
 
 include Daemonize
 
@@ -159,36 +158,6 @@ class DbOmatic < Qpid::Qmf::Console
             end
         end
 
-        begin
-          # find open vm host history for this vm,
-          history = VmHostHistory.find(:first, :conditions => ["vm_id =
? AND time_ended is NULL", vm.id])
-
-          if state == Vm::STATE_RUNNING
-             if history.nil?
-               history = VmHostHistory.new
-               history.vm = vm
-               history.host = vm.host
-               history.vnc_port = vm.vnc_port
-               history.state = state
-               history.time_started = Time.now
-               history.save!
-             end
-
-             VmVnc.forward(vm)
-          elsif state != Vm::STATE_PENDING
-             VmVnc.close(vm, history)
-
-             unless history.nil? # throw an exception if this fails?
-               history.time_ended = Time.now
-               history.state = state
-               history.save!
-             end
-          end
-
-        rescue Exception => e # just log any errors here
-            @logger.error "Error with VM #{domain['name']}
operation: " + e
-        end
-
         @logger.info "Updating VM #{domain['name']} to state
#{state}"
 
         if state == Vm::STATE_STOPPED
diff --git a/src/db-omatic/vnc.rb b/src/db-omatic/vnc.rb
deleted file mode 100644
index d826841..0000000
--- a/src/db-omatic/vnc.rb
+++ /dev/null
@@ -1,173 +0,0 @@
-# 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.
-
-# provides static 'forward' and 'close' methods to forward a
specified vm's vnc connections
-class VmVnc
-
-  private
-
-    # TODO no ruby/libiptc wrapper exists, when
-    # it does replace iptables command w/ calls to it
-    IPTABLES_CMD='/sbin/iptables '
-
-    VNC_DEBUG = false
-
-    def self.debug(msg)
-      puts "\n" + msg + "\n" if VNC_DEBUG
-    end
-
-    def self.find_host_ip(hostname)
-      # FIXME
-      addrinfo = Socket::getaddrinfo(hostname, nil)
-      unless addrinfo.size > 0
-        raise "Could not retreive address for " + hostname
-      end
-      result = addrinfo[0][3] # return ip address of first entry
-      debug( "vm host hostname  resolved to " + result.to_s )
-      return result
-    end
-
-    def self.port_open?(port)
-      cmd=IPTABLES_CMD + ' -t nat -nL '
-      debug("vncPortOpen? iptables command: " + cmd)
-
-      `#{cmd}`.each_line do |l|
-          return true if l =~ /.*#{port}.*/
-      end
-      return false
-    end
-
-    def self.allocate_forward_vnc_port(vm)
-       Vm.transaction do
-         ActiveRecord::Base.connection.execute('LOCK TABLE vms')
-         vm.forward_vnc_port = Vm.available_forward_vnc_port
-         debug("Allocating forward vnc port " +
vm.forward_vnc_port.to_s)
-         vm.save!
-       end
-    end
-
-    def self.deallocate_forward_vnc_port(vm)
-       debug("Deallocating forward vnc port " +
vm.forward_vnc_port.to_s)
-       vm.forward_vnc_port = nil
-       vm.save!
-    end
-
-    def self.get_forward_rules(vm, history_record = nil)
-      if history_record.nil?
-        ip = find_host_ip(vm.host.hostname)
-        vnc_port = vm.vnc_port.to_s
-      else
-        ip = find_host_ip(history_record.host.hostname)
-        vnc_port = history_record.vnc_port.to_s
-      end
-
-      return " -d " + ip + " -p tcp --dport " + vnc_port +
" -j ACCEPT",
-             " -s " + ip + " -p tcp --sport " + vnc_port +
" -j ACCEPT"
-    end
-
-    def self.get_nat_rules(vm, history_record = nil)
-      if history_record.nil?
-        ip = find_host_ip(vm.host.hostname)
-        vnc_port = vm.vnc_port.to_s
-      else
-        ip = find_host_ip(history_record.host.hostname)
-        vnc_port = history_record.vnc_port.to_s
-      end
-
-      server,port = get_srv('ovirt', 'tcp')
-      local_ip = find_host_ip(server)
-      return " -p tcp --dport " + vm.forward_vnc_port.to_s + "
-j DNAT --to " + ip + ":" + vnc_port,
-             " -d " + ip + " -p tcp --dport " + vnc_port +
" -j SNAT --to " + local_ip
-    end
-
-    def self.run_command(cmd)
-      debug("Running command " + cmd)
-      status = system(cmd)
-      raise 'Command terminated with error code ' + $?.to_s unless
status
-    end
-
-  public
-
-    def self.forward(vm)
-       return unless vm.forward_vnc
-
-       allocate_forward_vnc_port(vm)
-       if port_open?(vm.forward_vnc_port)
-         deallocate_forward_vnc_port(vm)
-         raise "Port already open " + vm.forward_vnc_port.to_s
-       end
-
-       forward_rule1, forward_rule2 = get_forward_rules(vm)
-       forward_rule1 = IPTABLES_CMD + " -A FORWARD " + forward_rule1
-       forward_rule2 = IPTABLES_CMD + " -A FORWARD " + forward_rule2
-
-       prerouting_rule, postrouting_rule = get_nat_rules(vm)
-       prerouting_rule = IPTABLES_CMD + " -t nat -A PREROUTING " +
prerouting_rule
-       postrouting_rule = IPTABLES_CMD + " -t nat -A POSTROUTING " +
postrouting_rule
-
-       debug(" open\n forward rule 1: "     + forward_rule1 +
-              "\n forward_rule 2: "   + forward_rule2 +
-              "\n prerouting rule: "  + prerouting_rule +
-              "\n postrouting rule: " + postrouting_rule)
-
-
-       File::open("/proc/sys/net/ipv4/ip_forward", "w") {
|f| f.puts "1" }
-       run_command(forward_rule1)
-       run_command(forward_rule2)
-       run_command(prerouting_rule)
-       run_command(postrouting_rule)
-    end
-
-    def self.close(vm, history_record = nil)
-       return unless vm.forward_vnc
-
-       unless port_open?(vm.forward_vnc_port)
-         raise "Port not open " + vm.forward_vnc_port.to_s
-       end
-
-       forward_rule1, forward_rule2 = get_forward_rules(vm, history_record)
-       forward_rule1 = IPTABLES_CMD + " -D FORWARD " + forward_rule1
-       forward_rule2 = IPTABLES_CMD + " -D FORWARD " + forward_rule2
-
-       prerouting_rule, postrouting_rule = get_nat_rules(vm, history_record)
-       prerouting_rule = IPTABLES_CMD + " -t nat -D PREROUTING " +
prerouting_rule
-       postrouting_rule = IPTABLES_CMD + " -t nat -D POSTROUTING " +
postrouting_rule
-
-       debug(" close\n forward rule 1: "     + forward_rule1 +
-              "\n forward_rule 2: "   + forward_rule2 +
-              "\n prerouting rule: "  + prerouting_rule +
-              "\n postrouting rule: " + postrouting_rule)
-
-       run_command(forward_rule1)
-       run_command(forward_rule2)
-       run_command(prerouting_rule)
-       run_command(postrouting_rule)
-
-
-       deallocate_forward_vnc_port(vm)
-    end
-
-    # should be used in cases which
-    # closing iptables ports is not needed
-    # (ex. on server startup)
-    def self.deallocate_all
-        Vm.find(:all).each{ |vm|
-          deallocate_forward_vnc_port(vm)
-        }
-    end
-end
diff --git a/src/db/migrate/040_remove_vm_history_and_forward_vnc.rb
b/src/db/migrate/040_remove_vm_history_and_forward_vnc.rb
new file mode 100644
index 0000000..b815f18
--- /dev/null
+++ b/src/db/migrate/040_remove_vm_history_and_forward_vnc.rb
@@ -0,0 +1,65 @@
+# Copyright (C) 2008 Red Hat, Inc.
+# Written by Mohammed Morsi
+#
+# 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.
+
+# reverses 034_add_vm_vnc migration and 036_vm_host_history
+# pretty much copied the self.up from 034 / 036 into self.down
+# and self.down from 034 / 036 into self.up
+class RemoveVmHistoryAndForwardVnc < ActiveRecord::Migration
+  def self.up
+   # 034 self.down
+   remove_column :vms, :forward_vnc
+   remove_column :vms, :forward_vnc_port
+
+   # 036 self.down
+   drop_table :vm_host_histories
+  end
+
+  def self.down
+   # 034 self.up
+   add_column :vms, :forward_vnc, :bool, :default => false
+   add_column :vms, :forward_vnc_port, :int, :default => 0, :unique =>
true
+
+   # 036 self.up
+   # this table gets populated in db-omatic
+    create_table :vm_host_histories do |t|
+       # vm / host association
+       t.integer :vm_id
+       t.integer :host_id
+
+       # records operating info of vm
+       #  (most likey we will want to add a
+       #   slew of more info here or in a future
+       #   migration)
+       t.integer :vnc_port
+       t.string  :state
+
+       # start / end timestamps
+       t.timestamp :time_started
+       t.timestamp :time_ended
+    end
+
+    execute "alter table vm_host_histories add constraint
+             fk_vm_host_histories_vms foreign key (vm_id) references
vms(id)"
+
+    execute "alter table vm_host_histories add constraint
+             fk_vm_host_histories_hosts foreign key (host_id) references
+             hosts(id)"
+
+  end
+end
+
diff --git a/src/test/fixtures/vms.yml b/src/test/fixtures/vms.yml
index b2711b2..0d2caa3 100644
--- a/src/test/fixtures/vms.yml
+++ b/src/test/fixtures/vms.yml
@@ -11,8 +11,6 @@ production_httpd_vm:
   boot_device: hd
   host: prod_corp_com
   vm_resource_pool: corp_com_production_vmpool
-  forward_vnc: true
-  forward_vnc_port: 5901
 production_mysqld_vm:
   uuid: 89e62d32-04d9-4351-b573-b1a253397296
   description: production mysqld appliance
diff --git a/src/test/unit/vm_test.rb b/src/test/unit/vm_test.rb
index a28f183..d7b9d40 100644
--- a/src/test/unit/vm_test.rb
+++ b/src/test/unit/vm_test.rb
@@ -96,22 +96,6 @@ class VmTest < ActiveSupport::TestCase
        flunk 'Vm must specify valid state' if @vm.valid?
   end
 
-  # ensure duplicate forward_vnc_ports cannot exist
-  def test_invalid_without_unique_forward_vnc_port
-     vm = vms(:production_mysqld_vm)
-     vm.forward_vnc = true
-     vm.forward_vnc_port = 1234 # duplicate
-     assert !vm.valid?, "forward vnc port must be unique"
-  end
-
-  # ensure bad forward_vnc_ports cannot exist
-  def test_invalid_without_bad_forward_vnc_port
-     vm = vms(:production_mysqld_vm)
-     vm.forward_vnc = true
-     vm.forward_vnc_port = 1 # too small
-     assert !vm.valid?, "forward vnc port must be >= 5900"
-  end
-
   # Ensures that, if the VM does not contain the Cobbler prefix, that it
   # does not claim to be a Cobbler VM.
   #
-- 
1.6.0.6