Repository navigation
Conversation
Signed-off-by: Eduard Zateev <hackerowskiy@gmail.com>
315f805 to
5b13f6b
Compare
| } | ||
|
|
||
| # Create a script in the container rootfs for a test. | ||
| create_test_script() { |
There was a problem hiding this comment.
create_test_script and update_container_args are moved here, but the original definitions are still in test/06-priority-parsing.bats (lines 22 and 31), so they are now duplicated (06 silently overrides these). Please remove them from 06, the same way skip_if_no_journald was.
There was a problem hiding this comment.
... and make it a separate cleanup commit (perhaps the first one)
|
|
||
| create_test_script "slow_stdin.sh" '#!/bin/sh | ||
| sleep 2 | ||
| cat >/dev/null' |
There was a problem hiding this comment.
Instead of discarding the input, consider having the container count it (e.g. wc -c) and then checking that the log contains 8388608. That would also verify the main claim behind the fix: on EAGAIN the bytes are retried and nothing is lost.
| wait_for_runtime_status "$CTR_ID" running | ||
|
|
||
| # Keep the container stdin pipe full while its reader is sleeping. | ||
| head -c 8388608 /dev/zero | socat -u STDIN "UNIX:${ATTACH_PATH},socktype=5" |
There was a problem hiding this comment.
Nit: 8 MiB is much more than needed to fill a pipe (64 KiB by default); something like 1 MiB should be enough and makes the test faster.
|
|
||
| local journal_output | ||
| journal_output=$(get_conmon_journal_output "$CONMON_PID") | ||
| if [[ "$journal_output" == *"Failed to write to fd container stdin"* ]]; then |
There was a problem hiding this comment.
This is a negative check, so it passes vacuously if the journal returns nothing for this conmon (journald lag, filtering mismatch, etc.), and the test would not catch a regression. Please first assert that journal_output is non-empty (conmon runs with --log-level trace --syslog, so it always logs something), and then use the existing helper for the actual check, e.g.
[ -n "$journal_output" ] || die "no journal output for conmon $CONMON_PID"
assert "$journal_output" !~ "Failed to write to fd container stdin"|
Hi @sairex53 — it looks like this PR may have gone quiet while waiting for the review feedback to be addressed. The approach looks right to me, so I went ahead and prepared the changes @kolyshkin requested:
Verified locally: the new test fails on the unfixed binary and passes with the fix; 07-attach and 06-priority-parsing suites are green. If you'd like to keep driving your PR, the branch and patch are yours to use. Otherwise I'll open a follow-up PR referencing this one, with your Signed-off-by preserved. Either way, thanks for the fix! @kolyshkin, happy to push it for your review if the original |
|
@TDnorthgarden feel free to open a carry PR! |
Thanks @kolyshkin! Carry PR opened: #715 — @sairex53's Signed-off-by is preserved in both commits, and all your review comments are |
Fixes #689.
Writes to the container stdin pipe and socket are nonblocking. When they return
EAGAINorEWOULDBLOCK, the pending bytes remain queued and are retried when the fd becomes writable, so this is expected backpressure rather than a write failure. Avoid logging a warning for those errno values while preserving warnings for other errors.Add an attach regression test that delays the container's stdin reader, sends an 8 MiB payload, and checks conmon's journal for the spurious warning.
Testing:
make -j2(passed)batsandsocat; CI will run it.