Bridge script no longer responsible for bond-raise-delay

Note: this is missing additional state tests for the new
reconfigureDelay parameter passed to FindMissingBridgesForContainer().

api/provisioner/provisioner.go

@@ -252,26 +252,25 @@ func (st *State) SetHostMachineNetworkConfig(hostMachineID string, netConfig []p
 	return nil
 }
 
-func (st *State) HostChangesForContainer(containerTag names.MachineTag) ([]network.DeviceToBridge, error) {
+func (st *State) HostChangesForContainer(containerTag names.MachineTag) ([]network.DeviceToBridge, int, error) {
 	var result params.HostNetworkChangeResults
 	args := params.Entities{
 		Entities: []params.Entity{{Tag: containerTag.String()}},
 	}
 	if err := st.facade.FacadeCall("HostChangesForContainers", args, &result); err != nil {
-		return nil, err
+		return nil, 0, err
 	}
 	if len(result.Results) != 1 {
-		return nil, errors.Errorf("expected 1 result, got %d", len(result.Results))
+		return nil, 0, errors.Errorf("expected 1 result, got %d", len(result.Results))
 	}
 	if err := result.Results[0].Error; err != nil {
-		return nil, err
+		return nil, 0, err
 	}
 	newBridges := result.Results[0].NewBridges
 	res := make([]network.DeviceToBridge, len(newBridges))
 	for i, bridgeInfo := range newBridges {
 		res[i].BridgeName = bridgeInfo.BridgeName
 		res[i].DeviceName = bridgeInfo.HostDeviceName
-		res[i].NetBondReconfigureDelay = bridgeInfo.NetBondReconfigureDelay
 	}
-	return res, nil
+	return res, result.Results[0].ReconfigureDelay, nil
 }

api/provisioner/provisioner_test.go

@@ -709,12 +709,13 @@ func (s *provisionerSuite) TestHostChangesForContainer(c *gc.C) {
 	container, err := s.State.AddMachineInsideMachine(containerTemplate, s.machine.Id(), instance.LXD)
 	c.Assert(err, jc.ErrorIsNil)
 
-	changes, err := s.provisioner.HostChangesForContainer(container.MachineTag())
+	changes, reconfigureDelay, err := s.provisioner.HostChangesForContainer(container.MachineTag())
 	c.Assert(err, gc.ErrorMatches, "dummy provider network config not supported.*")
 	c.Skip("can't test without network support")
 	c.Assert(err, jc.ErrorIsNil)
 	c.Check(changes, gc.DeepEquals, []network.DeviceToBridge{{
 		BridgeName: "br-ens3",
 		DeviceName: "ens3",
 	}})
+	c.Check(reconfigureDelay, gc.Equals, 0)
 }

apiserver/params/network.go

@@ -135,9 +135,8 @@ type NetworkConfig struct {
 // DeviceBridgeInfo lists the host device and the expected bridge to be
 // created.
 type DeviceBridgeInfo struct {
-	HostDeviceName          string `json:"host-device-name"`
-	BridgeName              string `json:"bridge-name"`
-	NetBondReconfigureDelay int    `json:"net-bond-reconfigure-delay"`
+	HostDeviceName string `json:"host-device-name"`
+	BridgeName     string `json:"bridge-name"`
 }
 
 // ProviderInterfaceInfoResults holds the results of a
@@ -471,6 +470,10 @@ type HostNetworkChange struct {
 	// NewBridges lists the bridges that need to be created and what host
 	// device they should be connected to.
 	NewBridges []DeviceBridgeInfo `json:"new-bridges"`
+
+	// ReconfigureDelay is the duration in seconds to sleep before
+	// raising the bridged interface
+	ReconfigureDelay int `json:"reconfigure-delay"`
 }
 
 // HostNetworkChangeResults holds the network changes that are necessary for multiple containers to be created.

apiserver/provisioner/provisioner.go

@@ -797,18 +797,19 @@ type hostChangesContext struct {
 }
 
 func (ctx *hostChangesContext) ProcessOneContainer(netEnv environs.NetworkingEnviron, idx int, host, container *state.Machine) error {
-	bridges, err := host.FindMissingBridgesForContainer(container)
+	netBondReconfigureDelay := netEnv.Config().NetBondReconfigureDelay()
+	bridges, reconfigureDelay, err := host.FindMissingBridgesForContainer(container, netBondReconfigureDelay)
 	if err != nil {
 		return err
 	}
-	netBondReconfigureDelay := netEnv.Config().NetBondReconfigureDelay()
+
+	ctx.result.Results[idx].ReconfigureDelay = reconfigureDelay
 	for _, bridgeInfo := range bridges {
 		ctx.result.Results[idx].NewBridges = append(
 			ctx.result.Results[idx].NewBridges,
 			params.DeviceBridgeInfo{
-				HostDeviceName:          bridgeInfo.DeviceName,
-				BridgeName:              bridgeInfo.BridgeName,
-				NetBondReconfigureDelay: netBondReconfigureDelay,
+				HostDeviceName: bridgeInfo.DeviceName,
+				BridgeName:     bridgeInfo.BridgeName,
 			})
 	}
 	return nil

network/add-juju-bridge.py

@@ -393,7 +393,7 @@ def arg_parser():
     parser.add_argument('--interfaces-to-bridge', help="interfaces to bridge; space delimited", type=str, required=True)
     parser.add_argument('--dry-run', help="dry run, no activation", action='store_true', default=False, required=False)
     parser.add_argument('--bridge-name', help="bridge name", type=str, required=False)
-    parser.add_argument('--net-bond-reconfigure-delay', help="delay in seconds before raising bonded interfaces", type=int, required=False, default=30)
+    parser.add_argument('--reconfigure-delay', help="delay in seconds before raising interfaces", type=int, required=False, default=10)
     parser.add_argument('filename', help="interfaces(5) based filename")
     return parser
 
@@ -440,22 +440,8 @@ def main(args):
             print_stanzas(stanzas, f)
             f.close()
 
-    # On configurations that have bonds in 802.3ad mode there is a
-    # race condition betweeen an immediate ifdown then ifup.
-    #
-    # On the h/w I have a 'sleep 0.1' is sufficient but to accommodate
-    # other setups we arbitrarily choose something larger. We don't
-    # want to massively slow bootstrap down but, equally, 0.1 may be
-    # too small for other configurations.
-
-    for s in stanzas:
-        if s.is_logical_interface and s.iface.is_bonded:
-            print("working around https://bugs.launchpad.net/ubuntu/+source/ifenslave/+bug/1269921")
-            print("working around https://bugs.launchpad.net/juju-core/+bug/1594855")
-            if args.dry_run and args.dry_run > 0:
-                shell_cmd("sleep {}".format(args.net_bond_reconfigure_delay, dry_run=args.dry_run))
-            break
-
+    if args.reconfigure_delay and args.reconfigure_delay > 0 :
+        shell_cmd("sleep {}".format(args.reconfigure_delay), dry_run=args.dry_run)
     shell_cmd("cat {}".format(args.filename), dry_run=args.dry_run)
     shell_cmd("ifup --exclude=lo --interfaces={} -a".format(args.filename), dry_run=args.dry_run)
     shell_cmd("ip -d link show", dry_run=args.dry_run)

network/bridge.go

@@ -19,7 +19,7 @@ type Bridger interface {
 	// TODO(frobware) - we may want a different type to encompass
 	// and reflect how bridging should be done vis-a-vis what
 	// needs to be bridged.
-	Bridge(devices []DeviceToBridge) error
+	Bridge(devices []DeviceToBridge, reconfigureDelay int) error
 }
 
 type etcNetworkInterfacesBridger struct {
@@ -64,9 +64,9 @@ func pythonInterpreters() []string {
 	return result
 }
 
-func (b *etcNetworkInterfacesBridger) Bridge(devices []DeviceToBridge) error {
-	cmd := bridgeCmd(devices, b.PythonInterpreter, b.BridgePrefix, b.Filename, BridgeScriptPythonContent, b.DryRun)
-	infoCmd := bridgeCmd(devices, b.PythonInterpreter, b.BridgePrefix, b.Filename, "<script-redacted>", b.DryRun)
+func (b *etcNetworkInterfacesBridger) Bridge(devices []DeviceToBridge, reconfigureDelay int) error {
+	cmd := bridgeCmd(devices, b.PythonInterpreter, b.BridgePrefix, b.Filename, BridgeScriptPythonContent, b.DryRun, reconfigureDelay)
+	infoCmd := bridgeCmd(devices, b.PythonInterpreter, b.BridgePrefix, b.Filename, "<script-redacted>", b.DryRun, reconfigureDelay)
 	logger.Infof("bridgescript command=%s", infoCmd)
 	result, err := runCommand(cmd, b.Environ, b.Clock, b.Timeout)
 	if err != nil {
@@ -86,7 +86,7 @@ func (b *etcNetworkInterfacesBridger) Bridge(devices []DeviceToBridge) error {
 	return nil
 }
 
-func bridgeCmd(devices []DeviceToBridge, pythonInterpreter, bridgePrefix, filename, pythonScript string, dryRun bool) string {
+func bridgeCmd(devices []DeviceToBridge, pythonInterpreter, bridgePrefix, filename, pythonScript string, dryRun bool, reconfigureDelay int) string {
 	dryRunOption := ""
 
 	if bridgePrefix != "" {
@@ -97,20 +97,16 @@ func bridgeCmd(devices []DeviceToBridge, pythonInterpreter, bridgePrefix, filena
 		dryRunOption = "--dry-run"
 	}
 
-	netBondReconfigureDelay := 0
-	netBondReconfigureDelayOption := ""
+	reconfigureDelayOption := ""
 
 	deviceNames := make([]string, len(devices))
 
 	for i, d := range devices {
-		if d.NetBondReconfigureDelay > netBondReconfigureDelay {
-			netBondReconfigureDelay = d.NetBondReconfigureDelay
-		}
 		deviceNames[i] = d.DeviceName
 	}
 
-	if netBondReconfigureDelay > 0 {
-		netBondReconfigureDelayOption = fmt.Sprintf("--net-bond-reconfigure-delay=%v", netBondReconfigureDelay)
+	if reconfigureDelay >= 0 {
+		reconfigureDelayOption = fmt.Sprintf("--reconfigure-delay=%v", reconfigureDelay)
 	}
 
 	return fmt.Sprintf(`
@@ -122,7 +118,7 @@ EOF
 		strings.Join(deviceNames, " "),
 		bridgePrefix,
 		dryRunOption,
-		netBondReconfigureDelayOption,
+		reconfigureDelayOption,
 		filename,
 		pythonScript)
 }

network/bridge_test.go

@@ -47,17 +47,17 @@ func assertCmdResult(c *gc.C, cmd, expected string) {
 	c.Assert(string(result.Stderr), gc.Equals, "")
 }
 
-func assertBridgeCmd(c *gc.C, devices []network.DeviceToBridge, bridgePrefix, filename, script string, dryRun bool, expected string) {
+func assertBridgeCmd(c *gc.C, devices []network.DeviceToBridge, bridgePrefix, filename, script string, dryRun bool, reconfigureDelay int, expected string) {
 	for _, python := range network.PythonInterpreters() {
-		cmd := network.BridgeCmd(devices, python, bridgePrefix, filename, script, dryRun)
+		cmd := network.BridgeCmd(devices, python, bridgePrefix, filename, script, dryRun, reconfigureDelay)
 		assertCmdResult(c, cmd, expected)
 	}
 }
 
-func assertENIBridgerError(c *gc.C, devices []network.DeviceToBridge, environ []string, timeout time.Duration, clock clock.Clock, bridgePrefix, filename string, dryRun bool, expected string) {
+func assertENIBridgerError(c *gc.C, devices []network.DeviceToBridge, environ []string, timeout time.Duration, clock clock.Clock, bridgePrefix, filename string, dryRun bool, reconfigureDelay int, expected string) {
 	for _, python := range network.PythonInterpreters() {
 		bridger := network.NewEtcNetworkInterfacesBridger(python, environ, clock, timeout, bridgePrefix, filename, dryRun)
-		err := bridger.Bridge(devices)
+		err := bridger.Bridge(devices, reconfigureDelay)
 		c.Assert(err, gc.NotNil)
 		c.Assert(err, gc.ErrorMatches, expected)
 	}
@@ -76,10 +76,11 @@ func (*BridgeSuite) TestBridgeCmdArgumentsNoBridgePrefixAndDryRun(c *gc.C) {
 		},
 	}
 	dryRun := true
-	assertBridgeCmd(c, devices, "", "/etc/network/interfaces", echoArgsScript, dryRun, `
+	assertBridgeCmd(c, devices, "", "/etc/network/interfaces", echoArgsScript, dryRun, 0, `
 --interfaces-to-bridge=ens3 ens4 bond0
 --activate
 --dry-run
+--reconfigure-delay=0
 /etc/network/interfaces
 `[1:])
 }
@@ -96,10 +97,11 @@ func (*BridgeSuite) TestBridgeCmdArgumentsNoNetBondReconfigureDelay(c *gc.C) {
 			DeviceName: "bond0",
 		},
 	}
-	assertBridgeCmd(c, devices, "", "/etc/network/interfaces", echoArgsScript, true, `
+	assertBridgeCmd(c, devices, "", "/etc/network/interfaces", echoArgsScript, true, 0, `
 --interfaces-to-bridge=ens3 ens4 bond0
 --activate
 --dry-run
+--reconfigure-delay=0
 /etc/network/interfaces
 `[1:])
 }
@@ -116,11 +118,12 @@ func (*BridgeSuite) TestBridgeCmdArgumentsWithBridgePrefixAndDryRun(c *gc.C) {
 			DeviceName: "bond0",
 		},
 	}
-	assertBridgeCmd(c, devices, "foo-", "/etc/network/interfaces", echoArgsScript, true, `
+	assertBridgeCmd(c, devices, "foo-", "/etc/network/interfaces", echoArgsScript, true, 0, `
 --interfaces-to-bridge=ens3 ens4 bond0
 --activate
 --bridge-prefix=foo-
 --dry-run
+--reconfigure-delay=0
 /etc/network/interfaces
 `[1:])
 }
@@ -137,10 +140,11 @@ func (*BridgeSuite) TestBridgeCmdArgumentsWithBridgePrefixWithoutDryRun(c *gc.C)
 			DeviceName: "bond0",
 		},
 	}
-	assertBridgeCmd(c, devices, "foo-", "/etc/network/interfaces", echoArgsScript, false, `
+	assertBridgeCmd(c, devices, "foo-", "/etc/network/interfaces", echoArgsScript, false, 0, `
 --interfaces-to-bridge=ens3 ens4 bond0
 --activate
 --bridge-prefix=foo-
+--reconfigure-delay=0
 /etc/network/interfaces
 `[1:])
 }
@@ -157,46 +161,44 @@ func (*BridgeSuite) TestBridgeCmdArgumentsWithoutBridgePrefixAndWithoutDryRun(c
 			DeviceName: "bond0",
 		},
 	}
-	assertBridgeCmd(c, devices, "", "/etc/network/interfaces", echoArgsScript, false, `
+	assertBridgeCmd(c, devices, "", "/etc/network/interfaces", echoArgsScript, false, 0, `
 --interfaces-to-bridge=ens3 ens4 bond0
 --activate
+--reconfigure-delay=0
 /etc/network/interfaces
 `[1:])
 }
 
 func (*BridgeSuite) TestBridgeCmdArgumentsWithNetBondReconfigureDelay(c *gc.C) {
 	devices := []network.DeviceToBridge{
 		network.DeviceToBridge{
-			DeviceName:              "ens3",
-			NetBondReconfigureDelay: 0,
+			DeviceName: "ens3",
 		},
 		network.DeviceToBridge{
-			DeviceName:              "ens4",
-			NetBondReconfigureDelay: 4,
+			DeviceName: "ens4",
 		},
 		network.DeviceToBridge{
-			DeviceName:              "bond0",
-			NetBondReconfigureDelay: 2,
+			DeviceName: "bond0",
 		},
 	}
-	assertBridgeCmd(c, devices, "", "/etc/network/interfaces", echoArgsScript, false, `
+	assertBridgeCmd(c, devices, "", "/etc/network/interfaces", echoArgsScript, false, 4, `
 --interfaces-to-bridge=ens3 ens4 bond0
 --activate
---net-bond-reconfigure-delay=4
+--reconfigure-delay=4
 /etc/network/interfaces
 `[1:])
 }
 
 func (*BridgeSuite) TestENIBridgerWithMissingFilenameArgument(c *gc.C) {
 	devices := []network.DeviceToBridge{}
 	expected := `(?s)bridgescript failed:.*(too few arguments|the following arguments are required: filename)\n`
-	assertENIBridgerError(c, devices, os.Environ(), 0, clock.WallClock, "br-", "", true, expected)
+	assertENIBridgerError(c, devices, os.Environ(), 0, clock.WallClock, "br-", "", true, 0, expected)
 }
 
 func (*BridgeSuite) TestENIBridgerWithEmptyDeviceNamesArgument(c *gc.C) {
 	devices := []network.DeviceToBridge{}
 	expected := `(?s)bridgescript failed:.*(too few arguments|no interfaces specified)\n`
-	assertENIBridgerError(c, devices, os.Environ(), 0, clock.WallClock, "br-", "testdata/non-existent-filename", true, expected)
+	assertENIBridgerError(c, devices, os.Environ(), 0, clock.WallClock, "br-", "testdata/non-existent-filename", true, 0, expected)
 }
 
 func (*BridgeSuite) TestENIBridgerWithNonExistentFile(c *gc.C) {
@@ -206,7 +208,7 @@ func (*BridgeSuite) TestENIBridgerWithNonExistentFile(c *gc.C) {
 		},
 	}
 	expected := `(?s).*(IOError|FileNotFoundError):.*No such file or directory: 'testdata/non-existent-file'\n`
-	assertENIBridgerError(c, devices, os.Environ(), 0, clock.WallClock, "br-", "testdata/non-existent-file", true, expected)
+	assertENIBridgerError(c, devices, os.Environ(), 0, clock.WallClock, "br-", "testdata/non-existent-file", true, 0, expected)
 }
 
 func (*BridgeSuite) TestENIBridgerWithTimeout(c *gc.C) {
@@ -217,7 +219,8 @@ func (*BridgeSuite) TestENIBridgerWithTimeout(c *gc.C) {
 	}
 	environ := os.Environ()
 	environ = append(environ, "ADD_JUJU_BRIDGE_SLEEP_PREAMBLE_FOR_TESTING=10")
-	assertENIBridgerError(c, devices, environ, 500*time.Millisecond, clock.WallClock, "br-", "testdata/non-existent-file", true, "bridgescript timed out after 500ms")
+	expected := "bridgescript timed out after 500ms"
+	assertENIBridgerError(c, devices, environ, 500*time.Millisecond, clock.WallClock, "br-", "testdata/non-existent-file", true, 0, expected)
 }
 
 func (*BridgeSuite) TestENIBridgerWithDryRun(c *gc.C) {
@@ -228,7 +231,7 @@ func (*BridgeSuite) TestENIBridgerWithDryRun(c *gc.C) {
 	}
 	for _, python := range network.PythonInterpreters() {
 		bridger := network.NewEtcNetworkInterfacesBridger(python, os.Environ(), clock.WallClock, 0, "", "testdata/interfaces", true)
-		err := bridger.Bridge(devices)
+		err := bridger.Bridge(devices, 0)
 		c.Assert(err, gc.IsNil)
 	}
 }

network/bridgescript.go

@@ -397,7 +397,7 @@ def arg_parser():
     parser.add_argument('--interfaces-to-bridge', help="interfaces to bridge; space delimited", type=str, required=True)
     parser.add_argument('--dry-run', help="dry run, no activation", action='store_true', default=False, required=False)
     parser.add_argument('--bridge-name', help="bridge name", type=str, required=False)
-    parser.add_argument('--net-bond-reconfigure-delay', help="delay in seconds before raising bonded interfaces", type=int, required=False, default=30)
+    parser.add_argument('--reconfigure-delay', help="delay in seconds before raising interfaces", type=int, required=False, default=10)
     parser.add_argument('filename', help="interfaces(5) based filename")
     return parser
 
@@ -444,22 +444,8 @@ def main(args):
             print_stanzas(stanzas, f)
             f.close()
 
-    # On configurations that have bonds in 802.3ad mode there is a
-    # race condition betweeen an immediate ifdown then ifup.
-    #
-    # On the h/w I have a 'sleep 0.1' is sufficient but to accommodate
-    # other setups we arbitrarily choose something larger. We don't
-    # want to massively slow bootstrap down but, equally, 0.1 may be
-    # too small for other configurations.
-
-    for s in stanzas:
-        if s.is_logical_interface and s.iface.is_bonded:
-            print("working around https://bugs.launchpad.net/ubuntu/+source/ifenslave/+bug/1269921")
-            print("working around https://bugs.launchpad.net/juju-core/+bug/1594855")
-            if args.dry_run and args.dry_run > 0:
-                shell_cmd("sleep {}".format(args.net_bond_reconfigure_delay, dry_run=args.dry_run))
-            break
-
+    if args.reconfigure_delay and args.reconfigure_delay > 0 :
+        shell_cmd("sleep {}".format(args.reconfigure_delay), dry_run=args.dry_run)
     shell_cmd("cat {}".format(args.filename), dry_run=args.dry_run)
     shell_cmd("ifup --exclude=lo --interfaces={} -a".format(args.filename), dry_run=args.dry_run)
     shell_cmd("ip -d link show", dry_run=args.dry_run)