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

[Mohammed Morsi]

NACK. While testing this out, I have discovered some major issues with
this patch.

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/lib/managed_node_configuration.rb              |   77 +++++++++-----------
>  src/test/fixtures/ip_addresses.yml                 |    9 +++
>  src/test/fixtures/nics.yml                         |    4 +-
>  .../functional/managed_node_configuration_test.rb  |   21 +++---
>  4 files changed, 59 insertions(+), 52 deletions(-)
>
> diff --git a/src/lib/managed_node_configuration.rb b/src/lib/managed_node_configuration.rb
> index c412917..61ee09e 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"
> +      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)
>  
>        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
> +        iface_name = macs[nic.mac]
>   
This does not work here, as the nic mac addresses are upcased when they
arrive via host-browser, but when they arrive here they are in their
default casing (on my node, lowercase but I'm not sure we can assume
that or not).

Furthermore I've discovered a big issue (more to do w/ the
managed_node_controller) where the mac addresses aren't being parsed
correctly from the query string. Looking at the controller, line 57
where the hash is populated "@macs[key] = value" I inserted a "print
"Got mac " + key.to_s + " - " + value.to_s + " \n"" preceeding it, and
see this in the logs "54%3A52%3A00%3A28%3Ae1%3Ae3%3Deth1".

Looking at /var/log/ovirt.log on the node, I see in the request url it
seems to be escaping the "%" symbols w/ their ascii value "%25"
resulting in the colons and equal signs in the url never getting decoded
properly when it arrives at the server.

I believed this regression was not introduced here but in the "Changes
how macs are collected by ovirt-early" recently committed but will need
to be fixed neverless.

Manually running wget on the server w/ a correct url works in retreiving
the config.

> +        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)
> +        if iface_name
> +          add_bridge(result,nic.mac,iface_name,
> +                     nic.physical_network.boot_type.proto,ipaddress,netmask,broadcast)
> +          add_nic(result, nic.mac, iface_name)
> +        end
>        end
>      end
>   
NICs and bondings don't necessarily need to be assigned to networks, and
thus both sections will blow up if there is at least one device on a
host not associated w/ a network (very likely / common scenario). At the
very least we should be checking to see if nic.physical_network and
bonding.vlan is nil before trying to proceed. If the network is nil, I
believe the requirements is to ignore the device all together and not
return it w/ the config that's sent back to the node.


      -Mo