Join GitHub today
GitHub is home to over 20 million developers working together to host and review code, manage projects, and build software together.
Fix lxd ipv6 #6916
Conversation
perrito666
added some commits
Feb 1, 2017
|
My test output with this PR: |
stgraber
commented
Feb 4, 2017
|
Looks reasonable to me, though I'd recommend you only check if "address family not supported by protocol" is in the error string rather than matching the whole thing as there's no guarantee we won't be changing the error prefix for this in a future LXD bugfix release. |
jameinel
requested changes
Feb 5, 2017
A couple small changes.
I'm ok in principle, though I'd really like some sort of test so that we have a chance of noticing when something in the environment changes an error string (Go, LXD, or something else.)
| @@ -65,6 +66,8 @@ func IsRunningLocally() (bool, error) { | ||
| return running, nil | ||
| } | ||
| +const errIPV6NotSupported = `cannot listen on https socket: listen tcp \[::\]:\d*: socket: address family not supported by protocol` |
jameinel
Feb 5, 2017
Owner
I don't have a particularly better regex to use. This feels a little brittle (version of Go, version of LXD API, platform Trusty vs Xenial, etc.)
Can we get some sort of Functional test so that we have an idea when this string is no longer valid and can update it?
stgraber
Feb 5, 2017
The "socket: address family not supported by protocol" part is effectively considered Linux API (glibc) and so is very unlikely to change so long as LXD doesn't mask the error.
The rest of the string comes from a mix of underlying golang packages and LXD, and I'd strongly recommend not relying on that part. So checking if "socket: address family not supported by protocol" is part of the error string should work fine and be much more reliable.
| + err := client.SetServerConfig("core.https_address", "[::]") | ||
| + | ||
| + if err != nil { | ||
| + // if the error hints that the problem might be a protocol unsopported |
| + cause := errors.Cause(err) | ||
| + matched, merr := regexp.MatchString(errIPV6NotSupported, cause.Error()) | ||
| + if merr != nil { | ||
| + logger.Errorf("cant match error: %v", merr) |
|
Also, should this be targetting 2.1 instead of 2.2? That seems the more relevant place to get the fix. |
|
@jameinel ptal again |
|
Please re-target for 2.1 \o/ |
perrito666
changed the base branch from
develop
to
2.1
Feb 7, 2017
perrito666
changed the base branch from
2.1
to
develop
Feb 7, 2017
|
you'll need to cherry-pick now to get to 2.1 since it includes all the extra 'develop' changes. |
jameinel
approved these changes
Feb 7, 2017
overall I think its good enough. I definitely worry about the fragility of our detection, but we can add to it as we get more understanding.
| @@ -65,6 +66,8 @@ func IsRunningLocally() (bool, error) { | ||
| return running, nil | ||
| } | ||
| +const errIPV6NotSupported = `socket: address family not supported by protocol` |
jameinel
Feb 7, 2017
Owner
a comment here about how we arrived at this string would be useful.
did you trace where the actual string was generated to know if it is stable?
| + err := client.SetServerConfig("core.https_address", "[::]") | ||
| + | ||
| + if err != nil { | ||
| + // if the error hints that the problem might be a protocol unsoported |
|
$$merge$$ |
|
Status: merge request accepted. Url: http://juju-ci.vapour.ws:8080/job/github-merge-juju |
perrito666 commentedFeb 3, 2017
Detect when IPV6.address is empty in lxd config and fallback to IPV4 when 6 is disabled in kernel.
This change prevents juju lxd provider from failing when running in a machine with ipv6 disabled in different possible ways.
QA steps
Try reproduction steps in bugs:
Bug reference
https://bugs.launchpad.net/juju/+bug/1656205
https://bugs.launchpad.net/juju/+bug/1633362