-
Notifications
You must be signed in to change notification settings - Fork 573
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
tests: update offline_log_viewer and use in ducktape tests #5315
Conversation
959c779
to
52ad639
Compare
CI failures is: #5241 |
Note: merge https://github.com/redpanda-data/vtools/pull/772 before merging this |
tools/offline_log_viewer/storage.py
Outdated
try: | ||
[part, ntp_id] = part_ntp_id.split("_") | ||
except ValueError: | ||
# Not a partition dir: this may happen if we have a namespace dir | ||
# with no partitions. | ||
continue |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
seems like a more specific test is necessary to handle cases where a topic has an underscore?
print(head, part_ntp_id, part, ntp_id)
data/namespace topic1_asdf topic1 asdf
like here I end up with topic1/asdf as the partition and ntp_id.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This commit went away in favor of 906a9e7 fix for empty dirs upstream
tools/offline_log_viewer/kvstore.py
Outdated
@@ -349,6 +351,10 @@ def decode(self): | |||
s = Segment(path) | |||
for batch in s: | |||
for r in batch: | |||
offset = batch.header.base_offset + r.offset_delta | |||
if snapshot_offset is not None and offset < snapshot_offset: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
hmm, would offset be inclusive here: "all messages from before up to including the snapshot offset"?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
you are correct, I was thinking of next_offset, but the save_snapshot path in kvstore already subtracts 1. Fixed.
tools/offline_log_viewer/storage.py
Outdated
if len(data) < HEADER_SIZE: | ||
# Short read, probably log being actively written or unclean shutdown | ||
return None | ||
|
||
assert len(data) == 0 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
seems like assert len(data) == 0
is no longer possible. indeed, the if statement also doesn't seem necessary now since data will definitely be < header size.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fixed.
|
||
result = {} | ||
for n in redpanda.nodes: | ||
kvstore_json = n.account.ssh_output(viewer_cmd, combine_stderr=False) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is really pretty neat
Triggered rebuild on CI since the PR is a little old. |
52ad639
to
bab8c59
Compare
This should be pulled into #6233 because this PR can't decode modern logs, and that PR doesn't have anything to test it. |
f107f42
to
3f18cd5
Compare
/ci-repeat 1 |
On stderr it gets mixed with logs. On stdout it is straightforward to capture and decode json for use in tests.
This was wasteful: all messages from before the snapshot should already be accounted for in the state we loaded from the snapshot.
A None value is a deletion. Previously, we ignored these and thereby the kvstore dump would include all keys ever created, even if they had since been deleted.
…batch.header.type instead of batch.type)
... and augmented Reader.read_envelope to accept a type_read Callable[rdr: Reader, version: Int]: Dict and a max_version parameter. If a type_read is used, it is invoked with reader and envelope if envelope.version <= max_version parameter. the result is joined with a dict {'envelope': envelope} and returned, otherwise an error dict is returned. If no type_read is used, envelope is returned immediately
the new code tries to mimic the structure of the adl decoding functions. the new code is aware of the version number embedded in the binary stream and will skip the field if the version is not supported, while reporting the error.
…luster_config_command
…are processed in case of unread byte, an 'unread': {'k': ..., 'v': ...} field is added and an error is logged this check is not exact but is a good proxy to know if the decoding code is correct
Partitions have various kvstore items, stored by the raft + storage layers. When the topic is deleted, they should be deleted too. Use our offline log decoder tool to inspect the kvstore out-of-band and validate the keys go away when the topic is deleted.
This needs fixing, but better to comment it out and have the rest of the command work.
6d8b202
to
189e475
Compare
Cover letter
The tool is already in clustered ducktape AMI via https://github.com/redpanda-data/vtools/pull/772
Release notes