[blivet:master 0/2] Remove test which needs updating on btrfs-progs version change. (v3)
Rebased against most recent version of master and seems OK.
Originally my plan was just to get rid of the test that was failing on Jenkins, since it did not test anything we use. But on further consideration, it appeared that that was a mistake, i.e., we should be using that information. So that's what this patch set does instead.
mulhern (2): Further abstract loopbackedtestcase on block_size. Check the minimum member size for BtrfsVolumeDevices.
blivet/devicelibs/btrfs.py | 4 ++++ blivet/devices.py | 10 +++++++++ tests/devicelibs_test/btrfs_test.py | 21 +++++++++++++++++- tests/devicelibs_test/mdraid_interrogate_test.py | 5 +++-- tests/devicelibs_test/mdraid_test.py | 6 +++--- tests/devices_test.py | 26 +++++++++++++++++------ tests/formats_test/fslabeling.py | 3 ++- tests/formats_test/selinux_test.py | 3 ++- tests/loopbackedtestcase.py | 27 +++++++++++++++--------- 9 files changed, 80 insertions(+), 25 deletions(-)
Also, have deviceSpec contain a list of actual Sizes() instead of counts of unchangeable size blocks and do the math to infer block size and number of blocks to pass to makeLoopDev.
Change any overrides of deviceSpec in subclasses appropriately.
Signed-off-by: mulhern amulhern@redhat.com --- tests/devicelibs_test/btrfs_test.py | 3 ++- tests/devicelibs_test/mdraid_interrogate_test.py | 5 +++-- tests/devicelibs_test/mdraid_test.py | 6 +++--- tests/formats_test/fslabeling.py | 3 ++- tests/formats_test/selinux_test.py | 3 ++- tests/loopbackedtestcase.py | 27 +++++++++++++++--------- 6 files changed, 29 insertions(+), 18 deletions(-)
diff --git a/tests/devicelibs_test/btrfs_test.py b/tests/devicelibs_test/btrfs_test.py index dcf1bcd..91950d2 100755 --- a/tests/devicelibs_test/btrfs_test.py +++ b/tests/devicelibs_test/btrfs_test.py @@ -7,6 +7,7 @@ import unittest
import blivet.devicelibs.btrfs as btrfs from blivet.errors import BTRFSError +from blivet.size import Size
from tests import loopbackedtestcase
@@ -164,7 +165,7 @@ class BTRFSAsRootTestCase2(BTRFSMountDevice): class BTRFSAsRootTestCase3(loopbackedtestcase.LoopBackedTestCase):
def __init__(self, methodName='runTest'): - super(BTRFSAsRootTestCase3, self).__init__(methodName=methodName, deviceSpec=[8192]) + super(BTRFSAsRootTestCase3, self).__init__(methodName=methodName, deviceSpec=[Size("8192 KiB")])
def testSmallDevice(self): """ Creation of a smallish device will result in an error if the diff --git a/tests/devicelibs_test/mdraid_interrogate_test.py b/tests/devicelibs_test/mdraid_interrogate_test.py index 2178d79..034c929 100755 --- a/tests/devicelibs_test/mdraid_interrogate_test.py +++ b/tests/devicelibs_test/mdraid_interrogate_test.py @@ -8,6 +8,7 @@ import blivet.devicelibs.mdraid as mdraid
from . import mdraid_test from blivet.errors import MDRaidError +from blivet.size import Size
class MDRaidInterrogateTestCase(mdraid_test.MDRaidAsRootTestCase):
@@ -33,7 +34,7 @@ class MDRaidInterrogateTestCase(mdraid_test.MDRaidAsRootTestCase): class MDExamineTestCase(MDRaidInterrogateTestCase):
def __init__(self, methodName='runTest'): - super(MDExamineTestCase, self).__init__(methodName=methodName, deviceSpec=[102400, 102400, 102400]) + super(MDExamineTestCase, self).__init__(methodName=methodName, deviceSpec=[Size("100 MiB") for _ in range(3)])
names_0 = [ 'DEVICE', @@ -130,7 +131,7 @@ class MDExamineTestCase(MDRaidInterrogateTestCase): class MDDetailTestCase(MDRaidInterrogateTestCase):
def __init__(self, methodName='runTest'): - super(MDDetailTestCase, self).__init__(methodName=methodName, deviceSpec=[102400, 102400, 102400]) + super(MDDetailTestCase, self).__init__(methodName=methodName, deviceSpec=[Size("100 MiB") for _ in range(3)])
names = [ 'ACTIVE DEVICES', diff --git a/tests/devicelibs_test/mdraid_test.py b/tests/devicelibs_test/mdraid_test.py index 380d853..687ec12 100755 --- a/tests/devicelibs_test/mdraid_test.py +++ b/tests/devicelibs_test/mdraid_test.py @@ -84,7 +84,7 @@ class JustAddTestCaseFactory(TestCaseFactory): def makeClass(name, level):
def __init__(self, methodName='runTest'): - super(self.__class__, self).__init__(methodName=methodName, deviceSpec=[102400, 102400, 102400, 102400, 102400]) + super(self.__class__, self).__init__(methodName=methodName, deviceSpec=[Size("100 MiB") for _ in range(5)]) self.longMessage = True
def testAdd(self): @@ -127,7 +127,7 @@ class GrowTestCaseFactory(object): def makeClass(name, level):
def __init__(self, methodName='runTest'): - super(self.__class__, self).__init__(methodName=methodName, deviceSpec=[102400, 102400, 102400, 102400, 102400, 102400]) + super(self.__class__, self).__init__(methodName=methodName, deviceSpec=[Size("100 MiB") for _ in range(6)]) self.longMessage = True
def testGrow(self): @@ -210,7 +210,7 @@ for l in levels: class SimpleRaidTest(MDRaidAsRootTestCase):
def __init__(self, methodName='runTest'): - super(SimpleRaidTest, self).__init__(methodName=methodName, deviceSpec=[102400, 102400, 102400]) + super(SimpleRaidTest, self).__init__(methodName=methodName, deviceSpec=[Size("100 MiB") for _ in range(3)]) self.longMessage = True
def testMDRaidAsRoot(self): diff --git a/tests/formats_test/fslabeling.py b/tests/formats_test/fslabeling.py index 22d74ee..c35a7b0 100644 --- a/tests/formats_test/fslabeling.py +++ b/tests/formats_test/fslabeling.py @@ -5,6 +5,7 @@ from six import add_metaclass
from tests import loopbackedtestcase from blivet.errors import FSError +from blivet.size import Size
@add_metaclass(abc.ABCMeta) class LabelingAsRoot(loopbackedtestcase.LoopBackedTestCase): @@ -21,7 +22,7 @@ class LabelingAsRoot(loopbackedtestcase.LoopBackedTestCase): doc="A label which is invalid for this filesystem.")
def __init__(self, methodName='runTest'): - super(LabelingAsRoot, self).__init__(methodName=methodName, deviceSpec=[102400]) + super(LabelingAsRoot, self).__init__(methodName=methodName, deviceSpec=[Size("100 MiB")])
def setUp(self): an_fs = self._fs_class() diff --git a/tests/formats_test/selinux_test.py b/tests/formats_test/selinux_test.py index 00771ad..14433da 100755 --- a/tests/formats_test/selinux_test.py +++ b/tests/formats_test/selinux_test.py @@ -7,13 +7,14 @@ import unittest import blivet from tests import loopbackedtestcase import blivet.formats.fs as fs +from blivet.size import Size
class SELinuxContextTestCase(loopbackedtestcase.LoopBackedTestCase): """Testing SELinux contexts. """
def __init__(self, methodName='runTest'): - super(SELinuxContextTestCase, self).__init__(methodName=methodName, deviceSpec=[102400]) + super(SELinuxContextTestCase, self).__init__(methodName=methodName, deviceSpec=[Size("100 MiB")])
def testMountingExt2FS(self): an_fs = fs.Ext2FS(device=self.loopDevices[0], label="test") diff --git a/tests/loopbackedtestcase.py b/tests/loopbackedtestcase.py index cd09bf7..6b996f7 100644 --- a/tests/loopbackedtestcase.py +++ b/tests/loopbackedtestcase.py @@ -2,16 +2,18 @@ import unittest import os import subprocess
+from blivet.size import Size
-def makeLoopDev(device_name, file_name, num_blocks=102400): +def makeLoopDev(device_name, file_name, num_blocks=102400, block_size=1024): """ Set up a loop device with a backing store.
:param str device_name: the path of the loop device :param str file_name: the path of the backing file :param int num_blocks: the size of file_name in number of blocks + :param int block_size: the number of bytes in a block """ proc = subprocess.Popen(["dd", "if=/dev/zero", "of=%s" % file_name, - "bs=1024", "count=%d" % num_blocks], + "bs=%d" % block_size, "count=%d" % num_blocks], stdout=subprocess.PIPE, stderr=subprocess.PIPE) while True: proc.communicate() @@ -67,33 +69,38 @@ def getFreeLoopDev(): @unittest.skipUnless(os.geteuid() == 0, "requires root privileges") class LoopBackedTestCase(unittest.TestCase):
- DEFAULT_STORE_SIZE = 102400 + DEFAULT_BLOCK_SIZE = Size(1024) + DEFAULT_STORE_SIZE = Size("100 MiB") _DEFAULT_DEVICE_SPEC = [DEFAULT_STORE_SIZE, DEFAULT_STORE_SIZE] _STORE_FILE_TEMPLATE = 'test-virtdev%d' _STORE_FILE_PATH = '/var/tmp'
- def __init__(self, methodName='runTest', deviceSpec=None): + def __init__(self, methodName='runTest', deviceSpec=None, block_size=None): """ DevicelibsTestCase manages loop devices.
It constructs loop devices according to loopDeviceSpec, sets them up, and tears them down again.
- :param deviceSpec: Specification for the loop devices. - :type deviceSpec: list of int - - deviceSpec is currently just a list of ints corresponding - to the number of blocks for each backing store. + :param deviceSpec: list containing the size of each loop device + :type deviceSpec: list of Size or NoneType + :param block_size: block size for dd command when making devices + :type block_size: Size or NoneType """ unittest.TestCase.__init__(self, methodName=methodName) self._deviceSpec = deviceSpec or self._DEFAULT_DEVICE_SPEC self._loopMap = {} self.loopDevices = [] + self.block_size = block_size or self.DEFAULT_BLOCK_SIZE + + if any(d % self.block_size != Size(0) for d in self._deviceSpec): + raise ValueError("Every device size must be a multiple of the block size.")
def setUp(self): for index, size in enumerate(self._deviceSpec): store = os.path.join(self._STORE_FILE_PATH, self._STORE_FILE_TEMPLATE % index) dev = getFreeLoopDev() - makeLoopDev(dev, store, num_blocks=size) + num_blocks = int(size / self.block_size) + makeLoopDev(dev, store, num_blocks=num_blocks, block_size=int(self.block_size)) self._loopMap[dev] = store self.loopDevices.append(dev)
On Tue, 2014-09-30 at 09:02 -0400, mulhern wrote:
Also, have deviceSpec contain a list of actual Sizes() instead of counts of unchangeable size blocks and do the math to infer block size and number of blocks to pass to makeLoopDev.
Change any overrides of deviceSpec in subclasses appropriately.
Signed-off-by: mulhern amulhern@redhat.com
tests/devicelibs_test/btrfs_test.py | 3 ++- tests/devicelibs_test/mdraid_interrogate_test.py | 5 +++-- tests/devicelibs_test/mdraid_test.py | 6 +++--- tests/formats_test/fslabeling.py | 3 ++- tests/formats_test/selinux_test.py | 3 ++- tests/loopbackedtestcase.py | 27 +++++++++++++++--------- 6 files changed, 29 insertions(+), 18 deletions(-)
diff --git a/tests/devicelibs_test/btrfs_test.py b/tests/devicelibs_test/btrfs_test.py index dcf1bcd..91950d2 100755 --- a/tests/devicelibs_test/btrfs_test.py +++ b/tests/devicelibs_test/btrfs_test.py @@ -7,6 +7,7 @@ import unittest
import blivet.devicelibs.btrfs as btrfs from blivet.errors import BTRFSError +from blivet.size import Size
from tests import loopbackedtestcase
@@ -164,7 +165,7 @@ class BTRFSAsRootTestCase2(BTRFSMountDevice): class BTRFSAsRootTestCase3(loopbackedtestcase.LoopBackedTestCase):
def __init__(self, methodName='runTest'):
super(BTRFSAsRootTestCase3, self).__init__(methodName=methodName, deviceSpec=[8192])
super(BTRFSAsRootTestCase3, self).__init__(methodName=methodName, deviceSpec=[Size("8192 KiB")])def testSmallDevice(self): """ Creation of a smallish device will result in an error if the
diff --git a/tests/devicelibs_test/mdraid_interrogate_test.py b/tests/devicelibs_test/mdraid_interrogate_test.py index 2178d79..034c929 100755 --- a/tests/devicelibs_test/mdraid_interrogate_test.py +++ b/tests/devicelibs_test/mdraid_interrogate_test.py @@ -8,6 +8,7 @@ import blivet.devicelibs.mdraid as mdraid
from . import mdraid_test from blivet.errors import MDRaidError +from blivet.size import Size
class MDRaidInterrogateTestCase(mdraid_test.MDRaidAsRootTestCase):
@@ -33,7 +34,7 @@ class MDRaidInterrogateTestCase(mdraid_test.MDRaidAsRootTestCase): class MDExamineTestCase(MDRaidInterrogateTestCase):
def __init__(self, methodName='runTest'):
super(MDExamineTestCase, self).__init__(methodName=methodName, deviceSpec=[102400, 102400, 102400])
super(MDExamineTestCase, self).__init__(methodName=methodName, deviceSpec=[Size("100 MiB") for _ in range(3)])names_0 = [ 'DEVICE',
@@ -130,7 +131,7 @@ class MDExamineTestCase(MDRaidInterrogateTestCase): class MDDetailTestCase(MDRaidInterrogateTestCase):
def __init__(self, methodName='runTest'):
super(MDDetailTestCase, self).__init__(methodName=methodName, deviceSpec=[102400, 102400, 102400])
super(MDDetailTestCase, self).__init__(methodName=methodName, deviceSpec=[Size("100 MiB") for _ in range(3)])names = [ 'ACTIVE DEVICES',
diff --git a/tests/devicelibs_test/mdraid_test.py b/tests/devicelibs_test/mdraid_test.py index 380d853..687ec12 100755 --- a/tests/devicelibs_test/mdraid_test.py +++ b/tests/devicelibs_test/mdraid_test.py @@ -84,7 +84,7 @@ class JustAddTestCaseFactory(TestCaseFactory): def makeClass(name, level):
def __init__(self, methodName='runTest'):
super(self.__class__, self).__init__(methodName=methodName, deviceSpec=[102400, 102400, 102400, 102400, 102400])
super(self.__class__, self).__init__(methodName=methodName, deviceSpec=[Size("100 MiB") for _ in range(5)]) self.longMessage = True def testAdd(self):@@ -127,7 +127,7 @@ class GrowTestCaseFactory(object): def makeClass(name, level):
def __init__(self, methodName='runTest'):
super(self.__class__, self).__init__(methodName=methodName, deviceSpec=[102400, 102400, 102400, 102400, 102400, 102400])
super(self.__class__, self).__init__(methodName=methodName, deviceSpec=[Size("100 MiB") for _ in range(6)]) self.longMessage = True def testGrow(self):@@ -210,7 +210,7 @@ for l in levels: class SimpleRaidTest(MDRaidAsRootTestCase):
def __init__(self, methodName='runTest'):
super(SimpleRaidTest, self).__init__(methodName=methodName, deviceSpec=[102400, 102400, 102400])
super(SimpleRaidTest, self).__init__(methodName=methodName, deviceSpec=[Size("100 MiB") for _ in range(3)]) self.longMessage = Truedef testMDRaidAsRoot(self):
diff --git a/tests/formats_test/fslabeling.py b/tests/formats_test/fslabeling.py index 22d74ee..c35a7b0 100644 --- a/tests/formats_test/fslabeling.py +++ b/tests/formats_test/fslabeling.py @@ -5,6 +5,7 @@ from six import add_metaclass
from tests import loopbackedtestcase from blivet.errors import FSError +from blivet.size import Size
@add_metaclass(abc.ABCMeta) class LabelingAsRoot(loopbackedtestcase.LoopBackedTestCase): @@ -21,7 +22,7 @@ class LabelingAsRoot(loopbackedtestcase.LoopBackedTestCase): doc="A label which is invalid for this filesystem.")
def __init__(self, methodName='runTest'):
super(LabelingAsRoot, self).__init__(methodName=methodName, deviceSpec=[102400])
super(LabelingAsRoot, self).__init__(methodName=methodName, deviceSpec=[Size("100 MiB")])def setUp(self): an_fs = self._fs_class()
diff --git a/tests/formats_test/selinux_test.py b/tests/formats_test/selinux_test.py index 00771ad..14433da 100755 --- a/tests/formats_test/selinux_test.py +++ b/tests/formats_test/selinux_test.py @@ -7,13 +7,14 @@ import unittest import blivet from tests import loopbackedtestcase import blivet.formats.fs as fs +from blivet.size import Size
class SELinuxContextTestCase(loopbackedtestcase.LoopBackedTestCase): """Testing SELinux contexts. """
def __init__(self, methodName='runTest'):
super(SELinuxContextTestCase, self).__init__(methodName=methodName, deviceSpec=[102400])
super(SELinuxContextTestCase, self).__init__(methodName=methodName, deviceSpec=[Size("100 MiB")])def testMountingExt2FS(self): an_fs = fs.Ext2FS(device=self.loopDevices[0], label="test")
diff --git a/tests/loopbackedtestcase.py b/tests/loopbackedtestcase.py index cd09bf7..6b996f7 100644 --- a/tests/loopbackedtestcase.py +++ b/tests/loopbackedtestcase.py @@ -2,16 +2,18 @@ import unittest import os import subprocess
+from blivet.size import Size
-def makeLoopDev(device_name, file_name, num_blocks=102400): +def makeLoopDev(device_name, file_name, num_blocks=102400, block_size=1024): """ Set up a loop device with a backing store.
:param str device_name: the path of the loop device :param str file_name: the path of the backing file :param int num_blocks: the size of file_name in number of blocks
:param int block_size: the number of bytes in a block
"bs=1024", "count=%d" % num_blocks],
"bs=%d" % block_size, "count=%d" % num_blocks], stdout=subprocess.PIPE, stderr=subprocess.PIPE)@@ -67,33 +69,38 @@ def getFreeLoopDev(): @unittest.skipUnless(os.geteuid() == 0, "requires root privileges") class LoopBackedTestCase(unittest.TestCase):
- DEFAULT_STORE_SIZE = 102400
- DEFAULT_BLOCK_SIZE = Size(1024)
I think 'Size("1 KiB")' would be nicer here.
- DEFAULT_STORE_SIZE = Size("100 MiB") _DEFAULT_DEVICE_SPEC = [DEFAULT_STORE_SIZE, DEFAULT_STORE_SIZE] _STORE_FILE_TEMPLATE = 'test-virtdev%d' _STORE_FILE_PATH = '/var/tmp'
- def __init__(self, methodName='runTest', deviceSpec=None):
def __init__(self, methodName='runTest', deviceSpec=None, block_size=None): """ DevicelibsTestCase manages loop devices.
It constructs loop devices according to loopDeviceSpec, sets them up, and tears them down again.
:param deviceSpec: Specification for the loop devices.:type deviceSpec: list of intdeviceSpec is currently just a list of ints correspondingto the number of blocks for each backing store.
:param deviceSpec: list containing the size of each loop device:type deviceSpec: list of Size or NoneType:param block_size: block size for dd command when making devices:type block_size: Size or NoneType """ unittest.TestCase.__init__(self, methodName=methodName) self._deviceSpec = deviceSpec or self._DEFAULT_DEVICE_SPEC self._loopMap = {} self.loopDevices = []self.block_size = block_size or self.DEFAULT_BLOCK_SIZEif any(d % self.block_size != Size(0) for d in self._deviceSpec):raise ValueError("Every device size must be a multiple of the block size.")def setUp(self): for index, size in enumerate(self._deviceSpec): store = os.path.join(self._STORE_FILE_PATH, self._STORE_FILE_TEMPLATE % index) dev = getFreeLoopDev()
makeLoopDev(dev, store, num_blocks=size)
num_blocks = int(size / self.block_size)makeLoopDev(dev, store, num_blocks=num_blocks, block_size=int(self.block_size)) self._loopMap[dev] = store self.loopDevices.append(dev)
----- Original Message -----
From: "Vratislav Podzimek" vpodzime@redhat.com To: "anaconda patch review" anaconda-patches@lists.fedorahosted.org Sent: Wednesday, October 1, 2014 5:47:46 AM Subject: Re: [blivet:master 1/2] Further abstract loopbackedtestcase on block_size.
On Tue, 2014-09-30 at 09:02 -0400, mulhern wrote:
<-- SNIP -->
@@ -67,33 +69,38 @@ def getFreeLoopDev(): @unittest.skipUnless(os.geteuid() == 0, "requires root privileges") class LoopBackedTestCase(unittest.TestCase):
- DEFAULT_STORE_SIZE = 102400
- DEFAULT_BLOCK_SIZE = Size(1024)
I think 'Size("1 KiB")' would be nicer here.
I think so, too.
Can we think about humanReadable() one last time? By default, it displays "1 KiB" as "1024 B". I would prefer "1 KiB" and I think most people would.
Trickier is:
Which is better "1044 B" or "1.02 KiB"?
What about "9.99 KiB" vs. "10229 B"?
Why?
<-- SNIP -->
-- Vratislav Podzimek
Anaconda Rider | Red Hat, Inc. | Brno - Czech Republic
anaconda-patches mailing list anaconda-patches@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/anaconda-patches
On Wed, 2014-10-01 at 09:46 -0400, Anne Mulhern wrote:
----- Original Message -----
From: "Vratislav Podzimek" vpodzime@redhat.com To: "anaconda patch review" anaconda-patches@lists.fedorahosted.org Sent: Wednesday, October 1, 2014 5:47:46 AM Subject: Re: [blivet:master 1/2] Further abstract loopbackedtestcase on block_size.
On Tue, 2014-09-30 at 09:02 -0400, mulhern wrote:
<-- SNIP -->
@@ -67,33 +69,38 @@ def getFreeLoopDev(): @unittest.skipUnless(os.geteuid() == 0, "requires root privileges") class LoopBackedTestCase(unittest.TestCase):
- DEFAULT_STORE_SIZE = 102400
- DEFAULT_BLOCK_SIZE = Size(1024)
I think 'Size("1 KiB")' would be nicer here.
I think so, too.
Can we think about humanReadable() one last time? By default, it displays "1 KiB" as "1024 B". I would prefer "1 KiB" and I think most people would.
Trickier is:
Which is better "1044 B" or "1.02 KiB"?
What about "9.99 KiB" vs. "10229 B"?
I like the old rule that less than 10 of something means that users will probably care about small differences, more than 10 means less than 0.01 is not that important difference. With B and KiB it gets a bit weird, but we hardly ever show such values. And if we really encounter something that is smaller than 10 KiB I think it is okay to show the number of bytes.
----- Original Message -----
From: "Vratislav Podzimek" vpodzime@redhat.com To: "anaconda patch review" anaconda-patches@lists.fedorahosted.org Sent: Wednesday, October 1, 2014 10:05:38 AM Subject: Re: [blivet:master 1/2] Further abstract loopbackedtestcase on block_size.
On Wed, 2014-10-01 at 09:46 -0400, Anne Mulhern wrote:
----- Original Message -----
From: "Vratislav Podzimek" vpodzime@redhat.com To: "anaconda patch review" anaconda-patches@lists.fedorahosted.org Sent: Wednesday, October 1, 2014 5:47:46 AM Subject: Re: [blivet:master 1/2] Further abstract loopbackedtestcase on block_size.
On Tue, 2014-09-30 at 09:02 -0400, mulhern wrote:
<-- SNIP -->
@@ -67,33 +69,38 @@ def getFreeLoopDev(): @unittest.skipUnless(os.geteuid() == 0, "requires root privileges") class LoopBackedTestCase(unittest.TestCase):
- DEFAULT_STORE_SIZE = 102400
- DEFAULT_BLOCK_SIZE = Size(1024)
I think 'Size("1 KiB")' would be nicer here.
I think so, too.
Can we think about humanReadable() one last time? By default, it displays "1 KiB" as "1024 B". I would prefer "1 KiB" and I think most people would.
Trickier is:
Which is better "1044 B" or "1.02 KiB"?
What about "9.99 KiB" vs. "10229 B"?
I like the old rule that less than 10 of something means that users will probably care about small differences, more than 10 means less than 0.01 is not that important difference. With B and KiB it gets a bit weird, but we hardly ever show such values. And if we really encounter something that is smaller than 10 KiB I think it is okay to show the number of bytes.
-- Vratislav Podzimek
Anaconda Rider | Red Hat, Inc. | Brno - Czech Republic
anaconda-patches mailing list anaconda-patches@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/anaconda-patches
B and KiBs behave the same as anything else, really.
I can rephrase as:
Which is better "1044 Kib" or "1.02 MiB", "1 GiB" or "1024 Mib", "9.99 MiB" or "10229 KiB"?
Does putting the values in larger units make any difference?
- mulhern
On 10/01/2014 10:49 AM, Anne Mulhern wrote:
----- Original Message -----
From: "Vratislav Podzimek" vpodzime@redhat.com To: "anaconda patch review" anaconda-patches@lists.fedorahosted.org Sent: Wednesday, October 1, 2014 10:05:38 AM Subject: Re: [blivet:master 1/2] Further abstract loopbackedtestcase on block_size.
On Wed, 2014-10-01 at 09:46 -0400, Anne Mulhern wrote:
----- Original Message -----
From: "Vratislav Podzimek" vpodzime@redhat.com To: "anaconda patch review" anaconda-patches@lists.fedorahosted.org Sent: Wednesday, October 1, 2014 5:47:46 AM Subject: Re: [blivet:master 1/2] Further abstract loopbackedtestcase on block_size.
On Tue, 2014-09-30 at 09:02 -0400, mulhern wrote:
<-- SNIP -->
@@ -67,33 +69,38 @@ def getFreeLoopDev(): @unittest.skipUnless(os.geteuid() == 0, "requires root privileges") class LoopBackedTestCase(unittest.TestCase):
- DEFAULT_STORE_SIZE = 102400
- DEFAULT_BLOCK_SIZE = Size(1024)
I think 'Size("1 KiB")' would be nicer here.
I think so, too.
Can we think about humanReadable() one last time? By default, it displays "1 KiB" as "1024 B". I would prefer "1 KiB" and I think most people would.
Trickier is:
Which is better "1044 B" or "1.02 KiB"?
What about "9.99 KiB" vs. "10229 B"?
I like the old rule that less than 10 of something means that users will probably care about small differences, more than 10 means less than 0.01 is not that important difference. With B and KiB it gets a bit weird, but we hardly ever show such values. And if we really encounter something that is smaller than 10 KiB I think it is okay to show the number of bytes.
-- Vratislav Podzimek
Anaconda Rider | Red Hat, Inc. | Brno - Czech Republic
anaconda-patches mailing list anaconda-patches@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/anaconda-patches
B and KiBs behave the same as anything else, really.
I can rephrase as:
Which is better "1044 Kib" or "1.02 MiB", "1 GiB" or "1024 Mib", "9.99 MiB" or "10229 KiB"?
Does putting the values in larger units make any difference?
I prefer the smaller number of digits (larger unit) in every case. I've found the current behavior odd. I draw the line at one of the larger unit, however -- I prefer "768 MiB" to "0.75 GiB".
David
- mulhern
anaconda-patches mailing list anaconda-patches@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/anaconda-patches
----- Original Message -----
From: "David Lehman" dlehman@redhat.com To: anaconda-patches@lists.fedorahosted.org Sent: Wednesday, October 1, 2014 12:55:49 PM Subject: Re: [blivet:master 1/2] Further abstract loopbackedtestcase on block_size.
On 10/01/2014 10:49 AM, Anne Mulhern wrote:
----- Original Message -----
From: "Vratislav Podzimek" vpodzime@redhat.com To: "anaconda patch review" anaconda-patches@lists.fedorahosted.org Sent: Wednesday, October 1, 2014 10:05:38 AM Subject: Re: [blivet:master 1/2] Further abstract loopbackedtestcase on block_size.
On Wed, 2014-10-01 at 09:46 -0400, Anne Mulhern wrote:
----- Original Message -----
From: "Vratislav Podzimek" vpodzime@redhat.com To: "anaconda patch review" anaconda-patches@lists.fedorahosted.org Sent: Wednesday, October 1, 2014 5:47:46 AM Subject: Re: [blivet:master 1/2] Further abstract loopbackedtestcase on block_size.
On Tue, 2014-09-30 at 09:02 -0400, mulhern wrote:
<-- SNIP -->
@@ -67,33 +69,38 @@ def getFreeLoopDev(): @unittest.skipUnless(os.geteuid() == 0, "requires root privileges") class LoopBackedTestCase(unittest.TestCase):
- DEFAULT_STORE_SIZE = 102400
- DEFAULT_BLOCK_SIZE = Size(1024)
I think 'Size("1 KiB")' would be nicer here.
I think so, too.
Can we think about humanReadable() one last time? By default, it displays "1 KiB" as "1024 B". I would prefer "1 KiB" and I think most people would.
Trickier is:
Which is better "1044 B" or "1.02 KiB"?
What about "9.99 KiB" vs. "10229 B"?
I like the old rule that less than 10 of something means that users will probably care about small differences, more than 10 means less than 0.01 is not that important difference. With B and KiB it gets a bit weird, but we hardly ever show such values. And if we really encounter something that is smaller than 10 KiB I think it is okay to show the number of bytes.
-- Vratislav Podzimek
Anaconda Rider | Red Hat, Inc. | Brno - Czech Republic
anaconda-patches mailing list anaconda-patches@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/anaconda-patches
B and KiBs behave the same as anything else, really.
I can rephrase as:
Which is better "1044 Kib" or "1.02 MiB", "1 GiB" or "1024 Mib", "9.99 MiB" or "10229 KiB"?
Does putting the values in larger units make any difference?
I prefer the smaller number of digits (larger unit) in every case. I've found the current behavior odd. I draw the line at one of the larger unit, however -- I prefer "768 MiB" to "0.75 GiB".
That behavior is easy to get...set min_value default to 1 instead of 10 as it is now.
- mulhern
David
- mulhern
anaconda-patches mailing list anaconda-patches@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/anaconda-patches
anaconda-patches mailing list anaconda-patches@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/anaconda-patches
On Wed, 2014-10-01 at 14:16 -0400, Anne Mulhern wrote:
----- Original Message -----
From: "David Lehman" dlehman@redhat.com To: anaconda-patches@lists.fedorahosted.org Sent: Wednesday, October 1, 2014 12:55:49 PM Subject: Re: [blivet:master 1/2] Further abstract loopbackedtestcase on block_size.
On 10/01/2014 10:49 AM, Anne Mulhern wrote:
----- Original Message -----
From: "Vratislav Podzimek" vpodzime@redhat.com To: "anaconda patch review" anaconda-patches@lists.fedorahosted.org Sent: Wednesday, October 1, 2014 10:05:38 AM Subject: Re: [blivet:master 1/2] Further abstract loopbackedtestcase on block_size.
On Wed, 2014-10-01 at 09:46 -0400, Anne Mulhern wrote:
----- Original Message -----
From: "Vratislav Podzimek" vpodzime@redhat.com To: "anaconda patch review" anaconda-patches@lists.fedorahosted.org Sent: Wednesday, October 1, 2014 5:47:46 AM Subject: Re: [blivet:master 1/2] Further abstract loopbackedtestcase on block_size.
On Tue, 2014-09-30 at 09:02 -0400, mulhern wrote:
<-- SNIP -->
> @@ -67,33 +69,38 @@ def getFreeLoopDev(): > @unittest.skipUnless(os.geteuid() == 0, "requires root privileges") > class LoopBackedTestCase(unittest.TestCase): > > - DEFAULT_STORE_SIZE = 102400 > + DEFAULT_BLOCK_SIZE = Size(1024) I think 'Size("1 KiB")' would be nicer here.
I think so, too.
Can we think about humanReadable() one last time? By default, it displays "1 KiB" as "1024 B". I would prefer "1 KiB" and I think most people would.
Trickier is:
Which is better "1044 B" or "1.02 KiB"?
What about "9.99 KiB" vs. "10229 B"?
I like the old rule that less than 10 of something means that users will probably care about small differences, more than 10 means less than 0.01 is not that important difference. With B and KiB it gets a bit weird, but we hardly ever show such values. And if we really encounter something that is smaller than 10 KiB I think it is okay to show the number of bytes.
-- Vratislav Podzimek
Anaconda Rider | Red Hat, Inc. | Brno - Czech Republic
anaconda-patches mailing list anaconda-patches@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/anaconda-patches
B and KiBs behave the same as anything else, really.
I can rephrase as:
Which is better "1044 Kib" or "1.02 MiB", "1 GiB" or "1024 Mib", "9.99 MiB" or "10229 KiB"?
Does putting the values in larger units make any difference?
I prefer the smaller number of digits (larger unit) in every case. I've found the current behavior odd. I draw the line at one of the larger unit, however -- I prefer "768 MiB" to "0.75 GiB".
That behavior is easy to get...set min_value default to 1 instead of 10 as it is now.
Okay, let's go with this change. People will let us know if they like/dislike it, I guess. Still it seems a bit weird to me that with a 2 TiB disk, we would show them e.g. "1.06 TiB" where that ".06" is actually 60 GiB, a size of my SSD in my laptop, though.
----- Original Message -----
From: "Anne Mulhern" amulhern@redhat.com To: "anaconda patch review" anaconda-patches@lists.fedorahosted.org Sent: Wednesday, October 1, 2014 11:49:40 AM Subject: Re: [blivet:master 1/2] Further abstract loopbackedtestcase on block_size.
----- Original Message -----
From: "Vratislav Podzimek" vpodzime@redhat.com To: "anaconda patch review" anaconda-patches@lists.fedorahosted.org Sent: Wednesday, October 1, 2014 10:05:38 AM Subject: Re: [blivet:master 1/2] Further abstract loopbackedtestcase on block_size.
On Wed, 2014-10-01 at 09:46 -0400, Anne Mulhern wrote:
----- Original Message -----
From: "Vratislav Podzimek" vpodzime@redhat.com To: "anaconda patch review" anaconda-patches@lists.fedorahosted.org Sent: Wednesday, October 1, 2014 5:47:46 AM Subject: Re: [blivet:master 1/2] Further abstract loopbackedtestcase on block_size.
On Tue, 2014-09-30 at 09:02 -0400, mulhern wrote:
<-- SNIP -->
@@ -67,33 +69,38 @@ def getFreeLoopDev(): @unittest.skipUnless(os.geteuid() == 0, "requires root privileges") class LoopBackedTestCase(unittest.TestCase):
- DEFAULT_STORE_SIZE = 102400
- DEFAULT_BLOCK_SIZE = Size(1024)
I think 'Size("1 KiB")' would be nicer here.
I think so, too.
Can we think about humanReadable() one last time? By default, it displays "1 KiB" as "1024 B". I would prefer "1 KiB" and I think most people would.
Trickier is:
Which is better "1044 B" or "1.02 KiB"?
What about "9.99 KiB" vs. "10229 B"?
I like the old rule that less than 10 of something means that users will probably care about small differences, more than 10 means less than 0.01 is not that important difference. With B and KiB it gets a bit weird, but we hardly ever show such values. And if we really encounter something that is smaller than 10 KiB I think it is okay to show the number of bytes.
-- Vratislav Podzimek
Anaconda Rider | Red Hat, Inc. | Brno - Czech Republic
anaconda-patches mailing list anaconda-patches@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/anaconda-patches
B and KiBs behave the same as anything else, really.
I can rephrase as:
Which is better "1044 Kib" or "1.02 MiB", "1 GiB" or "1024 Mib", "9.99 MiB" or "10229 KiB"?
Actually, it's "9.99 MiB" or "10229.76 KiB" and "1044.48 KiB" or "1.02 MiB", for some funny algebraic reason.
Does putting the values in larger units make any difference?
- mulhern
anaconda-patches mailing list anaconda-patches@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/anaconda-patches
This change forces devices tests to be slightly more realistic in order to pass, so update them as well.
Also make failing test that relied on literal constant value that was supposed to be minimum member size pass again and add a few tests to bracket the minimum member size in case it changes again.
Signed-off-by: mulhern amulhern@redhat.com --- blivet/devicelibs/btrfs.py | 4 ++++ blivet/devices.py | 10 ++++++++++ tests/devicelibs_test/btrfs_test.py | 22 ++++++++++++++++++++-- tests/devices_test.py | 26 +++++++++++++++++++------- 4 files changed, 53 insertions(+), 9 deletions(-)
diff --git a/blivet/devicelibs/btrfs.py b/blivet/devicelibs/btrfs.py index 3e411c8..bead746 100644 --- a/blivet/devicelibs/btrfs.py +++ b/blivet/devicelibs/btrfs.py @@ -26,6 +26,7 @@ import re from . import raid from .. import util from ..errors import BTRFSError, BTRFSValueError +from ..size import Size
import logging log = logging.getLogger("blivet") @@ -33,6 +34,9 @@ log = logging.getLogger("blivet") # this is the volume id btrfs always assigns to the top-level volume/tree MAIN_VOLUME_ID = 5
+# if any component device is less than this size, mkfs.btrfs will fail +MIN_MEMBER_SIZE = Size("16 MiB") + RAID_levels = raid.RAIDLevels(["raid0", "raid1", "raid10", "single"])
metadata_levels = raid.RAIDLevels(["raid0", "raid1", "raid10", "single", "dup"]) diff --git a/blivet/devices.py b/blivet/devices.py index 12ad234..5092bfb 100644 --- a/blivet/devices.py +++ b/blivet/devices.py @@ -5115,6 +5115,12 @@ class BTRFSVolumeDevice(BTRFSDevice, ContainerDevice):
self._defaultSubVolumeID = None
+ def _addParent(self, member): + log_method_call(self, self.name, member=member.name) + if not self.exists and member.size < btrfs.MIN_MEMBER_SIZE: + raise errors.BTRFSValueError("BTRFS member device must have size at least %s." % btrfs.MIN_MEMBER_SIZE) + super(BTRFSVolumeDevice, self)._addParent(member) + def _validateRaidLevel(self, level): """ Returns an error message if the RAID level is invalid for this device, otherwise None. @@ -5330,6 +5336,10 @@ class BTRFSVolumeDevice(BTRFSDevice, ContainerDevice):
return default
+ def _preCreate(self): + if any(p.size < btrfs.MIN_MEMBER_SIZE for p in self.parents): + raise errors.DeviceCreateError("All BTRFS member devices must have size at least %s." % btrfs.MIN_MEMBER_SIZE) + def _create(self): log_method_call(self, self.name, status=self.status) btrfs.create_volume(devices=[d.path for d in self.parents], diff --git a/tests/devicelibs_test/btrfs_test.py b/tests/devicelibs_test/btrfs_test.py index 91950d2..e3f658e 100755 --- a/tests/devicelibs_test/btrfs_test.py +++ b/tests/devicelibs_test/btrfs_test.py @@ -7,7 +7,6 @@ import unittest
import blivet.devicelibs.btrfs as btrfs from blivet.errors import BTRFSError -from blivet.size import Size
from tests import loopbackedtestcase
@@ -165,7 +164,7 @@ class BTRFSAsRootTestCase2(BTRFSMountDevice): class BTRFSAsRootTestCase3(loopbackedtestcase.LoopBackedTestCase):
def __init__(self, methodName='runTest'): - super(BTRFSAsRootTestCase3, self).__init__(methodName=methodName, deviceSpec=[Size("8192 KiB")]) + super(BTRFSAsRootTestCase3, self).__init__(methodName=methodName, deviceSpec=[btrfs.MIN_MEMBER_SIZE, btrfs.MIN_MEMBER_SIZE])
def testSmallDevice(self): """ Creation of a smallish device will result in an error if the @@ -176,5 +175,24 @@ class BTRFSAsRootTestCase3(loopbackedtestcase.LoopBackedTestCase): btrfs.create_volume(self.loopDevices, data="single", metadata="dup") self.assertEqual(btrfs.create_volume(self.loopDevices), 0)
+class BTRFSTooSmallDeviceTestCase(loopbackedtestcase.LoopBackedTestCase): + + def __init__(self, methodName='runTest'): + super(BTRFSTooSmallDeviceTestCase, self).__init__(methodName=methodName, deviceSpec=[btrfs.MIN_MEMBER_SIZE / 2, btrfs.MIN_MEMBER_SIZE]) + + def testTooSmallDevice(self): + """ If just one device is too small mkfs.btrfs will fail. """ + with self.assertRaises(BTRFSError): + btrfs.create_volume(self.loopDevices) + +class BTRFSBigEnoughDeviceTestCase(loopbackedtestcase.LoopBackedTestCase): + + def __init__(self, methodName='runTest'): + super(BTRFSBigEnoughDeviceTestCase, self).__init__(methodName=methodName, deviceSpec=[btrfs.MIN_MEMBER_SIZE]) + + def testBigEnoughDevice(self): + """ If all devices are large enough, mkfs.btrfs will succeed. """ + self.assertEqual(btrfs.create_volume(self.loopDevices), 0) + if __name__ == "__main__": unittest.main() diff --git a/tests/devices_test.py b/tests/devices_test.py index 31b4de3..658713f 100644 --- a/tests/devices_test.py +++ b/tests/devices_test.py @@ -7,6 +7,7 @@ from mock import Mock
import blivet
+from blivet.errors import BTRFSValueError from blivet.errors import DeviceError from blivet.errors import RaidError
@@ -20,7 +21,6 @@ from blivet.devices import LVMThinLogicalVolumeDevice from blivet.devices import LVMThinSnapShotDevice from blivet.devices import LVMVolumeGroupDevice from blivet.devices import MDRaidArrayDevice -from blivet.devices import OpticalDevice from blivet.devices import StorageDevice from blivet.devices import ParentList from blivet.devicelibs import btrfs @@ -437,8 +437,9 @@ class BTRFSDeviceTestCase(DeviceStateTestCase): "vol_id" : lambda x, m: self.assertEqual(x, btrfs.MAIN_VOLUME_ID, m)}
self.dev1 = BTRFSVolumeDevice("dev1", - parents=[OpticalDevice("deva", - fmt=blivet.formats.getFormat("btrfs"))]) + parents=[StorageDevice("deva", + fmt=blivet.formats.getFormat("btrfs"), + size=btrfs.MIN_MEMBER_SIZE)])
self.dev2 = BTRFSSubVolumeDevice("dev2", parents=[self.dev1])
@@ -455,7 +456,10 @@ class BTRFSDeviceTestCase(DeviceStateTestCase): """
self.stateCheck(self.dev1, + currentSize=lambda x, m: self.assertEqual(x, btrfs.MIN_MEMBER_SIZE), parents=lambda x, m: self.assertEqual(len(x), 1, m), + maxSize=lambda x, m: self.assertEqual(x, btrfs.MIN_MEMBER_SIZE), + size=lambda x, m: self.assertEqual(x, btrfs.MIN_MEMBER_SIZE), type=lambda x, m: self.assertEqual(x, "btrfs volume", m))
self.stateCheck(self.dev3, @@ -469,11 +473,11 @@ class BTRFSDeviceTestCase(DeviceStateTestCase): BTRFSVolumeDevice("dev")
with self.assertRaisesRegexp(ValueError, "member has wrong format"): - BTRFSVolumeDevice("dev", parents=[OpticalDevice("deva")]) + BTRFSVolumeDevice("dev", parents=[StorageDevice("deva", size=btrfs.MIN_MEMBER_SIZE)])
with self.assertRaisesRegexp(DeviceError, "btrfs subvolume.*must be a btrfs volume"): fmt = blivet.formats.getFormat("btrfs") - device = OpticalDevice("deva", fmt=fmt) + device = StorageDevice("deva", fmt=fmt, size=btrfs.MIN_MEMBER_SIZE) BTRFSSubVolumeDevice("dev1", parents=[device])
self.assertEqual(self.dev1.isleaf, False) @@ -502,7 +506,7 @@ class BTRFSDeviceTestCase(DeviceStateTestCase): self.dev1.size = 32
def testBTRFSSnapShotDeviceInit(self): - parents = [StorageDevice("p1", fmt=blivet.formats.getFormat("btrfs"))] + parents = [StorageDevice("p1", fmt=blivet.formats.getFormat("btrfs"), size=btrfs.MIN_MEMBER_SIZE)] vol = BTRFSVolumeDevice("test", parents=parents) with self.assertRaisesRegexp(ValueError, "non-existent btrfs snapshots must have a source"): BTRFSSnapShotDevice("snap1", parents=[vol]) @@ -513,7 +517,7 @@ class BTRFSDeviceTestCase(DeviceStateTestCase): with self.assertRaisesRegexp(ValueError, "btrfs snapshot source must be a btrfs subvolume"): BTRFSSnapShotDevice("snap1", parents=[vol], source=parents[0])
- parents2 = [StorageDevice("p1", fmt=blivet.formats.getFormat("btrfs"))] + parents2 = [StorageDevice("p1", fmt=blivet.formats.getFormat("btrfs"), size=btrfs.MIN_MEMBER_SIZE)] vol2 = BTRFSVolumeDevice("test2", parents=parents2, exists=True) with self.assertRaisesRegexp(ValueError, ".*snapshot and source must be in the same volume"): BTRFSSnapShotDevice("snap1", parents=[vol], source=vol2) @@ -528,6 +532,14 @@ class BTRFSDeviceTestCase(DeviceStateTestCase): self.assertEqual(snap.dependsOn(vol), True) self.assertEqual(vol.dependsOn(snap), False)
+ def testMinMemberSize(self): + parent = StorageDevice("deva", fmt=blivet.formats.getFormat("btrfs")) + parent.size=btrfs.MIN_MEMBER_SIZE / 2 + with self.assertRaisesRegexp(BTRFSValueError, "BTRFS member device must have size at least %s" % btrfs.MIN_MEMBER_SIZE): + BTRFSVolumeDevice("dev1", parents = [parent]) + parent.size=btrfs.MIN_MEMBER_SIZE + self.assertIsNotNone(BTRFSVolumeDevice("dev1", parents=[parent])) + class LVMDeviceTest(unittest.TestCase): def testLVMSnapShotDeviceInit(self): pv = StorageDevice("pv1", fmt=blivet.formats.getFormat("lvmpv"),
On Tue, 2014-09-30 at 09:02 -0400, mulhern wrote:
Rebased against most recent version of master and seems OK.
Originally my plan was just to get rid of the test that was failing on Jenkins, since it did not test anything we use. But on further consideration, it appeared that that was a mistake, i.e., we should be using that information. So that's what this patch set does instead.
mulhern (2): Further abstract loopbackedtestcase on block_size. Check the minimum member size for BtrfsVolumeDevices.
blivet/devicelibs/btrfs.py | 4 ++++ blivet/devices.py | 10 +++++++++ tests/devicelibs_test/btrfs_test.py | 21 +++++++++++++++++- tests/devicelibs_test/mdraid_interrogate_test.py | 5 +++-- tests/devicelibs_test/mdraid_test.py | 6 +++--- tests/devices_test.py | 26 +++++++++++++++++------ tests/formats_test/fslabeling.py | 3 ++- tests/formats_test/selinux_test.py | 3 ++- tests/loopbackedtestcase.py | 27 +++++++++++++++--------- 9 files changed, 80 insertions(+), 25 deletions(-)
Other than the neatpick comment these both look good to me.
anaconda-patches@lists.fedorahosted.org
-
Anne Mulhern -
David Lehman -
Vratislav Podzimek