Unit testing questions for FileStore::_detect_fs
diff mbox

Message ID 511243A9.8060804@dachary.org
State New, archived
Headers show

Commit Message

Loic Dachary Feb. 6, 2013, 11:51 a.m. UTC
Hi,

The patch below adds unit tests for FileStore::_detect_fs, but it needs to run
as root in order to mount the ext3, ext4 and btrfs file systems created for test purposes.

Is there a way to mount filesystems without root privileges ?

configure.ac does not detect the presence of commands such as
mkfs.ext4 or mkfs.btrfs. The patch below does not add
AC_CHECK_TOOL([BTRFS], [mkfs.btrfs], ...)  because it has no use
except for unit testing. Instead, the TEST_F(StoreTest, _detect_fs)
function checks if /sbin/mkfs.btrfs is an executable. If it is not,
"SKIP btrfs because /sbin/mkfs.btrfs is not executable" is displayed
and nothing is done.

The drawback is that it is quite difficult to figure out what tools
must be installed to maximize test coverage.

How would you implement unit test tools detection ?

The error returned when ext4 is mounted without user_xattr is the same
as the error returned when ext4 is is mounted with user_xattr but
without the filestore_xattr_use_omap option : -ENOTSUP. From the point
of view of the unit tests, they cannot be distinguished, except by
parsing the messages sent to derr which show

Extended attributes don't appear to work.

or

limited size xattrs -- enable filestore_xattr_use_omap

Is parsing the output a good practice to assert success or failure during unit testing ?

Cheers

Comments

Sage Weil Feb. 6, 2013, 4:47 p.m. UTC | #1
On Wed, 6 Feb 2013, Loic Dachary wrote:
> Hi,
> 
> The patch below adds unit tests for FileStore::_detect_fs, but it needs to run
> as root in order to mount the ext3, ext4 and btrfs file systems created for test purposes.
> 
> Is there a way to mount filesystems without root privileges ?

Not that I know of.

There are several other tests that are meant ot be run separate from the
build environment that use gtest (test_filejournal, the api functional 
tests, etc.).  As long as this is built to a separate binary, I don't 
think it's a problem.  We should converge on unittest_* for the stuff 
'make check' runs and test_* for stuff that is meant to run elsewhere.  
Having our normal qa harness be root is not a problem.

> configure.ac does not detect the presence of commands such as
> mkfs.ext4 or mkfs.btrfs. The patch below does not add
> AC_CHECK_TOOL([BTRFS], [mkfs.btrfs], ...)  because it has no use
> except for unit testing. Instead, the TEST_F(StoreTest, _detect_fs)
> function checks if /sbin/mkfs.btrfs is an executable. If it is not,
> "SKIP btrfs because /sbin/mkfs.btrfs is not executable" is displayed
> and nothing is done.
> 
> The drawback is that it is quite difficult to figure out what tools
> must be installed to maximize test coverage.
> 
> How would you implement unit test tools detection ?
> 
> The error returned when ext4 is mounted without user_xattr is the same
> as the error returned when ext4 is is mounted with user_xattr but
> without the filestore_xattr_use_omap option : -ENOTSUP. From the point
> of view of the unit tests, they cannot be distinguished, except by
> parsing the messages sent to derr which show
> 
> Extended attributes don't appear to work.
> 
> or
> 
> limited size xattrs -- enable filestore_xattr_use_omap
> 
> Is parsing the output a good practice to assert success or failure during unit testing ?

I'm not sure it is worth the complexity.  IMO it would be better to change 
the return value (suggestions?) or not both distinguishing between the two 
cases in the test.

sage

