Ovirt devel - Mar 2009 - [PATCH server] forward vnc bugfix, move code to forward vnc connections to dbomatic

since a vm may be shutdown by means other than taskomatic, the code
to open / close forwarded vnc connections must be in dbomatic to
catch all cases. relies on patch to introduce vm / host history since
by the time a shutdown vm arrives in dbomatic, host has already been
disassociated
---
 src/db-omatic/db_omatic.rb    |   11 +++
 src/db-omatic/vnc.rb          |  173 +++++++++++++++++++++++++++++++++++++++++
 src/task-omatic/taskomatic.rb |    5 -
 src/task-omatic/vnc.rb        |  151 -----------------------------------
 4 files changed, 184 insertions(+), 156 deletions(-)
 create mode 100644 src/db-omatic/vnc.rb
 delete mode 100644 src/task-omatic/vnc.rb

diff --git a/src/db-omatic/db_omatic.rb b/src/db-omatic/db_omatic.rb
index c31577c..16a0cbf 100755
--- a/src/db-omatic/db_omatic.rb
+++ b/src/db-omatic/db_omatic.rb
@@ -1,6 +1,7 @@
 #!/usr/bin/ruby
 
 $: << File.join(File.dirname(__FILE__), "../dutils")
+$: << File.join(File.dirname(__FILE__), ".")
 
 require "rubygems"
 require "qpid"
@@ -9,6 +10,7 @@ require 'dutils'
 require 'daemons'
 require 'optparse'
 require 'logger'
+require 'vnc'
 
 include Daemonize
 
@@ -156,7 +158,10 @@ class DbOmatic < Qpid::Qmf::Console
                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
@@ -379,6 +384,12 @@ class DbOmatic < Qpid::Qmf::Console
             host.save
         end
 
+        begin
+           VmVnc.deallocate_all
+         rescue Exception => e # just log any errors here
+            @logger.error "Error with closing all VM VNCs operation:
" + e
+         end
+
         db_vm = Vm.find(:all)
         db_vm.each do |vm|
             @logger.info "Marking vm #{vm.description} as stopped."
diff --git a/src/db-omatic/vnc.rb b/src/db-omatic/vnc.rb
new file mode 100644
index 0000000..7506ba8
--- /dev/null
+++ b/src/db-omatic/vnc.rb
@@ -0,0 +1,173 @@
+# 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/task-omatic/taskomatic.rb b/src/task-omatic/taskomatic.rb
index 40d494a..7e3540c 100755
--- a/src/task-omatic/taskomatic.rb
+++ b/src/task-omatic/taskomatic.rb
@@ -33,7 +33,6 @@ include Daemonize
 
 require 'task_vm'
 require 'task_storage'
-require 'vnc'
 
 # This sad and pathetic readjustment to ruby logger class is
 # required to fix the formatting because rails does the same
@@ -283,8 +282,6 @@ class TaskOmatic
       raise "Error destroying VM: #{result.text}" unless
result.status == 0
     end
 
-    VmVnc.close(db_vm)
-
     # undefine can fail, for instance, if we live migrated from A -> B, and
     # then we are shutting down the VM on B (because it only has
"transient"
     # XML).  Therefore, just ignore undefine errors so we do the rest
@@ -381,8 +378,6 @@ class TaskOmatic
     db_vm.host_id = db_host.id
     db_vm.save!
 
-    VmVnc.forward(db_vm)
-
     # We write the new state here even though dbomatic will set it soon anyway.
     # This is just to let the UI know that it's good to go right away and
really
     # dbomatic will just write the same thing over top of it soon enough.
diff --git a/src/task-omatic/vnc.rb b/src/task-omatic/vnc.rb
deleted file mode 100644
index b0a800a..0000000
--- a/src/task-omatic/vnc.rb
+++ /dev/null
@@ -1,151 +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)
-      ip = find_host_ip(vm.host.hostname)
-      return " -d " + ip + " -p tcp --dport " +
vm.vnc_port.to_s + " -j ACCEPT",
-             " -s " + ip + " -p tcp --sport " +
vm.vnc_port.to_s + " -j ACCEPT"
-    end
-
-    def self.get_nat_rules(vm)
-      ip = find_host_ip(vm.host.hostname)
-
-      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 + ":" + vm.vnc_port.to_s,
-             " -d " + ip + " -p tcp --dport " +
vm.vnc_port.to_s + " -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)
-       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)
-       forward_rule1 = IPTABLES_CMD + " -D FORWARD " + forward_rule1
-       forward_rule2 = IPTABLES_CMD + " -D FORWARD " + forward_rule2
-
-       prerouting_rule, postrouting_rule = get_nat_rules(vm)
-       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
-end
-- 
1.6.0.6

On Tue, 10 Mar 2009 19:15:20 -0400
Mohammed Morsi <mmorsi at redhat.com> wrote:
> since a vm may be shutdown by means other than taskomatic, the code
> to open / close forwarded vnc connections must be in dbomatic to
> catch all cases. relies on patch to introduce vm / host history since
> by the time a shutdown vm arrives in dbomatic, host has already been
> disassociated
ACK.  This works very nicely :)  Oh, there are whitespace errors in it
however.. should clean those up :)

    

    Ian