Add IBI End-To-End Preinstall Test#1325
Open
josclark42 wants to merge 4 commits intorh-ecosystem-edge:mainfrom
Open
Add IBI End-To-End Preinstall Test#1325josclark42 wants to merge 4 commits intorh-ecosystem-edge:mainfrom
josclark42 wants to merge 4 commits intorh-ecosystem-edge:mainfrom
Conversation
Contributor
📝 WalkthroughWalkthroughAdds a new Image-Based Installation (IBI) preinstall E2E test suite for CNF RAN with configuration, defaults, helpers, tests, reporting, and provisioning utilities; updates lint/run scripts and module requirements. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes 🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
⚔️ Resolve merge conflicts
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 |
Collaborator
Author
|
Requires ran-integration PR https://gitlab.cee.redhat.com/ran/ran-integration/-/merge_requests/524 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 16
🧹 Nitpick comments (2)
tests/lca/imagebasedupgrade/cnf/upgrade-talm/tests/rollback-after-failed-upgrade-test.go (1)
78-78: Use shared abort IBGU name constant instead of hardcoded literal.Line 78 hardcodes
"abortibgu". Prefertsparams.AbortIbguNameto keep naming consistent with other suite tests and avoid drift in cleanup behavior.Proposed fix
- abortIbguBuilder := ibgu.NewIbguBuilder(TargetHubAPIClient, "abortibgu", tsparams.IbguNamespace). + abortIbguBuilder := ibgu.NewIbguBuilder(TargetHubAPIClient, tsparams.AbortIbguName, tsparams.IbguNamespace).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/lca/imagebasedupgrade/cnf/upgrade-talm/tests/rollback-after-failed-upgrade-test.go` at line 78, Replace the hardcoded abort IBGU name literal in the NewIbguBuilder call with the shared constant: change the second argument in ibgu.NewIbguBuilder(TargetHubAPIClient, "abortibgu", tsparams.IbguNamespace) to use tsparams.AbortIbguName so the call becomes ibgu.NewIbguBuilder(TargetHubAPIClient, tsparams.AbortIbguName, tsparams.IbguNamespace), ensuring consistent naming across tests and cleanup.tests/lca/imagebasedinstall/cnf/ran/preinstall/internal/helpers/ibi_config.go (1)
115-136: Use a YAML decoder here instead of splitting on---.Manual splitting is brittle for multi-document YAML, and the
continueon unmarshal error turns a malformedClusterInstancedocument into a misleading “not found”. A streaming decoder would preserve the real parse failure.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/lca/imagebasedinstall/cnf/ran/preinstall/internal/helpers/ibi_config.go` around lines 115 - 136, The ParseClusterInstance function should use a streaming YAML decoder instead of splitting on '---'; replace the strings.Split logic with a yaml.NewDecoder(bytes.NewReader(kustomizeOutput)) loop that repeatedly calls decoder.Decode(&clusterInst), check for io.EOF to finish, and for any non-EOF decode error return that error (do not continue silently), and when a successfully decoded document has clusterInst.Kind == "ClusterInstance" return it; otherwise continue to the next decoded document and finally return the "ClusterInstance not found" error if none matched.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@tests/lca/imagebasedinstall/cnf/ran/internal/ranconfig/config.go`:
- Around line 185-209: ResolveBootstrapOCPath returns early with an error if
c.BootstrapOC is set to a non-existent path, preventing the intended fallback to
oc on PATH; update RanConfig.ResolveBootstrapOCPath to treat a configured but
missing bootstrap path as a warning (or ignore it) and continue to call
exec.LookPath("oc") instead of returning immediately when os.Stat(bootstrapPath)
yields os.IsNotExist(err), while preserving other stat errors (wrap and return
those) and keeping the existing checks for directories (fi.IsDir()) and the
successful return of bootstrapPath when it exists.
In
`@tests/lca/imagebasedinstall/cnf/ran/preinstall/internal/helpers/bmh_provisioning.go`:
- Around line 111-126: The deletion code returns as soon as Delete is called,
which can race with finalizers and cause AlreadyExists/Terminating errors; after
calling apiClient.Delete on bmhObj (and after deleting the secret via
apiClient.Secrets(...).Delete), wait for the resources to actually disappear by
polling Get for the BareMetalHost (using goclient.ObjectKey{Namespace:
bmhNamespace, Name: bmhName} and bmhObj) until it returns a NotFound error or a
timeout, and likewise poll for the secret name bmcSecretName in bmhNamespace
until Get/Secrets(...).Get returns NotFound; if polling times out return a
descriptive error, otherwise proceed once both are confirmed deleted.
- Around line 78-95: The loop currently discards SSH errors so a bad key/host
becomes a generic timeout; store the last SSH error and output by declaring
locals (e.g., lastErr, lastOutput) and updating them whenever
SSHExecOnProvisioningHost returns an error or non-successful output, then when
returning the timeout error include lastErr and/or lastOutput in the fmt.Errorf
message (reference SSHExecOnProvisioningHost, err, output, startTime, timeout)
so the returned error shows the real SSH failure details.
In
`@tests/lca/imagebasedinstall/cnf/ran/preinstall/internal/helpers/hub_image_digest_sources.go`:
- Around line 142-161: registryLocationFromConf currently scans a 4KB window
after the first registryPrefix match and returns the last location= it finds,
which can accidentally pick a location from a subsequent [[registry]] block;
instead, after finding idx in registryLocationFromConf, truncate the search
window at the start of the next `[[registry]]` block (e.g. search for the next
`[[registry]]` literal and slice up to it) before running the
`location\s*=\s*"([^"]+)"` regexp, and return the first regexp match found
within that truncated window (not the last), ensuring the location is taken only
from the same registry block.
In
`@tests/lca/imagebasedinstall/cnf/ran/preinstall/internal/helpers/hub_resources.go`:
- Around line 55-63: mcBuilder is dereferenced without checking for existence
after calling mco.PullMachineConfig; add a nil-existence guard before
unmarshalling: verify mcBuilder != nil and mcBuilder.Object != nil (and
optionally that mcBuilder.Object.Spec.Config.Raw is non-nil/len>0) and return a
clear formatted error (e.g., "MachineConfig 99-master-ssh not found" or "missing
Spec.Config.Raw") if the check fails, then proceed to call json.Unmarshal on
mcBuilder.Object.Spec.Config.Raw; references: PullMachineConfig, mcBuilder,
mcBuilder.Object.Spec.Config.Raw, json.Unmarshal.
In
`@tests/lca/imagebasedinstall/cnf/ran/preinstall/internal/helpers/iso_generation.go`:
- Around line 25-27: The os.Stat call's error is only checked for os.IsNotExist,
which ignores other failures; update the iso existence check around
os.Stat(isoPath) to handle all errors: if err != nil then if os.IsNotExist(err)
return the existing "ISO file was not found..." error, else return a
wrapped/descriptive error including the original err (e.g., "failed to stat
isoPath: %w") so permission/I/O errors are surfaced; reference the
os.Stat(isoPath) call and the isoPath variable when making this change.
- Around line 16-21: Wrap the openshift-install invocation in a context with a
timeout and use exec.CommandContext instead of exec.Command so the process is
killed if it exceeds the timeout (e.g., create a context via context.WithTimeout
and pass it to exec.CommandContext when constructing cmd for
openshiftInstallPath "image-based create image"); also change the subsequent
os.Stat error handling to check and return any error (not just os.IsNotExist) so
permission/I/O errors are surfaced (validate the error returned by os.Stat and
return it if non-nil). Finally, update the call site that invokes this ISO
creation helper (the caller in preinstall-e2e-test.go) to pass a concrete
timeout value such as 30*time.Minute.
In
`@tests/lca/imagebasedinstall/cnf/ran/preinstall/internal/helpers/openshift_install.go`:
- Around line 56-61: The subprocess invocations creating cmd with exec.Command
and calling cmd.CombinedOutput must use a context with a timeout to avoid
hanging; replace exec.Command(...) with exec.CommandContext(ctx, ...) where ctx
is from context.WithTimeout(parentCtx, <duration>) (e.g., 2m), defer cancel(),
and use cmd.CombinedOutput (or cmd.Output) as before, returning a clear
timeout-aware error when ctx expires; update both occurrences that build cmd
(the bootstrapOC call and the openshift-install invocation) and keep using
withExtractKubeconfig(os.Environ(), hubKubeconfig) for env setup so the behavior
and output capture remain unchanged.
In
`@tests/lca/imagebasedinstall/cnf/ran/preinstall/internal/helpers/ssh_utils.go`:
- Around line 25-27: The subprocess calls using
exec.Command(...)->cmd.CombinedOutput() in ssh_utils.go currently have no
timeout and can hang; update both occurrences to use context.WithTimeout and
exec.CommandContext with a reasonable timeout (e.g., configurable or a constant)
so the child is killed if it blocks, ensure you cancel the context, handle
ctx.Err()/context.DeadlineExceeded to return a clear timeout error, and
propagate the subprocess output and original error from the exec call; replace
direct exec.Command + CombinedOutput usage with this context-aware pattern for
both the SCP and SSH invocations.
- Around line 15-16: In ssh_utils.go the SSH args "-o StrictHostKeyChecking=no"
and "-o UserKnownHostsFile=/dev/null" are hardcoded and must be made opt-in: add
a boolean config/flag or environment variable (e.g., SSH_DISABLE_HOST_KEY_CHECK
or EnableInsecureSSH) defaulting to false, update the code path that builds the
SSH args to append these two strings only when that flag is true, and ensure the
same change is applied to the other occurrence of those options (the second
occurrence around the other SSH invocation); also add a short comment noting
this is insecure and should only be used for testing.
In
`@tests/lca/imagebasedinstall/cnf/ran/preinstall/internal/helpers/ztp_config.go`:
- Around line 47-52: Replace the blocking exec.Command call with a context-aware
command so kustomize cannot hang indefinitely: create a context with timeout
(e.g., ctx, cancel := context.WithTimeout(context.Background(), 2*time.Minute);
defer cancel()), use exec.CommandContext(ctx, "kustomize", "build",
siteConfigDir) instead of exec.Command, then call cmd.CombinedOutput() as before
and handle the error; also add the required imports (context and time). Ensure
you reference the existing variables cmd and siteConfigDir and preserve the
existing error wrapping logic.
- Around line 17-24: The current removal code calls os.RemoveAll(destDir)
directly; guard against unsafe paths by cleaning and validating destDir before
calling RemoveAll. In the block around os.Stat(destDir) and
os.RemoveAll(destDir) (referencing the destDir variable), call cleaned :=
filepath.Clean(destDir) and reject/return an error if cleaned is empty or equals
"/" or "." or ".." (and on Windows if cleaned is a drive root like "C:/" ),
otherwise proceed to RemoveAll; this prevents accidental recursive deletes of
root/current-directory equivalents.
In
`@tests/lca/imagebasedinstall/cnf/ran/preinstall/internal/tsparams/preinstallvars.go`:
- Around line 22-27: ReporterCRDsToDump currently includes an unrestricted
SecretList which will dump all cluster secrets (including sensitive data);
update ReporterCRDsToDump (the k8sreporter.CRData slice) to either remove the
{Cr: &corev1.SecretList{}} entry entirely or replace it with a namespace-scoped
secret list (e.g., create a corev1.SecretList with FieldSelector/Namespace set
to only the required namespace(s) or use the same scoping pattern other LCA
tests use) so that secrets from "openshift-config" and "openshift-machine-api"
are not globally exported; change the entry near ReporterCRDsToDump to implement
the chosen safe option and ensure any tests expecting the full list are adjusted
accordingly.
In `@tests/lca/imagebasedinstall/cnf/ran/preinstall/README.md`:
- Around line 6-7: The README uses the non-hyphenated phrase "bare metal host"
in multiple places; update all occurrences (e.g., the phrase in the preinstall
section and the later mentions around the preinstall/Provisioning steps) to use
the hyphenated compound adjective "bare-metal host" for consistent technical
phrasing—search for "bare metal host" and replace with "bare-metal host"
throughout the document (including the occurrences noted near the
preinstall/provisioning descriptions).
In `@tests/lca/imagebasedinstall/cnf/ran/preinstall/tests/preinstall-e2e-test.go`:
- Around line 188-200: The isoURL is built using a hardcoded filename
("rhcos-ibi.iso") while the ISO was copied to
raninittools.RanConfig.RemoteISOPath (remoteISO); update the isoURL construction
to use the actual remote filename derived from
raninittools.RanConfig.RemoteISOPath (e.g., take the basename of remoteISO) and
pass that to raninittools.RanConfig.ISOArtifactURL (or otherwise construct the
URL from RemoteISOPath) so the BMH boot URL matches the file uploaded by
helpers.SCPToProvisioningHost; adjust the code around the isoURL variable (which
follows SCPToProvisioningHost) to use this derived name instead of the hardcoded
"rhcos-ibi.iso".
In
`@tests/lca/imagebasedupgrade/cnf/upgrade-talm/tests/rollback-after-failed-upgrade-test.go`:
- Around line 61-77: The single-read of the IBU status can race with
asynchronous updates and trigger an unnecessary abort IBGU; instead, poll
lca.PullImageBasedUpgrade(TargetSNOAPIClient) in a short retry loop (e.g., retry
every few seconds up to a small timeout) re-evaluating
ibu.Object.Status.Conditions for condition.Type == "Idle" && condition.Status ==
"True" before deciding to create the abort IBGU; if the loop times out without
Idle becoming true, then proceed to create the abort IBGU as before.
---
Nitpick comments:
In
`@tests/lca/imagebasedinstall/cnf/ran/preinstall/internal/helpers/ibi_config.go`:
- Around line 115-136: The ParseClusterInstance function should use a streaming
YAML decoder instead of splitting on '---'; replace the strings.Split logic with
a yaml.NewDecoder(bytes.NewReader(kustomizeOutput)) loop that repeatedly calls
decoder.Decode(&clusterInst), check for io.EOF to finish, and for any non-EOF
decode error return that error (do not continue silently), and when a
successfully decoded document has clusterInst.Kind == "ClusterInstance" return
it; otherwise continue to the next decoded document and finally return the
"ClusterInstance not found" error if none matched.
In
`@tests/lca/imagebasedupgrade/cnf/upgrade-talm/tests/rollback-after-failed-upgrade-test.go`:
- Line 78: Replace the hardcoded abort IBGU name literal in the NewIbguBuilder
call with the shared constant: change the second argument in
ibgu.NewIbguBuilder(TargetHubAPIClient, "abortibgu", tsparams.IbguNamespace) to
use tsparams.AbortIbguName so the call becomes
ibgu.NewIbguBuilder(TargetHubAPIClient, tsparams.AbortIbguName,
tsparams.IbguNamespace), ensuring consistent naming across tests and cleanup.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 6afdc9a5-60aa-4d22-8d7e-318be5e1fe83
⛔ Files ignored due to path filters (2)
go.sumis excluded by!**/*.sumvendor/modules.txtis excluded by!vendor/**
📒 Files selected for processing (23)
.golangci.ymlgo.modscripts/golangci-lint.shtests/lca/imagebasedinstall/cnf/ran/internal/ranconfig/config.gotests/lca/imagebasedinstall/cnf/ran/internal/ranconfig/default.yamltests/lca/imagebasedinstall/cnf/ran/internal/raninittools/raninittools.gotests/lca/imagebasedinstall/cnf/ran/internal/ranparams/const.gotests/lca/imagebasedinstall/cnf/ran/internal/ranparams/ranvars.gotests/lca/imagebasedinstall/cnf/ran/preinstall/README.mdtests/lca/imagebasedinstall/cnf/ran/preinstall/internal/helpers/bmh_provisioning.gotests/lca/imagebasedinstall/cnf/ran/preinstall/internal/helpers/cluster_instance.gotests/lca/imagebasedinstall/cnf/ran/preinstall/internal/helpers/hub_image_digest_sources.gotests/lca/imagebasedinstall/cnf/ran/preinstall/internal/helpers/hub_resources.gotests/lca/imagebasedinstall/cnf/ran/preinstall/internal/helpers/ibi_config.gotests/lca/imagebasedinstall/cnf/ran/preinstall/internal/helpers/iso_generation.gotests/lca/imagebasedinstall/cnf/ran/preinstall/internal/helpers/openshift_install.gotests/lca/imagebasedinstall/cnf/ran/preinstall/internal/helpers/ssh_utils.gotests/lca/imagebasedinstall/cnf/ran/preinstall/internal/helpers/ztp_config.gotests/lca/imagebasedinstall/cnf/ran/preinstall/internal/tsparams/const.gotests/lca/imagebasedinstall/cnf/ran/preinstall/internal/tsparams/preinstallvars.gotests/lca/imagebasedinstall/cnf/ran/preinstall/preinstall_suite_test.gotests/lca/imagebasedinstall/cnf/ran/preinstall/tests/preinstall-e2e-test.gotests/lca/imagebasedupgrade/cnf/upgrade-talm/tests/rollback-after-failed-upgrade-test.go
Comment on lines
+55
to
+63
| mcBuilder, err := mco.PullMachineConfig(apiClient, "99-master-ssh") | ||
| if err != nil { | ||
| return "", fmt.Errorf("failed to pull MachineConfig 99-master-ssh: %w", err) | ||
| } | ||
|
|
||
| var configData map[string]interface{} | ||
|
|
||
| err = json.Unmarshal(mcBuilder.Object.Spec.Config.Raw, &configData) | ||
| if err != nil { |
Contributor
There was a problem hiding this comment.
Check MachineConfig existence before dereference
Line 62 dereferences mcBuilder.Object without verifying the resource exists. Add an existence (and nil) check before unmarshalling to avoid potential nil dereference on missing 99-master-ssh.
Suggested guard
mcBuilder, err := mco.PullMachineConfig(apiClient, "99-master-ssh")
if err != nil {
return "", fmt.Errorf("failed to pull MachineConfig 99-master-ssh: %w", err)
}
+ if mcBuilder == nil || !mcBuilder.Exists() || mcBuilder.Object == nil {
+ return "", fmt.Errorf("MachineConfig 99-master-ssh not found")
+ }
var configData map[string]interface{}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@tests/lca/imagebasedinstall/cnf/ran/preinstall/internal/helpers/hub_resources.go`
around lines 55 - 63, mcBuilder is dereferenced without checking for existence
after calling mco.PullMachineConfig; add a nil-existence guard before
unmarshalling: verify mcBuilder != nil and mcBuilder.Object != nil (and
optionally that mcBuilder.Object.Spec.Config.Raw is non-nil/len>0) and return a
clear formatted error (e.g., "MachineConfig 99-master-ssh not found" or "missing
Spec.Config.Raw") if the check fails, then proceed to call json.Unmarshal on
mcBuilder.Object.Spec.Config.Raw; references: PullMachineConfig, mcBuilder,
mcBuilder.Object.Spec.Config.Raw, json.Unmarshal.
Comment on lines
+6
to
+7
| 2. Provisioning a bare metal host using the generated ISO | ||
| 3. Waiting for the preinstall process to complete |
Contributor
There was a problem hiding this comment.
Use hyphenated compound adjective in docs
Use “bare-metal host” (Line 6 and Line 102) for consistent technical phrasing.
Also applies to: 102-103
🧰 Tools
🪛 LanguageTool
[uncategorized] ~6-~6: If this is a compound adjective that modifies the following noun, use a hyphen.
Context: ...nd site configuration 2. Provisioning a bare metal host using the generated ISO 3. Waiting...
(EN_COMPOUND_ADJECTIVE_INTERNAL)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/lca/imagebasedinstall/cnf/ran/preinstall/README.md` around lines 6 - 7,
The README uses the non-hyphenated phrase "bare metal host" in multiple places;
update all occurrences (e.g., the phrase in the preinstall section and the later
mentions around the preinstall/Provisioning steps) to use the hyphenated
compound adjective "bare-metal host" for consistent technical phrasing—search
for "bare metal host" and replace with "bare-metal host" throughout the document
(including the occurrences noted near the preinstall/provisioning descriptions).
Comment on lines
+188
to
+200
| remoteISO := raninittools.RanConfig.RemoteISOPath | ||
| By(fmt.Sprintf("Copying ISO to provisioning host %s:%s", raninittools.RanConfig.ProvisioningHost, remoteISO)) | ||
|
|
||
| err = helpers.SCPToProvisioningHost( | ||
| isoPath, | ||
| remoteISO, | ||
| raninittools.RanConfig.ProvisioningHost, | ||
| raninittools.RanConfig.ProvisioningUser, | ||
| raninittools.RanConfig.EffectiveProvisioningSSHKey(), | ||
| ) | ||
| Expect(err).NotTo(HaveOccurred()) | ||
|
|
||
| isoURL := raninittools.RanConfig.ISOArtifactURL("rhcos-ibi.iso") |
Contributor
There was a problem hiding this comment.
Derive the boot ISO URL from RemoteISOPath.
SCPToProvisioningHost uploads to the configured remote path, but the BMH URL is always built as rhcos-ibi.iso. Any non-default remote_iso_path leaves the host booting a file that was never copied.
Suggested fix
remoteISO := raninittools.RanConfig.RemoteISOPath
By(fmt.Sprintf("Copying ISO to provisioning host %s:%s", raninittools.RanConfig.ProvisioningHost, remoteISO))
@@
- isoURL := raninittools.RanConfig.ISOArtifactURL("rhcos-ibi.iso")
+ isoURL := raninittools.RanConfig.ISOArtifactURL(filepath.Base(remoteISO))📝 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
| remoteISO := raninittools.RanConfig.RemoteISOPath | |
| By(fmt.Sprintf("Copying ISO to provisioning host %s:%s", raninittools.RanConfig.ProvisioningHost, remoteISO)) | |
| err = helpers.SCPToProvisioningHost( | |
| isoPath, | |
| remoteISO, | |
| raninittools.RanConfig.ProvisioningHost, | |
| raninittools.RanConfig.ProvisioningUser, | |
| raninittools.RanConfig.EffectiveProvisioningSSHKey(), | |
| ) | |
| Expect(err).NotTo(HaveOccurred()) | |
| isoURL := raninittools.RanConfig.ISOArtifactURL("rhcos-ibi.iso") | |
| remoteISO := raninittools.RanConfig.RemoteISOPath | |
| By(fmt.Sprintf("Copying ISO to provisioning host %s:%s", raninittools.RanConfig.ProvisioningHost, remoteISO)) | |
| err = helpers.SCPToProvisioningHost( | |
| isoPath, | |
| remoteISO, | |
| raninittools.RanConfig.ProvisioningHost, | |
| raninittools.RanConfig.ProvisioningUser, | |
| raninittools.RanConfig.EffectiveProvisioningSSHKey(), | |
| ) | |
| Expect(err).NotTo(HaveOccurred()) | |
| isoURL := raninittools.RanConfig.ISOArtifactURL(filepath.Base(remoteISO)) |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/lca/imagebasedinstall/cnf/ran/preinstall/tests/preinstall-e2e-test.go`
around lines 188 - 200, The isoURL is built using a hardcoded filename
("rhcos-ibi.iso") while the ISO was copied to
raninittools.RanConfig.RemoteISOPath (remoteISO); update the isoURL construction
to use the actual remote filename derived from
raninittools.RanConfig.RemoteISOPath (e.g., take the basename of remoteISO) and
pass that to raninittools.RanConfig.ISOArtifactURL (or otherwise construct the
URL from RemoteISOPath) so the BMH boot URL matches the file uploaded by
helpers.SCPToProvisioningHost; adjust the code around the isoURL variable (which
follows SCPToProvisioningHost) to use this derived name instead of the hardcoded
"rhcos-ibi.iso".
Comment on lines
+61
to
+77
| // Check if IBU is already Idle after auto-rollback | ||
| ibu, err = lca.PullImageBasedUpgrade(TargetSNOAPIClient) | ||
| Expect(err).ToNot(HaveOccurred(), "Failed to pull IBU resource") | ||
|
|
||
| abortIbguBuilder, err = abortIbguBuilder.Create() | ||
| Expect(err).ToNot(HaveOccurred(), "Failed to create abort Ibgu.") | ||
| // Check the Idle condition status | ||
| isIdle := false | ||
|
|
||
| _, err = abortIbguBuilder.WaitUntilComplete(5 * time.Minute) | ||
| Expect(err).NotTo(HaveOccurred(), "abort IBGU did not complete in time.") | ||
| for _, condition := range ibu.Object.Status.Conditions { | ||
| if condition.Type == "Idle" && condition.Status == "True" { | ||
| isIdle = true | ||
|
|
||
| _, err = abortIbguBuilder.DeleteAndWait(1 * time.Minute) | ||
| Expect(err).ToNot(HaveOccurred(), "Failed to delete abort ibgu on target hub cluster") | ||
| break | ||
| } | ||
| } | ||
|
|
||
| // Only create abort IBGU if not already Idle | ||
| if !isIdle { |
Contributor
There was a problem hiding this comment.
One-time Idle check can still race and trigger unnecessary abort flow.
Line 61-Line 77 does a single pull+scan of conditions, but IBU condition updates are asynchronous. A transient non-idle read can still create an abort IBGU even when auto-rollback is about to settle to Idle.
Proposed fix (poll briefly before deciding to create abort IBGU)
- // Check if IBU is already Idle after auto-rollback
- ibu, err = lca.PullImageBasedUpgrade(TargetSNOAPIClient)
- Expect(err).ToNot(HaveOccurred(), "Failed to pull IBU resource")
-
- // Check the Idle condition status
- isIdle := false
-
- for _, condition := range ibu.Object.Status.Conditions {
- if condition.Type == "Idle" && condition.Status == "True" {
- isIdle = true
-
- break
- }
- }
+ // Poll briefly because IBU status updates are asynchronous.
+ isIdle := false
+ for deadline := time.Now().Add(2 * time.Minute); time.Now().Before(deadline); time.Sleep(10 * time.Second) {
+ ibu, err = lca.PullImageBasedUpgrade(TargetSNOAPIClient)
+ Expect(err).ToNot(HaveOccurred(), "Failed to pull IBU resource")
+
+ for _, condition := range ibu.Object.Status.Conditions {
+ if condition.Type == "Idle" && condition.Status == "True" {
+ isIdle = true
+ break
+ }
+ }
+
+ if isIdle {
+ break
+ }
+ }📝 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
| // Check if IBU is already Idle after auto-rollback | |
| ibu, err = lca.PullImageBasedUpgrade(TargetSNOAPIClient) | |
| Expect(err).ToNot(HaveOccurred(), "Failed to pull IBU resource") | |
| abortIbguBuilder, err = abortIbguBuilder.Create() | |
| Expect(err).ToNot(HaveOccurred(), "Failed to create abort Ibgu.") | |
| // Check the Idle condition status | |
| isIdle := false | |
| _, err = abortIbguBuilder.WaitUntilComplete(5 * time.Minute) | |
| Expect(err).NotTo(HaveOccurred(), "abort IBGU did not complete in time.") | |
| for _, condition := range ibu.Object.Status.Conditions { | |
| if condition.Type == "Idle" && condition.Status == "True" { | |
| isIdle = true | |
| _, err = abortIbguBuilder.DeleteAndWait(1 * time.Minute) | |
| Expect(err).ToNot(HaveOccurred(), "Failed to delete abort ibgu on target hub cluster") | |
| break | |
| } | |
| } | |
| // Only create abort IBGU if not already Idle | |
| if !isIdle { | |
| // Poll briefly because IBU status updates are asynchronous. | |
| isIdle := false | |
| for deadline := time.Now().Add(2 * time.Minute); time.Now().Before(deadline); time.Sleep(10 * time.Second) { | |
| ibu, err = lca.PullImageBasedUpgrade(TargetSNOAPIClient) | |
| Expect(err).ToNot(HaveOccurred(), "Failed to pull IBU resource") | |
| for _, condition := range ibu.Object.Status.Conditions { | |
| if condition.Type == "Idle" && condition.Status == "True" { | |
| isIdle = true | |
| break | |
| } | |
| } | |
| if isIdle { | |
| break | |
| } | |
| } | |
| // Only create abort IBGU if not already Idle | |
| if !isIdle { |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@tests/lca/imagebasedupgrade/cnf/upgrade-talm/tests/rollback-after-failed-upgrade-test.go`
around lines 61 - 77, The single-read of the IBU status can race with
asynchronous updates and trigger an unnecessary abort IBGU; instead, poll
lca.PullImageBasedUpgrade(TargetSNOAPIClient) in a short retry loop (e.g., retry
every few seconds up to a small timeout) re-evaluating
ibu.Object.Status.Conditions for condition.Type == "Idle" && condition.Status ==
"True" before deciding to create the abort IBGU; if the loop times out without
Idle becoming true, then proceed to create the abort IBGU as before.
josclark42
force-pushed
the
AddIbiPreinstallTest
branch
from
9a11491 to
bdeb487
Compare
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (6)
tests/lca/imagebasedinstall/cnf/ran/preinstall/README.md (1)
6-7:⚠️ Potential issue | 🟡 MinorUse "bare-metal host" (hyphenated compound adjective).
For consistent technical phrasing, use the hyphenated form "bare-metal host" on lines 6 and 102.
Suggested fix
-2. Provisioning a bare metal host using the generated ISO +2. Provisioning a bare-metal host using the generated ISOAlso at line 102:
-3. **Test 2**: Provision bare metal host +3. **Test 2**: Provision bare-metal host🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/lca/imagebasedinstall/cnf/ran/preinstall/README.md` around lines 6 - 7, Replace the unhyphenated phrase "bare metal host" with the hyphenated compound adjective "bare-metal host" in the README occurrences (search for the exact string "bare metal host" used in the prose near the provisioning and preinstall steps) so both instances are updated for consistent technical phrasing.tests/lca/imagebasedinstall/cnf/ran/preinstall/internal/helpers/bmh_provisioning.go (2)
78-95:⚠️ Potential issue | 🟠 MajorPreserve the last SSH error in the timeout message.
When SSH fails (bad key, unreachable host), all errors are discarded and the function reports only a generic timeout. This makes debugging difficult. Track the last error and include it in the timeout failure.
Suggested fix
func WaitForPreinstallCompletion(host, user, sshKeyPath string, timeout, pollInterval time.Duration) error { klog.Infof("Waiting for preinstall to complete on %s", host) startTime := time.Now() + var lastErr error for time.Since(startTime) < timeout { cmd := "journalctl -l -u install-rhcos-and-restore-seed.service | tail -2" output, err := SSHExecOnProvisioningHost(host, user, sshKeyPath, cmd) + if err != nil { + lastErr = err + } if err == nil { if strings.Contains(output, "Finished SNO Image-based Installation") || strings.Contains(output, "Finished SNO Image Based Installation") { klog.Infof("Preinstall completed successfully on %s", host) return nil } } klog.V(5).Infof("Preinstall not yet complete, waiting %v...", pollInterval) time.Sleep(pollInterval) } + if lastErr != nil { + return fmt.Errorf("timed out waiting for preinstall after %v: last ssh error: %w", timeout, lastErr) + } + return fmt.Errorf("timed out waiting for preinstall to complete after %v", timeout) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/lca/imagebasedinstall/cnf/ran/preinstall/internal/helpers/bmh_provisioning.go` around lines 78 - 95, The loop in the function that polls journalctl discards SSH errors so a timeout returns only a generic message; capture the most recent SSH error from SSHExecOnProvisioningHost (e.g., store it in a variable like lastErr each time SSHExecOnProvisioningHost returns a non-nil err) and after the loop include that error in the returned fmt.Errorf (e.g., "timed out ...; last SSH error: %v", lastErr) so the timeout message preserves the underlying SSH failure; update references in bmh_provisioning.go accordingly.
111-126:⚠️ Potential issue | 🟠 MajorWait for resources to be fully deleted before returning.
Deleteonly initiates removal. The E2E test immediately recreates resources with the same names (lines 108-116 and 204-222 of preinstall-e2e-test.go), which can race with Metal3 finalizers causingAlreadyExistsorTerminatingerrors.Suggested fix - add polling for deletion completion
+import ( + "time" + // ... existing imports + "k8s.io/apimachinery/pkg/util/wait" +) func DeletePreinstallBMHResources(apiClient *clients.Settings, bmhName, bmhNamespace, bmcSecretName string) error { // ... existing validation and delete code ... err = apiClient.Delete(ctx, bmhObj) if err != nil && !k8serrors.IsNotFound(err) { return fmt.Errorf("delete bmh: %w", err) } + // Wait for BMH to be fully removed + err = wait.PollUntilContextTimeout(ctx, 2*time.Second, 60*time.Second, true, func(ctx context.Context) (bool, error) { + checkErr := apiClient.Get(ctx, goclient.ObjectKey{Namespace: bmhNamespace, Name: bmhName}, &bmhv1alpha1.BareMetalHost{}) + if k8serrors.IsNotFound(checkErr) { + return true, nil + } + return false, nil + }) + if err != nil { + return fmt.Errorf("wait for bmh deletion: %w", err) + } err = apiClient.Secrets(bmhNamespace).Delete(ctx, bmcSecretName, metav1.DeleteOptions{}) // ... }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/lca/imagebasedinstall/cnf/ran/preinstall/internal/helpers/bmh_provisioning.go` around lines 111 - 126, The deletion code currently calls apiClient.Delete for the BareMetalHost (bmhObj) and apiClient.Secrets(...).Delete for the bmcSecretName but returns immediately, causing races with finalizers; change the logic in the block handling bmhObj and the subsequent secret deletion to poll until the resources are actually gone (e.g., loop with timeout/context and short backoff) by attempting apiClient.Get for the BareMetalHost (using goclient.ObjectKey{Namespace: bmhNamespace, Name: bmhName}) and apiClient.Secrets(bmhNamespace).Get for bmcSecretName and only proceed/return success once both return a NotFound error (or timeout with a clear error), ensuring you still handle and propagate non-NotFound errors from Get/Delete appropriately.tests/lca/imagebasedinstall/cnf/ran/preinstall/tests/preinstall-e2e-test.go (1)
188-200:⚠️ Potential issue | 🟠 MajorISO URL filename does not match the uploaded file path.
The ISO is uploaded to
RemoteISOPath(line 193), butisoURLis constructed using the hardcoded"rhcos-ibi.iso"(line 200). IfRemoteISOPathhas a different filename, the BMH will reference a non-existent file.Suggested fix
err = helpers.SCPToProvisioningHost( isoPath, remoteISO, raninittools.RanConfig.ProvisioningHost, raninittools.RanConfig.ProvisioningUser, raninittools.RanConfig.EffectiveProvisioningSSHKey(), ) Expect(err).NotTo(HaveOccurred()) - isoURL := raninittools.RanConfig.ISOArtifactURL("rhcos-ibi.iso") + isoURL := raninittools.RanConfig.ISOArtifactURL(filepath.Base(remoteISO))🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/lca/imagebasedinstall/cnf/ran/preinstall/tests/preinstall-e2e-test.go` around lines 188 - 200, The ISO URL construction mismatches the uploaded filename: after uploading to remoteISO (raninittools.RanConfig.RemoteISOPath) via helpers.SCPToProvisioningHost, replace the hardcoded "rhcos-ibi.iso" when calling raninittools.RanConfig.ISOArtifactURL with the actual uploaded filename (e.g., derive the basename from remoteISO) so isoURL points to the same file you uploaded; update the code around remoteISO and isoURL (where ISOArtifactURL is called) to use that basename.tests/lca/imagebasedinstall/cnf/ran/preinstall/internal/helpers/hub_resources.go (1)
55-62:⚠️ Potential issue | 🟠 MajorAdd existence check before dereferencing
mcBuilder.Object.
mco.PullMachineConfigmay return a builder with a nilObjectif the resource doesn't exist or has issues. UnlikeGetPullSecretFromHubandGetCACertFromHubwhich callExists(), this function dereferencesmcBuilder.Object.Spec.Config.Rawdirectly after error check.Suggested guard
mcBuilder, err := mco.PullMachineConfig(apiClient, "99-master-ssh") if err != nil { return "", fmt.Errorf("failed to pull MachineConfig 99-master-ssh: %w", err) } + + if mcBuilder == nil || !mcBuilder.Exists() || mcBuilder.Object == nil { + return "", fmt.Errorf("MachineConfig 99-master-ssh not found") + } var configData map[string]interface{}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/lca/imagebasedinstall/cnf/ran/preinstall/internal/helpers/hub_resources.go` around lines 55 - 62, The code dereferences mcBuilder.Object.Spec.Config.Raw without verifying mcBuilder.Object exists; add a nil-check (or call mcBuilder.Exists() like GetPullSecretFromHub/GetCACertFromHub do) after PullMachineConfig returns and before json.Unmarshal, and return a clear error if the object is nil/does not exist (e.g., "machine config 99-master-ssh not found" or wrap an error) so you avoid panics when mcBuilder.Object is nil.tests/lca/imagebasedinstall/cnf/ran/preinstall/internal/helpers/hub_image_digest_sources.go (1)
142-162:⚠️ Potential issue | 🟠 Major
registryLocationFromConfmay return alocationfrom a subsequent[[registry]]block.After finding the
registryPrefix, the function scans up to 4KB and returns the lastlocation=match. If another[[registry]]block appears in that window, this returns the wrong mirror. Truncate the window at the next[[registry]]and return the first match.Suggested fix
func registryLocationFromConf(registriesConf, registryPrefix string) string { idx := strings.Index(registriesConf, registryPrefix) if idx < 0 { return "" } window := registriesConf[idx:] - if len(window) > 4000 { - window = window[:4000] - } + // Truncate at next [[registry]] block to stay within the current block + if nextBlock := strings.Index(window[1:], "[[registry]]"); nextBlock >= 0 { + window = window[:nextBlock+1] + } else if len(window) > 4000 { + window = window[:4000] + } re := regexp.MustCompile(`location\s*=\s*"([^"]+)"`) - matches := re.FindAllStringSubmatch(window, -1) + matches := re.FindStringSubmatch(window) if len(matches) == 0 { return "" } - return matches[len(matches)-1][1] + return matches[1] }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/lca/imagebasedinstall/cnf/ran/preinstall/internal/helpers/hub_image_digest_sources.go` around lines 142 - 162, The function registryLocationFromConf currently scans a 4KB window after registryPrefix and returns the last location= match, which can pick up a location from a subsequent [[registry]] block; fix it by truncating the window at the next occurrence of the literal string "[[registry]]" (if present) immediately after idx, then search that truncated window and return the first regex match (matches[0][1]) instead of the last one; update the logic in registryLocationFromConf to perform the "[[registry]]" cut before calling re.FindAllStringSubmatch and to pick the first match.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In
`@tests/lca/imagebasedinstall/cnf/ran/preinstall/internal/helpers/bmh_provisioning.go`:
- Around line 78-95: The loop in the function that polls journalctl discards SSH
errors so a timeout returns only a generic message; capture the most recent SSH
error from SSHExecOnProvisioningHost (e.g., store it in a variable like lastErr
each time SSHExecOnProvisioningHost returns a non-nil err) and after the loop
include that error in the returned fmt.Errorf (e.g., "timed out ...; last SSH
error: %v", lastErr) so the timeout message preserves the underlying SSH
failure; update references in bmh_provisioning.go accordingly.
- Around line 111-126: The deletion code currently calls apiClient.Delete for
the BareMetalHost (bmhObj) and apiClient.Secrets(...).Delete for the
bmcSecretName but returns immediately, causing races with finalizers; change the
logic in the block handling bmhObj and the subsequent secret deletion to poll
until the resources are actually gone (e.g., loop with timeout/context and short
backoff) by attempting apiClient.Get for the BareMetalHost (using
goclient.ObjectKey{Namespace: bmhNamespace, Name: bmhName}) and
apiClient.Secrets(bmhNamespace).Get for bmcSecretName and only proceed/return
success once both return a NotFound error (or timeout with a clear error),
ensuring you still handle and propagate non-NotFound errors from Get/Delete
appropriately.
In
`@tests/lca/imagebasedinstall/cnf/ran/preinstall/internal/helpers/hub_image_digest_sources.go`:
- Around line 142-162: The function registryLocationFromConf currently scans a
4KB window after registryPrefix and returns the last location= match, which can
pick up a location from a subsequent [[registry]] block; fix it by truncating
the window at the next occurrence of the literal string "[[registry]]" (if
present) immediately after idx, then search that truncated window and return the
first regex match (matches[0][1]) instead of the last one; update the logic in
registryLocationFromConf to perform the "[[registry]]" cut before calling
re.FindAllStringSubmatch and to pick the first match.
In
`@tests/lca/imagebasedinstall/cnf/ran/preinstall/internal/helpers/hub_resources.go`:
- Around line 55-62: The code dereferences mcBuilder.Object.Spec.Config.Raw
without verifying mcBuilder.Object exists; add a nil-check (or call
mcBuilder.Exists() like GetPullSecretFromHub/GetCACertFromHub do) after
PullMachineConfig returns and before json.Unmarshal, and return a clear error if
the object is nil/does not exist (e.g., "machine config 99-master-ssh not found"
or wrap an error) so you avoid panics when mcBuilder.Object is nil.
In `@tests/lca/imagebasedinstall/cnf/ran/preinstall/README.md`:
- Around line 6-7: Replace the unhyphenated phrase "bare metal host" with the
hyphenated compound adjective "bare-metal host" in the README occurrences
(search for the exact string "bare metal host" used in the prose near the
provisioning and preinstall steps) so both instances are updated for consistent
technical phrasing.
In `@tests/lca/imagebasedinstall/cnf/ran/preinstall/tests/preinstall-e2e-test.go`:
- Around line 188-200: The ISO URL construction mismatches the uploaded
filename: after uploading to remoteISO (raninittools.RanConfig.RemoteISOPath)
via helpers.SCPToProvisioningHost, replace the hardcoded "rhcos-ibi.iso" when
calling raninittools.RanConfig.ISOArtifactURL with the actual uploaded filename
(e.g., derive the basename from remoteISO) so isoURL points to the same file you
uploaded; update the code around remoteISO and isoURL (where ISOArtifactURL is
called) to use that basename.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: b523471d-c568-49dd-bbb9-5b6916c1a4d9
⛔ Files ignored due to path filters (2)
go.sumis excluded by!**/*.sumvendor/modules.txtis excluded by!vendor/**
📒 Files selected for processing (22)
.golangci.ymlgo.modscripts/golangci-lint.shtests/lca/imagebasedinstall/cnf/ran/internal/ranconfig/config.gotests/lca/imagebasedinstall/cnf/ran/internal/ranconfig/default.yamltests/lca/imagebasedinstall/cnf/ran/internal/raninittools/raninittools.gotests/lca/imagebasedinstall/cnf/ran/internal/ranparams/const.gotests/lca/imagebasedinstall/cnf/ran/internal/ranparams/ranvars.gotests/lca/imagebasedinstall/cnf/ran/preinstall/README.mdtests/lca/imagebasedinstall/cnf/ran/preinstall/internal/helpers/bmh_provisioning.gotests/lca/imagebasedinstall/cnf/ran/preinstall/internal/helpers/cluster_instance.gotests/lca/imagebasedinstall/cnf/ran/preinstall/internal/helpers/hub_image_digest_sources.gotests/lca/imagebasedinstall/cnf/ran/preinstall/internal/helpers/hub_resources.gotests/lca/imagebasedinstall/cnf/ran/preinstall/internal/helpers/ibi_config.gotests/lca/imagebasedinstall/cnf/ran/preinstall/internal/helpers/iso_generation.gotests/lca/imagebasedinstall/cnf/ran/preinstall/internal/helpers/openshift_install.gotests/lca/imagebasedinstall/cnf/ran/preinstall/internal/helpers/ssh_utils.gotests/lca/imagebasedinstall/cnf/ran/preinstall/internal/helpers/ztp_config.gotests/lca/imagebasedinstall/cnf/ran/preinstall/internal/tsparams/const.gotests/lca/imagebasedinstall/cnf/ran/preinstall/internal/tsparams/preinstallvars.gotests/lca/imagebasedinstall/cnf/ran/preinstall/preinstall_suite_test.gotests/lca/imagebasedinstall/cnf/ran/preinstall/tests/preinstall-e2e-test.go
✅ Files skipped from review due to trivial changes (7)
- tests/lca/imagebasedinstall/cnf/ran/internal/ranparams/ranvars.go
- tests/lca/imagebasedinstall/cnf/ran/internal/ranparams/const.go
- tests/lca/imagebasedinstall/cnf/ran/internal/raninittools/raninittools.go
- tests/lca/imagebasedinstall/cnf/ran/internal/ranconfig/default.yaml
- tests/lca/imagebasedinstall/cnf/ran/preinstall/internal/tsparams/const.go
- tests/lca/imagebasedinstall/cnf/ran/preinstall/internal/tsparams/preinstallvars.go
- tests/lca/imagebasedinstall/cnf/ran/preinstall/internal/helpers/openshift_install.go
🚧 Files skipped from review as they are similar to previous changes (8)
- .golangci.yml
- scripts/golangci-lint.sh
- tests/lca/imagebasedinstall/cnf/ran/preinstall/internal/helpers/iso_generation.go
- go.mod
- tests/lca/imagebasedinstall/cnf/ran/preinstall/internal/helpers/ztp_config.go
- tests/lca/imagebasedinstall/cnf/ran/preinstall/internal/helpers/ssh_utils.go
- tests/lca/imagebasedinstall/cnf/ran/preinstall/internal/helpers/ibi_config.go
- tests/lca/imagebasedinstall/cnf/ran/internal/ranconfig/config.go
josclark42
force-pushed
the
AddIbiPreinstallTest
branch
from
bdeb487 to
e76ddbc
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (3)
tests/lca/imagebasedinstall/cnf/ran/preinstall/internal/helpers/hub_resources.go (1)
55-63:⚠️ Potential issue | 🟠 MajorGuard the pulled
MachineConfigbefore dereferencing it.This still dereferences
mcBuilder.Object.Spec.Config.Rawwithout checking whethermco.PullMachineConfigreturned a missing or nil object. Line 62 will panic on an absent99-master-sshinstead of returning a useful setup error.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/lca/imagebasedinstall/cnf/ran/preinstall/internal/helpers/hub_resources.go` around lines 55 - 63, The code dereferences mcBuilder.Object.Spec.Config.Raw after calling mco.PullMachineConfig without checking for a nil/missing MachineConfig; update the logic that calls mco.PullMachineConfig (variable mcBuilder) to validate mcBuilder and its nested fields (mcBuilder.Object, mcBuilder.Object.Spec, and mcBuilder.Object.Spec.Config) are non-nil before accessing Raw, and if any are nil return a clear formatted error (e.g., "missing MachineConfig 99-master-ssh or invalid spec") instead of proceeding to json.Unmarshal.tests/lca/imagebasedinstall/cnf/ran/internal/ranconfig/config.go (1)
189-201:⚠️ Potential issue | 🟡 MinorReject non-executable
bootstrap_ocpaths early.This accepts any existing file. If the configured path exists but lacks execute permission, the suite fails much later when
ocis invoked instead of surfacing a clear config error here.💡 Proposed fix
} else { if fi.IsDir() { return "", fmt.Errorf("bootstrap oc %q is a directory", bootstrapPath) } + if fi.Mode()&0o111 == 0 { + return "", fmt.Errorf("bootstrap oc %q is not executable", bootstrapPath) + } return bootstrapPath, nil }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/lca/imagebasedinstall/cnf/ran/internal/ranconfig/config.go` around lines 189 - 201, The code currently accepts any existing file at bootstrapPath without verifying it is executable; after the os.Stat and fi.IsDir checks in the function handling bootstrap oc, add a permission check on fi (the FileInfo returned by os.Stat) to ensure the file has at least one execute bit for the user/group/other (e.g., fi.Mode().Perm() & 0111 != 0); if it is not executable, return a clear error like "bootstrap oc %q is not executable" referencing bootstrapPath so misconfiguration is surfaced early. Ensure this check runs before returning bootstrapPath, so non-executable files are rejected immediately.tests/lca/imagebasedinstall/cnf/ran/preinstall/README.md (1)
6-6:⚠️ Potential issue | 🟡 MinorUse hyphenated compound adjective.
Change "bare metal host" to "bare-metal host" for correct technical phrasing (also applies to line 102).
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/lca/imagebasedinstall/cnf/ran/preinstall/README.md` at line 6, Replace the un-hyphenated phrase "bare metal host" with the hyphenated compound adjective "bare-metal host" wherever it appears (e.g., the instances in the README that currently read "bare metal host" around the provisioning steps and the later occurrence near line 102) to use correct technical phrasing.
🧹 Nitpick comments (1)
tests/lca/imagebasedinstall/cnf/ran/preinstall/README.md (1)
13-13: Break down the long technical description for readability.Line 13 contains over 500 characters in a single bullet point describing
internal/helpers. Consider restructuring this as a parent bullet with sub-bullets for each major category (e.g., ZTP/kustomize operations, hub resource retrieval, image digest handling, installer API, provisioning, subprocess environment).📝 Example restructure
-- `internal/helpers` — ZTP clone (go-git), kustomize, **typed** ClusterInstance (`siteconfig.open-cluster-management.io/v1alpha1`) parsing, hub pull secret / CA / SSH, **image digest mirrors** (IDMS / ICSP / mirror-registry-ca), **installer API types** (`github.com/openshift/installer/pkg/types/imagebased.InstallationConfig`) for `image-based-installation-config.yaml`, BMH + BMC secret (live-ISO), SSH/SCP, `oc adm release extract` for `openshift-install` + `oc` (hub pull secret as `--registry-config`, hub kubeconfig as `KUBECONFIG` for that subprocess so it does not use the spoke-only default in the container) +- `internal/helpers` — suite-specific utilities: + - ZTP clone (go-git) and kustomize + - **Typed** ClusterInstance (`siteconfig.open-cluster-management.io/v1alpha1`) parsing + - Hub resource retrieval (pull secret, CA, SSH) + - **Image digest mirrors** (IDMS / ICSP / mirror-registry-ca) + - **Installer API types** (`github.com/openshift/installer/pkg/types/imagebased.InstallationConfig`) for `image-based-installation-config.yaml` + - BMH + BMC secret (live-ISO), SSH/SCP + - `oc adm release extract` for `openshift-install` + `oc` (hub pull secret as `--registry-config`, hub kubeconfig as `KUBECONFIG` for subprocess isolation)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/lca/imagebasedinstall/cnf/ran/preinstall/README.md` at line 13, The long single-line bullet describing internal/helpers should be split into a parent bullet and concise sub-bullets to improve readability: modify the README entry for internal/helpers to list high-level categories (ZTP & kustomize ops, hub resource retrieval, image digest/mirror handling, installer API types, provisioning/credentials, and subprocess utilities) and move details into short sub-bullets that mention relevant identifiers like ClusterInstance (`siteconfig.open-cluster-management.io/v1alpha1`), image-based-installation-config.yaml / InstallationConfig (`github.com/openshift/installer/pkg/types/imagebased.InstallationConfig`), IDMS/ICSP/mirror-registry-ca, BMH + BMC secret, and the use of `oc adm release extract` with registry-config / KUBECONFIG so each line is focused and much shorter.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@tests/lca/imagebasedinstall/cnf/ran/internal/ranconfig/config.go`:
- Around line 42-43: Remove the YAML tags from the BMC credentials so they are
only sourced from environment variables: update the struct fields BMCUsername
and BMCPassword in config.go to drop the `yaml:"..."` tags and keep only the
`envconfig:"ECO_LCA_IBI_BMC_USERNAME"` and
`envconfig:"ECO_LCA_IBI_BMC_PASSWORD"` tags (or otherwise ensure those fields
are not unmarshaled from YAML), so BMC credentials are env-only.
In
`@tests/lca/imagebasedinstall/cnf/ran/preinstall/internal/helpers/cluster_instance.go`:
- Around line 79-103: IBICPUArchitectureFromClusterInstance currently only maps
"aarch64"→"arm64" and leaves "x86_64" unchanged; update the mapping inside that
function (after s := string(arch) or where architectures are checked) to also
translate "x86_64" to "amd64" (i.e., add a branch that returns "amd64" when s ==
"x86_64") so the value returned by IBICPUArchitectureFromClusterInstance matches
the OpenShift installer expectation.
In
`@tests/lca/imagebasedinstall/cnf/ran/preinstall/internal/helpers/hub_image_digest_sources.go`:
- Around line 61-77: The current logic returns an error as soon as an
ImageDigestMirrorSet (idms) exists but has no usable mirrors, preventing the
subsequent check of the fallback redhat-internal-icsp; change the flow so that
when idms != nil you still scan entries and return host if found, but do NOT
return an error immediately after the loop — instead continue to check the ICSP
fallback (redhat-internal-icsp) and only return an error if both IDMS produced
no usable mirrors and ICSP also yields no usable host; update the block
referencing idms, idmsName, and entry.Mirrors to remove the immediate return of
fmt.Errorf and defer error reporting until after ICSP is checked.
In
`@tests/lca/imagebasedinstall/cnf/ran/preinstall/internal/helpers/ibi_config.go`:
- Around line 138-145: SeedVersionFromSeedImage currently returns the substring
after the last ':' which can be the digest fragment for digest-pinned images
(e.g., quay.io/...@sha256:...), producing an invalid seedVersion; update the
function to first strip any digest by splitting seedImage on '@' and taking the
left side, then parse the remaining reference for a tag by locating the last ':'
and returning the trailing part only if that ':' is part of a tag (i.e., after
the last '/' ); ensure it returns empty when no tag exists; refer to the
SeedVersionFromSeedImage function and the seedImage parameter when making this
change.
---
Duplicate comments:
In `@tests/lca/imagebasedinstall/cnf/ran/internal/ranconfig/config.go`:
- Around line 189-201: The code currently accepts any existing file at
bootstrapPath without verifying it is executable; after the os.Stat and fi.IsDir
checks in the function handling bootstrap oc, add a permission check on fi (the
FileInfo returned by os.Stat) to ensure the file has at least one execute bit
for the user/group/other (e.g., fi.Mode().Perm() & 0111 != 0); if it is not
executable, return a clear error like "bootstrap oc %q is not executable"
referencing bootstrapPath so misconfiguration is surfaced early. Ensure this
check runs before returning bootstrapPath, so non-executable files are rejected
immediately.
In
`@tests/lca/imagebasedinstall/cnf/ran/preinstall/internal/helpers/hub_resources.go`:
- Around line 55-63: The code dereferences mcBuilder.Object.Spec.Config.Raw
after calling mco.PullMachineConfig without checking for a nil/missing
MachineConfig; update the logic that calls mco.PullMachineConfig (variable
mcBuilder) to validate mcBuilder and its nested fields (mcBuilder.Object,
mcBuilder.Object.Spec, and mcBuilder.Object.Spec.Config) are non-nil before
accessing Raw, and if any are nil return a clear formatted error (e.g., "missing
MachineConfig 99-master-ssh or invalid spec") instead of proceeding to
json.Unmarshal.
In `@tests/lca/imagebasedinstall/cnf/ran/preinstall/README.md`:
- Line 6: Replace the un-hyphenated phrase "bare metal host" with the hyphenated
compound adjective "bare-metal host" wherever it appears (e.g., the instances in
the README that currently read "bare metal host" around the provisioning steps
and the later occurrence near line 102) to use correct technical phrasing.
---
Nitpick comments:
In `@tests/lca/imagebasedinstall/cnf/ran/preinstall/README.md`:
- Line 13: The long single-line bullet describing internal/helpers should be
split into a parent bullet and concise sub-bullets to improve readability:
modify the README entry for internal/helpers to list high-level categories (ZTP
& kustomize ops, hub resource retrieval, image digest/mirror handling, installer
API types, provisioning/credentials, and subprocess utilities) and move details
into short sub-bullets that mention relevant identifiers like ClusterInstance
(`siteconfig.open-cluster-management.io/v1alpha1`),
image-based-installation-config.yaml / InstallationConfig
(`github.com/openshift/installer/pkg/types/imagebased.InstallationConfig`),
IDMS/ICSP/mirror-registry-ca, BMH + BMC secret, and the use of `oc adm release
extract` with registry-config / KUBECONFIG so each line is focused and much
shorter.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: ae9ab3ec-ae20-4470-843d-f43bfe28e38e
⛔ Files ignored due to path filters (2)
go.sumis excluded by!**/*.sumvendor/modules.txtis excluded by!vendor/**
📒 Files selected for processing (22)
.golangci.ymlgo.modscripts/golangci-lint.shtests/lca/imagebasedinstall/cnf/ran/internal/ranconfig/config.gotests/lca/imagebasedinstall/cnf/ran/internal/ranconfig/default.yamltests/lca/imagebasedinstall/cnf/ran/internal/raninittools/raninittools.gotests/lca/imagebasedinstall/cnf/ran/internal/ranparams/const.gotests/lca/imagebasedinstall/cnf/ran/internal/ranparams/ranvars.gotests/lca/imagebasedinstall/cnf/ran/preinstall/README.mdtests/lca/imagebasedinstall/cnf/ran/preinstall/internal/helpers/bmh_provisioning.gotests/lca/imagebasedinstall/cnf/ran/preinstall/internal/helpers/cluster_instance.gotests/lca/imagebasedinstall/cnf/ran/preinstall/internal/helpers/hub_image_digest_sources.gotests/lca/imagebasedinstall/cnf/ran/preinstall/internal/helpers/hub_resources.gotests/lca/imagebasedinstall/cnf/ran/preinstall/internal/helpers/ibi_config.gotests/lca/imagebasedinstall/cnf/ran/preinstall/internal/helpers/iso_generation.gotests/lca/imagebasedinstall/cnf/ran/preinstall/internal/helpers/openshift_install.gotests/lca/imagebasedinstall/cnf/ran/preinstall/internal/helpers/ssh_utils.gotests/lca/imagebasedinstall/cnf/ran/preinstall/internal/helpers/ztp_config.gotests/lca/imagebasedinstall/cnf/ran/preinstall/internal/tsparams/const.gotests/lca/imagebasedinstall/cnf/ran/preinstall/internal/tsparams/preinstallvars.gotests/lca/imagebasedinstall/cnf/ran/preinstall/preinstall_suite_test.gotests/lca/imagebasedinstall/cnf/ran/preinstall/tests/preinstall-e2e-test.go
✅ Files skipped from review due to trivial changes (9)
- tests/lca/imagebasedinstall/cnf/ran/internal/ranparams/const.go
- tests/lca/imagebasedinstall/cnf/ran/preinstall/internal/tsparams/const.go
- tests/lca/imagebasedinstall/cnf/ran/internal/ranconfig/default.yaml
- tests/lca/imagebasedinstall/cnf/ran/preinstall/internal/helpers/ztp_config.go
- tests/lca/imagebasedinstall/cnf/ran/preinstall/internal/tsparams/preinstallvars.go
- tests/lca/imagebasedinstall/cnf/ran/preinstall/internal/helpers/openshift_install.go
- tests/lca/imagebasedinstall/cnf/ran/preinstall/preinstall_suite_test.go
- tests/lca/imagebasedinstall/cnf/ran/preinstall/internal/helpers/ssh_utils.go
- tests/lca/imagebasedinstall/cnf/ran/preinstall/tests/preinstall-e2e-test.go
🚧 Files skipped from review as they are similar to previous changes (4)
- scripts/golangci-lint.sh
- .golangci.yml
- tests/lca/imagebasedinstall/cnf/ran/internal/ranparams/ranvars.go
- tests/lca/imagebasedinstall/cnf/ran/preinstall/internal/helpers/iso_generation.go
Comment on lines
+42
to
+43
| BMCUsername string `yaml:"bmc_username" envconfig:"ECO_LCA_IBI_BMC_USERNAME"` | ||
| BMCPassword string `yaml:"bmc_password" envconfig:"ECO_LCA_IBI_BMC_PASSWORD"` |
Contributor
There was a problem hiding this comment.
Keep BMC credentials env-only.
These YAML tags allow file-backed config to supply BMC credentials. That contradicts the stated requirement for this flow that BMC username/password come from environment variables only.
💡 Proposed fix
- BMCUsername string `yaml:"bmc_username" envconfig:"ECO_LCA_IBI_BMC_USERNAME"`
- BMCPassword string `yaml:"bmc_password" envconfig:"ECO_LCA_IBI_BMC_PASSWORD"`
+ BMCUsername string `yaml:"-" envconfig:"ECO_LCA_IBI_BMC_USERNAME"`
+ BMCPassword string `yaml:"-" envconfig:"ECO_LCA_IBI_BMC_PASSWORD"`📝 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
| BMCUsername string `yaml:"bmc_username" envconfig:"ECO_LCA_IBI_BMC_USERNAME"` | |
| BMCPassword string `yaml:"bmc_password" envconfig:"ECO_LCA_IBI_BMC_PASSWORD"` | |
| BMCUsername string `yaml:"-" envconfig:"ECO_LCA_IBI_BMC_USERNAME"` | |
| BMCPassword string `yaml:"-" envconfig:"ECO_LCA_IBI_BMC_PASSWORD"` |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/lca/imagebasedinstall/cnf/ran/internal/ranconfig/config.go` around
lines 42 - 43, Remove the YAML tags from the BMC credentials so they are only
sourced from environment variables: update the struct fields BMCUsername and
BMCPassword in config.go to drop the `yaml:"..."` tags and keep only the
`envconfig:"ECO_LCA_IBI_BMC_USERNAME"` and
`envconfig:"ECO_LCA_IBI_BMC_PASSWORD"` tags (or otherwise ensure those fields
are not unmarshaled from YAML), so BMC credentials are env-only.
Comment on lines
+79
to
+103
| // IBICPUArchitectureFromClusterInstance returns spec.cpuArchitecture or spec.nodes[0].cpuArchitecture | ||
| // mapped for ImageBasedInstallationConfig (aarch64 -> arm64), matching the Ansible playbook. | ||
| func IBICPUArchitectureFromClusterInstance(clusterInst *siteconfigv1alpha1.ClusterInstance) (string, error) { | ||
| if clusterInst == nil { | ||
| return "", fmt.Errorf("clusterinstance: nil") | ||
| } | ||
|
|
||
| var arch siteconfigv1alpha1.CPUArchitecture | ||
|
|
||
| if clusterInst.Spec.CPUArchitecture != "" { | ||
| arch = clusterInst.Spec.CPUArchitecture | ||
| } else if node, err := firstNode(clusterInst); err == nil && node.CPUArchitecture != "" { | ||
| arch = node.CPUArchitecture | ||
| } | ||
|
|
||
| if arch == "" { | ||
| return "", nil | ||
| } | ||
|
|
||
| s := string(arch) | ||
| if s == "aarch64" { | ||
| return "arm64", nil | ||
| } | ||
|
|
||
| return s, nil |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "SiteConfig CPU architecture definitions:"
rg -n -C2 'type CPUArchitecture|x86_64|aarch64' vendor
echo
echo "Installer architecture definitions:"
rg -n -C3 'type Architecture|amd64|arm64|x86_64' vendorRepository: rh-ecosystem-edge/eco-gotests
Length of output: 50386
Add missing x86_64 → amd64 translation.
The siteconfig side uses x86_64, but the OpenShift installer expects amd64. This function only translates aarch64 → arm64 and returns x86_64 unchanged, which will be rejected as an unsupported architecture by the installer. Add the missing translation:
if s == "x86_64" {
return "amd64", nil
}
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@tests/lca/imagebasedinstall/cnf/ran/preinstall/internal/helpers/cluster_instance.go`
around lines 79 - 103, IBICPUArchitectureFromClusterInstance currently only maps
"aarch64"→"arm64" and leaves "x86_64" unchanged; update the mapping inside that
function (after s := string(arch) or where architectures are checked) to also
translate "x86_64" to "amd64" (i.e., add a branch that returns "amd64" when s ==
"x86_64") so the value returned by IBICPUArchitectureFromClusterInstance matches
the OpenShift installer expectation.
Comment on lines
+61
to
+77
| } else if idms != nil && len(idms.Spec.ImageDigestMirrors) > 0 { | ||
| for _, entry := range idms.Spec.ImageDigestMirrors { | ||
| if len(entry.Mirrors) == 0 { | ||
| continue | ||
| } | ||
|
|
||
| full := string(entry.Mirrors[0]) | ||
| host := strings.Split(full, "/")[0] | ||
|
|
||
| if host != "" { | ||
| klog.Infof("Using mirror host from ImageDigestMirrorSet %q: %s", idmsName, host) | ||
|
|
||
| return host, nil | ||
| } | ||
| } | ||
|
|
||
| return "", fmt.Errorf("ImageDigestMirrorSet %q exists but has no usable mirrors", idmsName) |
Contributor
There was a problem hiding this comment.
Keep the ICSP fallback when IDMS exists but is unusable.
If image-digest-mirror exists but has no usable mirrors, the function returns here and never checks redhat-internal-icsp. That makes the disconnected flow fail even when the ICSP still contains a valid mirror host.
💡 Proposed fix
- return "", fmt.Errorf("ImageDigestMirrorSet %q exists but has no usable mirrors", idmsName)
+ klog.Warningf(
+ "ImageDigestMirrorSet %q has no usable mirrors, falling back to ICSP %q",
+ idmsName, icspName,
+ )
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@tests/lca/imagebasedinstall/cnf/ran/preinstall/internal/helpers/hub_image_digest_sources.go`
around lines 61 - 77, The current logic returns an error as soon as an
ImageDigestMirrorSet (idms) exists but has no usable mirrors, preventing the
subsequent check of the fallback redhat-internal-icsp; change the flow so that
when idms != nil you still scan entries and return host if found, but do NOT
return an error immediately after the loop — instead continue to check the ICSP
fallback (redhat-internal-icsp) and only return an error if both IDMS produced
no usable mirrors and ICSP also yields no usable host; update the block
referencing idms, idmsName, and entry.Mirrors to remove the immediate return of
fmt.Errorf and defer error reporting until after ICSP is checked.
Comment on lines
+138
to
+145
| // SeedVersionFromSeedImage returns the tag after the last ':' (Ansible ibi_ocp_build from ibi_seed_image). | ||
| func SeedVersionFromSeedImage(seedImage string) string { | ||
| parts := strings.Split(seedImage, ":") | ||
| if len(parts) == 0 { | ||
| return "" | ||
| } | ||
|
|
||
| return parts[len(parts)-1] |
Contributor
There was a problem hiding this comment.
Don't derive SeedVersion from the digest suffix.
For values like quay.io/org/seed@sha256:... or quay.io/org/seed:4.21@sha256:..., this returns the text after the last :, which is the digest fragment, not the release version. That will write an invalid seedVersion for digest-pinned inputs.
💡 Proposed fix
func SeedVersionFromSeedImage(seedImage string) string {
- parts := strings.Split(seedImage, ":")
- if len(parts) == 0 {
- return ""
- }
-
- return parts[len(parts)-1]
+ withoutDigest, _, _ := strings.Cut(seedImage, "@")
+ idx := strings.LastIndex(withoutDigest, ":")
+ if idx < 0 || idx == len(withoutDigest)-1 {
+ return ""
+ }
+
+ return withoutDigest[idx+1:]
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@tests/lca/imagebasedinstall/cnf/ran/preinstall/internal/helpers/ibi_config.go`
around lines 138 - 145, SeedVersionFromSeedImage currently returns the substring
after the last ':' which can be the digest fragment for digest-pinned images
(e.g., quay.io/...@sha256:...), producing an invalid seedVersion; update the
function to first strip any digest by splitting seedImage on '@' and taking the
left side, then parse the remaining reference for a tag by locating the last ':'
and returning the trailing part only if that ':' is part of a tag (i.e., after
the last '/' ); ensure it returns empty when no tag exists; refer to the
SeedVersionFromSeedImage function and the seedImage parameter when making this
change.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Adds an Image-Based Installation (IBI) preinstall suite under tests/lca/imagebasedinstall/cnf/ran/preinstall f
Workflow:
Loads RanConfig from default.yaml with ECO_LCA_IBI_* env overrides and validates required settings (hub kubeconfig, seed/release images, siteconfig repo/branch, provisioning host, BMC username/password from env only, ISO HTTP base URL, SSH paths, timeouts, bootstrap oc, etc.).
BeforeAll / e2e: oc adm release extract for openshift-install and release-matched oc (bootstrap oc path + hub KUBECONFIG + pull secret as --registry-config); clone siteconfig with go-git (optional TLS skip); kustomize build; openshift-install image-based create image; SCP ISO to provisioning host; create BMC secret + BareMetalHost (live ISO); wait for install-rhcos-and-restore-seed; clean up hub objects.
Config generation: image-based-installation-config.yaml built from imagebased.InstallationConfig (OpenShift installer API), with pullSecret, additionalTrustBundle, and imageDigestSources from the hub (plus typed ClusterInstance / assisted NetConfig from kustomize output).
Docs: preinstall/README.md describes layout, env vars, and flow.
Summary by CodeRabbit
New Features
Documentation
Tests
Improvements
Chores