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

libcephfs/pybind: Add missing python bindings #33406

Merged
merged 14 commits into from Jun 12, 2020

Conversation

kotreshhr
Copy link
Contributor

@kotreshhr kotreshhr commented Feb 19, 2020

libcephfs/pybind: Add missing python bindings

Fixes: https://tracker.ceph.com/issues/44171
Signed-off-by: Kotresh HR khiremat@redhat.com

Checklist

  • References tracker ticket
  • Updates documentation if necessary
  • Includes tests for new functionality or reproducer for bug

Show available Jenkins commands
  • jenkins retest this please
  • jenkins test crimson perf
  • jenkins test signed
  • jenkins test make check
  • jenkins test make check arm64
  • jenkins test submodules
  • jenkins test dashboard
  • jenkins test dashboard backend
  • jenkins test docs
  • jenkins render docs
  • jenkins test ceph-volume all
  • jenkins test ceph-volume tox

@kotreshhr kotreshhr changed the title libcephfs/pybind: Add fchown, fchmod, lchown libcephfs/pybind: Add missing python bindings Feb 19, 2020
@kotreshhr kotreshhr force-pushed the libcephfs-pybind-1 branch 3 times, most recently from b7b3cfe to 8ea41ee Compare February 20, 2020 14:31
@varshar16
Copy link
Contributor

varshar16 commented Feb 25, 2020

Please add tests to src/test/pybind/test_cephfs.py. And in the commit message, change Updates: 44171 to Fixes: https://tracker.ceph.com/issues/44171.

@kotreshhr kotreshhr force-pushed the libcephfs-pybind-1 branch 5 times, most recently from 937f14c to 38969b9 Compare March 3, 2020 06:27
@kotreshhr
Copy link
Contributor Author

Rebased and added tests to few fops. I will add tests to remaining fops and refresh

@kotreshhr kotreshhr force-pushed the libcephfs-pybind-1 branch 2 times, most recently from fb056d4 to 7d7ab98 Compare March 5, 2020 21:01
@kotreshhr
Copy link
Contributor Author

Fixed pwritev/preadv issue

@kotreshhr kotreshhr force-pushed the libcephfs-pybind-1 branch 3 times, most recently from 075289b to ec37282 Compare March 11, 2020 04:38
@kotreshhr
Copy link
Contributor Author

Please add tests to src/test/pybind/test_cephfs.py. And in the commit message, change Updates: 44171 to Fixes: https://tracker.ceph.com/issues/44171.

Done

@kotreshhr
Copy link
Contributor Author

@gregsfortytwo could you assign someone to review this ?

@kotreshhr kotreshhr force-pushed the libcephfs-pybind-1 branch 3 times, most recently from 6388220 to a123c6f Compare April 14, 2020 11:40
@kotreshhr
Copy link
Contributor Author

@rishabh-d-dave please take a look at this patch when you find time.

Copy link
Contributor

@rishabh-d-dave rishabh-d-dave left a comment

Choose a reason for hiding this comment

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

Looks good to me. But I intend to take look again since the PR has plenty commits.

Copy link
Contributor

@rishabh-d-dave rishabh-d-dave left a comment

Choose a reason for hiding this comment

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

Except for few minor issue, looks good.

src/test/pybind/test_cephfs.py Outdated Show resolved Hide resolved
src/test/pybind/test_cephfs.py Show resolved Hide resolved
src/test/pybind/test_cephfs.py Show resolved Hide resolved
src/test/pybind/test_cephfs.py Outdated Show resolved Hide resolved
@rishabh-d-dave rishabh-d-dave self-requested a review May 5, 2020 13:27
@rishabh-d-dave
Copy link
Contributor

Also, there are no tests for start_reclaim() and finish_recalim().

@ukernel
Copy link
Contributor

ukernel commented May 6, 2020

Also, there are no tests for start_reclaim() and finish_recalim().

These function are for nfs-ganesha. maybe not export them to python bindings.

Fixes: https://tracker.ceph.com/issues/44171
Signed-off-by: Kotresh HR <khiremat@redhat.com>
Fixes: https://tracker.ceph.com/issues/44171
Signed-off-by: Kotresh HR <khiremat@redhat.com>
Fixes: https://tracker.ceph.com/issues/44171
Signed-off-by: Kotresh HR <khiremat@redhat.com>
@kotreshhr
Copy link
Contributor Author

Also, there are no tests for start_reclaim() and finish_recalim().

These function are for nfs-ganesha. maybe not export them to python bindings.

Done

assert('pool_id' in dp_dict.keys())
assert('pool_name' in dp_dict.keys())
assert_equal(cephfs.get_pool_id(dp_dict["pool_name"]), dp_dict["pool_id"])
get_rep_cnt_cmd = "ceph osd pool get " + dp_dict["pool_name"] + " size"
Copy link
Contributor

Choose a reason for hiding this comment

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

Just a note, tests in qa/ are written with the assumption that Ceph binaries are not accessible via $PATH. But I'm not sure if we should go that way here too.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@batrick Is that the case here too ?

Copy link
Member

Choose a reason for hiding this comment

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

Just a note, tests in qa/ are written with the assumption that Ceph binaries are not accessible via $PATH. But I'm not sure if we should go that way here too.

? The workunits like qa/workunits/fs/test_python.sh assume ceph utils are in $PATH?

Copy link
Contributor

Choose a reason for hiding this comment

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

AFAIS, test_python.sh run test_cephfs.py and test_cephfs.py doesn't use Ceph commands yet, so it's upto us to decide.

@batrick
Copy link
Member

batrick commented Jun 12, 2020

@batrick
Copy link
Member

batrick commented Jun 12, 2020

Great work, thanks @kotreshhr !

@batrick batrick merged commit 9c09027 into ceph:master Jun 12, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
6 participants