/juju

Bump lxd api #4131

Merged
merged 6 commits into from Feb 5, 2016

Conversation

Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
6 participants
@tych0 @jameinel @jujubot @cherylj @javacruft @perrito666
@tych0
Contributor

tych0 commented Jan 17, 2016

update the version of the go LXD bindings, and update some small incompatibilities.
@jameinel
Owner

jameinel commented Jan 19, 2016

I wonder why this didn't get a review-board request. Is it a user issue, or is it because you didn't provide a review comment (which would then get used in the commit message when it is merged).

Anyway, the changes LGTM.
@jameinel
Owner

jameinel commented Jan 19, 2016

$$merge$$
@jujubot
Contributor

jujubot commented Jan 19, 2016

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

jujubot commented Jan 19, 2016

Build failed: Does not match ['fixes-1533750']
build url: http://juju-ci.vapour.ws:8080/job/github-merge-juju/6005
@tych0
Contributor

tych0 commented Jan 19, 2016

On Tue, Jan 19, 2016 at 12:35:41AM -0800, John Arbash Meinel wrote:

I wonder why this didn't get a review-board request. Is it a user issue, or is it because you didn't provide a review comment (which would then get used in the commit message when it is merged).

I don't know :). Is there something I have to do?

Anyway, the changes LGTM.

Reply to this email directly or view it on GitHub:
#4131 (comment)
@tych0
Contributor

tych0 commented Jan 19, 2016

On Tue, Jan 19, 2016 at 12:39:04AM -0800, Juju bot wrote:

Build failed: Does not match ['fixes-1533750']
build url: http://juju-ci.vapour.ws:8080/job/github-merge-juju/6005

Not quite sure I understand the output here. Is this my fault?

Reply to this email directly or view it on GitHub:
#4131 (comment)
@cherylj

cherylj commented Jan 19, 2016

@tych0 No, master is currently blocked trying to get stabilization for an upcoming 2.0-alpha1 release. Once we get a blessed build and release 2.0-alpha1, master will be unblocked and you can try and merge this PR again.
@jameinel
Owner

jameinel commented Jan 20, 2016

master is now blessed, so likely we just have to wait for a release to go.

I think one thing to change is that when you create a pull request, just make sure to include a comment. That should allow our automated system to generate a Review Board entry, where we prefer to do our reviews. http://reviews.vapour.ws/dashboard/

It is also the commit message we will use when merging your changes, so it is nice to have a simple summary of the changes you've made. I'll try to keep tabs on the bug so I can land this as soon as Master is unblocked again.
@jameinel
Owner

jameinel commented Jan 20, 2016

I tried testing this out after having bootstrapped lxd previously.
It turns out the path is not consistent with the old code.
Specifically I got this error:
2016-01-20 12:57:09 INFO juju.cmd supercommand.go:58 running juju [2.0-alpha1 gc]
2016-01-20 12:57:12 DEBUG juju.container.lxd.lxdclient client.go:53 loading LXD client config from "/home/jameinel/.config/lxc/juju-lxd"
2016-01-20 12:57:12 DEBUG juju.cmd.juju common.go:98 Destroying environment info.
2016-01-20 12:57:12 INFO cmd cmd.go:129 Bootstrap failed, cleaning up the environment.
2016-01-20 12:57:12 ERROR cmd supercommand.go:448 there was an issue examining the environment: invalid config: cannot read config file: read /home/jameinel/.config/lxc/juju-lxd: is a directory

If I then deleted that directory, because I had already destroyed the environment, I then got this failure:
2016-01-20 12:58:50 INFO juju.cmd supercommand.go:58 running juju [2.0-alpha1 gc]
2016-01-20 12:58:50 DEBUG juju.container.lxd.lxdclient client.go:53 loading LXD client config from "/home/jameinel/.config/lxc/juju-lxd"
2016-01-20 12:58:50 DEBUG juju.container.lxd.lxdclient client.go:60 using LXD remote "local"
2016-01-20 12:58:51 DEBUG juju.cmd.juju common.go:98 Destroying environment info.
2016-01-20 12:58:51 INFO cmd cmd.go:129 Bootstrap failed, cleaning up the environment.
2016-01-20 12:58:51 ERROR cmd supercommand.go:448 there was an issue examining the environment: invalid config: Can not change ZFS config. Images or containers are still using the ZFS pool: [images/1032903165a677e18ed93bde5057ae6287841ae756d1a6296eef8f2e5a825e4a]

Obviously, I didn't look over the code thoroughly enough. Apparently passing in config isn't doing the same thing that we were getting by creating a directory for config.
@tych0
Contributor

tych0 commented Jan 20, 2016

On Wed, Jan 20, 2016 at 05:00:53AM -0800, John Arbash Meinel wrote:

I tried testing this out after having bootstrapped lxd previously.
It turns out the path is not consistent with the old code.
Specifically I got this error:
2016-01-20 12:57:09 INFO juju.cmd supercommand.go:58 running juju [2.0-alpha1 gc]
2016-01-20 12:57:12 DEBUG juju.container.lxd.lxdclient client.go:53 loading LXD client config from "/home/jameinel/.config/lxc/juju-lxd"
2016-01-20 12:57:12 DEBUG juju.cmd.juju common.go:98 Destroying environment info.
2016-01-20 12:57:12 INFO cmd cmd.go:129 Bootstrap failed, cleaning up the environment.
2016-01-20 12:57:12 ERROR cmd supercommand.go:448 there was an issue examining the environment: invalid config: cannot read config file: read /home/jameinel/.config/lxc/juju-lxd: is a directory

