Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Add live-iso support #759

Merged
merged 3 commits into from
Jan 20, 2021
Merged

Add live-iso support #759

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
aac441c
Add live-iso option to DiskFormat
Jan 7, 2021
0f12bb2
Add live-iso support to ironic provisioner
Dec 15, 2020
78fa25a
Add api docs for live-iso
Jan 14, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
17 changes: 13 additions & 4 deletions apis/metal3.io/v1alpha1/baremetalhost_types.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -297,15 +297,18 @@ type Image struct {
URL string `json:"url"`

// Checksum is the checksum for the image.
Checksum string `json:"checksum"`
Checksum string `json:"checksum,omitempty"`

// ChecksumType is the checksum algorithm for the image.
// e.g md5, sha256, sha512
ChecksumType ChecksumType `json:"checksumType,omitempty"`

// DiskFormat contains the format of the image (raw, qcow2, ...)
// Needs to be set to raw for raw images streaming
// +kubebuilder:validation:Enum=raw;qcow2;vdi;vmdk
// DiskFormat contains the format of the image (raw, qcow2, ...).
// Needs to be set to raw for raw images streaming.
// Note live-iso means an iso referenced by the url will be live-booted
// and not deployed to disk, and in this case the checksum options
// are not required and if specified will be ignored.
// +kubebuilder:validation:Enum=raw;qcow2;vdi;vmdk;live-iso
DiskFormat *string `json:"format,omitempty"`
}

Expand Down Expand Up @@ -812,6 +815,12 @@ func (image *Image) GetChecksum() (checksum, checksumType string, ok bool) {
return
}

if image.DiskFormat != nil && *image.DiskFormat == "live-iso" {
// Checksum is not required for live-iso
ok = true
return
}

