lxd: prefer cached images, support CentOS #6970

axw · Feb 13, 2017

Description of change

We update the tools/lxdclient code to support
CentOS images if LXD knows about them. To do
this, several changes are required:

we give all images we use a juju-specific
alias: juju/<series>/<arch>
if an image exists locally with that alias,
we use it without consulting the remote
sources

These changes enable a user to supply their
own image for a given series/arch pair.

QA steps

go get github.com/axw/juju-lxd-centos-image-builder
$GOPATH/bin/juju-lxd-centos-image-builder
juju bootstrap localhost
go get github.com/axw/juju-tools
juju tools build 2.1-rc2.1-centos7-amd64
juju tools upload juju-2.1-rc2.1-centos7-amd64.tgz
juju add-machine --series=centos7
confirm centos machine comes up

Also tested existing behaviour on a non-amd64 host:

juju bootstrap azure
start a packet.net type 2A AArch64 server, manually provision it into juju
juju add-machine lxd:0 (where 0 is the packet.net server)
check that the container uses the alias "juju/xenial/arm64", and it points at the upstream xenial/arm64 image.

Documentation changes

Maybe? We might want to link to the image builder from the docs?

Bug reference

https://bugs.launchpad.net/juju/+bug/1495978

wallyworld · Feb 13, 2017

wallyworld reviewed Feb 13, 2017

View changes

Any images that the user already has as "ubuntu-" will no longer be used and new ones will be downloaded. I don't suppose this matters too much, but it will need to be highlighted in release notes I think. eg to allow the user to delete the now obsoleted images etc

Note totally a fan of juju/trusty/amd64 as opposed to juju-trusty-amd64. I think a flat image name, not one that looks like a path, is easier to grok.

Maybe discuss before landing?

axw · Feb 13, 2017

Any images that the user already has as "ubuntu-" will no longer be used and new ones will be downloaded. I don't suppose this matters too much, but it will need to be highlighted in release notes I think. eg to allow the user to delete the now obsoleted images etc

They will be used, I have confirmed it already. The "name" is just an alias, and an image can have multiple aliases. What happens in practice is that the image you had already ends up with two aliases: ubuntu-series, and juju/series/amd64.

Note totally a fan of juju/trusty/amd64 as opposed to juju-trusty-amd64. I think a flat image name, not one that looks like a path, is easier to grok.

Not too fussed, but the LXD image aliases in linuxcontainers.org and cloud-images have aliases in a path format. e.g. "ubuntu/xenial/s390x", "centos/7". I'd prefer to go with the flow.

jameinel · Feb 13, 2017

jameinel requested changes Feb 13, 2017

View changes

Unfortunately, I don't think we can land this without fixing the "Arch" issue. It was hard to get the right Arch into the system, because the point at which we figure that out is not the same time that we figure out Series. But without that fix, this will break on all machines that aren't AMD64.

A couple options:

We go without an 'arch' tag, and just assume we don't cross architecture. Which is true as long as we aren't caching the images in a central location. Then "juju/SERIES" is just whatever the local architecture is.
We actually fix it to request the right architecture and cache it appropriately.
We cheat and use local information (os.* sort of hacks), rather than passing in the constraints we're supposedly getting.

This is actually pretty dangerous, as by not specifying an arch, it actually uses the 'platform' arch, which means it works on ARM and PPC as long as we don't cross the streams. We'll need that more if we ever implement caching on the server.

I'm quite hesitant about this part, because I'm pretty sure this will break all our non-AMD64 deployments.

This is actually pretty dangerous, as by not specifying an arch, it actually uses the 'platform' arch, which means it works on ARM and PPC as long as we don't cross the streams. We'll need that more if we ever implement caching on the server.

How does it use the platform arch? We were looking for the plain alias "trusty" (for example), which is an alias for "trusty/amd64".

I'm quite hesitant about this part, because I'm pretty sure this will break all our non-AMD64 deployments.

I'm wondering how they were working at all...

I'll update it to pass the arch in though.

OK, seems that there's some magic somewhere that filters on the client's arch automatically.

I've fixed this, passing the container arch into the function.

I go back and forth on this one. If we had implemented caching of images on the controller, I heavily prefer using the upstream alias, because I don't really want one machine that deployed a container a long time ago to have a separate notion of what it is from all the other machines.

Aside from that, we have sort-of wanted the ability to sneak custom images into the workflow. So I'm reasonably happy to have some way for users to supply their own images, but doing it on each machine where you want to operate is a nightmare of distributed state, which is just ugly.

Is there a way to do this by 'provider', so that lxd provider prefers local aliases, but cloud images prefer official upstreams?

I go back and forth on this one. If we had implemented caching of images on the controller, I heavily prefer using the upstream alias, because I don't really want one machine that deployed a container a long time ago to have a separate notion of what it is from all the other machines.

I don't understand what the alias has to do with that. The alias used is orthogonal to image freshness. If you copy the upstream image into your LXD and give it the name "foo", you can still have it kept up-to-date by having the auto-update flag enabled.

I did consider just using the upstream alias(es), but decided against it to support the very scenario that this PR is addressing: centos support. Since the image needs to be customised, I don't think it's a good idea to give it the same alias as the upstream image.

