OCPBUGS-54776: fix(ignition-server): log MCS output on HTTP request failures - #8936
Conversation
|
Pipeline controller notification For optional jobs, comment This repository is configured in: LGTM mode |
openshift-ci-robot
added
jira/valid-reference
|
@hypershift-jira-solve-ci[bot]: This pull request references Jira Issue OCPBUGS-54776, which is invalid:
Comment The bug has been updated to refer to the pull request using the external bug tracker. DetailsIn response to this:
Instructions 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 openshift-eng/jira-lifecycle-plugin repository. |
|
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:
📝 WalkthroughWalkthroughThe change adds a mutex-protected, tail-truncated buffer for Sequence Diagram(s)sequenceDiagram
participant runMCSAndFetchPayload
participant machine-config-server
participant fetchMCSIgnitionPayload
participant syncBuffer
runMCSAndFetchPayload->>machine-config-server: start with syncBuffer stdout and stderr
runMCSAndFetchPayload->>fetchMCSIgnitionPayload: poll for payload
machine-config-server->>syncBuffer: write process output
fetchMCSIgnitionPayload-->>runMCSAndFetchPayload: payload or retry signal
runMCSAndFetchPayload->>machine-config-server: cancel context and wait for exit
runMCSAndFetchPayload->>syncBuffer: read final output
runMCSAndFetchPayload-->>runMCSAndFetchPayload: return payload or wrapped error
Suggested reviewers: Important Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional. ❌ Failed checks (1 error, 1 inconclusive)
✅ Passed checks (9 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
openshift-ci
Bot
added
area/control-plane-operator
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #8936 +/- ##
==========================================
+ Coverage 44.51% 44.53% +0.01%
==========================================
Files 774 774
Lines 96997 97036 +39
==========================================
+ Hits 43179 43215 +36
- Misses 50830 50832 +2
- Partials 2988 2989 +1
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
ignition-server/controllers/local_ignitionprovider.go (1)
618-628: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winConsider bounding the captured MCS output in the log field and error message.
mcsOutput.String()is emitted in full at both Line 622 and Line 625. MCS bootstrap output is unbounded, so a verbose/looping process can bloat structured logs and, worse, produce very large error strings that propagate up the call chain. The PR intent notes truncating to the last 8KB — that safeguard isn't present here. Truncating to the tail keeps the most recent (usually most relevant) output while capping size.Correctness of the buffer read itself is fine: both reads occur after
<-mcsDone(i.e., aftercmd.Wait()and its stdout/stderr copy goroutines have completed), so there's no concurrent access to thebytes.Buffer.♻️ Example: truncate to the last 8KB
+ const maxMCSLog = 8 << 10 // 8KB + mcsLog := mcsOutput.String() + if len(mcsLog) > maxMCSLog { + mcsLog = mcsLog[len(mcsLog)-maxMCSLog:] + } + // Stop MCS and wait for process exit so all output is flushed to the buffer. cancel() mcsErr := <-mcsDone - log.Info("machine-config-server process exited", "output", mcsOutput.String(), "error", mcsErr) + log.Info("machine-config-server process exited", "output", mcsLog, "error", mcsErr) if err != nil { - return nil, fmt.Errorf("mcs logs: %s: %w", mcsOutput.String(), err) + return nil, fmt.Errorf("mcs logs: %s: %w", mcsLog, err) }Note: move the truncation computation after
<-mcsDoneso the buffer is fully populated before reading.🤖 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 `@ignition-server/controllers/local_ignitionprovider.go` around lines 618 - 628, The MCS exit logging and returned error in local_ignitionprovider.go are using the full mcsOutput.String(), which can be unbounded and bloat logs/error chains. Update the post-<mcsDone> handling around the machine-config-server shutdown to cap the captured output to the last 8KB before using it in log.Info and the fmt.Errorf return path. Keep the truncation logic near the existing mcsOutput.String() reads so the tail of the output is preserved while limiting size.
🤖 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.
Nitpick comments:
In `@ignition-server/controllers/local_ignitionprovider.go`:
- Around line 618-628: The MCS exit logging and returned error in
local_ignitionprovider.go are using the full mcsOutput.String(), which can be
unbounded and bloat logs/error chains. Update the post-<mcsDone> handling around
the machine-config-server shutdown to cap the captured output to the last 8KB
before using it in log.Info and the fmt.Errorf return path. Keep the truncation
logic near the existing mcsOutput.String() reads so the tail of the output is
preserved while limiting size.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: 90494ab4-8c09-47a1-9cb6-ad201dc3979b
📒 Files selected for processing (1)
ignition-server/controllers/local_ignitionprovider.go
There was a problem hiding this comment.
this is the use case we want to cover, something like
if res.StatusCode != http.StatusOK {
if closeErr := res.Body.Close(); closeErr != nil {
log.Error(closeErr, "failed to close mcs response body")
}
log.Error(fmt.Errorf("unexpected status code %d", res.StatusCode), "mcs returned unexpected response code", "code", res.StatusCode, "mcsOutput", mcsOutput.String())
return false, nil
}
``
There was a problem hiding this comment.
Done. Moved defer res.Body.Close() before the status check, replaced the nil err with a real fmt.Errorf, and added mcsOutput to the log line — matching the snippet you suggested. Also replaced bytes.Buffer with a mutex-protected syncBuffer capped at 8 KiB (per the truncation request in the general comment) and added unit tests for truncateTail and syncBuffer.
AI-assisted response via Claude Code
|
Implement truncation as suggested by code rabbit. cover these with units |
|
/test address-review-comments |
|
Review agent triggered. View job |
There was a problem hiding this comment.
🧹 Nitpick comments (2)
ignition-server/controllers/local_ignitionprovider.go (2)
121-126: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueParameter
maxshadows the built-inmax.Since Go 1.21,
maxis a predeclared built-in. Shadowing it here is a variable-shadowing smell (andpredeclaredlinters flag it). Rename tolimitor similar.♻️ Proposed rename
-func truncateTail(s string, max int) string { - if len(s) <= max { - return s - } - return s[len(s)-max:] +func truncateTail(s string, limit int) string { + if len(s) <= limit { + return s + } + return s[len(s)-limit:] }Update the
TestTruncateTailtable field name accordingly if you also rename there.As per coding guidelines: "Avoid variable shadowing."
🤖 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 `@ignition-server/controllers/local_ignitionprovider.go` around lines 121 - 126, Rename the `max` parameter in `truncateTail` to a non-shadowing name such as `limit` to avoid clashing with Go’s predeclared built-in, and update the length comparison and slicing logic accordingly. If `TestTruncateTail` or any related table-driven tests use a matching field name, rename that field too so the test data and call sites stay consistent.Source: Coding guidelines
104-119: 🚀 Performance & Scalability | 🔵 Trivial | 💤 Low value
syncBuffertruncates only on read, not in memory.
String()caps output at 8 KiB, but the underlyingbufis never reset, so it grows unbounded for the lifetime of the MCS process. Log size is bounded as intended, but memory is not — a verbose MCS that keeps emitting output during a long polling window can retain everything. If unbounded MCS output is plausible, consider trimmingbufinWriteonce it exceeds a threshold (e.g., a few multiples ofmaxMCSLogBytes) so retained memory stays bounded too.🤖 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 `@ignition-server/controllers/local_ignitionprovider.go` around lines 104 - 119, The syncBuffer implementation only truncates in String(), so the underlying bytes.Buffer in syncBuffer keeps growing unbounded over time. Update syncBuffer.Write to trim or reset buf when it exceeds a reasonable cap based on maxMCSLogBytes, and keep String() as the read-side limit; use the syncBuffer methods and maxMCSLogBytes constant to locate and adjust this 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.
Nitpick comments:
In `@ignition-server/controllers/local_ignitionprovider.go`:
- Around line 121-126: Rename the `max` parameter in `truncateTail` to a
non-shadowing name such as `limit` to avoid clashing with Go’s predeclared
built-in, and update the length comparison and slicing logic accordingly. If
`TestTruncateTail` or any related table-driven tests use a matching field name,
rename that field too so the test data and call sites stay consistent.
- Around line 104-119: The syncBuffer implementation only truncates in String(),
so the underlying bytes.Buffer in syncBuffer keeps growing unbounded over time.
Update syncBuffer.Write to trim or reset buf when it exceeds a reasonable cap
based on maxMCSLogBytes, and keep String() as the read-side limit; use the
syncBuffer methods and maxMCSLogBytes constant to locate and adjust this
behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: 678824ff-5799-4304-9d9b-d62460b6cbbb
📒 Files selected for processing (2)
ignition-server/controllers/local_ignitionprovider.goignition-server/controllers/local_ignitionprovider_test.go
|
/approve |
openshift-ci
Bot
added
the
approved
|
/jira refresh |
openshift-ci-robot
added
jira/valid-bug
|
@enxebre: This pull request references Jira Issue OCPBUGS-54776, which is valid. The bug has been moved to the POST state. 3 validation(s) were run on this bug
DetailsIn response to this:
Instructions 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 openshift-eng/jira-lifecycle-plugin repository. |
|
codecov/patch is failing because the changes do not have adequate unit test coverage. Use https://github.com/bryan-cox/personal-claude-skills/blob/main/plugins/behavior-driven-testing/skills/behavior-driven-testing/SKILL.md to create unit tests for the changes in the PR. |
|
/test address-review-comments |
|
Review agent triggered (Gangway execution ID: |
|
@hypershift-jira-solve-ci[bot]: This pull request references Jira Issue OCPBUGS-54776, which is valid. 3 validation(s) were run on this bug
DetailsIn response to this:
Instructions 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 openshift-eng/jira-lifecycle-plugin repository. |
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
ignition-server/controllers/local_ignitionprovider_test.go (1)
1390-1447: 🚀 Performance & Scalability | 🟠 Major | ⚡ Quick winBound the buffer during
Write, not only when formattingString.The test proves only that
String()returns at most 8 KiB. However,syncBuffer.Writeinlocal_ignitionprovider.gostill appends all data tobytes.Buffer; truncation happens only when reading it. Long-running or noisy MCS output can therefore grow memory without limit. MakeWriteretain only the tail, and assert the retained buffer itself stays bounded.🤖 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 `@ignition-server/controllers/local_ignitionprovider_test.go` around lines 1390 - 1447, Update syncBuffer.Write in local_ignitionprovider.go to truncate stored data immediately, retaining only the newest maxMCSLogBytes bytes after every write instead of allowing bytes.Buffer to grow unbounded. Preserve the existing tail semantics and synchronization, then extend TestSyncBuffer to verify the underlying retained buffer length remains at most maxMCSLogBytes, not just the length returned by String.
🧹 Nitpick comments (2)
ignition-server/controllers/local_ignitionprovider_test.go (2)
1449-1528: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winAdd a response-body closure assertion.
These tests verify payloads and status handling, but they cannot detect a regression that omits
resp.Body.Close(). Add a customRoundTripperreturning a trackingReadCloser, and assert closure for both successful and non-200 responses.🤖 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 `@ignition-server/controllers/local_ignitionprovider_test.go` around lines 1449 - 1528, Add response-body closure coverage to TestFetchMCSIgnitionPayload by using a custom http.RoundTripper that returns a tracking ReadCloser, then assert its Close method is called for both successful and non-200 responses. Preserve the existing payload/status assertions while routing the client through this transport so regressions omitting resp.Body.Close() are detected.Source: Coding guidelines
1551-1574: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winAssert the log output, not only buffer readability.
This test does not verify that MCS output is actually emitted in the failure log; it only verifies that the buffer remains readable. Capture and assert the relevant logger output, or rename the test to reflect the narrower buffer-preservation contract.
🤖 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 `@ignition-server/controllers/local_ignitionprovider_test.go` around lines 1551 - 1574, Update TestFetchMCSIgnitionPayloadMCSOutputIncludedInLog to capture the logger output used by fetchMCSIgnitionPayload and assert it contains the MCS buffer text when the request returns non-200; alternatively, rename the test to describe only buffer preservation if logging cannot be captured.
🤖 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 `@ignition-server/controllers/local_ignitionprovider_test.go`:
- Line 1464: The test fixtures currently discard errors returned by w.Write in
the affected handlers. Update each Write call, including those in the fixtures
around the referenced locations, to capture and check the returned error,
failing the test or returning from the handler when writing fails; do not use
blank assignments to ignore errors.
---
Outside diff comments:
In `@ignition-server/controllers/local_ignitionprovider_test.go`:
- Around line 1390-1447: Update syncBuffer.Write in local_ignitionprovider.go to
truncate stored data immediately, retaining only the newest maxMCSLogBytes bytes
after every write instead of allowing bytes.Buffer to grow unbounded. Preserve
the existing tail semantics and synchronization, then extend TestSyncBuffer to
verify the underlying retained buffer length remains at most maxMCSLogBytes, not
just the length returned by String.
---
Nitpick comments:
In `@ignition-server/controllers/local_ignitionprovider_test.go`:
- Around line 1449-1528: Add response-body closure coverage to
TestFetchMCSIgnitionPayload by using a custom http.RoundTripper that returns a
tracking ReadCloser, then assert its Close method is called for both successful
and non-200 responses. Preserve the existing payload/status assertions while
routing the client through this transport so regressions omitting
resp.Body.Close() are detected.
- Around line 1551-1574: Update
TestFetchMCSIgnitionPayloadMCSOutputIncludedInLog to capture the logger output
used by fetchMCSIgnitionPayload and assert it contains the MCS buffer text when
the request returns non-200; alternatively, rename the test to describe only
buffer preservation if logging cannot be captured.
🪄 Autofix (Beta)
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: Repository YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: 66ccfe58-3f29-4755-9ddd-b69437d75325
📒 Files selected for processing (2)
ignition-server/controllers/local_ignitionprovider.goignition-server/controllers/local_ignitionprovider_test.go
🚧 Files skipped from review as they are similar to previous changes (1)
- ignition-server/controllers/local_ignitionprovider.go
| name: "When MCS returns 200 with payload, it should return the payload", | ||
| handler: func(w http.ResponseWriter, r *http.Request) { | ||
| w.WriteHeader(http.StatusOK) | ||
| _, _ = w.Write([]byte(`{"ignition":{"version":"3.2.0"}}`)) |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
Do not discard test-fixture write errors.
The _, _ = ...Write(...) assignments hide failures in HTTP handlers and buffer setup. Check the returned errors and fail or return from the test fixture when a write fails.
As per coding guidelines: “Always check errors — don’t ignore them.” As per path instructions: “Never ignore error returns.”
Also applies to: 1481-1481, 1493-1493, 1511-1511, 1565-1565
🤖 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 `@ignition-server/controllers/local_ignitionprovider_test.go` at line 1464, The
test fixtures currently discard errors returned by w.Write in the affected
handlers. Update each Write call, including those in the fixtures around the
referenced locations, to capture and check the returned error, failing the test
or returning from the handler when writing fails; do not use blank assignments
to ignore errors.
Sources: Coding guidelines, Path instructions
There was a problem hiding this comment.
Done. Checked and fixed all w.Write error handling in test fixtures: HTTP handler closures now check the error and return, and mcsOutput.Write calls use gomega assertions where available. The goroutine concurrency test (syncBuffer thread-safety test) is left as-is since t.Fatal is unsafe from goroutines and bytes.Buffer.Write cannot error with small writes.
AI-assisted response via Claude Code
There was a problem hiding this comment.
Skipped: comment is from another GitHub bot.
Test Failure Analysis CompleteJob Information
Test Failure AnalysisErrorSummaryThe Root CauseThe 39 uncovered lines (36 missing + 3 partial) all reside within 1. Process setup and lifecycle (~10 lines)
These lines spawn a real 2. HTTP poll callback body (~15 lines)
While the extracted 3. Post-poll cleanup and error wrapping (~7 lines)
These are pure logic operating on channels and strings but are only reachable through the full Why existing tests don't cover these: The PR adds comprehensive tests for the independently testable parts:
The remaining 39 uncovered lines are embedded inside a method that requires an actual filesystem binary ( Coverage arithmetic: The target 43.37% is Codecov's auto-computed threshold from the base branch. The PR achieves 42.64% — a gap of just 0.73 percentage points. Recommendations
Evidence
|
|
/rebase |
|
/test address-review-comments |
|
Review agent triggered. View job |
3255fa6 to
0ca6f46
Compare
|
/restructure-commits |
|
🤖 Restructuring commits: workflow run |
…ors and add unit tests When the MCS HTTP polling fails, the error returned to callers did not include the MCS binary's own logs, making troubleshooting ignition payload failures significantly harder. - Restructure MCS process management to capture stdout/stderr in a thread-safe syncBuffer with 8 KiB truncation, propagating output in both logs and returned error messages - Extract HTTP polling into a standalone fetchMCSIgnitionPayload function for testability - Add cmd.WaitDelay to prevent unbounded blocking from orphaned child processes holding pipe file descriptors - Fix non-OK status code path to log errors with MCS output and close the response body - Add comprehensive unit tests for fetch logic using httptest Signed-off-by: OpenShift CI Bot <ci-bot@redhat.com> Commit-Message-Assisted-by: Claude (via Claude Code) Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
0ca6f46 to
877c3fb
Compare
|
Scheduling tests matching the |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: bryan-cox, enxebre, hypershift-jira-solve-ci[bot] 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 |
|
/test verify |
|
/test e2e-aro-hcp |
1 similar comment
|
/test e2e-aro-hcp |
Empirical Verification of Code Changes via e2e-aws CI RunAnalysis of the e2e-aws job artifacts confirms the PR's modified code paths in 1. PR image confirmed running in ignition-server podsThe ignition-server pod YAML from the TestCreateCluster HCP namespace shows:
2. GetPayload events prove the full code chain executedThe call chain modified by this PR is:
Each event proves: the 3. Per-change verification
4. SummaryAll 16 test suites bootstrapped nodes through ignition-server pods running this PR's image. The |
|
/verified by e2e See #8936 (comment). I think its acceptable to not test those 2 error paths since they are covered by a unit test. |
openshift-ci-robot
added
the
verified
|
@bryan-cox: This PR has been marked as verified by DetailsIn response to this:
Instructions 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 openshift-eng/jira-lifecycle-plugin repository. |
|
/retest-required |
|
/test e2e-aws |
|
/test e2e-aws |
|
@hypershift-jira-solve-ci[bot]: The following test failed, say
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. |
|
/test e2e-v2-gke |
8761071
into
openshift:main
|
@hypershift-jira-solve-ci[bot]: Jira Issue Verification Checks: Jira Issue OCPBUGS-54776 Jira Issue OCPBUGS-54776 has been moved to the MODIFIED state and will move to the VERIFIED state when the change is available in an accepted nightly payload. 🕓 DetailsIn response to this:
Instructions 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 openshift-eng/jira-lifecycle-plugin repository. |
3 participants
What this PR does / why we need it:
When the MCS HTTP polling loop encounters errors (connection failures, unexpected status codes, or body read failures), the log messages now include the MCS process stdout/stderr output to aid debugging.
Changes:
syncBuffertype for thread-safe capture of MCS process outputcmd.Start()+ deferredcmd.Wait()instead of goroutine for process lifecycle managementlog.Error()on non-200 status codessyncBufferincluding concurrent safetyWhich issue(s) this PR fixes:
Fixes https://redhat.atlassian.net/browse/OCPBUGS-54776
Special notes for your reviewer:
Checklist:
Always review AI generated responses prior to use.
Generated with Claude Code via openshift-developer plugin
Summary by CodeRabbit