If I then deleted that directory, because I had already destroyed the environment, I then got this failure:
2016-01-20 12:58:50 INFO juju.cmd supercommand.go:58 running juju [2.0-alpha1 gc]
2016-01-20 12:58:50 DEBUG juju.container.lxd.lxdclient client.go:53 loading LXD client config from "/home/jameinel/.config/lxc/juju-lxd"
2016-01-20 12:58:50 DEBUG juju.container.lxd.lxdclient client.go:60 using LXD remote "local"
2016-01-20 12:58:51 DEBUG juju.cmd.juju common.go:98 Destroying environment info.
2016-01-20 12:58:51 INFO cmd cmd.go:129 Bootstrap failed, cleaning up the environment.
2016-01-20 12:58:51 ERROR cmd supercommand.go:448 there was an issue examining the environment: invalid config: Can not change ZFS config. Images or containers are still using the ZFS pool: [images/1032903165a677e18ed93bde5057ae6287841ae756d1a6296eef8f2e5a825e4a]

Obviously, I didn't look over the code thoroughly enough. Apparently passing in config isn't doing the same thing that we were getting by creating a directory for config.

Bah, I think I know what happened. I broke it during a rebase :(. I
will post a fix shortly.

Reply to this email directly or view it on GitHub:
#4131 (comment)
@tych0
Contributor

tych0 commented Jan 21, 2016

Ok, I believe I've fixed that and one other issue that @jamespage reported by bumping to the 0.27 client.
@tych0
Contributor

tych0 commented Jan 21, 2016

Oh, whoops. Wrong james page, sorry :)

@tych0 tych0 referenced this pull request Jan 22, 2016

Closed

Add lxd container type to cloud + local providers #4191

@javacruft
Contributor

javacruft commented Feb 2, 2016

This fix really needs to land; juju is not currently compatible with the lxd version in xenial

Tycho Andersen added some commits Jan 14, 2016

lxd: update config for latest version 
The LXD API has updated to allow config files to be loaded from arbitrary
locations on disk without mucking around with the environment (sorry about
that), which gets rid of some ugly code.

Additionally, there are some minor symbol signature updates as well.

Signed-off-by: Tycho Andersen <tycho.andersen@canonical.com>
2a17adb
lxdclient: fix typo 
Signed-off-by: Tycho Andersen <tycho.andersen@canonical.com>
44ef5f4
lxd provider: remove irrelevent documentation 
Since lxd-images is completely independent of images.linuxcontainers.org
(and linuxcontainers.org don't have cloud-init anyway), let's not have
people do this unnecessary step.

Signed-off-by: Tycho Andersen <tycho.andersen@canonical.com>
860ea77
lxd provider: use the right config path in all places 
Signed-off-by: Tycho Andersen <tycho.andersen@canonical.com>
70c8b25
update lxd client version to 0.27 
There is a breakage with version 0.27 such that older versions of the go
client don't behave correctly when setting configuration. Let's use 0.27.

Signed-off-by: Tycho Andersen <tycho.andersen@canonical.com>
fd9f50e
lxdclient: get rid of spurious println 
I introduced this while debugging something, but didn't get rid of it.
Happy to squash this commit into the previous ones if necessary.

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

perrito666 commented Feb 3, 2016

$$merge$$
@jujubot
Contributor

jujubot commented Feb 3, 2016

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

cherylj commented Feb 3, 2016

Please don't merge this until master is unblocked.
@jujubot
Contributor

jujubot commented Feb 3, 2016

Build failed: Does not match ['fixes-1538241']
build url: http://juju-ci.vapour.ws:8080/job/github-merge-juju/6196
@jameinel
Owner

jameinel commented Feb 5, 2016

$$merge$$
@jujubot
Contributor

jujubot commented Feb 5, 2016

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

jujubot added a commit that referenced this pull request Feb 5, 2016

@jujubot
Merge pull request #4131 from tych0/bump-lxd-api 
Bump lxd api

update the version of the go LXD bindings, and update some small incompatibilities.
0f40661

@jujubot jujubot merged commit 0f40661 into juju:master Feb 5, 2016

@cherylj

cherylj commented Feb 5, 2016

This shouldn't have been merged into master yet. It was being tested in a feature branch and had known issues and now master is broken :(

cherylj pushed a commit to cherylj/juju that referenced this pull request Feb 5, 2016

lp:1542336 Revert PR #4131 from tych0/bump-lxd-api 
This reverts commit 0f40661, reversing
changes made to e437afc.

PR 4131 brought in a new dependency that wasn't included
in dependencies.tsv.  tych0 was working on updating lxd
to not pull in "github.com/gosexy/gettext", so I am
reverting this patch until his work is done.
719bc83

jujubot added a commit that referenced this pull request Feb 5, 2016

@jujubot
Merge pull request #4310 from cherylj/revert_lxd_bump 
lp:1542336 Revert PR #4131 from tych0/bump-lxd-api

This reverts commit 0f40661, reversing
changes made to e437afc.

PR 4131 brought in a new dependency that wasn't included
in dependencies.tsv.  tych0 was working on updating lxd
to not pull in "github.com/gosexy/gettext", so I am
reverting this patch until his work is done.

(Review request: http://reviews.vapour.ws/r/3752/)
1cd7ac8
@jameinel
Owner

jameinel commented Feb 9, 2016

So the feature branch was involving a lot more code rather than just bringing in an updated LXD agent version, wasn't it? How did it pass the bot if it broke master? Is there a bigger dependency problem that I was missing? I had tested it a bit manually and it seemed sane, was it just a dependency we are/were missing on some platforms/versions of go?
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment