libradosstriper: conditional compile #18197

chardan · 2017-10-09T17:32:17Z

This PR affects the "rados" tool:

enables support for libradosstriper features by default (unchanged from current feature);
adds a new do_cmake.sh and cmake switch called "WITH_LIBRADOSSTRIPER", set to ON by default;
if [2] is enabled, libradosstriper features are enabled in the "rados" tool. If disabled, libradosstriper features are disabled in the "rados" tool.
modifies the RPM spec file to set WITH_LIBRADOSSTRIPER as needed

Please note that this PR is not meant to affect whether libradosstriper itself is built or linked, it is limited to governing the exposed features in the tool.

Fixes: https://tracker.ceph.com/issues/22750

tchaikov · 2017-10-10T02:12:57Z

@chardan could you offer more context in the commit message, w.r.t. why we shall disable libradosstriper by default?

smithfarm · 2017-10-10T05:36:47Z

CMakeLists.txt

@@ -614,3 +614,6 @@ add_tags(ctags
  EXCLUDE_OPTS ${CTAG_EXCLUDES}
  EXCLUDES "*.js" "*.css")
 add_custom_target(tags DEPENDS ctags)
+
+option(WITH_LIBRADOSSTRIPER "Build with libradosstriper support." OFF)


Wouldn't this change the current upstream behavior?

Yes-- looks like I thought the opposite of what we actually want-- fixed.

smithfarm · 2017-10-10T05:37:30Z

src/tools/CMakeLists.txt

@@ -104,3 +111,5 @@ if(WITH_RBD)
    add_subdirectory(rbd_ggate)
  endif()
 endif(WITH_RBD)
+
+


Adding gratuitous whitespace at end of file?

smithfarm · 2017-10-10T05:40:04Z

src/tools/rados/rados.cc

@@ -3661,8 +3925,10 @@ int main(int argc, const char **argv)
      opts["target_locator"] = val;
    } else if (ceph_argparse_witharg(args, i, &val, "--target-nspace" , (char *)NULL)) {
      opts["target_nspace"] = val;
+#ifdef USE_LIBRADSSTRIPER


typo? Should it be #ifdef WITH_LIBRADOSSTRIPER here?

Good catch, thank you!

chardan · 2017-10-10T17:50:02Z

@smithfarm Thanks for your comments! Updated.
@tchaikov Notes updated (and reflect the correct behavior now). Thanks!

chardan · 2017-10-10T17:53:29Z

I'll point out a little implementation oddity, which is that there are some nested conditionals. This is technically redundant, but I'm ok leaving them there unless there are objections. (This is an ugly thing to do, I'm ok with it having an ugly, attention-getting implementation. Another, neater way to go about this is with the detail idiom, but I'd rather give all of the code TLC if we go that route.)

tchaikov · 2017-10-10T18:01:27Z

src/tools/rados/rados.cc

@@ -2405,11 +2653,17 @@ static int rados_tool_common(const std::map < std::string, std::string > &opts,
    string oid(nargs[1]);
    map<std::string, bufferlist> attrset;
    bufferlist bl;
+


@chardan i'd suggest use following pattern to avoid repeating.

#ifdef WITH_LIBRADOSSTRIPER if (use_striper) { ret = striper.getxattrs(oid, attrset); } else #endif { ret = io_ctx.getxattrs(oid, attrset); }

I considered that, but it risks introducing subtly different behavior in that the current implementation has both the presence of a striper object /and/ a runtime flag. That said, the flag will always be set to true if creating the striper succeeds. I'd rather see all the plumbing in here get reworked...

tchaikov · 2017-10-10T18:02:10Z

src/tools/rados/rados.cc

@@ -373,6 +428,69 @@ static int do_copy_pool(Rados& rados, const char *src_pool, const char *target_p
  return 0;
 }

+#ifndef USE_LIBIRADOSSTRIPER


where is USE_LIBIRADOSSTRIPER defined?

It is not, that's at typo. Good catch, thank you!

tchaikov · 2017-10-10T18:03:29Z

src/tools/CMakeLists.txt

@@ -9,6 +9,11 @@ add_executable(rados ${rados_srcs})
 target_link_libraries(rados librados global ${BLKID_LIBRARIES} ${CMAKE_DL_LIBS} radosstriper)
 install(TARGETS rados DESTINATION bin)

+if(WITH_LIBRADOSSTRIPER)


could you avoid adding options to the command line, but instead define this in src/include/config-h.in.cmake?

I'm not completely clear on how cmakedefine works. If I opt for that, are all of those variables still things that can be changed from the command-line/do_cmake?

(I will RTFM in the meantime.) It /looks/ like this isn't quite what we want. That lets you change the behavior by editing that file, but we want to be able to do it from the command-line via do_cmake.
@smithfarm Is that right? Would it be equally satisfactory to make this something we change in config-h.in.cmake?

@tchaikov The idea was that we could control striper on/off from a %bcond switch in the spec file.

@chardan git grep -w WITH_BLKIN for an example.

chardan · 2017-10-19T00:00:14Z

@smithfarm / @tchaikov Okay, I think this should work! Please have a look.

smithfarm · 2018-01-10T11:57:23Z

@chardan Please rebase?

smithfarm · 2018-01-10T12:00:13Z

@chardan Also, please change the title of the 3rd commit to something other than ("compile fix"). There are some basic guidelines at https://github.com/ceph/ceph/blob/master/SubmittingPatches.rst#title-of-pull-requests-and-title-of-commits

chardan · 2018-01-22T08:26:22Z

Jenkins retest this please.

smithfarm · 2018-01-22T12:51:33Z

CMakeLists.txt

@@ -389,6 +389,13 @@ if(WITH_DPDK)
  endif()
 endif()

+option(WITH_LIBRADOSSTRIPER "Build with libradosstriper support." ON)
+if(WITH_LIBRADOSSTRIPER)
+  message(STATUS "Enabling libradosstriper in \"rados\" tool.")


@chardan Could you add "support" after "libradosstriper" in this line?

I sure can!

This doesn’t appear to have happened.

Ah, now I see :-) @chardan Please add the word "support" as requested above

Yeah, the threading display on my review is a bit weird.

I’m indifferent to the wording but noticed it was discussed and not done. :)

