NO-JIRA: Include the target version in the logging #2050
Conversation
openshift-ci-robot
added
the
jira/valid-reference
|
@hongkailiu: This pull request explicitly references no jira issue. In 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: hongkailiu The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
openshift-merge-robot
added
the
needs-rebase
hongkailiu
force-pushed
the
target-version
branch
from
41608bd to
c08af1a
Compare
openshift-merge-robot
removed
the
needs-rebase
hongkailiu
force-pushed
the
target-version
branch
from
c08af1a to
ae3a133
Compare
Before this full, the logging was only for the case that `findClusterIncludeConfigFromInstallConfig` is called, i.e., the path from an install-config file is provided. This pull extends it to the case where the configuration is taken from the current cluster. Another change from the pull is that the logging messages include the target version that is determined by inspecting the release image. The implementation for this is adding a new callback `ImageConfigCallback`.
hongkailiu
force-pushed
the
target-version
branch
from
ae3a133 to
7056f53
Compare
|
/cc |
| inclusionConfig, err = findClusterIncludeConfig(ctx, o.RESTConfig, versionInImageConfig) | ||
| } else { | ||
| inclusionConfig, err = findClusterIncludeConfigFromInstallConfig(ctx, o.InstallConfig) | ||
| inclusionConfig, err = findClusterIncludeConfigFromInstallConfig(ctx, o.InstallConfig, versionInImageConfig) |
There was a problem hiding this comment.
Maybe I am missing something but I don't understand when is versionInImageConfig actually populated. What I'm reading is:
- It is empty on L354 when declared
- It can be populated on L371 inside the
opts.ImageConfigCallbackcallback (which itself is populated on L355) - It can only be actually populated once
opts.ImageConfigCallbackis called at least one - The callback is called in
pkg/cli/image/extract/extract.go:L426ino.Run() - Here on L384 / L386 we expect this to be a populated string already but I do not see any opportunity betwen L372 and the L383
ifwhereo.Run()could be called, even indirectly.
How is versionInImageConfig not always an empty string on L384/L386??
There was a problem hiding this comment.
This is reiterated #1958 (comment) but I do not understand the answer.
| return | ||
| } | ||
| klog.V(2).Infof("Retrieved the version from image configuration in the image to extract: %s", v) | ||
| versionInImageConfig = v |
There was a problem hiding this comment.
We assume we never reach this place twice here (= we assume the callback is never called multiple time in a way that reaches this assignment) otherwise the old value is overwritten.
That assumption may be fine but I'd either add a comment here about it or logged if we reach here while versionInImageConfig is not empty.
| } | ||
|
|
||
| // versionInImageConfig stores the version from the label of the image | ||
| // At the moment it is used later only for the logging purpose and thus is not blocking if failures occur upon getting its value |
There was a problem hiding this comment.
If the errors are not blocking then maybe they should be logged as warnings, not errors?
|
@hongkailiu: The following tests failed, say
Full PR test history. Your PR dashboard. 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. |
|
Issues go stale after 90d of inactivity. Mark the issue as fresh by commenting If this issue is safe to close now please do so with /lifecycle stale |
openshift-ci
bot
added
the
lifecycle/stale
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
WalkthroughThe changes introduce a callback-based mechanism to extract and propagate the release version from Docker image configuration labels through the image extraction and cluster inclusion workflow, enabling version-aware filtering and centralized capability difference logging. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes The changes span three files with coherent, callback-based additions and parameter propagation. The logic involves version extraction from labels, callback infrastructure setup, and refactoring of capability difference handling. Complexity is moderate—understanding the intent and flow across functions requires careful reading, but the individual changes are straightforward modifications to existing logic. Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 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: 0
♻️ Duplicate comments (2)
pkg/cli/admin/release/extract.go (2)
352-373: Make version capture non-blocking, stable, and avoid overwrites.
- Use Warning/Info (not Error) for non-blocking failures.
- Don’t overwrite if callback fires multiple times; log and keep first value.
Apply:
- // versionInImageConfig stores the version from the label of the image - // At the moment it is used later only for the logging purpose and thus is not blocking if failures occur upon getting its value + // versionInImageConfig stores the version from the image label. + // Best-effort and non-blocking if retrieval fails. var versionInImageConfig string opts.ImageConfigCallback = func(imageConfig *dockerv1client.DockerImageConfig) { - if imageConfig == nil { - // This should never happen - klog.Error("Cannot retrieve the version because no image configuration is provided in the image to extract") - return - } - if imageConfig.Config == nil { - klog.Error("Cannot retrieve the version from image configuration in the image to extract because it has no configuration") - return - } + if imageConfig == nil || imageConfig.Config == nil { + klog.V(2).Info("Skipping target-version capture: image has no config") + return + } v, ok := imageConfig.Config.Labels["io.openshift.release"] if !ok { - klog.Error("Cannot retrieve the version from image configuration in the image to extract because it does not have the required label 'io.openshift.release'") + klog.V(2).Info("Skipping target-version capture: label 'io.openshift.release' not present") return } - klog.V(2).Infof("Retrieved the version from image configuration in the image to extract: %s", v) - versionInImageConfig = v + if versionInImageConfig != "" && versionInImageConfig != v { + klog.V(2).Infof("Target version label changed from %q to %q; keeping the first value", versionInImageConfig, v) + return + } + klog.V(2).Infof("Retrieved target version from image configuration: %s", v) + versionInImageConfig = v }Earlier feedback requested logging when overwriting and warning level for non-blocking cases.
384-387: versionInImageConfig is used before the callback can populate it.The callback runs during opts.Run(); inclusionConfig is computed earlier, so target version is usually empty here.
Two fixes (pick one):
- Preferred: Pre-populate from release metadata before building inclusionConfig.
Add just before computing inclusionConfig:
// Pre-populate target version to avoid callback-ordering race if versionInImageConfig == "" { infoOpts := NewInfoOptions(o.IOStreams) infoOpts.SecurityOptions = o.SecurityOptions infoOpts.FilterOptions = o.FilterOptions infoOpts.FileDir = o.FileDir infoOpts.ICSPFile = o.ICSPFile infoOpts.IDMSFile = o.IDMSFile if rel, err := infoOpts.LoadReleaseInfo(o.From, false); err == nil { versionInImageConfig = rel.PreferredName() klog.V(2).Infof("Determined target version from release metadata: %s", versionInImageConfig) } else { klog.V(2).Infof("Could not determine target version from release metadata yet: %v", err) } }
- Minimal: Keep code as-is but rely on the helper’s empty-target guard (see prior comment). This avoids bad logs but loses the “target version” detail.
I can push a PR patch with option 1 if preferred.
🧹 Nitpick comments (1)
pkg/cli/image/extract/extract.go (1)
144-146: Document concurrency for ImageConfigCallback.Note this callback may be invoked concurrently when MaxPerRegistry > 1, mirroring ImageMetadataCallback semantics.
Apply:
- // ImageConfigCallback is invoked once image config retrieved - ImageConfigCallback func(imageConfig *dockerv1client.DockerImageConfig) + // ImageConfigCallback is invoked once per image after its config is retrieved. + // May be called concurrently when MaxPerRegistry > 1. + ImageConfigCallback func(imageConfig *dockerv1client.DockerImageConfig)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge base: Disabled due to Reviews -> Disable Knowledge Base setting
📒 Files selected for processing (3)
pkg/cli/admin/release/extract.go(2 hunks)pkg/cli/admin/release/extract_tools.go(3 hunks)pkg/cli/image/extract/extract.go(2 hunks)
🔇 Additional comments (3)
pkg/cli/admin/release/extract_tools.go (2)
1220-1222: Passing target version through is good; be aware these calls may now fail.Because logCapabilitySetMayDiffer currently returns an error on version lookup failure, these paths can now fail where they previously only logged. If non-breaking behavior is desired, adopt the non-blocking implementation in the previous comment.
Also applies to: 1265-1267
1170-1189: Confirm UX intent: should version detection failure silently degrade or block config extraction?The suggested refactor is semantically reasonable but requires explicit approval:
Current behavior (blocking): If version lookup fails → entire config extraction fails → command stops.
Suggested behavior (best-effort): If version lookup fails → log gracefully at V(2) level → config extraction continues.
Evidence supports the change being intentional:
- Function is named
logCapabilitySetMayDiffer— it's purely advisory/logging, not critical data extraction- Recent commit "NO-JIRA: Include the target version in the logging" indicates this logging was recently added
- It's reasonable that a logging-only function shouldn't become a hard failure point
However, the change alters error semantics for both callers (lines 1220, 1265), which currently treat errors as fatal. The review comment itself ends with `` asking you to confirm "that changing version-lookup failure from hard-error to best-effort log aligns with intended UX."
Before applying the diff:
- Confirm this behavior change is intentional
- If yes, the suggested implementation is sound (version.Info.Major/Minor fields confirmed, regex logic correct, imports present)
pkg/cli/image/extract/extract.go (1)
426-428: Good placement of the callback.Called immediately after parsing image config and before filters; matches intended usage.
5 participants
Before this full, the logging was only for the case that
findClusterIncludeConfigFromInstallConfigis called, i.e., the path from an install-config file is provided.This pull extends it to the case where the
configuration is taken from the current cluster.
Another change from the pull is that the logging
messages include the target version that is determined by inspecting the release image. The implementation for this is adding a new callback
ImageConfigCallback.