Skip to content

OCPBUGS-61465: Updated openshift-tests images to utilize mapped images from external binary #30873

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
jubittajohn wants to merge 2 commits into
base: main
Choose a base branch
from
Open

OCPBUGS-61465: Updated openshift-tests images to utilize mapped images from external binary #30873

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
47ee916
return raw image data with mapped field from external binaries
jubittajohn Mar 12, 2026
47b3556
updated openshift-tests images to utilize mapped images from external…
jubittajohn Mar 12, 2026
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
116 changes: 88 additions & 28 deletions pkg/cmd/openshift-tests/images/images_command.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@ import (
"strings"
"time"

"github.com/openshift-eng/openshift-tests-extension/pkg/extension"
"github.com/sirupsen/logrus"
"golang.org/x/exp/slices"
k8simage "k8s.io/kubernetes/test/utils/image"
Expand Down Expand Up @@ -138,12 +139,34 @@ type imagesOptions struct {
func createImageMirrorForInternalImages(prefix string, ref reference.DockerImageReference, mirrored bool) ([]string, error) {
source := ref.Exact()

initialImageSets := []extensions.ImageSet{
k8simage.GetOriginalImageConfigs(),
}
// initialImageSets contains all the original images discovered either from
// internal image configs or from external test binaries.
initialImageSets := []extensions.ImageSet{}

// defaultImageSets contains the subset of initialImageSets after filtering
// out images whose E2E image matches any entry in the exceptions list.
defaultImageSets := []extensions.ImageSet{}

// If ENV is not set, the list of images should come from external binaries
if len(os.Getenv("OPENSHIFT_SKIP_EXTERNAL_TESTS")) == 0 {
// updatedImageSets contains the mapped versions of images from defaultImageSets
// (e.g., images rewritten to point to the mirror location).
updatedImageSets := []extensions.ImageSet{}

// exceptions is a list of images we don't mirror temporarily due to various problems.
exceptions := image.Exceptions.List()

// If the ENV is set, then list the images used come from the vendored tests
if len(os.Getenv("OPENSHIFT_SKIP_EXTERNAL_TESTS")) != 0 {
initialImageSets = []extensions.ImageSet{
k8simage.GetOriginalImageConfigs(),
}

defaultImageSets = filterExceptionsFromImageSets(initialImageSets)

for i := range defaultImageSets {
updatedImageSets = append(updatedImageSets, k8simage.GetMappedImageConfigs(defaultImageSets[i], ref.Exact()))
}
} else {
// If ENV is not set, the list of images should come from external binaries
// Extract all test binaries
extractionContext, extractionContextCancel := context.WithTimeout(context.Background(), 30*time.Minute)
defer extractionContextCancel()
Expand All @@ -156,39 +179,37 @@ func createImageMirrorForInternalImages(prefix string, ref reference.DockerImage
// List test images from all available binaries
listContext, listContextCancel := context.WithTimeout(context.Background(), time.Minute)
defer listContextCancel()
imageSetsFromBinaries, err := externalBinaries.ListImages(listContext, 10)
imagesFromBinaries, err := externalBinaries.ListImages(listContext, 10)
if err != nil {
return nil, err
}
if len(imageSetsFromBinaries) == 0 {
if len(imagesFromBinaries) == 0 {
return nil, fmt.Errorf("no test images were reported by external binaries")
}
initialImageSets = imageSetsFromBinaries
}

// Take the initial images coming from external binaries and remove any exceptions that might exist.
exceptions := image.Exceptions.List()
defaultImageSets := []extensions.ImageSet{}
for i := range initialImageSets {
filtered := extensions.ImageSet{}
for imageID, imageConfig := range initialImageSets[i] {
if !slices.ContainsFunc(exceptions, func(e string) bool {
return strings.Contains(imageConfig.GetE2EImage(), e)
}) {
filtered[imageID] = imageConfig
for _, image := range imagesFromBinaries {
// Add original image
imageID := k8simage.ImageID(image.Index)
origImageConfig := convertImageToImageConfig(image)

initialSet := extensions.ImageSet{imageID: origImageConfig}
initialImageSets = append(initialImageSets, initialSet)

// Only add to default and mapped if original image passes exceptions
if !imageMatchesExceptions(origImageConfig, exceptions) {
defaultImageSets = append(defaultImageSets, initialSet)

if image.Mapped != nil {
mappedSet := extensions.ImageSet{
imageID: convertImageToImageConfig(*image.Mapped),
}
updatedImageSets = append(updatedImageSets, mappedSet)
}
}
}
if len(filtered) > 0 {
defaultImageSets = append(defaultImageSets, filtered)
}
}

// Created a new slice with the updatedImageSets addresses for the images
updatedImageSets := []extensions.ImageSet{}
for i := range defaultImageSets {
updatedImageSets = append(updatedImageSets, k8simage.GetMappedImageConfigs(defaultImageSets[i], ref.Exact()))
}
Comment on lines +190 to 210
Copy link

@coderabbitai coderabbitai bot Mar 12, 2026

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟠 Major

Potential index mismatch between defaultImageSets and updatedImageSets.

When image.Mapped is nil (Line 202), the image is added to defaultImageSets but not to updatedImageSets. This creates a size mismatch between the two slices.

Later at Lines 270-272, the code iterates over updatedImageSets using index i and accesses defaultImageSets[i][imageID]:

for i := range updatedImageSets {
    for imageID := range updatedImageSets[i] {
        a, b := defaultImageSets[i][imageID], updatedImageSets[i][imageID]

If defaultImageSets has more entries than updatedImageSets (because some images lack Mapped data), accessing defaultImageSets[i] won't correspond to the correct original image for updatedImageSets[i].

Consider either:

  1. Always adding an entry to updatedImageSets (using the original config as a fallback when Mapped is nil), or
  2. Using a different data structure that maintains the relationship between original and mapped images.
Proposed fix: Always populate updatedImageSets 
 			// Only add to default and mapped if original image passes exceptions
 			if !imageMatchesExceptions(origImageConfig, exceptions) {
 				defaultImageSets = append(defaultImageSets, initialSet)
 
 				if image.Mapped != nil {
 					mappedSet := extensions.ImageSet{
 						imageID: convertImageToImageConfig(*image.Mapped),
 					}
 					updatedImageSets = append(updatedImageSets, mappedSet)
+				} else {
+					// Use original config as fallback when no mapping exists
+					updatedImageSets = append(updatedImageSets, initialSet)
 				}
 			}
🤖 Prompt for AI Agents 
Verify each finding against the current code and only fix it if needed.

In `@pkg/cmd/openshift-tests/images/images_command.go` around lines 190 - 210, The
issue is that defaultImageSets and updatedImageSets can diverge because when
image.Mapped is nil you append to defaultImageSets but skip updatedImageSets;
fix by always appending an entry to updatedImageSets for each defaultImageSets
entry: inside the loop over imagesFromBinaries (the block that builds
initialSet/defaultImageSets), create a mappedConfig variable set to
convertImageToImageConfig(*image.Mapped) when image.Mapped != nil otherwise
fallback to origImageConfig, then always append extensions.ImageSet{imageID:
mappedConfig} to updatedImageSets so indexes of defaultImageSets and
updatedImageSets remain aligned for later iteration over updatedImageSets.


// OpenShift defaults
openshiftDefaults := image.OriginalImages()
openshiftUpdated := image.GetMappedImages(openshiftDefaults, imagesetup.DefaultTestImageMirrorLocation)

Expand Down Expand Up @@ -323,3 +344,42 @@ func setLogLevel(level string) error {
logrus.SetLevel(lvl)
return nil
}

// filterExceptionsFromImageSets takes a list of image sets and filters out images
// whose E2E image matches any exception.Exceptions is a list of images we
// don't mirror temporarily due to various problems.
func filterExceptionsFromImageSets(initialImageSets []extensions.ImageSet) []extensions.ImageSet {
exceptions := image.Exceptions.List()
filteredImageSets := []extensions.ImageSet{}

for _, imgSet := range initialImageSets {
filtered := extensions.ImageSet{}
for imageID, imageConfig := range imgSet {
if !imageMatchesExceptions(imageConfig, exceptions) {
filtered[imageID] = imageConfig
}
}
if len(filtered) > 0 {
filteredImageSets = append(filteredImageSets, filtered)
}
}

return filteredImageSets
}

// imageMatchesExceptions checks if a single image matches any exception
func imageMatchesExceptions(imageConfig k8simage.Config, exceptions []string) bool {
return slices.ContainsFunc(exceptions, func(e string) bool {
return strings.Contains(imageConfig.GetE2EImage(), e)
})
}

// convertImageToImageConfig converts an extension.Image to k8simage.Config
func convertImageToImageConfig(image extension.Image) k8simage.Config {
imageConfig := k8simage.Config{}
imageConfig.SetName(image.Name)
imageConfig.SetVersion(image.Version)
imageConfig.SetRegistry(image.Registry)

return imageConfig
}
23 changes: 7 additions & 16 deletions pkg/test/extensions/binary.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -511,7 +511,7 @@ func (b *TestBinary) RunTests(ctx context.Context, timeout time.Duration, env []
return results
}

func (b *TestBinary) ListImages(ctx context.Context) (ImageSet, error) {
func (b *TestBinary) ListImages(ctx context.Context) ([]extension.Image, error) {
start := time.Now()
binName := filepath.Base(b.binaryPath)

Expand All @@ -522,23 +522,14 @@ func (b *TestBinary) ListImages(ctx context.Context) (ImageSet, error) {
return nil, fmt.Errorf("failed running '%s list': %w\nOutput: %s", b.binaryPath, err, output)
}

var images []Image
var images []extension.Image
err = json.Unmarshal(output, &images)
if err != nil {
return nil, err
}

result := make(ImageSet, len(images))
for _, image := range images {
imageConfig := k8simage.Config{}
imageConfig.SetName(image.Name)
imageConfig.SetVersion(image.Version)
imageConfig.SetRegistry(image.Registry)
result[k8simage.ImageID(image.Index)] = imageConfig
}

logrus.Infof("Listed %d test images for %q in %v", len(images), binName, time.Since(start))
return result, nil
return images, nil
}

// ExtractAllTestBinaries determines the optimal release payload to use, and extracts all the external
Expand Down Expand Up @@ -727,9 +718,9 @@ func (binaries TestBinaries) Info(ctx context.Context, parallelism int) ([]*Exte
return infos, nil
}

func (binaries TestBinaries) ListImages(ctx context.Context, parallelism int) ([]ImageSet, error) {
func (binaries TestBinaries) ListImages(ctx context.Context, parallelism int) ([]extension.Image, error) {
var (
allImages []ImageSet
allImages []extension.Image
mu sync.Mutex
wg sync.WaitGroup
errCh = make(chan error, len(binaries))
Expand Down Expand Up @@ -765,12 +756,12 @@ func (binaries TestBinaries) ListImages(ctx context.Context, parallelism int) ([
continue // Skip self - only external binaries need to be queried for images
}

imageConfig, err := binary.ListImages(ctx)
images, err := binary.ListImages(ctx)
if err != nil {
errCh <- err
}
mu.Lock()
allImages = append(allImages, imageConfig)
allImages = append(allImages, images...)
mu.Unlock()
Comment on lines +759 to 765
Copy link

@coderabbitai coderabbitai bot Mar 12, 2026

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟡 Minor

Missing continue after error handling.

When ListImages returns an error, the error is sent to errCh, but execution continues to append the (likely empty) images slice. Consider adding a continue statement after sending the error to errCh for cleaner control flow, consistent with typical error-then-continue patterns.

Note: The same pattern exists at Line 824-826 in ListTests, so this appears to be a pre-existing pattern in this file.

Proposed fix 
 				images, err := binary.ListImages(ctx)
 				if err != nil {
 					errCh <- err
+					continue
 				}
 				mu.Lock()
 				allImages = append(allImages, images...)
 				mu.Unlock()
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
images, err := binary.ListImages(ctx)
if err != nil {
errCh <- err
}
mu.Lock()
allImages = append(allImages, imageConfig)
allImages = append(allImages, images...)
mu.Unlock()
images, err := binary.ListImages(ctx)
if err != nil {
errCh <- err
continue
}
mu.Lock()
allImages = append(allImages, images...)
mu.Unlock()
🤖 Prompt for AI Agents 
Verify each finding against the current code and only fix it if needed.

In `@pkg/test/extensions/binary.go` around lines 759 - 765, In the loop calling
binary.ListImages, after sending the error to errCh you must immediately skip
further processing for that iteration (add a continue) so you don't append a
potentially empty/invalid images slice; update the block referencing
binary.ListImages, errCh, images, allImages and mu to send the error then
continue. Also apply the same fix to the analogous error handling in ListTests
(the block around ListTests/errCh/images at lines ~824-826) so both places
consistently return to the loop after reporting errors.

}
}
Expand Down