smithfarm · 2018-01-22T12:52:08Z

src/include/config-h.in.cmake

@@ -336,4 +335,7 @@
 /* Defined if boost::context is available */
 #cmakedefine HAVE_BOOST_CONTEXT

+/* Define if libradosstriper is enabled: */


@chardan change "Define" to "Defined" here?

The style in the file isn't entirely consistent-- but I'll go with "defined".

This doesn’t appear to have happened.

smithfarm · 2018-01-22T13:01:15Z

src/tools/rados/rados.cc

@@ -302,11 +350,17 @@ static int do_get(IoCtx& io_ctx, RadosStriper& striper,
  int ret;
  while (true) {
    bufferlist outdata;
+
+#ifndef WITH_LIBRADOSSTRIPER
+    ret = io_ctx.read(oid, outdata, op_size, offset);


@chardan since this is inside the #else // WITH_LIBRADOSSTRIPER, the #ifndef WITH_LIBRADOSSTRIPER will never be true and the preprocessor will always remove it, right?

True, please refer to my note:
#18197 (comment)

I can remove them if you feel strongly about it (I agree it is gnarly and potentially confusing).

I don’t understand this even after reading that note. This is a separate striper-aware compilation path; why would we add an implementation that doesn’t contain the striper?

smithfarm · 2018-01-23T22:24:06Z

removed my testing label because this cannot be tested at the same time as #19122

chardan · 2018-02-05T10:10:49Z

@tchaikov How is this looking to you?

Signed-off-by: Jesse Williamson <jwilliamson@suse.de>

… features Signed-off-by: Jesse Williamson <jwilliamson@suse.de>

Enable downstreams (such as SLE) to not ship libradosstriper. Fixes: https://tracker.ceph.com/issues/22750 Signed-off-by: Nathan Cutler <ncutler@suse.com>

Signed-off-by: Jesse Williamson <jwilliamson@suse.de>

chardan · 2018-02-06T08:35:05Z

Jenkins retest this please.

smithfarm · 2018-02-27T13:05:45Z

http://pulpito.ceph.com/smithfarm-2018-02-21_22:45:50-rados-wip-smithfarm-testing-distro-basic-smithi/
http://pulpito.ceph.com/smithfarm-2018-02-22_15:48:12-rados-wip-smithfarm-testing-distro-basic-smithi/
http://pulpito.ceph.com/smithfarm-2018-02-27_12:29:57-rados-wip-smithfarm-testing-distro-basic-smithi/

Summary: rados suite passes with one failure in rados/standalone/scrub.yaml - not clear to me why this is failing

smithfarm · 2018-02-27T13:06:49Z

@gregsfortytwo This has passed a rados suite (with one failure - see the previous comment). Can you review?

gregsfortytwo

So, I guess this works. But I note two problems:

it appears that users who attempt to invoke the striper when it has been disabled do not get an error message (because it still parses the flag), but merely a silent different behavior.
we’ve duplicated all our IO function implementations. We’re going to get bugs as they diverge.

To avoid that, I’d suggest doing something like what we do with tcmalloc functionality. Add in a stub striper interface implementation that errors out whenever invoked, and if the striper is compiled out we construct one of those stub failure objects instead. Then in rados.cc we can (depending on implementation choices) either rely on that stub to inform the user they’ve messed up, or else bail out when they specify the striper and actually throw an assertion failure in the stub,

gregsfortytwo · 2018-03-03T00:37:16Z

src/tools/rados/rados.cc

@@ -302,11 +350,17 @@ static int do_get(IoCtx& io_ctx, RadosStriper& striper,
  int ret;
  while (true) {
    bufferlist outdata;
+
+#ifndef WITH_LIBRADOSSTRIPER
+    ret = io_ctx.read(oid, outdata, op_size, offset);


I don’t understand this even after reading that note. This is a separate striper-aware compilation path; why would we add an implementation that doesn’t contain the striper?

gregsfortytwo · 2018-03-03T00:44:36Z

CMakeLists.txt

@@ -389,6 +389,13 @@ if(WITH_DPDK)
  endif()
 endif()

+option(WITH_LIBRADOSSTRIPER "Build with libradosstriper support." ON)
+if(WITH_LIBRADOSSTRIPER)
+  message(STATUS "Enabling libradosstriper in \"rados\" tool.")


This doesn’t appear to have happened.

gregsfortytwo · 2018-03-03T00:44:42Z

src/include/config-h.in.cmake

@@ -336,4 +335,7 @@
 /* Defined if boost::context is available */
 #cmakedefine HAVE_BOOST_CONTEXT

+/* Define if libradosstriper is enabled: */


This doesn’t appear to have happened.

smithfarm · 2018-03-03T06:30:51Z

@gregsfortytwo Regarding the status message in CMakeLists.txt, what do you mean by "This doesn't appear to have happened"? I see the status message in the build log:

-- Enabling libradosstriper in "rados" tool.

in light of Greg's review

chardan · 2018-03-03T11:38:07Z

Hi Greg, thanks for the useful feedback! I entirely agree that a stub implementation would be much tidier-- that's what I was suggesting in my note with "detail" implementations. I'm happy to do that, and while I'm at it have a look at some of the surrounding code. In any case, there's now officially enough confusion from the direction taken that it seems best to change it-- I'll look into this further next week. Thanks!

chardan · 2018-05-14T17:54:38Z

Let's try this again.
#21983

smithfarm reviewed Oct 10, 2017

View reviewed changes

chardan force-pushed the jfw-wip-libradosstriper-conditional-compile branch from 866f42c to 91b70da Compare October 10, 2017 17:47

chardan force-pushed the jfw-wip-libradosstriper-conditional-compile branch 2 times, most recently from 323c860 to cb83566 Compare October 10, 2017 17:52

tchaikov reviewed Oct 10, 2017

View reviewed changes

chardan force-pushed the jfw-wip-libradosstriper-conditional-compile branch 4 times, most recently from 4b30d24 to 121ec3c Compare October 16, 2017 20:29

chardan force-pushed the jfw-wip-libradosstriper-conditional-compile branch 2 times, most recently from 880788a to 9aa4e32 Compare October 17, 2017 23:00

liewegas changed the title ~~jfw-wip-libradosstriper_conditional_compile~~ libradosstriper: conditional compile Nov 16, 2017

liewegas added cleanup common labels Nov 16, 2017

chardan force-pushed the jfw-wip-libradosstriper-conditional-compile branch from 9aa4e32 to 32e56d7 Compare January 10, 2018 19:54

chardan force-pushed the jfw-wip-libradosstriper-conditional-compile branch from 32e56d7 to 5c00321 Compare January 22, 2018 09:32

smithfarm reviewed Jan 22, 2018

View reviewed changes

smithfarm removed the wip-smithfarm-testing label Jan 23, 2018

chardan force-pushed the jfw-wip-libradosstriper-conditional-compile branch 3 times, most recently from f512cb9 to 9281cfb Compare January 30, 2018 13:31

smithfarm added the needs-review label Feb 2, 2018

smithfarm previously approved these changes Feb 2, 2018

View reviewed changes

smithfarm added the needs-qa label Feb 2, 2018

rados: conditionally-compile libradosstriper features

df86939

Signed-off-by: Jesse Williamson <jwilliamson@suse.de>

chardan force-pushed the jfw-wip-libradosstriper-conditional-compile branch 3 times, most recently from 016252c to 6b2aaef Compare February 5, 2018 12:01

Jesse Williamson and others added 3 commits February 5, 2018 04:01

rados: update build system for conditional compile of libradosstriper…

6fc27b4

… features Signed-off-by: Jesse Williamson <jwilliamson@suse.de>

build/ops: rpm: conditionalize libradosstriper

0e1f269

Enable downstreams (such as SLE) to not ship libradosstriper. Fixes: https://tracker.ceph.com/issues/22750 Signed-off-by: Nathan Cutler <ncutler@suse.com>

test: conditionally-compile libradosstriper tests.

6b2aaef

Signed-off-by: Jesse Williamson <jwilliamson@suse.de>

smithfarm added the wip-smithfarm-testing label Feb 5, 2018

smithfarm requested a review from gregsfortytwo February 27, 2018 13:06

smithfarm removed the needs-qa label Feb 27, 2018

gregsfortytwo requested changes Mar 3, 2018

View reviewed changes

smithfarm removed the wip-smithfarm-testing label Mar 3, 2018

chardan mentioned this pull request May 14, 2018

libradosstriper: conditional compile #21983

Merged

chardan closed this May 14, 2018

libradosstriper: conditional compile #18197

libradosstriper: conditional compile #18197

Conversation

chardan commented Oct 9, 2017 • edited by smithfarm

tchaikov commented Oct 10, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chardan commented Oct 10, 2017

chardan commented Oct 10, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tchaikov Oct 10, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chardan Oct 10, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tchaikov Oct 11, 2017 • edited

Choose a reason for hiding this comment

chardan commented Oct 19, 2017

smithfarm commented Jan 10, 2018

smithfarm commented Jan 10, 2018

chardan commented Jan 22, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

smithfarm commented Jan 23, 2018

chardan commented Feb 5, 2018

chardan commented Feb 6, 2018

smithfarm commented Feb 27, 2018

smithfarm commented Feb 27, 2018

gregsfortytwo left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

smithfarm commented Mar 3, 2018 • edited

chardan commented Mar 3, 2018

chardan commented May 14, 2018

chardan commented Oct 9, 2017 •

edited by smithfarm

chardan commented Oct 10, 2017 •

edited

tchaikov Oct 10, 2017 •

edited

chardan Oct 10, 2017 •

edited

tchaikov Oct 11, 2017 •

edited

smithfarm commented Mar 3, 2018 •

edited