[linux-lvm] [PATCH 1/2] python: Make lv addTag/removeTag persistant

[linux-lvm] [PATCH 1/2] python: Make lv addTag/removeTag persistant

[Tony Asleson]

Michael Schmidt posted an email on linux-lvm about this bug.
Posting this patch for review which corrects the issue and
adds a unit test to verify functionality.

Signed-off-by: Tony Asleson <tasleson@redhat.com>
---
 python/liblvm.c             |   17 +++++++-
 test/api/pytest.sh          |    1 +
 test/api/python_lvm_unit.py |   94 +++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 111 insertions(+), 1 deletions(-)

diff --git a/python/liblvm.c b/python/liblvm.c
index 3828f27..002174f 100644
--- a/python/liblvm.c
+++ b/python/liblvm.c
@@ -1449,8 +1449,16 @@ static PyObject *_liblvm_lvm_lv_add_tag(lvobject *self, PyObject *args)
 		return NULL;
 	}
 
+    if (lvm_vg_write(self->parent_vgobj->vg) == -1)
+		goto error;
+
 	Py_INCREF(Py_None);
 	return Py_None;
+
+error:
+	PyErr_SetObject(_LibLVMError, _liblvm_get_last_error());
+
+	return NULL;
 }
 
 static PyObject *_liblvm_lvm_lv_remove_tag(lvobject *self, PyObject *args)
@@ -1467,9 +1475,16 @@ static PyObject *_liblvm_lvm_lv_remove_tag(lvobject *self, PyObject *args)
 		return NULL;
 	}
 
-	Py_INCREF(Py_None);
+    if (lvm_vg_write(self->parent_vgobj->vg) == -1)
+        goto error;
 
+	Py_INCREF(Py_None);
 	return Py_None;
+
+error:
+	PyErr_SetObject(_LibLVMError, _liblvm_get_last_error());
+
+	return NULL;
 }
 
 static PyObject *_liblvm_lvm_lv_get_tags(lvobject *self)
diff --git a/test/api/pytest.sh b/test/api/pytest.sh
index 36f55fa..00ab051 100644
--- a/test/api/pytest.sh
+++ b/test/api/pytest.sh
@@ -45,6 +45,7 @@ export PY_UNIT_PVS=$(cat DEVICES)
 #python_lvm_unit.py -v -f
 
 # Run individual tests for shorter error trace
+python_lvm_unit.py -v TestLvm.test_lv_persistence
 python_lvm_unit.py -v TestLvm.test_config_find_bool
 python_lvm_unit.py -v TestLvm.test_config_override
 python_lvm_unit.py -v TestLvm.test_config_reload
diff --git a/test/api/python_lvm_unit.py b/test/api/python_lvm_unit.py
index 2f22fae..76f7f8a 100755
--- a/test/api/python_lvm_unit.py
+++ b/test/api/python_lvm_unit.py
@@ -368,6 +368,100 @@ class TestLvm(unittest.TestCase):
 		lv.rename(current_name)
 		vg.close()
 
+	def test_lv_persistence(self):
+		# Make changes to the lv, close the vg and re-open to make sure that
+		# the changes persist
+		lv_name = 'lv_test_persist'
+		TestLvm._create_thick_lv(TestLvm._get_pv_device_names(), lv_name)
+
+		# Test rename
+		lv, vg = TestLvm._get_lv(None, lv_name)
+		current_name = lv.getName()
+		new_name = rs()
+		lv.rename(new_name)
+
+		vg.close()
+		vg = None
+
+		lv, vg = TestLvm._get_lv(None, new_name)
+
+		self.assertTrue(lv is not None)
+		
+		if lv and vg:
+			lv.rename(lv_name)
+			vg.close()
+			vg = None
+
+		# Test lv tag add
+		tag = 'hello_world'
+
+		lv, vg = TestLvm._get_lv(None, lv_name)
+		lv.addTag(tag)
+		vg.close()
+		vg = None
+
+		lv, vg = TestLvm._get_lv(None, lv_name)
+		tags = lv.getTags()
+
+		self.assertTrue(tag in tags)
+		vg.close()
+		vg = None
+
+		# Test lv tag delete
+		lv, vg = TestLvm._get_lv(None, lv_name)
+		self.assertTrue(lv is not None and vg is not None)
+		
+		if lv and vg:
+			tags = lv.getTags()
+
+			for t in tags:
+				lv.removeTag(t)	
+
+			vg.close()
+			vg = None
+		
+		lv, vg = TestLvm._get_lv(None, lv_name)
+		self.assertTrue(lv is not None and vg is not None)
+
+		if lv and vg:
+			tags = lv.getTags()
+			
+			if tags:
+				self.assertEqual(len(tags), 0)		
+			vg.close()
+			vg = None
+
+		# Test lv deactivate
+		lv, vg = TestLvm._get_lv(None, lv_name)
+		self.assertTrue(lv is not None and vg is not None)		
+
+		if lv and vg:
+			lv.deactivate()
+			vg.close()
+			vg = None
+
+		lv, vg = TestLvm._get_lv(None, lv_name)
+		self.assertTrue(lv is not None and vg is not None)
+		if lv and vg:	
+			self.assertFalse(lv.isActive())		
+			vg.close()
+			vg = None
+
+		# Test lv activate
+		lv, vg = TestLvm._get_lv(None, lv_name)
+		self.assertTrue(lv is not None and vg is not None)
+		if lv and vg:
+			lv.activate()
+			vg.close()
+			vg = None
+
+		lv, vg = TestLvm._get_lv(None, lv_name)
+		self.assertTrue(lv is not None and vg is not None)
+		if lv and vg:
+			self.assertTrue(lv.isActive())
+			vg.close()
+			vg = None
+
 	def test_lv_snapshot(self):
 
 		thin_lv = 'thin_lv'
-- 
1.7.1

[linux-lvm] [PATCH 2/2] python unit test ws fixes

[Tony Asleson]

Signed-off-by: Tony Asleson <tasleson@redhat.com>
---
 test/api/python_lvm_unit.py |   85 ++++++++++++++++++++++++-------------------
 1 files changed, 47 insertions(+), 38 deletions(-)

diff --git a/test/api/python_lvm_unit.py b/test/api/python_lvm_unit.py
index 76f7f8a..d0df727 100755
--- a/test/api/python_lvm_unit.py
+++ b/test/api/python_lvm_unit.py
@@ -44,8 +44,8 @@ def rs(rand_len=10):
 	"""
 	Generate a random string
 	"""
-	return ''.join(random.choice(string.ascii_uppercase)
-				   for x in range(rand_len))
+	return ''.join(
+		random.choice(string.ascii_uppercase)for x in range(rand_len))
 
 
 def _get_allowed_devices():
@@ -131,15 +131,15 @@ class TestLvm(unittest.TestCase):
 		for d in device_list:
 			vg.extend(d)
 
-		vg.createLvThinpool(pool_name, vg.getSize()/2, 0, 0,
-				    lvm.THIN_DISCARDS_PASSDOWN, 1)
+		vg.createLvThinpool(
+			pool_name, vg.getSize() / 2, 0, 0, lvm.THIN_DISCARDS_PASSDOWN, 1)
 		return vg
 
 	@staticmethod
 	def _create_thin_lv(pv_devices, name):
 		thin_pool_name = 'thin_vg_pool_' + rs(4)
 		vg = TestLvm._create_thin_pool(pv_devices, thin_pool_name)
-		vg.createLvThin(thin_pool_name, name, vg.getSize()/8)
+		vg.createLvThin(thin_pool_name, name, vg.getSize() / 8)
 		vg.close()
 		vg = None
 
@@ -239,7 +239,7 @@ class TestLvm(unittest.TestCase):
 			curr_size = pv.getSize()
 			dev_size = pv.getDevSize()
 			self.assertTrue(curr_size == dev_size)
-			pv.resize(curr_size/2)
+			pv.resize(curr_size / 2)
 		with AllowedPVS() as pvs:
 			pv = pvs[0]
 			resized_size = pv.getSize()
@@ -298,17 +298,21 @@ class TestLvm(unittest.TestCase):
 			self.assertEqual(type(pv.getUuid()), str)
 			self.assertTrue(len(pv.getUuid()) > 0)
 
-			self.assertTrue(type(pv.getMdaCount()) == int or
-							type(pv.getMdaCount()) == long)
+			self.assertTrue(
+				type(pv.getMdaCount()) == int or
+				type(pv.getMdaCount()) == long)
 
