/juju

Updates for LXD network API in 2.3 #6367

Merged
merged 15 commits into from Oct 5, 2016

Conversation

Reviewers

@dimitern dimitern

@dooferlad dooferlad

@stgraber stgraber

@wallyworld wallyworld

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
9 participants
@tych0 @mitechie @dooferlad @jujubot @AlexisBruemmer @sinzui @dimitern @stgraber @wallyworld
@tych0
Contributor

tych0 commented Sep 30, 2016

Some thoughts:

we might not need the 3rd patch in the series (about falling back to the daily), but without it there's no way to get yakkety containers right now.

the second patch we definitely need, as it is a bug :)

Tycho Andersen added some commits Sep 29, 2016

update for lxd 2.3: use the new network API to create bridges 
In particular, fix up the old bridge configuration code to use the new API
when it is available.

Signed-off-by: Tycho Andersen <tycho.andersen@canonical.com>
61019e9
lxd: fail if image isn't imported 
Before, the error is initialized to nil, and it looks like we succeed when
we don't really.

Signed-off-by: Tycho Andersen <tycho.andersen@canonical.com>
4e2d536
lxd: fall back to the daily remote if nothing is in releases 
We may or may not want to drop this patch, but I needed it for yakkety
testing and it doesn't seem that unreasonable.

Signed-off-by: Tycho Andersen <tycho.andersen@canonical.com>
754bd36
tools/lxdclient/client_network.go
+ nicType := "macvlan"
+ if n.Type == "bridge" {
+ nicType = "bridged"
+ }
@stgraber

stgraber Sep 30, 2016

You could save yourself the second NetworkGet and that branch. Since you're calling NetworkCreate, you're guaranteed to be given a bridge.

The n.Type check is only used in our client for when someone does "lxc network attach eth0 blah" where eth0 isn't a bridge.

@tych0

tych0 Sep 30, 2016

Contributor

That's only true as long as the default continues to be bridge, though.

@stgraber

stgraber Sep 30, 2016

Right, but changing network creation to something other than a bridge would be considered a major API breakage and would only happen if we rev the API to 2.0 (from 1.0).

@tych0

tych0 Sep 30, 2016

Contributor

And this way if that happens we don't have to go back and patch juju :)

dependencies.tsv
@@ -52,7 +52,7 @@ github.com/juju/xml git eb759a627588d35166bc505fceb51b88500e291e 2015-04-13T13:1
github.com/juju/zip git f6b1e93fa2e29a1d7d49b566b2b51efb060c982a 2016-02-05T10:52:21Z
github.com/julienschmidt/httprouter git 77a895ad01ebc98a4dc95d8355bc825ce80a56f6 2015-10-13T22:55:20Z
github.com/lunixbochs/vtclean git 4fbf7632a2c6d3fbdb9931439bdbbeded02cbe36 2016-01-25T03:51:06Z
-github.com/lxc/lxd git 0d172e3080f768fd419f0c97f5246983797db243 2016-08-12T05:08:18Z
@dimitern

dimitern Sep 30, 2016

Contributor

Please, use godeps github.com/juju/juju/... | grep lxc/lxd to update that line, so it includes the correct date, not just the commit hash. Assuming your github.com/lxc/lxd work dir is at rev 48f.. it will produce the correct line to replace.

@tych0

tych0 Oct 3, 2016

Contributor

Thanks, updated.

update timestamp on lxd dep too 
Signed-off-by: Tycho Andersen <tycho.andersen@canonical.com>
a652eb2
@wallyworld

wallyworld requested changes Oct 4, 2016
View changes

Code looks ok but there's a lack of unit tests. There's a lot of new code paths introduced and no test coverage.
I have pull the branch and tested locally with LXD on xenial and it appears to work ok with the older APIs, so that's good.

container/lxd/initialisation.go
@@ -140,6 +143,20 @@ var configureZFS = func() {
}
var configureLXDBridge = func() error {
+ client, err := ConnectLocal()
+ if err != nil {
+ return err
@wallyworld

wallyworld Oct 4, 2016

Owner

For consistency, should be errors.Trace(err)

tools/lxdclient/client.go
+ // If this is the LXD provider on the localhost, let's do an extra check to
+ // make sure the default profile has a correctly configured bridge, and
+ // which one is it.
+ bridgeName, err = verifyDefaultProfileBridgeConfig(raw, networkAPISupported)
@wallyworld

wallyworld Oct 4, 2016

Owner

We are ignoring the error here, oops

tools/lxdclient/client.go
@@ -140,6 +150,7 @@ func Connect(cfg Config) (*Client, error) {
profileClient: &profileClient{raw},
instanceClient: &instanceClient{raw, remoteID},
imageClient: &imageClient{raw, connectToRaw},
+ networkClient: &networkClient{raw, networkAPISupported},
@wallyworld

wallyworld Oct 4, 2016

Owner

this line contains tabs not spaces so will fail go fmt

tools/lxdclient/client.go
+ */
+ if networkAPISupported {
+ if err := CreateDefaultBridgeInDefaultProfile(client); err != nil {
+ return "", errors.Errorf("couldn't create default bridge %v", err)
@wallyworld

wallyworld Oct 4, 2016

Owner

Should be errors.Annotate()

tools/lxdclient/client.go
+ }
+
+ return network.DefaultLXDBridge, nil
+ }
return "", errors.Errorf("unexpected LXD %q profile config without eth0: %+v", defaultProfileName, config)
@wallyworld

wallyworld Oct 4, 2016

Owner

errors.Annotatef() here
(and anywhere else relevant)

@tych0

tych0 Oct 4, 2016

Contributor

In fact I don't think it applies here, because there's no error here to annotate.

@wallyworld

wallyworld Oct 4, 2016

Owner

Doh, yes, sorry

tools/lxdclient/client_image.go
@@ -98,7 +98,7 @@ func (i *imageClient) EnsureImageExists(series string, sources []Remote, copyPro
// at private methods so we can't easily tweak it.
name := i.ImageNameForSeries(series)
- var lastErr error
+ lastErr := errors.Errorf("image not imported!")
@wallyworld

wallyworld Oct 4, 2016

Owner

errors.New()

tools/lxdclient/client_network.go
+ supported bool
+}
+
+func (c *networkClient) NetworkCreate(name string, config map[string]string) error {
@wallyworld

wallyworld Oct 4, 2016

Owner

exported functions need to be commented, here and below

tools/lxdclient/client_network.go
+
+func (c *networkClient) NetworkCreate(name string, config map[string]string) error {
+ if !c.supported {
+ return fmt.Errorf("network API not supported on this remote")
@wallyworld

wallyworld Oct 4, 2016 •
Edited 1 time

Owner

should be using juju/errors
and there's a NotSupported error type

tools/lxdclient/remote.go
@@ -44,8 +44,16 @@ var CloudImagesRemote = Remote{
ServerPEMCert: "",
}
+var CloudImagesDailyRemote = Remote{
@wallyworld

wallyworld Oct 4, 2016

Owner

Pre-existing code, but not of the exported consts or vars etc in this file are commented and they should be.
Would be great to see a driveby here to fix that

Tycho Andersen added some commits Oct 4, 2016

don't ignore error 
Signed-off-by: Tycho Andersen <tycho.andersen@canonical.com>
77f4eb4
go fmt ./... 
Signed-off-by: Tycho Andersen <tycho.andersen@canonical.com>
130c555
s/errors.Errorf/errors.Annotate 
Signed-off-by: Tycho Andersen <tycho.andersen@canonical.com>
084b537
use errors.Trace() where necessary 
Signed-off-by: Tycho Andersen <tycho.andersen@canonical.com>
5b73b34
s/errors.Errorf/errors.New/ 
arbitrary is my middle name!

Signed-off-by: Tycho Andersen <tycho.andersen@canonical.com>
54c5834
add some comments 
doge would be proud.

Signed-off-by: Tycho Andersen <tycho.andersen@canonical.com>
e42a2ff
s/fmt.Errorf/errors.NotSupportedf/g 
it should be blue!

Signed-off-by: Tycho Andersen <tycho.andersen@canonical.com>
c0cfc82
pontification on "release" vs. "daily" 
This is my opinion only, and does not reflect the position or opinion of
my employer.

Signed-off-by: Tycho Andersen <tycho.andersen@canonical.com>
c455d59
@tych0
Contributor

tych0 commented Oct 4, 2016

In theory I've addressed all of these; thoughts welcome!

@tych0 tych0 closed this Oct 4, 2016

@tych0 tych0 reopened this Oct 4, 2016

@tych0
Contributor

tych0 commented Oct 4, 2016

But also, I can't push the right button :(
@wallyworld

wallyworld approved these changes Oct 4, 2016
View changes

Thanks for addressing the previous issues. We need to land this so I've just left a final nitpic issue to address.
But my comment on the lack of unit test coverage remains. There really should be additional testing around the new code paths.

tools/lxdclient/client_network.go
+ ProfileConfig(profile string) (*shared.ProfileConfig, error)
+}
+
+// Create a default bridge and (if necessary) insert it into the default profile.
@wallyworld

wallyworld Oct 4, 2016

Owner

This is a nitpic sorry, but the Go doc convention is to start method comments with the name of the method.
// CreateDefaultBridgeInDefaultProfile creates a default bridge...

(here and elsewheren - the same also pplies to the comment on the CloudImagesDailyRemote var)

@mitechie
Owner

mitechie commented Oct 4, 2016

Will get this reviewed and landed this week as part of getting to GA. I've got two folks out today, but should get eyes on it tomorrow.
update comments 
kid tested, godoc approved!

Signed-off-by: Tycho Andersen <tycho.andersen@canonical.com>
7e50ab5
@tych0
Contributor

tych0 commented Oct 4, 2016

Cool, I just pushed an update to the comments.
tools/lxdclient/client_network.go
+}
+
+// Create a default bridge and (if necessary) insert it into the default profile.
+func CreateDefaultBridgeInDefaultProfile(client creator) error {
@dooferlad

dooferlad Oct 4, 2016

Contributor

Is this really Juju's place to do this? If the user hasn't created a bridge, we could just error out and ask them to do so. It sounds like the LXD client is capable of doing this given the conversation below as is dpkg.

@tych0

tych0 Oct 4, 2016

Contributor

In the container type case yes definitely, as there is no "user" that's on the machine there. In the provider case you could make an argument that it's not needed, but then it's just another step that someone has to do to start using the LXD provider, so I think it's a better UX to JFDI.

@dooferlad

dooferlad Oct 4, 2016

Contributor

...and you can't contact LXD and tell it to set up the bridge? It seems like we have code that is needed wherever LXD is running and it being in the server rather than the client could reduce duplication.

@tych0

tych0 Oct 4, 2016

Contributor

I guess I don't understand. This code does contact LXD to set up the bridge. What code is it duplicating?

@dooferlad

dooferlad Oct 4, 2016

Contributor

@stgraber wrote:
"The n.Type check is only used in our client for when someone does "lxc network attach eth0 blah" where eth0 isn't a bridge."

This implies that your client (I assume lxc) can do the bridge set up and that very similar code has been put into Juju.

This is all a bit academic though because unless LXD grows an API call to set up the bridge that both Juju and LXC can call, then we need to duplicate the code.

@dooferlad
Contributor

dooferlad commented Oct 4, 2016

$$merge$$
@jujubot
Contributor

jujubot commented Oct 4, 2016

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

tych0 commented Oct 4, 2016

Hmm. so it looks like merge failed because of an additional dependency; looking into it now. What's the story on adding new dependencies?
@tych0
Contributor

tych0 commented Oct 4, 2016

Ok, I think I've untangled this: lxc/lxd#2456

when the above is merged, i'll push a new commit with the updated hash and we can merge this one, I think.

Tycho Andersen added some commits Oct 5, 2016

update LXD client hash 
This should get rid of the extra log15 dep that juju was seeing.

Signed-off-by: Tycho Andersen <tycho.andersen@canonical.com>
065a5d4
go fmt ./... 
Signed-off-by: Tycho Andersen <tycho.andersen@canonical.com>
1930d8c
@tych0
Contributor

tych0 commented Oct 5, 2016

I believe this is updated and should hopefully be able to merge now :)
@tych0
Contributor

tych0 commented Oct 5, 2016

$$merge$$
@AlexisBruemmer

AlexisBruemmer commented Oct 5, 2016

$$merge$$
@sinzui
Contributor

sinzui commented Oct 5, 2016

Build failed: Tests failed
build url: foo
@sinzui
Contributor

sinzui commented Oct 5, 2016

$$merge$$
@jujubot
Contributor

jujubot commented Oct 5, 2016

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

@jujubot jujubot merged commit 0d66e38 into juju:master Oct 5, 2016

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