test(buffer): add functional test for disk buffer corruption recovery (LOG-9386) - #3393
Conversation
PR Summary by QodoAdd functional test for Vector disk buffer corruption recovery
AI Description
Diagram
High-Level Assessment
Files changed (3)
|
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Enterprise Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughAdds a tools sidecar visitor for shared disk access, helpers that corrupt Vector disk-buffer records while preserving checksums, and a functional test that verifies collector recovery and resumed delivery. ChangesDisk-buffer recovery validation
Estimated code review effort: 4 (Complex) | ~45 minutes Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
openshift-ci
Bot
added
the
do-not-merge/work-in-progress
Code Review by Qodo
1.
|
![qodo-for-rh-openshift[bot]](https://avatars.githubusercontent.com/u/792337?s=60&v=4){kind=small}
| b.AddContainer(ToolsContainerName, "registry.access.redhat.com/ubi9/ubi-minimal:latest"). | ||
| AddVolumeMount(volumeName, mountPath, "", false). | ||
| WithCmd([]string{"sleep", "infinity"}). | ||
| End() |
There was a problem hiding this comment.
1. Unpinned tools image tag 🐞 Bug ⚙ Maintainability
AddToolsContainerVisitor hard-codes the tools sidecar image as ubi-minimal:latest, making functional tests non-reproducible and susceptible to upstream image changes. This can cause CI to start failing without any source change in this repo.
Agent Prompt
### Issue description
The tools sidecar image is referenced with the mutable `:latest` tag, so the test environment can change over time.
### Issue Context
This is introduced in the functional framework helper used to add a "tools" container.
### Fix Focus Areas
- Pin the image to a specific version or (preferably) an immutable digest.
- Consider using an image already managed/pinned by the test framework (if available) to avoid introducing a new external moving dependency.
### Fix Focus Areas (code)
- test/framework/functional/framework.go[195-206]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (8)
test/functional/misc/disk_buffer_corruption_test.go (4)
146-157: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winParse
restartCountas an integer.Line 155 compares
restartCountas a string:strings.TrimSpace(countStr) >= "1". This is a lexicographic comparison, not a numeric one. It happens to work for the observed values, but it breaks for any zero-padded or unexpected output and it hides intent.♻️ Proposed fix
- return strings.TrimSpace(countStr) >= "1", nil + count, convErr := strconv.Atoi(strings.TrimSpace(countStr)) + if convErr != nil { + return false, nil + } + return count >= 1, nilAdd the import:
import "strconv"🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@test/functional/misc/disk_buffer_corruption_test.go` around lines 146 - 157, Update the restart-count check in the PollUntilContextTimeout callback to trim countStr, parse it with strconv.Atoi, and compare the resulting integer against 1. Preserve the existing retry behavior when the command or parsing fails, and add the strconv import.
159-172: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueThe assertions depend on exact Vector log strings.
Lines 166, 169, and 171 match
"Vector has started.","Corrupted record found during buffer initialization seek", and"Healthcheck failed.". These strings come from Vector internals and change between releases. The middle string is the point of the test, so keep it. The"Healthcheck failed."assertion at line 171 adds little: it only restates that the receiver is still frozen, and it couples the test to an unrelated log message.Consider removing the healthcheck assertion, or replacing it with a check on delivery behavior.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@test/functional/misc/disk_buffer_corruption_test.go` around lines 159 - 172, Remove the exact “Healthcheck failed.” log assertion from the restart verification in the disk corruption test. Keep the corruption-warning assertion and startup wait unchanged; do not add a replacement unless an existing delivery-behavior check is readily available in this test.
184-196: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winWait for the post-restart message.
ReadFileFrompollscatevery 10 seconds, but it returns after the first successful non-empty result. Replayed disk-buffer records can satisfy that condition before the five new records arrive. Poll a bounded read andContainSubstring(postRestartMessage)withEventually; avoid nesting a two-minute timeout around the helper’s five-minute maximum read duration.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@test/functional/misc/disk_buffer_corruption_test.go` around lines 184 - 196, Replace the immediate post-restart ReadFileFrom assertion with a bounded Eventually that repeatedly reads the HTTP application log and checks for postRestartMessage via ContainSubstring. Use a polling duration and timeout that do not wrap ReadFileFrom’s five-minute maximum duration, while preserving error validation for each read.
119-133: 🚀 Performance & Scalability | 🔵 Trivial | 💤 Low valueUse one upload operation for the corrupted buffer.
framework.RunCommandinvokesoc execfor each 64 KiB chunk. A 1 MiB buffer requires 16 exec calls, and a 4 MiB buffer requires 64. Add a framework-level streaming or copy path becauseoc.Execdoes not expose stdin support.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@test/functional/misc/disk_buffer_corruption_test.go` around lines 119 - 133, Replace the per-chunk framework.RunCommand loop in the corrupted-buffer setup with a framework-level streaming or copy operation that uploads the complete corrupted buffer in one command. Extend the relevant framework/container execution API, since oc.Exec lacks stdin support, and preserve the subsequent base64 decode and staging-file cleanup flow.test/functional/misc/corrupt_buffer.go (2)
10-19: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueMove the type doc comment to the type.
The comment block at lines 10-18 describes
corruptDiskBufferPayload, but it is attached tocorruptDiskBufferResultbecause line 19 continues the same block. Godoc renders the whole block as the type documentation.♻️ Proposed fix
-// corruptDiskBufferPayload corrupts the protobuf payload of records in a -// Vector disk buffer data file while keeping the rkyv record structure and CRC32 -// checksum valid. This causes Vector's T::decode() to fail with -// ReaderError::Decode — the specific error path fixed by -// vectordotdev/vector#25691 (LOG-9386). -// -// The last record is left intact because Vector's writer validates it during -// initialization (writer.validate_last_write). Only the reader's seek path -// uses is_bad_read() to skip decode errors. // corruptDiskBufferResult holds the outcome of a corruption operation. type corruptDiskBufferResult struct { TotalRecords int CorruptedRecords int FileBytes int } +// corruptDiskBufferPayload corrupts the protobuf payload of records in a +// Vector disk buffer data file while keeping the rkyv record structure and CRC32 +// checksum valid. This causes Vector's T::decode() to fail with +// ReaderError::Decode — the specific error path fixed by +// vectordotdev/vector#25691 (LOG-9386). +// +// The last record is left intact because Vector's writer validates it during +// initialization (writer.validate_last_write). Only the reader's seek path +// uses is_bad_read() to skip decode errors. func corruptDiskBufferPayload(datFilePath string) (*corruptDiskBufferResult, error) {🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@test/functional/misc/corrupt_buffer.go` around lines 10 - 19, Move the descriptive doc comment for corruptDiskBufferPayload so it directly precedes that function, and separate it from the corruptDiskBufferResult documentation. Add or retain a distinct doc comment for corruptDiskBufferResult that describes the result type.
77-112: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winDocument the pinned Vector archive version
Vector
v0.54.0usesrkyv 0.7.46andcrc32fast::Hasher, socrc32.NewIEEE()matches itsBE(id) || BE(metadata) || payloadchecksum calculation.Add these versions to the layout comment. Add a focused test for the synthetic record and checksum to detect future layout drift.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@test/functional/misc/corrupt_buffer.go` around lines 77 - 112, Update the ArchivedRecord layout comment near the checksum logic to document the pinned Vector v0.54.0, rkyv 0.7.46, and crc32fast::Hasher versions and checksum assumptions. Add a focused test covering the synthetic record and expected checksum, using the existing corruption/checksum helpers, so future layout or checksum drift is detected.test/framework/functional/framework.go (2)
197-202: 🩺 Stability & Availability | 🔵 Trivial | 💤 Low valueGuard
GetContainerwhen the collector is absent.
GetContainerreturns a builder with a nil container when no match exists, soAddVolumeMountpanics.DeployWithVisitorscurrently addsconstants.CollectorNamefirst. Guard this precondition if callers can provide an arbitraryPodBuilder.WithCmdrequires at least one argument; the current call is valid.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@test/framework/functional/framework.go` around lines 197 - 202, Update AddToolsContainerVisitor to verify that GetContainer(constants.CollectorName) returns a valid container before calling AddVolumeMount, returning an appropriate error when the collector is absent while preserving the existing volume setup for valid builders.
203-206: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winMake the tools image configurable and pin the default image.
Read
TOOLS_IMAGEwithos.Getenvand use a pinned default. Do not useutils.GetEnvVar; it accepts[]v1.EnvVar, not a default string. Use an image that containsbash,find,base64, andwc, or install the required packages. Avoid:latestbecause it can change between runs and fail on clusters without access to the registry.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@test/framework/functional/framework.go` around lines 203 - 206, Update the tools-container setup around ToolsContainerName to read TOOLS_IMAGE via os.Getenv, falling back to a pinned image tag that provides bash, find, base64, and wc (or installs them). Replace the hardcoded :latest image while preserving the existing volume mount, command, and container configuration.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@test/functional/misc/disk_buffer_corruption_test.go`:
- Around line 101-104: Close the file returned by os.CreateTemp in the
temporary-file setup around tmpFile before writing with os.WriteFile, while
preserving the existing cleanup via os.Remove and subsequent test behavior.
---
Nitpick comments:
In `@test/framework/functional/framework.go`:
- Around line 197-202: Update AddToolsContainerVisitor to verify that
GetContainer(constants.CollectorName) returns a valid container before calling
AddVolumeMount, returning an appropriate error when the collector is absent
while preserving the existing volume setup for valid builders.
- Around line 203-206: Update the tools-container setup around
ToolsContainerName to read TOOLS_IMAGE via os.Getenv, falling back to a pinned
image tag that provides bash, find, base64, and wc (or installs them). Replace
the hardcoded :latest image while preserving the existing volume mount, command,
and container configuration.
In `@test/functional/misc/corrupt_buffer.go`:
- Around line 10-19: Move the descriptive doc comment for
corruptDiskBufferPayload so it directly precedes that function, and separate it
from the corruptDiskBufferResult documentation. Add or retain a distinct doc
comment for corruptDiskBufferResult that describes the result type.
- Around line 77-112: Update the ArchivedRecord layout comment near the checksum
logic to document the pinned Vector v0.54.0, rkyv 0.7.46, and crc32fast::Hasher
versions and checksum assumptions. Add a focused test covering the synthetic
record and expected checksum, using the existing corruption/checksum helpers, so
future layout or checksum drift is detected.
In `@test/functional/misc/disk_buffer_corruption_test.go`:
- Around line 146-157: Update the restart-count check in the
PollUntilContextTimeout callback to trim countStr, parse it with strconv.Atoi,
and compare the resulting integer against 1. Preserve the existing retry
behavior when the command or parsing fails, and add the strconv import.
- Around line 159-172: Remove the exact “Healthcheck failed.” log assertion from
the restart verification in the disk corruption test. Keep the
corruption-warning assertion and startup wait unchanged; do not add a
replacement unless an existing delivery-behavior check is readily available in
this test.
- Around line 184-196: Replace the immediate post-restart ReadFileFrom assertion
with a bounded Eventually that repeatedly reads the HTTP application log and
checks for postRestartMessage via ContainSubstring. Use a polling duration and
timeout that do not wrap ReadFileFrom’s five-minute maximum duration, while
preserving error validation for each read.
- Around line 119-133: Replace the per-chunk framework.RunCommand loop in the
corrupted-buffer setup with a framework-level streaming or copy operation that
uploads the complete corrupted buffer in one command. Extend the relevant
framework/container execution API, since oc.Exec lacks stdin support, and
preserve the subsequent base64 decode and staging-file cleanup flow.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 63b0e985-8b50-4795-91b6-0b16b2208fd2
📒 Files selected for processing (3)
test/framework/functional/framework.gotest/functional/misc/corrupt_buffer.gotest/functional/misc/disk_buffer_corruption_test.go
70753ff to
db6efc0
Compare
openshift-ci
Bot
removed
the
do-not-merge/work-in-progress
| out, err := framework.RunCommand(functional.ToolsContainerName, "bash", "-c", findCmd) | ||
| if err != nil || strings.TrimSpace(out) == "" { | ||
| return false, nil | ||
| } |
There was a problem hiding this comment.
2. Tools sidecar bash dependency 🐞 Bug ☼ Reliability
The new tools sidecar is used exclusively via RunCommand(..., "bash", "-c", ...), but the sidecar definition doesn’t enforce/provision bash (it just runs sleep infinity). If bash is unavailable in the tools image at runtime, these commands fail and the functional test aborts before validating corruption recovery.
Agent Prompt
### Issue description
The test assumes `bash` exists in the tools sidecar and uses it for all tooling commands. The tools container definition does not guarantee that assumption, so the test can fail early if the image environment lacks `bash`.
### Issue Context
The tools sidecar is started with a simple sleep command and no bootstrap/install step. The test then execs into it using `bash -c` for `ls`, `wc`, `base64`, and chunked writes.
### Fix Focus Areas
- test/framework/functional/framework.go[195-206]
- test/functional/misc/disk_buffer_corruption_test.go[164-173]
- test/functional/misc/disk_buffer_corruption_test.go[223-274]
### Suggested fix direction
- Prefer `sh -c` with POSIX-safe commands to minimize shell dependency.
- Or change the tools image / init to guarantee `bash` (and required utilities) are present.
- Optionally add a quick preflight check in the test (e.g., `command -v bash`) to fail fast with a clear error message.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
|
Code review by qodo was updated up to the latest commit db6efc0 |
|
/hold |
openshift-ci
Bot
added
the
do-not-merge/hold
db6efc0 to
2926140
Compare
|
/hold cancel |
openshift-ci
Bot
removed
the
do-not-merge/hold
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (3)
test/functional/misc/disk_buffer_corruption_test.go (3)
125-133: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueDo not write to the outer
errinside the poll closure.Line 127 assigns the outer
err, and line 126 overwrites it with the poll result. The assertion on line 133 therefore never reports the command error. Use a local variable inside the closure.♻️ Proposed change
var sinkResult string err = wait.PollUntilContextTimeout(context.TODO(), 2*time.Second, 90*time.Second, true, func(ctx context.Context) (bool, error) { - sinkResult, err = framework.RunCommand(string(obs.OutputTypeHTTP), "cat", functional.ApplicationLogFile) - if err != nil { + out, readErr := framework.RunCommand(string(obs.OutputTypeHTTP), "cat", functional.ApplicationLogFile) + if readErr != nil { return false, nil } + sinkResult = out return strings.Contains(sinkResult, fmt.Sprintf("post-corruption-msg-%d", postRestartCount)), nil })🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@test/functional/misc/disk_buffer_corruption_test.go` around lines 125 - 133, Update the poll closure in the disk-buffer corruption test to capture the RunCommand error in a local variable rather than assigning the outer err. Keep the poll result assigned to the outer err so the final Expect(err) assertion reports polling failures while command errors remain handled within the closure.
77-81: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueResume the frozen receiver in a deferred cleanup.
If any assertion between lines 81 and 116 fails, the receiver process stays in the
STOPstate and thekill -CONTon line 115 never runs. Ginkgo aborts theItbody on failure.framework.Cleanup()deletes the pod, so this is not a leak across tests, but the frozen process hides receiver-side diagnostics in the failure output. Register aDeferCleanupthat resumes the process right after you stop it.♻️ Proposed change
out, err := framework.RunCommand(functional.ToolsContainerName, "sh", "-c", fmt.Sprintf("kill -STOP %s", receiverPid)) Expect(err).To(BeNil(), "Failed to freeze HTTP receiver: %s", out) + DeferCleanup(func() { + _, _ = framework.RunCommand(functional.ToolsContainerName, "sh", "-c", fmt.Sprintf("kill -CONT %s", receiverPid)) + }) time.Sleep(5 * time.Second)Also applies to: 114-116
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@test/functional/misc/disk_buffer_corruption_test.go` around lines 77 - 81, After stopping the receiver in the disk-buffer corruption test, immediately register a Ginkgo DeferCleanup callback that runs kill -CONT for receiverPid and verifies the command succeeds, ensuring the process is resumed even when later assertions abort the It body. Remove or consolidate the existing manual resume at the later cleanup location so the receiver is resumed exactly once.
100-101: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueAssert on the corrupted record count at the call site.
corruptBufferFilereturns the number of corrupted records, but line 101 discards it. Either assert on it in the test body or drop the return value, because the internalExpect(corruptedCount).To(BeNumerically(">=", 1))already covers the lower bound.♻️ Proposed change
- corruptBufferFile(framework, datFilePath) + corruptedCount := corruptBufferFile(framework, datFilePath) + By(fmt.Sprintf("corrupted %d buffer records", corruptedCount))Also applies to: 274-274
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@test/functional/misc/disk_buffer_corruption_test.go` around lines 100 - 101, Capture the return value from corruptBufferFile at both call sites in the test and assert that the corrupted record count meets the expected lower bound; alternatively, remove the return value if the internal assertion is intended to be the sole validation. Update the test body around the “corrupting the disk buffer via the tools sidecar” step without changing the corruption behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@test/functional/misc/corrupt_buffer.go`:
- Around line 37-40: Remove the single-record fallback that sets corruptUpTo to
1. In the corrupt-buffer setup, detect when len(records) is less than two and
return a clear error immediately, preserving the invariant that the final record
remains intact and allowing the caller to retry or fail explicitly.
In `@test/functional/misc/disk_buffer_corruption_test.go`:
- Around line 256-269: Update waitForCollectorRestart to parse countStr with
strconv.Atoi before comparing it, returning false when parsing fails and
otherwise requiring the numeric restart count to be at least one. Add the
necessary strconv import and preserve the existing polling and error behavior.
---
Nitpick comments:
In `@test/functional/misc/disk_buffer_corruption_test.go`:
- Around line 125-133: Update the poll closure in the disk-buffer corruption
test to capture the RunCommand error in a local variable rather than assigning
the outer err. Keep the poll result assigned to the outer err so the final
Expect(err) assertion reports polling failures while command errors remain
handled within the closure.
- Around line 77-81: After stopping the receiver in the disk-buffer corruption
test, immediately register a Ginkgo DeferCleanup callback that runs kill -CONT
for receiverPid and verifies the command succeeds, ensuring the process is
resumed even when later assertions abort the It body. Remove or consolidate the
existing manual resume at the later cleanup location so the receiver is resumed
exactly once.
- Around line 100-101: Capture the return value from corruptBufferFile at both
call sites in the test and assert that the corrupted record count meets the
expected lower bound; alternatively, remove the return value if the internal
assertion is intended to be the sole validation. Update the test body around the
“corrupting the disk buffer via the tools sidecar” step without changing the
corruption behavior.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: dc309310-a89e-4696-900f-26a70a1026ae
📒 Files selected for processing (2)
test/functional/misc/corrupt_buffer.gotest/functional/misc/disk_buffer_corruption_test.go
2926140 to
a59a8f4
Compare
|
@CodeRabbit review |
|
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: jcantrill, vparfonov The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
openshift-ci
Bot
added
the
approved
|
/test functional-target |
|
PR-Agent: could not find a component named |
… (LOG-9386) Verify that Vector does not CrashLoop when the disk buffer contains corrupted protobuf records. The test corrupts buffer record payloads while the collector is running, then kills it so the restart reads the corrupted buffer. Asserts that Vector logs a warning about corrupted records during seek, starts successfully, and continues delivering logs after recovery. Signed-off-by: Vitalii Parfonov <vparfono@redhat.com>
a59a8f4 to
25a51a7
Compare
|
@vparfonov: all tests passed! Full PR test history. Your PR dashboard. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here. |
|
/lgtm |
3 participants
Description
Verify that Vector does not CrashLoop when the disk buffer contains corrupted protobuf records. The test corrupts buffer record payloads while the collector is running, then kills it so the restart reads the corrupted buffer. Asserts that Vector logs a warning about corrupted records during seek, starts successfully, and continues delivering logs after recovery.
/cc @Clee2691
/assign @jcantrill
Links
Summary by CodeRabbit