/juju

network: fix bridge script for multiple stanzas #6962

Merged
merged 1 commit into from Feb 10, 2017

Conversation

Reviewers

@jameinel jameinel

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
3 participants
@axw @jujubot @jameinel
@axw
Member

axw commented Feb 10, 2017

Description of change

The add-juju-bridge.py script was not coping
with /etc/network/interfaces files containing
multiple stanzas for the same logical interface.
For example, if an interface "eth0" has two
"iface" stanzas, one for each of inet and inet6,
the script would generate two bridge ifaces
each with bridge_ports set to eth0. Only one
may have bridge_ports.

The program has been restructured a little so
that a LogicalInterface contains a collection
of stanzas. This better matches the concepts
in /e/n/i.

At the same time, we now only add bridge_ports
to the first logical interface stanza for a
bridged interface.

QA steps

  1. juju bootstrap rackspace
  2. juju add-machine lxc
  3. confirm 0/lxc/0 is provisioned successfully
  4. confirm you can "juju ssh 0" successfully

Documentation changes

None.

Bug reference

At least partially fixes https://bugs.launchpad.net/juju/+bug/1650304
@jameinel

jameinel approved these changes Feb 10, 2017
View changes

Some comments but as we likely need this for 2.1 we should probably land it.

network/add-juju-bridge.py
+ self.bond_master_options = False
+ self.bridge_ports = []
+
+ for s in stanzas:
@jameinel

jameinel Feb 10, 2017

Owner

can you move this into a _parse_stanzas helper?

@axw

axw Feb 10, 2017

Member

Done

network/add-juju-bridge.py
- return True, words[1:]
- return False, []
+ if not self.is_bonded:
+ self.is_bonded = any(("bond-" in x for x in s.options))
@jameinel

jameinel Feb 10, 2017

Owner

This seems odd vs X.startswith("bond-") I wonder why

@axw

axw Feb 10, 2017

Member

Agreed. I've changed it to startswith.

network/add-juju-bridge.py
if self.is_physical_interface:
self.phy = PhysicalInterface(definition)
def __str__(self):
return self.definition
+ def has_option(self, options):
@jameinel

jameinel Feb 10, 2017

Owner

Isn't this has_any_option? (Vs has_all_options)

@axw

axw Feb 10, 2017

Member

Indeed. I just moved it but I'll rename it.

network/add-juju-bridge.py
+ def _collect_logical_interfaces(self):
+ """
+ Collects the parsed stanzas related to logical interfaces,
+ returning a list of LogicalInterfaces.
@jameinel

jameinel Feb 10, 2017

Owner

Doesn't return anything. But populates logical interfaces

@axw

axw Feb 10, 2017

Member

Fixed

@axw
network: fix bridge script for multiple stanzas 
The add-juju-bridge.py script was not coping
with /etc/network/interfaces files containing
multiple stanzas for the same logical interface.
The program has been restructured a little so
that a LogicalInterface contains a collection
of stanzas.

At the same time, we now only add bridge_ports
to the first logical interface stanza for a
bridged interface. Adding it to all of them
breaks networking.

At least partially fixes https://bugs.launchpad.net/juju/+bug/1650304
ad953df
@axw
Member

axw commented Feb 10, 2017

$$merge$$
@jujubot
Contributor

jujubot commented Feb 10, 2017

Status: merge request accepted. Url: http://juju-ci.vapour.ws:8080/job/github-merge-juju

@jujubot jujubot merged commit 77ab102 into juju:2.1 Feb 10, 2017

frobware pushed a commit to frobware/juju that referenced this pull request Feb 10, 2017

Go implementation of bridge script 
This is a Go implementation of the Python-based add-juju-bridge script
that I started in my spare some time ago. I have spent some time
tidying it up this week and the test coverage is now approaching
100% (currently 98%).

I have deliberately put this into its own package --
juju/network/debinterfaces -- so that it doesn't clash with any of the
existing bridging machinations.

This parses and handles the existing test cases with the exception of
Andrew's recent PR #6962 to handle multiple iface stanzas against a
physical interface. I guess I (and many people) were not using IPv6
that much given how long the Python logic has been around.

The juju-bridge(1) command is an example of how to drive the Go
implementation and interface. I have historically found it imperative
that you are able to give an end-user the script that Juju uses and
ask them to run it on their hardware which quite often finds new and
interesting ways that bridging fails.

I have focused much of my recent effort on the test cases and the
general logic in the package; It was only this week I created
juju-bridge(1) and as a result that has had limited exposure. Having
said that, the logic it relies on is almost identical to the way
dynamic bridging invocation is done today.

I think the Go logic is much better than the accreted wisdom (and
mess) that is in the Python script. And having "ported" it to Go
there's also a lot of cruft that could be removed from the Python
version should you not want this PR.

Good luck and sorry for the mess that comes with bridging...

Signed-off-by: Andrew McDermott <andrew.mcdermott@frobware.com>
56461ed

This was referenced Feb 10, 2017

Merged
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment