From fdcf0af7b28d7c293b5e818edb7c09ab9ffdccd6 Mon Sep 17 00:00:00 2001 From: Andrew McDermott Date: Thu, 26 Jan 2017 09:59:44 +0000 Subject: [PATCH] Extend network/bridge.go tests to python2 and python3 --- network/bridge.go | 64 +++++++------ network/bridge_test.go | 89 ++++++++----------- network/export_test.go | 10 ++- .../provisioner/container_initialisation.go | 9 +- 4 files changed, 83 insertions(+), 89 deletions(-) diff --git a/network/bridge.go b/network/bridge.go index 19742da3e6f..a445149c9b5 100644 --- a/network/bridge.go +++ b/network/bridge.go @@ -23,18 +23,19 @@ type Bridger interface { } type etcNetworkInterfacesBridger struct { - BridgePrefix string - Clock clock.Clock - DryRun bool - Environ []string - Filename string - Timeout time.Duration + PythonInterpreter string + BridgePrefix string + Clock clock.Clock + DryRun bool + Environ []string + Filename string + Timeout time.Duration } var _ Bridger = (*etcNetworkInterfacesBridger)(nil) -// bestPythonVersion returns a string to the best python interpreter -// found. +// pythonInterpreters returns a slice of all the Python interpreters +// found on the machine. // // For ubuntu series < xenial we prefer python2 over python3 as we // don't want to invalidate lots of testing against known cloud-image @@ -49,22 +50,23 @@ var _ Bridger = (*etcNetworkInterfacesBridger)(nil) // 16.04 xenial: python 3 only (3.5.1) // // going forward: python 3 only -func bestPythonVersion() string { +func pythonInterpreters() []string { + result := []string{} for _, version := range []string{ "/usr/bin/python2", "/usr/bin/python3", "/usr/bin/python", } { if _, err := os.Stat(version); err == nil { - return version + result = append(result, version) } } - return "" + return result } func (b *etcNetworkInterfacesBridger) Bridge(devices []DeviceToBridge) error { - cmd := bridgeCmd(devices, b.BridgePrefix, b.Filename, BridgeScriptPythonContent, b.DryRun) - infoCmd := bridgeCmd(devices, b.BridgePrefix, b.Filename, "", b.DryRun) + cmd := bridgeCmd(devices, b.PythonInterpreter, b.BridgePrefix, b.Filename, BridgeScriptPythonContent, b.DryRun) + infoCmd := bridgeCmd(devices, b.PythonInterpreter, b.BridgePrefix, b.Filename, "", b.DryRun) logger.Infof("bridgescript command=%s", infoCmd) result, err := runCommand(cmd, b.Environ, b.Clock, b.Timeout) if err != nil { @@ -84,7 +86,7 @@ func (b *etcNetworkInterfacesBridger) Bridge(devices []DeviceToBridge) error { return nil } -func bridgeCmd(devices []DeviceToBridge, bridgePrefix, filename, pythonScript string, dryRun bool) string { +func bridgeCmd(devices []DeviceToBridge, pythonInterpreter, bridgePrefix, filename, pythonScript string, dryRun bool) string { dryRunOption := "" if bridgePrefix != "" { @@ -116,7 +118,7 @@ func bridgeCmd(devices []DeviceToBridge, bridgePrefix, filename, pythonScript st %s EOF `[1:], - bestPythonVersion(), + pythonInterpreter, strings.Join(deviceNames, " "), bridgePrefix, dryRunOption, @@ -125,19 +127,25 @@ EOF pythonScript) } -// NewEtcNetworkInterfacesBridger returns a Bridger that can parse -// /etc/network/interfaces and create new stanzas to bridge existing -// interfaces. -// -// TODO(frobware): We shouldn't expose DryRun; once we implement the -// Python-based bridge script in Go this interface can change. -func NewEtcNetworkInterfacesBridger(environ []string, clock clock.Clock, timeout time.Duration, bridgePrefix, filename string, dryRun bool) Bridger { +func newEtcNetworkInterfacesBridger(pythonInterpreter string, environ []string, clock clock.Clock, timeout time.Duration, bridgePrefix, filename string, dryRun bool) Bridger { return &etcNetworkInterfacesBridger{ - BridgePrefix: bridgePrefix, - Clock: clock, - DryRun: dryRun, - Environ: environ, - Filename: filename, - Timeout: timeout, + PythonInterpreter: pythonInterpreter, + BridgePrefix: bridgePrefix, + Clock: clock, + DryRun: dryRun, + Environ: environ, + Filename: filename, + Timeout: timeout, + } +} + +// DefaultEtcNetworkInterfacesBridger returns a Bridger instance that +// can parse an interfaces(5) to transform existing devices into +// bridged devices. +func DefaultEtcNetworkInterfacesBridger(timeout time.Duration, bridgePrefix, filename string) (Bridger, error) { + pythonVersions := pythonInterpreters() + if len(pythonVersions) == 0 { + return nil, errors.Errorf("no python interpreter found") } + return newEtcNetworkInterfacesBridger(pythonVersions[0], os.Environ(), clock.WallClock, timeout, bridgePrefix, filename, false), nil } diff --git a/network/bridge_test.go b/network/bridge_test.go index 7a2b5e15395..9eea13d650e 100644 --- a/network/bridge_test.go +++ b/network/bridge_test.go @@ -47,6 +47,22 @@ 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) { + for _, python := range network.PythonInterpreters() { + cmd := network.BridgeCmd(devices, python, bridgePrefix, filename, script, dryRun) + 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) { + for _, python := range network.PythonInterpreters() { + bridger := network.NewEtcNetworkInterfacesBridger(python, environ, clock, timeout, bridgePrefix, filename, dryRun) + err := bridger.Bridge(devices) + c.Assert(err, gc.NotNil) + c.Assert(err, gc.ErrorMatches, expected) + } +} + func (*BridgeSuite) TestBridgeCmdArgumentsNoBridgePrefixAndDryRun(c *gc.C) { devices := []network.DeviceToBridge{ network.DeviceToBridge{ @@ -59,8 +75,8 @@ func (*BridgeSuite) TestBridgeCmdArgumentsNoBridgePrefixAndDryRun(c *gc.C) { DeviceName: "bond0", }, } - cmd := network.BridgeCmd(devices, "", "/etc/network/interfaces", echoArgsScript, true) - assertCmdResult(c, cmd, ` + dryRun := true + assertBridgeCmd(c, devices, "", "/etc/network/interfaces", echoArgsScript, dryRun, ` --interfaces-to-bridge=ens3 ens4 bond0 --activate --dry-run @@ -80,8 +96,7 @@ func (*BridgeSuite) TestBridgeCmdArgumentsNoBondReconfigureDelay(c *gc.C) { DeviceName: "bond0", }, } - cmd := network.BridgeCmd(devices, "", "/etc/network/interfaces", echoArgsScript, true) - assertCmdResult(c, cmd, ` + assertBridgeCmd(c, devices, "", "/etc/network/interfaces", echoArgsScript, true, ` --interfaces-to-bridge=ens3 ens4 bond0 --activate --dry-run @@ -101,8 +116,7 @@ func (*BridgeSuite) TestBridgeCmdArgumentsWithBridgePrefixAndDryRun(c *gc.C) { DeviceName: "bond0", }, } - cmd := network.BridgeCmd(devices, "foo-", "/etc/network/interfaces", echoArgsScript, true) - assertCmdResult(c, cmd, ` + assertBridgeCmd(c, devices, "foo-", "/etc/network/interfaces", echoArgsScript, true, ` --interfaces-to-bridge=ens3 ens4 bond0 --activate --bridge-prefix=foo- @@ -123,8 +137,7 @@ func (*BridgeSuite) TestBridgeCmdArgumentsWithBridgePrefixWithoutDryRun(c *gc.C) DeviceName: "bond0", }, } - cmd := network.BridgeCmd(devices, "foo-", "/etc/network/interfaces", echoArgsScript, false) - assertCmdResult(c, cmd, ` + assertBridgeCmd(c, devices, "foo-", "/etc/network/interfaces", echoArgsScript, false, ` --interfaces-to-bridge=ens3 ens4 bond0 --activate --bridge-prefix=foo- @@ -144,8 +157,7 @@ func (*BridgeSuite) TestBridgeCmdArgumentsWithoutBridgePrefixAndWithoutDryRun(c DeviceName: "bond0", }, } - cmd := network.BridgeCmd(devices, "", "/etc/network/interfaces", echoArgsScript, false) - assertCmdResult(c, cmd, ` + assertBridgeCmd(c, devices, "", "/etc/network/interfaces", echoArgsScript, false, ` --interfaces-to-bridge=ens3 ens4 bond0 --activate /etc/network/interfaces @@ -167,8 +179,7 @@ func (*BridgeSuite) TestBridgeCmdArgumentsWithBondReconfigureDelay(c *gc.C) { BondReconfigureDelay: 2, }, } - cmd := network.BridgeCmd(devices, "", "/etc/network/interfaces", echoArgsScript, false) - assertCmdResult(c, cmd, ` + assertBridgeCmd(c, devices, "", "/etc/network/interfaces", echoArgsScript, false, ` --interfaces-to-bridge=ens3 ens4 bond0 --activate --bond-reconfigure-delay=4 @@ -177,26 +188,15 @@ func (*BridgeSuite) TestBridgeCmdArgumentsWithBondReconfigureDelay(c *gc.C) { } func (*BridgeSuite) TestENIBridgerWithMissingFilenameArgument(c *gc.C) { - devices := []network.DeviceToBridge{ - network.DeviceToBridge{ - DeviceName: "ens3", - }, - network.DeviceToBridge{ - DeviceName: "ens4", - }, - network.DeviceToBridge{ - DeviceName: "bond0", - }, - } - bridger := network.NewEtcNetworkInterfacesBridger(os.Environ(), clock.WallClock, 0, "", "", true) - err := bridger.Bridge(devices) - c.Assert(err, gc.ErrorMatches, `(?s)bridgescript failed:.*too few arguments\n`) + 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) } func (*BridgeSuite) TestENIBridgerWithEmptyDeviceNamesArgument(c *gc.C) { - bridger := network.NewEtcNetworkInterfacesBridger(os.Environ(), clock.WallClock, 0, "", "", true) - err := bridger.Bridge([]network.DeviceToBridge{}) - c.Assert(err, gc.ErrorMatches, `(?s)bridgescript failed:.*too few arguments\n`) + 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) } func (*BridgeSuite) TestENIBridgerWithNonExistentFile(c *gc.C) { @@ -204,17 +204,9 @@ func (*BridgeSuite) TestENIBridgerWithNonExistentFile(c *gc.C) { network.DeviceToBridge{ DeviceName: "ens3", }, - network.DeviceToBridge{ - DeviceName: "ens4", - }, - network.DeviceToBridge{ - DeviceName: "bond0", - }, } - bridger := network.NewEtcNetworkInterfacesBridger(os.Environ(), clock.WallClock, 0, "", "testdata/non-existent-file", true) - err := bridger.Bridge(devices) - c.Assert(err, gc.NotNil) - c.Check(err, gc.ErrorMatches, `(?s).*IOError:.*No such file or directory: 'testdata/non-existent-file'\n`) + 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) } func (*BridgeSuite) TestENIBridgerWithTimeout(c *gc.C) { @@ -222,19 +214,10 @@ func (*BridgeSuite) TestENIBridgerWithTimeout(c *gc.C) { network.DeviceToBridge{ DeviceName: "ens3", }, - network.DeviceToBridge{ - DeviceName: "ens4", - }, - network.DeviceToBridge{ - DeviceName: "bond0", - }, } environ := os.Environ() environ = append(environ, "ADD_JUJU_BRIDGE_SLEEP_PREAMBLE_FOR_TESTING=10") - bridger := network.NewEtcNetworkInterfacesBridger(environ, clock.WallClock, 500*time.Millisecond, "", "testdata/non-existent-file", true) - err := bridger.Bridge(devices) - c.Assert(err, gc.NotNil) - c.Check(err, gc.ErrorMatches, `bridgescript timed out after 500ms`) + assertENIBridgerError(c, devices, environ, 500*time.Millisecond, clock.WallClock, "br-", "testdata/non-existent-file", true, "bridgescript timed out after 500ms") } func (*BridgeSuite) TestENIBridgerWithDryRun(c *gc.C) { @@ -243,7 +226,9 @@ func (*BridgeSuite) TestENIBridgerWithDryRun(c *gc.C) { DeviceName: "ens123", }, } - bridger := network.NewEtcNetworkInterfacesBridger(os.Environ(), clock.WallClock, 0, "", "testdata/interfaces", true) - err := bridger.Bridge(devices) - c.Assert(err, gc.IsNil) + for _, python := range network.PythonInterpreters() { + bridger := network.NewEtcNetworkInterfacesBridger(python, os.Environ(), clock.WallClock, 0, "", "testdata/interfaces", true) + err := bridger.Bridge(devices) + c.Assert(err, gc.IsNil) + } } diff --git a/network/export_test.go b/network/export_test.go index 2db754806f9..2fbc01a944e 100644 --- a/network/export_test.go +++ b/network/export_test.go @@ -4,8 +4,10 @@ package network var ( - NetLookupIP = &netLookupIP - NetListen = &netListen - RunCommand = runCommand - BridgeCmd = bridgeCmd + NetLookupIP = &netLookupIP + NetListen = &netListen + RunCommand = runCommand + BridgeCmd = bridgeCmd + PythonInterpreters = pythonInterpreters + NewEtcNetworkInterfacesBridger = newEtcNetworkInterfacesBridger ) diff --git a/worker/provisioner/container_initialisation.go b/worker/provisioner/container_initialisation.go index 73dd5765a29..2b891a9676d 100644 --- a/worker/provisioner/container_initialisation.go +++ b/worker/provisioner/container_initialisation.go @@ -5,7 +5,6 @@ package provisioner import ( "fmt" - "os" "sync/atomic" "time" @@ -208,10 +207,10 @@ func (cs *ContainerSetup) getContainerArtifacts( return nil, nil, nil, err } - bridger := network.NewEtcNetworkInterfacesBridger(os.Environ(), clock.WallClock, - activateBridgesTimeout, instancecfg.DefaultBridgePrefix, systemNetworkInterfacesFile, - false, // --dry-run - ) + bridger, err := network.DefaultEtcNetworkInterfacesBridger(activateBridgesTimeout, instancecfg.DefaultBridgePrefix, systemNetworkInterfacesFile) + if err != nil { + return nil, nil, nil, err + } switch containerType { case instance.KVM: