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

[Mohammed Morsi]

Almost there, just a few caveats below

Darryl L. Pierce wrote:
> Added public APIs to Bonding and Nic to retrieve networking details for
> the respective interface.
>
> 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     |    6 +-
>  src/app/models/bonding.rb                          |   40 +++++++++
>  src/app/models/nic.rb                              |   39 +++++++++
>  src/lib/managed_node_configuration.rb              |   86 +++++++++----------
>  src/test/fixtures/ip_addresses.yml                 |    9 ++
>  src/test/fixtures/nics.yml                         |    4 +-
>  .../functional/managed_node_configuration_test.rb  |   21 +++--
>  7 files changed, 146 insertions(+), 59 deletions(-)
>
> diff --git a/src/app/controllers/managed_node_controller.rb b/src/app/controllers/managed_node_controller.rb
> index f5948be..0922d92 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
>   
Removing this :status => error seems to break
managed_node_controller_test::test_config_with_invalid_hostname. This
test probably can now be removed.


>  
>    def load_macs
> @@ -54,10 +54,10 @@ class ManagedNodeController < ApplicationController
>      if mac_text != nil
>        mac_text.scan(/([^,]+)\,?/).each do |line|
>          key, value = line.first.split("=")
> -        @macs[key] = value
> +        @macs[key] = value.upcase
>        end
>      end
>   
As discussed, key (mac) should be upcased as opposed to value
(interface_name). So this should be changed to
@mac[key.upcase] = value


>  
> -    render :nothing => true, :status => :error if @macs.empty?
> +    render :nothing => true if @macs.empty?
>    end
>  end
>   
Removing this :status => error seems to break
managed_node_controller_test::test_config_with_invalid_macs. This test
probably can now be removed.


> diff --git a/src/app/models/bonding.rb b/src/app/models/bonding.rb
> index 32b9a37..d87f3a0 100644
> --- a/src/app/models/bonding.rb
> +++ b/src/app/models/bonding.rb
> @@ -72,6 +72,46 @@ class Bonding < ActiveRecord::Base
>       :greater_than_or_equal_to => 0,
>       :unless => Proc.new { |bonding| bonding.arp_interval.nil? }
>  
> +  # Returns whether networking is defined for this interface.
> +  def networking?
> +    # there is network if there's a vlan defined.
> +    (vlan.nil? == false)
> +  end
> +
> +  # Returns the boot protocol for the interface, or +nil+ if networking
> +  # is not defined.
> +  def boot_protocol
> +    return vlan.boot_type.proto if networking?
> +    return nil
> +  end
> +
> +  # Returns the ip address assigned to this bonded interface, or +nil+
> +  # if no networking is defined.
> +  def ip_address
> +    return ip_addresses.first.address unless ip_addresses.empty?
> +    return nil
> +  end
> +
> +  # Returns the netmask assigned to this bonded interface, or +nil+
> +  # if no networking is defined.
> +  def netmask
> +    return vlan.ip_addresses.first.netmask if networking? && !vlan.ip_addresses.empty?
> +    return nil
> +  end
> +
> +  # Returns the broadcast address assigned to this bonded interface,
> +  # or +nil if no networking is defined.
> +  def broadcast
> +    return vlan.ip_addresses.first.broadcast if networking? && !vlan.ip_addresses.empty?
> +    return nil
> +  end
> +
> +  # Returns the gateway address for the bonded interface if networking is defined.
> +  def gateway
> +    return vlan.ip_addresses.first.gateway if networking? && !vlan.ip_addresses.empty?
> +    return nil
> +  end
> +
>   protected
>    def validate
>      if ! vlan.nil? and
> diff --git a/src/app/models/nic.rb b/src/app/models/nic.rb
> index e26c110..8e19c26 100644
> --- a/src/app/models/nic.rb
> +++ b/src/app/models/nic.rb
> @@ -43,6 +43,45 @@ class Nic < ActiveRecord::Base
>       :scope => :host_id,
>       :unless => Proc.new { |nic| nic.physical_network_id.nil? }
>  
> +  # Returns whether the nic has networking defined.
> +  def networking?
> +    (physical_network != nil)
> +  end
> +
> +  # Returns the boot protocol for the nic if networking is defined.
> +  def boot_protocol
> +    return physical_network.boot_type.proto if networking?
> +  end
> +
> +  # Returns whether the nic is enslaved by a bonded interface.
> +  def bonded?
> +    !bondings.empty?
> +  end
> +
> +  # Returns the ip address for the nic if networking is defined.
> +  def ip_address
> +    return ip_addresses.first.address if networking? && !ip_addresses.empty?
> +    return nil
> +  end
>   
I'm not sure if we need to check networking? here, as we're not
currently performing the same check in the 'ip_address' method in
bonding. Doesn't currently break anything though, so could be left as
is, just want to highlight the discrepancy as it may cause future confusion.


> +
> +  # Returns the netmask for the nic if networking is defined.
> +  def netmask
> +    return physical_network.ip_addresses.first.netmask if networking? && !ip_addresses.empty?
> +    return nil
> +  end
> +
> +  # Returns the broadcast address for the nic if networking is defined.
> +  def broadcast
> +    return physical_network.ip_addresses.first.broadcast if networking? && !ip_addresses.empty?
> +    return nil
> +  end
> +
> +  # Returns the gateway address fo rthe nic if networking is defined.
> +  def gateway
> +    return physical_network.ip_addresses.first.gateway if networking? && !ip_addresses.empty?
> +    return nil
> +  end
> +
>   
The conditional first line of the previous three methods should be
changed to check physical_network.ip_addresses.empty?



>    # validate 'bridge' or 'usage_type' attribute ?
>  
>    protected
> diff --git a/src/lib/managed_node_configuration.rb b/src/lib/managed_node_configuration.rb
> index c412917..028573b 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
> -
> -      result.puts "#{entry}|ONBOOT=yes"
> -
> -      bonding.nics.each do |nic|
> -        process_nic result, nic, macs, bonding
> +      if bonding.networking?
>   
Should we not be creating the bonding whatsoever if no network is
assigned? This "bonding.networking?" conditional should surround the
add_bridge call for sure. But I'm not sure if the bonding should be
created regardless of whether a network is assigned. Hugh? Alan?


   -Mo