[Ovirt-devel] [PATCH server] Fixes to the managed_node_configuration code.
Mohammed Morsi
mmorsi at redhat.com
Wed Apr 1 00:17:44 UTC 2009
Looks good for most part, save a few specifics, comments below.
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/nics.yml | 6 +-
> .../functional/managed_node_configuration_test.rb | 15 +++--
> 3 files changed, 48 insertions(+), 50 deletions(-)
>
> diff --git a/src/lib/managed_node_configuration.rb b/src/lib/managed_node_configuration.rb
> index c412917..f864c46 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"
> + add_bridge(result,
> + "none",
> + bonding.interface_name,
> + (bonding.ip_addresses.empty? ? "dhcp" : "static"),
> + (bonding.ip_addresses.empty? ? nil : bonding.ip_addresses[0]))
>
>
Are you sure bonding bridges don't need to have an assigned mac address?
Also instead of checking for bonding.ip_addresses.empty? shouldn't we
utilize bonding.vlan.nil?. Furthermore bonding.vlan.proto should be
employed similar to nic.physical_network.proto below
Furthermore I believe simply passing in / using the bonding ip address
for netmask / broadcast won't work as explained below.
> 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]
> + if iface_name
> + add_bridge(result,
> + nic.mac,
> + iface_name,
> + nic.physical_network.boot_type.proto,
> + nic.ip_addresses[0])
>
I believe we should have the same ip_addresses.empty? check here as we
do w/ bondings as if a nic can be assigned to a dhcp network and have no
explicitly associated ip addresses.
Furthermore nic.physical_network should be checked to see if null, as a
nic doesn't have to be assigned to a network. (actually I believe one of
the reqs is that if it a nic isn't assigned to a network, it shouldn't
be included in the config returned to the node here, but we should check
w/ Hugh to make sure)
Also the same issue w/ passing in / using the nic ip address for netmask
/ broadcast won't work as explained below.
> + add_nic(result, nic.mac, iface_name)
> + end
> end
> end
>
> @@ -102,27 +104,20 @@ 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}"
>
While for static networks the ip associated w/ the nic / bonding will
contain the assigned ip address, it will not contain the netmask and the
broadcast. Rather this data is contained in the ip_address associated w/
the network which the nic / bonding is assigned to (physical net or
vlan). Essentially you configure the gateway / netmask / broadcast when
configuring the network and simply assign an ip address to the actual
device.
> - 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)
> + entry = "ifcfg=#{mac}|br#{iface_name}|BOOTPROTO=#{bootproto}"
> + if ipaddress
> + entry += "|IPADDR=#{ipaddress.address}|NETMASK=#{ipaddress.netmask}|BROADCAST=#{ipaddress.broadcast}"
> end
> + entry += "|TYPE=bridge|ONBOOT=yes"
>
We also need 'PEERDNS=no' associated w/ bridge configuration here.
> + 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/nics.yml b/src/test/fixtures/nics.yml
> index b8bb6c7..048d80c 100644
> --- a/src/test/fixtures/nics.yml
> +++ b/src/test/fixtures/nics.yml
> @@ -10,7 +10,7 @@ mailserver_nic_two:
> usage_type: 1
> bandwidth: 100
> host: mailservers_managed_node
> - physical_network: dhcp_physical_network_one
> + physical_network: dhcp_physical_network_two
>
> fileserver_nic_one:
> mac: 00:99:00:99:13:07
> @@ -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
>
> @@ -53,4 +53,4 @@ mediaserver_nic_two:
> usage_type: 1
> bandwidth: 100
> host: mediaserver_managed_node
> - physical_network: dhcp_physical_network_one
> + physical_network: dhcp_physical_network_two
> diff --git a/src/test/functional/managed_node_configuration_test.rb b/src/test/functional/managed_node_configuration_test.rb
> index 6ce4885..b6e591c 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}|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}|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}|eth0|BRIDGE=breth0|ONBOOT=yes
> +ifcfg=#{nic2.mac}|breth1|BOOTPROTO=#{nic2.physical_network.boot_type.proto}|TYPE=bridge|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=static|IPADDR=#{bonding.ip_addresses.first.address}|NETMASK=#{bonding.ip_addresses.first.netmask}|BROADCAST=#{bonding.ip_addresses.first.broadcast}|TYPE=bridge|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=dhcp|TYPE=bridge|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
>
-Mo
More information about the ovirt-devel
mailing list