Skip to content

OCPBUGS-77670: Handle incomplete workspace directory structures gracefully #1464

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
redhat-chai-bot wants to merge 1 commit into
base: main
Choose a base branch
from
+216 −2
Open

OCPBUGS-77670: Handle incomplete workspace directory structures gracefully #1464

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
54 changes: 52 additions & 2 deletions internal/pkg/release/local_stored_collector.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@ package release

import (
"context"
"encoding/json"
"fmt"
"io"
"os"
Expand Down Expand Up @@ -190,8 +191,17 @@ func (o *LocalStorageCollector) collectReleaseImages(ctx context.Context, releas
func (o *LocalStorageCollector) ensureReleaseInOCIFormat(ctx context.Context, release v2alpha1.CopyImageSchema, dir string) error {
_, err := os.Stat(dir)
if err == nil {
o.Log.Debug(collectorPrefix+"release-images index directory alredy exists %s", dir)
return nil
// OCPBUGS-77670: directory exists, but may be incomplete from a previous
// interrupted run (Ctrl+C, disk full, reboot). Validate that the OCI
// layout is complete before reusing.
if isValidOCIDirectory(dir) {
o.Log.Debug(collectorPrefix+"release-images index directory already exists %s", dir)
return nil
}
o.Log.Warn(collectorPrefix+"detected incomplete OCI directory %s, removing and re-downloading", dir)
if err := os.RemoveAll(dir); err != nil {
return fmt.Errorf("remove incomplete OCI directory %s: %w", dir, err)
}
}
o.Log.Debug(collectorPrefix+"copying release image %s ", release.Source)
if err := os.MkdirAll(dir, 0o755); err != nil {
Expand All @@ -214,6 +224,46 @@ func (o *LocalStorageCollector) ensureReleaseInOCIFormat(ctx context.Context, re
return nil
}

// 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
}

Comment on lines +227 to +266

@coderabbitai coderabbitai Bot Jul 6, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🎯 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/*.go

Repository: 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.

// collects release images from the disk
// all errors will be propagated to the caller; no redundant error logging to console
func (o *LocalStorageCollector) collectImageFromDisk(ctx context.Context) ([]v2alpha1.CopyImageSchema, error) {
Expand Down
164 changes: 164 additions & 0 deletions internal/pkg/release/local_stored_collector_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@ package release
import (
"context"
"fmt"
"os"
"path/filepath"
"strings"
"testing"
Expand Down Expand Up @@ -841,3 +842,166 @@ func (o *ManifestMock) ImageDigest(ctx context.Context, sourceCtx *types.SystemC
func (o *ManifestMock) ImageManifest(ctx context.Context, sourceCtx *types.SystemContext, imgRef string, instanceDigest *digest.Digest) ([]byte, string, error) {
return nil, "", nil
}

// createValidOCIDir creates a minimal valid OCI directory structure for testing.
func createValidOCIDir(t *testing.T, dir string) {
t.Helper()
err := os.MkdirAll(dir, 0o755)
assert.NoError(t, err)
err = os.WriteFile(filepath.Join(dir, "oci-layout"), []byte(`{"imageLayoutVersion":"1.0.0"}`), 0o644)
assert.NoError(t, err)
err = os.WriteFile(filepath.Join(dir, "index.json"), []byte(`{"schemaVersion":2,"manifests":[]}`), 0o644)
assert.NoError(t, err)
}

func TestIsValidOCIDirectory(t *testing.T) {
t.Run("valid OCI directory", func(t *testing.T) {
dir := t.TempDir()
createValidOCIDir(t, dir)
assert.True(t, isValidOCIDirectory(dir))
})

t.Run("non-existent directory", func(t *testing.T) {
assert.False(t, isValidOCIDirectory(filepath.Join(t.TempDir(), "does-not-exist")))
})

t.Run("empty directory", func(t *testing.T) {
dir := t.TempDir()
assert.False(t, isValidOCIDirectory(dir))
})

t.Run("missing oci-layout", func(t *testing.T) {
dir := t.TempDir()
err := os.WriteFile(filepath.Join(dir, "index.json"), []byte(`{"schemaVersion":2,"manifests":[]}`), 0o644)
assert.NoError(t, err)
assert.False(t, isValidOCIDirectory(dir))
})

t.Run("missing index.json", func(t *testing.T) {
dir := t.TempDir()
err := os.WriteFile(filepath.Join(dir, "oci-layout"), []byte(`{"imageLayoutVersion":"1.0.0"}`), 0o644)
assert.NoError(t, err)
assert.False(t, isValidOCIDirectory(dir))
})

t.Run("malformed oci-layout", func(t *testing.T) {
dir := t.TempDir()
err := os.WriteFile(filepath.Join(dir, "oci-layout"), []byte(`not valid json`), 0o644)
assert.NoError(t, err)
err = os.WriteFile(filepath.Join(dir, "index.json"), []byte(`{"schemaVersion":2,"manifests":[]}`), 0o644)
assert.NoError(t, err)
assert.False(t, isValidOCIDirectory(dir))
})

t.Run("malformed index.json", func(t *testing.T) {
dir := t.TempDir()
err := os.WriteFile(filepath.Join(dir, "oci-layout"), []byte(`{"imageLayoutVersion":"1.0.0"}`), 0o644)
assert.NoError(t, err)
err = os.WriteFile(filepath.Join(dir, "index.json"), []byte(`{truncated`), 0o644)
assert.NoError(t, err)
assert.False(t, isValidOCIDirectory(dir))
})

t.Run("oci-layout missing imageLayoutVersion", func(t *testing.T) {
dir := t.TempDir()
err := os.WriteFile(filepath.Join(dir, "oci-layout"), []byte(`{"other":"field"}`), 0o644)
assert.NoError(t, err)
err = os.WriteFile(filepath.Join(dir, "index.json"), []byte(`{"schemaVersion":2,"manifests":[]}`), 0o644)
assert.NoError(t, err)
assert.False(t, isValidOCIDirectory(dir))
})

t.Run("index.json missing schemaVersion", func(t *testing.T) {
dir := t.TempDir()
err := os.WriteFile(filepath.Join(dir, "oci-layout"), []byte(`{"imageLayoutVersion":"1.0.0"}`), 0o644)
assert.NoError(t, err)
err = os.WriteFile(filepath.Join(dir, "index.json"), []byte(`{"manifests":[]}`), 0o644)
assert.NoError(t, err)
assert.False(t, isValidOCIDirectory(dir))
})
}

func TestEnsureReleaseInOCIFormat_IncompleteDir(t *testing.T) {
t.Run("incomplete directory is removed and re-downloaded", func(t *testing.T) {
log := clog.New("trace")
tempDir := t.TempDir()

// Create an incomplete release-images directory (missing oci-layout and index.json)
releaseDir := filepath.Join(tempDir, "release-images", "ocp-release", "4.13.10-x86_64")
err := os.MkdirAll(releaseDir, 0o755)
assert.NoError(t, err)

// Verify the directory exists but is invalid
assert.False(t, isValidOCIDirectory(releaseDir))

mirrorRun := false
collector := &LocalStorageCollector{
Log: log,
Mirror: &MockMirror{Fail: false},
Opts: mirror.CopyOptions{},
LocalStorageFQDN: "localhost:9999",
LogsDir: "/tmp/",
}

release := v2alpha1.CopyImageSchema{
Source: "quay.io/openshift-release-dev/ocp-release:4.13.10-x86_64",
}

// The MockMirror.Run will succeed (returns nil), proving re-download was attempted.
// After ensureReleaseInOCIFormat, the directory should have been removed (by our fix)
// and then recreated by MkdirAll inside the function.
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
})

t.Run("valid directory is not re-downloaded", func(t *testing.T) {
log := clog.New("trace")
releaseDir := filepath.Join(t.TempDir(), "release-images", "ocp-release", "4.13.10-x86_64")
createValidOCIDir(t, releaseDir)

// Use a mirror that would fail if called - proving it's NOT called
collector := &LocalStorageCollector{
Log: log,
Mirror: &MockMirror{Fail: true},
Opts: mirror.CopyOptions{},
LocalStorageFQDN: "localhost:9999",
LogsDir: "/tmp/",
}

release := v2alpha1.CopyImageSchema{
Source: "quay.io/openshift-release-dev/ocp-release:4.13.10-x86_64",
}

err := collector.ensureReleaseInOCIFormat(context.Background(), release, releaseDir)
assert.NoError(t, err)
// Directory should still exist and be valid
assert.True(t, isValidOCIDirectory(releaseDir))
})

t.Run("non-existent directory triggers download", func(t *testing.T) {
log := clog.New("trace")
releaseDir := filepath.Join(t.TempDir(), "release-images", "ocp-release", "4.13.10-x86_64")

collector := &LocalStorageCollector{
Log: log,
Mirror: &MockMirror{Fail: false},
Opts: mirror.CopyOptions{},
LocalStorageFQDN: "localhost:9999",
LogsDir: "/tmp/",
}

release := v2alpha1.CopyImageSchema{
Source: "quay.io/openshift-release-dev/ocp-release:4.13.10-x86_64",
}

err := collector.ensureReleaseInOCIFormat(context.Background(), release, releaseDir)
assert.NoError(t, err)
// Directory should have been created
_, statErr := os.Stat(releaseDir)
assert.NoError(t, statErr)
})
}