[3/3] supermicro firmware install rewrite #363
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## new-firmware-interfaces #363 +/- ##
==========================================================
Coverage ? 45.02%
==========================================================
Files ? 60
Lines ? 5017
Branches ? 0
==========================================================
Hits ? 2259
Misses ? 2501
Partials ? 257 ☔ View full report in Codecov by Sentry. |
providers/supermicro/firmware.go
Outdated
|
|
||
| // FirmwareTaskStatus returns the status of a firmware related task queued on the BMC. | ||
| func (c *Client) FirmwareTaskStatus(ctx context.Context, kind constants.FirmwareInstallStep, component, taskID, installVersion string) (state, status string, err error) { | ||
| if err := c.firmwareInstallSupported(ctx); err != nil { |
There was a problem hiding this comment.
It feels like there should be a better way to encapsulate this rather than repeat a sanity check 4-ish times, but right now I don't have a ready answer. More food-for-thought.
There was a problem hiding this comment.
This is because of the way these bmc client interface methods are called - FirmwareUpload, FirmwareTaskStatus - each method is attempted on every provider (dell, redfish, smc...) and so each provider implementation has to make sure its running on supported hardware before proceeding (atleast I'd like to be twice as careful for firmware related actions).
| // at the end of the install the BMC resets itself and the response is in HTML | ||
| // | ||
| switch { | ||
| part := strings.Split(string(resp), "<percent>")[1] |
There was a problem hiding this comment.
This sort of string manipulation is brittle and only used for reporting progress. Do we need a fine-grained measure of progress? Maybe we'd be better served with a watchdog timer or something?
There was a problem hiding this comment.
We don't need it as much as having to track if the install succeeded or not, and its a nice to have when installing firmware that takes a while. A watchdog timer would be more of a hammer - how do we define a timeout that works across models.
tries to validate we have the percent values to extract, and hopefully makes it less brittle.
joelrebel
force-pushed
the
new-firmware-interfaces
branch
from
4004cbc to
49efe08
Compare
There has been no real use for having an io.Reader passed in and this interface is to expect a file instead.
joelrebel
force-pushed
the
smc-rewrite
branch
from
4c4d249 to
3a43b92
Compare
|
rebased on parent branch, fixed up |
… x11, x12 based on feedback recieved this approach made more sense
There was a problem hiding this comment.
Are there tests for the x12 side of things?
There was a problem hiding this comment.
All of the x12 code is calling into the redfishwrapper, which is pretty well covered https://github.com/bmc-toolbox/bmclib/blob/smc-rewrite/internal/redfishwrapper/firmware_test.go
One of the methods could use some testing - I'll add that in a separate PR
bmclib/providers/supermicro/x12.go
Line 144 in 370e066
What does this PR implement/change/remove?
Checklist