add timeout to watch/notify #11378

ryneli · 2016-10-08T14:39:19Z

Hi @tchaikov, I have added a timeout parameter to watch-related API. Could you please help me to review my code? Thanks.

tchaikov · 2016-10-09T04:27:24Z

src/librados/librados.cc

+extern "C" int rados_watch3(rados_ioctx_t io, const char *o, uint64_t *handle,
+			    rados_watchcb2_t watchcb,
+			    rados_watcherrcb_t watcherrcb,
+			    uint32_t timeout,
 			    void *arg)
 {
  tracepoint(librados, rados_watch2_enter, io, o, handle, watchcb, arg);


might want to s/rados_watch3_enter/rados_watch3_enter/,

and could you update src/tracing/librados.tp accordingly?

tchaikov · 2016-10-09T05:23:37Z

src/include/rados.h

@@ -517,6 +517,7 @@ struct ceph_osd_op {
 			__le64 ver;     /* no longer used */
 			__u8 op;	/* CEPH_OSD_WATCH_OP_* */
 			__u32 gen;      /* registration generation */
+			__u32 timeout; /* connection timeout */


i was wondering how we handled the backward/forward compatibility here. the op struct is wrapped in MOSDOp::ops::op. and ceph_osd_op is encoded using the raw encoder, i.e. just copy the memory chunk. so the maximum size of the member of union in ceph_osd_op is sizeof(writesame) = (64 + 64 + 64) bits. in other words, as long as the size of all struct are less or equal to 24 bytes, we are safe.

could you add a

static_assert(sizeof(ceph_osd_op) == (2+4+(8+8+8)+4), "sizeof(ceph_osd_op) breaks the compatibility");

somewhere in this header file in a separated commit? in which,

(2+4+(8+8+8)+4) = (sizeof(ceph_osd_op::op) + sizeof(ceph_osd_op::flags) + sizeof(ceph_osd_op::writesame) + sizeof(ceph_osd_op::payload_len))

@athanatos am i right?

@tchaikov, you are right. There is compile error after adding the static check. Do I need to find another place to pass timeout to OSD?

Following is the compile error.

In file included from /home/ryne/ceph/src/include/ceph_fs.h:16:0,
from /home/ryne/ceph/src/include/types.h:32,
from /home/ryne/ceph/src/msg/msg_types.h:20,
from /home/ryne/ceph/src/common/entity_name.h:24,
from /home/ryne/ceph/src/common/config.h:24,
from /home/ryne/ceph/src/common/dout.h:20,
from /home/ryne/ceph/src/compressor/AsyncCompressor.cc:15:
/home/ryne/ceph/src/include/rados.h:573:1: error: static assertion failed: sizeof(ceph_osd_op) breaks the compatibility
static_assert(sizeof(ceph_osd_op) == (2+4+(8+8+8)+4),

@ryneli i was wrong the largest struct in the union is

struct { int64_t offset, length; int64_t truncate_size; int32_t truncate_seq; } __attribute__ ((packed)) extent;

so the assert should be

static_assert(sizeof(ceph_osd_op) == (2+4+(2*8+8+4)+4),

@tchaikov I have finished these changes. Could you please help to review my code again? Also, this static_assert involves some magic number "2+4+(2*8+8+4)+4". Do I need to add a global constant value to represent this magic number, i.e. "const uint32_t MAX_BYTES_OF_CEPH_OSD_OP = 38;"

tchaikov · 2016-10-09T05:49:20Z

src/osd/ReplicatedPG.cc

-        uint64_t cookie = op.watch.cookie;
-        entity_name_t entity = ctx->reqid.name;
+	uint64_t cookie = op.watch.cookie;
+	entity_name_t entity = ctx->reqid.name;


better off not s/<tab>/ /, if you are not touching this area.

The original code seems not indent correctly. So I try to modify the indent to align with the code below.

@ryneli we use tabstop = 8, so it's okay. and we want to avoid formatting only changes if possible. otherwise it would be more difficult to read the output of git blame.

tchaikov · 2016-10-10T07:28:32Z

src/librados/librados.cc

+                                uint64_t *handle,
+                                rados_watchcb2_t watchcb,
+                                rados_watcherrcb_t watcherrcb,
+                                uint32_t timeout, void *arg)
 {
  tracepoint(librados, rados_aio_watch_enter, io, o, completion, handle, watchcb, arg);


now that rados_aio_watch2() is delegated to do the work. we can probably remove rados_aio_watch_enter and rados_aio_watch_exit tracing points, and add rados_aio_watch2_enter and rados_aio_watch2_exit to rados_aio_watch2().

tchaikov · 2016-10-10T07:37:14Z

src/include/rados/librados.h

+ * @param completion what to do when operation has been attempted
+ * @param handle where to store the internal id assigned to this watch
+ * @param watchcb what to do when a notify is received on this object
+ * @param watcherrcb what to do when the watch session encounters an error


timeout is not documented.

tchaikov · 2016-10-10T07:55:25Z

src/include/rados.h

+ *                     sizeof(ceph_osd_op::payload_len))
+ */
+#ifdef __cplusplus
+static_assert(sizeof(ceph_osd_op) == (2+4+(2*8+8+4)+4),


Also, this static_assert involves some magic number "2+4+(2*8+8+4)+4". Do I need to add a global constant value to represent this magic number, i.e. "const uint32_t MAX_BYTES_OF_CEPH_OSD_OP = 38;"

ahh, i just realized that rados.h is shared by C and C++ source files! MAX_BYTES_OF_CEPH_OSD_OP does not seem right. the size of ceph_osd_op must be exactly 38 bytes. and we can only add more fields at the end of union structs (extent excluded), and the new behavior should be backward compatible if the newly added field(s) reads zero. this static assert is just a better-than-nothing message to posterity.

but i think the comment is good enough to serve this purpose. what do you think?

tchaikov · 2016-10-10T08:02:17Z

src/include/rados/librados.h

- * the normal recovery process. If the client loses its connection to
- * the primary OSD for a watched object, the watch will be removed
- * after 30 seconds. Watches are automatically reestablished when a new
+ * the normal recovery process. The timeout of the connection between


can we keep the original doc which is more specific? and amend it like:

If the client loses its connection to the primary OSD for a watched object, the watch will be removed after a timeout configured with osd_client_watch_timeout.

?

tchaikov · 2016-10-10T08:09:21Z

also, probably you can split the first commit into two separated ones. one is the changes in osd. the other is for exposing the new feature in librados. and then prefix the title of your commit message withs the subcomponent your are changing ? see https://github.com/ceph/ceph/blob/master/SubmittingPatches.rst#preparing-and-sending-patches

ryneli · 2016-10-10T20:33:52Z

@tchaikov, I have split the commits and add prefix to my commit message. Is there anything else to modify?

tchaikov · 2016-10-11T08:08:38Z

src/tracing/librados.tp

+        ctf_integer(uint64_t, handle, handle)
+    )
+)
+
 TRACEPOINT_EVENT(librados, rados_aio_watch_enter,


so we can remove rados_watch2_enter and rados_watch2_exit now?

I have removed both rados_watch2_enter/exit and rados_aio_watch_enter/exit :)

Signed-off-by: Ryne Li <lizhenqiangsnake@gmail.com>

tchaikov · 2016-10-12T02:39:22Z

~~lgtm. adding needs-qa label.~~

ryneli · 2016-10-12T11:23:27Z

So could I merge it to the master branch now?

tchaikov · 2016-10-12T11:32:01Z

@ryneli, for most non-trivial changes, we'd like to run them through a qa suite before merging them. and in your case, i'd like to run a rados qa suite. that's what the "needs-qa" label is for.

ryneli · 2016-10-12T11:34:03Z

So could I do something in the QA test or just wait the result?

tchaikov · 2016-10-12T11:38:51Z

i think your change is already covered by existing tests. you can push a branch with your changes to ceph/ceph, and schedule a qa run with enough coverage in sepia after your branch is built. or just wait. normally, it will take 1 week or two.

ryneli · 2016-10-12T11:43:19Z

Since this is not an emergent change. Maybe I can just wait for the coverage test. Thanks for your review and comments, @tchaikov

tchaikov · 2016-10-28T05:38:21Z

src/tracing/librados.tp

    TP_ARGS(
        rados_ioctx_t, ioctx,
        const char*, oid,
        rados_completion_t, completion,
        uint64_t*, phandle,
        rados_watchcb2_t, callback,
+        uint32_t timeout,


@ryneli you missed a ,. see http://gitbuilder.sepia.ceph.com/gitbuilder-ceph-deb-trusty-amd64-notcmalloc/log.cgi?log=29c70f92ec89d00399952d13145c9fe55daa7a01.

@ryneli ping?

liewegas · 2016-11-04T18:05:03Z

@ryneli ping

ryneli · 2016-11-06T22:30:03Z

@liewegas I am so sorry for the delay. I didn't receive the message. Just find the message when I check the status of this PR.

ryneli · 2016-11-08T01:38:03Z

@tchaikov I am so sorry for the delay. Due to wrong Github setting, I didn't receive your message. I have corrected the error. Could you please help me check it again?

tchaikov · 2016-11-08T02:46:47Z

@ryneli sure, will do.

tchaikov · 2016-11-10T12:09:27Z

lgtm.

tchaikov · 2016-11-11T01:45:23Z

jenkins, retest this please

tchaikov · 2016-11-11T04:57:08Z

src/tracing/librados.tp

        void*, arg),
    TP_FIELDS(
        ctf_integer_hex(rados_ioctx_t, ioctx, ioctx)
        ctf_string(oid, oid)
        ctf_integer_hex(rados_completion_t, completion, completion)
        ctf_integer_hex(uint64_t, phandle, phandle)
        ctf_integer_hex(rados_watchcb2_t, callback, callback)
+        ctf_inteter(uint32_t, timeout, timeout)


s/ctf_inteter/ctf_integer/

Sorry for the stupid typo. Do you know how to test librados.tp file? It seems that the librados.tp file isn't be used when I compile the source code locally.

@ryneli

cmake -DWITH_LTTNG=ON .. make -C src/tracing rados_tp

tchaikov · 2016-11-13T06:31:24Z

src/test/librados/watch_notify.cc

+  notify_oid = "foo";
+  notify_ioctx = &ioctx;
+  notify_cookies.clear();
+  uint32_t timeout = 3; // configured timeout


http://pulpito.ceph.com/kchai-2016-11-13_05:09:22-rados-wip-kefu-testing---basic-smithi/

quite a few tests with harsh cluster env (high messenger failure rate or thrash interruptions) failed due to -ENOTCONN, i am prolonging the timeout to 12 to see if it helps.

the tests at http://pulpito.ceph.com/kchai-2016-11-13_07:03:13-rados-wip-kefu-testing---basic-smithi/ look promising.

@ryneli could you change the timeout to 12 or longer?

tchaikov · 2016-11-13T07:27:27Z

src/test/librados/watch_notify.cc

+  notify_oid = "foo";
+  notify_ioctx = &ioctx;
+  notify_cookies.clear();
+  uint32_t timeout = 3; // configured timeout


the tests at http://pulpito.ceph.com/kchai-2016-11-13_07:03:13-rados-wip-kefu-testing---basic-smithi/ look promising.

@ryneli could you change the timeout to 12 or longer?

Signed-off-by: Ryne Li <lizhenqiangsnake@gmail.com>

ryneli · 2016-11-13T22:15:12Z

@tchaikov I have changed the timeout to 12.

ryneli force-pushed the watch_notify_add_timeout branch from b6c9dfe to ec422d7 Compare October 8, 2016 14:41

tchaikov self-assigned this Oct 8, 2016

tchaikov requested changes Oct 9, 2016

View reviewed changes

ryneli force-pushed the watch_notify_add_timeout branch 4 times, most recently from 250f9c4 to fae542f Compare October 9, 2016 19:35

tchaikov requested changes Oct 10, 2016

View reviewed changes

tchaikov added cephfs Ceph File System feature labels Oct 10, 2016

tchaikov added core and removed cephfs Ceph File System labels Oct 10, 2016

ryneli force-pushed the watch_notify_add_timeout branch 3 times, most recently from 1bac9f2 to ed178cc Compare October 10, 2016 20:00

tchaikov requested changes Oct 11, 2016

View reviewed changes

ryneli added 2 commits October 11, 2016 16:22

osd: add check of ceph_osd_op for compatibility

037014c

Signed-off-by: Ryne Li <lizhenqiangsnake@gmail.com>

osd: add comfigurable timeout for OSD_OP_WATCH

6fef17b

Signed-off-by: Ryne Li <lizhenqiangsnake@gmail.com>

ryneli force-pushed the watch_notify_add_timeout branch from ed178cc to e467337 Compare October 11, 2016 20:25

tchaikov approved these changes Oct 12, 2016

View reviewed changes

tchaikov added the needs-qa label Oct 12, 2016

liewegas added the wip-sage-testing label Oct 19, 2016

tchaikov requested changes Oct 28, 2016

View reviewed changes

tchaikov removed the needs-qa label Oct 28, 2016

tchaikov assigned ryneli Oct 28, 2016

liewegas removed the wip-sage-testing label Nov 1, 2016

liewegas added this to the kraken milestone Nov 1, 2016

ryneli force-pushed the watch_notify_add_timeout branch from e467337 to a0e0ece Compare November 6, 2016 22:27

ryneli force-pushed the watch_notify_add_timeout branch from a0e0ece to 7a344ef Compare November 6, 2016 22:42

tchaikov added the needs-qa label Nov 10, 2016

tchaikov approved these changes Nov 10, 2016

View reviewed changes

tchaikov added the wip-kefu-testing label Nov 10, 2016

tchaikov requested changes Nov 11, 2016

View reviewed changes

tchaikov removed needs-qa wip-kefu-testing labels Nov 11, 2016

ryneli force-pushed the watch_notify_add_timeout branch from 7a344ef to 4e23511 Compare November 11, 2016 05:09

tchaikov added needs-qa wip-kefu-testing labels Nov 11, 2016

tchaikov approved these changes Nov 11, 2016

View reviewed changes

tchaikov reviewed Nov 13, 2016

View reviewed changes

tchaikov requested changes Nov 13, 2016

View reviewed changes

librados: add timeout parameter to rados_watch

e3ca1e6

Signed-off-by: Ryne Li <lizhenqiangsnake@gmail.com>

ryneli force-pushed the watch_notify_add_timeout branch from 4e23511 to e3ca1e6 Compare November 13, 2016 12:22

tchaikov approved these changes Nov 14, 2016

View reviewed changes

tchaikov merged commit 473ae1d into ceph:master Nov 14, 2016

add timeout to watch/notify #11378

add timeout to watch/notify #11378

Conversation

ryneli commented Oct 8, 2016

Choose a reason for hiding this comment

tchaikov Oct 9, 2016 • 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

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

tchaikov Oct 10, 2016 • edited

Choose a reason for hiding this comment

tchaikov commented Oct 10, 2016

ryneli commented Oct 10, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tchaikov commented Oct 12, 2016 • edited

ryneli commented Oct 12, 2016

tchaikov commented Oct 12, 2016

ryneli commented Oct 12, 2016

tchaikov commented Oct 12, 2016

ryneli commented Oct 12, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

liewegas commented Nov 4, 2016

ryneli commented Nov 6, 2016

ryneli commented Nov 8, 2016

tchaikov commented Nov 8, 2016

tchaikov commented Nov 10, 2016

tchaikov commented Nov 11, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tchaikov Nov 11, 2016 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tchaikov Nov 13, 2016 • edited

Choose a reason for hiding this comment

tchaikov Nov 13, 2016 • edited

Choose a reason for hiding this comment

ryneli commented Nov 13, 2016

tchaikov Oct 9, 2016 •

edited

tchaikov Oct 10, 2016 •

edited

tchaikov commented Oct 12, 2016 •

edited

tchaikov Nov 11, 2016 •

edited

tchaikov Nov 13, 2016 •

edited

tchaikov Nov 13, 2016 •

edited