new design proposal
jacobweinstock opened this issue · 4 comments
BMCLIB Design Doc
Context and Scope
There are many ways to interact with a BMC. IPMI, Redfish, web API, RACADM, SSH, etc. Not all interaction types are implemented for every vendor. Even if they were, there is no functionality to try multiple interaction types for a request.
The main entrypoint into BMCLIB is discover.ScanAndConnect
. This function returns one of two very large interfaces. The user then must type assert or switch on the interface to start using the functionality.
The aim of this new design is to
- create stronger abstractions through smaller interfaces
- enhance the success of BMC interactions by trying requests through multiple interaction types
- allow easier extension of functionality, vendors and interaction types.
Goals and Non-Goals
Goals:
- Create stronger abstraction through a smaller interfaces
- Allow provider implementations the ability to grow in implemented functionality without breaking Users
- Increase the success of BMC requests by trying multiple interaction types
- Increase simplicity of adding providers
Non-Goals:
- Change any existing provider implementation details
- Make API breaking changes
New Design
Overview
Some core tenets of this new design are
- Small interfaces
- Implementations are only run if they satisfy the interface requirements
- Focus on accomplishing a desired BMC interaction not how its done or via what mechanism (ie redfish, ipmitool, web api, etc)
- All interface functions will try multiple implementations (ipmitool, web api, redfish, ssh, etc) until success or failure by receiving a parameter that is a slice of PowerStateSetter interfaces, shown below.
func SetPowerState(ctx context.Context, s string, p []PowerStateSetter) (bool, error)
Details
The existing interface methods are all pretty good. The idea here is to split them out into smaller interfaces. This new design can be introduced without breaking existing functionality. Existing functionality would plugin to this and users could choose to use either or both. The ScanAndConnect
function would be integrated into the Client box below under the “provider registry helper”. See the code here for working code of this design.
Context Diagram
Code Snippets
User Package
package main
import (
"context"
"log"
"github.com/bmc-toolbox/bmclib"
)
func main() {
ctx := context.Background()
client := bmclib.NewClient("127.0.0.1", "ADMIN", "ADMIN")
err := client.GenerateDefaultRegistry(ctx)
if err != nil {
panic(err)
}
// Get BMC Power State
state, err := client.GetPowerState(ctx)
if err != nil {
panic(err)
}
log.Println(state)
// List BMC Users
users, err := client.ReadUsers(ctx)
if err != nil {
panic(err)
}
log.Println(users)
}
// Output
power state: on
users: [
{
"Auth": "true",
"Callin": "true",
"ID": "2",
"Link": "false",
"Name": "ADMIN"
}
]
Client Package
This package provides a GenerateDefaultRegistry
helper that gets/generates provider implementations and puts them into a slice for consumption by a base package executor (see the base example below).
package bmclib
import (
"context"
"errors"
"github.com/bmc-toolbox/bmclib/bmc"
"github.com/bmc-toolbox/bmclib/discover"
"github.com/bmc-toolbox/bmclib/logging"
"github.com/bmc-toolbox/bmclib/providers/ipmitool"
"github.com/go-logr/logr"
"github.com/hashicorp/go-multierror"
)
// Client for BMC interactions
type Client struct {
Auth Auth
Logger logr.Logger
Registry []interface{}
}
// GenerateDefaultRegistry updates the registry with the default provider implementations
func (c *Client) GenerateDefaultRegistry(ctx context.Context) (err error) {
// try discovering and registering a vendor specifc provider
vendor, scanErr := discover.ScanAndConnect(c.Auth.Host, c.Auth.User, c.Auth.Pass)
if scanErr != nil {
err = multierror.Append(err, scanErr)
} else {
c.Registry = append(c.Registry, vendor)
}
// register generic providers
c.Registry = append(c.Registry, &ipmitool.Conn{ Host: c.Auth.Host,
User: c.Auth.User,
Pass: c.Auth.Pass,
Log: c.Logger,
})
return nil
}
// GetPowerState pass through to library function
func (c *Client) GetPowerState(ctx context.Context) (state string, err error) {
var powerStateSetters []bmc.PowerStateSetter
for _, elem := range c.Registry {
switch p := elem.(type) {
case bmc.PowerStateSetter:
powerStateSetters = append(powerStateSetters, p)
default:
}
}
if len(powerStateSetters) == 0 {
return state, errors.New("no registered providers found")
}
return bmc.GetPowerState(ctx, powerStateSetters)
}
Base Package
This package holds the interfaces and executor functions. Notice the executor functions take a slice of the interfaces. This allows us to try the function with multiple implementations.
package bmc
import (
"context"
"errors"
"github.com/hashicorp/go-multierror"
)
// PowerStateSetter get power state and set power state
type PowerStateSetter interface {
PowerState(ctx context.Context) (state string, err error)
PowerSet(ctx context.Context, state string) (ok bool, err error)
}
// SetPowerState sets the power state for a BMC, trying all interface implementations passed in
func SetPowerState(ctx context.Context, state string, p []PowerStateSetter) (ok bool, err error) {
for _, elem := range p {
ok, setErr := elem.PowerSet(ctx, state)
if setErr != nil {
err = multierror.Append(err, setErr)
continue
}
if !ok {
err = multierror.Append(err, errors.New("failed to set power state"))
continue
}
return ok, err
}
return ok, multierror.Append(err, errors.New("failed to set power state"))
}
// GetPowerState sets the power state for a BMC, trying all interface implementations passed in
func GetPowerState(ctx context.Context, p []PowerStateSetter) (state string, err error) {
for _, elem := range p {
state, stateErr := elem.PowerState(ctx)
if stateErr != nil {
err = multierror.Append(err, stateErr)
continue
}
return state, err
}
return state, multierror.Append(err, errors.New("failed to get power state"))
}
Trade-Offs
- There will be lots of small interfaces. This will be a good amount of upfront work to create them all, along with the corresponding functions that take in the interfaces.
- There will be a lot of similar code duplicated in the corresponding functions that take in the interfaces. Again, just more upfront work.
- Code readability changes. Instead of being able to look at a single interface and know methods that are available, one would have to search across interfaces for behavior they want to implement.
References
Existing interface
package devices
import (
"crypto/x509"
"github.com/bmc-toolbox/bmclib/cfgresources"
)
// Bmc represents all the required bmc items
type Bmc interface {
Resources() []string
User([]*cfgresources.User) error
Syslog(*cfgresources.Syslog) error
Ntp(*cfgresources.Ntp) error
Ldap(*cfgresources.Ldap) error
LdapGroup([]*cfgresources.LdapGroup, *cfgresources.Ldap) error
Network(*cfgresources.Network) (bool, error)
SetLicense(*cfgresources.License) error
Bios(*cfgresources.Bios) error
Power(*cfgresources.Power) error
CurrentHTTPSCert() ([]*x509.Certificate, bool, error)
GenerateCSR(*cfgresources.HTTPSCertAttributes) ([]byte, error)
UploadHTTPSCert([]byte, string, []byte, string) (bool, error)
BiosVersion() (string, error)
HardwareType() string
Version() (string, error)
CPU() (string, int, int, int, error)
Disks() ([]*Disk, error)
IsBlade() (bool, error)
License() (string, string, error)
Memory() (int, error)
Model() (string, error)
Name() (string, error)
Nics() ([]*Nic, error)
PowerKw() (float64, error)
PowerState() (string, error)
IsOn() (bool, error)
Serial() (string, error)
Status() (string, error)
TempC() (int, error)
Vendor() string
Slot() (int, error)
Screenshot() ([]byte, string, error)
ServerSnapshot() (interface{}, error)
ChassisSerial() (string, error)
CheckCredentials() error
Close() error
PowerOn() (bool, error)
PowerOff() (bool, error)
PxeOnce() (bool, error)
PowerCycleBmc() (bool, error)
PowerCycle() (bool, error)
UpdateCredentials(string, string)
UpdateFirmware(string, string) (bool, error)
}
Proposed interfaces
package bmc
import (
"context"
"errors"
)
// PowerStateSetter get power state and set power state
type PowerStateSetter interface {
PowerState(ctx context.Context) (state string, err error)
PowerSet(ctx context.Context, state string) (ok bool, err error)
}
// UserCreator creates a user on a BMC
type UserCreator interface {
UserCreate(ctx context.Context, user, pass, role string) (ok bool, err error)
}
// UserUpdater updates a user on a BMC
type UserUpdater interface {
UserUpdate(ctx context.Context, user, pass, role string) (ok bool, err error)
}
// UserDeleter deletes a user on a BMC
type UserDeleter interface {
UserDelete(ctx context.Context, user string) (ok bool, err error)
}
// UserReader lists all users on a BMC
type UserReader interface {
UserRead(ctx context.Context) (users []map[string]string, err error)
}
The proposal looks good 👍 to me and seems like the right way ahead - both in terms of maintainability and supporting functionality that we'd like to have.
If we do go through this revamp, I'd suggest,
- Enable the end user to set the slice of preferred BMC interaction methods (Web/Redfish/SSH/IPMI)
ScanAndConnect
in the probe stage identifies which interaction methods are possible, and sets them as a property on the client.- The Setter/Getter executor methods are 'aware' of which interaction methods are supported, tries and returns accordingly
To give examples of where this is useful,
a.
As an end user of bmclib, the BMC network environment restricts IPMI,
if I could set the client 'allowed methods' (redfish/web/ssh), I could then prevent my code from connecting over IPMI,
or if I know that vendor X BMCs are slow over SSH, I would want the executors to try the web interface/redfish first.
b.
As an end user of bmclib, all the devices it interacts with belongs to vendor Y, vendor Y supports Redfish,
I'd like to have bmclib just use Redfish for all of its interaction.
Thanks for the review and feedback @joelrebel .
Enable the end user to set the slice of preferred BMC interaction methods (Web/Redfish/SSH/IPMI)
Yup, this should be covered. The end user can set the interactions by adding them to Client.Registry
type Client struct {
Auth Auth
Logger logr.Logger
Registry []interface{}
}
ScanAndConnect in the probe stage identifies which interaction methods are possible, and sets them as a property on the client.
yeah, for this one, one possible option is to extent to ScanAndConnect
to make it aware of new providers and whatnot.
The Setter/Getter executor methods are 'aware' of which interaction methods are supported, tries and returns accordingly
This one should be good. the executor functions will run through all implementations for the specific behavior requested. Like so,
// SetPowerState sets the power state for a BMC, trying all interface implementations passed in
func SetPowerState(ctx context.Context, state string, p []PowerStateSetter) (ok bool, err error) {
for _, elem := range p {
ok, setErr := elem.PowerSet(ctx, state)
if setErr != nil {
err = multierror.Append(err, setErr)
continue
}
if !ok {
err = multierror.Append(err, errors.New("failed to set power state"))
continue
}
return ok, err
}
return ok, multierror.Append(err, errors.New("failed to set power state"))
}
I think instead of having a generic registry function that appends all the providers with GenerateDefaultRegistry
there should just be a registry collection that can be appended to that can then be queried / filtered on.
For example, we can use the init
function to automatically register new registries / providers.
Here we're registering a generic
provider which uses the ipmi
protocol. I'm also providing an init function that will return a new instance of the provider that will have all the supported functions. And finally passing a list of supported features. In this instance this provider supports serial,power,boot,user
options.
// providers/generic/ipmi
func init(){
registries.Register("generic", "ipmi", func(creds devices.Credentials) (devices.Provider, error){
return NewRegistry(creds.Host, creds.User, creds.Pass)
}, []string{
"serial",
"power",
"boot",
"user",
})
}
Here we're doing the same thing however we're registering a generic
provider using the redfish
protocol. This has a similar init function, but if the driver requires a more complicated configuration that could be done in this init function. This provider however only supports serial,power,boot
features.
// providers/generic/redfish
func init(){
registries.Register("generic", "redfish", func(creds devices.Credentials) (devices.Provider, error){
return NewRegistry(creds.Host, creds.User, creds.Pass)
}, []string{
"serial",
"power",
"boot",
}
}
With the two providers registered above, we can now query for them by simple requesting registries.All()
or filtering by calling some of the other functions.
Supports()
filters registries which support all specified features.Exclude()
filters out registries using a specific technology.Using()
filters out registries not using a specific technology.Prefer()
sets the preference order for each technology.
All functions would return a new collection with the filtered results allowing you to chain multiple filters such as registries.Supports("boot").Prefer("redfish", "ipmi")
for example.
And below is some examples on using them.
func main(){
// Returns generic ipmi and redfish providers
regs := registries.Supports("boot", "power")
// Returns only generic/ipmi provider
usrRegs := registries.Supports("user")
// Pass filtered registry list
client, err := client.NewWithRegistries("1.2.3.4", "admin", "pass", regs)
client, err = client.NewWithRegistries("1.2.3.4", "admin", "pass", userRegs)
// Filter specific registries
client, err = client.NewWithRegistries("1.2.3.4", "admin", "pass", regs.Exclude("redfish"))
// Using specific protocols
client, err = client.NewWithRegistries("1.2.3.4", "admin", "pass", regs.Using("redfish"))
// Set registry order preference
client, err = client.NewWithRegistries("1.2.3.4", "admin", "pass", regs.Prefer("redfish", "ipmi"))
}
closing as new interfaces have been created
https://github.com/bmc-toolbox/bmclib/tree/master/bmc