Fix logs command hanging due to output wrapper buffering #109
Conversation
The velero->oadp output wrapper was causing logs commands to hang because it buffered all output before processing. For streaming commands like logs, this created a deadlock since the command never fully completes. Solution: Skip wrapping logs commands entirely to allow real-time streaming without buffering. This also prevents log content from being modified, which could hide important velero-related information in actual logs. Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com> Signed-off-by: Joseph <jvaikath@redhat.com>
There was a problem hiding this comment.
Pull request overview
This PR fixes a deadlock issue that caused the logs command to hang indefinitely. The fix has two components: (1) skipping the velero->oadp output wrapper entirely for logs commands to allow unmodified real-time streaming, and (2) introducing a goroutine-based approach for non-logs commands to prevent deadlocks when commands produce output.
Changes:
- Added detection logic to identify logs commands and skip output wrapping for them
- Refactored Run and RunE wrappers to use concurrent goroutines that read from the output pipe while the command executes
- Updated comments to explain the goroutine approach
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
cmd/root.go
Outdated
| // Start goroutine to read from pipe and write to real stdout | ||
| // This prevents deadlock with streaming commands (like logs) |
There was a problem hiding this comment.
The comment is misleading. It states "This prevents deadlock with streaming commands (like logs)" but logs commands are already excluded from wrapping on line 253 via the !isLogsCommand condition. The goroutine prevents deadlock for ALL wrapped commands (not just streaming ones) because the original code tried to read from the pipe only after the command completed, but if the command wrote enough data to fill the pipe buffer, it would block forever. The comment should clarify that this prevents deadlock for any command that produces output, not specifically for streaming commands.
cmd/root.go
Outdated
| go func() { | ||
| var buf strings.Builder | ||
| _, err := io.Copy(&buf, r) | ||
| if err != nil { | ||
| done <- err | ||
| return | ||
| } | ||
| output := replaceVeleroCommandWithOADP(buf.String()) | ||
| fmt.Fprint(oldStdout, output) | ||
| done <- nil | ||
| }() |
There was a problem hiding this comment.
The implementation still buffers all output in memory using strings.Builder before writing it out. While this prevents the original deadlock (by reading in a goroutine), it doesn't enable true real-time streaming as suggested in the PR description. For logs commands, this is correctly addressed by skipping the wrapper entirely. However, for non-logs commands that might produce large amounts of output, this buffering approach could consume significant memory. Consider whether this is acceptable for the use case, or if a streaming replacement approach (that replaces on the fly without buffering) would be better.
There was a problem hiding this comment.
its ok.. limited output for anything outside of logs.
Removed goroutine complexity from the velero->oadp output wrapper since logs commands are now excluded. The remaining wrapped commands are all non-streaming and complete normally, so the simpler synchronous approach is sufficient. Added comprehensive test to ensure logs commands are never wrapped. The test verifies both Run and RunE functions are not wrapped for logs commands, and that wrapped commands still get output replacement while logs commands stream unmodified output. This prevents regressions if the exclusion logic is accidentally removed. Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com> Signed-off-by: Joseph <jvaikath@redhat.com>
Signed-off-by: Joseph <jvaikath@redhat.com>
Added Makefile targets for linting and testing that follow the same approach as the OADP operator project: - lint: Runs golangci-lint v1.63.4 (auto-downloads to bin/ if needed) - test: Runs all tests (unit + integration) - test-unit: Runs unit tests only - test-integration: Runs integration tests only - golangci-lint: Helper target to install linter locally Tools are installed locally via 'go install' to bin/ directory instead of using containers, providing faster execution and matching the OADP operator development workflow. Cleanup: - Removed unused CONTAINER_TOOL variable - Removed unused get_binary_name function - Added bin/ to .gitignore to prevent committing downloaded tools - Updated clean target to remove bin/ directory Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com> Signed-off-by: Joseph <jvaikath@redhat.com>
2 participants
Why the changes were made
The velero->oadp output wrapper was causing logs commands to hang because it buffered all output before processing. For streaming commands like logs, this created a deadlock since the command never fully completes.
Solution: Skip wrapping logs commands entirely to allow real-time streaming without buffering. This also prevents log content from being modified, which could hide important velero-related information in actual logs.
How to test the changes made
logs command should never hang any more
This was a big oversight from the moment the velero->oadp wrapper was introduced
Closes #110