MCO-1906: Add Extra Validations - #6357
Conversation
|
Pipeline controller notification For optional jobs, comment This repository is configured in: LGTM mode |
openshift-ci-robot
added
the
jira/valid-reference
|
@anandram2: This pull request references MCO-1906 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the story to target the "5.0.0" version, but no target version was set. DetailsIn 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: anandram2 The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
|
Warning Review limit reached
Next review available in: 52 minutes You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Repository: openshift/coderabbit/.coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
WalkthroughBootstrap now validates install-time pre-built images with a timeout-bound image inspector. Validation checks registry access, OpenShift version metadata, and base-OS digest compatibility before creating component MachineConfigs. ChangesPre-built image validation
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant Bootstrap
participant SystemContextFactory
participant ImagesInspector
participant ClusterVersion
participant MachineOSConfig
Bootstrap->>SystemContextFactory: create timeout-bound system context
Bootstrap->>ImagesInspector: inspect pre-built image
ImagesInspector->>ImagesInspector: read registry metadata and /etc/os-release
Bootstrap->>ClusterVersion: compare OpenShift major/minor version
Bootstrap->>MachineOSConfig: append validated component MachineConfigs
Suggested reviewers: Important Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional. ❌ Failed checks (1 error, 1 inconclusive)
✅ Passed checks (13 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: 3
🧹 Nitpick comments (1)
pkg/controller/bootstrap/bootstrap.go (1)
371-376: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winConsider a per-image deadline for pre-built image validation.
One
time.Minutebudget covers validation of every MachineOSConfig. Each validation may pull and unpack image layers to read/etc/os-release, then also callInspect. With several MachineOSConfigs or a slow registry, the deadline can expire andcreatePreBuiltImageMachineConfigsreturns a hard bootstrap error. Scale the budget with the number of images, or derive a per-image context inside the loop.
defer preBuiltImageCancel()also runs only whenRunreturns, not when the block ends. Moving the context creation intocreatePreBuiltImageMachineConfigsscopes both concerns.🤖 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/controller/bootstrap/bootstrap.go` around lines 371 - 376, Update the pre-built image validation flow around createPreBuiltImageMachineConfigs so the timeout budget is scoped per image or scales with the number of MachineOSConfigs, rather than applying one fixed minute to the entire batch. Move context creation and cancellation into createPreBuiltImageMachineConfigs or otherwise ensure each validation receives its own deadline and cancellation occurs when that validation/block completes; preserve the existing bootstrap error behavior.
🤖 Prompt for all review comments with 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.
Inline comments:
In `@pkg/controller/bootstrap/bootstrap.go`:
- Around line 841-851: In the pre-built image validation flow around
validatePreBuiltImageDigestFallback, add a guard after the results[0].Error
check to detect nil results[0].InspectInfo and return a descriptive error
instead of dereferencing it. Extend TestValidatePreBuiltImageVersion with a case
where both Error and InspectInfo are nil, preserving the existing validation
path for valid inspection results.
- Around line 809-812: Update the bootstrap validation around imageDigest and
expectedBaseOSImage so tag-form cluster base OS images do not return a hard
error when digest resolution fails. Log a warning and skip verification,
matching the existing missing-OPENSHIFT_VERSION handling, while preserving the
error path for other invalid image conditions.
- Around line 828-838: Update the bootstrap setup in runBootstrapCmd to bind the
cluster release version into the bootstrap container’s RELEASE_VERSION
environment variable before validatePreBuiltImageVersion compares it with the
pre-built image version; preserve the existing --release-image handling and
ensure the value comes from the cluster release version already available to
bootstrap.
---
Nitpick comments:
In `@pkg/controller/bootstrap/bootstrap.go`:
- Around line 371-376: Update the pre-built image validation flow around
createPreBuiltImageMachineConfigs so the timeout budget is scoped per image or
scales with the number of MachineOSConfigs, rather than applying one fixed
minute to the entire batch. Move context creation and cancellation into
createPreBuiltImageMachineConfigs or otherwise ensure each validation receives
its own deadline and cancellation occurs when that validation/block completes;
preserve the existing bootstrap error behavior.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 868c8848-2ad8-413a-a63a-49dbf40ecbb9
📒 Files selected for processing (2)
pkg/controller/bootstrap/bootstrap.gopkg/controller/bootstrap/bootstrap_test.go
| imgVersion, versionErr := openshiftVersionFromImage(ctx, inspector, imageSpec) | ||
| if versionErr == nil { | ||
| matches, err := sameMajorMinor(imgVersion, version.ReleaseVersion) | ||
| if err != nil { | ||
| return fmt.Errorf("could not compare OCP versions for pre-built image %q: %w", imageSpec, err) | ||
| } | ||
| if !matches { | ||
| return fmt.Errorf("pre-built image %q OCP version %q does not match the cluster's OCP version %q", imageSpec, imgVersion, version.ReleaseVersion) | ||
| } | ||
| return nil | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Inspect how ReleaseVersion is defined and populated.
fd -t f 'version.go' -p pkg/version --exec cat -n {}
# Find any injection of the release version (env var, ldflags, flags).
rg -n 'RELEASE_VERSION|ReleaseVersion' --type go -g '!vendor/**' -C 3
rg -n 'ReleaseVersion' --iglob '*.yaml' --iglob 'Makefile' --iglob 'Dockerfile*' -C 3Repository: openshift/machine-config-operator
Length of output: 2499
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== files =="
git ls-files | rg '(^|/)(Dockerfile|.*Dockerfile.*|.*\.ya?ml$|Makefile|.*Makefile.*)$' || true
echo "== bootstrap outline =="
ast-grep outline pkg/controller/bootstrap/bootstrap.go --match main --view expanded || true
ast-grep outline pkg/controller/bootstrap/bootstrap.go --match controllerStart --view expanded || true
echo "== main/start references =="
rg -n 'bootstrap|bootstrap-|MachineConfigBootstrap|controller-start|bootstrapController|ReleaseVersion|RELEASE_VERSION' --type go -g '!vendor/**' -C 3
echo "== Docker/build manifests references =="
rg -n 'RELEASE_VERSION|ReleaseVersion|bootstrap|machine-config|make' -g 'Dockerfile*' -g '*.yaml' -g 'Makefile' -g 'Makefile*' -g '*.mk' -C 4 || trueRepository: openshift/machine-config-operator
Length of output: 50389
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== bootstrap.go package/main outline small slices =="
sed -n '1,140p' pkg/controller/bootstrap/bootstrap.go
printf '\n--- bootstrap.go around prebuilt check ---\n'
sed -n '808,848p' pkg/controller/bootstrap/bootstrap.go
echo "== Go sources mentioning bootstrap command/env injection =="
rg -n 'bootstrap[- ]|bootstrap-|MachineConfigBootstrap|mco-bootstrap|ReleaseVersion|RELEASE_VERSION|os.Setenv|environmentFrom|env' \
--type go --glob 'cmd/**' --glob 'pkg/**' -C 2 || true
echo "== focused build env var refs =="
rg -n 'RELEASE_VERSION|ReleaseVersion|LD_FLAGS|GOFLAGS|make binary|machine-config-operator|bootstrap' \
Dockerfile Dockerfile.rhel7 Makefile .ci-operator.yaml --C 4 || trueRepository: openshift/machine-config-operator
Length of output: 7738
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== command entrypoints =="
git ls-files cmd pkg/controller | rg '(^cmd/|bootstrap|machine|controller)' | sed -n '1,200p'
echo "== find command files by path =="
git ls-files cmd | sed -n '1,200p'
git ls-files 'pkg/cmd*' 'pkg/controller/**/**' | sed -n '1,200p'
echo "== release version exact refs outside tests/vendor =="
rg -n 'RELEASE_VERSION|ReleaseVersion' --glob '!vendor/**' --glob '!**/testdata/**' --glob '!**/*test*.go' --glob '!pkg/version/version.go' -C 3 || true
echo "== build manifest exact refs outside tests/vendor =="
rg -n 'RELEASE_VERSION|ReleaseVersion' Dockerfile Dockerfile.rhel7 Makefile .ci-operator.yaml --C 4 || trueRepository: openshift/machine-config-operator
Length of output: 22290
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== bootstrap command file =="
sed -n '1,220p' cmd/machine-config-operator/bootstrap.go
printf '\n--- version.go for command ---\n'
sed -n '1,220p' cmd/machine-config-operator/version.go
echo "== bootstrap bootstrap.go helper implementation section =="
rg -n 'func (b \*Bootstrap) Run|validatePreBuiltImageVersion|openshiftVersionFromImage|sameMajorMinor|ReleaseVersion|manifestDir|pullSecretFile|destDir' pkg/controller/bootstrap/bootstrap.go -C 3
echo "== full bootstrap.go helper lines with validate/openshift version =="
python3 - <<'PY'
from pathlib import Path
p=Path('pkg/controller/bootstrap/bootstrap.go')
text=p.read_text()
lines=text.splitlines()
for name in ['validatePreBuiltImageVersion','openshiftVersionFromImage','sameMajorMinor']:
try:
i=next(lines.index(f'{line} {name}') for line in ['func','func(') for start in range(len(lines)) if lines[start]==f'func {name}')
except StopIteration:
continue
PYRepository: openshift/machine-config-operator
Length of output: 17318
Bind the cluster release version for bootstrap before comparing pre-built images.
mco-bootstrap/runBootstrapCmd only receives --release-image, while validatePreBuiltImageVersion compares OPENSHIFT_VERSION against version.ReleaseVersion, whose only initialization is from RELEASE_VERSION. Without binding RELEASE_VERSION for the bootstrap container, images reporting 4.x fail the comparison and manifest rendering can stop.
🤖 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/controller/bootstrap/bootstrap.go` around lines 828 - 838, Update the
bootstrap setup in runBootstrapCmd to bind the cluster release version into the
bootstrap container’s RELEASE_VERSION environment variable before
validatePreBuiltImageVersion compares it with the pre-built image version;
preserve the existing --release-image handling and ensure the value comes from
the cluster release version already available to bootstrap.
aac0a16 to
67aa42a
Compare
|
/retest |
|
@anandram2: The following tests failed, say
Full PR test history. Your PR dashboard. DetailsInstructions 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. |
2 participants
- What I did
- How to verify it
-
go test ./pkg/controller/bootstrap/...or
pre-built image "..." OCP version "X.Y" does not match the cluster's OCP version "A.B".could not access pre-built image "..." (registry unreachable or image not found): ....- Description for the changelog
Add OCP-version-match and registry-accessibility validation for hybrid-OCL pre-built images at bootstrap time
MCO-1906
Summary by CodeRabbit