/juju

Add lxd container type to cloud + local providers #4191

Closed
wants to merge 20 commits into
from

Conversation

Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
4 participants
@tych0 @jameinel @ericsnowcurrently @cherylj
@tych0
Contributor

tych0 commented Jan 22, 2016

Hello folks,

Here's a patchset that adds lxd container type support for all the cloud providers + local provider. Some notes:

  • This depends on my two other PRs #4131 and #4176; I'll be happy to rebase it when those are merged.
  • I've only tested this on GCE (and local), but since I didn't have to make any GCE specific changes, hopefully it'll work everywhere else. Any testing/thoughts about what needs to be done for other providers are much appreciated.
  • the lxd-images invocation here will need to change soon, as we're trying to get rid of lxd-images (probably next week those changes will land, and we'll have a go api for importing images). I can send a patch for that when we land it.
  • I haven't added any more tests for this.
  • I haven't gotten rid of the lxc container type. I assume we'll need to keep this so that we can support upgrades at some point.
  • I don't know anything about juju, so it's entirely possible I've screwed all of this up :)

Thoughts appreciated!
cmd/jujud/agent/machine.go
- if err != nil {
- errors = append(errors, err)
- } else if insideLXC {
+ insideLXC := container.RunningInContainer()
@jameinel

jameinel Jan 26, 2016

Owner

Probably we should switch to "insideContainer" as the variable name for clarity.

container/factory/factory.go
@@ -21,6 +22,8 @@ func NewContainerManager(forType instance.ContainerType, conf container.ManagerC
switch forType {
case instance.LXC:
return lxc.NewContainerManager(conf, imageURLGetter, looputil.NewLoopDeviceManager())
+ case instance.LXD:
@jameinel

jameinel Jan 26, 2016

Owner

I would think we need similar support for LoopDeviceManager from LXC containers. But I suppose that can be on a TODO.

container/lxd/initialisation.go
+)
+
+var requiredPackages = []string{
+ "lxd",
@jameinel

jameinel Jan 26, 2016

Owner

Don't we also need things like "lxd-tools" to get lxd-images import?

@tych0

tych0 Jan 26, 2016

Contributor

lxd-images is in lxd proper, so I don't think so. Calling out to that will go away soon as well, and we'll just use the API to import images as soon as the LXD simplestreams code lands.

@tych0

tych0 Jan 26, 2016

Contributor

see, http://packages.ubuntu.com/xenial/amd64/lxd/filelist

container/lxd/initialisation.go
+
+ for _, pack := range requiredPackages {
+ pkg := pack
+ if config.SeriesRequiresCloudArchiveTools(series) &&
@jameinel

jameinel Jan 26, 2016

Owner

I'm not sure if we need this one, but I believe we do need to add something that knows to install "lxd/trusty-backports" on Trusty machines. IIRC CloudArchive was only needed for Precise? (And I'm guessing LXD isn't going to be available there.) I suppose you could still use a Trusty host with a Precise container?

@tych0

tych0 Jan 26, 2016

Contributor

I thought cloud archive existed for trusty too (and will probably exist for xenial when it comes time to push updated versions), but I'm not an expert here. I'll see about adding some code to do the backports thing, though, thanks.

tools/lxdclient/client_image.go
+}
+
+func (i imageClient) EnsureImageExists(series string) error {
+ name := i.ImageNameForSeries(series)
@jameinel

jameinel Jan 26, 2016

Owner

do we have to worry about architecture? Or that is naturally determined just by the local host architecture?

@tych0

tych0 Jan 26, 2016

Contributor

Right, although it is possible to run i386 images on amd64 hosts, the architectures in general match. Of course we can support the i386 bit if that is necessary, but I suppose it isn't?

tools/lxdclient/client.go
- defer updateLXDVars(origDirname)
-
- cfg, err := lxdLoadConfig()
+ cfg, err := lxdLoadConfig(path.Join(configDir, "config.yml"))
@jameinel

jameinel Jan 26, 2016

Owner

Were we going to try and change this to be config strings that we pass in, rather than maintaining a somewhat arbitrary config directory?

@tych0

tych0 Jan 26, 2016

Contributor

Sure, but I don't know where/how to store it in juju's database, so I just left it as-is.

@jameinel

jameinel Jan 27, 2016

Owner

So we do have the issue that we don't generally let Providers (and thus Broker/container.Managers) have direct DB access. So we'd need to be passing this information in via just extra parameters to Init or Config.
What are the things we need, certificates and the config that says to use "local"?
I know we have support for some of those things at the Provider level, to date we haven't needed them for Brokers. Will need some thought as you need a custom cert for every machine to every LXD, right? How do we validate the LXD's certificate, or do we just accept it as long as it is valid?

@tych0

tych0 Jan 29, 2016

Contributor

On Tue, Jan 26, 2016 at 08:27:30PM -0800, John Arbash Meinel wrote:

@@ -52,11 +56,7 @@ var lxdLoadConfig = lxd.LoadConfig
func newRawClient(remote, configDir string) (*lxd.Client, error) {
logger.Debugf("loading LXD client config from %q", configDir)

  • // This will go away once LoadConfig takes a dirname argument.
  • origDirname := updateLXDVars(configDir)

- defer updateLXDVars(origDirname)

  • cfg, err := lxdLoadConfig()
  • cfg, err := lxdLoadConfig(path.Join(configDir, "config.yml"))

So we do have the issue that we don't generally let Providers (and thus Broker/container.Managers) have direct DB access. So we'd need to be passing this information in via just extra parameters to Init or Config.
What are the things we need, certificates and the config that says to use "local"?

We don't need any certificates if you only want to talk to the LXD
provider on the local host, just enough perms to write to
/var/lib/lxd/unix.socket

I know we have support for some of those things at the Provider level, to date we haven't needed them for Brokers. Will need some thought as you need a custom cert for every machine to every LXD, right? How do we validate the LXD's certificate, or do we just accept it as long as it is valid?

If you're only talking to the local lxd, since it's the only thing
that can listen on /var/lib/lxd/unix.socket, so there is no
authentication required. Things get a little more complicated when you
want to talk to remotes, but as far as I can tell we don't need to do
that (at least right now with the LXD provider/container type).

provider/local/prereqs.go
@@ -68,6 +69,8 @@ var VerifyPrerequisites = func(containerType instance.ContainerType) error {
switch containerType {
case instance.LXC:
return verifyLxc()
+ case instance.LXD:
+ return nil
@jameinel

jameinel Jan 26, 2016

Owner

shouldn't this be the verifyLxd function you just implemented?

@jameinel
Owner

jameinel commented Jan 26, 2016

I think there are still a few steps that we'll need to add to bring it up to par with LXC. Things like creating template images and caching them in global state so that the time to bring up a large deployment with containers doesn't regress.

I'm also concerned that there aren't any new tests, even though there is a fair amount of new code being added. While I have some faith that Tycho is writing decent code, I'm concerned about future maintenance for people that don't understand this code as well, and may not know what the assumptions are.
@tych0
Contributor

tych0 commented Jan 26, 2016

On Tue, Jan 26, 2016 at 03:56:44AM -0800, John Arbash Meinel wrote:

I think there are still a few steps that we'll need to add to bring it up to par with LXC. Things like creating template images and caching them in global state so that the time to bring up a large deployment with containers doesn't regress.

Yep, definitely.

I'm also concerned that there aren't any new tests, even though there is a fair amount of new code being added. While I have some faith that Tycho is writing decent code, I'm concerned about future maintenance for people that don't understand this code as well, and may not know what the assumptions are.

Yep, I can add some tests for this (similar to what the LXC tests do).
Mostly I just wanted to get this in front of someone who knows what
needs to happen to review it. I'm traveling tomorrow, so hopefully
I'll have some time then to write/push some new tests. (I'll also be
on a european timezone at that point, so perhaps we can coordinate
more easily.)

tych0 pushed a commit to tych0/juju-utils that referenced this pull request Jan 26, 2016

add config.RequiresBackports 
This will be used for the implementation of
juju/juju#4191 the LXD container type.

Signed-off-by: Tycho Andersen <tycho.andersen@canonical.com>
efd3d0c

@tych0 tych0 referenced this pull request in juju/utils Jan 26, 2016

Merged

add config.RequiresBackports #192

@tych0
Contributor

tych0 commented Jan 26, 2016

Ok, I believe I've addressed all the review comments modulo the test bits which I'll try to address tomorrow (got some other kernel goop to debug today). I've also rebased things since there were patches that landed in master that conflicted.

jujubot added a commit to juju/utils that referenced this pull request Jan 27, 2016

@jujubot
Merge pull request #192 from tych0/add-requires-backports 
add config.RequiresBackports

This will be used for the implementation of
juju/juju#4191 the LXD container type.

Signed-off-by: Tycho Andersen <tycho.andersen@canonical.com>
6a56fe5
@jameinel
Owner

jameinel commented Jan 27, 2016

I did just try to bootstrap this branch on EC2 with a Trusty target, and it failed because it couldn't install just "LXD". The worker that failed kept dying and trying again.
I then manually tried to "apt-get install lxd/trusty-backports" but that fails because it can't satisfy the rest of lxd's dependencies. I needed to do:
sudo apt-get install lxd/trusty-backports lxc/trusty-backports liblxc1/trusty-backports python3-lxc/trusty-backports

So we'll have to think carefully about how we enable trusty, given the need for more than just the obvious package.
@tych0
Contributor

tych0 commented Jan 27, 2016

On Wed, Jan 27, 2016 at 04:27:10AM -0800, John Arbash Meinel wrote:

I did just try to bootstrap this branch on EC2 with a Trusty target, and it failed because it couldn't install just "LXD". The worker that failed kept dying and trying again.
I then manually tried to "apt-get install lxd/trusty-backports" but that fails because it can't satisfy the rest of lxd's dependencies. I needed to do:
sudo apt-get install lxd/trusty-backports lxc/trusty-backports liblxc1/trusty-backports python3-lxc/trusty-backports

So we'll have to think carefully about how we enable trusty, given the need for more than just the obvious package.

Right, this is a known bug (see the commit message), which apparently
hasn't been fixed yet :). I'll ping stgraber or someone about trying
to get an apt fix uploaded.

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

tych0 commented Jan 27, 2016

On Wed, Jan 27, 2016 at 04:27:10AM -0800, John Arbash Meinel wrote:

I did just try to bootstrap this branch on EC2 with a Trusty target, and it failed because it couldn't install just "LXD". The worker that failed kept dying and trying again.
I then manually tried to "apt-get install lxd/trusty-backports" but that fails because it can't satisfy the rest of lxd's dependencies. I needed to do:
sudo apt-get install lxd/trusty-backports lxc/trusty-backports liblxc1/trusty-backports python3-lxc/trusty-backports

So we'll have to think carefully about how we enable trusty, given the need for more than just the obvious package.

I guess that's a good point, though, that even with the bugfix we'll
need a newer LXC on trusty, which might break everything for users who
depend on a specific version of LXC in trusty.
@ericsnowcurrently
Contributor

ericsnowcurrently commented Feb 1, 2016

@tycho FYI, this repo has integration with reviewboard:

https://github.com/juju/juju/blob/master/CONTRIBUTING.md#code-review

However, that does not work until you have logged in to reviewboard the first time. I'd recommend doing that so future PRs will automatically have review requests created.
@tych0
Contributor

tych0 commented Feb 3, 2016

Hi folks. I've pushed some sanity tests here, as well as fixed another bug. I think it would be good to land this so we can get people to start playing with it (and/or land #4131 so at least the 0.27 incompatibility is fixed). Thoughts?
@cherylj

cherylj commented Feb 3, 2016

@tych0 master is attempting to get a bless so we can ship alpha2. Once we get a bless and ship alpha2, then we should merge this.
@tych0
Contributor

tych0 commented Feb 3, 2016

Ok. It's probably worth landing #4131 at least, then.
@cherylj

cherylj commented Feb 3, 2016

Could you guys drop this in a feature branch so it could get some CI testing before landing into master?

You will need to rebase as the api-command-rename branch has landed, and without those changes, nothing will pass CI.
@tych0
Contributor

tych0 commented Feb 3, 2016

On Wed, Feb 03, 2016 at 06:48:19AM -0800, Cheryl Jennings wrote:

Could you guys drop this in a feature branch so it could get some CI testing before landing into master?

I don't know how to do that, exactly. Are there any docs on how you
guys do this?

You will need to rebase as the api-command-rename branch has landed, and without those changes, nothing will pass CI.

Thanks for the heads up, I've rebased both this and #4131 in case you
want to land that one separately.

Tycho Andersen and others 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>
2012fe4
lxdclient: fix typo 
Signed-off-by: Tycho Andersen <tycho.andersen@canonical.com>
c65e9f1
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>
57edf6a
lxd provider: use the right config path in all places 
Signed-off-by: Tycho Andersen <tycho.andersen@canonical.com>
8e58356
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>
7c15515
lxc: get rid of custom container detection code 
The mechanism for detecting whether or not the task is in a container here
(checking if we're in the / cgroup) is very broken, and will not work with
cgroup namespaces (to land in kernel 4.5, but will be backported to
xenial's 4.4 kernel).

Instead, we should use the running-in-container script, as provided by the
init-system-helpers package, which encapsulates all the logic required for
various init systems. (n.b. that this adds a dependency; I'm not sure how
juju handles this.)

Also, I've mode the code to container/ instead of in lxc/lxcutils in
anticipation of the lxd container type which will also need to use this
function.

Signed-off-by: Tycho Andersen <tycho.andersen@canonical.com>
24fef1a
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>
b59d585
add a LXD container type 
This patch adds a LXD container type to juju. Note that currently it uses
the same image name schema that the lxd provider uses, i.e. ubuntu-$series.

Signed-off-by: Tycho Andersen <tycho.andersen@canonical.com>
1441569
@tych0
switch default local container type to lxd 
Signed-off-by: Tycho Andersen <tycho.andersen@canonical.com>
f733783
lxd: move lxdclient from /provider to /tools 
This is in preparation for adding the LXD container type.

Signed-off-by: Tycho Andersen <tycho.andersen@canonical.com>
1acefbd
s/insideLXC/insideContainer 
Signed-off-by: Tycho Andersen <tycho.andersen@canonical.com>
3a08b62
actually use the verifyLxd function 
Signed-off-by: Tycho Andersen <tycho.andersen@canonical.com>
9ec4403
add backports logic for lxd 
This commit needs to be updated when juju/utils#192
is merged.

This assumes that
https://bugs.launchpad.net/ubuntu/+source/apt/+bug/1512219 has been fixed
in trusty, which I'm not sure it has. I'm trying to verify that now. If
not, we'll have to do something more complicated.

Signed-off-by: Tycho Andersen <tycho.andersen@canonical.com>
8874a83
remove debugging 
Signed-off-by: Tycho Andersen <tycho.andersen@canonical.com>
6bad3ff
add lxd container initialization tests 
Signed-off-by: Tycho Andersen <tycho.andersen@canonical.com>
4c1ce6e
fix lxdclient test import path 
Signed-off-by: Tycho Andersen <tycho.andersen@canonical.com>
fcafc4b
commonize container detection code 
Use the same container detection code for both LXC and LXD. Also add some
tests for this.

Signed-off-by: Tycho Andersen <tycho.andersen@canonical.com>
9a74e10
lxd container type: remember to use the namespace 
If we don't do this everything works fine, but if you destroy your
juju environment, it destroys all the containers on the host. Instead,
let's not do that :)

Signed-off-by: Tycho Andersen <tycho.andersen@canonical.com>
12053bc
lxd container type: add some tests 
Here's a basic testcase for the previous commit. It also adds some plumbing
necessary to write more LXD tests.

Signed-off-by: Tycho Andersen <tycho.andersen@canonical.com>
868e9b4
lxd: bump version to 2.0.0beta1 
Signed-off-by: Tycho Andersen <tycho.andersen@canonical.com>
fc4f4c5
@tych0
Contributor

tych0 commented Feb 4, 2016

And rebased again :)
@cherylj

cherylj commented Feb 5, 2016

This PR is currently in feature branch lxd-container-type for CI testing and can be closed. We will merge the feature branch into master when it has passed CI.

@cherylj cherylj closed this Feb 5, 2016

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