-			self.assertTrue(type(pv.getSize()) == int or
-							type(pv.getSize()) == long)
+			self.assertTrue(
+				type(pv.getSize()) == int or
+				type(pv.getSize()) == long)
 
-			self.assertTrue(type(pv.getDevSize()) == int or
-							type(pv.getSize()) == long)
+			self.assertTrue(
+				type(pv.getDevSize()) == int or
+				type(pv.getSize()) == long)
 
-			self.assertTrue(type(pv.getFree()) == int or
-							type(pv.getFree()) == long)
+			self.assertTrue(
+				type(pv.getFree()) == int or
+				type(pv.getFree()) == long)
 
 	def _test_prop(self, prop_obj, prop, var_type, settable):
 		result = prop_obj.getProperty(prop)
@@ -516,7 +520,7 @@ class TestLvm(unittest.TestCase):
 		lv, vg = TestLvm._get_lv(None, lv_name)
 
 		curr_size = lv.getSize()
-		lv.resize(curr_size+(1024*1024))
+		lv.resize(curr_size + (1024 * 1024))
 		latest = lv.getSize()
 		self.assertTrue(curr_size != latest)
 
@@ -542,8 +546,9 @@ class TestLvm(unittest.TestCase):
 
 		new_extent = 1024 * 1024 * 4
 
-		self.assertFalse(vg.getExtentSize() != new_extent,
-						 "Cannot determine if it works if they are the same")
+		self.assertFalse(
+			vg.getExtentSize() != new_extent,
+			"Cannot determine if it works if they are the same")
 
 		vg.setExtentSize(new_extent)
 		self.assertEqual(vg.getExtentSize(), new_extent)
@@ -602,8 +607,8 @@ class TestLvm(unittest.TestCase):
 			if len(lvs):
 				lv = lvs[0]
 				lv_name = lv.getName()
-				self.assertRaises(lvm.LibLVMError, vg.createLvLinear, lv_name,
-								  lv.getSize())
+				self.assertRaises(
+					lvm.LibLVMError, vg.createLvLinear, lv_name, lv.getSize())
 			vg.close()
 
 	def test_vg_uuids(self):
@@ -653,10 +658,10 @@ class TestLvm(unittest.TestCase):
 				pv_name_lookup = vg.pvFromName(name)
 				pv_uuid_lookup = vg.pvFromUuid(uuid)
 
-				self.assertTrue(pv_name_lookup.getName() ==
-								pv_uuid_lookup.getName())
-				self.assertTrue(pv_name_lookup.getUuid() ==
-								pv_uuid_lookup.getUuid())
+				self.assertTrue(
+					pv_name_lookup.getName() == pv_uuid_lookup.getName())
+				self.assertTrue(
+					pv_name_lookup.getUuid() == pv_uuid_lookup.getUuid())
 
 				self.assertTrue(name == pv_name_lookup.getName())
 				self.assertTrue(uuid == pv_uuid_lookup.getUuid())
@@ -738,9 +743,10 @@ class TestLvm(unittest.TestCase):
 			self.assertTrue(len(uuid) > 0)
 			vg.close()
 
-	RETURN_NUMERIC = ["getSeqno", "getSize", "getFreeSize", "getFreeSize",
-					  "getExtentSize", "getExtentCount", "getFreeExtentCount",
-					  "getPvCount", "getMaxPv", "getMaxLv"]
+	RETURN_NUMERIC = [
+		"getSeqno", "getSize", "getFreeSize", "getFreeSize",
+		"getExtentSize", "getExtentCount", "getFreeExtentCount",
+		"getPvCount", "getMaxPv", "getMaxLv"]
 
 	def test_vg_getters(self):
 		device_names = TestLvm._get_pv_device_names()
@@ -804,7 +810,7 @@ class TestLvm(unittest.TestCase):
 		i = 0
 		for d in device_names:
 			if i % 2 == 0:
-				TestLvm._create_thin_lv([d],  "thin_lv%d" % i)
+				TestLvm._create_thin_lv([d], "thin_lv%d" % i)
 			else:
 				TestLvm._create_thick_lv([d], "thick_lv%d" % i)
 			i += 1
@@ -854,7 +860,7 @@ class TestLvm(unittest.TestCase):
 			lvm.pvCreate(d)
 
 	def test_pv_create(self):
