Add live-iso support #759
Add live-iso support #759
Conversation
metal3-io-bot
added
do-not-merge/work-in-progress
|
The new interface looks like: |
| op = nodes.AddOp | ||
| p.log.Info("adding boot_iso") | ||
| } else { | ||
| op = nodes.ReplaceOp |
There was a problem hiding this comment.
A weird property of JSON patch (at least as applied in ironic): add can be used to replace values (but not the other way).
There was a problem hiding this comment.
Thanks, that sounds like a potential simplification we could address via a followup
hardys
force-pushed
the
liveimage2
branch
2 times, most recently
from
de55f31
to
ccb90dc
Compare
|
/retitle Add live-iso support |
metal3-io-bot
removed
the
do-not-merge/work-in-progress
|
/test-integration |
metal3-io-bot
added
size/L
|
/test-integration |
| // To make it easier to test if ironic is configured with | ||
| // the same image we are trying to provision to the host. | ||
| if p.host.Spec.Image != nil && p.host.Spec.Image.DiskFormat != nil && *p.host.Spec.Image.DiskFormat == "live-iso" { | ||
| sameImage = (ironicNode.InstanceInfo["boot_iso"] == p.host.Spec.Image.URL) |
There was a problem hiding this comment.
This is not cleared from Ironic when switching back from live-iso to a regular boot, so it may not be operative. I think we should either clear it at the same time as setting deploy_interface back to direct, or also verify the deploy_interface here, or both.
There was a problem hiding this comment.
Ah thanks, good catch - I updated getImageUpdateOptsForNode to account for this, and added tests for both image->iso and iso->image in updateopts_test.go
I'm not sure adding an explicit check for deploy_interface here gains us much though, since in that case we expect *p.host.Spec.Image.DiskFormat to change from live-iso to something else, then we'll hit the else and the checksum/image_source values in ironicNode.InstanceInfo will always be empty, so sameImage will always evaluate to false AFAICT?
That said, it does raise the question of whether we should check *p.host.Spec.Image.DiskFormat since in any other change of disk-format we might want to set sameImage to false, even if the URL is the same? I guess that's something to potentially address in a different PR though.
There was a problem hiding this comment.
Agree that doing both is redundant. (Wouldn't hurt, in a belt-and-braces sense, but this is fine too.)
You make a good point though that if provisioning fails because you specified the wrong image format, and you then fix it, it currently won't retry, at least on paper. I'm not actually even 100% sure this code is still needed, since after a failure is first recorded we change the state to Deprovisioning. Definitely a question for elsewhere though.
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: dhellmann, hardys, imain The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
This allows iso images to be booted via the Ironic ramdisk deploy
interface, e.g:
image:
url: http://172.22.0.1/images/fedora-coreos-33.20201214.2.0-live.x86_64.iso
format: live-iso
metal3-io-bot
added
size/XL
|
/test-integration |
Alternative to #754
@dhellmann @zaneb PTAL - if you're happy with this interface I'll update the design doc PR
This is working but I'm planning to add some more tests and try to clean up the diff a little before removing WIP