[Ovirt-devel] [PATCH server] Fixes to the managed_node_configuration code.
Mohammed Morsi
mmorsi at redhat.com
Thu Apr 2 16:25:59 UTC 2009
Looks and works great. ACK.
-Mo
Darryl L. Pierce wrote:
> NOTE: with feedback from mmorsi dated 02 April @ 12:12:47 -0400
>
> 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.
>
> If a hostname is unknown, or it does not send up any macs, then it is
> returned an empty configuration payload and not an error status code.
>
> 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 | 78 +++++++++----------
> src/test/fixtures/ip_addresses.yml | 9 ++
> src/test/fixtures/nics.yml | 4 +-
> .../functional/managed_node_configuration_test.rb | 21 +++--
> .../functional/managed_node_controller_test.rb | 21 -----
> 8 files changed, 142 insertions(+), 76 deletions(-)
>
> diff --git a/src/app/controllers/managed_node_controller.rb b/src/app/controllers/managed_node_controller.rb
> index f5948be..062b3d4 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
> @@ -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.upcase] = value
> end
> end
>
> - render :nothing => true, :status => :error if @macs.empty?
> + render :nothing => true if @macs.empty?
> end
> end
> diff --git a/src/app/models/bonding.rb b/src/app/models/bonding.rb
> index 32b9a37..2dd84fa 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 if networking? && !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..e8b7768 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
> +
> + # Returns the netmask for the nic if networking is defined.
> + def netmask
> + return physical_network.ip_addresses.first.netmask if networking? && !physical_network.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? && !physical_network.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? && !physical_network.ip_addresses.empty?
> + return nil
> + end
> +
> # 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..f93bccd 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\""
> + 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
>
> - 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}"
> + if bonding.networking?
> + add_bridge(result,"none",bonding.interface_name,
> + bonding.boot_protocol, bonding.ip_address,
> + bonding.netmask, bonding.broadcast,
> + bonding.gateway)
> end
>
> - result.puts "#{entry}|ONBOOT=yes"
> -
> 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.networking? && !nic.bonded?
> + iface_name = macs[nic.mac]
> + if iface_name
> + add_bridge(result, nic.mac, iface_name,
> + nic.boot_protocol, nic.ip_address,
> + nic.netmask, nic.broadcast,
> + nic.gateway)
> + add_nic(result, nic.mac, iface_name)
> + end
> end
> end
>
> @@ -102,27 +104,21 @@ class ManagedNodeConfiguration
>
> private
>
> - def self.process_nic(result, nic, macs, bonding = nil, is_bridge = false, bridged = true)
> - iface_name = macs[nic.mac]
> -
> - if iface_name
> - entry = "ifcfg=#{nic.mac}|#{iface_name}"
> -
> - if bonding
> - entry += "|MASTER=#{bonding.interface_name}|SLAVE=yes"
> - else
> - entry += "|BOOTPROTO=#{nic.physical_network.boot_type.proto}"
> - if nic.physical_network.boot_type.proto == 'static'
> - ip = nic.ip_addresses[0]
> - entry += "|IPADDR=#{ip.address}|NETMASK=#{ip.netmask}|BROADCAST=#{ip.broadcast}"
> - end
> - entry += "|BRIDGE=#{nic.bridge}" if nic.bridge && !is_bridge
> - entry += "|BRIDGE=breth0" if !nic.bridge && !is_bridge
> - entry += "|TYPE=bridge" if is_bridge
> - end
> - entry += "|ONBOOT=yes"
> + def self.add_bridge(result, mac, iface_name, bootproto,
> + ipaddress, netmask, broadcast, gateway)
> + entry = "ifcfg=#{mac}|br#{iface_name}|BOOTPROTO=#{bootproto}"
> + if bootproto == "static"
> + entry += "|IPADDR=#{ipaddress}|NETMASK=#{netmask}|BROADCAST=#{broadcast}|GATEWAY=#{gateway}"
> end
> + entry += "|TYPE=bridge|PEERDNS=no|ONBOOT=yes"
> + result.puts entry
> + end
> +
> + def self.add_nic(result, mac, iface_name)
> + result.puts "ifcfg=#{mac}|#{iface_name}|BRIDGE=br#{iface_name}|ONBOOT=yes"
> + end
>
> - result.puts entry if defined? entry
> + def self.add_slave(result, mac, iface_name, master)
> + result.puts "ifcfg=#{mac}|#{iface_name}|MASTER=#{master}|SLAVE=yes|ONBOOT=yes"
> end
> end
> diff --git a/src/test/fixtures/ip_addresses.yml b/src/test/fixtures/ip_addresses.yml
> index b19e9e1..bca0f11 100644
> --- a/src/test/fixtures/ip_addresses.yml
> +++ b/src/test/fixtures/ip_addresses.yml
> @@ -10,6 +10,15 @@ ip_v4_ldapserver_nic_one:
> nic: ldapserver_nic_one
> type: IpV4Address
> address: 172.31.0.25
> + netmask: 255.255.255.0
> + broadcast: 172.31.0.255
> + gateway: 172.31.0.1
> +
> +ip_v4_ldapserver_physical_nic_one:
> + network: static_physical_network_one
> + address: 172.31.0.26
> + netmask: 255.255.255.0
> + broadcast: 172.31.0.255
> gateway: 172.31.0.1
>
> ip_v4_buildserver_nic_one:
> diff --git a/src/test/fixtures/nics.yml b/src/test/fixtures/nics.yml
> index 1cf3223..97397cd 100644
> --- a/src/test/fixtures/nics.yml
> +++ b/src/test/fixtures/nics.yml
> @@ -23,7 +23,7 @@ ldapserver_nic_one:
> mac: 00:03:02:00:09:06
> usage_type: 1
> bandwidth: 100
> - bridge: breth0
> + bridge:
> host: ldapserver_managed_node
> physical_network: static_physical_network_one
>
> @@ -39,7 +39,7 @@ buildserver_nic_two:
> usage_type: 1
> bandwidth: 100
> host: buildserver_managed_node
> - physical_network: static_physical_network_two
> + physical_network: static_physical_network_one
>
> mediaserver_nic_one:
> mac: 07:17:19:65:03:32
> diff --git a/src/test/functional/managed_node_configuration_test.rb b/src/test/functional/managed_node_configuration_test.rb
> index 6ce4885..b66705e 100644
> --- a/src/test/functional/managed_node_configuration_test.rb
> +++ b/src/test/functional/managed_node_configuration_test.rb
> @@ -48,8 +48,8 @@ class ManagedNodeConfigurationTest < Test::Unit::TestCase
>
> expected = <<-HERE
> # THIS FILE IS GENERATED!
> -ifcfg=#{nic.mac}|eth0|BOOTPROTO=dhcp|BRIDGE=breth0|ONBOOT=yes
> -ifcfg=#{nic.mac}|breth0|BOOTPROTO=dhcp|TYPE=bridge|ONBOOT=yes
> +ifcfg=#{nic.mac}|breth0|BOOTPROTO=#{nic.boot_protocol}|TYPE=bridge|PEERDNS=no|ONBOOT=yes
> +ifcfg=#{nic.mac}|eth0|BRIDGE=breth0|ONBOOT=yes
> HERE
>
> result = ManagedNodeConfiguration.generate(
> @@ -67,8 +67,8 @@ ifcfg=#{nic.mac}|breth0|BOOTPROTO=dhcp|TYPE=bridge|ONBOOT=yes
>
> expected = <<-HERE
> # THIS FILE IS GENERATED!
> -ifcfg=#{nic.mac}|eth0|BOOTPROTO=#{nic.physical_network.boot_type.proto}|IPADDR=#{nic.ip_addresses.first.address}|NETMASK=#{nic.ip_addresses.first.netmask}|BROADCAST=#{nic.ip_addresses.first.broadcast}|BRIDGE=#{nic.bridge}|ONBOOT=yes
> -ifcfg=#{nic.mac}|breth0|BOOTPROTO=#{nic.physical_network.boot_type.proto}|IPADDR=#{nic.ip_addresses.first.address}|NETMASK=|BROADCAST=#{nic.ip_addresses.first.netmask}|TYPE=bridge|ONBOOT=yes
> +ifcfg=#{nic.mac}|breth0|BOOTPROTO=#{nic.boot_protocol}|IPADDR=#{nic.ip_address}|NETMASK=#{nic.netmask}|BROADCAST=#{nic.broadcast}|GATEWAY=#{nic.gateway}|TYPE=bridge|PEERDNS=no|ONBOOT=yes
> +ifcfg=#{nic.mac}|eth0|BRIDGE=breth0|ONBOOT=yes
> HERE
>
> result = ManagedNodeConfiguration.generate(
> @@ -87,9 +87,10 @@ ifcfg=#{nic.mac}|breth0|BOOTPROTO=#{nic.physical_network.boot_type.proto}|IPADDR
>
> expected = <<-HERE
> # THIS FILE IS GENERATED!
> -ifcfg=#{nic1.mac}|eth0|BOOTPROTO=#{nic1.physical_network.boot_type.proto}|IPADDR=#{nic1.ip_addresses.first.address}|NETMASK=#{nic1.ip_addresses.first.netmask}|BROADCAST=#{nic1.ip_addresses.first.broadcast}|BRIDGE=breth0|ONBOOT=yes
> -ifcfg=#{nic1.mac}|breth0|BOOTPROTO=#{nic1.physical_network.boot_type.proto}|IPADDR=#{nic1.ip_addresses.first.address}|NETMASK=#{nic1.ip_addresses.first.netmask}|BROADCAST=#{nic1.ip_addresses.first.broadcast}|TYPE=bridge|ONBOOT=yes
> -ifcfg=#{nic2.mac}|eth1|BOOTPROTO=#{nic2.physical_network.boot_type.proto}|BRIDGE=breth0|ONBOOT=yes
> +ifcfg=#{nic1.mac}|breth0|BOOTPROTO=#{nic1.boot_protocol}|IPADDR=#{nic1.ip_address}|NETMASK=#{nic1.netmask}|BROADCAST=#{nic1.broadcast}|GATEWAY=#{nic1.gateway}|TYPE=bridge|PEERDNS=no|ONBOOT=yes
> +ifcfg=#{nic1.mac}|eth0|BRIDGE=breth0|ONBOOT=yes
> +ifcfg=#{nic2.mac}|breth1|BOOTPROTO=#{nic2.boot_protocol}|TYPE=bridge|PEERDNS=no|ONBOOT=yes
> +ifcfg=#{nic2.mac}|eth1|BRIDGE=breth1|ONBOOT=yes
> HERE
>
> result = ManagedNodeConfiguration.generate(
> @@ -114,7 +115,8 @@ ifcfg=#{nic2.mac}|eth1|BOOTPROTO=#{nic2.physical_network.boot_type.proto}|BRIDGE
> expected = <<-HERE
> # THIS FILE IS GENERATED!
> bonding=#{bonding.interface_name}
> -ifcfg=none|#{bonding.interface_name}|BONDING_OPTS="mode=#{bonding.bonding_type.mode} miimon=100"|BOOTPROTO=static|IPADDR=#{bonding.ip_addresses.first.address}|NETMASK=#{bonding.ip_addresses.first.netmask}|BROADCAST=#{bonding.ip_addresses.first.broadcast}|ONBOOT=yes
> +ifcfg=none|#{bonding.interface_name}|BONDING_OPTS="mode=#{bonding.bonding_type.mode} miimon=100"|BRIDGE=br#{bonding.interface_name}|ONBOOT=yes
> +ifcfg=none|br#{bonding.interface_name}|BOOTPROTO=dhcp|TYPE=bridge|PEERDNS=no|ONBOOT=yes
> ifcfg=#{nic1.mac}|eth0|MASTER=#{bonding.interface_name}|SLAVE=yes|ONBOOT=yes
> ifcfg=#{nic2.mac}|eth1|MASTER=#{bonding.interface_name}|SLAVE=yes|ONBOOT=yes
> HERE
> @@ -140,7 +142,8 @@ HERE
> expected = <<-HERE
> # THIS FILE IS GENERATED!
> bonding=#{bonding.interface_name}
> -ifcfg=none|#{bonding.interface_name}|BONDING_OPTS="mode=#{bonding.bonding_type.mode} miimon=100"|BOOTPROTO=dhcp|ONBOOT=yes
> +ifcfg=none|#{bonding.interface_name}|BONDING_OPTS="mode=#{bonding.bonding_type.mode} miimon=100"|BRIDGE=br#{bonding.interface_name}|ONBOOT=yes
> +ifcfg=none|br#{bonding.interface_name}|BOOTPROTO=#{bonding.boot_protocol}|TYPE=bridge|PEERDNS=no|ONBOOT=yes
> ifcfg=#{nic1.mac}|eth0|MASTER=#{bonding.interface_name}|SLAVE=yes|ONBOOT=yes
> ifcfg=#{nic2.mac}|eth1|MASTER=#{bonding.interface_name}|SLAVE=yes|ONBOOT=yes
> HERE
> diff --git a/src/test/functional/managed_node_controller_test.rb b/src/test/functional/managed_node_controller_test.rb
> index a6d2667..dd93e80 100644
> --- a/src/test/functional/managed_node_controller_test.rb
> +++ b/src/test/functional/managed_node_controller_test.rb
> @@ -26,27 +26,6 @@ class ManagedNodeControllerTest < ActionController::TestCase
> fixtures :hosts
> fixtures :nics
>
> - # Ensures that the request fails if it's missing a hostname, or if the
> - # hostname is invalid.
> - #
> - def test_config_with_invalid_hostname
> - get :config
> -
> - assert_response :error
> -
> - get :config, {:host => 'invalid.prod.com'}
> -
> - assert_response :error
> - end
> -
> - # Ensures the request fails if no mac addresses are supplied.
> - #
> - def test_config_without_macs
> - get :config, {:host => hosts(:mailservers_managed_node).hostname}
> -
> - assert_response :error
> - end
> -
> # Ensures the request succeeds if it is well-formed.
> #
> def test_config
>
More information about the ovirt-devel
mailing list