CLI cpo-util HLD - #2489
Conversation
|
/azp run |
|
No pipelines are associated with this pull request. |
9720325 to
8d14bec
Compare
|
/azp run |
|
No pipelines are associated with this pull request. |
8d14bec to
a049b79
Compare
|
/azp run |
|
No pipelines are associated with this pull request. |
| | commit | Set the new firmware as the default. | Yes | | ||
| | upgrade | Perform download, run, and commit. | Yes | | ||
| | unlock | Enter module password. | No | | ||
| | target | Select target firmware for `Y-cable`. | No (not relevant) | |
There was a problem hiding this comment.
@Junchao-Mellanox Need an option to be able to select the target so that all subsequent CDB commands is directed to the targeted end point (Eg, MCU, OE, ELSP) on that port#
There was a problem hiding this comment.
We can choose it from --component option, which supports: oe | els | all
|
|
||
| Firmware execution is delegated only after `cpoutil` completes parsing, matching, and optional checksum verification. Failures are isolated per component; a failed ELS does not roll back a successful OE. The CLI reports status per internal component identity. Fatal conditions yield a non-zero exit; per-target warnings do not mask fatal errors. | ||
|
|
||
| **Frontend (CLI):** |
There was a problem hiding this comment.
@Junchao-Mellanox Please add the CLI output. User should be able to see the progress of the operation (download) and if there is a error from CDB transaction, the CLI should capture the Error code mentioned in the CDB/CMIS spec
There was a problem hiding this comment.
Will add it later.
|
|
||
| Corresponding CLI documentation should be added to https://github.com/sonic-net/sonic-utilities/blob/master/doc/Command-Reference.md when the feature is implemented. | ||
|
|
||
| > Note: Only one CLI instance may run at a time. This constraint should be enforced with a file lock. |
There was a problem hiding this comment.
@Junchao-Mellanox for simplicity we can leave this to the user where multiple ports can share resources MCU/OE/ELSP. This is same as we do for 8x100G on pluggable modules.
| > Note: currently, ElsfpApi is not inherit from CmisApi, each vendor has to implement `get_fw_ops_match_attrs` by their own. | ||
|
|
||
|
|
||
| #### 7.7 Error handling |
There was a problem hiding this comment.
@Junchao-Mellanox @eddyk-nvidia I would wish if this HLD can be split into two phases
- Basic CPO component firmware upgrade via CLI , download, run, commit using port#, firmware binary file
- OIF based package which is built on top of app table schema #1
This gives the user flexibility to build their own services around #1 (who don't want to pursue OIF package based approach). Its also easier to debug issues using #1 and much simpler to implement and on par with pluggable firmware download procedure.
| Matching stage outputs a list of `plan_entries`: | ||
|
|
||
| ``` | ||
| plan_entry { |
There was a problem hiding this comment.
Would it make sense to rename this match_entry and have the match_entries list only contain entries for matches? I am not sure of the use-case of a plan_entry that has matching_decision == None.
There was a problem hiding this comment.
matching_decision = None is used for logging purposes. For example, if a user wants to upgrade Ethernet0 and Ethernet120, but only Ethernet0 has a matching entry, we need to log that Ethernet120 has no match. Maybe we don't really need this entry, we will see in the implementation and update acoordingly.
| - Add a new function `get_fw_ops_match_attrs` to get all the FW operation matching attributes on demand. | ||
| - `ElsfpApi` should be updated to inherit from, or mix in, `CmisCdbFw` so that it can support CDB-based firmware operations. (Or, should each vendor provide a `VendorCdbFw`?) | ||
|
|
||
| > Note: currently, ElsfpApi is not inherit from CmisApi, each vendor has to implement `get_fw_ops_match_attrs` by their own. |
There was a problem hiding this comment.
If get_fw_ops_match_attrs was moved into the CmisCdbFw mixin, would this be solved? CmisApi and ElsfpApi would then share the same definition via the mixin.
There was a problem hiding this comment.
Moving it to CmisCdbFw is an option. Another option is to put it to XcvrApi. I will compare the 2 options and update.
| from dataclasses import dataclass | ||
|
|
||
| @dataclass(slots=True) | ||
| class CpoFwOpsEntry: |
There was a problem hiding this comment.
We could keep these cpoutil related data structures out of the platform API by instead adding the download, run, commit and upgrade methods on CpoDeviceBase instead.
Then both the OeBase and ElsfpBase classes will expose the firmware methods, but we no longer need the CpoFwOpsEntry to communicate the component information. Instead, cpoutil will just access the device it cares about based on the matching firmware operation entry it is about to perform an upgrade for:
# in cpoutil
cpo = get_cpo_object(fw_op_entry)
if fw_op_entry.component_class == "OpticalEngine":
cpo.oe.upgrade_firmware(file_path=fw_op_entry.file_path)
elif fw_op_entry.component_class == "ExternalLaserSource":
cpo.elsfp.upgrade_firmware(file_path=fw_op_entry.file_path)There was a problem hiding this comment.
I prefer the current way because: implementing it in CpoBase will give the max flexibility for vendor to support different hardware topology. For example, it support both joint mode and separate mode well. Implementing it in OeBase and ElsfpBase will limit it in separate mode.
There was a problem hiding this comment.
I am not sure I understand, both approaches should support arbitrary hardware topologies (separate/joint). There will always be OE and ELSFP objects present on the CpoBase object, regardless of whether the hardware operates in joint or separate mode.
The only change in my suggestion is that we don't leak cpoutil's abstraction into the platform API by removing the need to add CpoFwOpsEntry to the platform API entirely. Instead, we just add the method definitions and let cpoutil do the decision making of selecting the appropriate component to operate on instead of the platform API doing that. The main motivations being:
cpoutil's data model (CpoFwOpsEntry) ideally should not be added to the platform API as it tightly couplescpoutiland the platform API- By keeping
CpoFwOpsEntryincpoutilit makes any changes to the data model easier since you don't have to do cross-repo changes. Imagine a new field had to be added toCpoFwOpsEntry, then you might need two PRs across two repos -- one to add the field, one to plumb it through incpoutil. - Defining methods on
CpoBasecan be useful for backwards compatibility, so you can treat the OE/ELSFP as a single aggregated device like some form of pluggable transceiver. Sincecpoutilis new code, this doesn't apply and we are in the convenient position to just reason about the OE and ELSFP directly.
There was a problem hiding this comment.
- Yes, OE and ELSFP objects will always be present on the CpoBase object. However, that does not mean each OE or ELSFP has its own FW bank. For example, a single Cpo object may contain two ELSFPs that share the same FW bank. Your approach does not support this deduplication, and it would also be difficult for the vendor platform API to override this behavior.
- Regarding
CpoFwOpsEntry, I do not fully understand your point. If the interface between the CLI andsonic-platform-commonneeds to change, both repositories will need to be updated. To decouple them, I think a possible approach would be to definedict-based parameters. Would you prefer that approach?
There was a problem hiding this comment.
BTW, I am fine to replace CpoFwOpsEntry with something else.
There was a problem hiding this comment.
CpoBase does not support multiple ELSFP or OEs per port as-is today so I think sharing of resources like CDB instances and firmware banks is something that would require changes in general to support since it is a new concept. I am not sure how firmware bank sharing would work on a single CpoBase object for instance, the same problems would have to be solved.
Current approach is fine to me, no complaints if you'd prefer to keep this approach 👍 Just wanted to highlight that there is an alternative that does not require adding an extra data structure to store the decision for which component should be upgraded: client code like cpoutil can make that decision pretty easily and the firmware methods could just take the file path as a sole arg if they were defined on each device object instead of the overall cpo object.
|
/azp run |
|
No pipelines are associated with this pull request. |
Signed-off-by: Junchao-Mellanox <junchao@nvidia.com>
Signed-off-by: Junchao-Mellanox <junchao@nvidia.com>
415630b to
fbbe9c9
Compare
|
/azp run |
|
No pipelines are associated with this pull request. |
fbbe9c9 to
ebd3d2e
Compare
|
/azp run |
|
No pipelines are associated with this pull request. |
ebd3d2e to
4bffa7d
Compare
|
/azp run |
|
No pipelines are associated with this pull request. |
Signed-off-by: Junchao-Mellanox <junchao@nvidia.com>
4bffa7d to
9b98a33
Compare
|
/azp run |
|
No pipelines are associated with this pull request. |
Signed-off-by: Junchao-Mellanox <junchao@nvidia.com>
0191db9 to
48d3f1f
Compare
Signed-off-by: Junchao-Mellanox <junchao@nvidia.com>
|
/azp run |
|
No pipelines are associated with this pull request. |
No description provided.