Libvirt updates #552

ijc · 2017-05-19T00:31:28Z

Two updates:

Pass an entire libvirt domain configuration in spec.Properties.Domain, allowing for more flexibility.
Expose any assigned IP addresses in the instance.Description if asked too (properties == true).

YujiOshima · 2017-05-19T10:53:03Z

Hi @ijc25 !
Flexible configuration look so nice!
Some points.

Do we need to set all Domain config? Do you have any default value? Previous code looks having default cpu, memory config.
Though previous code checked property Kernel exist, this PR looks does not. Is it OK?
Maybe it is better to use version of libvirt-go after c3209e4.
Ref https://github.com/libvirt/libvirt-go/blob/0e52f11fc51cb385bdc638bd4a731e433e610f99/connect.go

ijc · 2017-05-19T17:38:17Z

Do we need to set all Domain config? Do you have any default value? Previous code looks having default cpu, memory config.

The new code doesn't set any defaults, that's true. I could add them if you think it is useful, neither the hyperkit nor packet instance plugins seem to do that though.

Though previous code checked property Kernel exist, this PR looks does not. Is it OK?

There are valid VM configurations which do not boot a kernel directly, for example ones which boot BIOS or UEFI firmware and a bootloader on the disk so there would be no kernel given in that case.

Maybe it is better to use version of libvirt-go after c3209e4.

This PR switches to v3.3.0 which is the latest release, there has only been that one commit (c3209e4 which you referenced) since and it is to do with go 1.5 compat in the lxc backend so probably not too interesting to us.

ijc · 2017-05-19T22:20:34Z

CI failure is:

# github.com/docker/infrakit/vendor/github.com/libvirt/libvirt-go
vendor/github.com/libvirt/libvirt-go/lxc.go:63: cannot use cfdlist (type *C.int) as type unsafe.Pointer in argument to func literal
vendor/github.com/libvirt/libvirt-go/lxc.go:85: cannot use coldfdlist (type *C.int) as type unsafe.Pointer in argument to func literal

which might be down to the new commit in libvirt-go: libvirt/libvirt-go@c3209e4. I'll add a patch to update to that and see.

GordonTheTurtle · 2017-05-19T22:24:15Z

Please sign your commits following these rules:
https://github.com/moby/moby/blob/master/CONTRIBUTING.md#sign-your-work
The easiest way to do this is to amend the last commit:

$ git clone -b "libvirt-updates" git@github.com:ijc25/infrakit.git somewhere
$ cd somewhere
$ git rebase -i HEAD~842354025144
editor opens
change each 'pick' to 'edit'
save the file and quit
$ git commit --amend -s --no-edit
$ git rebase --continue # and repeat the amend for each commit
$ git push -f

Amending updates the existing PR. You DO NOT need to open a new one.

YujiOshima · 2017-05-20T15:26:02Z

@ijc25 Thank you for the reply.
Since the presence or absence of a default value is not essential, I think that it will be fine as it is.
I was wondering if there is an implied default value.

Kernel

I understood, thanks.

ijc · 2017-05-23T10:01:42Z

@YujiOshima

I was wondering if there is an implied default value.

It's not impossible that there is one at the libvirt level. I actually don't know. I think it probably makes sense to leave it to libvirt to have defaults, rather than add a second layer of potentially contradictory defaults on top.

chungers · 2017-05-25T07:09:22Z

@ijc25 - there are errors with the tests

?   	github.com/docker/infrakit/pkg/plugin/instance/image/gcp	[no test files]
time="2017-05-25T03:30:14Z" level=error msg="Creating Domain: virError(Code=1, Domain=20, Message='internal error: missing domain type attribute')" instance=infrakit-7adef51f 
--- FAIL: TestBasicLifecycle (0.00s)
	Error Trace:	instance_test.go:78
	Error:		Received unexpected error:
			virError(Code=1, Domain=20, Message='internal error: missing domain type attribute')
			Creating Domain
			github.com/docker/infrakit/pkg/plugin/instance/libvirt.libvirtPlugin.Provision
				github.com/docker/infrakit/pkg/plugin/instance/libvirt/_test/_obj_test/instance.go:283
			github.com/docker/infrakit/pkg/plugin/instance/libvirt.(*libvirtPlugin).Provision
				<autogenerated>:2
			github.com/docker/infrakit/pkg/plugin/instance/libvirt.TestBasicLifecycle
				/home/ubuntu/.go_workspace/src/github.com/docker/infrakit/pkg/plugin/instance/libvirt/instance_test.go:77
			testing.tRunner
				/usr/local/go/src/testing/testing.go:657
			runtime.goexit
				/usr/local/go/src/runtime/asm_amd64.s:2197

Can you have a look?

ijc · 2017-05-25T10:03:11Z

Sure, I somehow missed that in the logs. In hindsight it is rather suspicious that pkg/plugin/instance/libvirt/instance_test.go wasn't touched by this PR.

codecov · 2017-05-25T13:40:57Z

Codecov Report

Merging #552 into master will decrease coverage by 0.02%.
The diff coverage is 58.82%.

@@            Coverage Diff             @@
##           master     #552      +/-   ##
==========================================
- Coverage   56.55%   56.52%   -0.03%     
==========================================
  Files          57       57              
  Lines        3977     3945      -32     
==========================================
- Hits         2249     2230      -19     
+ Misses       1440     1433       -7     
+ Partials      288      282       -6

Impacted Files	Coverage Δ
pkg/plugin/instance/libvirt/instance.go	`47.38% <58.82%> (-1.28%)`	⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update fcfc8d1...fed472d. Read the comment docs.

ijc · 2017-05-25T13:41:28Z

@chungers fixed and force pushed. I added a test case for that specific failure as well as for another I found having fixed the first one.

Amongst other changes this fixed Domain.ListAllInterfaceAddresses() when there are multiple interfaces. Signed-off-by: Ian Campbell <ian.campbell@docker.com>

This maps directly to the libvirtxml.Domain object which allows the user much more flexibility and is more in line with other instance plugins. Switch the example configuration to yml which is a little more readable. Signed-off-by: Ian Campbell <ian.campbell@docker.com>

Thus tries the qemu guest agent first (if it is running) and falls back to interrogating the libvirt leases database (which is not available with `qemu://session`). Filters devices which have no addresses on them. Modify the example configuration to add a network interface and the necessary stanza to setup the virtio channel which is used by the guest agent. Signed-off-by: Ian Campbell <ian.campbell@docker.com>

This pulls in a single commit since v3.3.0 libvirt/libvirt-go@c3209e4 which improves compatibility with Go 1.5 resolving: vendor/github.com/libvirt/libvirt-go/lxc.go:63: cannot use cfdlist (type *C.int) as type unsafe.Pointer in argument to func literal vendor/github.com/libvirt/libvirt-go/lxc.go:85: cannot use coldfdlist (type *C.int) as type unsafe.Pointer in argument to func literal Signed-off-by: Ian Campbell <ian.campbell@docker.com>

This is easier to work with in the context of libvirt than trying to assign IP addresses. The network object should be configured with static mappings. Signed-off-by: Ian Campbell <ian.campbell@docker.com>

ijc · 2017-05-30T10:21:24Z

Rebased, passed properties=true to DescribeInstances and added TestInvalidProperties to improve coverage.

chungers

Thanks! Waiting for the tests to pass before merging...

ijc · 2017-05-31T09:17:10Z

@chungers tests have passed ;-)

GordonTheTurtle added the dco/no label May 19, 2017

ijc force-pushed the libvirt-updates branch from ef8170c to 63affb4 Compare May 19, 2017 22:26

GordonTheTurtle added dco/no and removed dco/no labels May 19, 2017

ijc force-pushed the libvirt-updates branch from fd8a91f to 971eb06 Compare May 25, 2017 13:40

GordonTheTurtle removed the dco/no label May 25, 2017

Ian Campbell added 5 commits May 30, 2017 11:19

Bump libvirt bindings from v3.2.0 to v3.3.0

d541a47

Amongst other changes this fixed Domain.ListAllInterfaceAddresses() when there are multiple interfaces. Signed-off-by: Ian Campbell <ian.campbell@docker.com>

libvirt: Use the logical ID as the MAC for the first network interface.

760b5b5

This is easier to work with in the context of libvirt than trying to assign IP addresses. The network object should be configured with static mappings. Signed-off-by: Ian Campbell <ian.campbell@docker.com>

ijc force-pushed the libvirt-updates branch from 971eb06 to 760b5b5 Compare May 30, 2017 10:20

Merge branch 'master' into libvirt-updates

fed472d

GordonTheTurtle added the dco/no label May 30, 2017

chungers approved these changes May 30, 2017

View reviewed changes

chungers merged commit aa4c34d into docker-archive:master May 31, 2017

ijc deleted the libvirt-updates branch June 7, 2017 17:05

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Libvirt updates #552

Libvirt updates #552

ijc commented May 19, 2017

YujiOshima commented May 19, 2017

ijc commented May 19, 2017

ijc commented May 19, 2017

GordonTheTurtle commented May 19, 2017

YujiOshima commented May 20, 2017 •

edited

Loading

ijc commented May 23, 2017

chungers commented May 25, 2017

ijc commented May 25, 2017

codecov bot commented May 25, 2017 •

edited

Loading

ijc commented May 25, 2017

ijc commented May 30, 2017

chungers left a comment

ijc commented May 31, 2017

Libvirt updates #552

Libvirt updates #552

Conversation

ijc commented May 19, 2017

YujiOshima commented May 19, 2017

ijc commented May 19, 2017

ijc commented May 19, 2017

GordonTheTurtle commented May 19, 2017

YujiOshima commented May 20, 2017 • edited Loading

ijc commented May 23, 2017

chungers commented May 25, 2017

ijc commented May 25, 2017

codecov bot commented May 25, 2017 • edited Loading

Codecov Report

ijc commented May 25, 2017

ijc commented May 30, 2017

chungers left a comment

Choose a reason for hiding this comment

ijc commented May 31, 2017

YujiOshima commented May 20, 2017 •

edited

Loading

codecov bot commented May 25, 2017 •

edited

Loading