Bug 24446

Created attachment 30256 [details] [review]
Provide option to track ram/loop devices

Hey,

FWIW, we want to properly handle loop devices (ram devices are probably not that interesting though) - there was a thread on the list about this

 http://lists.freedesktop.org/archives/devkit-devel/2009-August/000418.html

and I think this might be the way forward. We probably also want to export D-Bus methods to setup/teardown loop devices much like we allow setup/teardown of md devices.

Anyway, writing a test-suite sounds like a good idea. It might be nice to use loop devices for this but AFAIK the kernel doesn't handle partitioning on these. I suspect we could fix that in the kernel but...

On the other hand, the kernel md driver does (it was a recent change in the kernel - maybe 6-12 months back) so I suspect we can set up a md linear device backed by a loop device. Then we test things like partitioning on the md device.

Thoughts?

Your referenced (In reply to comment #2)
> FWIW, we want to properly handle loop devices (ram devices are probably not
> that interesting though) - there was a thread on the list about this
> 
>  http://lists.freedesktop.org/archives/devkit-devel/2009-August/000418.html
> 
> and I think this might be the way forward.

That would do as well, of course. :-)

> Anyway, writing a test-suite sounds like a good idea. It might be nice to use
> loop devices for this but AFAIK the kernel doesn't handle partitioning on
> these. I suspect we could fix that in the kernel but...

We can test the partition table detection with ram devices (you can run fdisk on /dev/ram0), but yes, contrary to what fdisk claims there won't actually be a /dev/ram0p1.

Disk /dev/ram0: 67 MB, 67108864 bytes
[...]
/dev/ram0p1               1           8       64228+  83  Linux

> On the other hand, the kernel md driver does (it was a recent change in the
> kernel - maybe 6-12 months back) so I suspect we can set up a md linear device
> backed by a loop device.

