[mellanox_firmware] Add Mellanox firmware plugin #3407
Conversation
|
Congratulations! One of the builds has completed. 🍾 You can install the built RPMs by following these steps:
Please note that the RPMs should be used only in a testing environment. |
There was a problem hiding this comment.
Comments below about the gating approach used - I think we can clean that up a bit, but overall I think the design of the plugin is acceptable once that is done and we have more details on what the bits gated by --allow-system-changes is wanting to do.
In addition to the comments below, please note that this project is attempting to transition/convert to f-strings for string substitution/interpolation, so let's look at replacing the legacy substitutions before merge.
|
@TurboTurtle thank you for the review and suggestions, I will try to revisit this PR tomorrow |
ghost
marked this pull request as draft
ghost
marked this pull request as ready for review
|
I revisited the approach taken on how to mine for the packages and when to enable the plugin. Thanks again for the guidance and reviews @TurboTurtle |
| return False | ||
| return self.MLNX_STRING in lspci['output'] | ||
|
|
||
| def collect(self): |
There was a problem hiding this comment.
Why do you need to overwrite collect method? Usually, whatever add_cmd_output you call in setup, the plugin will automatically collect, knowing the whole list of commands in advance.
Is there some reason of having collect_cmd_output in collect, instead of having the commands under add_cmd_output in setup?
(same applies to "no --allow-system-changes or if flint --version fails, return")
There was a problem hiding this comment.
It was suggested here: #3407 (comment)
There was a problem hiding this comment.
Yeah, since we're modifying system state, I think it best if we do this during collect(), and that requires using collect_cmd_output() for the just-in-time writing to the plugin dir.
There was a problem hiding this comment.
Ah I see, it makes sense (and I should read whole PR discussion before reviewing).
Maybe then it is worth having a comment abou the unusual approach in the code?
| return False | ||
| return self.MLNX_STRING in lspci['output'] | ||
|
|
||
| def collect(self): |
There was a problem hiding this comment.
Yeah, since we're modifying system state, I think it best if we do this during collect(), and that requires using collect_cmd_output() for the just-in-time writing to the plugin dir.
This patch reports the output of the following Mellanox firmware commands: mlxdump mstconfig mstdump mstflint mlxreg mlxlink mlxcables mst Additionally, if the user allows system changes, we will atempt to start the mst device, add the cables and stop the device in the end. Signed-off-by: Alin-Gabriel Serdean <[email protected]>
| if device_list['status'] != 0: | ||
| # bail out if there no Mellanox PCI devices | ||
| return |
There was a problem hiding this comment.
This is required over the check_enabled method in cases when user manually enables the plugin, ACK.
This patch reports the output of the following Mellanox firmware commands:
mlxdump
mstconfig
mstdump
mstflint
mlxreg
mlxlink
mlxcables
mst
Additionally, if the user allows system changes, we will atempt to start the mst device, add the cables and stop the device in the end.
Please place an 'X' inside each '[]' to confirm you adhere to our Contributor Guidelines