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 LiveImage support #754

Closed
wants to merge 5 commits into from
Closed

Add LiveImage support #754

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
76d0536
Add LiveImage to BMH CRD
Nov 10, 2020
242f88d
ironic provisioner de-duplicate image options
Dec 14, 2020
11eb45e
Add LiveImage support to ironic provisioner
Dec 15, 2020
1caec9d
Add API docs for liveImage
Dec 16, 2020
7012e2d
Add LiveImage support to host_state_machine
Dec 17, 2020
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
49 changes: 39 additions & 10 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 @@ -252,6 +252,11 @@ type BareMetalHostSpec struct {
// Image holds the details of the image to be provisioned.
Image *Image `json:"image,omitempty"`

// LiveImage holds the details of the live-image to be booted.
// Only one of Image or Live image may be specified, and
// at present UserData cannot be used with LiveImage.
LiveImage *LiveImage `json:"liveImage,omitempty"`

// UserData holds the reference to the Secret containing the user
// data to be passed to the host before it boots.
UserData *corev1.SecretReference `json:"userData,omitempty"`
Expand Down Expand Up @@ -309,6 +314,13 @@ type Image struct {
DiskFormat *string `json:"format,omitempty"`
}

// LiveImage holds the details of a live-image either to boot or that
// has been booted.
type LiveImage struct {
// URL is a location of an iso image to boot.
URL string `json:"url"`
}

// FIXME(dhellmann): We probably want some other module to own these
// data structures.

Expand Down Expand Up @@ -570,6 +582,10 @@ type ProvisionStatus struct {
// provisioned to the host.
Image Image `json:"image,omitempty"`

// LiveImage holds the details of the last live-image
// successfully booted by the host.
LiveImage LiveImage `json:"liveImage,omitempty"`

// The RootDevicehints set by the user
RootDeviceHints *RootDeviceHints `json:"rootDeviceHints,omitempty"`

Expand Down Expand Up @@ -752,18 +768,27 @@ func (host *BareMetalHost) NeedsProvisioning() bool {
// The host is not supposed to be powered on.
return false
}
if host.Spec.Image == nil {
// Without an image, there is nothing to provision.
return false
}
if host.Spec.Image.URL == "" {
// We have an Image struct but it is empty
return false
if host.Spec.LiveImage != nil {
Copy link
Member

Choose a reason for hiding this comment

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

We may need to make the logic in this function a bit more complicated to deal with switching a host from live image to provisioned image, because in that case we may have a non-nil pointer to a struct with an empty string for the live image and we would want to treat that as though the image was set. We can probably deal with that case in another PR, after we have the basic case working.

if host.Spec.LiveImage.URL == "" {
// We have a LiveImage struct but it is empty
return false
}
if host.Status.Provisioning.LiveImage.URL == "" {
// We have an image set, but not provisioned.
return true
}
}
if host.Status.Provisioning.Image.URL == "" {
// We have an image set, but not provisioned.
return true
if host.Spec.Image != nil {
if host.Spec.Image.URL == "" {
// We have an Image struct but it is empty
return false
}
if host.Status.Provisioning.Image.URL == "" {
// We have an image set, but not provisioned.
return true
}
}
// Without an image, there is nothing to provision.
return false
}

Expand All @@ -777,6 +802,10 @@ func (host *BareMetalHost) WasProvisioned() bool {
// We have an image provisioned.
return true
}
if host.Status.Provisioning.LiveImage.URL != "" {
// We have an image provisioned.
return true
}
return false
}

Expand Down
60 changes: 59 additions & 1 deletion apis/metal3.io/v1alpha1/baremetalhost_types_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -206,6 +206,23 @@ func TestHostNeedsProvisioning(t *testing.T) {
Expected: true,
},

{
Scenario: "with live image url, online",
Host: BareMetalHost{
ObjectMeta: metav1.ObjectMeta{
Name: "myhost",
Namespace: "myns",
},
Spec: BareMetalHostSpec{
LiveImage: &LiveImage{
URL: "not-empty",
},
Online: true,
},
},
Expected: true,
},

{
Scenario: "with image url, offline",
Host: BareMetalHost{
Expand All @@ -224,7 +241,24 @@ func TestHostNeedsProvisioning(t *testing.T) {
},

{
Scenario: "already provisioned",
Scenario: "with live-image url, offline",
Host: BareMetalHost{
ObjectMeta: metav1.ObjectMeta{
Name: "myhost",
Namespace: "myns",
},
Spec: BareMetalHostSpec{
LiveImage: &LiveImage{
URL: "not-empty",
},
Online: false,
},
},
Expected: false,
},

{
Scenario: "image already provisioned",
Host: BareMetalHost{
ObjectMeta: metav1.ObjectMeta{
Name: "myhost",
Expand All @@ -247,6 +281,30 @@ func TestHostNeedsProvisioning(t *testing.T) {
Expected: false,
},

{
Scenario: "live-image already provisioned",
Host: BareMetalHost{
ObjectMeta: metav1.ObjectMeta{
Name: "myhost",
Namespace: "myns",
},
Spec: BareMetalHostSpec{
LiveImage: &LiveImage{
URL: "not-empty",
},
Online: true,
},
Status: BareMetalHostStatus{
Provisioning: ProvisionStatus{
LiveImage: LiveImage{
URL: "also-not-empty",
},
},
},
},
Expected: false,
},

{
Scenario: "externally provisioned",
Host: BareMetalHost{
Expand Down
21 changes: 21 additions & 0 deletions apis/metal3.io/v1alpha1/zz_generated.deepcopy.go
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

18 changes: 18 additions & 0 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 @@ -155,6 +155,15 @@ spec:
- checksum
- url
type: object
liveImage:
description: LiveImage holds the details of the live-image to be booted. Only one of Image or Live image may be specified, and at present UserData cannot be used with LiveImage.
properties:
url:
description: URL is a location of an iso image to boot.
type: string
required:
- url
type: object
metaData:
description: MetaData holds the reference to the Secret containing host metadata (e.g. meta_data.json which is passed to Config Drive).
properties:
Expand Down Expand Up @@ -568,6 +577,15 @@ spec:
- checksum
- url
type: object
liveImage:
description: LiveImage holds the details of the last live-image successfully booted by the host.
properties:
url:
description: URL is a location of an iso image to boot.
type: string
required:
- url
type: object
rootDeviceHints:
description: The RootDevicehints set by the user
properties:
Expand Down
18 changes: 18 additions & 0 deletions config/render/capm3.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -153,6 +153,15 @@ spec:
- checksum
- url
type: object
liveImage:
description: LiveImage holds the details of the live-image to be booted. Only one of Image or Live image may be specified, and at present UserData cannot be used with LiveImage.
properties:
url:
description: URL is a location of an iso image to boot.
type: string
required:
- url
type: object
metaData:
description: MetaData holds the reference to the Secret containing host metadata (e.g. meta_data.json which is passed to Config Drive).
properties:
Expand Down Expand Up @@ -566,6 +575,15 @@ spec:
- checksum
- url
type: object
liveImage:
description: LiveImage holds the details of the last live-image successfully booted by the host.
properties:
url:
description: URL is a location of an iso image to boot.
type: string
required:
- url
type: object
rootDeviceHints:
description: The RootDevicehints set by the user
properties:
Expand Down
14 changes: 11 additions & 3 deletions controllers/metal3.io/baremetalhost_controller.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -546,9 +546,17 @@ func (r *BareMetalHostReconciler) actionProvisioning(prov provisioner.Provisione
}

// If the provisioner had no work, ensure the image settings match.
if info.host.Status.Provisioning.Image != *(info.host.Spec.Image) {
info.log.Info("updating deployed image in status")
info.host.Status.Provisioning.Image = *(info.host.Spec.Image)
if info.host.Spec.Image != nil {
if info.host.Status.Provisioning.Image != *(info.host.Spec.Image) {
info.log.Info("updating deployed image in status")
info.host.Status.Provisioning.Image = *(info.host.Spec.Image)
}
}
if info.host.Spec.LiveImage != nil {
if info.host.Status.Provisioning.LiveImage != *(info.host.Spec.LiveImage) {
info.log.Info("updating booted liveImage in status")
info.host.Status.Provisioning.LiveImage = *(info.host.Spec.LiveImage)
}
}

// After provisioning we always requeue to ensure we enter the
Expand Down
17 changes: 13 additions & 4 deletions controllers/metal3.io/host_state_machine.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -293,16 +293,25 @@ func (hsm *hostStateMachine) provisioningCancelled() bool {
if hsm.Host.HasError() {
return true
}
if hsm.Host.Spec.Image == nil {
if hsm.Host.Spec.Image == nil && hsm.Host.Spec.LiveImage == nil {
Copy link
Member Author

Choose a reason for hiding this comment

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

This logic is very similar to the BMH NeedsProvisioning() - I wonder if we could use the inverse of that here instead of duplicating nearly the same conditionals?

Copy link
Member

Choose a reason for hiding this comment

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

This used to be the NeedsDeprovisioning() method; we moved it here because having methods on the BMH object is actually bad. They're not inverses of each other.

return true
}
if hsm.Host.Spec.Image.URL == "" {
if hsm.Host.Spec.LiveImage != nil && hsm.Host.Spec.LiveImage.URL == "" {
return true
}
if hsm.Host.Status.Provisioning.Image.URL == "" {
if hsm.Host.Spec.LiveImage != nil && hsm.Host.Status.Provisioning.LiveImage.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 doesn't handle the case where Spec.LiveImage is set and Status.Provisioning.Image.URL is also set, in which case we want to return true.

Similarly, below we don't handle the case where Spec.Image is set and Status.Provisioning.LiveImage.URL is also set.

I think the correct logic is something like:

switch {
case hsm.Host.Spec.LiveImage != nil:
    if hsm.Host.Spec.LiveImage.URL == "" {
        return true
    }
    if hsm.Host.Status.Provisioning.Image.URL != "" {
        return true
    }
    if hsm.Hosts.Status.Provisioning.LiveImage.URL == "" {
        return false
    }
    if hsm.Host.Spec.LiveImage != nil && hsm.Host.Spec.LiveImage.URL != hsm.Host.Status.Provisioning.LiveImage.URL {
        return true
    }
}
case hsm.Host.Spec.Image != nil:
    if hsm.Host.Spec.Image.URL == "" {
        return true
    }
    if hsm.Host.Status.Provisioning.LiveImage.URL != "" {
        return true
    }
    if hsm.Host.Status.Provisioning.Image.URL == "" {
        return false
    }
    if hsm.Host.Spec.Image.URL != hsm.Host.Status.Provisioning.Image.URL {
        return true
    }
default:
    return true
}
return false

Assuming that hsm.Host.Spec.LiveImage and hsm.Host.Spec.Image cannot both be non-nil. If they both can be non-nil it quickly devolves even further into a nightmare. Seeing all this duplicated/permuted logic makes me wonder whether we should just have an ISO flag in the regular Image struct instead. At least in the Status maybe?

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 for the review @zaneb - In the docstring I do mention that only Image or LiveImage should be set, but I agree long term they should be validated as mutually exclusive.

Re the ISO flag, that was my original proposal but this approach was preferred since it's possible we might support a live kernel/ramdisk in future, and the checksum argument to the current Image section currently isn't applicable to the LiveImage case: metal3-io/metal3-docs#150 (comment)

return false
}
if hsm.Host.Spec.Image.URL != hsm.Host.Status.Provisioning.Image.URL {
if hsm.Host.Spec.LiveImage != nil && hsm.Host.Spec.LiveImage.URL != hsm.Host.Status.Provisioning.LiveImage.URL {
return true
}
if hsm.Host.Spec.Image != nil && hsm.Host.Spec.Image.URL == "" {
return true
}
if hsm.Host.Spec.Image != nil && hsm.Host.Status.Provisioning.Image.URL == "" {
return false
}
if hsm.Host.Spec.Image != nil && hsm.Host.Spec.Image.URL != hsm.Host.Status.Provisioning.Image.URL {
return true
}
return false
Expand Down
32 changes: 32 additions & 0 deletions controllers/metal3.io/host_state_machine_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -61,6 +61,38 @@ func TestProvisioningCancelled(t *testing.T) {
Expected: true,
},

{
Scenario: "with liveimage url, unprovisioned",
Host: metal3v1alpha1.BareMetalHost{
ObjectMeta: metav1.ObjectMeta{
Name: "myhost",
Namespace: "myns",
},
Spec: metal3v1alpha1.BareMetalHostSpec{
LiveImage: &metal3v1alpha1.LiveImage{
URL: "not-empty",
},
Online: true,
},
},
Expected: false,
},

{
Scenario: "with liveimage, unprovisioned",
Host: metal3v1alpha1.BareMetalHost{
ObjectMeta: metav1.ObjectMeta{
Name: "myhost",
Namespace: "myns",
},
Spec: metal3v1alpha1.BareMetalHostSpec{
LiveImage: &metal3v1alpha1.LiveImage{},
Online: true,
},
},
Expected: true,
},

{
Scenario: "without, unprovisioned",
Host: metal3v1alpha1.BareMetalHost{
Expand Down
8 changes: 8 additions & 0 deletions docs/api.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -99,6 +99,14 @@ Even though the image sub-fields are required by Ironic,
when the host provisioning is managed externally via `externallyProvisioned: true`,
and power control isn't needed, the fields can be left empty.

#### liveImage

Holds details for the live-image to be booted on a given host.

The sub-fields are

* *url* -- The URL of an iso image to boot.

#### userData

A reference to the Secret containing the cloudinit user data and its
Expand Down
Loading