Brilliant trick, thanks! I suspect this will also work with ramdisks (which are much faster for tests, since you don't need to write stuff on disk). It would introduce a dependency on mdadm for the test suite, but I think that's fine. This stuff can't ever be run on a build machine during package build, so it's just for local usage anyway.

BTW, I started the test suite in shell to avoid introducing more dependencies, but I really don't like it, it becomes clumsy very quickly. Would you mind having a Python (unittest is sooo sweet) script there?

(In reply to comment #3)
> We can test the partition table detection with ram devices (you can run fdisk
> on /dev/ram0), but yes, contrary to what fdisk claims there won't actually be a
> /dev/ram0p1.
> 
> Disk /dev/ram0: 67 MB, 67108864 bytes
> [...]
> /dev/ram0p1               1           8       64228+  83  Linux

Yeah, fdisk is not really related to the kernel (except that the passed file is used as the "disk".

> Brilliant trick, thanks! I suspect this will also work with ramdisks (which are
> much faster for tests, since you don't need to write stuff on disk).

Yeah. Hmm. We need to figure out if we want to support ram devices in DeviceKit-disks. It might make sense but we need to do it in a way such that the concerns in

http://lists.freedesktop.org/archives/devkit-devel/2009-August/000422.html

are addressed. I really would like to support all block devices exported by the kernel - mostly because people set up stacked block devices (like md) using these - and if we ignore ram/loop then things won't work properly.

> It would
> introduce a dependency on mdadm for the test suite, but I think that's fine.
> This stuff can't ever be run on a build machine during package build, so it's
> just for local usage anyway.
> 
> BTW, I started the test suite in shell to avoid introducing more dependencies,
> but I really don't like it, it becomes clumsy very quickly. Would you mind
> having a Python (unittest is sooo sweet) script there?

Python sounds like a good tool for this job.

I guess another dependency, that won't be available on builders, is the presence of a system message bus and also the fact you that need to be uid 0 for the test suite to work.

But that's fine, initially the test suite only works on a running system - if we want to be cute later on, we can always do things like run the test in a virtual machine (like the dracut test suite does).

(In reply to comment #4)

> I really would like to support all block devices exported by the
> kernel - mostly because people set up stacked block devices (like md) using
> these - and if we ignore ram/loop then things won't work properly.

I just played around with this, and it at least seems to work well enough to have a /dev/md127 and a /dev/md127p1.

I think this is good enough for at least an initial test suite, until this actually grows to testing the detailled functionality of raids.

I'll centralize this in the test suite, so that there's just one point that says "give me a block device I can mess with", so we can always rip it out later on.

> I guess another dependency, that won't be available on builders, is the
> presence of a system message bus and also the fact you that need to be uid 0
> for the test suite to work.

Same for Debian/Ubuntu.
 
> But that's fine, initially the test suite only works on a running system

*nod*.

Thanks,

Martin

I rewrote the test suite in Python (it's much nicer now), and also keep extending it. For testing I now use an md built from a RAM disk, which doesn't require any modifications to DK-disks, and also supports partitioning.

Thus I retract this report/patch.

Cool. Are you planning on submitting the test suite to the main project?

(In reply to comment #7)
> Cool. Are you planning on submitting the test suite to the main project?

Of course. I just didn't yet, since I just started working on it this week, and the rate of changes is still very high. And as long as I can't commit yet (bug 22578), getting it updated in trunk is a bit cumbersome and wastes lots of your time, too.

Just now I got it complete enough to cover all supported file systems and all Filesystem* D-Bus functions, and I'm now reasonably happy about its structure. So with this being useful already, I'll send a bug with an initial patch then.

I'd appreciate if you could try it on Fedora, though, just to ensure that I didn't do anything distro specific:

wget http://people.canonical.com/~pitti/tmp/dk-disks-test.py
chmod 755 dk-disks-test.py
sudo ./dk-disks-test.py

It will check if /dev/ram8 and /dev/md125 are unused and whether you are root (and bail out if any of those fail), then build /dev/md125 as a raid-0 of /dev/ram8, and then do all tests on those. If you run it from a built tree with ./configure --libexecdir=`pwd`/src, it will use the binaries from the local tree; otherwise it will use the system binaries (which is currently hardcoded as /usr/lib/devicekit-disks/devkit-disk-daemon, sorry; I'll make that more dynamic).

With trunk git head, the minix and swap test cases will fail. If you apply the patches in bug 24673 and 24679, it should succeed:

$ sudo ~/dk-disks-test.py  
mdadm: array /dev/md125 started.
Testing binaries from local build tree
fs: ext2 ... [cli] [dkd] ok
fs: ext3 ... [cli] [dkd] ok
fs: ext4 ... [cli] [dkd] ok
fs: minix ... [cli] [dkd] ok
fs: NTFS ... [cli] [dkd] ok
fs: swap ... [cli] [dkd] ok
fs: FAT ... [cli] [dkd] ok
fs: XFS ... [cli] [dkd] ok
properties of zeroed out md device ... ok

----------------------------------------------------------------------
Ran 9 tests in 32.663s

OK
mdadm: stopped /dev/md125

Just reusing this one..

To determine the system path of the daemon, I think I'll just use

grep Exec= /usr/share/dbus-1/system-services/org.freedesktop.DeviceKit.Disks.service

Updated script on people.c.c. to grab daemon path from d-bus .service file.

(In reply to comment #8)
> And as long as I can't commit yet (bug 22578)

I'll try to physically ping some people.

> Just now I got it complete enough to cover all supported file systems and all
> Filesystem* D-Bus functions, and I'm now reasonably happy about its structure.
> So with this being useful already, I'll send a bug with an initial patch then.
> 
> I'd appreciate if you could try it on Fedora, though, just to ensure that I
> didn't do anything distro specific:

Sounds great, I'll try it out.

Report on the current status:

Coverage:
 - tests local build tree or system binaries, depending from where it is run
 - devkit-disks --show-info
 - create/relabel/mount/unmount/fsck/open files/take ownership for supported file systems (ext[234], minix, xfs, ntfs, vfat, swap), with both command line tools (and DK-d detection) as well as DK-D D-Bus interface
 - LUKS create/teardown/mount/unmount/change password
 - Partition create/flags/modify/delete for MBR and GPT (APM tests are present, but disabled)
 - SMART status/real HD/simulate
 - LVM single LV/single LV with RAID-1
 - Manager functionality: Enumerate*, FindDeviceBy*, Inhibition

In order to succeed, you need a couple of patches sent to bugzilla (which fix some bugs and add some missing bits), and also a bit of luck: the test suite is not very forgiving about race conditions, etc. (which is a feature, since we want to detect them); so right now it might need another attempt if it fails once. (I'll look into these later).

Still on my TODO list for coverage:

 - LinuxMd* D-Bus interface
 - hotplug stresstest (check that we don't drop events/signals)
 - LUKS stresstest (was racy in the past)

Do you have anything else in mind which should be covered?

I'm still interested in how much of this works on a current Fedora/SUSE box. I didn't deliberaly add distro specific stuff, but distros all have different udev rules and package versions, etc.

Also, I played around with receiving D-Bus signals, but it's utterly hard to do. Running the D-Bus and glib main loop in a separate thread doesn't work, and having just one thread is very inconvenient to do with the general flow of a noninteractive test suite. Do you have some hints/pointers how to receive D-Bus signals "in the background"?

Thanks,

Martin

In case someone wonders, one test fails for me as well:

======================================================================
FAIL: Partitions: GUID
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/home/martin/dk-disks-test.py", line 690, in test_gpt
    '0657FD6D-A4AB-43C4-84E5-0933C84B4F4F', 'required')
  File "/home/martin/dk-disks-test.py", line 783, in _do_schema
    self.assertEqual(p1_props.Get(DK_D, 'PartitionFlags'), [flag])
AssertionError: dbus.Array([], signature=dbus.Signature('s'), variant_level=1) != ['required']

For some reason, PartitionFlags property doesn't remember the "required" flag that I set with PartitionModify. It might be a bug in the test suite or a real bug in DK-D, still need to investigate.

> Also, I played around with receiving D-Bus signals, but it's utterly hard to
> do. Running the D-Bus and glib main loop in a separate thread doesn't work

Nevermind. Forgot to call threads_init().

Created attachment 31119 [details] [review]
git formatted patch

With the GUID partitioning fix I sent today, the test suite finally passes all tests now (at least sometimes -- there are still some race conditions in DK-D which return from operations before it is completely done).

It already has been immensely useful for myself to find bugs in both DK-D itself, spot a regression in 009, and find bugs in our distro integration (too old version of cryptsetup, bugs in the lvm2 rules, and so on).

I updated it again to have a proper copyright header, as well as some usage help (as comments).

The test suite is still in heavy flux, but I believe this is a good snapshot which is worth putting into upstream now. 

I'll probably submit a lot of updates for this, though; how do you want to handle those? Can I just occasionally ping you on IRC to run git am on the current stack of patches that I have? I'll also ping daniels on IRC again about adding me to the devicekit group.

Created attachment 31616 [details] [review]
Add --log option to write daemon log into file

Improvement to be able to save daemon log into a file.

Created attachment 31641 [details] [review]
git formatted patch

Rebased the previous two patches to current git head and updated to follow the renaming. Merged into one patch now.

(In reply to comment #18)
> Created an attachment (id=31641) [details]
> git formatted patch
> 
> Rebased the previous two patches to current git head and updated to follow the
> renaming. Merged into one patch now.
> 

Would be nice to move into a tests/ directory and also have some Makefile integration so 'make check' does the right thing - maybe just use sudo in the Makefile to launch the test suite?

For tighter integration it would probably be prudent to add a --helper-dir argument to udisks-daemon, so that we don't have to rely on a particular --libexecdir configure argument.

Since it always needs to rely on the system-installed D-Bus policy, polkit .policy, and udev rule, it can never be "truly" local (and thus run in things like build servers), but the local mode is useful enough to keep indeed.

I'll see to doing that soon, thanks!

(In reply to comment #20)
> For tighter integration it would probably be prudent to add a --helper-dir
> argument to udisks-daemon, so that we don't have to rely on a particular
> --libexecdir configure argument.

Sounds good to me - should just take a colon separated string and prepend it to PATH - that should do the trick I think.

> Since it always needs to rely on the system-installed D-Bus policy, polkit
> .policy, and udev rule, it can never be "truly" local (and thus run in things
> like build servers), but the local mode is useful enough to keep indeed.

Right.

> I'll see to doing that soon, thanks!

Thanks.

Created attachment 31736 [details] [review]
Add --helper-dir option

As discussed, this patch adds a --helper-dir option. It's useful and works on its own, so keeping as a separate patch.

Created attachment 31737 [details] [review]
Add test suite

This adds the test suite as tests/run.

Also pushed to my private branch (http://cgit.freedesktop.org/~martin/udisks/).

automake integration will follow.

Created attachment 31740 [details] [review]
integrate into automake

This runs the tests on "make check" and also adds it to the "make dist" tarball.

The test suite is currently designed to run from the top level directory. It gets a little clumsy to run it from tests/ (or I need to change the test suite to cope with that). Please let me know if you prefer having the tests started from tests/Makefile.am instead.

Created attachment 31741 [details] [review]
fix test suite exit status

This fixes "make check" to properly report failure if one of the tests fails, by making tests/run exit with nonzero on a failure.

All pushed to http://cgit.freedesktop.org/~martin/udisks/, in case you want to slurp these in as one merged commit.

Created attachment 31744 [details] [review]
testsuite: check that FilesystemUnmount() removes the mount point

This failed for me because during the migration I forgot to create /var/lib/udisks/. Can't hurt to ensure that FilesystemUnmount() cleans up properly.

Pushed to master (with the last two test suite fix commits being merged into the main one).