fix: properly change boot order on s390x machines (#517) - #601
skatsaounis wants to merge 1 commit into
Conversation
There was a problem hiding this comment.
[maas-code-reviewer review]
LLM-generated review from https://github.com/canonical/maas-code-reviewer.
Intended to assist a human reviewer, not replace one — suggestions may be
incorrect, please verify before acting.
This PR excellently moves the boot-order configuration out of the legacy AMP RPC and into Temporal, replacing the comma-separated maas.power --order format with robust JSON.
The adjustments to _get_boot_disk and get_boot_order in maastemporalworker/workflow/deploy.py to INNER JOIN the maasserver_physicalblockdevice table successfully resolve the KeyError: 'serial' bug and correctly align with what the power drivers expect. The fallback IS NOT NULL logic via SQLAlchemy when boot_disk.get("id") is None works correctly by matching all non-boot physical devices as intended.
No security concerns, logical errors, or performance regressions found. Solid work!
|
@AloizioMacedo could you please check if it is ok to merge now or add a block label if not? |
011c6e8 to
ac48393
Compare
Boot-order for non-deploy power actions (on/off/cycle) flowed through the legacy region→rack AMP RPC — a fragile path that duplicated the Temporal boot-order logic already used during deploy. This PR consolidates boot-order onto the Temporal power path, removes the legacy RPC path, switches the --order wire format to JSON across region → agent → maas.power CLI → driver, and fixes boot-disk selection in the deploy workflow. Boot-order flowed through two divergent paths: the legacy RPC for non-deploy power ops, and the Temporal `DeployWorkflow` during deploy. The RPC path was fragile and duplicated logic. On top of that, the comma-split `--order` format couldn't carry device dicts, and the deploy workflow's boot-disk query joined the wrong table and selected the wrong columns, raising `KeyError: 'serial'`. **Boot-order on the Temporal path** - Non-deploy power actions (on/off/cycle) now apply boot order through Temporal instead of the region RPC: `PowerParam` carries an optional `boot_order`, and the on/off/cycle workflows apply it via a shared `set-boot-order` agent activity before the power action. `convert_power_action_to_power_workflow()` threads the boot order onto those params. `SetBootOrderParam`/`SET_BOOT_ORDER_ACTIVITY_NAME` moved to `maascommon/workflows/power.py` (shared, avoids a circular import). - Removed the now-obsolete legacy path: the `SetBootOrder` AMP command (`rpc/cluster.py`) and its rack responder (`rpc/clusterservice.py`), `clusterrpc.power.set_boot_order` and its use in `node._power_control_node`, the orphaned `Node.set_boot_order()` model method, and the `node.set_boot_order()` netboot on/off calls in `metadataserver/api.py`. **JSON `--order` wire format** - `provisioningserver/power_driver_command.py`: registers `--order` on the driver subparser and parses it with `json.loads` (was comma-split). - `maasagent/internal/power/service.go`: forwards boot order as a JSON array, omits `--order` when empty (empty flag is rejected by the CLI), and fixes the `power_params` JSON tag drift. **Deploy workflow correctness (`maastemporalworker/workflow/deploy.py`)** - `_get_boot_disk` / `get_boot_order` now `INNER JOIN maasserver_physicalblockdevice` and select the exact fields drivers need (`id`, `name`, `id_path`, `model`, `serial`), fixing `KeyError: 'serial'` and dropping the wrong virtual-block-device join. - Network boot order is armed before power-on for both ephemeral and regular deploys, matching legacy behavior. - HMCZ/IBM Z HMC resilience — the 409,2 "busy" retry and paused-state guard in the HMCZ driver — is handled in a separate follow-up PR. - The wrongly-reported machine status from the ambiguous paused intermediate state of s390x machines is known, accepted, and intentionally not addressed here. Resolves: LP:2158480 (cherry picked from commit b18d623)
ac48393 to
cc22663
Compare
Boot-order for non-deploy power actions (on/off/cycle) flowed through the legacy region→rack AMP RPC — a fragile path that duplicated the Temporal boot-order logic already used during deploy. This PR consolidates boot-order onto the Temporal power path, removes the legacy RPC path, switches the --order wire format to JSON across region → agent → maas.power CLI → driver, and fixes boot-disk selection in the deploy workflow.
Boot-order flowed through two divergent paths: the legacy RPC for non-deploy power ops, and the Temporal
DeployWorkflowduring deploy. The RPC path was fragile and duplicated logic. On top of that, the comma-split--orderformat couldn't carry device dicts, and the deploy workflow's boot-disk query joined the wrong table and selected the wrong columns, raisingKeyError: 'serial'.Boot-order on the Temporal path
PowerParamcarries an optionalboot_order, and the on/off/cycle workflows apply it via a sharedset-boot-orderagent activity before the power action.convert_power_action_to_power_workflow()threads the boot order onto those params.SetBootOrderParam/SET_BOOT_ORDER_ACTIVITY_NAMEmoved tomaascommon/workflows/power.py(shared, avoids a circular import).SetBootOrderAMP command (rpc/cluster.py) and its rack responder (rpc/clusterservice.py),clusterrpc.power.set_boot_orderand its use innode._power_control_node, the orphanedNode.set_boot_order()model method, and thenode.set_boot_order()netboot on/off calls inmetadataserver/api.py.JSON
--orderwire formatprovisioningserver/power_driver_command.py: registers--orderon the driver subparser and parses it withjson.loads(was comma-split).maasagent/internal/power/service.go: forwards boot order as a JSON array, omits--orderwhen empty (empty flag is rejected by the CLI), and fixes thepower_paramsJSON tag drift.Deploy workflow correctness
(
maastemporalworker/workflow/deploy.py)_get_boot_disk/get_boot_ordernowINNER JOIN maasserver_physicalblockdeviceand select the exact fields drivers need (id,name,id_path,model,serial), fixingKeyError: 'serial'and dropping the wrong virtual-block-device join.Network boot order is armed before power-on for both ephemeral and regular deploys, matching legacy behavior.
HMCZ/IBM Z HMC resilience — the 409,2 "busy" retry and paused-state guard in the HMCZ driver — is handled in a separate follow-up PR.
The wrongly-reported machine status from the ambiguous paused intermediate state of s390x machines is known, accepted, and intentionally not addressed here.
Resolves: LP:2158480
(cherry picked from commit b18d623)