-		size = [0, 1024*1024*4]
+		size = [0, 1024 * 1024 * 4]
 		pvmeta_copies = [0, 1, 2]
 		pvmeta_size = [0, 255, 512, 1024]
 		data_alignment = [0, 2048, 4096]
@@ -873,9 +879,9 @@ class TestLvm(unittest.TestCase):
 		self.assertRaises(lvm.LibLVMError, lvm.pvCreate, '')
 		self.assertRaises(lvm.LibLVMError, lvm.pvCreate, d, 4)
 		self.assertRaises(lvm.LibLVMError, lvm.pvCreate, d, 0, 4)
-		self.assertRaises(lvm.LibLVMError, lvm.pvCreate, d, 0, 0, 0, 2**34)
-		self.assertRaises(lvm.LibLVMError, lvm.pvCreate, d, 0, 0, 0, 4096,
-						  2**34)
+		self.assertRaises(lvm.LibLVMError, lvm.pvCreate, d, 0, 0, 0, 2 ** 34)
+		self.assertRaises(
+			lvm.LibLVMError, lvm.pvCreate, d, 0, 0, 0, 4096, 2 ** 34)
 
 		#Try a number of combinations and permutations
 		for s in size:
@@ -891,12 +897,14 @@ class TestLvm(unittest.TestCase):
 						lvm.pvCreate(d, s, copies, pv_size, align)
 						lvm.pvRemove(d)
 						for align_offset in data_alignment_offset:
-							lvm.pvCreate(d, s, copies, pv_size, align,
-										 align * align_offset)
+							lvm.pvCreate(
+								d, s, copies, pv_size, align,
+								align * align_offset)
 							lvm.pvRemove(d)
 							for z in zero:
-								lvm.pvCreate(d, s, copies, pv_size, align,
-											 align * align_offset, z)
+								lvm.pvCreate(
+									d, s, copies, pv_size, align,
+									align * align_offset, z)
 								lvm.pvRemove(d)
 
 		#Restore
@@ -960,7 +968,7 @@ class TestLvm(unittest.TestCase):
 				method(t)
 
 	def _test_bad_names(self, method, dupe_name):
-		 # Test for duplicate name
+		# Test for duplicate name
 		self.assertRaises(lvm.LibLVMError, method, dupe_name)
 
 		# Test for too long a name
@@ -983,8 +991,9 @@ class TestLvm(unittest.TestCase):
 
 	def _lv_reserved_names(self, method):
 		prefixes = ['snapshot', 'pvmove']
-		reserved = ['_mlog', '_mimage', '_pmspare', '_rimage', '_rmeta',
-					'_vorigin', '_tdata', '_tmeta']
+		reserved = [
+			'_mlog', '_mimage', '_pmspare', '_rimage', '_rmeta',
+			'_vorigin', '_tdata', '_tmeta']
 
 		for p in prefixes:
 			self.assertRaises(lvm.LibLVMError, method, p + rs(3))
-- 
1.7.1

Re: [linux-lvm] [PATCH 1/2] python: Make lv addTag/removeTag persistant

[Alasdair G Kergon]

On Tue, Apr 07, 2015 at 03:54:01PM -0500, Tony Asleson wrote:
> +    if (lvm_vg_write(self->parent_vgobj->vg) == -1)

In the end, you may need to make this configurable for the library user
(consistently over all functions).
Some users will want to set lots of properties on lots of LVs followed
by a single atomic commit for them all; whereas other users will want to
commit each change independently.

Alasdair

Re: [linux-lvm] [PATCH 1/2] python: Make lv addTag/removeTag persistant

[Tony Asleson]

On 04/07/2015 04:04 PM, Alasdair G Kergon wrote:
> On Tue, Apr 07, 2015 at 03:54:01PM -0500, Tony Asleson wrote:
>> +    if (lvm_vg_write(self->parent_vgobj->vg) == -1)
> 
> In the end, you may need to make this configurable for the library user
> (consistently over all functions).
> Some users will want to set lots of properties on lots of LVs followed
> by a single atomic commit for them all; whereas other users will want to
> commit each change independently.

If a user wants that level of functionality they can use the C library
interface.  The python interface has only offered the mode where changes
are auto committed.

This is a bug fix to make the lv tag add/remove persist as expected.

-Tony