Implement the StorageMuxFlasher interface, and Flasher cli#324
Implement the StorageMuxFlasher interface, and Flasher cli#324
Conversation
WalkthroughThis pull request modifies several driver components to implement flash functionality across multiple classes and interfaces. The Changes
Sequence Diagram(s)sequenceDiagram
participant U as User
participant CLI as FlasherClient CLI
participant SMF as StorageMuxFlasherClient
participant H as Host
U->>CLI: Issue "flash" command with file path
CLI->>SMF: Call flash(path, partition=None)
SMF->>H: Check host connection and trigger flash
alt Partition provided
SMF-->>U: Error: Partition unsupported
else
H-->>SMF: Flash confirmed
SMF-->>CLI: Return success status
end
CLI-->>U: Output flash result
sequenceDiagram
participant T as Test
participant M as MockStorageMuxFlasher
participant H as Host Interface
participant D as DUT Interface
T->>M: Invoke flash(path)
M->>H: Perform write operation ("host")
M->>D: Execute flash procedure ("dut")
H-->>M: Confirm write call
T->>M: Invoke dump(path)
M->>H: Initiate read operation ("host")
M->>D: Execute dump procedure ("dut")
H-->>M: Return data during read
T-->>M: Verify returned data matches original
Possibly related PRs
Poem
✨ Finishing Touches
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
✅ Deploy Preview for jumpstarter-docs ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (5)
packages/jumpstarter-driver-opendal/jumpstarter_driver_opendal/driver.py (2)
244-248: New StorageMuxFlasherInterface adds flashing capability to storage mux.This interface establishes a clear extension point for adding flasher capabilities to storage mux implementations, keeping the client implementation separate from the underlying driver.
However, the relationship between
StorageMuxFlasherInterfaceandFlasherInterfaceisn't explicit. Consider adding documentation to clarify the expected behavior.class StorageMuxFlasherInterface(StorageMuxInterface): + """ + Interface that combines storage mux capabilities with flashing functionality. + + Implementations should provide flash and dump methods that internally use + the host, write/read, and dut methods from StorageMuxInterface. + + This interface works similarly to FlasherInterface but is specifically designed + for storage mux devices. + """ @classmethod def client(cls) -> str: return "jumpstarter_driver_opendal.client.StorageMuxFlasherClient"
280-282: Mock implementation for testing the StorageMuxFlasher interface.This mock implementation is simple but effective for testing purposes. The associated test checks the correct sequence of method calls.
I notice that unlike
FlasherInterface,StorageMuxFlasherInterfacedoesn't explicitly declare theflashanddumpmethods. Consider making this contract explicit to help future implementers.class StorageMuxFlasherInterface(StorageMuxInterface): @classmethod def client(cls) -> str: return "jumpstarter_driver_opendal.client.StorageMuxFlasherClient" + + @abstractmethod + def flash(self, source, partition: str | None = None): ... + + @abstractmethod + def dump(self, target, partition: str | None = None): ...packages/jumpstarter-driver-dutlink/jumpstarter_driver_dutlink/driver.py (1)
230-236: **Empty composite class implementation **Declaring
DutlinkStorageMuxFlasheras a simple pass-through class is valid if its necessary methods are adequately provided by the parent classes. However, consider adding docstrings to clarify the role of this composite class and any assumptions about method resolution order or driver responsibilities. This would enhance readability and maintainability.packages/jumpstarter-driver-opendal/jumpstarter_driver_opendal/client.py (2)
528-549: **New CLI for FlasherClient **This CLI adds valuable convenience for flashing and dumping images directly via command line. Consider adding tests to validate CLI commands (e.g.,
flashanddump) and ensure error handling (e.g., invalid partition names) is properly covered.
615-662: **StorageMuxFlasherClient synergy with host/dut switching **The automatic switching to
hostbefore operations and reverting todutin afinallyblock is a good approach. A few additional suggestions:
- Consider logging messages to help users diagnose potential connectivity issues, especially if
host()ordut()fails.- Evaluate whether more granular exception handling is needed when writing/reading fails midway, ensuring the storage is reliably reconnected to the DUT.
Overall, the approach to raise
ArgumentErrorwhen a partition is provided aligns well with your design constraints.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
packages/jumpstarter-driver-dutlink/jumpstarter_driver_dutlink/driver.py(3 hunks)packages/jumpstarter-driver-dutlink/jumpstarter_driver_dutlink/driver_test.py(1 hunks)packages/jumpstarter-driver-opendal/jumpstarter_driver_opendal/client.py(3 hunks)packages/jumpstarter-driver-opendal/jumpstarter_driver_opendal/driver.py(2 hunks)packages/jumpstarter-driver-opendal/jumpstarter_driver_opendal/driver_test.py(2 hunks)packages/jumpstarter-driver-sdwire/jumpstarter_driver_sdwire/driver.py(2 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (7)
- GitHub Check: pytest-matrix (3.13)
- GitHub Check: pytest-matrix (3.12)
- GitHub Check: e2e
- GitHub Check: pytest-matrix (3.11)
- GitHub Check: build-and-push-image (jumpstarter-dev/jumpstarter-devspace .devfile/Containerfile.client)
- GitHub Check: build-and-push-image (jumpstarter-dev/jumpstarter-dev .devfile/Containerfile)
- GitHub Check: build-and-push-image (jumpstarter-dev/jumpstarter Dockerfile)
🔇 Additional comments (8)
packages/jumpstarter-driver-dutlink/jumpstarter_driver_dutlink/driver_test.py (1)
20-20: Method signature change reflects the new Flasher interface.The change from
write_local_file()toflash()aligns with the updated StorageMuxFlasher interface that's being implemented across the codebase. This is consistent with the PR objective of simplifying storage mux usage.packages/jumpstarter-driver-sdwire/jumpstarter_driver_sdwire/driver.py (1)
11-11: Import updated to include new interface.Import statement correctly includes the new
StorageMuxFlasherInterfacewhich is required for implementing the flasher functionality.packages/jumpstarter-driver-opendal/jumpstarter_driver_opendal/driver_test.py (3)
8-8: Import added for mock functionality.The unittest.mock import is correctly added to support the new test case.
14-14: Import statement updated to include new mock class.The import includes the new
MockStorageMuxFlasherclass which is tested in the new test function.
152-176: Comprehensive test for the StorageMuxFlasher implementation.This test effectively verifies:
- The correct sequence of method calls during
flashoperation (host->write->dut)- The correct sequence of method calls during
dumpoperation (host->read->dut)- Data integrity by ensuring written data can be read back correctly
The mock approach is well-implemented, allowing verification of the call sequence while still executing the actual functionality.
packages/jumpstarter-driver-dutlink/jumpstarter_driver_dutlink/driver.py (2)
14-14: **New imports for flasher interfaces **Good job importing
StorageMuxFlasherInterfacealongsideStorageMuxInterface. This aligns with the new flasher functionality introduced below and appears consistent with its usage in the newDutlinkStorageMuxFlasherclass.
267-267:Details
❓ Verification inconclusive
**Verify usage of new DutlinkStorageMuxFlasher **
Replacing
DutlinkStorageMuxwithDutlinkStorageMuxFlashermight affect code and tests that expect the previous class. Please confirm no references callDutlinkStorageMux-specific methods that may not exist or behave differently in the new class.
🏁 Script executed:
#!/bin/bash # Description: Search codebase for references to children["storage"] that might rely on the old class's behavior. fd . --type f --exec rg 'children\s*\[\s*"storage"\s*\]'Length of output: 352
Action Required: Verify storage interface compatibility
While the new class appears to be instantiated in place of the old one, the search revealed that:
- The assignment using
DutlinkStorageMuxFlasheris in use forself.children["storage"].- Other instantiations (e.g., using
Opendal) and usages—such as accessing_operatorviaoperator=self.children["storage"]._operator—remain present.Please verify that:
- The new
DutlinkStorageMuxFlasherimplements all methods and properties (especially_operator) expected by downstream calls.- No parts of the codebase are relying on behavior unique to the old
DutlinkStorageMuximplementation that might be missing or altered inDutlinkStorageMuxFlasher.Once you confirm that the interface and behavior of
DutlinkStorageMuxFlasheralign with what is expected throughout the code (including in tests), we can safely proceed.packages/jumpstarter-driver-opendal/jumpstarter_driver_opendal/client.py (1)
19-19: **Additional import for ArgumentError **Importing
ArgumentErroris correct and clearly used in the new code below to raise partition‑related exceptions.
| @dataclass(kw_only=True) | ||
| class SDWireFlasher(StorageMuxFlasherInterface, SDWire): | ||
| # compose the StorageMuxFlasherInterface with the SDWire interface, so that | ||
| # the client interface is the flash interface, but the exporter driver is | ||
| # the storage mux driver | ||
| pass |
There was a problem hiding this comment.
💡 Verification agent
❓ Verification inconclusive
New SDWireFlasher class provides a composite implementation.
The class combines the new StorageMuxFlasherInterface with the existing SDWire implementation, enabling a clean way to expose flashing capabilities while maintaining backward compatibility. The commented explanation is helpful for understanding the design intention.
However, I recommend adding a unit test for this new class to verify it works as expected.
🏁 Script executed:
#!/bin/bash
# Check if there are any tests for the SDWireFlasher class
fd -e py "test_.*\.py" packages/jumpstarter-driver-sdwire/ | xargs grep -l "SDWireFlasher"Length of output: 90
Action: Add Unit Test for SDWireFlasher
The composite implementation in the new SDWireFlasher class correctly merges the StorageMuxFlasherInterface and SDWire behavior. However, our verification did not locate any unit tests referencing this class. To ensure its proper functioning and maintain backward compatibility, please add unit tests that verify its behavior and the interaction of its composed interfaces.
mangelajo
force-pushed
the
storage-mux-flasher
branch
from
6f57cd6 to
d0d057a
Compare
|
@NickCao I have finally merged both interfaces into one, so that we allow the mux functionality and cli in addition to the flash interface. We will have to provide a way of configuring the partition names, may be by having multiple device maps (one general, and then one per partition), but I will delay that for now. |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
packages/jumpstarter-driver-opendal/jumpstarter_driver_opendal/client.py (3)
528-549: Consider adding validation and error checks on the CLI arguments.
The new CLI correctly wires upflashanddumpmethods, but it only minimally validates the CLI arguments, such as the file path. For robustness, consider checking file existence or providing user-friendly error messages before attempting I/O operations.
615-637: Unify method call usage for ‘host’.
In this method,self.host()is invoked directly, whereas indumptheself.call("host")form is used. Although both accomplish the same goal, consider using a consistent approach across the class to avoid confusion and improve maintainability.
638-660: Maintain consistent approach for connecting and disconnecting storage.
This method appropriately callshostbefore reading, then reverts todutin the finally block. However, it usesself.call("host")instead ofself.host(). As mentioned in the previous comment, consistent usage of the host/dut/off methods would improve clarity.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
packages/jumpstarter-driver-dutlink/jumpstarter_driver_dutlink/driver.py(3 hunks)packages/jumpstarter-driver-dutlink/jumpstarter_driver_dutlink/driver_test.py(1 hunks)packages/jumpstarter-driver-opendal/jumpstarter_driver_opendal/client.py(4 hunks)packages/jumpstarter-driver-opendal/jumpstarter_driver_opendal/driver.py(2 hunks)packages/jumpstarter-driver-opendal/jumpstarter_driver_opendal/driver_test.py(2 hunks)packages/jumpstarter-driver-sdwire/jumpstarter_driver_sdwire/driver.py(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
- packages/jumpstarter-driver-dutlink/jumpstarter_driver_dutlink/driver_test.py
- packages/jumpstarter-driver-opendal/jumpstarter_driver_opendal/driver.py
- packages/jumpstarter-driver-opendal/jumpstarter_driver_opendal/driver_test.py
- packages/jumpstarter-driver-dutlink/jumpstarter_driver_dutlink/driver.py
⏰ Context from checks skipped due to timeout of 90000ms (7)
- GitHub Check: pytest-matrix (3.13)
- GitHub Check: pytest-matrix (3.12)
- GitHub Check: e2e
- GitHub Check: pytest-matrix (3.11)
- GitHub Check: build-and-push-image (jumpstarter-dev/jumpstarter-devspace .devfile/Containerfile.client)
- GitHub Check: build-and-push-image (jumpstarter-dev/jumpstarter-dev .devfile/Containerfile)
- GitHub Check: build-and-push-image (jumpstarter-dev/jumpstarter Dockerfile)
🔇 Additional comments (6)
packages/jumpstarter-driver-sdwire/jumpstarter_driver_sdwire/driver.py (2)
11-11: Import added for the new composite interface.The addition of
StorageMuxFlasherInterfaceimport is appropriate as it's required for the newSDWireFlasherclass defined later in this file.
131-136: New SDWireFlasher composite class provides a clean interface integration.The class effectively combines the
StorageMuxFlasherInterfacewith the existingSDWireimplementation, creating a composite component that exposes flashing capabilities while maintaining backward compatibility with the storage mux interface.The documentation comments clearly explain the design intention, which is to have the client interface act as a flash interface while keeping the exporter driver as the storage mux driver.
#!/bin/bash # Check if there are any tests for the SDWireFlasher class echo "Searching for tests that reference the SDWireFlasher class..." fd -e py "test_.*\.py" packages/jumpstarter-driver-sdwire/ | xargs grep -l "SDWireFlasher" # Check what methods are available in the StorageMuxFlasherInterface echo "Checking methods in StorageMuxFlasherInterface..." grep -A 20 "class StorageMuxFlasherInterface" packages/jumpstarter-driver-opendal/jumpstarter_driver_opendal/driver.pypackages/jumpstarter-driver-opendal/jumpstarter_driver_opendal/client.py (4)
19-19: No concerns with the new import.
TheArgumentErrorimport is used properly in this file.
588-613: CLI composition looks good.
Allowing an optionalbaseparameter for command composition is elegant and flexible. The approach avoids duplication and integrates well with the existing click commands.
614-614: Class declaration is consistent.
Deriving from bothFlasherClientandStorageMuxClientis logical given the combined functionality needed for a storage-based flasher.
661-664: Well-structured CLI chaining.
CallingFlasherClient.cli(self)and then passing the resulting group toStorageMuxClient.cli(self, top_cli)is a clean solution for composing CLI commands from multiple parents.
What do you mean by this? Mapping each partition to a mux? |
For example, if we want to let writing to the bootloader partition, the rootfs partition, etc... on the configmap of the driver we could have ... config:
storage_device: "/ nvme-nvme.1e4b-434e333638425430343033333137-323536474220535344-00000001"
partitions:
boot: "nvme-nvme.1e4b-434e333638425430343033333137-323536474220535344-00000001-part1"
rootfs: "nvme-nvme.1e4b-434e333638425430343033333137-323536474220535344-00000001-part2"
... etc
if we add support to the storage mux :) |
This enables simplified usage of mux storage, where the driver can be instantiated for use with a Flasher interface (which only provides flash and dump), or a more granular StorageMuxInterface with dut/host/write/read/off.
mangelajo
force-pushed
the
storage-mux-flasher
branch
from
d0d057a to
5e6af02
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (5)
packages/jumpstarter-driver-opendal/jumpstarter_driver_opendal/client.py (5)
614-637: Inconsistent method usage in flash implementation.In this method, you use
self.host()to connect to the host, but in the dump method on lines 650 and 659, you useself.call("host")andself.call("dut"). For consistency, it would be better to use the same approach in both methods.- self.host() + self.call("host")And on line 636:
- self.dut() + self.call("dut")Alternatively, you could use the
self.host()andself.dut()helper methods in both implementations, which would make the code more readable.
638-660: Inconsistent method usage in dump implementation.This method uses
self.call("host"),self.call("read"), andself.call("dut")directly instead of using the helper methodsself.host(),self.read(), andself.dut()that are available fromStorageMuxClient. For consistency with the rest of the codebase and improved readability, consider using these helper methods.- self.call("host") + self.host()And on lines 657-659:
- return self.call("read", handle) - finally: - self.call("dut") + return self.read(handle) + finally: + self.dut()
619-621: Consider adding documentation for the partition parameter.Even though partition is not supported, the parameter is still part of the method signature to maintain compatibility with the base class. It would be helpful to add a note in the docstring explaining that this implementation doesn't support partitions.
- """Flash image to DUT""" + """Flash image to DUT + + Note: This implementation doesn't support the partition parameter + and will raise an ArgumentError if it's provided. + """
645-646: Consider adding documentation for the partition parameter.Similar to the flash method, it would be helpful to add a note in the docstring about the partition parameter not being supported.
- """Dump image from DUT""" + """Dump image from DUT + + Note: This implementation doesn't support the partition parameter + and will raise an ArgumentError if it's provided. + """
614-614: Consider adding class-level documentation for StorageMuxFlasherClient.This new class combines important functionality from both parent classes. A class-level docstring would help users understand its purpose and when to use it instead of the parent classes.
class StorageMuxFlasherClient(FlasherClient, StorageMuxClient): + """ + A client that combines storage multiplexer and flasher functionality. + + This class provides a simplified interface for flashing images to a device + through a storage multiplexer, automatically handling the switching between + host and device connections. + """
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
packages/jumpstarter-driver-dutlink/jumpstarter_driver_dutlink/driver.py(2 hunks)packages/jumpstarter-driver-dutlink/jumpstarter_driver_dutlink/driver_test.py(1 hunks)packages/jumpstarter-driver-opendal/jumpstarter_driver_opendal/client.py(4 hunks)packages/jumpstarter-driver-opendal/jumpstarter_driver_opendal/driver.py(2 hunks)packages/jumpstarter-driver-opendal/jumpstarter_driver_opendal/driver_test.py(2 hunks)packages/jumpstarter-driver-sdwire/jumpstarter_driver_sdwire/driver.py(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
- packages/jumpstarter-driver-dutlink/jumpstarter_driver_dutlink/driver_test.py
- packages/jumpstarter-driver-sdwire/jumpstarter_driver_sdwire/driver.py
- packages/jumpstarter-driver-dutlink/jumpstarter_driver_dutlink/driver.py
- packages/jumpstarter-driver-opendal/jumpstarter_driver_opendal/driver.py
⏰ Context from checks skipped due to timeout of 90000ms (10)
- GitHub Check: Redirect rules - jumpstarter-docs
- GitHub Check: Header rules - jumpstarter-docs
- GitHub Check: Pages changed - jumpstarter-docs
- GitHub Check: pytest-matrix (3.13)
- GitHub Check: pytest-matrix (3.12)
- GitHub Check: e2e
- GitHub Check: pytest-matrix (3.11)
- GitHub Check: build-and-push-image (jumpstarter-dev/jumpstarter-devspace .devfile/Containerfile.client)
- GitHub Check: build-and-push-image (jumpstarter-dev/jumpstarter-dev .devfile/Containerfile)
- GitHub Check: build-and-push-image (jumpstarter-dev/jumpstarter Dockerfile)
🔇 Additional comments (8)
packages/jumpstarter-driver-opendal/jumpstarter_driver_opendal/driver_test.py (1)
152-175: Test implementation for MockStorageMuxFlasher looks good!The test correctly validates the behavior of the
MockStorageMuxFlasherclass by:
- Creating a test image file with content
- Verifying the correct sequence of method calls for flash operation (host → write → dut)
- Verifying the correct sequence for dump operation (host → read → dut)
- Confirming the dumped file contains the expected content
The use of
mock.patch.objectas a spy (while preserving the original functionality) is an excellent approach for this test case.packages/jumpstarter-driver-opendal/jumpstarter_driver_opendal/client.py (7)
528-548: CLI implementation for FlasherClient looks good!The implementation adds a clean CLI interface with two commands (flash and dump) that properly expose the flasher functionality. The commands have appropriate descriptions and accept the necessary arguments.
588-590: Good enhancement to StorageMuxClient.cli method!Making the base parameter optional allows for more flexible command grouping, which enables composition with other CLI interfaces as used in the
StorageMuxFlasherClient.
623-626: Good error handling for unsupported partition parameter.The clear error message with the specific partition value is helpful for debugging.
646-649: Good error handling for unsupported partition parameter.The error handling is consistent with the flash method.
661-663: Elegant CLI composition in StorageMuxFlasherClient.The implementation nicely combines both parent class CLIs, leveraging the optional base parameter feature added to StorageMuxClient.cli. This approach creates a unified interface without duplicating code.
633-636: Good use of try-finally for cleanup.The try-finally block ensures that the storage will be connected back to the DUT even if the write operation fails, which is important for maintaining the expected state of the system.
656-659: Good use of try-finally for cleanup.Similar to the flash method, this ensures proper cleanup regardless of operation success or failure.
|
The CLI composition part could use more ideas, the rest LGTM! |
|
Thanks, I will probably continue cleanups, how would you improve the cli composition? |
2 participants
This enables simplified usage of mux storage, where the driver can be instantiated for use with a Flasher interface (which only provides flash and dump), or a more granular StorageMuxInterface with dut/host/write/read/off.
Summary by CodeRabbit
New Features
Tests
MockStorageMuxFlasherclass to verify functionality.