tests leak file descriptors

[marmarek]

When running a lot of tests in one call it eventually leads to "Too many open files" error.
Some preliminary debugging with strace reveals that:

1. Most of leaked FDs are libvirt connections (pipe, socket), but some of them are also QubesDB connections (socket)
2. Libvirt connections are made only in two places: VMMConnection.init_vmm_connection, SystemTestsMixin.setUp
3. All leaked libvirt connections are from the second place (SystemTestsMixin)
4. Theoretically those are cleaned in SystemTestsMixin.tearDown, but they aren't - the function is called, but self.conn.close() does not close them

I guess it's because the connection is still referenced from a couple of QubesVm objects (vm.libvirt_domain), which aren't garbage collected because of some circular dependency (or some other reason). The same probably applies to QubesDB connections.

[marmarek]

@woju any idea? This wouldn't be the first place where not properly releasing QubesVM objects have some downsides...

[marmarek]

@woju any idea? This wouldn't be the first place where not properly releasing QubesVM objects have some downsides...

[woju]

On Wed, Nov 04, 2015 at 05:44:11PM -0800, Marek Marczykowski-Górecki wrote:

@woju any idea? This wouldn't be the first place where not properly
releasing QubesVM objects have some downsides...

If self.conn.close() won't really close them, that's bug in libvirt or
that python wrapper of theirs...

[woju]

On Wed, Nov 04, 2015 at 05:44:11PM -0800, Marek Marczykowski-Górecki wrote:

@woju any idea? This wouldn't be the first place where not properly
releasing QubesVM objects have some downsides...

If self.conn.close() won't really close them, that's bug in libvirt or
that python wrapper of theirs...

[marmarek]

self.conn.close() only decrease refcount, by design:
http://libvirt.org/html/libvirt-libvirt-host.html#virConnectClose

Best Regards,
Marek Marczykowski-Górecki
Invisible Things Lab
A: Because it messes up the order in which people normally read text.
Q: Why is top-posting such a bad thing?

[marmarek]

self.conn.close() only decrease refcount, by design:
http://libvirt.org/html/libvirt-libvirt-host.html#virConnectClose

Best Regards,
Marek Marczykowski-Górecki
Invisible Things Lab
A: Because it messes up the order in which people normally read text.
Q: Why is top-posting such a bad thing?

[woju]

On Thu, Nov 05, 2015 at 01:21:58PM -0800, Marek Marczykowski-Górecki wrote:

self.conn.close() only decrease refcount, by design:
http://libvirt.org/html/libvirt-libvirt-host.html#virConnectClose

Ah, if so, then judging by http://libvirt.org/git/?p=libvirt-python.git;a=blob;f=libvirt-override-virConnect.py#l11
the descriptor is freed when the virConnect is garbage collected.
See https://docs.python.org/2/reference/datamodel.html#object.__del__,
particulary the "Note:" there.

[woju]

On Thu, Nov 05, 2015 at 01:21:58PM -0800, Marek Marczykowski-Górecki wrote:

self.conn.close() only decrease refcount, by design:
http://libvirt.org/html/libvirt-libvirt-host.html#virConnectClose

Ah, if so, then judging by http://libvirt.org/git/?p=libvirt-python.git;a=blob;f=libvirt-override-virConnect.py#l11
the descriptor is freed when the virConnect is garbage collected.
See https://docs.python.org/2/reference/datamodel.html#object.__del__,
particulary the "Note:" there.

[marmarek]

Great. So how can we fix that? Preferably in some generic way...
As a workaround, I see we may simply iterate over each QubesVM object
and release vm.libvirt_domain (del?). But this is neither elegant, nor
complete (probably will not handle qubes.xml reload, VM restarts and
such).

Best Regards,
Marek Marczykowski-Górecki
Invisible Things Lab
A: Because it messes up the order in which people normally read text.
Q: Why is top-posting such a bad thing?

[marmarek]

Great. So how can we fix that? Preferably in some generic way...
As a workaround, I see we may simply iterate over each QubesVM object
and release vm.libvirt_domain (del?). But this is neither elegant, nor
complete (probably will not handle qubes.xml reload, VM restarts and
such).

Best Regards,
Marek Marczykowski-Górecki
Invisible Things Lab
A: Because it messes up the order in which people normally read text.
Q: Why is top-posting such a bad thing?

[woju]

On Thu, Nov 05, 2015 at 03:42:04PM -0800, Marek Marczykowski-Górecki wrote:

Great. So how can we fix that? Preferably in some generic way...
As a workaround, I see we may simply iterate over each QubesVM object
and release vm.libvirt_domain (del?). But this is neither elegant, nor
complete (probably will not handle qubes.xml reload, VM restarts and
such).

We could make a wrapper around the object, which will be wrapper's
private attribute. No one else will have reference to it. The wrapper
will pass most of the calls untouched, but the close() call will be done
in such a way that will ensure freeing the descriptor.

[woju]

On Thu, Nov 05, 2015 at 03:42:04PM -0800, Marek Marczykowski-Górecki wrote:

Great. So how can we fix that? Preferably in some generic way...
As a workaround, I see we may simply iterate over each QubesVM object
and release vm.libvirt_domain (del?). But this is neither elegant, nor
complete (probably will not handle qubes.xml reload, VM restarts and
such).

We could make a wrapper around the object, which will be wrapper's
private attribute. No one else will have reference to it. The wrapper
will pass most of the calls untouched, but the close() call will be done
in such a way that will ensure freeing the descriptor.

[marmarek]

Ok, a simple test reveals that libvirt_conn.close() doesn't work at all:

c = libvirt.open('xen:///')
c.close()
del c

And the socket is still open. I'm still not sure where the problem is

• our usage, or libvirt pythin bindings. But I tend to think the later
  one.

Anyway, can we not open a new libvirt connection for every single test?
Instead, reuse vmm.libvirt_conn for that. Any drawbacks?

Best Regards,
Marek Marczykowski-Górecki
Invisible Things Lab
A: Because it messes up the order in which people normally read text.
Q: Why is top-posting such a bad thing?

[marmarek]

Ok, a simple test reveals that libvirt_conn.close() doesn't work at all:

c = libvirt.open('xen:///')
c.close()
del c

And the socket is still open. I'm still not sure where the problem is

• our usage, or libvirt pythin bindings. But I tend to think the later
  one.

Anyway, can we not open a new libvirt connection for every single test?
Instead, reuse vmm.libvirt_conn for that. Any drawbacks?

Best Regards,
Marek Marczykowski-Górecki
Invisible Things Lab
A: Because it messes up the order in which people normally read text.
Q: Why is top-posting such a bad thing?

[marmarek]

Found it. When some event loop implementation is registered (libvirt.virEventRegisterDefaultImpl), connections are closed as part of that event loop (libvirt.virEventRunDefaultImpl). We do not call it (when running tests).
Actually FD leak is not the only problem - the other is not sending keep-alives - which we had to disable on daemon side because of that.

Background

1. We need that event loop for handling libvirt events, in Qubes Manager (through QubesWatch)
2. libvirt.virEventRegisterDefaultImpl (or other libvirt.virEventRegisterImpl) must be called before opening the connection

For now (R3.1 or even backport for R3.0)

I see two options here:

1. Always start a event loop, not only in Qubes Manager
2. Try to register event loop only when needed (which probably would involve opening another connection after registering it)

I think I've already tried the second option some time ago (but I maybe not tried with the second connection). That would be better (performance wise) than starting event loop for every qvm-ls. On the other hand, qvm-backup could use keep-alives. Anyway I'll retry the second option and fallback to the first one.

For R4.0/R4.1 (qubesd)

It will not be needed anymore, since "client" tools will not speak to libvirt directly.

[marmarek]

Found it. When some event loop implementation is registered (libvirt.virEventRegisterDefaultImpl), connections are closed as part of that event loop (libvirt.virEventRunDefaultImpl). We do not call it (when running tests).
Actually FD leak is not the only problem - the other is not sending keep-alives - which we had to disable on daemon side because of that.

Background

1. We need that event loop for handling libvirt events, in Qubes Manager (through QubesWatch)
2. libvirt.virEventRegisterDefaultImpl (or other libvirt.virEventRegisterImpl) must be called before opening the connection

For now (R3.1 or even backport for R3.0)

I see two options here:

1. Always start a event loop, not only in Qubes Manager
2. Try to register event loop only when needed (which probably would involve opening another connection after registering it)

I think I've already tried the second option some time ago (but I maybe not tried with the second connection). That would be better (performance wise) than starting event loop for every qvm-ls. On the other hand, qvm-backup could use keep-alives. Anyway I'll retry the second option and fallback to the first one.

For R4.0/R4.1 (qubesd)

It will not be needed anymore, since "client" tools will not speak to libvirt directly.

[marmarek]

Automated announcement from builder-github

The package qubes-core-dom0-3.0.26-1.fc20 has been pushed to the r3.0 testing repository for dom0.
To test this update, please install it with the following command:

sudo qubes-dom0-update --enablerepo=qubes-dom0-current-testing

Changes included in this update

[marmarek]

Automated announcement from builder-github

The package qubes-core-dom0-3.0.26-1.fc20 has been pushed to the r3.0 testing repository for dom0.
To test this update, please install it with the following command:

sudo qubes-dom0-update --enablerepo=qubes-dom0-current-testing

Changes included in this update

[marmarek]

Automated announcement from builder-github

The package qubes-core-dom0-3.0.26-1.fc20 has been pushed to the r3.0 stable repository for dom0.
To install this update, please use the standard update command:

sudo qubes-dom0-update

Or update dom0 via Qubes Manager.

Changes included in this update

[marmarek]

Automated announcement from builder-github

The package qubes-core-dom0-3.0.26-1.fc20 has been pushed to the r3.0 stable repository for dom0.
To install this update, please use the standard update command:

sudo qubes-dom0-update

Or update dom0 via Qubes Manager.

Changes included in this update