Conversation
A new machine is created in network boot so Seed OS can flash it. Any update that reached it before it reported back — an element re-rendering the node it belongs to, for instance — fell through to the default hd0 and rebuilt the domain off a disk nothing had written yet: the VM lived seven seconds, came back with an empty 196 KB image and never booted. Keep the network boot while the machine is still installing (the target asks for it and no actual guest machine exists yet); the image-changed path is unchanged.
Reviewer's GuideEnsure that machine updates preserve network boot for machines still in Seed OS installation and add unit tests around boot-mode behavior in PoolBuilderService. Flow diagram for boot mode selection in _actualize_machine_derivatives_on_create_updateflowchart TD
A[_actualize_machine_derivatives_on_create_update] --> B[check _is_core_machine]
B -->|is core| C[keep current boot/port]
B -->|not core| D[unpack machine_guest_pair to guest_target, guest_actual]
D --> E{guest_actual is None
and guest_target.boot == nc.BootAlternative.network}
E -->|yes| F[set boot = nc.BootAlternative.network.value]
F --> G[set port = models.Port.from_boot_network]
E -->|no| H{guest_actual and
guest_actual.image != resolved_image}
H -->|yes| I[set boot = nc.BootAlternative.network.value]
I --> J[set port for boot network]
H -->|no| C
G --> K[end]
J --> K
C --> K
File-Level Changes
Possibly linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
There was a problem hiding this comment.
Hey - I've left some high level feedback:
- In
_actualize_machine_derivatives_on_create_update, the new branch assumesguest_targetis always non-None; ifmachine_guest_paircan ever have aNonetarget, this will raise, so consider guardingguest_targetbefore accessingguest_target.bootor bailing out early in that case. - The
builderfixture instantiatesPoolBuilderServicevia__new__without running__init__, which can easily break if future logic in_actualize_machine_derivatives_on_create_updatedepends on initialized attributes; consider constructing a real instance and stubbing dependencies instead.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In `_actualize_machine_derivatives_on_create_update`, the new branch assumes `guest_target` is always non-None; if `machine_guest_pair` can ever have a `None` target, this will raise, so consider guarding `guest_target` before accessing `guest_target.boot` or bailing out early in that case.
- The `builder` fixture instantiates `PoolBuilderService` via `__new__` without running `__init__`, which can easily break if future logic in `_actualize_machine_derivatives_on_create_update` depends on initialized attributes; consider constructing a real instance and stubbing dependencies instead.Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
|
Closes #271 partly. It doesn't cover the legacy case and nodes with custom images without agents. |
phantomii
left a comment
There was a problem hiding this comment.
Could we also assert that the boot-network port is used while the machine is still installing? The current test verifies the boot mode, but not the port that makes that mode usable.
phantomii
left a comment
There was a problem hiding this comment.
Please add coverage for the boot-network port while the machine is still installing.
A new machine is created in network boot so Seed OS can flash it. Any update that reached it before it reported back — an element re-rendering the node it belongs to, for instance — fell through to the default hd0 and rebuilt the domain off a disk nothing had written yet: the VM lived seven seconds, came back with an empty 196 KB image and never booted.
Keep the network boot while the machine is still installing (the target asks for it and no actual guest machine exists yet); the image-changed path is unchanged.
Summary by Sourcery
Ensure pool builder keeps machines in network boot while Seed OS installation is in progress and only switches to network boot when the guest image actually changes.
Bug Fixes:
Tests: