OCPBUGS-77670: Handle incomplete workspace directory structures gracefully#1464
OCPBUGS-77670: Handle incomplete workspace directory structures gracefully#1464redhat-chai-bot wants to merge 1 commit into
Conversation
When oc-mirror v2 is interrupted (Ctrl+C, disk full, reboot), it can leave incomplete OCI directories under working-dir/release-images/ that are missing critical files like oci-layout and index.json. Subsequent runs fail because ensureReleaseInOCIFormat() only checks if the directory exists (os.Stat) but never validates its contents. Add isValidOCIDirectory() to verify that an OCI directory contains both a valid oci-layout file (with imageLayoutVersion) and a valid index.json file (with schemaVersion). Modify ensureReleaseInOCIFormat() to detect incomplete directories, log a warning, remove the corrupt directory, and re-download the release image. This follows the same pattern already used in the operator catalog handler (catalog_handler.go) for validating OCI directories. Co-Authored-By: Claude Opus 4.6 <[email protected]>
openshift-ci-robot
added
jira/severity-important
|
@redhat-chai-bot: This pull request references Jira Issue OCPBUGS-77670, 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. |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: redhat-chai-bot 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 |
WalkthroughAdds validation of cached OCI-format release directories in the local storage collector. A new ChangesOCI Directory Validation
Estimated code review effort: 2 (Simple) | ~15 minutes Sequence Diagram(s)sequenceDiagram
participant Caller
participant ensureReleaseInOCIFormat
participant isValidOCIDirectory
participant Filesystem
Caller->>ensureReleaseInOCIFormat: request release in OCI format
ensureReleaseInOCIFormat->>Filesystem: check if target directory exists
ensureReleaseInOCIFormat->>isValidOCIDirectory: validate oci-layout and index.json
isValidOCIDirectory->>Filesystem: read oci-layout and index.json
isValidOCIDirectory-->>ensureReleaseInOCIFormat: valid or invalid
alt valid
ensureReleaseInOCIFormat-->>Caller: reuse existing directory
else invalid
ensureReleaseInOCIFormat->>Filesystem: remove directory
ensureReleaseInOCIFormat->>Filesystem: copy release index image
ensureReleaseInOCIFormat-->>Caller: return new directory
end
Suggested reviewers: 🚥 Pre-merge checks | ✅ 15✅ Passed checks (15 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 |
![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
🧹 Nitpick comments (1)
internal/pkg/release/local_stored_collector_test.go (1)
925-959: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDead
mirrorRunvariable doesn't actually verify re-download occurred.
mirrorRunis declared, never settrue, and only referenced via_ = mirrorRunto satisfy the compiler. The comment implies it proves re-download happened, but the test only checks that the directory was recreated and no error occurred — it doesn't confirmMirror.Runwas actually invoked. Compare with the other two subtests, which useMockMirror{Fail: true}to prove/disprove invocation more convincingly.♻️ Suggested cleanup
- mirrorRun := false collector := &LocalStorageCollector{ Log: log, Mirror: &MockMirror{Fail: false}, Opts: mirror.CopyOptions{}, LocalStorageFQDN: "localhost:9999", LogsDir: "/tmp/", } @@ err = collector.ensureReleaseInOCIFormat(context.Background(), release, releaseDir) assert.NoError(t, err) // Confirm the directory was recreated (MkdirAll runs after removal) _, statErr := os.Stat(releaseDir) assert.NoError(t, statErr) - _ = mirrorRun🤖 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 `@internal/pkg/release/local_stored_collector_test.go` around lines 925 - 959, The “incomplete directory is removed and re-downloaded” test uses a dead mirrorRun variable that never proves Mirror.Run was called. Update this subtest around ensureReleaseInOCIFormat and MockMirror so it actually asserts re-download occurred, either by using the mock’s failure/success behavior consistently with the neighboring subtests or by recording invocation on the mock and asserting it. Remove the unused mirrorRun placeholder and keep the assertions focused on observable Mirror.Run execution plus the recreated directory.
🤖 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 `@internal/pkg/release/local_stored_collector.go`:
- Around line 227-266: isValidOCIDirectory is too permissive because it only
checks that oci-layout and index.json are parseable, so incomplete OCI layouts
can still be treated as valid. Tighten the validation in isValidOCIDirectory by
also rejecting empty manifests in index.json and verifying that the blob files
referenced by the index exist under blobs/, so interrupted mirror directories
are not reused and the failure is caught before GetOCIImageFromIndex and
ExtractOCILayers.
---
Nitpick comments:
In `@internal/pkg/release/local_stored_collector_test.go`:
- Around line 925-959: The “incomplete directory is removed and re-downloaded”
test uses a dead mirrorRun variable that never proves Mirror.Run was called.
Update this subtest around ensureReleaseInOCIFormat and MockMirror so it
actually asserts re-download occurred, either by using the mock’s
failure/success behavior consistently with the neighboring subtests or by
recording invocation on the mock and asserting it. Remove the unused mirrorRun
placeholder and keep the assertions focused on observable Mirror.Run execution
plus the recreated directory.
🪄 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: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 7c12f6c1-08a8-49c2-849b-09bdc98dc5c8
📒 Files selected for processing (2)
internal/pkg/release/local_stored_collector.gointernal/pkg/release/local_stored_collector_test.go
| // isValidOCIDirectory checks whether a directory contains a valid OCI image | ||
| // layout by verifying the presence and basic validity of oci-layout and | ||
| // index.json files. This is used to detect partially written directories left | ||
| // behind by interrupted mirror operations (OCPBUGS-77670). | ||
| func isValidOCIDirectory(dir string) bool { | ||
| // Check oci-layout | ||
| ociLayoutPath := filepath.Join(dir, "oci-layout") | ||
| ociLayoutData, err := os.ReadFile(ociLayoutPath) | ||
| if err != nil { | ||
| return false | ||
| } | ||
| var ociLayout struct { | ||
| ImageLayoutVersion string `json:"imageLayoutVersion"` | ||
| } | ||
| if err := json.Unmarshal(ociLayoutData, &ociLayout); err != nil { | ||
| return false | ||
| } | ||
| if ociLayout.ImageLayoutVersion == "" { | ||
| return false | ||
| } | ||
|
|
||
| // Check index.json | ||
| indexPath := filepath.Join(dir, "index.json") | ||
| indexData, err := os.ReadFile(indexPath) | ||
| if err != nil { | ||
| return false | ||
| } | ||
| var index struct { | ||
| SchemaVersion int `json:"schemaVersion"` | ||
| } | ||
| if err := json.Unmarshal(indexData, &index); err != nil { | ||
| return false | ||
| } | ||
| if index.SchemaVersion == 0 { | ||
| return false | ||
| } | ||
|
|
||
| return true | ||
| } | ||
|
|
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n## file outline\n'
ast-grep outline internal/pkg/release/local_stored_collector.go --view expanded || true
printf '\n## relevant symbols\n'
rg -n "isValidOCIDirectory|GetOCIImageFromIndex|ExtractOCILayers|ensureReleaseInOCIFormat|oci-layout|index.json|manifests|blobs/" internal/pkg/release -n
printf '\n## relevant slices\n'
sed -n '180,320p' internal/pkg/release/local_stored_collector.go
sed -n '1,260p' internal/pkg/release/*.goRepository: openshift/oc-mirror
Length of output: 24474
Validation still misses incomplete OCI layouts isValidOCIDirectory accepts directories with only a parseable oci-layout and index.json, so an interrupted mirror can still be reused even when index.json has no manifests or the referenced blobs are missing. That defers the failure to GetOCIImageFromIndex/ExtractOCILayers instead of forcing a re-download here. Consider rejecting empty manifests and checking the referenced blob paths under blobs/.
🤖 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 `@internal/pkg/release/local_stored_collector.go` around lines 227 - 266,
isValidOCIDirectory is too permissive because it only checks that oci-layout and
index.json are parseable, so incomplete OCI layouts can still be treated as
valid. Tighten the validation in isValidOCIDirectory by also rejecting empty
manifests in index.json and verifying that the blob files referenced by the
index exist under blobs/, so interrupted mirror directories are not reused and
the failure is caught before GetOCIImageFromIndex and ExtractOCILayers.
|
@redhat-chai-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. |
2 participants
Summary
Fixes OCPBUGS-77670 — Mirror process sometimes failing to mirror and ending abruptly.
Problem
When an
oc-mirrorrun is interrupted (e.g. via Ctrl+C, disk full, reboot), incomplete directory structures are left in theoc-mirror-workspace/working-dir/release-images/tree. On subsequent runs, oc-mirror encounters these incomplete directories (missingoci-layout,index.json, or blob files) and fails instead of recovering.Expected Behavior
oc-mirror should detect incomplete workspace directory structures and self-heal by re-downloading the affected content, rather than failing the entire mirror process.
Changes
Handles incomplete workspace directory structures gracefully so that interrupted mirror operations do not leave the workspace in an unrecoverable state.
Summary by CodeRabbit