rh_interface: Handle changes to multiple interfaces.

When more than one interface is affected by changes in a single Puppet
run, it was easy to get into a state where the network on the node was
not functioning properly, since we were reconfiguring each interface
independently.  Examples include when changing from a single interface
to a setup with bonded interfaces, when configuring bridging (and IP
addresses move from the physical interface to the bridge interface),
or in general when routes are moved from one interface to another.  We
would reconfigure (ifdown+ifup) e.g. eth0, and then, much later, the
ifdown+ifup for bond0 would be executed.  Inbetween those, the network
was broken, and many Puppet resources (e.g. package resources) would
fail.

In most cases, a manual 'service network restart' after the failed
Puppet run, would bring the network up again, and the next Puppet run
would succeed, bringing the failed resources into sync, but it is an
annoyance to users.

We solve this by doing a single 'service network restart' after all
interfaces have been configured.  That way, there is no point in time
where some interfaces have been reconfigured and some not; either none
or all of them have the correct configuration.  (However, if Puppet is
interrupted during a run, it can leave the /etc/sysconfig files in an
inconsistent state; doing 'service network restart' or rebooting then
will give you a non-functioning network.)

The downside to this, is that *all* network interfaces, including
those not managed by Puppet, will be brought down for a short period,
even when only a single interface has its configuration changed.

This was originally commit 146bec6c3a26 in nsc-puppet-utils.

manifests/init.pp

@@ -151,7 +151,8 @@ define rh_interface($bootproto='static',# 'static', 'dhcp' or 'unconfigured'
     $ipv6_inuse = ($x_ipv6addr != [])
 
     $qname = shellquote($name)
-    $ifconfig_exec_id = "rh_interface::ifconfig::${name}/${ensure}"
+
+    rh_interface::network_service::wrapper { $name: ; }
 
     if ($bonding_slaves != [])
     {
@@ -176,21 +177,22 @@ define rh_interface($bootproto='static',# 'static', 'dhcp' or 'unconfigured'
 		    ensure => file,
 		    content => template('nsc-puppet-utils/rh-ifcfg.erb'),
 		    owner => 'root', group => 'root', mode => 0444,
-		    notify => Exec[$ifconfig_exec_id];
+		    notify => Class[rh_interface::network_service];
 
 		"/etc/sysconfig/network-scripts/route-${name}":
 		    ensure => $ipv4_inuse ? { true => file, default => absent },
 		    content => template('nsc-puppet-utils/rh-ifroute-v4.erb'),
 		    owner => 'root', group => 'root', mode => 0444,
-		    notify => Exec[$ifconfig_exec_id];
+		    notify => Class[rh_interface::network_service];
 
 		"/etc/sysconfig/network-scripts/route6-${name}":
 		    ensure => $ipv6_inuse ? { true => file, default => absent },
 		    content => template('nsc-puppet-utils/rh-ifroute-v6.erb'),
 		    owner => 'root', group => 'root', mode => 0444,
-		    notify => Exec[$ifconfig_exec_id];
+		    notify => Class[rh_interface::network_service];
 	    }
 	}
+
 	'absent': {
 	    file {
 		[ "/etc/sysconfig/network-scripts/ifcfg-${name}",
@@ -199,6 +201,21 @@ define rh_interface($bootproto='static',# 'static', 'dhcp' or 'unconfigured'
 		]:
 		    ensure => absent;
 	    }
+	    exec {
+		# This case will not be handled by 'service network restart',
+		# as we have removed the sysconfig files for the interface.
+		# The network service will not touch interfaces without any
+		# sysconfig files
+		"rh_interface::absent::${name}":
+		    command => "ip link set dev ${qname} down",
+		    onlyif => "ip link show ${qname} | egrep ' state UP( |\$)'",
+		    provider => shell,	# Since we use a pipeline
+		    path => '/bin:/usr/bin:/sbin:/usr/sbin';
+	    }
+	}
+
+	default: {
+	    fail("Bad ensure parameter to rh_interface ${title}: ${ensure}")
 	}
     }
 
@@ -216,69 +233,78 @@ define rh_interface($bootproto='static',# 'static', 'dhcp' or 'unconfigured'
 	    default  => $ipv6_inuse ? { true => 0, default => 1 },
 	}
 
-	if $name !~ /[.][0-9]+$/ {
-	    # Non-VLAN interface
+	$if_re = sprintf('(^|.*,)%s(,.*|$)', regexp_quote($name))
+	if regsubst($::interfaces, $if_re, $name) == $name
+	{
+	    # Interface already exists, so it should be OK to do a sysctl
+	    # on it.
 	    sysctl {
 		"net.ipv6.conf.${sysctl_ifname}.disable_ipv6":
 		    value => $ipv6_sysctl_value,
-		    notify => Exec[$ifconfig_exec_id];
+		    ignoremissing => true,
+		    notify => Class[rh_interface::network_service];
 	    }
 	} else {
-	    # VLAN interfaces don't exist until ifup(8) brings them up, and
-	    # it is thus impossible to perform sysctl:s on them.  But that
-	    # is too late, as ifup(8) will already have failed to set the
-	    # addresses we want.  Luckily, ifup in EL-6 and EL-7 executes
-	    # the sysctl:s in sysctl.conf for the interface at the right
-	    # time, before it sets addresses.  In EL-5, that is not done,
-	    # but the disable_ipv6 setting defaults to 0 anyway, so if we
-	    # do want IPv6 addresses, ifup will be able to set them.  If
-	    # we don't, then performing the sysctl after bringing up the
-	    # interface, will remove the link-local address, but we will
-	    # have an interval where it exists.  Splitting it this way seem
-	    # to be the best we can do.
+	    # Probably some kind of virtual interface (bonding interface,
+	    # VLAN interface, bridge).  Those do not exist until ifup(8)
+	    # brings them up, and it is thus impossible to execute the
+	    # sysctl on them until then.  But that is too late, as ifup(8)
+	    # will already have failed to set the addresses we want.
+	    #
+	    # Luckily, ifup in EL-6 and EL-7 executes the sysctl:s in
+	    # /etc/sysctl.conf for the interface at the right time, before
+	    # it sets addresses.  Not so in EL-5, but the disable_ipv6
+	    # setting defaults to 0 anyway, so if we do want IPv6 addresses,
+	    # ifup will be able to set them.  If we don't, then performing
+	    # the sysctl after bringing up the interface, will remove the
+	    # link-local address, but we will have an interval where it
+	    # exists.  Splitting it this way seem to be the best we can do.
 
 	    sysctl::sysctl_conf {
 		"net.ipv6.conf.${sysctl_ifname}.disable_ipv6":
 		    value => $ipv6_sysctl_value,
-		    notify => Exec[$ifconfig_exec_id];
+		    notify => Class[rh_interface::network_service];
 	    }
 	    sysctl::exec {
 		"net.ipv6.conf.${sysctl_ifname}.disable_ipv6":
 		    value => $ipv6_sysctl_value,
 		    ignoremissing => true,
-		    require => Exec[$ifconfig_exec_id];
+		    require => Class[rh_interface::network_service];
 	    }
 	}
     }
+}
 
-    case $ensure {
-	'up': {
-	    exec {
-		$ifconfig_exec_id:
-		    command => "/sbin/ifdown ${qname} && /sbin/ifup ${qname}",
-		    refreshonly => true,
-		    path => '/bin:/usr/bin:/sbin:/usr/sbin';
-	    }
-	}
-	'down': {
-	    exec {
-		$ifconfig_exec_id:
-		    command => "/sbin/ifdown ${qname}",
-		    refreshonly => true,
-		    path => '/bin:/usr/bin:/sbin:/usr/sbin';
-	    }
-	}
-	'absent': {
-	    exec {
-		$ifconfig_exec_id:
-		    command => "ip link set dev ${qname} down",
-		    onlyif => "ip link show ${qname} | egrep ' state UP( |\$)'",
-		    provider => shell,	# Since we use a pipeline
-		    path => '/bin:/usr/bin:/sbin:/usr/sbin';
-	    }
-	}
-	default: {
-	    fail("Bad ensure parameter to rh_interface ${title}: ${ensure}")
-	}
+
+# Helper class for rh_interface
+#
+# When changes to more than one interface are performed, e.g. moving a
+# route, changing between bonded and non-bonded interfaces, et.c, we
+# want to apply all changes in one go, with no Puppet resources applied
+# between changing interface A and interface B, as we might be in a
+# state without working network then.
+# The disadvantage is that all interfaces will be brought down for a
+# while, even if only one interface has changes.
+
+class rh_interface::network_service
+{
+    service {
+	'network':
+	    enable => 'true', ensure => running,
+	    hasstatus => true, hasrestart => true;
     }
 }
+
+# Helper-wrapper for rh_interface::network_service...
+# If someone does 'require => Rh_interface["eth0"]', we want the network
+# service to be restarted before the requirement is fulfilled.  A simple
+# 'include rh_interface::network_service' inside rh_interface is not
+# enough to do so, but 'require' is.  However, we then get a dependency
+# cycle, since resources inside rh_interface does notify on that class.
+#     We work around that with this wrapper definition.  Puppet will
+# then not see it as a cycle, but instead do what we want.
+
+define rh_interface::network_service::wrapper()
+{
+    require rh_interface::network_service
+}