Join GitHub today
GitHub is home to over 20 million developers working together to host and review code, manage projects, and build software together.
provider/maas: conditionally run ifdown/up #6654
Conversation
| @@ -368,7 +376,10 @@ def shell_cmd(s): | ||
| return [out, err, p.returncode] | ||
| -def print_shell_cmd(s, verbose=True, exit_on_error=False): | ||
| +def print_shell_cmd(s, verbose=True, exit_on_error=False, dryrun=False): |
voidspace
Dec 5, 2016
Not convinced this is the right function name. It is running the shell command and only sometimes printing it.
| @@ -454,15 +469,15 @@ def main(args): | ||
| 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") | ||
| - print_shell_cmd("sleep 3") | ||
| + print_shell_cmd("sleep 3", dryrun=args.dryrun) |
added some commits
Dec 5, 2016
jameinel
approved these changes
Dec 6, 2016
I had one comment about how we validate the actual text for the bridge script output, otherwise LGTM.
| @@ -395,6 +393,7 @@ def arg_parser(): | ||
| parser.add_argument('--one-time-backup', help='A one time backup of filename', action='store_true', default=True, required=False) | ||
| parser.add_argument('--activate', help='activate new configuration', action='store_true', default=False, required=False) | ||
| parser.add_argument('--interfaces-to-bridge', help="interfaces to bridge; space delimited", type=str, required=True) | ||
| + parser.add_argument('--dryrun', help="dry run, no activation", action='store_true', default=False, required=False) |
| +func (s *bridgeConfigSuite) TestBridgeScriptWithoutBondedInterfaceSingle(c *gc.C) { | ||
| + bridgePrefix := "TestBridgeScriptWithoutBondedInterfaceSingle" | ||
| + interfacesToBridge := []string{"eth0"} | ||
| + expectedOutput := s.dryRunExpectedOutputHelper(false, false, bridgePrefix, interfacesToBridge) |
jameinel
Dec 6, 2016
Owner
This sort of testing bothers me a little bit, as it feels a bit like a "assert my helper code matches my generating code", rather than just seeing the actual config
s.dryRunExpectedOutput(false, false, bridgePrefix, []string{"eth0"},
actual string of the new /etc/network/interfaces)
Gives people an actual visual indication of what they think they should expect. And a little bit more of a human sanity check.
I've heard other opinions on this, so I wouldn't block on it. But I like seeing the concrete string.
|
$$merge$$ |
|
Status: merge request accepted. Url: http://juju-ci.vapour.ws:8080/job/github-merge-juju |
frobware commentedDec 5, 2016
If the nett result of bridging interfaces results in nothing-to-do,
because they are already bridged, then don't run ifdown/up
unnecessarily.
This state can exist in MAAS 2.1 because you now have the option of
bridging interfaces ahead of deployment or bootstrap. And as we move
to dynamic bridging in Juju we want to invoke this script on an
interface on an ad-hoc basis but don't want it to do anything if that
interface is already bridged.
QA steps:
On a machine run:
$ sudo ./add-juju-bridge.py /etc/network/interfaces
--activate
--interfaces-to-bridge=enp1s0f2
And note that enp1s0f2 gets bridged.
Verify with:
$ brctl show
Run the add-juju-bridge.py script again with the same arguments and
now you'll see no action taken as it is already bridged as
br-enp1s0f2.
I also configured a node in MAAS 2.1 that had two bridged NICs;
bootstrapping a node doesn't additionally run ifdown/up because
there's nothing to do.