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

osd: objclass sdk #14723

Merged
merged 2 commits into from May 3, 2017

Conversation

Projects
None yet
7 participants
@neha-ojha
Member

neha-ojha commented Apr 21, 2017

This pull request aims at creating an object class SDK interface for building Ceph object classes outside the tree. The cls_sdk object class makes use of this interface. It has unittests that can be run to verify correctness.

@liewegas liewegas added the core label Apr 24, 2017

@tchaikov

This comment has been minimized.

Contributor

tchaikov commented Apr 24, 2017

/home/jenkins-build/build/workspace/ceph-pull-requests/src/test/osd/osd-scrub-repair.sh:2214: corrupt_scrub_erasure:  test no = yes
/home/jenkins-build/build/workspace/ceph-pull-requests/src/test/osd/osd-scrub-repair.sh:2214: corrupt_scrub_erasure:  return 1
/home/jenkins-build/build/workspace/ceph-pull-requests/src/test/osd/osd-scrub-repair.sh:45: run:  return 1

https://jenkins.ceph.com/job/ceph-pull-requests/22697/

@neha-ojha i don't think the build failure is relevant to your change.

@tchaikov

This comment has been minimized.

Contributor

tchaikov commented Apr 24, 2017

retest this please.

@liewegas liewegas self-requested a review Apr 25, 2017

@liewegas liewegas changed the title from DNM: Wip Objclass-sdk to DNM: objclass sdk Apr 25, 2017

@liewegas

This comment has been minimized.

Member

liewegas commented Apr 25, 2017

Two questions:

  1. Eventually we should get everything out of objclass/objclass.h and into the rados/objclass.h, right? Is there any reason to hold off on that?
  2. I wonder if we should include zlog or not. Is it better to have a version in the tree?
@neha-ojha

This comment has been minimized.

Member

neha-ojha commented Apr 25, 2017

  1. Most parts of objclass/objclass.h can move into rados/objclass.h, except for the PGLSFilter class(requires changes in the osd). Moving some bits requires changes in objclass/class_api.cc.
    This initial version was created with the aim to keep the patch smaller, but I can incorporate the required changes and create another pull request.

  2. Not sure of about that. I added it here to demonstrate the use of the sdk.

@liewegas

This comment has been minimized.

Member

liewegas commented Apr 25, 2017

@liewegas liewegas added the needs-qa label Apr 25, 2017

#include <map>
#include <set>
#include "../include/buffer.h"

This comment has been minimized.

@jdurgin

jdurgin Apr 25, 2017

Member

one small thing: change this to #include "buffer.h" so it references the symlink in include/rados/buffer.h from the source tree, and the version installed by librados-dev when installed from packages (you've already got the dependency on that package so buffer.h will be installed)

This comment has been minimized.

@neha-ojha

neha-ojha Apr 25, 2017

Member

Sure, I will change it.

@jdurgin

This comment has been minimized.

Member

jdurgin commented Apr 25, 2017

great to see good docs and packaging included from the start! You can take DNM out of the commit message. Looks good to me beyond the small include change.

@neha-ojha

This comment has been minimized.

Member

neha-ojha commented Apr 25, 2017

@liewegas @jdurgin @tchaikov: Thanks for your feedback!

@liewegas liewegas changed the title from DNM: objclass sdk to librados: objclass sdk Apr 26, 2017

@liewegas liewegas changed the title from librados: objclass sdk to osd: objclass sdk Apr 26, 2017

@noahdesu

This comment has been minimized.

Member

noahdesu commented Apr 26, 2017

@neha-ojha cls_zlog in this PR is a little out-of-date. How do you want me to send you updates?

@liewegas

This comment has been minimized.

Member

liewegas commented Apr 26, 2017

@liewegas

This comment has been minimized.

Member

liewegas commented Apr 26, 2017

Checking for unpackaged file(s): /usr/lib/rpm/check-files /home/jenkins-build/build/workspace/ceph-dev-new-build/ARCH/x86_64/AVAILABLE_ARCH/x86_64/AVAILABLE_DIST/centos7/DIST/centos7/MACHINE_SIZE/huge/release/12.0.1-1548-g6b64459/rpm/el7/BUILDROOT/ceph-12.0.1-1548.g6b64459.el7.x86_64
error: Installed (but unpackaged) file(s) found:
   /usr/include/rados/objclass.h

during centos build

@liewegas

This comment has been minimized.

Member

liewegas commented Apr 26, 2017

looks like the %files section for the new package is missing from ceph.spec.in

@yuriw

This comment has been minimized.

Contributor

yuriw commented Apr 26, 2017

Per @liewegas removed as suspect breaking centos builds https://shaman.ceph.com/builds/ceph/wip-sage-testing_2017_4_27/

@yuriw yuriw removed the wip-sage-testing label Apr 26, 2017

@noahdesu

This comment has been minimized.

Member

noahdesu commented Apr 26, 2017

leaving cls_zlog out makes sense to me. I can provide feedback / patches as an SDK user. Some of the protocol buffer stuff seems to be a little slow, and that's an example of something that will be easier to handle maintaining it as an external sdk user, on a slower or faster pace than ceph releases.

@liewegas

This comment has been minimized.

Member

liewegas commented Apr 26, 2017

@neha-ojha

This comment has been minimized.

Member

neha-ojha commented Apr 26, 2017

Sure.

#ifdef __cplusplus
#include <map>
#include <set>

This comment has been minimized.

@tchaikov

tchaikov Apr 27, 2017

Contributor

why we need to include set and map here?

This comment has been minimized.

@neha-ojha

neha-ojha Apr 27, 2017

Member

Yes, they are not required now. I had included them for cls_cxx_getxattrs() and cls_cxx_map_get_keys(), which are no more a part of the api. I will remove them. Thanks for pointing that.

radostest
${UNITTEST_LIBS}
)
install(TARGETS

This comment has been minimized.

@tchaikov

tchaikov Apr 27, 2017

Contributor

why would we install this test? i believe it's but a unittest, and is not exercised by any qa suite, right?

This comment has been minimized.

@tchaikov

tchaikov Apr 27, 2017

Contributor

instead i'd recommend add it using add_ceph_unittest().

This comment has been minimized.

@neha-ojha

neha-ojha Apr 27, 2017

Member

I followed the CMakeLists.txt for unittests of existing object classes.

This comment has been minimized.

@tchaikov

tchaikov Apr 27, 2017

Contributor

@neha-ojha, ceph_test_cls_sdk is not a "cls", it's an executable. could you point me to a unittest of existing object class unittest which installs the executable?

This comment has been minimized.

@neha-ojha

neha-ojha Apr 27, 2017

Member

I mostly followed this https://github.com/ceph/ceph/blob/master/src/test/cls_hello/CMakeLists.txt#L15. But I can always incorporate your suggestion.

This comment has been minimized.

@tchaikov

tchaikov Apr 27, 2017

Contributor

@neha-ojha oic. it's up to you to run this test as part of unit test ("make check"), or exercise it using qa/workunits/cls/test_cls_hello.sh. i'd recommend the latter.

This comment has been minimized.

@neha-ojha

neha-ojha Apr 27, 2017

Member

@tchaikov I made the following changes and it works. Is there anything else that I would need to change?

  1. Removed the install section and added
    add_ceph_unittest(ceph_test_cls_sdk ${CMAKE_RUNTIME_OUTPUT_DIRECTORY}/ceph_test_cls_sdk) in CMakeLists.txt.

  2. Added qa/workunits/cls/test_cls_sdk.sh

This comment has been minimized.

@tchaikov

tchaikov Apr 27, 2017

Contributor

@neha-ojha you cannot do both. if you choose to run this test as part of "make check", then "add_ceph_unittest()", and there is no necessary to run it in qa/workunits/cls/test_cls_sdk.sh, but if you choose otherwise, you need to package this test by install(TARGET ...), so qa/workunits/cls/test_cls_sdk.sh can exercise it in a qa suite.

This comment has been minimized.

@neha-ojha

neha-ojha Apr 27, 2017

Member

Got it. I'll add qa/workunits/cls/test_cls_sdk.sh. Thanks!

@tchaikov

might want to use add_ceph_unittest() to add unittest.

@liewegas

This comment has been minimized.

Member

liewegas commented Apr 27, 2017

@neha-ojha

This comment has been minimized.

Member

neha-ojha commented Apr 27, 2017

yes.

@@ -718,6 +718,15 @@ This package contains the Java libraries for the Ceph File System.
%endif
%package -n rados-objclass-dev

This comment has been minimized.

@runsisi

runsisi Apr 27, 2017

Contributor

rados-objclass-devel ?

This comment has been minimized.

@neha-ojha
@liewegas

This comment has been minimized.

Member

liewegas commented Apr 27, 2017

objclass-sdk: create SDK for Ceph object classes
Creates an installable version of "src/include/rados/objclass.h" that allows
object classes to be built outside of the Ceph tree. cls_sdk is an example
of such an object class.

Signed-off-by: Neha Ojha <nojha@redhat.com>
@liewegas

This comment has been minimized.

Member

liewegas commented Apr 28, 2017

2017-04-28T22:01:36.530 INFO:tasks.workunit.client.0.smithi138.stdout:[==========] Running 2 tests from 1 test case.
2017-04-28T22:01:36.530 INFO:tasks.workunit.client.0.smithi138.stdout:[----------] Global test environment set-up.
2017-04-28T22:01:36.530 INFO:tasks.workunit.client.0.smithi138.stdout:[----------] 2 tests from ClsSDK
2017-04-28T22:01:36.530 INFO:tasks.workunit.client.0.smithi138.stdout:[ RUN      ] ClsSDK.TestSDKCoverageWrite
2017-04-28T22:01:38.682 INFO:tasks.workunit.client.0.smithi138.stdout:/home/jenkins-build/build/workspace/ceph-dev-new-build/ARCH/x86_64/AVAILABLE_ARCH/x86_64/AVAILABLE_DIST/centos7/DIST/centos7/MACHINE_SIZE/huge/release/12.0.1-1708-gfa34740/rpm/el7/BUILD/ceph-12.0.1-1708-gfa34740/src/test/cls_sdk/test_cls_sdk.cc:1
7: Failure
2017-04-28T22:01:38.683 INFO:tasks.workunit.client.0.smithi138.stdout:      Expected: 0
2017-04-28T22:01:38.683 INFO:tasks.workunit.client.0.smithi138.stdout:To be equal to: ioctx.exec("myobject", "sdk", "test_coverage_write", in, out)
2017-04-28T22:01:38.683 INFO:tasks.workunit.client.0.smithi138.stdout:      Which is: -1
2017-04-28T22:01:38.683 INFO:tasks.ceph.osd.1.smithi138.stderr:2017-04-28 22:01:38.686067 7f9c65131700 -1 osd.1 98 class sdk open got (1) Operation not permitted
2017-04-28T22:01:38.686 INFO:tasks.workunit.client.0.smithi138.stdout:[  FAILED  ] ClsSDK.TestSDKCoverageWrite (2156 ms)
2017-04-28T22:01:38.686 INFO:tasks.workunit.client.0.smithi138.stdout:[ RUN      ] ClsSDK.TestSDKCoverageReplay
2017-04-28T22:01:40.920 INFO:tasks.workunit.client.0.smithi138.stdout:/home/jenkins-build/build/workspace/ceph-dev-new-build/ARCH/x86_64/AVAILABLE_ARCH/x86_64/AVAILABLE_DIST/centos7/DIST/centos7/MACHINE_SIZE/huge/release/12.0.1-1708-gfa34740/rpm/el7/BUILD/ceph-12.0.1-1708-gfa34740/src/test/cls_sdk/test_cls_sdk.cc:3
0: Failure
2017-04-28T22:01:40.921 INFO:tasks.workunit.client.0.smithi138.stdout:      Expected: 0
2017-04-28T22:01:40.921 INFO:tasks.workunit.client.0.smithi138.stdout:To be equal to: ioctx.exec("myobject", "sdk", "test_coverage_write", in, out)
2017-04-28T22:01:40.921 INFO:tasks.workunit.client.0.smithi138.stdout:      Which is: -1
2017-04-28T22:01:40.921 INFO:tasks.ceph.osd.1.smithi138.stderr:2017-04-28 22:01:40.924241 7f9c65131700 -1 osd.1 100 class sdk open got (1) Operation not permitted
2017-04-28T22:01:40.923 INFO:tasks.workunit.client.0.smithi138.stdout:[  FAILED  ] ClsSDK.TestSDKCoverageReplay (2237 ms)
2017-04-28T22:01:40.923 INFO:tasks.workunit.client.0.smithi138.stdout:[----------] 2 tests from ClsSDK (4393 ms total)

/a/sage-2017-04-28_19:45:54-rados-wip-sage-testing---basic-smithi/1077690

I think we need to add sdk to the whitelist

@liewegas

This comment has been minimized.

Member

liewegas commented Apr 29, 2017

teuthology:1077690 12:44 AM $ cat summary.yaml
description: rados/basic/{clusters/{fixed-2.yaml openstack.yaml} mon_kv_backend/rocksdb.yaml
msgr-failures/few.yaml msgr/simple.yaml objectstore/bluestore-comp.yaml rados.yaml
tasks/rados_cls_all.yaml z-require-luminous/at-end.yaml}

so i would add an override to qa/suites/rados/basic/tasks/rados_cls_all.yaml with something like

override:
  ceph:
    conf:
      osd:
        ...

adding sdk to these options

OPTION(osd_class_load_list, OPT_STR, "cephfs hello journal lock log numops "
    "rbd refcount replica_log rgw statelog timeindex user version") // list of object classes allowed to be loaded (allow all: *)
OPTION(osd_class_default_list, OPT_STR, "cephfs hello journal lock log numops "
    "rbd refcount replica_log rgw statelog timeindex user version") // list of object classes with default execute perm (allow all: *)
objclass-sdk: add override to whitelist the sdk class
Signed-off-by: Neha Ojha <nojha@redhat.com>
@jdurgin

jdurgin approved these changes May 2, 2017

conf overrides look good

@jdurgin jdurgin added the needs-qa label May 2, 2017

@yuriw yuriw merged commit 1cce4c7 into ceph:master May 3, 2017

3 checks passed

Signed-off-by all commits in this PR are signed
Details
Unmodifed Submodules submodules for project are unmodified
Details
default Build finished.
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment