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

[Mohammed Morsi]

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

if bonding.vlan?
> +      ipaddress=(bonding.ip_addresses.empty?      ? nil : bonding.ip_addresses.first.address)
> +      netmask  =(bonding.vlan.ip_addresses.empty? ? nil : bonding.vlan.ip_addresses.first.netmask)
> +      broadcast=(bonding.vlan.ip_addresses.empty? ? nil : bonding.vlan.ip_addresses.first.broadcast)
> +      add_bridge(result,"none",bonding.interface_name,
> +                 bonding.vlan.boot_type.proto, ipaddress, netmask, broadcast)
>   
end

>  
>        bonding.nics.each do |nic|
> -        process_nic result, nic, macs, bonding
> +        iface_name = macs[nic.mac]
> +        if iface_name
> +          add_slave(result, nic.mac, iface_name, bonding.interface_name)
> +        end
>        end
>      end
>  
> -    has_bridge = false
>      host.nics.each do |nic|
> -      # only process this nic if it doesn't have a bonding
> -      # TODO remove the hack to force a bridge into the picture
> -      if nic.bondings.empty?
> -        process_nic result, nic, macs, nil, false, true
> -
> -	# TODO remove this when bridges are properly supported
> -	unless has_bridge
> -	  macs[nic.mac] = "breth0"
> -	  process_nic result, nic, macs, nil, true, false
> -	  has_bridge = true
> -	end
> +      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

 
      -Mo