Fix: relayd: viewer metadata is not rotated after a session clear
authorJérémie Galarneau <jeremie.galarneau@efficios.com>
Mon, 13 Jul 2020 22:42:22 +0000 (18:42 -0400)
committerJérémie Galarneau <jeremie.galarneau@efficios.com>
Fri, 17 Jul 2020 18:31:55 +0000 (14:31 -0400)
commitde755d0b2828d1104e7ae700113bc29dc945f028
tree53fe9f2c1b3c335aab362104d92e6e6d6e537d27
parentfc0c52e9fb0608ede702556178a68d15a1d25af8
Fix: relayd: viewer metadata is not rotated after a session clear

Issue observed
==============

Following a session clear, babeltrace sometimes doesn't receive
the content of the metadata that is announced in the get_metadata
reply header.

This causes babeltrace2 to assert (a fix has been submitted) since
the protocol state becomes de-synchronized, causing babeltrace to
interpret follow-up replies as garbage.

This was occasionally observed on the CI when running the "clear" tests.

Cause
=====

There are no provisions made for rotating a viewer metadata stream when
a clear is performed on a live session. This means that a client can
request metadata that is only present in a newer chunk.

In the case of the crashes observed on the CI, the relay daemon attempts
to service a get_new_metadata request of size `4096`. It then fails to
read the data (as it was never present in the original trace chunk).

The relay daemon does not interpret the `0` returned by lttng_read() as
an error and sends a reply announcing `4096`bytes of metadata and no
payload.

Solution
========

Two fixes are rolled into this patch.

First, the return of lttng_read is checked for `0` and that situation is
handled as an error. However, this still leaves the problem of the
metadata stream not being rotated.

Secondly, the metadata relay_stream is checked for a rotation on every
`get_metadata` command. If a rotation has been detected, a viewer
rotation is performed on the metadata stream (very similar to the data
stream).

This solves the problem, but it leaves a case which the protocol does
not account for.

Essentially, the following can take place:
  - relayd sets the "NEW_METADATA" flag as part of a `get_next_index`
    query reply
  - A rotation of the metadata stream occurs, no data is sent.
  - client requests metadata
    - metadata sent > received (was reset to 0 as part of the rotation)

In this scenario, the current implementation returned NO_NEW_METADATA,
but it is erroneous. Returning this guarantees to the viewer that it
will be able to decode all data packets that follow (until new metadata
is signalled, if ever).

Ideally, we would return a `RETRY` code, as is done by the data stream
handler when it detects that a rotation is taking place. Unfortunately,
such a code doesn't exist for the `get_metadata` command.

We return ̀ OK` with a length of 0, which is technically correct since
viewers are supposed to fetch metadata until the relay daemon returns
the `NO_NEW_METADATA` status code. However, supporting this has required
changes to babeltrace2's lttng-live source component.

I'm anticipating that most implementations don't handle the 0-length
case any better.

Known drawbacks
===============

Older viewers may not handle `OK` replies with a length of 0 gracefully.

Sending `NO_NEW_METADATA` is not an option as it breaks the guarantee
that all necessary metadata will be received before `NO_NEW_METADATA`.

Signed-off-by: Jérémie Galarneau <jeremie.galarneau@efficios.com>
Change-Id: I99cc1cea15125b48f5ee4e407d782b34b5e3dfb5
src/bin/lttng-relayd/live.c
src/bin/lttng-relayd/viewer-session.c
src/common/trace-chunk.c
src/common/trace-chunk.h
This page took 0.027966 seconds and 4 git commands to generate.