-
Notifications
You must be signed in to change notification settings - Fork 28
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
Conversation
2910374
to
a4ad602
Compare
This now passes the "automated testing" in client.py so I'm going to start working with real applications |
I performed the following practical test
Everything worked great so I think this is gtg. Awaiting feedback from @tasleson to proceed. |
@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 |
There was a problem hiding this comment.
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.') | |||
|
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 |
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
Once you make these 2 changes please go ahead and squash and push PR and we can then get this merged, thanks! |
@tasleson I think I had misunderstood that line when I originally wrote the code. Both fixed. |
9c4fc1d
to
d14b931
Compare
OK for real this time, squashed and ready to go |
There was a problem hiding this 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] |
There was a problem hiding this comment.
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: |
There was a problem hiding this comment.
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:
Oops I think I know what happened, fix incoming |
OK this should be the correct changeset |
@Queuecumber Thank you! |
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 andshould be held pendingfurther testing, feedback, and eventual squashing of the commits.