MGMT-23802: Fix manifest format in log bundles#10121
MGMT-23802: Fix manifest format in log bundles#10121zaneb wants to merge 1 commit intoopenshift:masterfrom
Conversation
Manifests included in cluster log bundles were being JSON-encoded as strings instead of preserving their original YAML format. This occurred because uploadDataAsFile() JSON-marshals all inputs, which is correct for cluster metadata and events but inappropriate for manifests that are already serialized YAML. This change bypasses uploadDataAsFile() and directly uploads manifest content using objectHandler.Upload(), preserving the original YAML format in downloaded log bundles. Assisted-by: Claude Code
openshift-ci-robot
added
the
jira/valid-reference
|
@zaneb: This pull request references MGMT-23802 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the bug to target the "4.22.0" version, but no target version was set. 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. |
WalkthroughThe Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
openshift-ci
bot
added
the
size/XS
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: zaneb 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.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
internal/cluster/cluster.go (1)
1365-1376:⚠️ Potential issue | 🟠 MajorClose the downloaded manifest reader to prevent handle leaks.
Downloadreturns anio.ReadCloser, butresponseis never closed in this loop. The reader leaks across all code paths: both whenReadAllfails (line 1373) and when it succeeds (before the next iteration). On clusters with many manifests, this exhausts available file/HTTP handles.Suggested fix
- fileContent, err := io.ReadAll(response) + fileContent, err := func() ([]byte, error) { + defer func() { + if closeErr := response.Close(); closeErr != nil { + log.WithError(closeErr).Warnf("failed to close file %s from cluster while building log: %s", path, *c.ID) + } + }() + return io.ReadAll(response) + }() if err != nil { log.WithError(err).Errorf("failed to read file data %s (%d) from cluster while building log: %s", path, numberOfBytes, *c.ID) continue }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/cluster/cluster.go` around lines 1365 - 1376, The Download call (m.objectHandler.Download) returns an io.ReadCloser (response) which is never closed; update the loop to close response on all paths by calling response.Close() — e.g., after a successful non-nil response do defer response.Close() immediately (or ensure explicit Close() in both the error branch after io.ReadAll fails and after the successful Upload) so the reader is released whether io.ReadAll fails or succeeds; keep references to response, io.ReadAll, and objectHandler.Upload when adding the close to ensure no handle leaks.
🧹 Nitpick comments (1)
internal/cluster/cluster.go (1)
1361-1376: Use the same storage handler for download and upload.This block checks existence and uploads via
objectHandler, but still downloads viam.objectHandler. If a caller passes a non-default handler, the copy path spans two backends.Suggested fix
- response, numberOfBytes, err := m.objectHandler.Download(ctx, objectName) + response, numberOfBytes, err := objectHandler.Download(ctx, objectName)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/cluster/cluster.go` around lines 1361 - 1376, The code uses objectHandler for DoesObjectExist and Upload but calls m.objectHandler.Download, which can mix backends; change the download call to use the same handler variable (objectHandler) so all operations (DoesObjectExist, Download, Upload) use the provided handler; specifically replace the m.objectHandler.Download(...) invocation with objectHandler.Download(...) and ensure any error logging/variables (response, numberOfBytes, err) remain unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@internal/cluster/cluster.go`:
- Around line 1365-1376: The Download call (m.objectHandler.Download) returns an
io.ReadCloser (response) which is never closed; update the loop to close
response on all paths by calling response.Close() — e.g., after a successful
non-nil response do defer response.Close() immediately (or ensure explicit
Close() in both the error branch after io.ReadAll fails and after the successful
Upload) so the reader is released whether io.ReadAll fails or succeeds; keep
references to response, io.ReadAll, and objectHandler.Upload when adding the
close to ensure no handle leaks.
---
Nitpick comments:
In `@internal/cluster/cluster.go`:
- Around line 1361-1376: The code uses objectHandler for DoesObjectExist and
Upload but calls m.objectHandler.Download, which can mix backends; change the
download call to use the same handler variable (objectHandler) so all operations
(DoesObjectExist, Download, Upload) use the provided handler; specifically
replace the m.objectHandler.Download(...) invocation with
objectHandler.Download(...) and ensure any error logging/variables (response,
numberOfBytes, err) remain unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 62de1fc7-a457-41ad-855b-df4573d1a457
📒 Files selected for processing (1)
internal/cluster/cluster.go
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #10121 +/- ##
=======================================
Coverage 44.24% 44.25%
=======================================
Files 415 415
Lines 72499 72499
=======================================
+ Hits 32079 32083 +4
+ Misses 37509 37507 -2
+ Partials 2911 2909 -2
🚀 New features to boost your workflow:
|
|
@zaneb: 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. |
2 participants
Manifests included in cluster log bundles were being JSON-encoded as strings instead of preserving their original YAML format. This occurred because uploadDataAsFile() JSON-marshals all inputs, which is correct for cluster metadata and events but inappropriate for manifests that are already serialized YAML.
This change bypasses uploadDataAsFile() and directly uploads manifest content using objectHandler.Upload(), preserving the original YAML format in downloaded log bundles.
List all the issues related to this PR
What environments does this code impact?
How was this code tested?
Checklist
docs, README, etc)Reviewers Checklist