Join GitHub today
GitHub is home to over 20 million developers working together to host and review code, manage projects, and build software together.
2.1.1 static routes for containers #7034
Conversation
jameinel
added some commits
Feb 22, 2017
|
In doing my own QA, this doesn't seem to actually set routes inside containers, I have to do some more digging. |
jameinel
closed this
Feb 24, 2017
|
I did track down the bug. We are creating data correctly in apiserver/provisioner, but the NetworkConfigFromInterfaceInfo is losing the information. |
|
Found the issue, reopening. It does seem to work for LXD containers. I'm seeing that /etc/network/interfaces-juju is getting written but isn't being made active as part of a KVM container, but that needs to be debugged separately. |
jameinel
reopened this
Feb 25, 2017
|
It appears the /e/n/i we are writing in 2.1 is really just wrong for KVM containers: The /e/n/i is always writing 'eth0' as the first device, when the first device is 'ens3' on this machine. Anyway, that has nothing to do with this patch, so I'm not blocking on it. |
jameinel
added some commits
Feb 25, 2017
jameinel
changed the base branch from
develop
to
2.1
Feb 27, 2017
axw
approved these changes
Feb 27, 2017
LGTM. Not too keen on swallowing the error. Can we either be more specific about which errors we (need to) swallow, with a comment explaining that?
| + // Make sure the CIDR is actually a CIDR not just an IP or hostname | ||
| + destinationIP, _, err := net.ParseCIDR(r.DestinationCIDR) | ||
| + if err != nil { | ||
| + return errors.Errorf("DestinationCIDR not valid: %v", err) |
| + return errors.Errorf("GatewayIP is not a valid IP address: %q", r.GatewayIP) | ||
| + } | ||
| + if r.Metric < 0 { | ||
| + return errors.Errorf("Metric is not a non-negative integer: %d", r.Metric) |
| @@ -276,6 +276,19 @@ func (env *maasEnviron) deviceInterfaceInfo2(deviceID string, nameToParentName m | ||
| NoAutoStart: !nic.Enabled(), | ||
| ParentInterfaceName: nameToParentName[nic.Name()], | ||
| } | ||
| + for _, link := range nic.Links() { | ||
| + subnet := link.Subnet() | ||
| + if subnet != nil { |
| + subnetToStaticRoutes := make(map[string][]gomaasapi.StaticRoute) | ||
| + staticRoutes, err := env.maasController.StaticRoutes() | ||
| + if err != nil { | ||
| + // Warning? |
jameinel
Feb 27, 2017
Owner
So my original thought was that I wasn't sure what versions of MAAS supported static-routes as an API, though the docs do seems to say that 2.0 onward support it.
That said, if you aren't using static routes, all the other work is still valid, so an error in accessing them didn't seem like it should be fatal vs anything else creating a container. But errors here should be rare, so I'm fine just propagating them up. I switched this to return errors.Annotate() which handles the next item as well.
| + if err != nil { | ||
| + // Warning? | ||
| + logger.Debugf("got an error looking for static-routes: %v", err) | ||
| + } else if err == nil { |
jameinel
reviewed
Feb 27, 2017
We don't need to swallow any errors that I'm aware of, I was more being cautious around requiring a new api that people may not be using. However, support should be there regardless.
| + // Make sure the CIDR is actually a CIDR not just an IP or hostname | ||
| + destinationIP, _, err := net.ParseCIDR(r.DestinationCIDR) | ||
| + if err != nil { | ||
| + return errors.Errorf("DestinationCIDR not valid: %v", err) |
| + return errors.Errorf("GatewayIP is not a valid IP address: %q", r.GatewayIP) | ||
| + } | ||
| + if r.Metric < 0 { | ||
| + return errors.Errorf("Metric is not a non-negative integer: %d", r.Metric) |
| @@ -276,6 +276,19 @@ func (env *maasEnviron) deviceInterfaceInfo2(deviceID string, nameToParentName m | ||
| NoAutoStart: !nic.Enabled(), | ||
| ParentInterfaceName: nameToParentName[nic.Name()], | ||
| } | ||
| + for _, link := range nic.Links() { | ||
| + subnet := link.Subnet() | ||
| + if subnet != nil { |
| + subnetToStaticRoutes := make(map[string][]gomaasapi.StaticRoute) | ||
| + staticRoutes, err := env.maasController.StaticRoutes() | ||
| + if err != nil { | ||
| + // Warning? |
jameinel
Feb 27, 2017
Owner
So my original thought was that I wasn't sure what versions of MAAS supported static-routes as an API, though the docs do seems to say that 2.0 onward support it.
That said, if you aren't using static routes, all the other work is still valid, so an error in accessing them didn't seem like it should be fatal vs anything else creating a container. But errors here should be rare, so I'm fine just propagating them up. I switched this to return errors.Annotate() which handles the next item as well.
| + if err != nil { | ||
| + // Warning? | ||
| + logger.Debugf("got an error looking for static-routes: %v", err) | ||
| + } else if err == nil { |
|
$$merge$$ |
|
Status: merge request accepted. Url: http://juju-ci.vapour.ws:8080/job/github-merge-juju |
|
Build failed: Generating tarball failed |
|
$$merge$$ |
|
Status: merge request accepted. Url: http://juju-ci.vapour.ws:8080/job/github-merge-juju |
|
Build failed: Tests failed |
|
$$merge$$ |
|
Status: merge request accepted. Url: http://juju-ci.vapour.ws:8080/job/github-merge-juju |
jameinel commentedFeb 24, 2017
Description of change
This change allows Juju to propagate Static Route information that MAAS declares into containers. It depends on this patch to gomaasapi: juju/gomaasapi#63. (I updated dependencies.tsv for my version of the code, but we should update dependencies to match the final merge.) The other changes to dependencies.tsv are just reordering because of how the file was generated.
We use the Static Routes API https://docs.ubuntu.com/maas/2.0/en/api#static-route to find what routes are needed, and apply them based on their Source subnet.
We add a post-up and a pre-down rule that uses 'ip route add'. This differs slightly from MAAS and Curtin that use "route add", but it means we can use CIDR instead of netmask.
http://bazaar.launchpad.net/~curtin-dev/curtin/trunk/view/head:/curtin/net/__init__.py#L380
QA steps
Using MAAS create a static route for one of your subnets (you can do so via the web ui by looking at the source subnet). You'll need to create destination subnets for it to route to. Afterwards, deploy a node, and you should see that "ip route show" lists that static route. You should then be able to deploy a container onto that machine using the same space. It should have the same route. Without this patch (stock juju 2.1.0) it should not.
Documentation changes
Potentially we should document static routes with containers, but it feels a bit like people using static routes just expect it to work.
Bug reference
lp:1653708