Freshen up dry run mode for the installer/controller

[omertuc]

The installer/controller dry run mode has not been kept up to date with
recent changes. This commit updates the dry run mode to skip actions
that would modify the system, such as writing to disk or starting
services when in dry run mode.

This will avoid damaging the host system when testing the installer in
dry run mode (used by https://github.com/openshift-assisted/assisted-swarm)

[codecov]

Codecov Report

❌ Patch coverage is 16.66667% with 25 lines in your changes missing coverage. Please review.
✅ Project coverage is 48.14%. Comparing base (753d15c) to head (abed5af).
⚠️ Report is 8 commits behind head on master.

Files with missing lines	Patch %	Lines
src/installer/installer.go	31.25%	6 Missing and 5 partials ⚠️
src/main/drymock/dry_mode_k8s_mock.go	0.00%	9 Missing ⚠️
src/ops/ops.go	0.00%	2 Missing and 1 partial ⚠️
...ed-installer-controller/assisted_installer_main.go	0.00%	2 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #1288      +/-   ##
==========================================
- Coverage   48.28%   48.14%   -0.15%     
==========================================
  Files          20       20              
  Lines        4316     4333      +17     
==========================================
+ Hits         2084     2086       +2     
- Misses       2011     2022      +11     
- Partials      221      225       +4     

Files with missing lines	Coverage Δ
...taller_controller/assisted_installer_controller.go	77.41% <ø> (ø)
...ed-installer-controller/assisted_installer_main.go	26.28% <0.00%> (-0.16%)	⬇️
src/ops/ops.go	48.70% <0.00%> (-0.19%)	⬇️
src/main/drymock/dry_mode_k8s_mock.go	0.00% <0.00%> (ø)
src/installer/installer.go	68.43% <31.25%> (-0.54%)	⬇️

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[openshift-bot]

Issues go stale after 90d of inactivity.

Mark the issue as fresh by commenting /remove-lifecycle stale.
Stale issues rot after an additional 30d of inactivity and eventually close.
Exclude this issue from closing by commenting /lifecycle frozen.

If this issue is safe to close now please do so with /close.

/lifecycle stale

[coderabbitai]

Walkthrough

The changes add and refactor dry-run mode support across multiple components. They include field reordering in controller configuration, conditional logic gates for system operations in dry-run mode, function signature updates to pass control plane count information to dry-run mock preparations, and a short-circuit execution path for image writing in dry-run mode.

Changes

Cohort / File(s)	Summary
Controller Configuration src/assisted_installer_controller/assisted_installer_controller.go	Field reordering: NotifyNumReboots moved to appear before DryRunEnabled within ControllerConfig struct.
Dry-Run Operation Guards src/installer/installer.go, src/ops/ops.go	Added conditional checks for DryRunEnabled to bypass system operations: daemon-reload, NetworkManager restart, node image pull/overlay startup, ETCD bootstrap, and MCO reboot. Introduced short-circuit path in WriteImageToExistingRoot for dry-run execution.
Dry-Run Mock Infrastructure src/main/drymock/dry_mode_k8s_mock.go	Updated function signatures for PrepareControllerDryMock and PrepareInstallerDryK8sMock to accept controlPlaneCount parameter. Added mocks for GetControlPlaneReplicas and GetArbiterReplicas. Updated call site in NewDryRunK8SClientBuilder to pass control plane count.
Main Entry Point src/main/assisted-installer-controller/assisted_installer_main.go	Updated call to PrepareControllerDryMock to include Options.ControllerConfig.ControlPlaneCount as additional argument.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

✨ Finishing touches

• 📝 Generate docstrings

---

---

📜 Recent review details

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Cache: Disabled due to data retention organization setting

Knowledge base: Disabled due to Reviews -> Disable Knowledge Base setting

📥 Commits

Reviewing files that changed from the base of the PR and between 89d9b55 and abed5af.

📒 Files selected for processing (5)

• src/assisted_installer_controller/assisted_installer_controller.go
• src/installer/installer.go
• src/main/assisted-installer-controller/assisted_installer_main.go
• src/main/drymock/dry_mode_k8s_mock.go
• src/ops/ops.go

🚧 Files skipped from review as they are similar to previous changes (1)

• src/installer/installer.go

🧰 Additional context used

📓 Path-based instructions (1)

**

⚙️ CodeRabbit configuration file

-Focus on major issues impacting performance, readability, maintainability and security. Avoid nitpicks and avoid verbosity.

Files:

• src/ops/ops.go
• src/assisted_installer_controller/assisted_installer_controller.go
• src/main/drymock/dry_mode_k8s_mock.go
• src/main/assisted-installer-controller/assisted_installer_main.go

🧬 Code graph analysis (2)

src/main/drymock/dry_mode_k8s_mock.go (2)

src/k8s_client/mock_k8s_client.go (1)

• MockK8SClient (29-32)

src/config/dry_run_config.go (1)

• DryClusterHosts (38-38)

src/main/assisted-installer-controller/assisted_installer_main.go (2)

src/main/drymock/dry_mode_k8s_mock.go (1)

• PrepareControllerDryMock (76-282)

src/assisted_installer_controller/assisted_installer_controller.go (1)

• ControllerConfig (86-103)

🔇 Additional comments (7)

src/assisted_installer_controller/assisted_installer_controller.go (1)

97-97: LGTM - Field reordering for better organization.

The NotifyNumReboots field has been moved to group related configuration options more logically. No functional change.

src/main/drymock/dry_mode_k8s_mock.go (4)

76-80: LGTM - Control plane count propagation for dry-run mocks.

The new controlPlaneCount parameter and corresponding GetControlPlaneReplicas/GetArbiterReplicas mocks allow dry-run mode to properly simulate control plane configuration. The explicit comment about arbiter not being supported in dry-run is helpful documentation.

---

287-291: LGTM - Consistent mock setup for installer dry-run.

The PrepareInstallerDryK8sMock function mirrors the changes made to PrepareControllerDryMock, maintaining consistency across both dry-run mock setups.

---

318-319: LGTM - Proper propagation of controlPlaneCount.

The NewDryRunK8SClientBuilder correctly passes installerConfig.ControlPlaneCount to the mock setup function.

---

114-114: MCS log format is correct and matches controller expectations.

The format at line 114 with port :12345 and User-Agent header User-Agent:"Ignition" correctly matches the parsing regex in the controller (src/common/common.go:67) and will be properly recognized when determining which nodes have downloaded ignition.

src/ops/ops.go (1)

271-277: LGTM - Appropriate dry-run guard for WriteImageToExistingRoot.

This change correctly prevents system modifications (remounting filesystems, deploying to stateroot, applying kargs, etc.) when in dry-run mode. The pattern is consistent with the existing WriteImageToDisk function which also delegates to dry-installer during dry-run.

src/main/assisted-installer-controller/assisted_installer_main.go (1)

101-102: LGTM - Correct propagation of ControlPlaneCount to dry-run mock.

The call site properly supplies the ControlPlaneCount from the controller configuration, enabling the dry-run mock to return accurate control plane replica counts.

_{✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.}

Warning

There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure.

🔧 golangci-lint (2.5.0)

Error: can't load config: unsupported version of the configuration: "" See https://golangci-lint.run/docs/product/migration-guide for migration instructions
The command is terminated due to an error: can't load config: unsupported version of the configuration: "" See https://golangci-lint.run/docs/product/migration-guide for migration instructions

---

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[omertuc]

/remove-lifecycle stale

[rccrdpccl]

In general I'd prefer if we push dry-run flag (or options) within each command, so we wouldn't pollute with if statement the logic flow - a bit like it's done here:

func (i *installer) skipMcoReboot(ignitionPath string) {
	if i.DryRunEnabled {
		return
	}

I know this is a PR just extending the existing dry-run, which is already done this way all-over the place - would you say it's feasible or should we just create a task to tidy up in the future?

[omertuc]

In general I'd prefer if we push dry-run flag (or options) within each command, so we wouldn't pollute with if statement the logic flow - a bit like it's done here:

func (i *installer) skipMcoReboot(ignitionPath string) {
	if i.DryRunEnabled {
		return
	}

I know this is a PR just extending the existing dry-run, which is already done this way all-over the place - would you say it's feasible or should we just create a task to tidy up in the future?

I don't have the capacity to test further changes, just want to fix the lint errors and merge for now

[omertuc]

/remove-label downstream-change-needed

[rccrdpccl]

/lgtm
/approve

[openshift-ci-robot]

Scheduling tests matching the pipeline_run_if_changed or not excluded by pipeline_skip_if_only_changed parameters:
/test edge-e2e-ai-operator-ztp
/test edge-e2e-metal-assisted-4-20

[openshift-ci]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: omertuc, rccrdpccl

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Details

Needs approval from an approver in each of these files:

• OWNERS [omertuc,rccrdpccl]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

[openshift-ci]

@omertuc: all tests passed!

Full PR test history. Your PR dashboard.

Details

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.

[rccrdpccl]

/retest