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

[Mohammed Morsi]

Just sent out a followup patch addressing these issues, comments inline
below.


David Lutterkort wrote:
> 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.
>   
Done, thanks for the performance tip.


> 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.
>
>   
This is not done as I'm a little concerned about locking the table (what
if the user doesn't submit the form and walks away, will it stay
locked?). I changed the additions to the server such that the
autogenerated port isn't displayed in the form, eg the user is prompted
to set the port or leave it at '0' after which the server will
autogenerate it immediately before saving. Because I'm not employing
locks, I believe the race condition you described is still possible, but
the rails validation / errors framework should take care of that, as
when the user submits the form, the uniqueness constraint will be
checked and fail, resulting in the save operation not completing and an
error message being presented in the wui indicating that the port is
already in use and to select another (or autoassign it).

>> 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
>
>
>   
Both the checking of the status and the open file call are now being
done. Thanks for these tips.

    -Mo