Alias interface set as an alias of the bridge #5790

Merged
merged 1 commit into from Jul 30, 2016

Conversation

Projects
None yet
4 participants
Contributor

freyes commented Jul 13, 2016

When the intefaces file is regenerated alias interfaces are left as
they are, this makes IPv6 aliases unusable.

Example input:

iface eth0:1 iface static
    address 192.168.122.2/24

Example output:

iface br-eth0:1 iface static
    address 192.168.122.2/24

Fixes LP:#1602716

Contributor

frobware commented Jul 13, 2016

The change is fine but I'm concerned about the semantics. What happens with LXD containers because we create interfaces for all bridged interfaces in the container. Have you tried launching a container (juju add-machine lxd:0) with this change?

Contributor

freyes commented Jul 13, 2016

On Wed, 13 Jul 2016 09:19:38 -0700
Andrew McDermott notifications@github.com wrote:

The change is fine but I'm concerned about the semantics. What
happens with LXD containers because we create interfaces for all
bridged interfaces in the container. Have you tried launching a
container (juju add-machine lxd:0) with this change?

I did not, I'll give it a try.

Contributor

frobware commented Jul 13, 2016

For this change you would also need to update the tests that are in bridgescript_test.go.

Contributor

freyes commented Jul 13, 2016

On Wed, 13 Jul 2016 13:36:53 -0700
Andrew McDermott notifications@github.com wrote:

For this change you would also need to update the tests that are in
bridgescript_test.go.

do I need to regenerate bridgescript.go too? is that done by the
building process?

Contributor

freyes commented Jul 13, 2016

Have you tried launching a container (juju add-machine lxd:0) with this change?

I tested it, I found no problems with the containers and the IP addresses in the host remained functional.

Contributor

frobware commented Jul 14, 2016

The way to test is:

cd .../juju/juju/provider/maas
make (any time you change add-juju-bridge.py)
go test -check.f bridgeConfigSuite
Alias interface set as an alias of the bridge
When the intefaces file is regenerated alias interfaces are left as
they are, this makes IPv6 aliases unusable.

Example input:

  iface eth0:1 iface static
      address 192.168.122.2/24

Example output:

  iface br-eth0:1 iface static
      address 192.168.122.2/24

Fixes [LP:#1602716](https://bugs.launchpad.net/juju-core/+bug/1602716)
Contributor

freyes commented Jul 15, 2016

@frobware , I pushed an updated version of the patch.

Changes:

  • Updated tests
  • Previously aliases were blindly moved to the bridge (bridge_name), so now when there is an alias for an interface that add-juju-bridge.py didn't modify, then related aliases aren't modified neither. To achieve this the method _connect_aliases() was added, it set a reference in the alias to the related alias Stanza() (e.g. eth0:1 -> eth0)
  • And the patch was rebased to fix a minor conflict.
Contributor

frobware commented Jul 15, 2016

Thank you. Will look over the patch and with some testing will merge as appropriate. Thanks for this.

On 15 Jul 2016, at 15:44, Felipe Reyes notifications@github.com wrote:

@frobware , I pushed an updated version of the patch.

Changes:

Updated tests
Previously aliases were blindly moved to the bridge (bridge_name), so now when there is an alias for an interface that add-juju-bridge.py didn't modify, then related aliases aren't modified neither. To achieve this the method _connect_aliases() was added, it set a reference in the alias to the related alias Stanza() (e.g. eth0:1 -> eth0)
And the patch was rebased to fix a minor conflict.

You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub, or mute the thread.

Contributor

frobware commented Jul 18, 2016

I'm sorry but I didn't get to to this today. We talked with Jay today and now understand why aliases are as you have them in your patch. Will try to get this merged tomorrow. Thanks.

Contributor

freyes commented Jul 28, 2016

Hi @frobware , is this patch anywhere close to the top of your list? :)

Contributor

frobware commented Jul 29, 2016

I have been testing this today on xenial. No problems. LGTM.

$$merge$$

Contributor

jujubot commented Jul 30, 2016

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

@jujubot jujubot merged commit ac6b6a3 into juju:master Jul 30, 2016

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