Join GitHub today
GitHub is home to over 20 million developers working together to host and review code, manage projects, and build software together.
feature: remove uvtool dependency #6748
Conversation
reedobrien
added some commits
Nov 2, 2016
|
!!BaronBomburst!! |
reedobrien
changed the base branch from
staging
to
develop
Dec 23, 2016
|
!!BaronessBomburst!! |
|
!!CaractacusPott!! |
|
Note to self. see #6733 Failures to start kvm containers occurring after seeing This started occurring after merging in develop to the feature branch. #6733 looks suspiciously related. failing logs http://paste.ubuntu.com/23674094/ |
| + | ||
| +package kvm | ||
| + | ||
| +/* |
| "github.com/juju/errors" | ||
| "github.com/juju/utils/packaging/manager" | ||
| "github.com/juju/utils/series" | ||
| "github.com/juju/juju/container" | ||
| + "github.com/juju/juju/juju/paths" | ||
| ) | ||
| var requiredPackages = []string{ |
perrito666
Dec 23, 2016
Contributor
my question is most likely way out of scope but ill make it anyway, how good is this when deploying to something other than ubuntu (say for instance CentOS)
reedobrien
Jan 4, 2017
Contributor
AFAIU this is only supported on trusty and xenial on arm64 amd64 and ppc64el. No Centos or other.
| +// Details of the domain XML format are at: https://libvirt.org/formatdomain.html | ||
| +// We only use a subset, just enough to create instances in the pool. We don't | ||
| +// check any argument types here. We expect incoming params to be validate-able | ||
| +// by a function on the incoming domainParams. XXX: Or validate the params before being called. |
| +} | ||
| + | ||
| +// InterfaceInfo is to allow callers to pass InterfaceInfo in domainParams. | ||
| +type InterfaceInfo interface { |
| + DeviceName() string | ||
| +} | ||
| + | ||
| +type domainParams interface { |
| } | ||
| gc.TestingT(t) | ||
| } | ||
| + | ||
| +func supportedArch() bool { | ||
| + for _, arch := range []string{"amd64", "arm64", "ppc64el"} { |
perrito666
Dec 23, 2016
Contributor
We have (I think) constants for these values, if we dont, lets have them.
| + | ||
| +import "github.com/juju/utils" | ||
| + | ||
| +const libvirtUser = "libvirt-qemu" |
perrito666
Dec 23, 2016
Contributor
I believe this is only the case for ubuntu? if so I would add a comment stating that.
| + "github.com/juju/juju/juju/paths" | ||
| +) | ||
| + | ||
| +const ( |
perrito666
Jan 4, 2017
Contributor
yes, I believe I was, just making a very unclear comment but I believe I meant: convert this to comment newline const declaration
| + | ||
| +// Oner is an interface which allows us to use sync params to call | ||
| +// imagedownloads.One or pass in a fake for testing. | ||
| +type Oner interface { |
perrito666
Dec 23, 2016
Contributor
this is just a nit, this name is terrible for non english natives.
| + One() (*imagedownloads.Metadata, error) | ||
| +} | ||
| + | ||
| +// SyncParams conveys the information necessary for calling imagedownloads.One. |
| + jc "github.com/juju/testing/checkers" | ||
| +) | ||
| + | ||
| +// cacheInternalSuite is gocheck boilerplate. |
| +// Copyright 2016 Canonical Ltd. | ||
| +// Licensed under the AGPLv3, see LICENCE file for details. | ||
| + | ||
| +package imagedownloads |
perrito666
Dec 23, 2016
Contributor
I believe something went wrong here as you want the docs below and the Copyright above?
|
!!merryXmas!! |
|
Small note, I'm @jameinel on Github, not jam, I'm not sure who that is. :) For the failure case, my guess is there is something odd about what spaces your host machine is seen as being part of. The logic is supposed to be:
I'd happily do a hangout with you to debug this, because I'm currently working on tweaking our logic and our fallback paths. |
|
fixed nick. I'm happy to debug live, but that will most certainly be next Tues at the earliest :( |
added a commit
to reedobrien/juju
that referenced
this pull request
Jan 4, 2017
babbageclunk
suggested changes
Jan 4, 2017
Whew, this is a big chunk of work!
It mostly looks great, I have a few queries and suggestions. I started off commenting on typos but stopped after a while because it seemed too nitpicky.
| @@ -19,6 +24,9 @@ type kvmContainer struct { | ||
| // this allows for checking when we don't know, but using a | ||
| // value if we already know it (like in the list situation). | ||
| started *bool | ||
| + | ||
| + pathfinder func(string) (string, error) |
reedobrien
Jan 4, 2017
Contributor
renamed. This started out as an internal package interface, but became just a function.
| +github.com/digitalocean/go-libvirt and github.com/digitalocean/go-qemu. Those | ||
| +packages are nascent and alpha at the time of this writing. They implement pure | ||
| +go interfaces to libvirt and qemu by way of libvirt/qemu's custom RPC protocol. | ||
| +While this would reduce the number of commnads that require shelling out, it |
reedobrien
Jan 4, 2017
Contributor
I don't think typos are too nitpicky. They are distracting when reading... fixed
| + | ||
| +After the provisioner initializes the kvm environment, we synchronise (fetch if | ||
| +we don't have one) an ubuntu qcow image for the appropriate series and | ||
| +architecture. This happens in sync.go and uses Juju's simplestreams |
reedobrien
Jan 4, 2017
Contributor
I think I am having pronoun problems. Sync or fetching used to happen in uvtool and we shelled out to it. It depended on a python simplestreams impl. Juju already had some simplestreams parsing code for cloud images and tools (agents) streams. I added the bits for also handling content-download stream which is dependent on the existing simplestreams code. That is why the simplestreams code lives in juju/juju. The kvm code implements kvm as a juju container, so it is not useful outside juju/juju.
| +we don't have one) an ubuntu qcow image for the appropriate series and | ||
| +architecture. This happens in sync.go and uses Juju's simplestreams | ||
| +implementation in juju/environs/simplestreams and juju/environs/imagedownloads. | ||
| +Once we fetch a compressed ubuntu image we then uncompress and conviert it for |
| + | ||
| +Once the backing store is ready, we create a system disk and a datasource disk. | ||
| +The system disk is a sparse file with a maximum file size which uses the | ||
| +aforementioned backing store as it's base image. The data source disk is an iso |
| +image with user-data and meta-data for cloud-init's NoCloud method to configure | ||
| +our system. The cloud init data is written in kvm.go, via a call to machinery | ||
| +in juju/cloudconfig/containerinit/container_userdata.go. Destruction of a | ||
| +container rmeoves the system and data source disk files, but leaves the backing |
reedobrien
Jan 4, 2017
•
Edited 1 time
-
reedobrien
Jan 4, 2017
Contributor
-
Jan 4, 2017
can you tell I was riding in a car? That's my excuse;)
| +image with user-data and meta-data for cloud-init's NoCloud method to configure | ||
| +our system. The cloud init data is written in kvm.go, via a call to machinery | ||
| +in juju/cloudconfig/containerinit/container_userdata.go. Destruction of a | ||
| +container rmeoves the system and data source disk files, but leaves the backing |
babbageclunk
Jan 4, 2017
Member
Is this a potential resource leak? Should there be some kind of refcount for the backing store?
reedobrien
Jan 4, 2017
•
Edited 1 time
-
reedobrien
Jan 4, 2017
Contributor
-
Jan 4, 2017
no. The backing store is a read only copy of the ubuntu image, which domains are based on.
So multiple domain drives use the same backing store which cannot change without breaking anything that uses it. qcow doesn't support updating the backing store like squashfs (lxd) AFAIU.
There is the possibility that you remove all the containers (kvm domains) that are using a backing store while it sticks around. However, in prior discussions we decided it was best to leave it so re-deploying an app to a kvm container on the same machine would consistently have the same backing store. So we left it. This also means that 2 different machines started at different times could have different backing store versions, e.g. 16.04.1 vs 16.04.2. There is discussion about getting this through the controller to alleviate that variation, but that is beyond the scope of this PR. Which is to simply replace the functionality of uvtool which worked this way. There is definitely room for improvement in the future.
babbageclunk
Jan 4, 2017
Member
Right - if that's how uvtool was already working then this seems fine.
| @@ -13,6 +18,41 @@ var ( | ||
| TestStartParams = &startParams | ||
| ) | ||
| +func MakeTestableCreateMachineParams(params *CreateMachineParams, pathfinder func(string) (string, error), runCmd runFunc) { |
babbageclunk
Jan 4, 2017
Member
From the name I'd expect this to return a new CreateMachineParams. Maybe call it MakeCreateMachineParamsTestable?
| + call = append(call, args...) | ||
| + s.calls = append(s.calls, strings.Join(call, " ")) | ||
| + if s.err != nil { | ||
| + return s.err.Error(), s.err |
babbageclunk
Jan 4, 2017
Member
Why return this info twice? Is this how the real Run function behaves?
reedobrien
Jan 4, 2017
Contributor
Exactly that. The run func was pre-existing, and seems based on the utils.RunCommand which returns combined output.
| + } | ||
| + | ||
| + // Check if we've done this already. | ||
| + poolInfo, err := poolInfo(run) |
reedobrien
Jan 4, 2017
Contributor
I don't think so. The worker/provisioner.ContainerSetup.runInitialiser method calls Initialise() in a mutex. This allows it to be called more than once (idempotently) if there is an error after createPool. Let me know if you read otherwise.
| +// createPool creates the libvirt storage pool directory. runCmd and chownFunc | ||
| +// are here for testing. runCmd so we can check the right shell out calls are | ||
| +// made, and chownFunc because we cannot chown unless we are root. | ||
| +func createPool(pathfinder func(string) (string, error), runCmd runFunc, chownFunc func(string) error) error { |
babbageclunk
Jan 4, 2017
Member
I feel like this would make more sense as a method of a type that had runCmd and chownFunc members. Then you wouldn't need to pass the functions down into definePool, buildPool, startPool &c. You could construct one for testing with the right test functions.
(Also why pass in the pathfinder func here? Could you just pass the string instead? Maybe there's some reason later.)
Something like:
type PoolCreator struct {
runCmd runFunc
chownFunc func(string) error
}
func (pc *PoolCreator) create(poolDir string) error {
// ...
}
func (pc *PoolCreator) definePool(poolDir string) error {
}
// and so on
reedobrien
Jan 4, 2017
Contributor
I can see that and I think if this were ever going to be used outside of this file -- more or less package -- it would be worthwhile to refactor. As it is, it should never be used outside of here, so an unexported type with unexported methods just means a bunch more more constructors and test code. As it is I don't see a cost-benefit worth changing it, is that OK with you?
findPath nee pathfinder fakes paths.DataDir which takes a string arg and returns an error:(
Hopefully, one day soon digital ocean's libvirt and qemu libraries grow the functionality we need and this could all just go away:)
| + "pool-define-as", | ||
| + poolName, | ||
| + "dir", | ||
| + "-", "-", "-", "-", |
babbageclunk
Jan 4, 2017
Member
Why are these needed? I can't understand it from reading the virsh help. Worth a comment at least.
reedobrien
Jan 4, 2017
•
Edited 1 time
-
reedobrien
Jan 4, 2017
Contributor
-
Jan 4, 2017
They are positional parameters for defining other non-directory pools: e.g. file, lvm, scsi, disk, NFS.
pool-define-as <name> <type> [source-host] [source-path] [source-dev] [source-name] <target>
I think you can now use --type --target to just name params, but trusty still has the above signature.
Comment added.
babbageclunk
Jan 4, 2017
Member
Ah, I looked at the virsh help on xenial, that's probably why I couldn't see the other positional args.
| + | ||
| +// chownToLibvirt changes ownership of the provided directory to | ||
| +// libvirt-qemu:kvm. | ||
| +func chownToLibvirt(dir string) error { |
babbageclunk
Jan 4, 2017
Member
This is the real chownFunc, right? Does it get unit tested? If you actually passed around a func(string, int, int) error you could test this as well.
reedobrien
Jan 4, 2017
Contributor
The function itself isn't, but we really can't. You cannot chown unless you're root. It is a couple of syscalls which work on linux. Abstracting them out to make them moar test-able would just make them more complex thereby much more bug prone, while the tests themselves would be rather contrived testing the abstraction not the function.
I also considered shelling out ("chown", libvirtUser, libvirtGroup, path`) but shelling out is also bad. So I though keeping it as simple as possible was the least of all evils.
babbageclunk
Jan 4, 2017
Member
I mean, I'm ok with this not being tested, but in general it would be better to mock out the thing we can't call and pass the fake one in - it reduces the amount of code that only runs in CI.
| + | ||
| +// buildPool sets up libvirt internals for the guest pool. | ||
| +func buildPool(runCmd runFunc) error { | ||
| + // This can run without errror if the pool isn't active. |
| @@ -26,20 +29,29 @@ import ( | ||
| var ( | ||
| logger = loggo.GetLogger("juju.container.kvm") | ||
| + // KvmObjectFactory is the container factory for kvm containers. |
babbageclunk
Jan 4, 2017
Member
This comment feels a bit redundant. Is there more useful info that could go here? I guess you define it so that you can swap it out for testing?
reedobrien
Jan 4, 2017
•
Edited 1 time
-
reedobrien
Jan 4, 2017
Contributor
-
Jan 4, 2017
This is pre-existing code. Horatio asked for comments here. So I just added comments. I didn't actually get up into the container factory and container interface code that is used by the worker/provisioner/kvm-broker code. I mostly made changes that removed the need for uvtool but left the API untouched.
Modified the comment a smidge.
| +// check any argument types here. We expect incoming params to be validate-able | ||
| +// by a function on the incoming domainParams. | ||
| + | ||
| +// DiskInfo is an interface to allow callers to pass DiskInfo in domainParams. |
babbageclunk
Jan 4, 2017
Member
This doc comment is a bit too focused on how we use the thing rather than what the thing is. How about "DiskInfo represents a KVM disk image."?
reedobrien
Jan 4, 2017
Contributor
changed DiskInfo represents the type and location of a libvirt pool image.
| + Driver() string | ||
| +} | ||
| + | ||
| +// InterfaceInfo is to allow callers to pass InterfaceInfo in domainParams. |
babbageclunk
Jan 4, 2017
Member
Same here - "InterfaceInfo represents a network interface for a KVM instance being created."? (These might be wrong in the details, but I think something like this would be more useful than what's here.)
| + DiskInfo() []DiskInfo | ||
| + // Host returns the host name. | ||
| + Host() string | ||
| + // NetworkInfo returns a slice of InterfaceInfo. |
babbageclunk
Jan 4, 2017
Member
"NetworkInfo contains the network interfaces to create in the domain."?
| + return fmt.Sprintf("vd%s", string('a'+i)), nil | ||
| +} | ||
| + | ||
| +// Domain describes a libvirt domain. A domain is an instance of an operating |
| - if runtime.GOOS == "windows" { | ||
| - t.Skip("KVM is currently not supported on windows") | ||
| + if runtime.GOOS != "linux" || !supportedArch() { | ||
| + t.Skip("KVM is currently only supported on linux architecures amd64, arm64, and ppc64el") |
| + "github.com/juju/juju/juju/paths" | ||
| +) | ||
| + | ||
| +const ( |
perrito666
Jan 4, 2017
Contributor
yes, I believe I was, just making a very unclear comment but I believe I meant: convert this to comment newline const declaration
| + FType = "disk1.img" | ||
| +) | ||
| + | ||
| +// Oner is an interface which allows us to use sync params to call |
babbageclunk
Jan 4, 2017
Member
What does it actually do? The description shouldn't really be in terms of the implementation - I don't know what imagedownloads.One does. Maybe say that it finds the first matching item to download?
| +// syncParams conveys the information necessary for calling imagedownloads.One. | ||
| +type syncParams struct { | ||
| + arch, series, ftype string | ||
| + srcFunc func() simplestreams.DataSource |
babbageclunk
Jan 4, 2017
Member
As mentioned below I think just passing in a DataSource rather than a func() DataSource for testing would make more sense.
reedobrien
Jan 4, 2017
Contributor
It is a func because we allow passing in a custom simplestreams url (if you make your own golden images/tools/whatever) and building a datasource from that. If the arg is empty then the default source is used. This was an early bit of work from the simplestreams implementation, so it isn't fresh in my head. I'll look at it after I get through the rest of the review.
| + }() | ||
| + | ||
| + hash := sha256.New() | ||
| + _, err := io.Copy(io.MultiWriter(i.tmpFile, hash), r) |
babbageclunk
Jan 4, 2017
Member
Ooh, I haven't seen io.MultiWriter before (or this way of calculating a hash). Neat!
| + | ||
| + output, err := i.runCmd( | ||
| + "qemu-img", "convert", "-f", "qcow2", tmpPath, i.FilePath) | ||
| + fmt.Println(output) |
reedobrien
Jan 4, 2017
Contributor
| + } | ||
| + | ||
| + // We need to run some commands as root and some as libvirt. Alternatively, | ||
| + // we could just run commands as root then change ownership as nessecary, |
| + ) | ||
| + // if there isn't a runCmd passed in, we're not testing so use real | ||
| + // runFuncs. | ||
| + if params.runCmd == nil { |
babbageclunk
Jan 4, 2017
Member
Having test-specific code in the middle of this function is a bit surprising - can you make it so the normal construction of the CreateMachineParams has the right values and the test constructs one with the test stubs so that this is cleaner?
| + // for creating our guest. | ||
| + templateDir := filepath.Dir(params.UserDataFile) | ||
| + | ||
| + // Write out the meta-data to disk. |
| + params.disks = append(params.disks, diskInfo{source: imgPath, driver: "qcow2"}) | ||
| + params.disks = append(params.disks, diskInfo{source: dsPath, driver: "raw"}) | ||
| + | ||
| + // Write out domain XML file. |
| + } | ||
| + logger.Debugf("create domain: %s", out) | ||
| + | ||
| + // Start the domain guest. |
babbageclunk
Jan 4, 2017
•
Edited 1 time
-
babbageclunk
Jan 4, 2017
Member
-
Jan 4, 2017
The comments in here read as if they were a todo list while writing the function. I do that too, but in general those kind of comments should be deleted as you replace them with the code. At best they're just noise.
| + return err | ||
| +} | ||
| + | ||
| +// DestroyMachine destroys the virtual machine identified by hostname. |
| +} | ||
| + | ||
| +// writeMetadata writes out a metadata file with an UUID instance-id. The | ||
| +// meta-data file is use din the data source image along with user-data nee |
| @@ -0,0 +1,251 @@ | ||
| +// Copyright 2016 Canonical Ltd. |
| + for i, im := range items { | ||
| + md[i] = im.(*Metadata) | ||
| + } | ||
| + // TODO(ro) 2016-11-04 Do we really need to sort here? If so what on? Other |
babbageclunk
Jan 4, 2017
Member
If nothing else deterministic output is probably nice for testing, although I guess you can get the same benefit by sorting in the test.
| +// One gets Metadata for one content download item -- the most recent of | ||
| +// 'series', for architecture, 'arch', of the format 'ftype'. 'src' exists to | ||
| +// pass in a data source for testing. | ||
| +func One(arch, release, ftype string, src func() simplestreams.DataSource) (*Metadata, error) { |
| +// One gets Metadata for one content download item -- the most recent of | ||
| +// 'series', for architecture, 'arch', of the format 'ftype'. 'src' exists to | ||
| +// pass in a data source for testing. | ||
| +func One(arch, release, ftype string, src func() simplestreams.DataSource) (*Metadata, error) { |
reedobrien
Jan 5, 2017
Contributor
It doesn't really fetch anything it gets the metadata needed to fetch an item. However, Fetch is the name used in imagemetadata (tool/agent stream). So I copied it. In a preimplementation discussion we called the convenience method to get one metadata item One. So I named it one. It also fulfilled the Oner interface in kvm/sync.go. In my head I read it as imagedownloads.One, so it gets me one image download item from simplestreams. Familiarity permits brevity and all that.
Also, in sync there is a fetcher that fetches the one item, so I'd prefer not to confuse names there with FetchOne.
| @@ -303,6 +303,21 @@ func (i *InterfaceInfo) CIDRAddress() string { | ||
| return ipNet.String() | ||
| } | ||
| +// MAC returns the MacAddress member value. |
babbageclunk
Jan 4, 2017
Member
Hmm, it's weird that you had to come up with slightly different names so that the accessors needed for the interface didn't collide with the public attributes. That suggests that this isn't the right thing.
Did you really need an interface for InterfaceInfo for testing? The structs in network are passive data structures and can be directly constructed, so I think it would make more sense to use them for testing. Is there some reason you can't?
|
!!Iwouldliketobuyavowel!! |
|
!!doitonemoretime!! |
|
!!Iwouldliketospinthewheelalex!! |
|
!!roullette!! |
added a commit
to reedobrien/juju
that referenced
this pull request
Jan 5, 2017
|
!!buildme!! |
| - runCmdAsRoot = params.runCmd | ||
| + } | ||
| + if params.runCmdAsRoot == nil { | ||
| + params.runCmdAsRoot = run |
babbageclunk
Jan 5, 2017
Member
Thanks - I think I was just confused because of the way the defaulting was intertwined before.
|
$$merge$$ |
|
Status: merge request accepted. Url: http://juju-ci.vapour.ws:8080/job/github-merge-juju |
|
Build failed: Tests failed |
added a commit
to reedobrien/juju
that referenced
this pull request
Jan 5, 2017
|
$$merge$$ |
|
Status: merge request accepted. Url: http://juju-ci.vapour.ws:8080/job/github-merge-juju |
|
Build failed: Tests failed |
|
$$merge$$ |
|
!!build!! |
|
Status: merge request accepted. Url: http://juju-ci.vapour.ws:8080/job/github-merge-juju |
|
Build failed: Tests failed |
|
!!reallybuild!! |
|
$$ormerge$$ |
|
Status: merge request accepted. Url: http://juju-ci.vapour.ws:8080/job/github-merge-juju |
|
$$isthereanybodyoutther$$ |
jujubot
merged commit b4b0632
into
juju:develop
Jan 5, 2017
1 check was pending
|
Woot!
…
|
reedobrien commentedDec 22, 2016
•
Edited 1 time
-
reedobrien
Dec 22, 2016
follow up to #6580
@perrito666 @babbageclunk
This removes uvtool as a dependency, which should allow kvm containers to work on arm64, amd64, and ppc64el. QA has only been done by me on amd64 with vmaas.
NB: Since merging develop back into this branch there are some network updates that break kvm containers. When the rest of the network updates are complete and land it should be fine again. In order to test, once you have checked out the branch you will need to undo the changes so you can QA. Do that with
git revert -m 1 37a1a3b87fc54dc7391b4027e385ac48722ddf42 --no-commitQA:
juju bootstrap vmaas21 kvm/purego --build-agentjuju add-machinejuju add-machine --series trustyjuju deploy ubuntu --to kvm:0juju deploy mysql --to kvm:0juju deploy wordpress --to kvm:1juju deploy postgresql --to kvm:1 --series xenialjuju add-relation wordpress mysqlverify that machines 0 and 1 came up with xenial and trusty respectively
verify that machines 0/kvm/0 cam up with xenial and 0/kvm/1 with trusty
verify that machine 1/kvm/0 came up with trusty and 1/kvm/1 with xenial
verify the relation was added...
why not create a wordpress blog to be sure.
juju remove-application ubuntujuju remove-application postgresqlverify that the disk images for those are gone ,by
juju ssh 0 (or 1)and look in/var/lib/juju/kvm/gueststo be sure they were removed.