Skip to content

NO-ISSUE: Fix resource leak and inconsistency in manifest download#10322

Open
zaneb wants to merge 1 commit into
openshift:masterfrom
zaneb:log-download-close
Open

NO-ISSUE: Fix resource leak and inconsistency in manifest download#10322
zaneb wants to merge 1 commit into
openshift:masterfrom
zaneb:log-download-close

Conversation

@zaneb
Copy link
Copy Markdown
Member

@zaneb zaneb commented May 12, 2026

Fix two issues identified by Claude Code in createClusterDataFiles():

  1. Close the io.ReadCloser returned by Download() to prevent resource leaks when processing multiple manifests
  2. Use the objectHandler parameter consistently instead of mixing m.objectHandler and objectHandler for different operations

The response body is closed immediately after io.ReadAll() rather than using defer, since this occurs in a loop and defer would accumulate all closes until function return.

Summary by CodeRabbit

  • Bug Fixes
    • Fixed resource management for cluster manifest downloads from object storage, ensuring proper cleanup of download streams to prevent potential leaks.

Fix two issues in createClusterDataFiles():

1. Close the io.ReadCloser returned by Download() to prevent
   resource leaks when processing multiple manifests
2. Use the objectHandler parameter consistently instead of mixing
   m.objectHandler and objectHandler for different operations

The response body is closed immediately after io.ReadAll() rather
than using defer, since this occurs in a loop and defer would
accumulate all closes until function return.

Assisted-by: Claude Code
@openshift-ci-robot openshift-ci-robot added the jira/valid-reference Indicates that this PR references a valid Jira ticket of any type. label May 12, 2026
@openshift-ci-robot
Copy link
Copy Markdown

@zaneb: This pull request explicitly references no jira issue.

Details

In response to this:

Fix two issues identified by Claude Code in createClusterDataFiles():

  1. Close the io.ReadCloser returned by Download() to prevent resource leaks when processing multiple manifests
  2. Use the objectHandler parameter consistently instead of mixing m.objectHandler and objectHandler for different operations

The response body is closed immediately after io.ReadAll() rather than using defer, since this occurs in a loop and defer would accumulate all closes until function return.

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.

@coderabbitai
Copy link
Copy Markdown

coderabbitai Bot commented May 12, 2026

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Repository: openshift/coderabbit/.coderabbit.yaml

Review profile: CHILL

Plan: Enterprise

Run ID: 67a80e96-13ff-4ade-9376-48e5570f8ea9

📥 Commits

Reviewing files that changed from the base of the PR and between babc11d and 6347b5c.

📒 Files selected for processing (1)
  • internal/cluster/cluster.go

Walkthrough

The createClusterDataFiles function in cluster.go now uses the passed-in objectHandler parameter for manifest downloads instead of the receiver's member field, and explicitly closes the download response after reading file content for proper resource cleanup.

Changes

Manifest Download Resource Management

Layer / File(s) Summary
Manifest download and response cleanup
internal/cluster/cluster.go
Updated manifest file download to use the passed-in objectHandler parameter instead of m.objectHandler, and added explicit response.Close() after reading file content to ensure S3 stream resources are properly released.

Estimated code review effort

🎯 1 (Trivial) | ⏱️ ~2 minutes


Important

Pre-merge checks failed

Please resolve all errors before merging. Addressing warnings is optional.

❌ Failed checks (1 error)

Check name Status Explanation Resolution
Stable And Deterministic Test Names ❌ Error Test at internal/cluster/cluster_test.go:4413 uses fmt.Sprintf to construct test name dynamically: It(fmt.Sprintf("success for stage '%s'", stage), func(). Violates requirement for stable test names. Use static test name instead of fmt.Sprintf. Replace with It("successfully updates finalizing stage") and move stage-specific logic to test body.
✅ Passed checks (11 passed)
Check name Status Explanation
Title check ✅ Passed The title clearly and specifically describes the main changes: fixing a resource leak and inconsistency in manifest downloading, which matches the file-level summary and PR objectives.
Description check ✅ Passed The description addresses the required template sections with meaningful content about the bug fix, testing approach, and implementation rationale, though some checklist items remain incomplete.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
Test Structure And Quality ✅ Passed Check not applicable. PR only modifies production code (internal/cluster/cluster.go); no Ginkgo test files are changed. The custom check specifically targets test code review.
Microshift Test Compatibility ✅ Passed This PR does not add any Ginkgo e2e tests. It modifies backend code in internal/cluster/cluster.go. The check is not applicable.
Single Node Openshift (Sno) Test Compatibility ✅ Passed This PR does not add any Ginkgo e2e tests. It fixes a resource leak in internal/cluster/cluster.go. The SNO Test Compatibility check only applies when new Ginkgo tests are added.
Topology-Aware Scheduling Compatibility ✅ Passed PR fixes resource leaks in internal log collection utility. Does not modify deployment manifests, operator code, or introduce scheduling constraints. Topology check not applicable.
Ote Binary Stdout Contract ✅ Passed The PR modifies internal/cluster/cluster.go, a regular service package not a test file. No process-level entry points or stdout writes. The OTE Binary Stdout Contract check is inapplicable here.
Ipv6 And Disconnected Network Test Compatibility ✅ Passed This PR only modifies internal/cluster/cluster.go to fix resource leaks. It does not add any new Ginkgo e2e tests, so the IPv6/disconnected test check is not applicable.
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

Tip

💬 Introducing Slack Agent: The best way for teams to turn conversations into code.

Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.

  • Generate code and open pull requests
  • Plan features and break down work
  • Investigate incidents and troubleshoot customer tickets together
  • Automate recurring tasks and respond to alerts with triggers
  • Summarize progress and report instantly

Built for teams:

  • Shared memory across your entire org—no repeating context
  • Per-thread sandboxes to safely plan and execute work
  • Governance built-in—scoped access, auditability, and budget controls

One agent for your entire SDLC. Right inside Slack.

👉 Get started


Comment @coderabbitai help to get the list of available commands and usage tips.

@openshift-ci openshift-ci Bot added the size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. label May 12, 2026
@openshift-ci openshift-ci Bot requested review from jhernand and tsorya May 12, 2026 21:02
@openshift-ci
Copy link
Copy Markdown

openshift-ci Bot commented May 12, 2026

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: zaneb
Once this PR has been reviewed and has the lgtm label, please assign adriengentil for approval. For more information see the Code Review Process.

The full list of commands accepted by this bot can be found here.

Details Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@codecov
Copy link
Copy Markdown

codecov Bot commented May 12, 2026

Codecov Report

❌ Patch coverage is 0% with 2 lines in your changes missing coverage. Please review.
✅ Project coverage is 44.33%. Comparing base (babc11d) to head (6347b5c).

Files with missing lines Patch % Lines
internal/cluster/cluster.go 0.00% 2 Missing ⚠️
Additional details and impacted files

Impacted file tree graph

@@           Coverage Diff           @@
##           master   #10322   +/-   ##
=======================================
  Coverage   44.33%   44.33%           
=======================================
  Files         417      417           
  Lines       72837    72838    +1     
=======================================
+ Hits        32294    32295    +1     
  Misses      37609    37609           
  Partials     2934     2934           
Files with missing lines Coverage Δ
internal/cluster/cluster.go 66.43% <0.00%> (-0.06%) ⬇️

... and 2 files with indirect coverage changes

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@openshift-ci
Copy link
Copy Markdown

openshift-ci Bot commented May 13, 2026

@zaneb: The following tests failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name Commit Details Required Rerun command
ci/prow/edge-e2e-ai-operator-ztp 6347b5c link true /test edge-e2e-ai-operator-ztp
ci/prow/edge-verify-generated-code 6347b5c link true /test edge-verify-generated-code
ci/prow/verify-generated-code 6347b5c link true /test verify-generated-code
ci/prow/edge-e2e-metal-assisted-4-22 6347b5c link true /test edge-e2e-metal-assisted-4-22

Full PR test history. Your PR dashboard.

Details

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 kubernetes-sigs/prow repository. I understand the commands that are listed here.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

jira/valid-reference Indicates that this PR references a valid Jira ticket of any type. size/XS Denotes a PR that changes 0-9 lines, ignoring generated files.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants