[Ovirt-devel] Re: [PATCH server] allow admin to setup iptables port forwarding on server for a vm's vnc port

[David Lutterkort]

On Wed, 2009-01-28 at 20:16 -0500, Mohammed Morsi wrote:

> diff --git a/src/app/models/vm.rb b/src/app/models/vm.rb
> index bf99e2d..63c9232 100644
> --- a/src/app/models/vm.rb
> +++ b/src/app/models/vm.rb

> @@ -335,6 +344,14 @@ class Vm < ActiveRecord::Base
>      super
>    end
>  
> +  def self.available_forward_vnc_port
> +    i = 5900
> +    until Vm.find(:first,  :conditions => [ "forward_vnc_port = ?", i]).nil?
> +       i += 1
> +    end
> +    return i
> +  end
> +

I like that you're auto-assigning those ports now :) Unfortunately, the
above code is (a) inefficient and (b) will blow up when multiple people
need a vnc port at almost the same time - it is possible that they both
get the same vnc port, and things will blow up when the second one tries
to store the vm to the database (thanks to the unique constraint on
forward_vnc_port - that's vital)

To make this more efficient, you can just suck all the assigned ports
into memory, something like 

  select forward_vnc_port from vms where forward_vnc_port is not null;

and then pick the next available from that list.

To address the race condition, the simplest solution is to take an
exclusive table lock on the vms table (before listing the assigned vnc
ports !) and hold that until the vm is saved in the DB, i.e. the end of
the current transaction.

> diff --git a/src/task-omatic/vnc.rb b/src/task-omatic/vnc.rb
> new file mode 100644
> index 0000000..bc3fd8f
> --- /dev/null
> +++ b/src/task-omatic/vnc.rb
> @@ -0,0 +1,136 @@
> +# 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

Much better :)

> +       system(forward_rule1)
> +       system(forward_rule2)
> +       system(prerouting_rule)
> +       system(postrouting_rule)
> +       system(IP_FORWARD_CMD)

You should at the very least check the exit status of all those system
commands and raise an error if any ofthem exit with nonzero.

BTW, you could save one shell invocation by replacing
'system(IP_FORWARD_CMD)' with

        File::open("/proc/sys/net/ipv4/ip_forward", "w") { |f| f.puts
        "1" }

David