Skip to content

Eliminate oc --image-for calls and narrow semaphore scope #773

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
sdodson wants to merge 2 commits into
base: main
Choose a base branch
from
+90 −46
Open

Eliminate oc --image-for calls and narrow semaphore scope #773

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
eaa1762
Eliminate oc --image-for calls by reusing cached release JSON
sdodson Jun 5, 2026
c05975a
Narrow semaphore scope to rpmdb-only operations
sdodson Jun 5, 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
41 changes: 25 additions & 16 deletions cmd/release-controller-api/http.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -723,11 +723,24 @@ func (c *Controller) apiReleaseInfo(w http.ResponseWriter, req *http.Request) {
return
}

// Acquire a single semaphore slot for all stream operations,
// matching the HTML path (pkg/rhcos/node_image.go).
// RpmDiffForStream with a non-empty tag uses ocCmd (no
// semaphore), so without this guard concurrent API requests
// could spawn unbounded oc processes.
// Pre-resolve which streams exist in the from-release before
// acquiring the semaphore. ImageReferenceForComponent resolves
// from groupcache (JSON parse, no oc subprocess).
type streamEntry struct {
releasecontroller.APINodeImageStream
hasFrom bool
}
entries := make([]streamEntry, len(streams))
for i, stream := range streams {
entries[i].Name = stream.DisplayName
entries[i].Tag = stream.Tag
if _, errFrom := c.releaseInfo.ImageReferenceForComponent(tagInfo.PreviousTagPullSpec, stream.Tag); errFrom == nil {
entries[i].hasFrom = true
}
}

