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 client to filter for compatible drivers #9

Open
joelrebel opened this issue May 25, 2022 · 6 comments
Open

bmclib client to filter for compatible drivers #9

joelrebel opened this issue May 25, 2022 · 6 comments
Labels
good first issue Good for newcomers kind/feature New feature or request

Comments

@joelrebel
Copy link

joelrebel commented May 25, 2022

Raising this as an issue to point out, it would be ideal if this used the FilterForCompatible method
here

client.Registry.Drivers = client.Registry.PreferDriver("gofish")

This way the BMC is 'probed' for compatibility and only drivers known to be working with the BMC will be attempted
when the caller invokes the client methods.

drivers := client.Registry.FilterForCompatible(ctx)
if len(drivers) == 0 {
  return nil, errors.New("no compatible bmclib drivers found")
}

client.Registry.Drivers = drivers

I understand the redfish driver is hardcoded at the moment, so whenever that is changed I suggest using the filter method above.

@chrisdoherty4 chrisdoherty4 added good first issue Good for newcomers kind/feature New feature or request labels May 25, 2022
@chrisdoherty4
Copy link
Member

@pokearu

@chrisdoherty4 chrisdoherty4 added this to the 0.1.0 milestone Jun 22, 2022
@chrisdoherty4
Copy link
Member

@joelrebel @jacobweinstock Am I right in saying FilterForCompatible() would probe every time its called? We create a client every time an object is reconciled so we'd need to go through that probing process each time - is that feasible?

@joelrebel
Copy link
Author

@chrisdoherty4 yep, FilterForCompatible() will attempt to check BMC compatibility by connecting to the BMC when invoked. At what frequency is the object going to be reconciled?

In my experience BMCs are tend to behave erratically as their uptime goes higher, or are hammered with a lot of requests.
I'd suggest the reconciliation frequency be kept lower, if thats feasible.

@chrisdoherty4
Copy link
Member

chrisdoherty4 commented Jun 28, 2022

@joelrebel It heavily depends on whats happening. If users submit a series of jobs then individual tasks will create a new client every time they're reconciled.

In the context of Tinkerbell (CAPT specifically), its probably not that often because once a machine is provisioned we no longer use BMC. That said, Rufio could be used by other things that execute jobs more frequently and we'd continually establish new connections, probe and execute.

We could, longer term, think about a client cache given controllers share the same process space. For now, I think we can proceed with FilterForCompatible() and accept the inefficiency (premature optimization n'all) - does that sound reasonable?

@joelrebel
Copy link
Author

joelrebel commented Jun 30, 2022

@chrisdoherty4 a suggestion would be to having each BMC guarded under a lock so theres just one job acting on a BMC to completion at any given moment. For example, a power reset while the BMC configuration is being updated would have a negative impact on the current BMC configuration, or a power reset during a firmware update. Is this sort of BMC lock feasible?

The client cache sounds fair, a cache along with a bmclib client option to provide "hints" would work - the older client has such an implemention. This could then mean FilterForCompaible() would be performed on the first connect and in subsequent requests, "hints" are used instead.

@chrisdoherty4
Copy link
Member

chrisdoherty4 commented Jun 30, 2022

@joelrebel The locking suggestion thing tackles a different problem - I was suggesting a user could submit jobs in serial, they execute in serial, and it would connect and run the filtering algorithm every time (i.e. its dependent on use).

However, your point on controlling whats executing on the BMC is understood; I don't think that's something we're interested in solving though. Thats something the BMC implementation really needs to guard against, not external entities given no 2 entities are necessarily in sync (for example, 2 human operators or 1 human and 1 machine).

@chrisdoherty4 chrisdoherty4 modified the milestones: 0.1, 0.2 Jul 15, 2022
@chrisdoherty4 chrisdoherty4 modified the milestones: v0.2, v0.3 Feb 14, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
good first issue Good for newcomers kind/feature New feature or request
Projects
None yet
Development

No branches or pull requests

2 participants