new design proposal

Question

new design proposal

jacobweinstock opened this issue 4 years ago · 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)
}

Answer 1 · 2020-11-18T09:23:17.000Z

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.

Answer 2 · 2020-11-18T20:20:17.000Z

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"))
}

Answer 3 · 2020-11-20T18:13:41.000Z

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"))
}

Answer 4 · 2021-03-26T02:45:38.000Z

closing as new interfaces have been created
https://github.com/bmc-toolbox/bmclib/tree/master/bmc