// Acquire a semaphore slot only for the rpmdb operations
// (RpmDiffForStream) which spawn oc subprocesses.
select {
case releasecontroller.RpmdbOCSlots <- struct{}{}:
default:
Expand All @@ -739,24 +752,20 @@ func (c *Controller) apiReleaseInfo(w http.ResponseWriter, req *http.Request) {
}
defer func() { <-releasecontroller.RpmdbOCSlots }()

result := make([]releasecontroller.APINodeImageStream, 0, len(streams))
for _, stream := range streams {
result := make([]releasecontroller.APINodeImageStream, 0, len(entries))
for _, e := range entries {
if req.Context().Err() != nil {
return
}
entry := releasecontroller.APINodeImageStream{
Name: stream.DisplayName,
Tag: stream.Tag,
}
if _, errFrom := c.releaseInfo.ImageReferenceForComponent(tagInfo.PreviousTagPullSpec, stream.Tag); errFrom == nil {
diff, err := c.releaseInfo.RpmDiffForStream(tagInfo.PreviousTagPullSpec, tagInfo.TagPullSpec, stream.Tag)
if e.hasFrom {
diff, err := c.releaseInfo.RpmDiffForStream(tagInfo.PreviousTagPullSpec, tagInfo.TagPullSpec, e.Tag)
if err != nil {
klog.V(4).Infof("Unable to retrieve RPM diff for stream %s in %s: %v", stream.Tag, tagInfo.Tag, err)
klog.V(4).Infof("Unable to retrieve RPM diff for stream %s in %s: %v", e.Tag, tagInfo.Tag, err)
} else {
entry.RpmDiff = &diff
e.RpmDiff = &diff
}
}
result = append(result, entry)
result = append(result, e.APINodeImageStream)
}
nodeImageStreams = result
}()
Expand Down
26 changes: 25 additions & 1 deletion pkg/release-controller/machine_os_tags.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,18 +13,42 @@ type MachineOSStreamInfo struct {
DisplayName string `json:"displayName,omitempty"`
}

// releaseInfoImageRefs is the subset of `oc adm release info -o json` needed to list payload tags.
// releaseInfoImageRefs is the subset of `oc adm release info -o json` needed to list payload tags
// and resolve component image references.
type releaseInfoImageRefs struct {
References struct {
Spec struct {
Tags []struct {
Name string `json:"name"`
Annotations map[string]string `json:"annotations"`
From struct {
Name string `json:"name"`
} `json:"from"`
} `json:"tags"`
} `json:"spec"`
} `json:"references"`
}

// imageRefFromReleaseJSON returns the image pull spec for componentName from the JSON produced
// by `oc adm release info -o json`. This avoids a separate `oc adm release info --image-for`
// call when the release JSON is already available (e.g. cached via ReleaseInfo).
func imageRefFromReleaseJSON(raw, componentName string) (string, error) {
var ri releaseInfoImageRefs
if err := json.Unmarshal([]byte(raw), &ri); err != nil {
return "", err
}
for _, t := range ri.References.Spec.Tags {
if t.Name == componentName {
ref := strings.TrimSpace(t.From.Name)
if ref == "" {
return "", fmt.Errorf("empty image reference for component %q", componentName)
}
return ref, nil
}
}
return "", fmt.Errorf("component %q not found in release", componentName)
Comment on lines +35 to +49
Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai Bot Jun 5, 2026

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟠 Major | ⚡ Quick win

Return a distinguishable not-found error here.

The new pre-resolution paths use ImageReferenceForComponent to decide whether a stream existed in the from-release, but this helper currently turns “component missing” into a generic error string. That makes genuine absence indistinguishable from malformed release JSON / empty from.name, so those callers can silently drop RPM diffs instead of surfacing the real failure. Please expose a sentinel/typed not-found error (or a boolean) so callers can ignore only the expected absence case.

Suggested direction 
+var ErrReleaseComponentNotFound = errors.New("release component not found")
+
 func imageRefFromReleaseJSON(raw, componentName string) (string, error) {
 	var ri releaseInfoImageRefs
 	if err := json.Unmarshal([]byte(raw), &ri); err != nil {
 		return "", err
 	}
 	for _, t := range ri.References.Spec.Tags {
 		if t.Name == componentName {
 			ref := strings.TrimSpace(t.From.Name)
 			if ref == "" {
 				return "", fmt.Errorf("empty image reference for component %q", componentName)
 			}
 			return ref, nil
 		}
 	}
-	return "", fmt.Errorf("component %q not found in release", componentName)
+	return "", fmt.Errorf("%w: %q", ErrReleaseComponentNotFound, componentName)
 }

Then the pre-resolution callers can use errors.Is(err, ErrReleaseComponentNotFound) and treat everything else as a real failure.

📝 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
func imageRefFromReleaseJSON(raw, componentName string) (string, error) {
var ri releaseInfoImageRefs
if err := json.Unmarshal([]byte(raw), &ri); err != nil {
return "", err
}
for _, t := range ri.References.Spec.Tags {
if t.Name == componentName {
ref := strings.TrimSpace(t.From.Name)
if ref == "" {
return "", fmt.Errorf("empty image reference for component %q", componentName)
}
return ref, nil
}
}
return "", fmt.Errorf("component %q not found in release", componentName)
var ErrReleaseComponentNotFound = errors.New("release component not found")
func imageRefFromReleaseJSON(raw, componentName string) (string, error) {
var ri releaseInfoImageRefs
if err := json.Unmarshal([]byte(raw), &ri); err != nil {
return "", err
}
for _, t := range ri.References.Spec.Tags {
if t.Name == componentName {
ref := strings.TrimSpace(t.From.Name)
if ref == "" {
return "", fmt.Errorf("empty image reference for component %q", componentName)
}
return ref, nil
}
}
return "", fmt.Errorf("%w: %q", ErrReleaseComponentNotFound, componentName)
}
🤖 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 `@pkg/release-controller/machine_os_tags.go` around lines 35 - 49, Define a
package-level sentinel error (e.g. ErrReleaseComponentNotFound =
errors.New("release component not found")) and update imageRefFromReleaseJSON to
return that sentinel when the loop finishes without finding componentName
(instead of the current fmt.Errorf("component %q not found in release", ...));
keep all other error returns (JSON unmarshal error and the empty ref fmt.Errorf)
unchanged so callers can differentiate a genuine not-found via errors.Is(err,
ErrReleaseComponentNotFound) while still treating other errors as real failures.

}

const versionDisplayNamesKey = "io.openshift.build.version-display-names"

// ListMachineOSStreams returns machine-OS base tags discovered by pairing each *coreos* extensions
Expand Down
47 changes: 27 additions & 20 deletions pkg/release-controller/release_info.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -64,7 +64,8 @@ type CachingReleaseInfo struct {
}

func NewCachingReleaseInfo(info ReleaseInfo, size int64, architecture string) ReleaseInfo {
cache := groupcache.NewGroup("release", size, groupcache.GetterFunc(func(ctx context.Context, key string, sink groupcache.Sink) error {
var cache *groupcache.Group
cache = groupcache.NewGroup("release", size, groupcache.GetterFunc(func(ctx context.Context, key string, sink groupcache.Sink) error {
var s string
var err error
parts := strings.Split(key, "\x00")
Expand Down Expand Up @@ -149,21 +150,31 @@ func NewCachingReleaseInfo(info ReleaseInfo, size int64, architecture string) Re
if len(parts) != 3 {
s, err = "", fmt.Errorf("invalid imagefor key")
} else {
s, err = info.ImageReferenceForComponent(parts[1], parts[2])
var raw string
if getErr := cache.Get(ctx, strings.Join([]string{"releaseinfo", parts[1]}, "\x00"), groupcache.StringSink(&raw)); getErr != nil {
err = getErr
} else {
s, err = imageRefFromReleaseJSON(raw, parts[2])
}
}
case "machineosstreams":
if len(parts) != 2 {
s, err = "", fmt.Errorf("invalid machineosstreams key")
} else {
var streams []MachineOSStreamInfo
streams, err = info.ListMachineOSStreams(parts[1])
if err == nil {
var b []byte
b, err = json.Marshal(streams)
if err != nil {
klog.V(4).Infof("Failed to Marshal machine OS streams for %s; %s", parts[1], err)
} else {
s = string(b)
var raw string
if getErr := cache.Get(ctx, strings.Join([]string{"releaseinfo", parts[1]}, "\x00"), groupcache.StringSink(&raw)); getErr != nil {
err = getErr
} else {
var streams []MachineOSStreamInfo
streams, err = machineOSStreamsFromReleaseJSON(raw)
if err == nil {
var b []byte
b, err = json.Marshal(streams)
if err != nil {
klog.V(4).Infof("Failed to Marshal machine OS streams for %s; %s", parts[1], err)
} else {
s = string(b)
}
}
}
}
Expand Down Expand Up @@ -617,24 +628,20 @@ func (r *ExecReleaseInfo) RpmList(image string) (RpmList, error) {
}

// ImageReferenceForComponent resolves the full image reference (pullspec) for a named component
// within a release image using oc adm release info --image-for. Returns the complete image reference
// including registry, repository, and digest.
// within a release image. It reuses the JSON from ReleaseInfo (already fetched for
// ListMachineOSStreams) instead of making a separate oc --image-for invocation.
func (r *ExecReleaseInfo) ImageReferenceForComponent(releaseImage, componentName string) (string, error) {
if _, err := imagereference.Parse(releaseImage); err != nil {
return "", fmt.Errorf("%s is not an image reference: %v", releaseImage, err)
}
if strings.HasPrefix(releaseImage, "-") {
return "", fmt.Errorf("not a valid reference")
}
out, _, err := ocCmd("adm", "release", "info", "--image-for", componentName, releaseImage)
raw, err := r.ReleaseInfo(releaseImage)
if err != nil {
return "", fmt.Errorf("failed to resolve image for component %q in %s: %w", componentName, releaseImage, err)
}
ref := strings.TrimSpace(string(out))
if ref == "" {
return "", fmt.Errorf("empty image reference for component %q in %s", componentName, releaseImage)
return "", fmt.Errorf("failed to get release info for %s: %w", releaseImage, err)
}
return ref, nil
return imageRefFromReleaseJSON(raw, componentName)
}

// RpmListForStream loads the RPM database for a machine-os component inside the release payload
Expand Down
22 changes: 13 additions & 9 deletions pkg/rhcos/node_image.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -41,10 +41,18 @@ func NodeImageSectionMarkdown(ctx context.Context, info releasecontroller.Releas
return RenderNodeImageInfo(changelogMarkdown, rpmlist, rpmdiff), nil
}

// Acquire single semaphore slot for all stream operations.
// Respect context cancellation so orphaned goroutines from timed-out HTTP
// handlers release their slot promptly instead of holding it until all
// streams are processed.
// Pre-resolve which streams exist in the from-release before acquiring
// the semaphore. ImageReferenceForComponent resolves from groupcache
// (JSON parse only, no oc subprocess) and does not need the slot.
streamHasFrom := make([]bool, len(streams))
for i, stream := range streams {
if _, errFrom := info.ImageReferenceForComponent(fromReleasePullSpec, stream.Tag); errFrom == nil {
streamHasFrom[i] = true
}
}

// Acquire a semaphore slot only for the rpmdb operations (RpmListForStream,
// RpmDiffForStream) which spawn oc subprocesses and write to /tmp/rpmdb/.
select {
case releasecontroller.RpmdbOCSlots <- struct{}{}:
case <-ctx.Done():
Expand All @@ -55,13 +63,9 @@ func NodeImageSectionMarkdown(ctx context.Context, info releasecontroller.Releas
}
defer func() { <-releasecontroller.RpmdbOCSlots }()

// Process all streams sequentially to avoid cache stomping
nodeStreams := make([]CoreOSNodeStream, len(streams))

for i, stream := range streams {
// Between stream iterations, check if the caller gave up (e.g. HTTP
// handler timed out). This releases the semaphore slot without waiting
// for remaining streams to finish.
if ctx.Err() != nil {
return "", ctx.Err()
}
Expand All @@ -73,7 +77,7 @@ func NodeImageSectionMarkdown(ctx context.Context, info releasecontroller.Releas
}

var diff releasecontroller.RpmDiff
if _, errFrom := info.ImageReferenceForComponent(fromReleasePullSpec, stream.Tag); errFrom == nil {
if streamHasFrom[i] {
diff, err = info.RpmDiffForStream(fromReleasePullSpec, toReleasePullSpec, stream.Tag)
if err != nil {
return "", fmt.Errorf("failed to fetch RPM diff for stream %s: %w", stream.Tag, err)
Expand Down