-
Notifications
You must be signed in to change notification settings - Fork 518
CNTRLPLANE-3553: Wire usesRunc detection into RHEL stream resolution #8832
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.
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -77,6 +77,9 @@ type rolloutConfig struct { | |
| // setting the default stream does not change the hash. | ||
| // Only a non-default stream (e.g. "rhel-9" on a ≥5.0 release) produces | ||
| // a non-empty value here and triggers a rollout. | ||
| // | ||
| // See also ConfigGenerator.resolvedRHELStreamForBootImage, which controls | ||
| // boot image resolution and is intentionally separate from the hash. | ||
| rhelStream string | ||
| } | ||
|
|
||
|
|
@@ -100,12 +103,16 @@ func NewConfigGenerator(ctx context.Context, client client.Client, hostedCluster | |
| // default explicitly does not change the hash and trigger a spurious rollout. | ||
| rhelStream := nodePool.Spec.OSImageStream.Name | ||
| if rhelStream != "" { | ||
| version, err := semver.Parse(releaseImage.Version()) | ||
| var version semver.Version | ||
| version, err = semver.Parse(releaseImage.Version()) | ||
| if err != nil { | ||
| return nil, fmt.Errorf("failed to parse release image version %q: %w", releaseImage.Version(), err) | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Minor style: usesRunc, err := usesRuncRuntime(ctx, client, nodePool)
if err != nil {
return nil, fmt.Errorf("failed to detect container runtime: %w", err)
}
defaultStream, err := GetRHELStream("", version, usesRunc)Up to you — it works as-is, just reads a bit unusual.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Doing
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Renamed runcErr→err, streamErr→err using block-scoped := |
||
| } | ||
| // TODO(CNTRLPLANE-3553): pass actual usesRunc once container runtime detection is wired in. | ||
| defaultStream, err := GetRHELStream("", version, false) | ||
| usesRunc, err := usesRuncRuntime(ctx, client, nodePool) | ||
| if err != nil { | ||
| return nil, fmt.Errorf("failed to detect container runtime: %w", err) | ||
| } | ||
| defaultStream, err := GetRHELStream("", version, usesRunc) | ||
| if err != nil { | ||
| return nil, fmt.Errorf("failed to resolve default RHEL stream: %w", err) | ||
| } | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Heads up: the old code returned
(nil, nil)whenStreamMetadata == nil, which let the caller indefaultAzureNodePoolImagefall through silently (no marketplace, no error, continue toAzureDiskImage). The new code returns an error fromStreamForName.Is this intentional? For pre-4.20 payloads or release images that legitimately have no stream metadata, this would now surface as
"failed to get Azure Marketplace metadata"instead of silently skipping marketplace defaulting. The version guard at line 48 (< 4.20returns early) may protect against this, but worth double-checking.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is, adding a comment