> 
> Cheers
> 
> diff --git a/src/test/filestore/store_test.cc b/src/test/filestore/store_test.cc
> index c98ffb0..7ba38e2 100644
> --- a/src/test/filestore/store_test.cc
> +++ b/src/test/filestore/store_test.cc
> @@ -16,6 +16,7 @@
>  #include <string.h>
>  #include <iostream>
>  #include <time.h>
> +#include <sys/mount.h>
>  #include "os/FileStore.h"
>  #include "include/Context.h"
>  #include "common/ceph_argparse.h"
> @@ -38,6 +39,7 @@ public:
> 
>    StoreTest() : store(0) {}
>    virtual void SetUp() {
> +    ::system("rm -fr store_test_temp_dir store_test_temp_journal");
>      ::mkdir("store_test_temp_dir", 0777);
>      ObjectStore *store_ = new FileStore(string("store_test_temp_dir"), string("store_test_temp_journal"));
>      store.reset(store_);
> @@ -823,6 +825,95 @@ TEST_F(StoreTest, ColSplitTest3) {
>  }
>  #endif
> 
> +TEST_F(StoreTest, _detect_fs) {
> +  if (getuid() != 0) {
> +    cerr << "SKIP because it does not run as root" << std::endl;
> +    return;
> +  }
> +
> +  if (access("/dev/zero", R_OK) != 0) {
> +    cerr << "SKIP because /dev/zero is not a readable file" << std::endl;
> +    return;
> +  }
> +
> +  const string mnt("/tmp/test_filestore");
> +  ::mkdir(mnt.c_str(), 0755);
> +  ::umount(mnt.c_str());
> +  const string disk(mnt + ".disk");
> +  ::unlink(disk.c_str());
> +  ASSERT_EQ(::system(string("dd if=/dev/zero of=" + disk + " bs=1 count=0 seek=1G").c_str()), 0);
> +
> +  const string dir("store_test_temp_dir");
> +  const string journal("store_test_temp_journal");
> +
> +  const string mkfs_ext4("/sbin/mkfs.ext4");
> +  if (::access(mkfs_ext4.c_str(), X_OK) == 0) {
> +
> +    ASSERT_EQ(::system((mkfs_ext4 + " -q -F " + disk).c_str()), 0);
> +    //
> +    // without user_xattr, ext4 fails
> +    //
> +    {
> +      g_ceph_context->_conf->set_val("filestore_xattr_use_omap", "true");
> +      ASSERT_EQ(::system((string("mount -o loop,nouser_xattr ") + disk + " " + mnt).c_str()), 0);
> +      ASSERT_EQ(::chdir(mnt.c_str()), 0);
> +      ASSERT_EQ(::mkdir(dir.c_str(), 0755), 0);
> +      FileStore store(dir, journal);
> +      ASSERT_EQ(store._detect_fs(), -ENOTSUP);
> +      ASSERT_EQ(::chdir("/tmp"), 0);
> +      ASSERT_EQ(::umount(mnt.c_str()), 0);
> +    }
> +    //
> +    // mounted with user_xattr, ext4 fails if filestore_xattr_use_omap is false
> +    //
> +    {
> +      g_ceph_context->_conf->set_val("filestore_xattr_use_omap", "false");
> +      ASSERT_EQ(::system((string("mount -o loop,user_xattr ") + disk + " " + mnt).c_str()), 0);
> +      ASSERT_EQ(::chdir(mnt.c_str()), 0);
> +      FileStore store(dir, journal);
> +      ASSERT_EQ(store._detect_fs(), -ENOTSUP);
> +      ASSERT_EQ(::chdir("/tmp"), 0);
> +      ASSERT_EQ(::umount(mnt.c_str()), 0);
> +    }
> +    //
> +    // mounted with user_xattr, ext4 succeeds if filestore_xattr_use_omap is true
> +    //
> +    {
> +      g_ceph_context->_conf->set_val("filestore_xattr_use_omap", "true");
> +      ASSERT_EQ(::system((string("mount -o loop,user_xattr ") + disk + " " + mnt).c_str()), 0);
> +      ASSERT_EQ(::chdir(mnt.c_str()), 0);
> +      FileStore store(dir, journal);
> +      ASSERT_EQ(store._detect_fs(), 0);
> +      ASSERT_EQ(::chdir("/tmp"), 0);
> +      ASSERT_EQ(::umount(mnt.c_str()), 0);
> +    }
> +  } else {
> +    cerr << "SKIP ext4 because " << mkfs_ext4 << " is not executable" << std::endl;
> +  }
> +
> +  g_ceph_context->_conf->set_val("filestore_xattr_use_omap", "false");
> +  ASSERT_EQ(::unlink(disk.c_str()), 0);
> +  ASSERT_EQ(::system(string("dd if=/dev/zero of=" + disk + " bs=1 count=0 seek=1G").c_str()), 0);
> +  const string mkfs_btrfs("/sbin/mkfs.btrfs");
> +  if (::access(mkfs_btrfs.c_str(), X_OK) == 0) {
> +    ASSERT_EQ(::system((mkfs_btrfs + " " + disk).c_str()), 0);
> +    //
> +    // btrfs succeeds
> +    //
> +    {
> +      ASSERT_EQ(::system((string("mount -o loop ") + disk + " " + mnt).c_str()), 0);
> +      ASSERT_EQ(::chdir(mnt.c_str()), 0);
> +      ASSERT_EQ(::mkdir(dir.c_str(), 0755), 0);
> +      FileStore store(dir, journal);
> +      ASSERT_EQ(store._detect_fs(), 0);
> +      ASSERT_EQ(::chdir("/tmp"), 0);
> +      ASSERT_EQ(::umount(mnt.c_str()), 0);
> +    }
> +  } else {
> +    cerr << "SKIP btrfs because " << mkfs_btrfs << " is not executable" << std::endl;
> +  }
> +}
> +
> 
> -- 
> Lo?c Dachary, Artisan Logiciel Libre
> 
> 
--
To unsubscribe from this list: send the line "unsubscribe ceph-devel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Loic Dachary Feb. 10, 2013, 10:36 a.m. UTC | #2
Hi,

On 02/06/2013 05:47 PM, Sage Weil wrote:
> On Wed, 6 Feb 2013, Loic Dachary wrote:
>> Hi,
>>
>> The patch below adds unit tests for FileStore::_detect_fs, but it needs to run
>> as root in order to mount the ext3, ext4 and btrfs file systems created for test purposes.
>>
>> Is there a way to mount filesystems without root privileges ?
> 
> Not that I know of.
> 
> There are several other tests that are meant ot be run separate from the
> build environment that use gtest (test_filejournal, the api functional 
> tests, etc.).  As long as this is built to a separate binary, I don't 
> think it's a problem.  We should converge on unittest_* for the stuff 
> 'make check' runs and test_* for stuff that is meant to run elsewhere.  
> Having our normal qa harness be root is not a problem.

If I understand correctly, these test_* binaries can then be included in scripts found in ceph/qa/workunits and used by teuthology. The simplest example being https://github.com/ceph/ceph/blob/master/qa/workunits/rbd/test_librbd.sh

Cheers
 
>> configure.ac does not detect the presence of commands such as
>> mkfs.ext4 or mkfs.btrfs. The patch below does not add
>> AC_CHECK_TOOL([BTRFS], [mkfs.btrfs], ...)  because it has no use
>> except for unit testing. Instead, the TEST_F(StoreTest, _detect_fs)
>> function checks if /sbin/mkfs.btrfs is an executable. If it is not,
>> "SKIP btrfs because /sbin/mkfs.btrfs is not executable" is displayed
>> and nothing is done.
>>
>> The drawback is that it is quite difficult to figure out what tools
>> must be installed to maximize test coverage.
>>
>> How would you implement unit test tools detection ?
>>
>> The error returned when ext4 is mounted without user_xattr is the same
>> as the error returned when ext4 is is mounted with user_xattr but
>> without the filestore_xattr_use_omap option : -ENOTSUP. From the point
>> of view of the unit tests, they cannot be distinguished, except by
>> parsing the messages sent to derr which show
>>
>> Extended attributes don't appear to work.
>>
>> or
>>
>> limited size xattrs -- enable filestore_xattr_use_omap
>>
>> Is parsing the output a good practice to assert success or failure during unit testing ?
> 
> I'm not sure it is worth the complexity.  IMO it would be better to change 
> the return value (suggestions?) or not both distinguishing between the two 
> cases in the test.
> 
> sage
> 
>>
>> Cheers
>>
>> diff --git a/src/test/filestore/store_test.cc b/src/test/filestore/store_test.cc
>> index c98ffb0..7ba38e2 100644
>> --- a/src/test/filestore/store_test.cc
>> +++ b/src/test/filestore/store_test.cc
>> @@ -16,6 +16,7 @@
>>  #include <string.h>
>>  #include <iostream>
>>  #include <time.h>
>> +#include <sys/mount.h>
>>  #include "os/FileStore.h"
>>  #include "include/Context.h"
>>  #include "common/ceph_argparse.h"
>> @@ -38,6 +39,7 @@ public:
>>
>>    StoreTest() : store(0) {}
>>    virtual void SetUp() {
>> +    ::system("rm -fr store_test_temp_dir store_test_temp_journal");
>>      ::mkdir("store_test_temp_dir", 0777);
>>      ObjectStore *store_ = new FileStore(string("store_test_temp_dir"), string("store_test_temp_journal"));
>>      store.reset(store_);
>> @@ -823,6 +825,95 @@ TEST_F(StoreTest, ColSplitTest3) {
>>  }
>>  #endif
>>
>> +TEST_F(StoreTest, _detect_fs) {
>> +  if (getuid() != 0) {
>> +    cerr << "SKIP because it does not run as root" << std::endl;
>> +    return;
>> +  }
>> +
>> +  if (access("/dev/zero", R_OK) != 0) {
>> +    cerr << "SKIP because /dev/zero is not a readable file" << std::endl;
>> +    return;
>> +  }
>> +
>> +  const string mnt("/tmp/test_filestore");
>> +  ::mkdir(mnt.c_str(), 0755);
>> +  ::umount(mnt.c_str());
>> +  const string disk(mnt + ".disk");
>> +  ::unlink(disk.c_str());
>> +  ASSERT_EQ(::system(string("dd if=/dev/zero of=" + disk + " bs=1 count=0 seek=1G").c_str()), 0);
>> +
>> +  const string dir("store_test_temp_dir");
>> +  const string journal("store_test_temp_journal");
>> +
>> +  const string mkfs_ext4("/sbin/mkfs.ext4");
>> +  if (::access(mkfs_ext4.c_str(), X_OK) == 0) {
>> +
>> +    ASSERT_EQ(::system((mkfs_ext4 + " -q -F " + disk).c_str()), 0);
>> +    //
>> +    // without user_xattr, ext4 fails
>> +    //
>> +    {
>> +      g_ceph_context->_conf->set_val("filestore_xattr_use_omap", "true");
>> +      ASSERT_EQ(::system((string("mount -o loop,nouser_xattr ") + disk + " " + mnt).c_str()), 0);
>> +      ASSERT_EQ(::chdir(mnt.c_str()), 0);
>> +      ASSERT_EQ(::mkdir(dir.c_str(), 0755), 0);
>> +      FileStore store(dir, journal);
>> +      ASSERT_EQ(store._detect_fs(), -ENOTSUP);
>> +      ASSERT_EQ(::chdir("/tmp"), 0);
>> +      ASSERT_EQ(::umount(mnt.c_str()), 0);
>> +    }
>> +    //
>> +    // mounted with user_xattr, ext4 fails if filestore_xattr_use_omap is false
>> +    //
>> +    {
>> +      g_ceph_context->_conf->set_val("filestore_xattr_use_omap", "false");
>> +      ASSERT_EQ(::system((string("mount -o loop,user_xattr ") + disk + " " + mnt).c_str()), 0);
>> +      ASSERT_EQ(::chdir(mnt.c_str()), 0);
>> +      FileStore store(dir, journal);
>> +      ASSERT_EQ(store._detect_fs(), -ENOTSUP);
>> +      ASSERT_EQ(::chdir("/tmp"), 0);
>> +      ASSERT_EQ(::umount(mnt.c_str()), 0);
>> +    }
>> +    //
>> +    // mounted with user_xattr, ext4 succeeds if filestore_xattr_use_omap is true
>> +    //
>> +    {
>> +      g_ceph_context->_conf->set_val("filestore_xattr_use_omap", "true");
>> +      ASSERT_EQ(::system((string("mount -o loop,user_xattr ") + disk + " " + mnt).c_str()), 0);
>> +      ASSERT_EQ(::chdir(mnt.c_str()), 0);
>> +      FileStore store(dir, journal);
>> +      ASSERT_EQ(store._detect_fs(), 0);
>> +      ASSERT_EQ(::chdir("/tmp"), 0);
>> +      ASSERT_EQ(::umount(mnt.c_str()), 0);
>> +    }
>> +  } else {
>> +    cerr << "SKIP ext4 because " << mkfs_ext4 << " is not executable" << std::endl;
>> +  }
>> +
>> +  g_ceph_context->_conf->set_val("filestore_xattr_use_omap", "false");
>> +  ASSERT_EQ(::unlink(disk.c_str()), 0);
>> +  ASSERT_EQ(::system(string("dd if=/dev/zero of=" + disk + " bs=1 count=0 seek=1G").c_str()), 0);
>> +  const string mkfs_btrfs("/sbin/mkfs.btrfs");
>> +  if (::access(mkfs_btrfs.c_str(), X_OK) == 0) {
>> +    ASSERT_EQ(::system((mkfs_btrfs + " " + disk).c_str()), 0);
>> +    //
>> +    // btrfs succeeds
>> +    //
>> +    {
>> +      ASSERT_EQ(::system((string("mount -o loop ") + disk + " " + mnt).c_str()), 0);
>> +      ASSERT_EQ(::chdir(mnt.c_str()), 0);
>> +      ASSERT_EQ(::mkdir(dir.c_str(), 0755), 0);
>> +      FileStore store(dir, journal);
>> +      ASSERT_EQ(store._detect_fs(), 0);
>> +      ASSERT_EQ(::chdir("/tmp"), 0);
>> +      ASSERT_EQ(::umount(mnt.c_str()), 0);
>> +    }
>> +  } else {
>> +    cerr << "SKIP btrfs because " << mkfs_btrfs << " is not executable" << std::endl;
>> +  }
>> +}
>> +
>>
>> -- 
>> Lo?c Dachary, Artisan Logiciel Libre
>>
>>
Sam Lang Feb. 16, 2013, 4:33 p.m. UTC | #3
On Sun, Feb 10, 2013 at 4:36 AM, Loic Dachary <loic@dachary.org> wrote:
> Hi,
>
> On 02/06/2013 05:47 PM, Sage Weil wrote:
>> On Wed, 6 Feb 2013, Loic Dachary wrote:
>>> Hi,
>>>
>>> The patch below adds unit tests for FileStore::_detect_fs, but it needs to run
>>> as root in order to mount the ext3, ext4 and btrfs file systems created for test purposes.
>>>
>>> Is there a way to mount filesystems without root privileges ?
>>
>> Not that I know of.
>>
>> There are several other tests that are meant ot be run separate from the
>> build environment that use gtest (test_filejournal, the api functional
>> tests, etc.).  As long as this is built to a separate binary, I don't
>> think it's a problem.  We should converge on unittest_* for the stuff
>> 'make check' runs and test_* for stuff that is meant to run elsewhere.
>> Having our normal qa harness be root is not a problem.
>
> If I understand correctly, these test_* binaries can then be included in scripts found in ceph/qa/workunits and used by teuthology. The simplest example being https://github.com/ceph/ceph/blob/master/qa/workunits/rbd/test_librbd.sh

Yep that's right.  Note that the test_* binaries were recently renamed
to ceph_test_*
-sam

>
> Cheers
>
>>> configure.ac does not detect the presence of commands such as
>>> mkfs.ext4 or mkfs.btrfs. The patch below does not add
>>> AC_CHECK_TOOL([BTRFS], [mkfs.btrfs], ...)  because it has no use
>>> except for unit testing. Instead, the TEST_F(StoreTest, _detect_fs)
>>> function checks if /sbin/mkfs.btrfs is an executable. If it is not,
>>> "SKIP btrfs because /sbin/mkfs.btrfs is not executable" is displayed
>>> and nothing is done.
>>>
>>> The drawback is that it is quite difficult to figure out what tools
>>> must be installed to maximize test coverage.
>>>
>>> How would you implement unit test tools detection ?
>>>
>>> The error returned when ext4 is mounted without user_xattr is the same
>>> as the error returned when ext4 is is mounted with user_xattr but
>>> without the filestore_xattr_use_omap option : -ENOTSUP. From the point
>>> of view of the unit tests, they cannot be distinguished, except by
>>> parsing the messages sent to derr which show
>>>
>>> Extended attributes don't appear to work.
>>>
>>> or
>>>
>>> limited size xattrs -- enable filestore_xattr_use_omap
>>>
>>> Is parsing the output a good practice to assert success or failure during unit testing ?
>>
>> I'm not sure it is worth the complexity.  IMO it would be better to change
>> the return value (suggestions?) or not both distinguishing between the two
>> cases in the test.
>>
>> sage
>>
>>>
>>> Cheers
>>>
>>> diff --git a/src/test/filestore/store_test.cc b/src/test/filestore/store_test.cc
>>> index c98ffb0..7ba38e2 100644
>>> --- a/src/test/filestore/store_test.cc
>>> +++ b/src/test/filestore/store_test.cc
>>> @@ -16,6 +16,7 @@
>>>  #include <string.h>
>>>  #include <iostream>
>>>  #include <time.h>
>>> +#include <sys/mount.h>
>>>  #include "os/FileStore.h"
>>>  #include "include/Context.h"
>>>  #include "common/ceph_argparse.h"
>>> @@ -38,6 +39,7 @@ public:
>>>
>>>    StoreTest() : store(0) {}
>>>    virtual void SetUp() {
>>> +    ::system("rm -fr store_test_temp_dir store_test_temp_journal");
>>>      ::mkdir("store_test_temp_dir", 0777);
>>>      ObjectStore *store_ = new FileStore(string("store_test_temp_dir"), string("store_test_temp_journal"));
>>>      store.reset(store_);
>>> @@ -823,6 +825,95 @@ TEST_F(StoreTest, ColSplitTest3) {
>>>  }
>>>  #endif
>>>
>>> +TEST_F(StoreTest, _detect_fs) {
>>> +  if (getuid() != 0) {
>>> +    cerr << "SKIP because it does not run as root" << std::endl;
>>> +    return;
>>> +  }
>>> +
>>> +  if (access("/dev/zero", R_OK) != 0) {
>>> +    cerr << "SKIP because /dev/zero is not a readable file" << std::endl;
>>> +    return;
>>> +  }
>>> +
>>> +  const string mnt("/tmp/test_filestore");
>>> +  ::mkdir(mnt.c_str(), 0755);
>>> +  ::umount(mnt.c_str());
>>> +  const string disk(mnt + ".disk");
>>> +  ::unlink(disk.c_str());
>>> +  ASSERT_EQ(::system(string("dd if=/dev/zero of=" + disk + " bs=1 count=0 seek=1G").c_str()), 0);
>>> +
>>> +  const string dir("store_test_temp_dir");
>>> +  const string journal("store_test_temp_journal");
>>> +
>>> +  const string mkfs_ext4("/sbin/mkfs.ext4");
>>> +  if (::access(mkfs_ext4.c_str(), X_OK) == 0) {
>>> +
>>> +    ASSERT_EQ(::system((mkfs_ext4 + " -q -F " + disk).c_str()), 0);
>>> +    //
>>> +    // without user_xattr, ext4 fails
>>> +    //
>>> +    {
>>> +      g_ceph_context->_conf->set_val("filestore_xattr_use_omap", "true");
>>> +      ASSERT_EQ(::system((string("mount -o loop,nouser_xattr ") + disk + " " + mnt).c_str()), 0);
>>> +      ASSERT_EQ(::chdir(mnt.c_str()), 0);
>>> +      ASSERT_EQ(::mkdir(dir.c_str(), 0755), 0);
>>> +      FileStore store(dir, journal);
>>> +      ASSERT_EQ(store._detect_fs(), -ENOTSUP);
>>> +      ASSERT_EQ(::chdir("/tmp"), 0);
>>> +      ASSERT_EQ(::umount(mnt.c_str()), 0);
>>> +    }
>>> +    //
>>> +    // mounted with user_xattr, ext4 fails if filestore_xattr_use_omap is false
>>> +    //
>>> +    {
>>> +      g_ceph_context->_conf->set_val("filestore_xattr_use_omap", "false");
>>> +      ASSERT_EQ(::system((string("mount -o loop,user_xattr ") + disk + " " + mnt).c_str()), 0);
>>> +      ASSERT_EQ(::chdir(mnt.c_str()), 0);
>>> +      FileStore store(dir, journal);
>>> +      ASSERT_EQ(store._detect_fs(), -ENOTSUP);
>>> +      ASSERT_EQ(::chdir("/tmp"), 0);
>>> +      ASSERT_EQ(::umount(mnt.c_str()), 0);
>>> +    }
>>> +    //
>>> +    // mounted with user_xattr, ext4 succeeds if filestore_xattr_use_omap is true
>>> +    //
>>> +    {
>>> +      g_ceph_context->_conf->set_val("filestore_xattr_use_omap", "true");
>>> +      ASSERT_EQ(::system((string("mount -o loop,user_xattr ") + disk + " " + mnt).c_str()), 0);
>>> +      ASSERT_EQ(::chdir(mnt.c_str()), 0);
>>> +      FileStore store(dir, journal);
>>> +      ASSERT_EQ(store._detect_fs(), 0);
>>> +      ASSERT_EQ(::chdir("/tmp"), 0);
>>> +      ASSERT_EQ(::umount(mnt.c_str()), 0);
>>> +    }
>>> +  } else {
>>> +    cerr << "SKIP ext4 because " << mkfs_ext4 << " is not executable" << std::endl;
>>> +  }
>>> +
>>> +  g_ceph_context->_conf->set_val("filestore_xattr_use_omap", "false");
>>> +  ASSERT_EQ(::unlink(disk.c_str()), 0);
>>> +  ASSERT_EQ(::system(string("dd if=/dev/zero of=" + disk + " bs=1 count=0 seek=1G").c_str()), 0);
>>> +  const string mkfs_btrfs("/sbin/mkfs.btrfs");
>>> +  if (::access(mkfs_btrfs.c_str(), X_OK) == 0) {
>>> +    ASSERT_EQ(::system((mkfs_btrfs + " " + disk).c_str()), 0);
>>> +    //
>>> +    // btrfs succeeds
>>> +    //
>>> +    {
>>> +      ASSERT_EQ(::system((string("mount -o loop ") + disk + " " + mnt).c_str()), 0);
>>> +      ASSERT_EQ(::chdir(mnt.c_str()), 0);
>>> +      ASSERT_EQ(::mkdir(dir.c_str(), 0755), 0);
>>> +      FileStore store(dir, journal);
>>> +      ASSERT_EQ(store._detect_fs(), 0);
>>> +      ASSERT_EQ(::chdir("/tmp"), 0);
>>> +      ASSERT_EQ(::umount(mnt.c_str()), 0);
>>> +    }
>>> +  } else {
>>> +    cerr << "SKIP btrfs because " << mkfs_btrfs << " is not executable" << std::endl;
>>> +  }
>>> +}
>>> +
>>>
>>> --
>>> Lo?c Dachary, Artisan Logiciel Libre
>>>
>>>
>
> --
> Loïc Dachary, Artisan Logiciel Libre
>
--
To unsubscribe from this list: send the line "unsubscribe ceph-devel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Loic Dachary Feb. 16, 2013, 6:54 p.m. UTC | #4
On 02/16/2013 05:33 PM, Sam Lang wrote:
> On Sun, Feb 10, 2013 at 4:36 AM, Loic Dachary <loic@dachary.org> wrote:
>> Hi,
>>
>> On 02/06/2013 05:47 PM, Sage Weil wrote:
>>> On Wed, 6 Feb 2013, Loic Dachary wrote:
>>>> Hi,
>>>>
>>>> The patch below adds unit tests for FileStore::_detect_fs, but it needs to run
>>>> as root in order to mount the ext3, ext4 and btrfs file systems created for test purposes.
>>>>
>>>> Is there a way to mount filesystems without root privileges ?
>>>
>>> Not that I know of.
>>>
>>> There are several other tests that are meant ot be run separate from the
>>> build environment that use gtest (test_filejournal, the api functional
>>> tests, etc.).  As long as this is built to a separate binary, I don't
>>> think it's a problem.  We should converge on unittest_* for the stuff
>>> 'make check' runs and test_* for stuff that is meant to run elsewhere.
>>> Having our normal qa harness be root is not a problem.
>>
>> If I understand correctly, these test_* binaries can then be included in scripts found in ceph/qa/workunits and used by teuthology. The simplest example being https://github.com/ceph/ceph/blob/master/qa/workunits/rbd/test_librbd.sh
> 
> Yep that's right.  Note that the test_* binaries were recently renamed
> to ceph_test_*

Thanks for the hint. Now that I'm done with unit testing buffer.{h,cc} ( https://github.com/dachary/ceph/commit/9e22d60eda488195337ed533448c679bf8d43639 ) I was about to work on improving ceph_test_filestore and run it on various file systems with teuthology. Let me know if my time would be better used working on something else ;-)

Cheers

> -sam
> 
>>
>> Cheers
>>
>>>> configure.ac does not detect the presence of commands such as
>>>> mkfs.ext4 or mkfs.btrfs. The patch below does not add
>>>> AC_CHECK_TOOL([BTRFS], [mkfs.btrfs], ...)  because it has no use
>>>> except for unit testing. Instead, the TEST_F(StoreTest, _detect_fs)
>>>> function checks if /sbin/mkfs.btrfs is an executable. If it is not,
>>>> "SKIP btrfs because /sbin/mkfs.btrfs is not executable" is displayed
>>>> and nothing is done.
>>>>
>>>> The drawback is that it is quite difficult to figure out what tools
>>>> must be installed to maximize test coverage.
>>>>
>>>> How would you implement unit test tools detection ?
>>>>
>>>> The error returned when ext4 is mounted without user_xattr is the same
>>>> as the error returned when ext4 is is mounted with user_xattr but
>>>> without the filestore_xattr_use_omap option : -ENOTSUP. From the point
>>>> of view of the unit tests, they cannot be distinguished, except by
>>>> parsing the messages sent to derr which show
>>>>
>>>> Extended attributes don't appear to work.
>>>>
>>>> or
>>>>
>>>> limited size xattrs -- enable filestore_xattr_use_omap
>>>>
>>>> Is parsing the output a good practice to assert success or failure during unit testing ?
>>>
>>> I'm not sure it is worth the complexity.  IMO it would be better to change
>>> the return value (suggestions?) or not both distinguishing between the two
>>> cases in the test.
>>>
>>> sage
>>>
>>>>
>>>> Cheers
>>>>
>>>> diff --git a/src/test/filestore/store_test.cc b/src/test/filestore/store_test.cc
>>>> index c98ffb0..7ba38e2 100644
>>>> --- a/src/test/filestore/store_test.cc
>>>> +++ b/src/test/filestore/store_test.cc
>>>> @@ -16,6 +16,7 @@
>>>>  #include <string.h>
>>>>  #include <iostream>
>>>>  #include <time.h>
>>>> +#include <sys/mount.h>
>>>>  #include "os/FileStore.h"
>>>>  #include "include/Context.h"
>>>>  #include "common/ceph_argparse.h"
>>>> @@ -38,6 +39,7 @@ public:
>>>>
>>>>    StoreTest() : store(0) {}
>>>>    virtual void SetUp() {
>>>> +    ::system("rm -fr store_test_temp_dir store_test_temp_journal");
>>>>      ::mkdir("store_test_temp_dir", 0777);
>>>>      ObjectStore *store_ = new FileStore(string("store_test_temp_dir"), string("store_test_temp_journal"));
>>>>      store.reset(store_);
>>>> @@ -823,6 +825,95 @@ TEST_F(StoreTest, ColSplitTest3) {
>>>>  }
>>>>  #endif
>>>>
>>>> +TEST_F(StoreTest, _detect_fs) {
>>>> +  if (getuid() != 0) {
>>>> +    cerr << "SKIP because it does not run as root" << std::endl;
>>>> +    return;
>>>> +  }
>>>> +
>>>> +  if (access("/dev/zero", R_OK) != 0) {
>>>> +    cerr << "SKIP because /dev/zero is not a readable file" << std::endl;
>>>> +    return;
>>>> +  }
>>>> +
>>>> +  const string mnt("/tmp/test_filestore");
>>>> +  ::mkdir(mnt.c_str(), 0755);
>>>> +  ::umount(mnt.c_str());
>>>> +  const string disk(mnt + ".disk");
>>>> +  ::unlink(disk.c_str());
>>>> +  ASSERT_EQ(::system(string("dd if=/dev/zero of=" + disk + " bs=1 count=0 seek=1G").c_str()), 0);
>>>> +
>>>> +  const string dir("store_test_temp_dir");
>>>> +  const string journal("store_test_temp_journal");
>>>> +
>>>> +  const string mkfs_ext4("/sbin/mkfs.ext4");
>>>> +  if (::access(mkfs_ext4.c_str(), X_OK) == 0) {
>>>> +
>>>> +    ASSERT_EQ(::system((mkfs_ext4 + " -q -F " + disk).c_str()), 0);
>>>> +    //
>>>> +    // without user_xattr, ext4 fails
>>>> +    //
>>>> +    {
>>>> +      g_ceph_context->_conf->set_val("filestore_xattr_use_omap", "true");
>>>> +      ASSERT_EQ(::system((string("mount -o loop,nouser_xattr ") + disk + " " + mnt).c_str()), 0);
>>>> +      ASSERT_EQ(::chdir(mnt.c_str()), 0);
>>>> +      ASSERT_EQ(::mkdir(dir.c_str(), 0755), 0);
>>>> +      FileStore store(dir, journal);
>>>> +      ASSERT_EQ(store._detect_fs(), -ENOTSUP);
>>>> +      ASSERT_EQ(::chdir("/tmp"), 0);
>>>> +      ASSERT_EQ(::umount(mnt.c_str()), 0);
>>>> +    }
>>>> +    //
>>>> +    // mounted with user_xattr, ext4 fails if filestore_xattr_use_omap is false
>>>> +    //
>>>> +    {
>>>> +      g_ceph_context->_conf->set_val("filestore_xattr_use_omap", "false");
>>>> +      ASSERT_EQ(::system((string("mount -o loop,user_xattr ") + disk + " " + mnt).c_str()), 0);
>>>> +      ASSERT_EQ(::chdir(mnt.c_str()), 0);
>>>> +      FileStore store(dir, journal);
>>>> +      ASSERT_EQ(store._detect_fs(), -ENOTSUP);
>>>> +      ASSERT_EQ(::chdir("/tmp"), 0);
>>>> +      ASSERT_EQ(::umount(mnt.c_str()), 0);
>>>> +    }
>>>> +    //
>>>> +    // mounted with user_xattr, ext4 succeeds if filestore_xattr_use_omap is true
>>>> +    //
>>>> +    {
>>>> +      g_ceph_context->_conf->set_val("filestore_xattr_use_omap", "true");
>>>> +      ASSERT_EQ(::system((string("mount -o loop,user_xattr ") + disk + " " + mnt).c_str()), 0);
>>>> +      ASSERT_EQ(::chdir(mnt.c_str()), 0);
>>>> +      FileStore store(dir, journal);
>>>> +      ASSERT_EQ(store._detect_fs(), 0);
>>>> +      ASSERT_EQ(::chdir("/tmp"), 0);
>>>> +      ASSERT_EQ(::umount(mnt.c_str()), 0);
>>>> +    }
>>>> +  } else {
>>>> +    cerr << "SKIP ext4 because " << mkfs_ext4 << " is not executable" << std::endl;
>>>> +  }
>>>> +
>>>> +  g_ceph_context->_conf->set_val("filestore_xattr_use_omap", "false");
>>>> +  ASSERT_EQ(::unlink(disk.c_str()), 0);
>>>> +  ASSERT_EQ(::system(string("dd if=/dev/zero of=" + disk + " bs=1 count=0 seek=1G").c_str()), 0);
>>>> +  const string mkfs_btrfs("/sbin/mkfs.btrfs");
>>>> +  if (::access(mkfs_btrfs.c_str(), X_OK) == 0) {
>>>> +    ASSERT_EQ(::system((mkfs_btrfs + " " + disk).c_str()), 0);
>>>> +    //
>>>> +    // btrfs succeeds
>>>> +    //
>>>> +    {
>>>> +      ASSERT_EQ(::system((string("mount -o loop ") + disk + " " + mnt).c_str()), 0);
>>>> +      ASSERT_EQ(::chdir(mnt.c_str()), 0);
>>>> +      ASSERT_EQ(::mkdir(dir.c_str(), 0755), 0);
>>>> +      FileStore store(dir, journal);
>>>> +      ASSERT_EQ(store._detect_fs(), 0);
>>>> +      ASSERT_EQ(::chdir("/tmp"), 0);
>>>> +      ASSERT_EQ(::umount(mnt.c_str()), 0);
>>>> +    }
>>>> +  } else {
>>>> +    cerr << "SKIP btrfs because " << mkfs_btrfs << " is not executable" << std::endl;
>>>> +  }
>>>> +}
>>>> +
>>>>
>>>> --
>>>> Lo?c Dachary, Artisan Logiciel Libre
>>>>
>>>>
>>
>> --
>> Loïc Dachary, Artisan Logiciel Libre
>>
> --
> To unsubscribe from this list: send the line "unsubscribe ceph-devel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Patch
diff mbox

diff --git a/src/test/filestore/store_test.cc b/src/test/filestore/store_test.cc
index c98ffb0..7ba38e2 100644
--- a/src/test/filestore/store_test.cc
+++ b/src/test/filestore/store_test.cc
@@ -16,6 +16,7 @@ 
 #include <string.h>
 #include <iostream>
 #include <time.h>
+#include <sys/mount.h>
 #include "os/FileStore.h"
 #include "include/Context.h"
 #include "common/ceph_argparse.h"
@@ -38,6 +39,7 @@  public:

   StoreTest() : store(0) {}
   virtual void SetUp() {
+    ::system("rm -fr store_test_temp_dir store_test_temp_journal");
     ::mkdir("store_test_temp_dir", 0777);
     ObjectStore *store_ = new FileStore(string("store_test_temp_dir"), string("store_test_temp_journal"));
     store.reset(store_);
@@ -823,6 +825,95 @@  TEST_F(StoreTest, ColSplitTest3) {
 }
 #endif

+TEST_F(StoreTest, _detect_fs) {
+  if (getuid() != 0) {
+    cerr << "SKIP because it does not run as root" << std::endl;
+    return;
+  }
+
+  if (access("/dev/zero", R_OK) != 0) {
+    cerr << "SKIP because /dev/zero is not a readable file" << std::endl;
+    return;
+  }
+
+  const string mnt("/tmp/test_filestore");
+  ::mkdir(mnt.c_str(), 0755);
+  ::umount(mnt.c_str());
+  const string disk(mnt + ".disk");
+  ::unlink(disk.c_str());
+  ASSERT_EQ(::system(string("dd if=/dev/zero of=" + disk + " bs=1 count=0 seek=1G").c_str()), 0);
+
+  const string dir("store_test_temp_dir");
+  const string journal("store_test_temp_journal");
+
+  const string mkfs_ext4("/sbin/mkfs.ext4");
+  if (::access(mkfs_ext4.c_str(), X_OK) == 0) {
+
+    ASSERT_EQ(::system((mkfs_ext4 + " -q -F " + disk).c_str()), 0);
+    //
+    // without user_xattr, ext4 fails
+    //
+    {
+      g_ceph_context->_conf->set_val("filestore_xattr_use_omap", "true");
+      ASSERT_EQ(::system((string("mount -o loop,nouser_xattr ") + disk + " " + mnt).c_str()), 0);
+      ASSERT_EQ(::chdir(mnt.c_str()), 0);
+      ASSERT_EQ(::mkdir(dir.c_str(), 0755), 0);
+      FileStore store(dir, journal);
+      ASSERT_EQ(store._detect_fs(), -ENOTSUP);
+      ASSERT_EQ(::chdir("/tmp"), 0);
+      ASSERT_EQ(::umount(mnt.c_str()), 0);
+    }
+    //
+    // mounted with user_xattr, ext4 fails if filestore_xattr_use_omap is false
+    //
+    {
+      g_ceph_context->_conf->set_val("filestore_xattr_use_omap", "false");
+      ASSERT_EQ(::system((string("mount -o loop,user_xattr ") + disk + " " + mnt).c_str()), 0);
+      ASSERT_EQ(::chdir(mnt.c_str()), 0);
+      FileStore store(dir, journal);
+      ASSERT_EQ(store._detect_fs(), -ENOTSUP);
+      ASSERT_EQ(::chdir("/tmp"), 0);
+      ASSERT_EQ(::umount(mnt.c_str()), 0);
+    }
+    //
+    // mounted with user_xattr, ext4 succeeds if filestore_xattr_use_omap is true
+    //
+    {
+      g_ceph_context->_conf->set_val("filestore_xattr_use_omap", "true");
+      ASSERT_EQ(::system((string("mount -o loop,user_xattr ") + disk + " " + mnt).c_str()), 0);
+      ASSERT_EQ(::chdir(mnt.c_str()), 0);
+      FileStore store(dir, journal);
+      ASSERT_EQ(store._detect_fs(), 0);
+      ASSERT_EQ(::chdir("/tmp"), 0);
+      ASSERT_EQ(::umount(mnt.c_str()), 0);
+    }
+  } else {
+    cerr << "SKIP ext4 because " << mkfs_ext4 << " is not executable" << std::endl;
+  }
+
+  g_ceph_context->_conf->set_val("filestore_xattr_use_omap", "false");
+  ASSERT_EQ(::unlink(disk.c_str()), 0);
+  ASSERT_EQ(::system(string("dd if=/dev/zero of=" + disk + " bs=1 count=0 seek=1G").c_str()), 0);
+  const string mkfs_btrfs("/sbin/mkfs.btrfs");
+  if (::access(mkfs_btrfs.c_str(), X_OK) == 0) {
+    ASSERT_EQ(::system((mkfs_btrfs + " " + disk).c_str()), 0);
+    //
+    // btrfs succeeds
+    //
+    {
+      ASSERT_EQ(::system((string("mount -o loop ") + disk + " " + mnt).c_str()), 0);
+      ASSERT_EQ(::chdir(mnt.c_str()), 0);
+      ASSERT_EQ(::mkdir(dir.c_str(), 0755), 0);
+      FileStore store(dir, journal);
+      ASSERT_EQ(store._detect_fs(), 0);
+      ASSERT_EQ(::chdir("/tmp"), 0);
+      ASSERT_EQ(::umount(mnt.c_str()), 0);
+    }
+  } else {
+    cerr << "SKIP btrfs because " << mkfs_btrfs << " is not executable" << std::endl;
+  }
+}
+

-- 
Loïc Dachary, Artisan Logiciel Libre