Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Rewrite LVM Code to libblockdev #27

Merged
merged 1 commit into from
Jul 3, 2019

Conversation

Queuecumber
Copy link
Contributor

@Queuecumber Queuecumber commented Jul 1, 2019

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
Copy link
Contributor Author

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

@Queuecumber Queuecumber force-pushed the modernize branch 2 times, most recently from 2910374 to a4ad602 Compare July 1, 2019 17:22
@Queuecumber
Copy link
Contributor Author

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

@Queuecumber
Copy link
Contributor Author

I performed the following practical test

  • Make a sparse file and mount it as a loop device
  • Provision a volume group and thin pool on the loop device
  • Use the targetd API to create a thin volume and export an iSCSI target
  • Use the exported iSCSI target as the primary disk for a libvirt/qemu virtual machine
  • Install ubuntu on the VM backed by the iSCSI target and perform some light tasks
  • Destroy the VM
  • Use the targetd API to remove the thin volume
  • Destroy the thin pool and volume group
  • Remove the loop device
  • Destroy the underlying sparse file

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

@tasleson
Copy link
Member

tasleson commented Jul 2, 2019

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

targetd/block.py Outdated
test_vg.close()
test_vg = bd.lvm.vginfo(vg_name)

# TODO is this the right error code and why isnt defined in the enum
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Based on the existing use of -151 being used as an error indicator that a volume was not exported, I think we should create a new one. I agree that this should be added to TargetdError which currently has:

class TargetdError(Exception):
    INVALID_ARGUMENT = -32602
    NO_SUPPORT = -153
    NAME_CONFLICT = -50
    EXISTS_INITIATOR = -52
    NO_FREE_HOST_LUN_ID = -1000
    NOT_FOUND_ACCESS_GROUP = -200
    VOLUME_MASKED = -303

It appears the following are missing and being used as magic numbers too:

  • -110
  • -1 (Probably should be -152 NO_SUPPORT)
  • -53
  • -303 (which has different meaning)
  • -104
  • -112
  • -51
  • -401
  • -400

Please pick a new value and add to TargetdError. Once we get this PR merged I'll go though and clean up the other magic number mess.

targetd/fs.py Outdated
@@ -52,6 +52,8 @@
def initialize(config_dict):

global pools
log.warning('Filesystem API is deprecated and no longer maintained. Use at your own risk.')

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm starting to question the decision to deprecate this. btrfs is shipping on many distributions, we could leave this out and ensure existing functionality continues to work by running the fs tests. I'm going to check around and see what others think about this.

Copy link
Member

@tasleson tasleson left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The changes look good, see comments for some changes. Also the commit 21998d6 can be removed if you want to force push a new PR which removes the changes introduced with c05a6c2.

I still need to run tests and that requires me to create a new VM which I'm in the process of doing.

@Queuecumber
Copy link
Contributor Author

Queuecumber commented Jul 2, 2019

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
Copy link
Member

tasleson commented Jul 3, 2019

@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
Copy link
Contributor Author

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

@Queuecumber Queuecumber force-pushed the modernize branch 2 times, most recently from 9c4fc1d to d14b931 Compare July 3, 2019 16:38
@Queuecumber
Copy link
Contributor Author

OK for real this time, squashed and ready to go

Copy link
Member

@tasleson tasleson left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmmm, not sure what happened to squash, but missing a couple changes.

README.md Outdated
@@ -59,7 +62,6 @@ an example:
target_name: iqn.2003-01.org.example.mach1:1234

block_pools: [vg-targetd/thin_pool, vg-targetd-too/thin_pool]
fs_pools: [/mnt/btrfs]
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please add this back :-)

targetd/block.py Outdated
output.append(dict(name=lv.lv_name, size=lv.size,
uuid=lv.uuid))
else:
if attrib[0] == 'V' and lv.lv_name == lv_pool:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should be:

if attrib[0] == 'V' and lv.pool_lv == lv_pool:

@Queuecumber
Copy link
Contributor Author

Oops I think I know what happened, fix incoming

@Queuecumber
Copy link
Contributor Author

OK this should be the correct changeset

@tasleson tasleson merged commit 1e59baa into open-iscsi:master Jul 3, 2019
@tasleson
Copy link
Member

tasleson commented Jul 3, 2019

@Queuecumber Thank you!

@Queuecumber Queuecumber deleted the modernize branch July 9, 2019 21:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants