Skip to content
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
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
69 changes: 69 additions & 0 deletions pkg/apis/metal3/v1alpha1/baremetalhost_types.go
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
package v1alpha1

import (
"github.com/gophercloud/gophercloud/openstack/baremetal/v1/nodes"
corev1 "k8s.io/api/core/v1"
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
"k8s.io/apimachinery/pkg/types"
Expand Down Expand Up @@ -86,6 +87,10 @@ const (
// learn about the hardware components available there
StateInspecting ProvisioningState = "inspecting"

// StateCleaning means Ironic are running the referenced CleanSteps via
// manual cleaning for RAID and BIOS configuration
StateCleaning ProvisioningState = "cleaning"
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

"Cleaning" is an ironic concept, and we do not want to expose it through the metal3 API. We should be able to use the existing provisioning state to indicate that work is happening to provision the server.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

And at least we shouldn't call it "cleaning". It's confusing enough in ironic world :)

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Thanks for you comment. I will abandon cleaning state from this prototype, then execute the cleansteps in the other provision state. But I am confusing between registering and inspecting :(


// StatePowerManagementError means something went wrong trying to
// power the server on or off.
StatePowerManagementError ProvisioningState = "power management error"
Expand All @@ -104,6 +109,49 @@ type BMCDetails struct {
CredentialsName string `json:"credentialsName"`
}

type RAIDVolume struct {
// Size (Integer) of the logical disk to be created in GiB. If unspecified, "MAX" will be used.
SizeGB *int `json:"sizeGB"`

// RAID level for the logical disk.
RAIDLevel string `json:"raidLevel" required:"true"`

// Name of the volume. Should be unique within the Node. If not specified, volume name will be auto-generated.
VolumeName string `json:"volumeName,omitempty"`

// Set to true if this logical disk can share physical disks with other logical disks.
SharePhysicalDisks *bool `json:"sharePhysicalDisks,omitempty"`

// If this is not specified, disk type will not be a criterion to find backing physical disks
DiskType string `json:"diskType,omitempty"`
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

What are valid values for disk type?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

OK, then DiskType should have an enum defined that restricts its values.


// If this is not specified, interface type will not be a criterion to find backing physical disks.
InterfaceType string `json:"interfaceType,omitempty"`
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

What are valid values for interface type?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

The valid values for interface type are: sata or scsi or sas [1]
[1] https://docs.openstack.org/ironic/latest/admin/raid.html#raid-configuration-json-format

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

InterfaceType should have an enum defined that restricts its values.


// Integer, number of disks to use for the logical disk. Defaults to minimum number of disks required
// for the particular RAID level.
NumberOfPhysicalDisks int `json:"numberOfPhysicalDisks,omitempty"`

// The name of the controller as read by the RAID interface.
Controller string `json:"controller,omitempty"`

// A list of physical disks to use as read by the RAID interface.
PhysicalDisks []string `json:"physicalDisks,omitempty"`
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Do we really want to expose all advanced features of ironic RAID setup?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I would prefer if we start small. It's not clear how that relates to the PhysicalDisks parameter. Does ironic make its own choice if no list is presented?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Software Raid also doesn't support physical disk specification. Some of the drivers try to automatically do the right thing, other drivers (and their vendors) disagree with that model though... :(

}

// RAIDConfig contains the configuration that are required to config RAID in Bare Metal server
type RAIDConfig struct {

// The logical disk that will be root volume
RootVolume *RAIDVolume `json:"rootVolume,omitempty"`

// The list of logical disks
Volumes []RAIDVolume `json:"volumes,omitempty"`
}

// BIOSConfig contains the configuration that are required to config BIOS in Bare Metal server
type BIOSConfig map[string]interface{}
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

As I said in an earlier review, the BIOS structure needs to have defined fields with appropriate types. If something is an on/off flag, then a boolean pointer. If something is a string, then a string. A map using interface is not going to be an acceptable API because it is impossible to validate it in the OpenAPI code and future versions of kubernetes are going to require more structural validation for CRDs.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Because BIOS setting depend on vendor's driver, so I declare BIOSConfig as interface{} to fit with all of BIOS struct from vendors, then each vendor have to defined VendorBIOSConfigSpec [1] for their own BIOS settings.

For BIOS validation, I also defined ValueType for each BIOS setting in VendorBIOSConfigSpec, so we can validate the BIOS setting from users via ValidateAndBuildBIOSCleanStep function.

[1] https://github.com/metal3-io/baremetal-operator/pull/279/files#diff-334e60af5353ff0c1ef95211da871902R53
[2] https://github.com/metal3-io/baremetal-operator/pull/279/files#diff-bc78df50e156a9dedd77791eea452d13R1085

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I realize it is going to make it more complicated to design an API that supports changing different settings on different types of hardware, but that's the task. A pass-through API is completely untenable, because it's not an API.

We must decide on some way to describe every API parameter. That may mean abstracting some of the BIOS settings so the same inputs can apply on different types of hardware. It may mean providing a way to specify hardware-type-specific BIOS settings. We will not have an API that accepts an unspecified input and relies on validation happening after the fact.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I think part of the conundrum is how this has been handled from the actual vendor interfaces. There is no standardization there even on the exact same API redfish interface as vendors have freely named individual settings as they wish. :\

This can vary between models as well, so I'm really not sure if there is any better way than lettering the vendor data model being represented...

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

So in discussing this with @dhellmann, I think the only path forward for cross-vendor consistency with Metal3 is to define a set list of names. Such as a single VT/VTX CPU flag, such as "Virtualization", that may actually map to specific settings based upon underlying hardware types/Hardware Models. That allows for other backends at a later point in time if necessary. The conundrum is that settings can vary wildly based upon back-end hardware and the best thing to do is for the underlying specific settings to be implementation details for the supported platforms as opposed to try and pass-through the data structure that the underlying driver in ironic or that the BMC may be able to parse.


// BareMetalHostSpec defines the desired state of BareMetalHost
type BareMetalHostSpec struct {
// Important: Run "operator-sdk generate k8s" to regenerate code
Expand All @@ -118,6 +166,12 @@ type BareMetalHostSpec struct {
// How do we connect to the BMC?
BMC BMCDetails `json:"bmc,omitempty"`

// RAID configuration for bare metal server
RAID RAIDConfig `json:"raid,omitempty"`

// BIOS configurations for bare metal server
BIOS BIOSConfig `json:"bios,omitempty"`

// What is the name of the hardware profile for this host? It
// should only be necessary to set this when inspection cannot
// automatically determine the profile.
Expand Down Expand Up @@ -351,6 +405,9 @@ type BareMetalHostStatus struct {
// The hardware discovered to exist on the host.
HardwareDetails *HardwareDetails `json:"hardware,omitempty"`

// The executed CleanSteps on the host.
CleanSteps []nodes.CleanStep `json:"cleanSteps,omitempty"`
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Is it possible for us to determine the steps to follow without requiring the user to specify them? Even if we always just follow the same steps, I would rather not expose them through the API.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This is a read-only field, I'm not sure why we need it here.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

@dhellmann: This CleanSteps is include both RAID and BIOS clean steps, so if there is no valid cleanstep for both, we no need to run cleaning step
@dtantsur: I will move this cleansteps into ironicProvisioner, so this field will be removed.


// Information tracked by the provisioner.
Provisioning ProvisionStatus `json:"provisioning"`

Expand Down Expand Up @@ -529,6 +586,18 @@ func (host *BareMetalHost) NeedsHardwareInspection() bool {
return host.Status.HardwareDetails == nil
}

// NeedManualCleaning looks at the state of the host to determine
// if Clean Steps are needed to be run
func (host *BareMetalHost) NeedsManualCleaning(cleanSteps []nodes.CleanStep) bool {
if host.Status.CleanSteps != nil {
return false
} else if len(cleanSteps) > 0 {
// Never run manual cleaning if RAID or BIOS is not configured
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This comment is confusing. The way the CRD is defined above, cleaning steps can be configured independently of either the BIOS or RAID settings. Should the condition on line 594 be checking those parts of the struct, too?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

This BIOS struct depend on vendor driver, and it fit with BIOS struct in baremetalhost_types.go via GetBIOSConfigDetails [1]

[1] https://github.com/metal3-io/baremetal-operator/pull/279/files#diff-bc78df50e156a9dedd77791eea452d13R1073

return true
}
return false
}

// NeedsProvisioning compares the settings with the provisioning
// status and returns true when more work is needed or false
// otherwise.
Expand Down
102 changes: 102 additions & 0 deletions pkg/apis/metal3/v1alpha1/baremetalhost_types_test.go
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
package v1alpha1

import (
"github.com/gophercloud/gophercloud/openstack/baremetal/v1/nodes"
"testing"
"time"

Expand Down Expand Up @@ -151,6 +152,107 @@ func TestHostNeedsHardwareInspection(t *testing.T) {
}
}

func TestHostNeedsManualCleaning(t *testing.T) {
testCases := []struct {
Scenario string
Host BareMetalHost
CleanSteps [] nodes.CleanStep
Expected bool
}{

{
Scenario: "without cleanSteps in host status and incoming cleanSteps",
Host: BareMetalHost{
ObjectMeta: metav1.ObjectMeta{
Name: "myhost",
Namespace: "myns",
},
},
CleanSteps: []nodes.CleanStep{
{
Interface: "fakeInterface",
Step: "fakeStep",
Args: map[string]interface{}{
"fakeKey": "fakeValue",
},
},
},
Expected: true,
},

{
Scenario: "with cleanSteps in host status and incoming cleanSteps",
Host: BareMetalHost{
ObjectMeta: metav1.ObjectMeta{
Name: "myhost",
Namespace: "myns",
},
Status: BareMetalHostStatus{
CleanSteps: []nodes.CleanStep{
{
Interface: "myInterface",
Step: "myStep",
},
},
},
},
CleanSteps: []nodes.CleanStep{
{
Interface: "fakeInterface",
Step: "fakeStep",
Args: map[string]interface{}{
"fakeKey": "fakeValue",
},
},
},
Expected: false,
},

{
Scenario: "with cleanSteps in host status and no incoming cleanSteps",
Host: BareMetalHost{
ObjectMeta: metav1.ObjectMeta{
Name: "myhost",
Namespace: "myns",
},
Status: BareMetalHostStatus{
CleanSteps: []nodes.CleanStep{
{
Interface: "myInterface",
Step: "myStep",
},
},
},
},
CleanSteps: []nodes.CleanStep{},
Expected: false,
},

{
Scenario: "without cleanSteps in host status and no incoming cleanSteps",
Host: BareMetalHost{
ObjectMeta: metav1.ObjectMeta{
Name: "myhost",
Namespace: "myns",
},
},
CleanSteps: []nodes.CleanStep{},
Expected: false,
},
}
for _, tc := range testCases {
t.Run(tc.Scenario, func(t *testing.T) {
actual := tc.Host.NeedsManualCleaning(tc.CleanSteps)
if tc.Expected && !actual {
t.Error("expected to need manual cleaning")
}
if !tc.Expected && actual {
t.Error("did not expect to need manual cleaning")
}
})
}
}

func TestHostNeedsProvisioning(t *testing.T) {
testCases := []struct {
Scenario string
Expand Down
23 changes: 23 additions & 0 deletions pkg/bmc/access.go
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@ package bmc
import (
"net"
"net/url"
"reflect"
"strings"

"github.com/pkg/errors"
Expand Down Expand Up @@ -34,6 +35,28 @@ type AccessDetails interface {

// Boot interface to set
BootInterface() string

// Return the map of supported BIOS configuration keys
GetBIOSConfigDetails() map[string]VendorBIOSConfigSpec
}

type VendorBIOSConfigSpec struct {
VendorKey string
ValueType reflect.Kind
SupportedValues []interface{}
}

func (v *VendorBIOSConfigSpec) IsSupportedConfigValue (value interface{}) (isSupport bool) {
if v.SupportedValues == nil {
return true
} else {
for _, supportedValue := range v.SupportedValues {
if value == supportedValue {
return true
}
}
}
return false
}

func getTypeHostPort(address string) (bmcType, host, port, path string, err error) {
Expand Down
43 changes: 43 additions & 0 deletions pkg/bmc/access_test.go
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
package bmc

import (
"reflect"
"testing"

logf "sigs.k8s.io/controller-runtime/pkg/runtime/log"
Expand Down Expand Up @@ -211,6 +212,16 @@ func TestIPMIBootInterface(t *testing.T) {
}
}

func TestIPMIBiosConfigDetails(t *testing.T) {
acc, err := NewAccessDetails("ipmi://192.168.122.1:6233")
if err != nil {
t.Fatalf("unexpected parse error: %v", err)
}
if acc.GetBIOSConfigDetails() != nil {
t.Fatal("expected BIOS configuration details to be nil")
}
}

func TestLibvirtNeedsMAC(t *testing.T) {
acc, err := NewAccessDetails("libvirt://192.168.122.1:6233/")
if err != nil {
Expand Down Expand Up @@ -242,6 +253,16 @@ func TestLibvirtBootInterface(t *testing.T) {
}
}

func TestLibvirtBiosConfigDetails(t *testing.T) {
acc, err := NewAccessDetails("libvirt://192.168.122.1:6233/")
if err != nil {
t.Fatalf("unexpected parse error: %v", err)
}
if acc.GetBIOSConfigDetails() != nil {
t.Fatal("expected BIOS configuration details to be nil")
}
}

func TestParseIDRACURL(t *testing.T) {
T, H, P, A, err := getTypeHostPort("idrac://192.168.122.1")
if err != nil {
Expand Down Expand Up @@ -469,6 +490,17 @@ func TestIDRACBootInterface(t *testing.T) {
}
}

func TestIRACBiosConfigDetails(t *testing.T) {
acc, err := NewAccessDetails("idrac://192.168.122.1")
if err != nil {
t.Fatalf("unexpected parse error: %v", err)
}
if acc.GetBIOSConfigDetails() != nil {
t.Fatal("expected BIOS configuration details to be nil")
}
}


func TestParseIRMCURL(t *testing.T) {
T, H, P, A, err := getTypeHostPort("irmc://192.168.122.1")
if err != nil {
Expand Down Expand Up @@ -613,6 +645,17 @@ func TestIRMCBootInterface(t *testing.T) {
}
}

func TestIRMCBiosConfigDetails(t *testing.T) {
acc, err := NewAccessDetails("irmc://192.168.122.1")
if err != nil {
t.Fatalf("unexpected parse error: %v", err)
}
if !reflect.DeepEqual(acc.GetBIOSConfigDetails(), iRMCBiosConfigDetails) {
t.Fatal("unexpected BIOS configuration details")
}
}


func TestUnknownType(t *testing.T) {
acc, err := NewAccessDetails("foo://192.168.122.1")
if err == nil || acc != nil {
Expand Down
6 changes: 6 additions & 0 deletions pkg/bmc/idrac.go
Original file line number Diff line number Diff line change
Expand Up @@ -54,3 +54,9 @@ func (a *iDracAccessDetails) DriverInfo(bmcCreds Credentials) map[string]interfa
func (a *iDracAccessDetails) BootInterface() string {
return "ipxe"
}

// GetBIOSConfigDetails return the mapping of supported BIOS configuration keys between Metal3 and iDRAC.
// If the user use the key that does not belong to this map, this key wil be marked as unsupported in iDRAC.
func (a *iDracAccessDetails) GetBIOSConfigDetails() map[string]VendorBIOSConfigSpec {
return nil
}
7 changes: 7 additions & 0 deletions pkg/bmc/ipmi.go
Original file line number Diff line number Diff line change
Expand Up @@ -49,3 +49,10 @@ func (a *ipmiAccessDetails) DriverInfo(bmcCreds Credentials) map[string]interfac
func (a *ipmiAccessDetails) BootInterface() string {
return "ipxe"
}

// GetBIOSConfigDetails return the mapping of supported BIOS configuration keys between Metal3 and IPMI.
// If the user use the key that does not belong to this map, this key wil be marked as unsupported in IPMI.
func (a *ipmiAccessDetails) GetBIOSConfigDetails() map[string] VendorBIOSConfigSpec {
return nil
}

Loading