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.

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

bmclib interface comparison #199

Closed
joelrebel opened this issue Feb 12, 2021 · 2 comments
Closed

bmclib interface comparison #199

joelrebel opened this issue Feb 12, 2021 · 2 comments

Comments

@joelrebel
Copy link
Member

About

This ticket is mainly a discussion around the newer and older bmclib interfaces

The newer interface methods are referred to as v2, while the older as v1.

For reference the #196 PR implements both the v1, v2 interfaces for Firmware

Outcome

The idea here is to figure if the following points make sense to work through ,

  1. Reduce the amount of boilerplate required for a v2 interface method to be added under the bmc directory
  2. Could we have device probing - to identify the hardware model, type be part of the v2 interface
  3. Is theres a possibility to have some of the v1 methods available as part of the v2 interface (of if this is not a good idea at all)

What follows is a comparison of the two interfaces...

v2 vs v1

v2 - the newer bmclib interface

Pros

  1. The interfaces are smaller, and don't have to be implemented by all providers
  2. No need to type cast the bmc connection/client returned by the New() method
  3. Protocol fallthrough
  4. Feature declaration
  5. Driver registry
  6. Context propagation

Cons

  1. Driver registry and setup could be simpler
  2. Quite a bit of scafolding/boilerplate code along with test code needs to to be implemented - for example,

BMCVersion method

BMCVersion interface declaration, implementation

BMCVersion tests

  1. Requires a Probing method to identify vendor/model
  • The Compatibility method based on the registrar.Verfier interface could be hacked up to do this although it would be ideal to have one method to probe and identify hardware.

v1 - the older bmclib interface

Pros

  1. A single interface to implement
  2. Low cognitive overhead to add support for new devices/updating
  3. Capable of probing the device to identify vendor/model and returns the appropriate Bmc interface

Cons

  1. All providers need to be updated for a newer interface method or interface method signature change
  2. Lack of context propagation
  3. A new provider needs to implement all Bmc interface methods - even if its going to support just a single method
@jacobweinstock
Copy link
Member

1. Reduce the amount of boilerplate required for a v2 interface method to be added under the bmc directory
I agree there is added boilerplate for each interface we add. For me, this is one-time work and just copy/paste too. The advantages we gain from these helper methods (single client interaction via client.go) far outway the one time need to add this code. Once all the interfaces have been added this won't need to be touched. Maybe when generics come out we revisit. We definitely need more documentation around implementing a provider with the new interfaces. That is sorely lacking.

2. Could we have device probing - to identify the hardware model, type be part of the v2 interface.
Device probing is acheived though client.Registry.FilterForCompatible. Each provider implements a Compatible function.

3. Is theres a possibility to have some of the v1 methods available as part of the v2 interface (of if this is not a good idea at all)
I don't see any reason by v1 methods couldn't be used in the implementation of v2 methods.

I'm not quite sure I follow the v1/v2 comparison you have. The new interfaces have been discussed for some time here. I definitely think we need more documentation around implementing a provider using the new interfaces. I, unfortunately, haven't had time to accomplish this yet. There is an issue open for that here.

@joelrebel
Copy link
Member Author

Yep, I admit I did not entirely realize the effort that would be required for adding in the boilerplate bits,

For the probing I see we need to have a few providers added that will provide ssh/redfish/generic https drivers,
which can then fall through similar to the existing ScanAndConnect code - do I have that right?

Now having implemented the newer interface methods and going through the proposal again,
I understand the approach better, the comparision was a result of stepping through implementing the v1/v2 interfaces, your proposal covers it better.

As for documenting some of this, I'll take a stab at it :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants