OCPBUGS-14392: make sync status Failure logs human-readable - #1440
OCPBUGS-14392: make sync status Failure logs human-readable#1440pdudley wants to merge 1 commit into
Conversation
openshift-ci-robot
added
jira/severity-low
|
@pdudley: This pull request references Jira Issue OCPBUGS-14392, 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. |
|
Warning Review limit reached
Next review available in: 23 minutes You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Repository YAML (base), Central YAML (inherited) Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (3)
WalkthroughThe change adds deterministic ChangesSync status formatting
Estimated code review effort: 2 (Simple) | ~10 minutes Important Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional. ❌ Failed checks (1 error)
✅ Passed checks (14 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: pdudley The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
![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
🤖 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 `@pkg/cvo/sync_worker.go`:
- Around line 164-176: Update SyncWorkerStatus.String() to emit only
allow-listed, redacted representations for Actual.Image and failure text instead
of raw values. Ensure the sanitized String() output is used unchanged at the
klog call sites in pkg/cvo/sync_worker.go lines 742, 944-950, and 972-978, and
pkg/cvo/status.go line 165; the formatter site in pkg/cvo/sync_worker.go lines
164-176 requires the direct change.
🪄 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: Repository YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Pro Plus
Run ID: 40775fd3-6c08-4067-8e9f-e604a7bedf34
📒 Files selected for processing (3)
pkg/cvo/status.gopkg/cvo/sync_worker.gopkg/cvo/sync_worker_status_string_test.go
| func (w SyncWorkerStatus) String() string { | ||
| failure := "<none>" | ||
| if w.Failure != nil { | ||
| failure = w.Failure.Error() | ||
| } | ||
| loadFailure := "<none>" | ||
| if w.loadPayloadStatus.Failure != nil { | ||
| loadFailure = w.loadPayloadStatus.Failure.Error() | ||
| } | ||
| return fmt.Sprintf("generation=%d done=%d/%d completed=%d reconciling=%t initial=%t version=%q image=%q failure=%q loadPayloadStep=%q loadPayloadFailure=%q", | ||
| w.Generation, w.Done, w.Total, w.Completed, w.Reconciling, w.Initial, | ||
| w.Actual.Version, w.Actual.Image, failure, | ||
| w.loadPayloadStatus.Step, loadFailure) |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | 🏗️ Heavy lift
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== files =="
git ls-files | rg '(^|/)pkg/cvo/(sync_worker|status)\.go$|sync_worker_status_string_test\.go$' || true
echo "== sync_worker relevant sections =="
wc -l pkg/cvo/sync_worker.go pkg/cvo/status.go pkg/cvo/sync_worker_status_string_test.go 2>/dev/null || true
sed -n '130,190p' pkg/cvo/sync_worker.go
sed -n '720,760p' pkg/cvo/sync_worker.go
sed -n '920,990p' pkg/cvo/sync_worker.go
sed -n '150,175p' pkg/cvo/status.go
echo "== Search for Image/Failure/Error logging around status formatter =="
rg -n "Actual\.Image|\.Failure|loadPayloadStatus\.Failure|fmt\.Sprintf|Infof|Status" pkg/cvo/sync_worker.go pkg/cvo/status.go -C 2
echo "== Imports and possible redaction helpers =="
sed -n '1,80p' pkg/cvo/sync_worker.go
rg -n "func .*Sanit|redact|sanitize|scrub|Replace|Mask|Secret|NoProxy|Proxy|Authorization|Bearer|password|token|key" pkg/cvo . --glob '*.go' -C 1 | head -n 200Repository: openshift/cluster-version-operator
Length of output: 50390
Information Disclosure (CWE-532): Insertion of Sensitive Information into Log File
Reachability: Internal
Reachability path
● Entry
pkg/cvo/sync_worker_status_string_test.go:14
TestSyncWorkerStatus_String: Contrast with %#v, which is the old buggy log format.
│
▼
● Sink
pkg/cvo/sync_worker.go
Redact sensitive fields in SyncWorkerStatus.String() before klog writes them.
String() returns Actual.Image and unfiltered failure text; the klog sites log this shared string unchanged. Use an allow-listed, redacted image and failure representation in the formatter and pass that sanitized status into the klog call sites in pkg/cvo/sync_worker.go and pkg/cvo/status.go.
📍 Affects 2 files
pkg/cvo/sync_worker.go#L164-L176(this comment)pkg/cvo/sync_worker.go#L742-L742pkg/cvo/sync_worker.go#L944-L950pkg/cvo/sync_worker.go#L972-L978pkg/cvo/status.go#L165-L165
🤖 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 `@pkg/cvo/sync_worker.go` around lines 164 - 176, Update
SyncWorkerStatus.String() to emit only allow-listed, redacted representations
for Actual.Image and failure text instead of raw values. Ensure the sanitized
String() output is used unchanged at the klog call sites in
pkg/cvo/sync_worker.go lines 742, 944-950, and 972-978, and pkg/cvo/status.go
line 165; the formatter site in pkg/cvo/sync_worker.go lines 164-176 requires
the direct change.
Source: Coding guidelines
|
/jira refresh |
openshift-ci-robot
added
jira/valid-bug
|
@pdudley: This pull request references Jira Issue OCPBUGS-14392, which is valid. The bug has been moved to the POST state. 3 validation(s) were run on this bug
No GitHub users were found matching the public email listed for the QA contact in Jira (jiajliu@redhat.com), skipping review request. 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. |
Replace %#v dumps of SyncWorkerStatus with curated String() output so Failure and loadPayloadStatus.Failure show error text instead of pointer addresses (OCPBUGS-14392). Signed-off-by: Paul Dudley <pdudley@redhat.com>
|
/retest |
|
@pdudley: 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. |
3 participants
Summary
Synchronizing status(and related sync-worker status dumps) so nestedFailurevalues print error text instead of(*payload.UpdateError)(0x…)under%#v.SyncWorkerStatus.String()/LoadPayloadStatus.String()and switch those log lines to%s/%v.loadPayloadStep/loadPayloadFailureso payload-load errors on the unexportedloadPayloadStatusare readable in the same log line.Resolves https://issues.redhat.com/browse/OCPBUGS-14392
Example
Before:
After:
Note on verbosity
String()intentionally omits some fields previously visible only in%#vdumps (e.g.VersionHash, capabilities, feature gates, full load-payload message). V(6) sites use the same format after this change.Summary by CodeRabbit