Skip to content

test: add Kafka CLI smoke tests for retention, compaction, kafka-delete-records and SCRAM - #923

Merged
solace-aross merged 5 commits into
mainfrom
test/sol-155325-maintenance-and-scram-checks
Oct 9, 2026
Merged

solace-aross merged 5 commits into
mainfrom
test/sol-155325-maintenance-and-scram-checks

Conversation

@solace-wkourlas

Copy link
Copy Markdown
Collaborator

Stacked on #902: review that one first.

What changes, and why?

It adds smoke tests that run the Kafka CLI tools against Nisshi for the broker's maintenance and authentication:

  • Retention (PostgreSQL and SQLite): expired records deleted and newer ones kept, the next offset after retention, a topic without cleanup.policy, retention.ms=-1, a 30-day retention.ms, kafka-configs --add-config and --delete-config retention.ms, retention.bytes, offsets after retention empties a partition.
  • Compaction (PostgreSQL and SQLite): the latest value per key, a tombstone as the key's only record, and compact,delete.
  • kafka-delete-records (every engine): the new low watermark, the earliest offset, the latest offset.
  • SCRAM: users created with kafka-configs and described with it; login with SCRAM-SHA-256 and SCRAM-SHA-512, a wrong password, and a client without credentials, all on PostgreSQL and SQLite, because a broker with --authentication can't create its first user, so each test restarts the broker. That restart duplicated three SCRAM tests in restart.rs, which this PR moves into scram.rs.
  • SQLite vacuum_into: a broker started on the snapshot, after its own database has been deleted, has the topics and records.

Harness changes: kafka-delete-records, kafka-configs --describe --entity-type users, a producer that sets old timestamps, a broker that runs maintenance every 2 seconds, and a restart onto other storage that can delete files first. Broker::file_exists now copies the file out with docker cp. It used docker exec … test, and the broker image has no test command, so it returned false for every file in a broker container.

Ignored tests

Each is waiting for:

Upgrade impact

None. This changes only the smoke tests.

How was this tested?

Local runs of just smoke <engine> with every test, the ignored ones included, and the Kafka 3.9 tools:

Engine Tests run Passed Failed, all ignored
SQLite 86 69 17
Memory 62 46 16
PostgreSQL 82 63 19

On each engine, the failed tests are exactly the tests ignored on that engine, so CI, which skips them, passes. The failures include the ignored tests from #902.

  • cargo clippy -p nisshi-smoke-test --all-features --all-targets -- -D warnings, and with --features memory
  • cargo fmt --check, and rustdoc with warnings denied
  • each commit builds and passes the crate's unit tests on its own
  • S3 and the Kafka 3.7 and 4.3 tools: CI's smoke jobs

🤖 Generated with Claude Code

@solace-wkourlas
solace-wkourlas added this pull request to stack #917 October 9, 2026 17:39
solace-aross
solace-aross previously approved these changes Oct 9, 2026

@solace-aross solace-aross left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Approving at 047b99b with two notes.

I read the harness and the new tests. What I checked:

  • Every ignored test names an open issue or PR (#864, #904, #918 to #922, and #835 for DeleteRecords), and the five filed issues match the behavior each test describes.
  • The retention tests are built to avoid flaking. They produce hour-old records, run maintenance every 2 seconds, wait up to 60 seconds, and keep a newer record in the topic so the earliest offset never reads as the 0 of an emptied partition. wait_for_maintenance_run covers the tests that expect records to survive.
  • The SCRAM login tests create the user before restarting with --authentication. They check that the credentials survive the restart, so moving the three restart tests out of restart.rs loses no coverage.
  • The file_exists fix is right. The broker image has no test, so the old docker exec version returned false for every file. docker cp with an explicit working directory fixes it, and a failure other than a missing file panics instead of reading as missing.
  • The snapshot test waits for two snapshot writes and deletes the live database and its -wal and -shm files, so the records can only come from the snapshot.

Notes:

  • No CI has run on this head except DCO, because the base is the #902 branch. The results in the description are local runs. Rerun the checks once it is retargeted to main, and watch the s3 leg and the Kafka 3.7 and 4.3 tools, which the description lists as untested locally.
  • Merge order is #862, then #902, then this PR.

@solace-wkourlas
solace-wkourlas force-pushed the test/sol-155325-maintenance-and-scram-checks branch from 047b99b to d68eaa3 Compare October 9, 2026 18:13
solace-aross
solace-aross previously approved these changes Oct 9, 2026

@solace-aross solace-aross left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Approving at d68eaa3. The PR's own commits are unchanged. The only difference from my last review is the base moving to #902's current head, which brings in random_password() for the SCRAM test passwords (the CodeQL fix I approved on #902).

CI is green on this head, including test (postgres:17), both compat legs and ci-gate. The base is still #902's branch, so the smoke legs are skipped; rerun the checks once this is retargeted to main, and watch the s3 leg and the Kafka 3.7 and 4.3 tools. Merge order is #862, then #902, then #923.

Base automatically changed from test/sol-155250-kafka-cli-checks to main October 9, 2026 19:37
solace-wkourlas and others added 5 commits October 9, 2026 15:37
…duce old records and restart on other storage

Adds kafka-delete-records and kafka-configs --describe --entity-type users
to the harness, a verifiable producer that sets the records' timestamps,
a broker that runs maintenance every 2 seconds, and a restart onto other
storage that can delete files first. Reading a file where a broker runs
now copies it out with docker cp, because the broker image has no test
or stat command, so file_exists used to return false for every file in a
broker container.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Signed-off-by: William Kourlas <156007774+solace-wkourlas@users.noreply.github.com>
…SQLite

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Signed-off-by: William Kourlas <156007774+solace-wkourlas@users.noreply.github.com>
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Signed-off-by: William Kourlas <156007774+solace-wkourlas@users.noreply.github.com>
… into them

Every SCRAM login test restarts the broker, because a broker with
--authentication can't create its first user, so the restart tests'
SCRAM checks were duplicates.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Signed-off-by: William Kourlas <156007774+solace-wkourlas@users.noreply.github.com>
…napshot

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Signed-off-by: William Kourlas <156007774+solace-wkourlas@users.noreply.github.com>
@solace-aross
solace-aross force-pushed the test/sol-155325-maintenance-and-scram-checks branch from d68eaa3 to 0193898 Compare October 9, 2026 19:37

@solace-aross solace-aross left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Approving at 0193898. The branch is retargeted to main now that #862, #863 and #902 have merged. The PR's own smoke-test changes are identical to what I approved at d68eaa3, and its diff against main is the same 15 files under nisshi-smoke-test.

Two things to watch:

  • CI was still running when I approved: Analyze, clippy, both compat legs and third-party-license. fmt, typos, zizmor, actionlint, cargo-deny and DCO pass.
  • The smoke legs don't run on PR pushes, so the s3 leg and the Kafka 3.7 and 4.3 tools get their first run in the merge queue. Worth a look at those results before treating this as done.

@solace-aross
solace-aross added this pull request to the merge queue Oct 9, 2026
Merged via the queue into main with commit 21f6502 Oct 9, 2026
27 checks passed
@solace-aross
solace-aross deleted the test/sol-155325-maintenance-and-scram-checks branch October 9, 2026 20:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants