open-iscsi / targetd

Remote configuration of a LIO-based storage appliance
GNU General Public License v3.0
71 stars 28 forks source link

Rewrite LVM Code to libblockdev #27

Closed Queuecumber closed 5 years ago

Queuecumber commented 5 years ago

This PR removes the deprecated lvm python bindings and replaces them with bindings from libblockdev which uses the more modern DBus API or even the binaries. It also officially deprecates the FS API which depends on BTRFS which itself is deprecated. The FS API will continue to function as it used to but is unmaintained may break at any time. A warning is logged if this API is used.

This PR is a placeholder and should be held pending further testing, feedback, and eventual squashing of the commits.

Queuecumber commented 5 years ago

Closes #25 and #22 (and #26 which is already closed)

Queuecumber commented 5 years ago

This now passes the "automated testing" in client.py so I'm going to start working with real applications

Queuecumber commented 5 years ago

I performed the following practical test

Everything worked great so I think this is gtg. Awaiting feedback from @tasleson to proceed.

tasleson commented 5 years ago

@Queuecumber Thanks for the PR. I'll take a look at it tomorrow and I'll also run the libStorageMgmt tests against it.

Queuecumber commented 5 years ago

There are a couple of comments I had left that I wanted to make sure you had a look at before we call this done, once you give the final sign-off I will squash the commits into a single one and force push over this PR to keep the history clean

tasleson commented 5 years ago

@Queuecumber

I got the tests to run for libStorageMgmt unit tests. In the process I found 1 issue with your change which I missed in my review:

diff --git a/targetd/block.py b/targetd/block.py
index c58827d..8adf226 100644
--- a/targetd/block.py
+++ b/targetd/block.py
@@ -128,7 +128,7 @@ def volumes(req, pool):
                 output.append(dict(name=lv.lv_name, size=lv.size,
                                    uuid=lv.uuid))
         else:
-            if attrib[0] == 'V' and lv.lv_name == lv_pool:
+            if attrib[0] == 'V' and lv.pool_lv == lv_pool:
                 output.append(dict(name=lv.lv_name, size=lv.size,
                                    uuid=lv.uuid))

and a couple of issues in areas you did not change (python, rtslib) which I'll fix once we get this PR merged.

Please also add back this line in the README.md which was removed in c05a6c2

fs_pools: [/mnt/btrfs]

Once you make these 2 changes please go ahead and squash and push PR and we can then get this merged, thanks!

Queuecumber commented 5 years ago

@tasleson I think I had misunderstood that line when I originally wrote the code. Both fixed.

Queuecumber commented 5 years ago

OK for real this time, squashed and ready to go

Queuecumber commented 5 years ago

Oops I think I know what happened, fix incoming

Queuecumber commented 5 years ago

OK this should be the correct changeset

tasleson commented 5 years ago

@Queuecumber Thank you!