Aside from that, we have sort-of wanted the ability to sneak custom images into the workflow. So I'm reasonably happy to have some way for users to supply their own images, but doing it on each machine where you want to operate is a nightmare of distributed state, which is just ugly.

The only change in behaviour is that we will now use images that don't exist in a remote. We still set the auto-update flag, which means all the LXDs will still update the juju/foo/bar image as they did for the ubuntu-foo image.

There is another reason to do the local alias check, which is to elide the remote image lookup/copy step. The copy will be a no-op, but those calls are not free, and can be slow (still involves several round trips to decide it has nothing to do).

Is there a way to do this by 'provider', so that lxd provider prefers local aliases, but cloud images prefer official upstreams?

We could, but I can't see the value in the added complexity.

FWIW, I prefer the foo/bar/baz path, because that does seem to be what upstream has been using. I was going to switch to it for "ubuntu/trusty" but it didn't seem worth it to break compatibility at the time.
If we're going with the full names, then we'll want to do this.

ubuntu/trusty is the alias format used on linuxcontainers.org. We want to use cloud-images, which has a collection of aliases per image. e.g. for trusty:

14.04
14.04/amd64
t
t/amd64
trusty
trusty/amd64

Do we have any idea what a correct value for this actually is?
This definitely feels like the sort of thing where we need more than one "image-metadata-url", because we're probably going to need a different upstream source for CentOS images than 'cloud-images.ubuntu.com'

This is the format from linuxcontainers.org. It can't be used directly, you have to make changes to enable cloud-init (see QA steps).

jameinel · Feb 14, 2017

jameinel approved these changes Feb 14, 2017

View changes

axw · Feb 14, 2017

$$merge$$

jujubot · Feb 14, 2017

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

deanmaniatis · Feb 19, 2017

@axw Andrew, go get github.com/axw/juju-tools breaks on my machine with this output /1/. If i follow
contribution guide /2/ then i get the following error /3/. I tried both master and 2.1 branches with first fetching proper dependencies but it fails. Any hints for me ?

/1/
http://paste.ubuntu.com/24026487/
/2/
https://github.com/juju/juju/blob/staging/CONTRIBUTING.md
/3/
http://paste.ubuntu.com/24026481/

axw · Feb 19, 2017

@deanmaniatis I've just pushed a fix for the "multiple-value ..." error seen in /3/. Please try again with juju on the 2.1 branch.

jujubot merged commit b1c40e4 into juju:2.1 Feb 14, 2017

+        +	// NOTE(axw) architecture should be passed into this function, as
+        +	// indicated by the TODO in the function's doc comment. When it is,
+        +	// use get rid of this.
+        +	const arch = "amd64"

+        +	var lastErr error
+        +	imageName := seriesLocalAlias(series, arch)
+        +	target := i.raw.GetAlias(imageName)
+        +	if target != "" {

+        +// specified series. The alias is juju-specific, to support the
+        +// user supplying a customised image (e.g. CentOS with cloud-init).
+        +func seriesLocalAlias(series, arch string) string {
+        +	return fmt.Sprintf("juju/%s/%s", series, arch)

+        +		return []string{path.Join(series, arch)}, nil
+        +	case os.CentOS:
+        +		if series == "centos7" && arch == "amd64" {
+        +			return []string{"centos/7/amd64"}, nil

juju/juju

lxd: prefer cached images, support CentOS #6970

axw commented Feb 13, 2017 •

Edited 1 time

axw
Feb 13, 2017

wallyworld reviewed Feb 13, 2017

View changes

axw commented Feb 13, 2017

jameinel requested changes Feb 13, 2017

View changes

jameinel approved these changes Feb 14, 2017

View changes

axw added some commits Feb 12, 2017

axw commented Feb 14, 2017

jujubot commented Feb 14, 2017

jujubot merged commit `b1c40e4` into juju:2.1 Feb 14, 2017

deanmaniatis commented Feb 19, 2017

axw commented Feb 19, 2017

juju/juju

Join GitHub today

lxd: prefer cached images, support CentOS #6970

Conversation

axw commented Feb 13, 2017 • edited Edited 1 time axw Feb 13, 2017

Description of change

QA steps

Documentation changes

Bug reference

wallyworld reviewed Feb 13, 2017 View changes

axw commented Feb 13, 2017

jameinel requested changes Feb 13, 2017 View changes

axw Feb 13, 2017 • edited Edited 1 time axw Feb 13, 2017

jameinel approved these changes Feb 14, 2017 View changes

axw added some commits Feb 12, 2017

axw commented Feb 14, 2017

jujubot commented Feb 14, 2017

jujubot merged commit b1c40e4 into juju:2.1 Feb 14, 2017

deanmaniatis commented Feb 19, 2017

axw commented Feb 19, 2017

axw commented Feb 13, 2017 •

Edited 1 time

axw
Feb 13, 2017

wallyworld reviewed Feb 13, 2017

View changes

jameinel requested changes Feb 13, 2017

View changes

axw Feb 13, 2017 •

Edited 1 time

axw
Feb 13, 2017

jameinel approved these changes Feb 14, 2017

View changes

jujubot merged commit `b1c40e4` into juju:2.1 Feb 14, 2017