[Ovirt-devel] [PATCH server] Fixes to the managed_node_configuration code.

[Darryl L. Pierce]

On Wed, Apr 01, 2009 at 07:51:21PM -0400, Mohammed Morsi wrote:
> NACK, still a few issues
> 
> Darryl L. Pierce wrote:
> > Refactored how interfaces, bonded interfaces and bridges are defined.
> >
> > When returning a configuration for the node, a bridge is first created,
> > using the networking definition for the device. The network interface is
> > then bridged over that device.
> >
> > When a bonded interface is created, a bridge is created for it as well.
> >
> > Signed-off-by: Darryl L. Pierce <dpierce at redhat.com>
> > ---
> >  src/app/controllers/managed_node_controller.rb     |    4 +-
> >  src/lib/managed_node_configuration.rb              |   79 +++++++++----------
> >  src/test/fixtures/ip_addresses.yml                 |    9 ++
> >  src/test/fixtures/nics.yml                         |    4 +-
> >  .../functional/managed_node_configuration_test.rb  |   21 +++--
> >  5 files changed, 62 insertions(+), 55 deletions(-)
> >
> > diff --git a/src/app/controllers/managed_node_controller.rb b/src/app/controllers/managed_node_controller.rb
> > index f5948be..29e4a4f 100644
> > --- a/src/app/controllers/managed_node_controller.rb
> > +++ b/src/app/controllers/managed_node_controller.rb
> > @@ -44,7 +44,7 @@ class ManagedNodeController < ApplicationController
> >    def load_host
> >      @host = Host.find_by_hostname(params[:host])
> >  
> > -    render :nothing => true, :status => :error unless @host
> > +    render :nothing => true unless @host
> >    end
> >  
> >    def load_macs
> > @@ -58,6 +58,6 @@ class ManagedNodeController < ApplicationController
> >        end
> >      end
> >  
> > -    render :nothing => true, :status => :error if @macs.empty?
> > +    render :nothing => true if @macs.empty?
> >    end
> >  end
> > diff --git a/src/lib/managed_node_configuration.rb b/src/lib/managed_node_configuration.rb
> > index c412917..054836f 100644
> > --- a/src/lib/managed_node_configuration.rb
> > +++ b/src/lib/managed_node_configuration.rb
> > @@ -65,35 +65,37 @@ class ManagedNodeConfiguration
> >      # now process the network interfaces  and bondings
> >  
> >      host.bondings.each do |bonding|
> > -      entry = "ifcfg=none|#{bonding.interface_name}|BONDING_OPTS=\"mode=#{bonding.bonding_type.mode} miimon=100\""
> > -
> > -      if bonding.ip_addresses.empty?
> > -        entry += "|BOOTPROTO=dhcp"
> > -      else
> > -        ip = bonding.ip_addresses[0]
> > -        entry += "|BOOTPROTO=static|IPADDR=#{ip.address}|NETMASK=#{ip.netmask}|BROADCAST=#{ip.broadcast}"
> > -      end
> > +      entry  = "ifcfg=none|#{bonding.interface_name}"
> > +      entry += "|BONDING_OPTS=\"mode=#{bonding.bonding_type.mode} miimon=100\""
> > +      entry += "|BRIDGE=br#{bonding.interface_name}"
> > +      entry += "|ONBOOT=yes"
> > +      result.puts entry
> >  
> > -      result.puts "#{entry}|ONBOOT=yes"
> >   
> The following needs nil network check like the one being done for nics

The updated patch has added the following methods to Bonding and Nic:

Bonding.networking? -- returns if networking is defined
Bonding.boot_protocol, Bonding.ip_address, Bonding.netmask,
Bonding.broadcast, Bonding.gatewway -- that part of the network address

Nic.networking? -- returns if networking is defined 
Nic.bonded? -- returns whether this nic is enslaved to a bonded
interface
Nic.boot_protocol, Nic.ip_address, Nic.netmask, Nic.broadcast,
Nic.gateway -- that part of the network address

This should isolate where the details for networking come from and how
networking is determined.

> > +      if nic.bondings.empty? && nic.physical_network
> > +        iface_name = macs[nic.mac]
> > +        ipaddress=(nic.physical_network.ip_addresses.empty? ? nil : nic.physical_network.ip_addresses.first.address)
> > +        netmask  =(nic.ip_addresses.empty?                  ? nil : nic.ip_addresses.first.netmask)
> > +        broadcast=(nic.ip_addresses.empty?                  ? nil : nic.ip_addresses.first.broadcast)
> >   
> Switch the source of the ip_addresses here. The previous should be:
> 
> +        ipaddress=(nic.physical_network.ip_addresses.empty? ? nil :
> nic.ip_addresses.first.address)
> +        netmask  =(nic.ip_addresses.empty?                  ? nil :
> nic.physical_network.ip_addresses.first.netmask)
> +        broadcast=(nic.ip_addresses.empty?                  ? nil :
> nic.physical_network.ip_addresses.first.broadcast)
> 
> Furthermore app/controllers/managed_node_controller needs a change. As
> previously mentioned the nics arriving are lowercase while being
> compared to nics stored in the db which are uppercase. The following
> change is needed for the controller line 57:
> -        @macs[key] = value
> +        @macs[key.upcase] = value
> 

Done.

Sending the updated patch.
-- 
Darryl L. Pierce, Sr. Software Engineer @ Red Hat, Inc.
Virtual Machine Management - http://www.ovirt.org/
Is fearr Gaeilge bhriste ná Béarla cliste.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 197 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/ovirt-devel/attachments/20090402/c087f13a/attachment.sig>