if image.Checksum == "" {
// Return empty if checksum is not provided
return
Expand Down
8 changes: 4 additions & 4 deletions config/crd/bases/metal3.io_baremetalhosts.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -141,18 +141,18 @@ spec:
- sha512
type: string
format:
description: DiskFormat contains the format of the image (raw, qcow2, ...) Needs to be set to raw for raw images streaming
description: DiskFormat contains the format of the image (raw, qcow2, ...). Needs to be set to raw for raw images streaming. Note live-iso means an iso referenced by the url will be live-booted and not deployed to disk, and in this case the checksum options are not required and if specified will be ignored.
enum:
- raw
- qcow2
- vdi
- vmdk
- live-iso
type: string
url:
description: URL is a location of an image to deploy.
type: string
required:
- checksum
- url
type: object
metaData:
Expand Down Expand Up @@ -554,18 +554,18 @@ spec:
- sha512
type: string
format:
description: DiskFormat contains the format of the image (raw, qcow2, ...) Needs to be set to raw for raw images streaming
description: DiskFormat contains the format of the image (raw, qcow2, ...). Needs to be set to raw for raw images streaming. Note live-iso means an iso referenced by the url will be live-booted and not deployed to disk, and in this case the checksum options are not required and if specified will be ignored.
enum:
- raw
- qcow2
- vdi
- vmdk
- live-iso
type: string
url:
description: URL is a location of an image to deploy.
type: string
required:
- checksum
- url
type: object
rootDeviceHints:
Expand Down
8 changes: 4 additions & 4 deletions config/render/capm3.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -139,18 +139,18 @@ spec:
- sha512
type: string
format:
description: DiskFormat contains the format of the image (raw, qcow2, ...) Needs to be set to raw for raw images streaming
description: DiskFormat contains the format of the image (raw, qcow2, ...). Needs to be set to raw for raw images streaming. Note live-iso means an iso referenced by the url will be live-booted and not deployed to disk, and in this case the checksum options are not required and if specified will be ignored.
enum:
- raw
- qcow2
- vdi
- vmdk
- live-iso
type: string
url:
description: URL is a location of an image to deploy.
type: string
required:
- checksum
- url
type: object
metaData:
Expand Down Expand Up @@ -552,18 +552,18 @@ spec:
- sha512
type: string
format:
description: DiskFormat contains the format of the image (raw, qcow2, ...) Needs to be set to raw for raw images streaming
description: DiskFormat contains the format of the image (raw, qcow2, ...). Needs to be set to raw for raw images streaming. Note live-iso means an iso referenced by the url will be live-booted and not deployed to disk, and in this case the checksum options are not required and if specified will be ignored.
enum:
- raw
- qcow2
- vdi
- vmdk
- live-iso
type: string
url:
description: URL is a location of an image to deploy.
type: string
required:
- checksum
- url
type: object
rootDeviceHints:
Expand Down
6 changes: 4 additions & 2 deletions docs/api.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -92,8 +92,10 @@ The sub-fields are
only `md5`, `sha256`, `sha512` are recognized. If nothing is specified
`md5` is assumed.
* *format* -- This is the disk format of the image. It can be one of `raw`,
`qcow2`, `vdi`, `vmdk`, or be left unset. Setting it to raw enables raw
image streaming in Ironic agent for that image.
`qcow2`, `vdi`, `vmdk`, `live-iso` or be left unset.
Setting it to raw enables raw image streaming in Ironic agent for that image.
Setting it to live-iso enables iso images to live boot without deploying
to disk, in this case the checksum fields are ignored.

Even though the image sub-fields are required by Ironic,
when the host provisioning is managed externally via `externallyProvisioned: true`,
Expand Down
114 changes: 99 additions & 15 deletions pkg/provisioner/ironic/ironic.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -372,6 +372,7 @@ func (p *ironicProvisioner) ValidateManagementAccess(credentialsChanged, force b
BootInterface: p.bmcAccess.BootInterface(),
Name: p.host.Name,
DriverInfo: driverInfo,
DeployInterface: p.deployInterface(),
InspectInterface: "inspector",
ManagementInterface: p.bmcAccess.ManagementInterface(),
PowerInterface: p.bmcAccess.PowerInterface(),
Expand Down Expand Up @@ -736,8 +737,72 @@ func (p *ironicProvisioner) getImageUpdateOptsForNode(ironicNode *nodes.Node, im
Value: string(p.host.ObjectMeta.UID),
},
)
// image_source
// live-iso format
var op nodes.UpdateOp
if imageData.DiskFormat != nil && *imageData.DiskFormat == "live-iso" {
if _, ok := ironicNode.InstanceInfo["boot_iso"]; !ok {
op = nodes.AddOp
p.log.Info("adding boot_iso")
} else {
op = nodes.ReplaceOp
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A weird property of JSON patch (at least as applied in ironic): add can be used to replace values (but not the other way).

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, that sounds like a potential simplification we could address via a followup

p.log.Info("updating boot_iso")
}
updates = append(
updates,
nodes.UpdateOperation{
Op: op,
Path: "/instance_info/boot_iso",
Value: imageData.URL,
},
)
updates = append(
updates,
nodes.UpdateOperation{
Op: nodes.ReplaceOp,
Path: "/deploy_interface",
hardys marked this conversation as resolved.
Show resolved Hide resolved
Value: "ramdisk",
},
)
// remove any image_source or checksum options
removals := []string{
"image_source", "image_os_hash_value", "image_os_hash_algo", "image_checksum"}
op = nodes.RemoveOp
for _, item := range removals {
if _, ok := ironicNode.InstanceInfo[item]; ok {
p.log.Info("removing " + item)
updates = append(
updates,
nodes.UpdateOperation{
Op: op,
Path: "/instance_info/" + item,
},
)
}
}
hardys marked this conversation as resolved.
Show resolved Hide resolved
return updates, nil
}

// Set deploy_interface direct when not booting a live-iso
updates = append(
updates,
nodes.UpdateOperation{
Op: nodes.ReplaceOp,
Path: "/deploy_interface",
Value: "direct",
},
)
// Remove any boot_iso field
if _, ok := ironicNode.InstanceInfo["boot_iso"]; ok {
p.log.Info("removing boot_iso")
updates = append(
updates,
nodes.UpdateOperation{
Op: nodes.RemoveOp,
Path: "/instance_info/boot_iso",
},
)
}
// image_source
if _, ok := ironicNode.InstanceInfo["image_source"]; !ok {
op = nodes.AddOp
p.log.Info("adding image_source")
Expand Down Expand Up @@ -1016,6 +1081,14 @@ func (p *ironicProvisioner) setUpForProvisioning(ironicNode *nodes.Node, hostCon
return
}

func (p *ironicProvisioner) deployInterface() (result string) {
result = "direct"
if p.host.Spec.Image != nil && p.host.Spec.Image.DiskFormat != nil && *p.host.Spec.Image.DiskFormat == "live-iso" {
result = "ramdisk"
}
return result
}

// Adopt allows an externally-provisioned server to be adopted by Ironic.
func (p *ironicProvisioner) Adopt(force bool) (result provisioner.Result, err error) {
var ironicNode *nodes.Node
Expand Down Expand Up @@ -1058,6 +1131,30 @@ func (p *ironicProvisioner) Adopt(force bool) (result provisioner.Result, err er
return operationComplete()
}

func (p *ironicProvisioner) ironicHasSameImage(ironicNode *nodes.Node) (sameImage bool) {
// 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)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

p.log.Info("checking image settings",
"boot_iso", ironicNode.InstanceInfo["boot_iso"],
"same", sameImage,
"provisionState", ironicNode.ProvisionState)
} else {
checksum, checksumType, _ := p.host.GetImageChecksum()
sameImage = (ironicNode.InstanceInfo["image_source"] == p.host.Spec.Image.URL &&
ironicNode.InstanceInfo["image_os_hash_algo"] == checksumType &&
ironicNode.InstanceInfo["image_os_hash_value"] == checksum)
p.log.Info("checking image settings",
"source", ironicNode.InstanceInfo["image_source"],
"image_os_hash_algo", checksumType,
"image_os_has_value", checksum,
"same", sameImage,
"provisionState", ironicNode.ProvisionState)
}
return sameImage
}

// Provision writes the image from the host spec to the host. It may
// be called multiple times, and should return true for its dirty flag
// until the deprovisioning operation is completed.
Expand All @@ -1073,20 +1170,7 @@ func (p *ironicProvisioner) Provision(hostConf provisioner.HostConfigData) (resu

p.log.Info("provisioning image to host", "state", ironicNode.ProvisionState)

checksum, checksumType, _ := p.host.GetImageChecksum()

// Local variable to make it easier to test if ironic is
// configured with the same image we are trying to provision to
// the host.
ironicHasSameImage := (ironicNode.InstanceInfo["image_source"] == p.host.Spec.Image.URL &&
ironicNode.InstanceInfo["image_os_hash_algo"] == checksumType &&
ironicNode.InstanceInfo["image_os_hash_value"] == checksum)
p.log.Info("checking image settings",
"source", ironicNode.InstanceInfo["image_source"],
"image_os_hash_algo", checksumType,
"image_os_has_value", checksum,
"same", ironicHasSameImage,
"provisionState", ironicNode.ProvisionState)
ironicHasSameImage := p.ironicHasSameImage(ironicNode)

// Ironic has the settings it needs, see if it finds any issues
// with them.
Expand Down
7 changes: 7 additions & 0 deletions pkg/provisioner/ironic/ironic_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -71,5 +71,12 @@ func makeHost() metal3v1alpha1.BareMetalHost {
}
}

func makeHostLiveIso() (host metal3v1alpha1.BareMetalHost) {
host = makeHost()
format := "live-iso"
host.Spec.Image.DiskFormat = &format
return host
}

// Implements provisioner.EventPublisher to swallow events for tests.
func nullEventPublisher(reason, message string) {}
Loading