Uh oh!
There was an error while loading. Please reload this page.
CLOUDSTACK-8715: Add channel to Instances for Qemu Guest Agent - #1545
Conversation
wido
commented
May 13, 2016
This PR follows up on #985, but this one does not have a libvirt-java dependency. It only adds the channel in the XML for all Instances and it installs the Qemu Guest Agent inside the SSVM. This allows is in the future to use the available channel when libvirt-java has the proper support. |
wido
commented
May 13, 2016
@swill and @rhtyd could you take a look at this one? Once we fork libvirt-java (which I want to do when we switch to new repos) we can implement the actual commands in the KVM Agent. Just having the channels there also allows admins to send commands to Instances using virsh manually. |
swill
commented
May 13, 2016
Ya, I like this. I think this is a nice advancement. If we are going to try to get this into 4.9, I will need some help getting this moving forward cause we are pretty tight on time. @wido are you running this in production and what can you provide to show it is working as expected? I need to review the details for releasing when including system VM changes. I know we wanted to try to get StrongSwan into this release (assuming we can get it there), so that would require a new system VM template as well. |
@swill Yes, running this in production on our cloud I just started an Instance and the Agent (backport to 4.8, clean merge) produced this XML: There you see this channel added: In the Ubuntu 14.04 the Guest Agent is running: root@vhosting-relay:~# ps aux|grep qemu root 2789 0.0 0.0 17468 700 ? Ss 22:19 0:00 /usr/sbin/qemu-ga --daemonize -m virtio-serial -p /dev/virtio-ports/org.qemu.guest_agent.0 root 2890 0.0 0.0 10460 908 pts/6 S+ 22:20 0:00 grep qemu root@vhosting-relay:~# From the HV I can send commands through libvirt: [root@n06 ~]# virsh qemu-agent-command i-2-25-VM '{"execute":"guest-ping"}'
{"return":{}}
[root@n06 ~]#
I also re-started a Console Proxy which has a additional channel, that works as well: The commands afterwards: By having the channels already in place it is later just a matter of updating libvirt-java and starting to use this communication channel. |
wido
commented
May 13, 2016
@swill I added a small fix for inside the SSVM. During some additional manual testing I found that the SSVM is picky in the order in the XML definition. My second commit makes sure it doesn't use the first VirtIO port, but uses the proper port. I tested that fix in production as well, works just fine. |
swill
commented
May 13, 2016
ok, thank you sir. 👍 |
2e65de2 to
7ac6627Comparewido
commented
May 19, 2016
@rhtyd and @DaanHoogland Could you maybe take a quick look at this one? We could finally get the first part of this in. |
| final VirtioSerialDef vserial = new VirtioSerialDef(vmTO.getName(), null); | ||
| devices.addDevice(vserial); | ||
| devices.addDevice(new ChannelDef(vmTO.getName() + ".vport", ChannelDef.ChannelType.UNIX, | ||
| "/var/lib/libvirt/qemu/" + vmTO.getName() + ".agent")); |
There was a problem hiding this comment.
these names and paths should not be hardcoded here.
There was a problem hiding this comment.
agree, @wido do you mind moving them something configurable via global settings or agent.properties
There was a problem hiding this comment.
This has always been hardcoded, it's just that I'm moving this part.
This is the SSVM socket which is already there. The socket patch code wants to find the .agent there. Not much I can do there.
DaanHoogland
commented
May 19, 2016
apart from some code style and convention things LGTM |
| String state = getAttrValue("target", "state", channel); | ||
| ChannelDef def = null; | ||
| if (state == null || state.length() == 0) { |
There was a problem hiding this comment.
we can use Strings.isNullOrEmpty here
yadvr
commented
May 19, 2016
In general, a good idea and would be useful for flushing out disk IO and doing clean shutdown of VRs and vms. There are some outstanding issues, but the feature is nice. I've not tested this, I suppose @wido would have tested this for Debian/Ubuntu based kvm hosts, we'll also need testing efforts on centos6 and 7 based kvm hosts. |
DaanHoogland
commented
May 19, 2016
@swill, did this pass the (not so C)I? |
wido
commented
May 19, 2016
Thanks for the feedback, I will work on that tomorrow. @rhtyd I tested this on both Ubuntu and CentOS systems. The main issue with Ubuntu is AppArmor where libvirt isn't allowed to write in /var/lib/qemu. I'll make that directory configurable, but keep in mind that on all platforms I know that is the directory where the sockets are stored. |
Sorry, I need to queue this one up... I will wait for @wido's changes though so I am testing his latest code. |
wido
commented
May 19, 2016
@swill I will work on the changes tomorrow morning (my time, CET). Shouldn't be very hard to do. |
swill
commented
May 20, 2016
CI RESULTSSummary of the problem(s): Associated Uploads
Uploads will be available until Comment created by |
swill
commented
May 20, 2016
These failures are not related to this PR, but if you have ideas why they are happening I would be interested. I will wait till @wido has a chance to work on this PR to run the CI again. Thx... |
fb41aaa to
7960733Compareyadvr
commented
Nov 21, 2016
@blueorangutan test |
blueorangutan
commented
Nov 21, 2016
@rhtyd a Trillian-Jenkins test job (centos7 mgmt + kvm-centos7) has been kicked to run smoke tests |
yadvr
commented
Nov 21, 2016
@wido the Trillian env failed to run the tests for your latest PR due to following error: Please modify the code to still work where an explicit path may not be defined, and that with your change KVM agent would run on CentOS6/7. Thanks. |
wido
commented
Nov 23, 2016
@jburwell @rhtyd I fixed most of the comments. The NPE should be fixed and the StringUtils comments as well. I didn't refactor the other stuff @jburwell mentioned as my code is in-line with how the existing classes are written in the same files. A new PR could do a refactor of those whole files, but it wouldn't change anything for a end-result. The functionality is still that we have a Qemu Guest Agent inside the VMs. But this PR is open since May. Could we please merge this one in? |
yadvr
commented
Nov 23, 2016
@wido can you confirm if you've fixed the NPE issue I've shared in the comment above? |
wido
commented
Nov 23, 2016
Yes @rhtyd, I added a check for NULL there. |
yadvr
commented
Nov 23, 2016
yadvr
commented
Nov 23, 2016
@blueorangutan package |
blueorangutan
commented
Nov 23, 2016
@rhtyd a Jenkins job has been kicked to build packages. I'll keep you posted as I make progress. |
blueorangutan
commented
Nov 23, 2016
Packaging result: ✔centos6 ✔centos7 ✔debian. JID-237 |
yadvr
commented
Nov 23, 2016
@blueorangutan test |
blueorangutan
commented
Nov 23, 2016
@rhtyd a Trillian-Jenkins test job (centos7 mgmt + kvm-centos7) has been kicked to run smoke tests |
yadvr
commented
Nov 23, 2016
@wido please fix the build failures: |
This commit adds a additional VirtIO channel with the name 'org.qemu.guest_agent.0' to all Instances. With the Qemu Guest Agent the Hypervisor gains more control over the Instance if these tools are present inside the Instance, for example: * Power control * Flushing filesystems * Fetching Network information In the future this should allow safer snapshots on KVM since we can instruct the Instance to flush the filesystems prior to snapshotting the disk. More information: http://wiki.qemu.org/Features/QAPI/GuestAgent Keep in mind that on Ubuntu AppArmor still needs to be disabled since the default AppArmor profile doesn't allow libvirt to write into /var/lib/libvirt/qemu This commit does not add any communication methods through API-calls, it merely adds the channel to the Instances and installs the Guest Agent in the SSVMs. With the addition of the Qemu Guest Agent channel a second channel appears in /dev on a SSVM as a VirtIO port. The order in which the ports are defined in the XML matters for the naming inside the SSVM VM and by not relying on /dev/vportXX but looking for a static name the SSVM still boots properly if the order in the XML definition is changed. A SSVM with both ports attached will have something like this: root@v-215-VM:~# ls -l /dev/virtio-ports total 0 lrwxrwxrwx 1 root root 11 May 13 21:41 org.qemu.guest_agent.0 -> ../vport0p2 lrwxrwxrwx 1 root root 11 May 13 21:41 v-215-VM.vport -> ../vport0p1 root@v-215-VM:~# ls -l /dev/vport* crw------- 1 root root 251, 1 May 13 21:41 /dev/vport0p1 crw------- 1 root root 251, 2 May 13 21:41 /dev/vport0p2 root@v-215-VM:~# In this case the SSVM port points to /dev/vport0p1, but if the order in the XML is different it might point to /dev/vport0p2 By looking for a portname with a pre-defined pattern in /dev/virtio-ports we do not rely on the order in the XML definition. Signed-off-by: Wido den Hollander <wido@widodh.nl>
wido
commented
Nov 23, 2016
@rhtyd Yes, fixed. Forgot to commit that change locally. Pushed a new version. |
yadvr
commented
Nov 23, 2016
@blueorangutan package |
blueorangutan
commented
Nov 23, 2016
@rhtyd a Jenkins job has been kicked to build packages. I'll keep you posted as I make progress. |
blueorangutan
commented
Nov 23, 2016
Packaging result: ✖centos6 ✖centos7 ✖debian. JID-239 |
yadvr
commented
Nov 24, 2016
@blueorangutan package |
blueorangutan
commented
Nov 24, 2016
@rhtyd a Jenkins job has been kicked to build packages. I'll keep you posted as I make progress. |
blueorangutan
commented
Nov 24, 2016
Packaging result: ✔centos6 ✔centos7 ✔debian. JID-240 |
yadvr
commented
Nov 24, 2016
@blueorangutan test |
blueorangutan
commented
Nov 24, 2016
@rhtyd a Trillian-Jenkins test job (centos7 mgmt + kvm-centos7) has been kicked to run smoke tests |
blueorangutan
commented
Nov 24, 2016
Trillian test result (tid-421)
|
yadvr
commented
Nov 24, 2016
LGTM, the tests are all good as one (one failure was due to cleanup in the create template test case and was env related). Based on this I'm merging this. |
This commit adds a additional VirtIO channel with the name
'org.qemu.guest_agent.0' to all Instances.
With the Qemu Guest Agent the Hypervisor gains more control over the Instance if
these tools are present inside the Instance, for example:
In the future this should allow safer snapshots on KVM since we can instruct the
Instance to flush the filesystems prior to snapshotting the disk.
More information: http://wiki.qemu.org/Features/QAPI/GuestAgent
Keep in mind that on Ubuntu AppArmor still needs to be disabled since the default
AppArmor profile doesn't allow libvirt to write into /var/lib/libvirt/qemu
This commit does not add any communication methods through API-calls, it merely
adds the channel to the Instances and installs the Guest Agent in the SSVMs.