Skip to content

Commit e25af44

Browse files
authored
Merge pull request #592 from jnovy/252
Fix missing F-sequence on container exit
2 parents 04b823f + 59c2b94 commit e25af44

File tree

2 files changed

+176
-1
lines changed

2 files changed

+176
-1
lines changed

src/ctr_logging.c

Lines changed: 36 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -411,13 +411,19 @@ static int write_journald(int pipe, char *buf, ssize_t buflen)
411411
* The CRI requires us to write logs with a (timestamp, stream, line) format
412412
* for every newline-separated line. write_k8s_log writes said format for every
413413
* line in buf, and will partially write the final line of the log if buf is
414-
* not terminated by a newline.
414+
* not terminated by a newline. A 0 buflen argument forces any buffered partial
415+
* line to be finalized with an F-sequence.
415416
*/
416417
static int write_k8s_log(stdpipe_t pipe, const char *buf, ssize_t buflen)
417418
{
419+
static bool stdout_has_partial = false;
420+
static bool stderr_has_partial = false;
421+
418422
writev_buffer_t bufv = {0};
419423
int64_t bytes_to_be_written = 0;
420424

425+
bool *has_partial = (pipe == STDOUT_PIPE) ? &stdout_has_partial : &stderr_has_partial;
426+
421427
/*
422428
* Use the same timestamp for every line of the log in this buffer.
423429
* There is no practical difference in the output since write(2) is
@@ -426,6 +432,32 @@ static int write_k8s_log(stdpipe_t pipe, const char *buf, ssize_t buflen)
426432
char tsbuf[TSBUFLEN];
427433
set_k8s_timestamp(tsbuf, sizeof tsbuf, stdpipe_name(pipe));
428434

435+
/* If buflen is 0, this is a drain operation. Generate terminating F-sequence if needed. */
436+
if (buflen == 0 && *has_partial) {
437+
/* Generate terminating F-sequence for previous partial line */
438+
bool timestamp_written = false;
439+
bool f_sequence_written = false;
440+
441+
if (writev_buffer_append_segment(k8s_log_fd, &bufv, tsbuf, TSBUFLEN - 1) >= 0) {
442+
timestamp_written = true;
443+
if (writev_buffer_append_segment(k8s_log_fd, &bufv, "F\n", 2) >= 0) {
444+
f_sequence_written = true;
445+
}
446+
}
447+
448+
if (timestamp_written && f_sequence_written) {
449+
k8s_bytes_written += TSBUFLEN - 1 + 2;
450+
k8s_total_bytes_written += TSBUFLEN - 1 + 2;
451+
} else {
452+
if (!timestamp_written) {
453+
nwarn("failed to write timestamp for terminating F-sequence");
454+
} else {
455+
nwarn("failed to write terminating F-sequence");
456+
}
457+
}
458+
*has_partial = false;
459+
}
460+
429461
ptrdiff_t line_len = 0;
430462
while (buflen > 0) {
431463
bool partial = get_line_len(&line_len, buf, buflen);
@@ -495,6 +527,9 @@ static int write_k8s_log(stdpipe_t pipe, const char *buf, ssize_t buflen)
495527

496528
k8s_bytes_written += bytes_to_be_written;
497529
k8s_total_bytes_written += bytes_to_be_written;
530+
531+
/* Track partial state for this pipe */
532+
*has_partial = partial;
498533
next:
499534
/* Update the head of the buffer remaining to output. */
500535
buf += line_len;

test/09-partial-log-test.bats

Lines changed: 140 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,140 @@
1+
#!/usr/bin/env bats
2+
3+
load test_helper
4+
5+
setup() {
6+
check_conmon_binary
7+
check_runtime_binary
8+
setup_container_env
9+
}
10+
11+
teardown() {
12+
cleanup_test_env
13+
}
14+
15+
@test "partial log: container with printf (no newline) generates F-sequence" {
16+
# Modify config to run printf instead of echo
17+
jq '.process.args = ["/busybox", "printf", "hello world"]' "$BUNDLE_PATH/config.json" > "$BUNDLE_PATH/config.json.tmp"
18+
mv "$BUNDLE_PATH/config.json.tmp" "$BUNDLE_PATH/config.json"
19+
20+
# Run conmon
21+
timeout 30s "$CONMON_BINARY" \
22+
--cid "$CTR_ID" \
23+
--cuuid "$CTR_ID" \
24+
--runtime "$RUNTIME_BINARY" \
25+
--log-path "k8s-file:$LOG_PATH" \
26+
--bundle "$BUNDLE_PATH" \
27+
--socket-dir-path "$SOCKET_PATH" \
28+
--log-level debug \
29+
--container-pidfile "$PID_FILE" \
30+
--conmon-pidfile "$CONMON_PID_FILE" &
31+
32+
local conmon_pid=$!
33+
sleep 2
34+
35+
if kill -0 $conmon_pid 2>/dev/null; then
36+
kill $conmon_pid 2>/dev/null || true
37+
wait $conmon_pid 2>/dev/null || true
38+
fi
39+
40+
# Verify log file exists
41+
[ -f "$LOG_PATH" ]
42+
43+
local log_content
44+
log_content=$(cat "$LOG_PATH")
45+
46+
# Test environment may not support container execution
47+
# If logs are empty, skip the functional test but verify the implementation exists
48+
if [ -z "$log_content" ]; then
49+
echo "Log is empty - container runtime not available in test environment"
50+
echo "Skipping functional test, verifying implementation exists in source code"
51+
52+
# Verify the fix is present in source code
53+
grep -q "If buflen is 0, this is a drain operation" "$PWD/src/ctr_logging.c" || {
54+
echo "Missing drain operation code"
55+
return 1
56+
}
57+
grep -q "stdout_has_partial" "$PWD/src/ctr_logging.c" || {
58+
echo "Missing stdout_has_partial tracking"
59+
return 1
60+
}
61+
grep -q "stderr_has_partial" "$PWD/src/ctr_logging.c" || {
62+
echo "Missing stderr_has_partial tracking"
63+
return 1
64+
}
65+
grep -q "Generate terminating F-sequence" "$PWD/src/ctr_logging.c" || {
66+
echo "Missing F-sequence generation code"
67+
return 1
68+
}
69+
70+
skip "Container runtime not available for functional testing"
71+
else
72+
echo "=== Actual log content ==="
73+
echo "$log_content"
74+
echo "=== End log ==="
75+
76+
# Test for actual F-sequence functionality
77+
# Should have partial line followed by F-sequence
78+
echo "$log_content" | grep -q "stdout P hello world" || {
79+
echo "Expected partial line not found"
80+
return 1
81+
}
82+
83+
echo "$log_content" | grep -q "stdout F$" || {
84+
echo "Expected F-sequence not found"
85+
return 1
86+
}
87+
fi
88+
}
89+
90+
@test "partial log: container with echo (with newline) does NOT generate standalone F-sequence" {
91+
# Default config already uses echo which outputs newline
92+
93+
# Run conmon
94+
timeout 30s "$CONMON_BINARY" \
95+
--cid "$CTR_ID" \
96+
--cuuid "$CTR_ID" \
97+
--runtime "$RUNTIME_BINARY" \
98+
--log-path "k8s-file:$LOG_PATH" \
99+
--bundle "$BUNDLE_PATH" \
100+
--socket-dir-path "$SOCKET_PATH" \
101+
--log-level debug \
102+
--container-pidfile "$PID_FILE" \
103+
--conmon-pidfile "$CONMON_PID_FILE" &
104+
105+
local conmon_pid=$!
106+
sleep 2
107+
108+
if kill -0 $conmon_pid 2>/dev/null; then
109+
kill $conmon_pid 2>/dev/null || true
110+
wait $conmon_pid 2>/dev/null || true
111+
fi
112+
113+
# Check if log file exists and has content
114+
if [ ! -f "$LOG_PATH" ] || [ ! -s "$LOG_PATH" ]; then
115+
echo "Log file missing or empty - container runtime not available"
116+
echo "Skipping functional test, verifying implementation exists"
117+
118+
# Verify the fix implementation exists
119+
grep -q "If buflen is 0, this is a drain operation" "$PWD/src/ctr_logging.c" || {
120+
echo "Missing drain operation code"
121+
return 1
122+
}
123+
124+
skip "Container runtime not available for functional testing"
125+
fi
126+
127+
local log_content
128+
log_content=$(cat "$LOG_PATH")
129+
130+
echo "=== Log content ==="
131+
echo "$log_content"
132+
echo "=== End log ==="
133+
134+
# For normal output with newlines, should NOT have standalone F-sequences
135+
# (F-sequences should only appear for partial line termination)
136+
! echo "$log_content" | grep -q "stdout F$" || {
137+
echo "Unexpected standalone F-sequence found for normal output"
138+
return 1
139+
}
140+
}

0 commit comments

Comments
 (0)