Conversation
WalkthroughA new iSCSI target server driver package, Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant ISCSIServerClient
participant ISCSI Driver
participant Storage
User->>ISCSIServerClient: start()
ISCSIServerClient->>ISCSI Driver: start()
ISCSI Driver-->>ISCSIServerClient: Server started
User->>ISCSIServerClient: upload_image()
ISCSIServerClient->>Storage: Check/Upload image
ISCSIServerClient->>ISCSI Driver: add_lun(name, file_path, size_mb)
ISCSI Driver-->>ISCSIServerClient: LUN added
User->>ISCSIServerClient: list_luns()
ISCSIServerClient->>ISCSI Driver: list_luns()
ISCSI Driver-->>ISCSIServerClient: LUN info
User->>ISCSIServerClient: stop()
ISCSIServerClient->>ISCSI Driver: stop()
ISCSI Driver-->>ISCSIServerClient: Server stopped
Suggested reviewers
Poem
✨ Finishing Touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. 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 project configuration. |
bennyz
force-pushed
the
iscsi
branch
3 times, most recently
from
b1ff357 to
6baa131
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (11)
packages/jumpstarter-driver-iscsi/README.md (1)
1-4: Consider expanding the README for better user experience.While the basic description is clear, the README could benefit from additional sections such as:
- Installation instructions
- Basic usage examples
- Configuration options
- Prerequisites (e.g., iSCSI tools)
- Links to examples directory
Given this is a WIP PR, this enhancement can be addressed in future iterations.
packages/jumpstarter-driver-iscsi/test_iscsi.py (3)
45-46: Move socket import to module level for consistency.Import statements should typically be placed at the module level rather than inside functions for better readability and consistency.
Apply this diff:
import os import tempfile +import socket from jumpstarter.common.utils import envAnd remove the import from inside the function:
print("5. Testing target accessibility...") - import socket
14-16: Consider using constants for magic numbers.The hardcoded values could be defined as constants for better maintainability.
+# Test configuration constants +TEST_IMAGE_SIZE_MB = 10 +TEST_IMAGE_SIZE_BYTES = TEST_IMAGE_SIZE_MB * 1024 * 1024 + def test_iscsi_driver(): """Test the iSCSI driver basic functionality""" print("Testing iSCSI driver...") # Create a test image with tempfile.NamedTemporaryFile(delete=False, suffix=".img") as f: - # Create a 10MB test file - f.write(b"0" * (10 * 1024 * 1024)) + # Create a test file + f.write(b"0" * TEST_IMAGE_SIZE_BYTES)
36-36: Ensure size parameter consistency.The
size_mb=10parameter should use the same constant as the file size to maintain consistency.- lun_name = iscsi.upload_image("test", test_image, size_mb=10) + lun_name = iscsi.upload_image("test", test_image, size_mb=TEST_IMAGE_SIZE_MB)packages/jumpstarter-driver-iscsi/examples/exporter.yaml (1)
16-16: Fix YAML formatting issues.Static analysis detected trailing spaces and missing newline at the end of the file.
Apply this diff to fix the formatting:
- port: 3260 + port: 3260 +packages/jumpstarter-driver-iscsi/jumpstarter_driver_iscsi/client.py (1)
106-123: Remove unnecessaryelseafterreturn.The
elseblock can be de-indented to improve code readability.def _calculate_file_hash(self, file_path: str, operator: Optional[Operator] = None) -> str: """Calculate SHA256 hash of a file""" if operator is None: hash_obj = hashlib.sha256() with open(file_path, "rb") as f: while chunk := f.read(8192): hash_obj.update(chunk) return hash_obj.hexdigest() - else: - from jumpstarter_driver_opendal.client import operator_for_path - - path, op, _ = operator_for_path(file_path) - hash_obj = hashlib.sha256() - with op.open(str(path), "rb") as f: - while chunk := f.read(8192): - hash_obj.update(chunk) - return hash_obj.hexdigest() + + from jumpstarter_driver_opendal.client import operator_for_path + + path, op, _ = operator_for_path(file_path) + hash_obj = hashlib.sha256() + with op.open(str(path), "rb") as f: + while chunk := f.read(8192): + hash_obj.update(chunk) + return hash_obj.hexdigest()packages/jumpstarter-driver-iscsi/examples/iscsi.py (2)
11-25: Simplify conditional structure.Remove unnecessary
elifafterreturnfor better readability.def determine_architecture(arch, image): """Determine target architecture from parameter or auto-detect""" if arch != "auto": return arch import platform if "aarch64" in image.lower() or "arm64" in image.lower(): return "aarch64" - elif "x86_64" in image.lower() or "amd64" in image.lower(): + if "x86_64" in image.lower() or "amd64" in image.lower(): return "x86_64" - else: - system_arch = platform.machine() - return "aarch64" if system_arch in ["aarch64", "arm64"] else "x86_64" + + system_arch = platform.machine() + return "aarch64" if system_arch in ["aarch64", "arm64"] else "x86_64"
48-54: Simplify function structure.def generate_qemu_command(target_arch, host, port, target_iqn): """Generate QEMU command based on architecture""" if target_arch == "aarch64": return f"qemu-system-aarch64 -m 2048 -machine virt -cpu cortex-a72 -drive file=iscsi://{host}:{port}/{target_iqn}/0,format=raw" - else: - return f"qemu-system-x86_64 -m 2048 -drive file=iscsi://{host}:{port}/{target_iqn}/0,format=raw" + + return f"qemu-system-x86_64 -m 2048 -drive file=iscsi://{host}:{port}/{target_iqn}/0,format=raw"packages/jumpstarter-driver-iscsi/jumpstarter_driver_iscsi/driver.py (3)
72-81: Consider improving default IP detection.The current approach of connecting to 8.8.8.8 may not work in isolated networks or when external DNS is blocked.
def get_default_ip(self): """Get the IP address of the default route interface""" try: + # First try to get the default route interface IP without external connection + import subprocess + result = subprocess.run(['ip', 'route', 'get', '1.1.1.1'], capture_output=True, text=True) + if result.returncode == 0: + # Extract IP from output like "1.1.1.1 via X.X.X.X dev eth0 src Y.Y.Y.Y" + for part in result.stdout.split(): + if part.count('.') == 3 and part != '1.1.1.1': + try: + socket.inet_aton(part) + return part + except socket.error: + continue + + # Fallback to socket connection method with socket.socket(socket.AF_INET, socket.SOCK_DGRAM) as s: s.connect(("8.8.8.8", 80)) return s.getsockname()[0]
174-176: Resolve TODO: Consider preserving LUNs on stop.The current implementation removes all LUNs when stopping the server, which might not be the desired behavior in all cases. Consider adding a parameter to control this behavior.
Would you like me to implement a solution that adds a
preserve_lunsparameter to the stop method?
223-233: Remove unnecessaryelseafterreturn.def _get_full_path(self, file_path: str, is_block: bool) -> str: """Get the full path for the LUN file or block device""" if is_block: if not os.path.isabs(file_path): raise ISCSIError("For block devices, file_path must be an absolute path") return file_path - else: - full_path = os.path.join(self.root_dir, file_path) - os.makedirs(os.path.dirname(full_path), exist_ok=True) - return full_path + + full_path = os.path.join(self.root_dir, file_path) + os.makedirs(os.path.dirname(full_path), exist_ok=True) + return full_path
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
packages/jumpstarter-driver-iscsi/README.md(1 hunks)packages/jumpstarter-driver-iscsi/examples/exporter.yaml(1 hunks)packages/jumpstarter-driver-iscsi/examples/iscsi.py(1 hunks)packages/jumpstarter-driver-iscsi/jumpstarter_driver_iscsi/client.py(1 hunks)packages/jumpstarter-driver-iscsi/jumpstarter_driver_iscsi/driver.py(1 hunks)packages/jumpstarter-driver-iscsi/pyproject.toml(1 hunks)packages/jumpstarter-driver-iscsi/test_iscsi.py(1 hunks)pyproject.toml(1 hunks)
🧰 Additional context used
🧠 Learnings (8)
📓 Common learnings
Learnt from: bennyz
PR: jumpstarter-dev/jumpstarter#241
File: packages/jumpstarter-driver-tftp/jumpstarter_driver_tftp/client.py:52-60
Timestamp: 2025-01-29T11:52:43.554Z
Learning: The TFTP driver (packages/jumpstarter-driver-tftp/jumpstarter_driver_tftp/driver.py) handles all low-level concerns like path validation, error handling, and checksum computation. The client (packages/jumpstarter-driver-tftp/jumpstarter_driver_tftp/client.py) should remain simple as it delegates these responsibilities to the driver.
Learnt from: bennyz
PR: jumpstarter-dev/jumpstarter#267
File: packages/jumpstarter-driver-tftp/jumpstarter_driver_tftp/driver.py:32-40
Timestamp: 2025-02-08T16:08:34.616Z
Learning: In the TFTP driver (jumpstarter_driver_tftp), the TFTP server component is read-only, while file management operations (upload, delete) are handled through the driver's client interface, not through the TFTP protocol itself.
Learnt from: NickCao
PR: jumpstarter-dev/jumpstarter#463
File: packages/jumpstarter-cli-admin/jumpstarter_cli_admin/get_test.py:270-270
Timestamp: 2025-05-28T15:09:35.768Z
Learning: The jumpstarter CLI is designed to match kubectl's behavior, including returning exit code 0 (success) when no resources are found, rather than exit code 1 (failure).
pyproject.toml (3)
Learnt from: bennyz
PR: jumpstarter-dev/jumpstarter#241
File: packages/jumpstarter-driver-tftp/jumpstarter_driver_tftp/client.py:52-60
Timestamp: 2025-01-29T11:52:43.554Z
Learning: The TFTP driver (packages/jumpstarter-driver-tftp/jumpstarter_driver_tftp/driver.py) handles all low-level concerns like path validation, error handling, and checksum computation. The client (packages/jumpstarter-driver-tftp/jumpstarter_driver_tftp/client.py) should remain simple as it delegates these responsibilities to the driver.
Learnt from: bennyz
PR: jumpstarter-dev/jumpstarter#267
File: packages/jumpstarter-driver-tftp/jumpstarter_driver_tftp/driver.py:32-40
Timestamp: 2025-02-08T16:08:34.616Z
Learning: In the TFTP driver (jumpstarter_driver_tftp), the TFTP server component is read-only, while file management operations (upload, delete) are handled through the driver's client interface, not through the TFTP protocol itself.
Learnt from: NickCao
PR: jumpstarter-dev/jumpstarter#463
File: packages/jumpstarter-cli-admin/jumpstarter_cli_admin/get_test.py:270-270
Timestamp: 2025-05-28T15:09:35.768Z
Learning: The jumpstarter CLI is designed to match kubectl's behavior, including returning exit code 0 (success) when no resources are found, rather than exit code 1 (failure).
packages/jumpstarter-driver-iscsi/README.md (3)
Learnt from: bennyz
PR: jumpstarter-dev/jumpstarter#267
File: packages/jumpstarter-driver-tftp/jumpstarter_driver_tftp/driver.py:32-40
Timestamp: 2025-02-08T16:08:34.616Z
Learning: In the TFTP driver (jumpstarter_driver_tftp), the TFTP server component is read-only, while file management operations (upload, delete) are handled through the driver's client interface, not through the TFTP protocol itself.
Learnt from: bennyz
PR: jumpstarter-dev/jumpstarter#241
File: packages/jumpstarter-driver-tftp/jumpstarter_driver_tftp/client.py:52-60
Timestamp: 2025-01-29T11:52:43.554Z
Learning: The TFTP driver (packages/jumpstarter-driver-tftp/jumpstarter_driver_tftp/driver.py) handles all low-level concerns like path validation, error handling, and checksum computation. The client (packages/jumpstarter-driver-tftp/jumpstarter_driver_tftp/client.py) should remain simple as it delegates these responsibilities to the driver.
Learnt from: NickCao
PR: jumpstarter-dev/jumpstarter#463
File: packages/jumpstarter-cli-admin/jumpstarter_cli_admin/get_test.py:270-270
Timestamp: 2025-05-28T15:09:35.768Z
Learning: The jumpstarter CLI is designed to match kubectl's behavior, including returning exit code 0 (success) when no resources are found, rather than exit code 1 (failure).
packages/jumpstarter-driver-iscsi/pyproject.toml (1)
Learnt from: bennyz
PR: jumpstarter-dev/jumpstarter#241
File: packages/jumpstarter-driver-tftp/jumpstarter_driver_tftp/client.py:52-60
Timestamp: 2025-01-29T11:52:43.554Z
Learning: The TFTP driver (packages/jumpstarter-driver-tftp/jumpstarter_driver_tftp/driver.py) handles all low-level concerns like path validation, error handling, and checksum computation. The client (packages/jumpstarter-driver-tftp/jumpstarter_driver_tftp/client.py) should remain simple as it delegates these responsibilities to the driver.
packages/jumpstarter-driver-iscsi/test_iscsi.py (1)
Learnt from: bennyz
PR: jumpstarter-dev/jumpstarter#241
File: packages/jumpstarter-driver-tftp/jumpstarter_driver_tftp/client.py:52-60
Timestamp: 2025-01-29T11:52:43.554Z
Learning: The TFTP driver (packages/jumpstarter-driver-tftp/jumpstarter_driver_tftp/driver.py) handles all low-level concerns like path validation, error handling, and checksum computation. The client (packages/jumpstarter-driver-tftp/jumpstarter_driver_tftp/client.py) should remain simple as it delegates these responsibilities to the driver.
packages/jumpstarter-driver-iscsi/jumpstarter_driver_iscsi/client.py (2)
Learnt from: bennyz
PR: jumpstarter-dev/jumpstarter#241
File: packages/jumpstarter-driver-tftp/jumpstarter_driver_tftp/client.py:52-60
Timestamp: 2025-01-29T11:52:43.554Z
Learning: The TFTP driver (packages/jumpstarter-driver-tftp/jumpstarter_driver_tftp/driver.py) handles all low-level concerns like path validation, error handling, and checksum computation. The client (packages/jumpstarter-driver-tftp/jumpstarter_driver_tftp/client.py) should remain simple as it delegates these responsibilities to the driver.
Learnt from: bennyz
PR: jumpstarter-dev/jumpstarter#267
File: packages/jumpstarter-driver-tftp/jumpstarter_driver_tftp/driver.py:32-40
Timestamp: 2025-02-08T16:08:34.616Z
Learning: In the TFTP driver (jumpstarter_driver_tftp), the TFTP server component is read-only, while file management operations (upload, delete) are handled through the driver's client interface, not through the TFTP protocol itself.
packages/jumpstarter-driver-iscsi/examples/iscsi.py (1)
Learnt from: bennyz
PR: jumpstarter-dev/jumpstarter#241
File: packages/jumpstarter-driver-tftp/jumpstarter_driver_tftp/client.py:52-60
Timestamp: 2025-01-29T11:52:43.554Z
Learning: The TFTP driver (packages/jumpstarter-driver-tftp/jumpstarter_driver_tftp/driver.py) handles all low-level concerns like path validation, error handling, and checksum computation. The client (packages/jumpstarter-driver-tftp/jumpstarter_driver_tftp/client.py) should remain simple as it delegates these responsibilities to the driver.
packages/jumpstarter-driver-iscsi/jumpstarter_driver_iscsi/driver.py (2)
Learnt from: bennyz
PR: jumpstarter-dev/jumpstarter#241
File: packages/jumpstarter-driver-tftp/jumpstarter_driver_tftp/client.py:52-60
Timestamp: 2025-01-29T11:52:43.554Z
Learning: The TFTP driver (packages/jumpstarter-driver-tftp/jumpstarter_driver_tftp/driver.py) handles all low-level concerns like path validation, error handling, and checksum computation. The client (packages/jumpstarter-driver-tftp/jumpstarter_driver_tftp/client.py) should remain simple as it delegates these responsibilities to the driver.
Learnt from: bennyz
PR: jumpstarter-dev/jumpstarter#267
File: packages/jumpstarter-driver-tftp/jumpstarter_driver_tftp/driver.py:32-40
Timestamp: 2025-02-08T16:08:34.616Z
Learning: In the TFTP driver (jumpstarter_driver_tftp), the TFTP server component is read-only, while file management operations (upload, delete) are handled through the driver's client interface, not through the TFTP protocol itself.
🧬 Code Graph Analysis (1)
packages/jumpstarter-driver-iscsi/jumpstarter_driver_iscsi/client.py (4)
packages/jumpstarter-driver-iscsi/jumpstarter_driver_iscsi/driver.py (9)
client(83-84)start(153-165)stop(168-185)get_host(188-194)get_port(197-203)get_target_iqn(206-212)add_lun(267-304)remove_lun(307-332)list_luns(335-352)packages/jumpstarter-driver-composite/jumpstarter_driver_composite/client.py (1)
CompositeClient(9-28)packages/jumpstarter/jumpstarter/client/base.py (1)
call(36-46)packages/jumpstarter-driver-opendal/jumpstarter_driver_opendal/client.py (1)
operator_for_path(45-57)
🪛 YAMLlint (1.37.1)
packages/jumpstarter-driver-iscsi/examples/exporter.yaml
[error] 16-16: no new line character at the end of file
(new-line-at-end-of-file)
[error] 16-16: trailing spaces
(trailing-spaces)
🪛 Pylint (3.3.7)
packages/jumpstarter-driver-iscsi/jumpstarter_driver_iscsi/client.py
[refactor] 108-122: Unnecessary "else" after "return", remove the "else" and de-indent the code inside it
(R1705)
[refactor] 152-152: Too many arguments (6/5)
(R0913)
[refactor] 152-152: Too many positional arguments (6/5)
(R0917)
packages/jumpstarter-driver-iscsi/examples/iscsi.py
[refactor] 18-24: Unnecessary "elif" after "return", remove the leading "el" from "elif"
(R1705)
[refactor] 50-53: Unnecessary "else" after "return", remove the "else" and de-indent the code inside it
(R1705)
[refactor] 66-66: Too many local variables (18/15)
(R0914)
[error] 121-121: No value for argument 'image' in function call
(E1120)
[error] 121-121: No value for argument 'location' in function call
(E1120)
[error] 121-121: No value for argument 'lun_name' in function call
(E1120)
[error] 121-121: No value for argument 'arch' in function call
(E1120)
packages/jumpstarter-driver-iscsi/jumpstarter_driver_iscsi/driver.py
[refactor] 32-32: Too many instance attributes (13/7)
(R0902)
[refactor] 225-232: Unnecessary "else" after "return", remove the "else" and de-indent the code inside it
(R1705)
⏰ Context from checks skipped due to timeout of 90000ms (7)
- GitHub Check: pytest-matrix (macos-15, 3.12)
- GitHub Check: pytest-matrix (macos-15, 3.13)
- GitHub Check: pytest-matrix (ubuntu-24.04, 3.11)
- GitHub Check: pytest-matrix (macos-15, 3.11)
- GitHub Check: pytest-matrix (ubuntu-24.04, 3.13)
- GitHub Check: pytest-matrix (ubuntu-24.04, 3.12)
- GitHub Check: e2e
🔇 Additional comments (7)
pyproject.toml (1)
32-32: LGTM - Workspace registration follows established pattern.The iSCSI driver package is correctly registered in the workspace sources with consistent formatting and proper alphabetical ordering.
packages/jumpstarter-driver-iscsi/test_iscsi.py (1)
9-77: Well-structured integration test with good error handling.The test provides comprehensive coverage of the iSCSI driver functionality including server lifecycle, LUN management, and network accessibility verification. The cleanup logic in the finally block ensures proper resource management.
packages/jumpstarter-driver-iscsi/examples/exporter.yaml (1)
1-16: Configuration structure follows Jumpstarter CRD pattern.The exporter configuration properly defines the iSCSI driver export with appropriate metadata and configuration parameters. The structure is consistent with other Jumpstarter driver exports.
packages/jumpstarter-driver-iscsi/pyproject.toml (1)
8-14: Dependencies are appropriate for iSCSI functionality.The dependency list includes
rtslib-fbwhich is the correct Python library for managing iSCSI targets, along with standard Jumpstarter dependencies. The selection aligns with the driver's purpose.packages/jumpstarter-driver-iscsi/jumpstarter_driver_iscsi/client.py (3)
1-19: LGTM!The imports and class structure follow the established pattern for Jumpstarter drivers.
20-104: Public methods correctly delegate to the driver.The client interface properly follows the established pattern where all low-level concerns are handled by the driver, keeping the client simple.
186-189: Replace private storage attribute access with a public APIAccessing
self.storage._storage.root_dirbreaks encapsulation. Please use (or add) a public method on the storage driver to get the full path or compute the file size.• Location
– File:packages/jumpstarter-driver-iscsi/jumpstarter_driver_iscsi/client.py
– Lines: 186–189• Suggested change
- src_path = os.path.join(self.storage._storage.root_dir, dst_path) + src_path = self.storage.get_full_path(dst_path) # use public API # …then compute size_mb based on src_path, or… + size_mb = self.storage.get_size(dst_path) # if availableIf no such method exists, please introduce one (e.g.
get_full_path(path)orget_size(path)) in the storage driver and update this call accordingly.
| [tool.pytest.ini_options] | ||
| asyncio_mode = "strict" | ||
| asyncio_default_fixture_loop_scope = "function" | ||
| testpaths = ["src"] |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Verify testpaths configuration matches package structure.
The testpaths = ["src"] setting expects a src/ directory layout, but based on the file structure shown, tests appear to be in the package root.
🏁 Script executed:
#!/bin/bash
# Description: Check if the package follows src layout or has tests in root
# Expected: Determine correct testpaths configuration
echo "Checking package structure for jumpstarter-driver-iscsi:"
fd -t f -e py . packages/jumpstarter-driver-iscsi/ | head -20
echo -e "\nChecking for src directory:"
fd -t d "^src$" packages/jumpstarter-driver-iscsi/
echo -e "\nChecking for test files location:"
fd -t f "test_*.py" packages/jumpstarter-driver-iscsi/Length of output: 759
Update testpaths to reflect actual test location
The testpaths = ["src"] setting in packages/jumpstarter-driver-iscsi/pyproject.toml won’t discover your tests because there is no src/ directory. All tests (e.g. test_iscsi.py) live at the package root. To fix this:
• File: packages/jumpstarter-driver-iscsi/pyproject.toml
– Change line 27 from:
diff - testpaths = ["src"] + testpaths = ["."]
(or remove the testpaths entry entirely to use pytest’s default discovery)
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| testpaths = ["src"] | |
| # In packages/jumpstarter-driver-iscsi/pyproject.toml, | |
| # update the pytest ini options to point at the project root: | |
| testpaths = ["."] |
🤖 Prompt for AI Agents
In packages/jumpstarter-driver-iscsi/pyproject.toml at line 27, the testpaths
setting incorrectly points to "src" which does not exist. Update the testpaths
value to point to the package root where the tests actually reside, or remove
the testpaths entry entirely to allow pytest to use its default test discovery
mechanism.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
packages/jumpstarter-driver-iscsi/examples/iscsi.py (2)
34-38: Safety confirmation implementation looks good.The user confirmation prompt before writing to block devices effectively addresses the safety concern from previous reviews.
42-42: Efficient file handling using storage API.Good use of
write_from_path()which handles streaming internally, addressing the memory efficiency concern from previous reviews.Also applies to: 47-47
🧹 Nitpick comments (2)
packages/jumpstarter-driver-iscsi/examples/iscsi.py (2)
18-24: Simplify control flow by removing unnecessary elif/else after return.The elif and else clauses are unnecessary since the previous conditions already return.
- if "aarch64" in image.lower() or "arm64" in image.lower(): - return "aarch64" - elif "x86_64" in image.lower() or "amd64" in image.lower(): - return "x86_64" - else: - system_arch = platform.machine() - return "aarch64" if system_arch in ["aarch64", "arm64"] else "x86_64" + if "aarch64" in image.lower() or "arm64" in image.lower(): + return "aarch64" + if "x86_64" in image.lower() or "amd64" in image.lower(): + return "x86_64" + + system_arch = platform.machine() + return "aarch64" if system_arch in ["aarch64", "arm64"] else "x86_64"
54-57: Remove unnecessary else after return.The else clause is unnecessary since the if condition already returns.
- if target_arch == "aarch64": - return f"qemu-system-aarch64 -m 2048 -machine virt -cpu cortex-a72 -drive file=iscsi://{host}:{port}/{target_iqn}/0,format=raw" - else: - return f"qemu-system-x86_64 -m 2048 -drive file=iscsi://{host}:{port}/{target_iqn}/0,format=raw" + if target_arch == "aarch64": + return f"qemu-system-aarch64 -m 2048 -machine virt -cpu cortex-a72 -drive file=iscsi://{host}:{port}/{target_iqn}/0,format=raw" + + return f"qemu-system-x86_64 -m 2048 -drive file=iscsi://{host}:{port}/{target_iqn}/0,format=raw"
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
packages/jumpstarter-driver-iscsi/README.md(1 hunks)packages/jumpstarter-driver-iscsi/examples/iscsi.py(1 hunks)packages/jumpstarter-driver-iscsi/pyproject.toml(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- packages/jumpstarter-driver-iscsi/README.md
- packages/jumpstarter-driver-iscsi/pyproject.toml
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: bennyz
PR: jumpstarter-dev/jumpstarter#241
File: packages/jumpstarter-driver-tftp/jumpstarter_driver_tftp/client.py:52-60
Timestamp: 2025-01-29T11:52:43.554Z
Learning: The TFTP driver (packages/jumpstarter-driver-tftp/jumpstarter_driver_tftp/driver.py) handles all low-level concerns like path validation, error handling, and checksum computation. The client (packages/jumpstarter-driver-tftp/jumpstarter_driver_tftp/client.py) should remain simple as it delegates these responsibilities to the driver.
Learnt from: bennyz
PR: jumpstarter-dev/jumpstarter#267
File: packages/jumpstarter-driver-tftp/jumpstarter_driver_tftp/driver.py:32-40
Timestamp: 2025-02-08T16:08:34.616Z
Learning: In the TFTP driver (jumpstarter_driver_tftp), the TFTP server component is read-only, while file management operations (upload, delete) are handled through the driver's client interface, not through the TFTP protocol itself.
Learnt from: NickCao
PR: jumpstarter-dev/jumpstarter#463
File: packages/jumpstarter-cli-admin/jumpstarter_cli_admin/get_test.py:270-270
Timestamp: 2025-05-28T15:09:35.768Z
Learning: The jumpstarter CLI is designed to match kubectl's behavior, including returning exit code 0 (success) when no resources are found, rather than exit code 1 (failure).
packages/jumpstarter-driver-iscsi/examples/iscsi.py (2)
Learnt from: bennyz
PR: jumpstarter-dev/jumpstarter#241
File: packages/jumpstarter-driver-tftp/jumpstarter_driver_tftp/client.py:52-60
Timestamp: 2025-01-29T11:52:43.554Z
Learning: The TFTP driver (packages/jumpstarter-driver-tftp/jumpstarter_driver_tftp/driver.py) handles all low-level concerns like path validation, error handling, and checksum computation. The client (packages/jumpstarter-driver-tftp/jumpstarter_driver_tftp/client.py) should remain simple as it delegates these responsibilities to the driver.
Learnt from: mangelajo
PR: jumpstarter-dev/jumpstarter#339
File: packages/jumpstarter-driver-flashers/jumpstarter_driver_flashers/client.py:349-349
Timestamp: 2025-03-14T16:05:14.577Z
Learning: The `_upload_artifact` method in `BaseFlasherClient` class should call `write_from_path` with the calculated filename parameter: `storage.write_from_path(filename, path, operator=operator)` instead of `storage.write_from_path(path, operator=operator)`.
🧬 Code Graph Analysis (1)
packages/jumpstarter-driver-iscsi/examples/iscsi.py (2)
packages/jumpstarter-driver-opendal/jumpstarter_driver_opendal/client.py (1)
operator_for_path(45-57)packages/jumpstarter-driver-iscsi/jumpstarter_driver_iscsi/client.py (8)
start(20-27)add_lun(67-83)get_host(40-47)get_port(49-56)get_target_iqn(58-65)list_luns(97-104)remove_lun(85-95)stop(29-38)
🪛 Pylint (3.3.7)
packages/jumpstarter-driver-iscsi/examples/iscsi.py
[refactor] 18-24: Unnecessary "elif" after "return", remove the leading "el" from "elif"
(R1705)
[refactor] 54-57: Unnecessary "else" after "return", remove the "else" and de-indent the code inside it
(R1705)
[refactor] 70-70: Too many local variables (18/15)
(R0914)
[error] 125-125: No value for argument 'image' in function call
(E1120)
[error] 125-125: No value for argument 'location' in function call
(E1120)
[error] 125-125: No value for argument 'lun_name' in function call
(E1120)
[error] 125-125: No value for argument 'arch' in function call
(E1120)
⏰ Context from checks skipped due to timeout of 90000ms (10)
- GitHub Check: e2e
- GitHub Check: pytest-matrix (ubuntu-24.04, 3.12)
- GitHub Check: pytest-matrix (macos-15, 3.13)
- GitHub Check: pytest-matrix (macos-15, 3.11)
- GitHub Check: pytest-matrix (macos-15, 3.12)
- GitHub Check: pytest-matrix (ubuntu-24.04, 3.13)
- GitHub Check: pytest-matrix (ubuntu-24.04, 3.11)
- GitHub Check: Redirect rules - jumpstarter-docs
- GitHub Check: Header rules - jumpstarter-docs
- GitHub Check: Pages changed - jumpstarter-docs
🔇 Additional comments (2)
packages/jumpstarter-driver-iscsi/examples/iscsi.py (2)
70-122: Main function implementation is well-structured.The CLI implementation correctly integrates with the iSCSI driver client interface and follows the established patterns from the retrieved learnings. The function appropriately delegates to the driver for all iSCSI operations while keeping the example simple and focused.
125-125: Click decorator handles CLI arguments correctly.The static analysis warning about missing function arguments is a false positive - the
@click.command()decorator automatically injects the arguments defined by the@click.optiondecorators.
bennyz
force-pushed
the
iscsi
branch
2 times, most recently
from
b761c39 to
2485c58
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
packages/jumpstarter-driver-iscsi/examples/iscsi.py (3)
22-22: Fix whitespace formatting issue.The blank line contains whitespace which violates formatting standards.
- +
52-58: Remove unnecessary else clause for cleaner code.The
elseclause afterreturnis redundant and can be simplified.def generate_qemu_command(target_arch, host, port, target_iqn): """Generate QEMU command based on architecture""" if target_arch == "aarch64": return f"qemu-system-aarch64 -m 2048 -machine virt -cpu cortex-a72 -drive file=iscsi://{host}:{port}/{target_iqn}/0,format=raw" - else: - return f"qemu-system-x86_64 -m 2048 -drive file=iscsi://{host}:{port}/{target_iqn}/0,format=raw" + return f"qemu-system-x86_64 -m 2048 -drive file=iscsi://{host}:{port}/{target_iqn}/0,format=raw"
70-122: Consider refactoring the main function to reduce complexity.The main function has 18 local variables (exceeding the recommended 15) and handles multiple responsibilities. Consider extracting some logic into separate functions to improve readability and maintainability.
Potential extractions:
- Server setup and configuration logic
- LUN management operations
- Output formatting and display logic
This would make the main function more focused and easier to test.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
packages/jumpstarter-driver-iscsi/README.md(1 hunks)packages/jumpstarter-driver-iscsi/examples/iscsi.py(1 hunks)packages/jumpstarter-driver-iscsi/pyproject.toml(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- packages/jumpstarter-driver-iscsi/README.md
- packages/jumpstarter-driver-iscsi/pyproject.toml
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: bennyz
PR: jumpstarter-dev/jumpstarter#241
File: packages/jumpstarter-driver-tftp/jumpstarter_driver_tftp/client.py:52-60
Timestamp: 2025-01-29T11:52:43.554Z
Learning: The TFTP driver (packages/jumpstarter-driver-tftp/jumpstarter_driver_tftp/driver.py) handles all low-level concerns like path validation, error handling, and checksum computation. The client (packages/jumpstarter-driver-tftp/jumpstarter_driver_tftp/client.py) should remain simple as it delegates these responsibilities to the driver.
Learnt from: bennyz
PR: jumpstarter-dev/jumpstarter#267
File: packages/jumpstarter-driver-tftp/jumpstarter_driver_tftp/driver.py:32-40
Timestamp: 2025-02-08T16:08:34.616Z
Learning: In the TFTP driver (jumpstarter_driver_tftp), the TFTP server component is read-only, while file management operations (upload, delete) are handled through the driver's client interface, not through the TFTP protocol itself.
Learnt from: NickCao
PR: jumpstarter-dev/jumpstarter#463
File: packages/jumpstarter-cli-admin/jumpstarter_cli_admin/get_test.py:270-270
Timestamp: 2025-05-28T15:09:35.768Z
Learning: The jumpstarter CLI is designed to match kubectl's behavior, including returning exit code 0 (success) when no resources are found, rather than exit code 1 (failure).
packages/jumpstarter-driver-iscsi/examples/iscsi.py (2)
Learnt from: bennyz
PR: jumpstarter-dev/jumpstarter#241
File: packages/jumpstarter-driver-tftp/jumpstarter_driver_tftp/client.py:52-60
Timestamp: 2025-01-29T11:52:43.554Z
Learning: The TFTP driver (packages/jumpstarter-driver-tftp/jumpstarter_driver_tftp/driver.py) handles all low-level concerns like path validation, error handling, and checksum computation. The client (packages/jumpstarter-driver-tftp/jumpstarter_driver_tftp/client.py) should remain simple as it delegates these responsibilities to the driver.
Learnt from: mangelajo
PR: jumpstarter-dev/jumpstarter#339
File: packages/jumpstarter-driver-flashers/jumpstarter_driver_flashers/client.py:349-349
Timestamp: 2025-03-14T16:05:14.577Z
Learning: The `_upload_artifact` method in `BaseFlasherClient` class should call `write_from_path` with the calculated filename parameter: `storage.write_from_path(filename, path, operator=operator)` instead of `storage.write_from_path(path, operator=operator)`.
🪛 Ruff (0.11.9)
packages/jumpstarter-driver-iscsi/examples/iscsi.py
22-22: Blank line contains whitespace
Remove whitespace from blank line
(W293)
🪛 GitHub Check: ruff
packages/jumpstarter-driver-iscsi/examples/iscsi.py
[failure] 22-22: Ruff (W293)
packages/jumpstarter-driver-iscsi/examples/iscsi.py:22:1: W293 Blank line contains whitespace
🪛 Pylint (3.3.7)
packages/jumpstarter-driver-iscsi/examples/iscsi.py
[refactor] 54-57: Unnecessary "else" after "return", remove the "else" and de-indent the code inside it
(R1705)
[refactor] 70-70: Too many local variables (18/15)
(R0914)
[error] 125-125: No value for argument 'image' in function call
(E1120)
[error] 125-125: No value for argument 'location' in function call
(E1120)
[error] 125-125: No value for argument 'lun_name' in function call
(E1120)
[error] 125-125: No value for argument 'arch' in function call
(E1120)
🪛 GitHub Actions: Lint
packages/jumpstarter-driver-iscsi/examples/iscsi.py
[warning] 22-22: Ruff: W293 Blank line contains whitespace
⏰ Context from checks skipped due to timeout of 90000ms (7)
- GitHub Check: pytest-matrix (macos-15, 3.12)
- GitHub Check: pytest-matrix (ubuntu-24.04, 3.12)
- GitHub Check: pytest-matrix (ubuntu-24.04, 3.11)
- GitHub Check: pytest-matrix (macos-15, 3.11)
- GitHub Check: pytest-matrix (ubuntu-24.04, 3.13)
- GitHub Check: pytest-matrix (macos-15, 3.13)
- GitHub Check: e2e
🔇 Additional comments (3)
packages/jumpstarter-driver-iscsi/examples/iscsi.py (3)
11-25: LGTM! Architecture detection logic is sound.The function correctly handles auto-detection by checking filename patterns and falling back to system architecture. The implementation covers the main use cases effectively.
27-50: LGTM! Safety measures and streaming approach implemented correctly.The function properly addresses the previous review concerns:
- Block device operations now require user confirmation to prevent accidental data loss
- Uses
write_from_pathfor efficient streaming instead of loading entire files into memory- Clear user feedback through colored output messages
The implementation handles both block device and file storage scenarios appropriately.
124-125: LGTM! Entry point correctly uses Click decorator.The static analysis tool incorrectly flags missing arguments - this is a false positive since the
@click.command()decorator handles argument parsing automatically.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
packages/jumpstarter-driver-iscsi/examples/iscsi.py (1)
11-24: Move platform import to the top of the file.The architecture detection logic is sound, but the platform import should be moved to the top of the file for better organization and to follow Python import conventions.
+import platform import os import clickdef determine_architecture(arch, image): """Determine target architecture from parameter or auto-detect""" if arch != "auto": return arch - import platform - if "aarch64" in image.lower() or "arm64" in image.lower(): return "aarch64" if "x86_64" in image.lower() or "amd64" in image.lower(): return "x86_64"
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
packages/jumpstarter-driver-iscsi/README.md(1 hunks)packages/jumpstarter-driver-iscsi/examples/iscsi.py(1 hunks)packages/jumpstarter-driver-iscsi/pyproject.toml(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- packages/jumpstarter-driver-iscsi/README.md
- packages/jumpstarter-driver-iscsi/pyproject.toml
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: bennyz
PR: jumpstarter-dev/jumpstarter#241
File: packages/jumpstarter-driver-tftp/jumpstarter_driver_tftp/client.py:52-60
Timestamp: 2025-01-29T11:52:43.554Z
Learning: The TFTP driver (packages/jumpstarter-driver-tftp/jumpstarter_driver_tftp/driver.py) handles all low-level concerns like path validation, error handling, and checksum computation. The client (packages/jumpstarter-driver-tftp/jumpstarter_driver_tftp/client.py) should remain simple as it delegates these responsibilities to the driver.
Learnt from: bennyz
PR: jumpstarter-dev/jumpstarter#267
File: packages/jumpstarter-driver-tftp/jumpstarter_driver_tftp/driver.py:32-40
Timestamp: 2025-02-08T16:08:34.616Z
Learning: In the TFTP driver (jumpstarter_driver_tftp), the TFTP server component is read-only, while file management operations (upload, delete) are handled through the driver's client interface, not through the TFTP protocol itself.
Learnt from: NickCao
PR: jumpstarter-dev/jumpstarter#463
File: packages/jumpstarter-cli-admin/jumpstarter_cli_admin/get_test.py:270-270
Timestamp: 2025-05-28T15:09:35.768Z
Learning: The jumpstarter CLI is designed to match kubectl's behavior, including returning exit code 0 (success) when no resources are found, rather than exit code 1 (failure).
packages/jumpstarter-driver-iscsi/examples/iscsi.py (2)
Learnt from: bennyz
PR: jumpstarter-dev/jumpstarter#241
File: packages/jumpstarter-driver-tftp/jumpstarter_driver_tftp/client.py:52-60
Timestamp: 2025-01-29T11:52:43.554Z
Learning: The TFTP driver (packages/jumpstarter-driver-tftp/jumpstarter_driver_tftp/driver.py) handles all low-level concerns like path validation, error handling, and checksum computation. The client (packages/jumpstarter-driver-tftp/jumpstarter_driver_tftp/client.py) should remain simple as it delegates these responsibilities to the driver.
Learnt from: mangelajo
PR: jumpstarter-dev/jumpstarter#339
File: packages/jumpstarter-driver-flashers/jumpstarter_driver_flashers/client.py:349-349
Timestamp: 2025-03-14T16:05:14.577Z
Learning: The `_upload_artifact` method in `BaseFlasherClient` class should call `write_from_path` with the calculated filename parameter: `storage.write_from_path(filename, path, operator=operator)` instead of `storage.write_from_path(path, operator=operator)`.
🪛 Pylint (3.3.7)
packages/jumpstarter-driver-iscsi/examples/iscsi.py
[refactor] 70-70: Too many local variables (18/15)
(R0914)
[error] 125-125: No value for argument 'image' in function call
(E1120)
[error] 125-125: No value for argument 'location' in function call
(E1120)
[error] 125-125: No value for argument 'lun_name' in function call
(E1120)
[error] 125-125: No value for argument 'arch' in function call
(E1120)
⏰ Context from checks skipped due to timeout of 90000ms (10)
- GitHub Check: pytest-matrix (ubuntu-24.04, 3.11)
- GitHub Check: pytest-matrix (macos-15, 3.12)
- GitHub Check: pytest-matrix (ubuntu-24.04, 3.12)
- GitHub Check: pytest-matrix (macos-15, 3.11)
- GitHub Check: pytest-matrix (macos-15, 3.13)
- GitHub Check: pytest-matrix (ubuntu-24.04, 3.13)
- GitHub Check: e2e
- GitHub Check: Redirect rules - jumpstarter-docs
- GitHub Check: Header rules - jumpstarter-docs
- GitHub Check: Pages changed - jumpstarter-docs
🔇 Additional comments (5)
packages/jumpstarter-driver-iscsi/examples/iscsi.py (5)
1-9: LGTM! Clean imports and dependencies.The imports are well-organized and appropriate for the iSCSI example functionality.
27-49: Excellent implementation addressing previous safety concerns.The function properly handles both block devices and regular files with appropriate safety checks. The confirmation prompt for block devices prevents accidental data loss, and the streaming approach using
write_from_pathis efficient for large files.
52-57: LGTM! Appropriate QEMU command generation.The function generates suitable QEMU commands for both supported architectures with appropriate machine types and memory allocation.
60-121: Well-structured CLI implementation with proper resource management.The main function effectively demonstrates the iSCSI driver usage with:
- Proper input validation and error handling
- Context manager for resource cleanup
- Comprehensive workflow from server start to cleanup
- Clear user feedback throughout the process
The static analysis warning about too many local variables is acceptable for a complete example script.
124-125: Standard entry point - static analysis errors are false positives.The entry point follows standard Python conventions. The static analysis errors about missing function arguments are false positives because the
main()function is decorated with@click.command(), which handles argument parsing from the command line.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (8)
packages/jumpstarter-driver-iscsi/examples/exporter.yaml (1)
16-16: Fix YAML formatting issues.The file has formatting issues that should be corrected for better compatibility with YAML parsers.
- port: 3260 + port: 3260Also ensure the file ends with a newline character.
packages/jumpstarter-driver-iscsi/jumpstarter_driver_iscsi/client.py (2)
106-122: Remove unnecessary else after return.The else block is not needed after a return statement.
def _calculate_file_hash(self, file_path: str, operator: Optional[Operator] = None) -> str: """Calculate SHA256 hash of a file""" if operator is None: hash_obj = hashlib.sha256() with open(file_path, "rb") as f: while chunk := f.read(8192): hash_obj.update(chunk) return hash_obj.hexdigest() - else: - from jumpstarter_driver_opendal.client import operator_for_path - - path, op, _ = operator_for_path(file_path) - hash_obj = hashlib.sha256() - with op.open(str(path), "rb") as f: - while chunk := f.read(8192): - hash_obj.update(chunk) - return hash_obj.hexdigest() + + from jumpstarter_driver_opendal.client import operator_for_path + + path, op, _ = operator_for_path(file_path) + hash_obj = hashlib.sha256() + with op.open(str(path), "rb") as f: + while chunk := f.read(8192): + hash_obj.update(chunk) + return hash_obj.hexdigest()
106-193: Consider moving complex logic to the driver.Based on the learnings from the TFTP driver, the client should remain simple and delegate responsibilities like file validation, hashing, and comparison to the driver.
The methods
_calculate_file_hash,_files_are_identical, and the complex logic inupload_imagecould be moved to the driver to maintain consistency with the established pattern where "the driver handles all low-level concerns like path validation, error handling, and checksum computation."Consider refactoring these methods to the driver and having the client simply call driver methods like:
def upload_image(self, dst_name: str, src: PathBuf, size_mb: int = 0, operator: Optional[Operator] = None, force_upload: bool = False) -> str: return self.call("upload_image", dst_name, src, size_mb, operator, force_upload)packages/jumpstarter-driver-iscsi/examples/iscsi.py (1)
70-122: Consider extracting display logic to reduce local variables.The main function has many local variables which makes it harder to follow. Consider extracting the display/output logic into separate functions.
Extract the display logic into helper functions:
def display_server_info(host: str, port: int, target_iqn: str): """Display iSCSI server connection information""" click.secho(f"\niSCSI server running at {host}:{port}", fg="green") click.secho(f"Target IQN: {target_iqn}", fg="green") def display_luns(luns: list): """Display available LUNs""" click.secho("\nAvailable LUNs:", fg="yellow") for lun in luns: click.secho(f" - {lun['name']} ({lun['size'] / (1024 * 1024):.1f}MB)", fg="white") def display_qemu_instructions(arch: str, image: str, host: str, port: int, target_iqn: str): """Display QEMU boot instructions""" click.secho("\nBoot with QEMU:", fg="yellow") target_arch = determine_architecture(arch, image) qemu_cmd = generate_qemu_command(target_arch, host, port, target_iqn) click.secho(f"Architecture: {target_arch}", fg="cyan") click.secho(qemu_cmd, fg="white")Then use these in the main function to reduce complexity.
packages/jumpstarter-driver-iscsi/jumpstarter_driver_iscsi/driver.py (4)
72-80: Consider making the default IP detection more robust.The current implementation connects to Google's DNS (8.8.8.8) which may not work in isolated or airgapped environments.
Consider allowing fallback configuration or using a more generic approach:
def get_default_ip(self): """Get the IP address of the default route interface""" try: with socket.socket(socket.AF_INET, socket.SOCK_DGRAM) as s: - s.connect(("8.8.8.8", 80)) + # Try multiple endpoints in case one is blocked + for endpoint in [("8.8.8.8", 80), ("1.1.1.1", 80), ("10.254.254.254", 80)]: + try: + s.connect(endpoint) + return s.getsockname()[0] + except Exception: + continue - return s.getsockname()[0] except Exception: - self.logger.warning("Could not determine default IP address, falling back to 0.0.0.0") - return "0.0.0.0" + pass + + self.logger.warning("Could not determine default IP address, falling back to 0.0.0.0") + return "0.0.0.0"Or consider using
netifacesor similar library for more reliable network interface detection.
174-176: Clarify the TODO comment about LUN removal.The TODO comment suggests uncertainty about whether to remove LUNs when stopping the server.
Consider the following approaches:
- Keep LUNs (persistent): LUNs remain configured across server restarts, useful for stateful deployments
- Remove LUNs (current): Clean slate on each start, simpler state management
The current implementation removes all LUNs. If you want to support persistent LUNs, you could add a configuration option or parameter to control this behavior. Would you like me to implement a configurable approach?
225-232: Remove unnecessary else after return.The else block is not needed after a return statement.
def _get_full_path(self, file_path: str, is_block: bool) -> str: """Get the full path for the LUN file or block device""" if is_block: if not os.path.isabs(file_path): raise ISCSIError("For block devices, file_path must be an absolute path") return file_path - else: - full_path = os.path.join(self.root_dir, file_path) - os.makedirs(os.path.dirname(full_path), exist_ok=True) - return full_path + + full_path = os.path.join(self.root_dir, file_path) + os.makedirs(os.path.dirname(full_path), exist_ok=True) + return full_path
316-317: Consider validating TPG state earlier.The check for
self._tpgexistence happens after validating the LUN name, but it would be more efficient to check the server state first.def remove_lun(self, name: str): """Remove a LUN from the iSCSI target Args: name (str): Name of the LUN to remove Raises: ISCSIError: If the LUN cannot be removed """ if not self._tpg: raise ISCSIError("iSCSI target not started, call start() first") if name not in self._luns: raise ISCSIError(f"LUN with name {name} does not exist")This pattern should also be applied to the
add_lunmethod for consistency.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
packages/jumpstarter-driver-iscsi/README.md(1 hunks)packages/jumpstarter-driver-iscsi/examples/exporter.yaml(1 hunks)packages/jumpstarter-driver-iscsi/examples/iscsi.py(1 hunks)packages/jumpstarter-driver-iscsi/jumpstarter_driver_iscsi/client.py(1 hunks)packages/jumpstarter-driver-iscsi/jumpstarter_driver_iscsi/driver.py(1 hunks)packages/jumpstarter-driver-iscsi/pyproject.toml(1 hunks)packages/jumpstarter-driver-iscsi/test_iscsi.py(1 hunks)pyproject.toml(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- packages/jumpstarter-driver-iscsi/pyproject.toml
🚧 Files skipped from review as they are similar to previous changes (3)
- packages/jumpstarter-driver-iscsi/README.md
- pyproject.toml
- packages/jumpstarter-driver-iscsi/test_iscsi.py
🧰 Additional context used
🧠 Learnings (4)
📓 Common learnings
Learnt from: bennyz
PR: jumpstarter-dev/jumpstarter#241
File: packages/jumpstarter-driver-tftp/jumpstarter_driver_tftp/client.py:52-60
Timestamp: 2025-01-29T11:52:43.554Z
Learning: The TFTP driver (packages/jumpstarter-driver-tftp/jumpstarter_driver_tftp/driver.py) handles all low-level concerns like path validation, error handling, and checksum computation. The client (packages/jumpstarter-driver-tftp/jumpstarter_driver_tftp/client.py) should remain simple as it delegates these responsibilities to the driver.
Learnt from: bennyz
PR: jumpstarter-dev/jumpstarter#267
File: packages/jumpstarter-driver-tftp/jumpstarter_driver_tftp/driver.py:32-40
Timestamp: 2025-02-08T16:08:34.616Z
Learning: In the TFTP driver (jumpstarter_driver_tftp), the TFTP server component is read-only, while file management operations (upload, delete) are handled through the driver's client interface, not through the TFTP protocol itself.
Learnt from: NickCao
PR: jumpstarter-dev/jumpstarter#463
File: packages/jumpstarter-cli-admin/jumpstarter_cli_admin/get_test.py:270-270
Timestamp: 2025-05-28T15:09:35.768Z
Learning: The jumpstarter CLI is designed to match kubectl's behavior, including returning exit code 0 (success) when no resources are found, rather than exit code 1 (failure).
packages/jumpstarter-driver-iscsi/jumpstarter_driver_iscsi/driver.py (2)
Learnt from: bennyz
PR: jumpstarter-dev/jumpstarter#241
File: packages/jumpstarter-driver-tftp/jumpstarter_driver_tftp/client.py:52-60
Timestamp: 2025-01-29T11:52:43.554Z
Learning: The TFTP driver (packages/jumpstarter-driver-tftp/jumpstarter_driver_tftp/driver.py) handles all low-level concerns like path validation, error handling, and checksum computation. The client (packages/jumpstarter-driver-tftp/jumpstarter_driver_tftp/client.py) should remain simple as it delegates these responsibilities to the driver.
Learnt from: bennyz
PR: jumpstarter-dev/jumpstarter#267
File: packages/jumpstarter-driver-tftp/jumpstarter_driver_tftp/driver.py:32-40
Timestamp: 2025-02-08T16:08:34.616Z
Learning: In the TFTP driver (jumpstarter_driver_tftp), the TFTP server component is read-only, while file management operations (upload, delete) are handled through the driver's client interface, not through the TFTP protocol itself.
packages/jumpstarter-driver-iscsi/examples/iscsi.py (2)
Learnt from: bennyz
PR: jumpstarter-dev/jumpstarter#241
File: packages/jumpstarter-driver-tftp/jumpstarter_driver_tftp/client.py:52-60
Timestamp: 2025-01-29T11:52:43.554Z
Learning: The TFTP driver (packages/jumpstarter-driver-tftp/jumpstarter_driver_tftp/driver.py) handles all low-level concerns like path validation, error handling, and checksum computation. The client (packages/jumpstarter-driver-tftp/jumpstarter_driver_tftp/client.py) should remain simple as it delegates these responsibilities to the driver.
Learnt from: mangelajo
PR: jumpstarter-dev/jumpstarter#339
File: packages/jumpstarter-driver-flashers/jumpstarter_driver_flashers/client.py:349-349
Timestamp: 2025-03-14T16:05:14.577Z
Learning: The `_upload_artifact` method in `BaseFlasherClient` class should call `write_from_path` with the calculated filename parameter: `storage.write_from_path(filename, path, operator=operator)` instead of `storage.write_from_path(path, operator=operator)`.
packages/jumpstarter-driver-iscsi/jumpstarter_driver_iscsi/client.py (2)
Learnt from: bennyz
PR: jumpstarter-dev/jumpstarter#241
File: packages/jumpstarter-driver-tftp/jumpstarter_driver_tftp/client.py:52-60
Timestamp: 2025-01-29T11:52:43.554Z
Learning: The TFTP driver (packages/jumpstarter-driver-tftp/jumpstarter_driver_tftp/driver.py) handles all low-level concerns like path validation, error handling, and checksum computation. The client (packages/jumpstarter-driver-tftp/jumpstarter_driver_tftp/client.py) should remain simple as it delegates these responsibilities to the driver.
Learnt from: bennyz
PR: jumpstarter-dev/jumpstarter#267
File: packages/jumpstarter-driver-tftp/jumpstarter_driver_tftp/driver.py:32-40
Timestamp: 2025-02-08T16:08:34.616Z
Learning: In the TFTP driver (jumpstarter_driver_tftp), the TFTP server component is read-only, while file management operations (upload, delete) are handled through the driver's client interface, not through the TFTP protocol itself.
🧬 Code Graph Analysis (2)
packages/jumpstarter-driver-iscsi/jumpstarter_driver_iscsi/driver.py (3)
packages/jumpstarter-driver-opendal/jumpstarter_driver_opendal/driver.py (1)
Opendal(21-203)packages/jumpstarter/jumpstarter/driver/base.py (1)
Driver(56-269)packages/jumpstarter-driver-iscsi/jumpstarter_driver_iscsi/client.py (8)
start(20-27)stop(29-38)remove_lun(85-95)get_host(40-47)get_port(49-56)get_target_iqn(58-65)add_lun(67-83)list_luns(97-104)
packages/jumpstarter-driver-iscsi/jumpstarter_driver_iscsi/client.py (4)
packages/jumpstarter-driver-iscsi/jumpstarter_driver_iscsi/driver.py (9)
client(83-84)start(153-165)stop(168-185)get_host(188-194)get_port(197-203)get_target_iqn(206-212)add_lun(267-304)remove_lun(307-332)list_luns(335-352)packages/jumpstarter-driver-composite/jumpstarter_driver_composite/client.py (1)
CompositeClient(9-28)packages/jumpstarter/jumpstarter/client/base.py (1)
call(36-46)packages/jumpstarter-driver-opendal/jumpstarter_driver_opendal/client.py (1)
operator_for_path(46-58)
🪛 Pylint (3.3.7)
packages/jumpstarter-driver-iscsi/jumpstarter_driver_iscsi/driver.py
[refactor] 32-32: Too many instance attributes (13/7)
(R0902)
[refactor] 225-232: Unnecessary "else" after "return", remove the "else" and de-indent the code inside it
(R1705)
packages/jumpstarter-driver-iscsi/examples/iscsi.py
[refactor] 70-70: Too many local variables (18/15)
(R0914)
[error] 125-125: No value for argument 'image' in function call
(E1120)
[error] 125-125: No value for argument 'location' in function call
(E1120)
[error] 125-125: No value for argument 'lun_name' in function call
(E1120)
[error] 125-125: No value for argument 'arch' in function call
(E1120)
packages/jumpstarter-driver-iscsi/jumpstarter_driver_iscsi/client.py
[refactor] 108-122: Unnecessary "else" after "return", remove the "else" and de-indent the code inside it
(R1705)
[refactor] 152-152: Too many arguments (6/5)
(R0913)
[refactor] 152-152: Too many positional arguments (6/5)
(R0917)
🪛 YAMLlint (1.37.1)
packages/jumpstarter-driver-iscsi/examples/exporter.yaml
[error] 16-16: no new line character at the end of file
(new-line-at-end-of-file)
[error] 16-16: trailing spaces
(trailing-spaces)
⏰ Context from checks skipped due to timeout of 90000ms (10)
- GitHub Check: pytest-matrix (macos-15, 3.12)
- GitHub Check: pytest-matrix (ubuntu-24.04, 3.12)
- GitHub Check: pytest-matrix (macos-15, 3.13)
- GitHub Check: pytest-matrix (ubuntu-24.04, 3.13)
- GitHub Check: pytest-matrix (ubuntu-24.04, 3.11)
- GitHub Check: pytest-matrix (macos-15, 3.11)
- GitHub Check: e2e
- GitHub Check: Redirect rules - jumpstarter-docs
- GitHub Check: Header rules - jumpstarter-docs
- GitHub Check: Pages changed - jumpstarter-docs
| dst_path = f"{dst_name}.img" | ||
|
|
||
| if not force_upload and self._files_are_identical(src, dst_path, operator): | ||
| print(f"File {dst_path} already exists and is identical to source. Skipping upload.") |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Avoid direct stdout printing in library code.
Using print() directly in library code is not recommended as it doesn't allow users to control logging behavior.
Consider using a logger or returning status information that the caller can handle:
- if not force_upload and self._files_are_identical(src, dst_path, operator):
- print(f"File {dst_path} already exists and is identical to source. Skipping upload.")
- else:
- print(f"Uploading {src} to {dst_path}...")
- self.storage.write_from_path(dst_path, src, operator)
+ if not force_upload and self._files_are_identical(src, dst_path, operator):
+ # Use logger if available or let caller handle the status
+ # self.logger.info(f"File {dst_path} already exists and is identical to source. Skipping upload.")
+ pass
+ else:
+ # self.logger.info(f"Uploading {src} to {dst_path}...")
+ self.storage.write_from_path(dst_path, src, operator)Also applies to: 182-182
🤖 Prompt for AI Agents
In packages/jumpstarter-driver-iscsi/jumpstarter_driver_iscsi/client.py at lines
180 and 182, replace the direct print() calls with proper logging using a logger
instance. Initialize a logger for the module if not already present, and use it
to log the messages at an appropriate level (e.g., info or debug) so that users
can control output via logging configuration instead of stdout printing.
| src_path = os.path.join(self.storage._storage.root_dir, dst_path) | ||
| size_mb = os.path.getsize(src_path) // (1024 * 1024) |
There was a problem hiding this comment.
Avoid accessing internal storage implementation details.
Accessing self.storage._storage.root_dir breaks encapsulation by reaching into the internal implementation of the storage object.
Consider adding a proper API method to get the root directory or use the stat method to determine file size:
- src_path = os.path.join(self.storage._storage.root_dir, dst_path)
- size_mb = os.path.getsize(src_path) // (1024 * 1024)
+ stat_info = self.storage.stat(dst_path)
+ size_mb = stat_info.content_length // (1024 * 1024)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| src_path = os.path.join(self.storage._storage.root_dir, dst_path) | |
| size_mb = os.path.getsize(src_path) // (1024 * 1024) | |
| stat_info = self.storage.stat(dst_path) | |
| size_mb = stat_info.content_length // (1024 * 1024) |
🤖 Prompt for AI Agents
In packages/jumpstarter-driver-iscsi/jumpstarter_driver_iscsi/client.py around
lines 186 to 187, avoid accessing the internal attribute
self.storage._storage.root_dir directly as it breaks encapsulation. Instead,
refactor the code to use a public API method of the storage object to retrieve
the root directory or use the storage's stat method to get the file size. If
such a method does not exist, add a proper accessor method to the storage class
to expose the root directory safely.
| @@ -0,0 +1,3 @@ | |||
| # Jumpstarter Driver for iSCSI | |||
|
|
|||
| Simple iSCSI server driver for Jumpstarter | |||
There was a problem hiding this comment.
We need detailed docs, cursor is awesome at getting this started if you give it the context of the driver.py and a good example (like pyserial docs)
|
Check the new bot comments, and try to squash or fix the commit messages "wip, ruff, etc.." :) |
yeah, once it's closer to being merged, i had some bad experiences with force pushing 😅 |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
packages/jumpstarter-driver-iscsi/jumpstarter_driver_iscsi/driver.py (1)
223-233: Simplify path logic by removing unnecessary else clause.The else clause after the return statement is unnecessary and can be simplified.
def _get_full_path(self, file_path: str, is_block: bool) -> str: """Get the full path for the LUN file or block device""" if is_block: if not os.path.isabs(file_path): raise ISCSIError("For block devices, file_path must be an absolute path") return file_path - else: - full_path = os.path.join(self.root_dir, file_path) - os.makedirs(os.path.dirname(full_path), exist_ok=True) - return full_path + + full_path = os.path.join(self.root_dir, file_path) + os.makedirs(os.path.dirname(full_path), exist_ok=True) + return full_path
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
packages/jumpstarter-driver-iscsi/README.md(1 hunks)packages/jumpstarter-driver-iscsi/jumpstarter_driver_iscsi/driver.py(1 hunks)
🧰 Additional context used
🧠 Learnings (3)
📓 Common learnings
Learnt from: bennyz
PR: jumpstarter-dev/jumpstarter#241
File: packages/jumpstarter-driver-tftp/jumpstarter_driver_tftp/client.py:52-60
Timestamp: 2025-01-29T11:52:43.554Z
Learning: The TFTP driver (packages/jumpstarter-driver-tftp/jumpstarter_driver_tftp/driver.py) handles all low-level concerns like path validation, error handling, and checksum computation. The client (packages/jumpstarter-driver-tftp/jumpstarter_driver_tftp/client.py) should remain simple as it delegates these responsibilities to the driver.
Learnt from: bennyz
PR: jumpstarter-dev/jumpstarter#267
File: packages/jumpstarter-driver-tftp/jumpstarter_driver_tftp/driver.py:32-40
Timestamp: 2025-02-08T16:08:34.616Z
Learning: In the TFTP driver (jumpstarter_driver_tftp), the TFTP server component is read-only, while file management operations (upload, delete) are handled through the driver's client interface, not through the TFTP protocol itself.
Learnt from: NickCao
PR: jumpstarter-dev/jumpstarter#463
File: packages/jumpstarter-cli-admin/jumpstarter_cli_admin/get_test.py:270-270
Timestamp: 2025-05-28T15:09:35.768Z
Learning: The jumpstarter CLI is designed to match kubectl's behavior, including returning exit code 0 (success) when no resources are found, rather than exit code 1 (failure).
packages/jumpstarter-driver-iscsi/README.md (2)
Learnt from: bennyz
PR: jumpstarter-dev/jumpstarter#241
File: packages/jumpstarter-driver-tftp/jumpstarter_driver_tftp/client.py:52-60
Timestamp: 2025-01-29T11:52:43.554Z
Learning: The TFTP driver (packages/jumpstarter-driver-tftp/jumpstarter_driver_tftp/driver.py) handles all low-level concerns like path validation, error handling, and checksum computation. The client (packages/jumpstarter-driver-tftp/jumpstarter_driver_tftp/client.py) should remain simple as it delegates these responsibilities to the driver.
Learnt from: bennyz
PR: jumpstarter-dev/jumpstarter#267
File: packages/jumpstarter-driver-tftp/jumpstarter_driver_tftp/driver.py:32-40
Timestamp: 2025-02-08T16:08:34.616Z
Learning: In the TFTP driver (jumpstarter_driver_tftp), the TFTP server component is read-only, while file management operations (upload, delete) are handled through the driver's client interface, not through the TFTP protocol itself.
packages/jumpstarter-driver-iscsi/jumpstarter_driver_iscsi/driver.py (2)
Learnt from: bennyz
PR: jumpstarter-dev/jumpstarter#241
File: packages/jumpstarter-driver-tftp/jumpstarter_driver_tftp/client.py:52-60
Timestamp: 2025-01-29T11:52:43.554Z
Learning: The TFTP driver (packages/jumpstarter-driver-tftp/jumpstarter_driver_tftp/driver.py) handles all low-level concerns like path validation, error handling, and checksum computation. The client (packages/jumpstarter-driver-tftp/jumpstarter_driver_tftp/client.py) should remain simple as it delegates these responsibilities to the driver.
Learnt from: bennyz
PR: jumpstarter-dev/jumpstarter#267
File: packages/jumpstarter-driver-tftp/jumpstarter_driver_tftp/driver.py:32-40
Timestamp: 2025-02-08T16:08:34.616Z
Learning: In the TFTP driver (jumpstarter_driver_tftp), the TFTP server component is read-only, while file management operations (upload, delete) are handled through the driver's client interface, not through the TFTP protocol itself.
🪛 LanguageTool
packages/jumpstarter-driver-iscsi/README.md
[uncategorized] ~8-~8: Possible missing comma found.
Context: ..._** (server). To access the > exported LUNs you still need a separate iSCSI **initi...
(AI_HYDRA_LEO_MISSING_COMMA)
[uncategorized] ~15-~15: Possible missing comma found.
Context: ...-fb` relies on the in-kernel LIO target framework which is packaged differently by each d...
(AI_HYDRA_LEO_MISSING_COMMA)
🪛 markdownlint-cli2 (0.17.2)
packages/jumpstarter-driver-iscsi/README.md
22-22: Dollar signs used before commands without showing output
(MD014, commands-show-output)
54-54: Emphasis style
Expected: underscore; Actual: asterisk
(MD049, emphasis-style)
54-54: Emphasis style
Expected: underscore; Actual: asterisk
(MD049, emphasis-style)
🪛 Ruff (0.11.9)
packages/jumpstarter-driver-iscsi/jumpstarter_driver_iscsi/driver.py
3-3: contextlib.contextmanager imported but unused
Remove unused import: contextlib.contextmanager
(F401)
🪛 GitHub Check: ruff
packages/jumpstarter-driver-iscsi/jumpstarter_driver_iscsi/driver.py
[failure] 3-3: Ruff (F401)
packages/jumpstarter-driver-iscsi/jumpstarter_driver_iscsi/driver.py:3:24: F401 contextlib.contextmanager imported but unused
🪛 Flake8 (7.2.0)
packages/jumpstarter-driver-iscsi/jumpstarter_driver_iscsi/driver.py
[error] 3-3: 'contextlib.contextmanager' imported but unused
(F401)
🪛 Pylint (3.3.7)
packages/jumpstarter-driver-iscsi/jumpstarter_driver_iscsi/driver.py
[refactor] 32-32: Too many instance attributes (13/7)
(R0902)
[refactor] 225-232: Unnecessary "else" after "return", remove the "else" and de-indent the code inside it
(R1705)
🪛 GitHub Actions: Lint
packages/jumpstarter-driver-iscsi/jumpstarter_driver_iscsi/driver.py
[error] 3-3: Ruff: contextlib.contextmanager imported but unused (F401)
⏰ Context from checks skipped due to timeout of 90000ms (10)
- GitHub Check: Redirect rules - jumpstarter-docs
- GitHub Check: Header rules - jumpstarter-docs
- GitHub Check: pytest-matrix (macos-15, 3.12)
- GitHub Check: pytest-matrix (ubuntu-24.04, 3.11)
- GitHub Check: pytest-matrix (macos-15, 3.11)
- GitHub Check: pytest-matrix (ubuntu-24.04, 3.13)
- GitHub Check: pytest-matrix (macos-15, 3.13)
- GitHub Check: pytest-matrix (ubuntu-24.04, 3.12)
- GitHub Check: e2e
- GitHub Check: Pages changed - jumpstarter-docs
🔇 Additional comments (18)
packages/jumpstarter-driver-iscsi/jumpstarter_driver_iscsi/driver.py (17)
13-28: Well-structured exception hierarchy.The custom exception classes provide clear categorization for different types of iSCSI-related errors, which will help with debugging and error handling.
31-56: Comprehensive driver configuration with proper dataclass usage.The dataclass structure with field definitions is well-organized. The number of instance attributes is justified for an iSCSI driver that needs to manage targets, portals, LUNs, and storage objects.
58-71: Solid initialization logic with proper dependency setup.The
__post_init__method correctly creates the storage directory, sets up the Opendal child driver, and handles IP address resolution. The integration with the Opendal driver follows the established pattern from other Jumpstarter drivers.
72-81: Robust default IP detection with appropriate fallback.The method uses a common technique to determine the default route interface IP and includes proper error handling with a sensible fallback to
0.0.0.0.
86-100: Clear target configuration with comprehensive error handling.The method properly orchestrates the target setup process and includes appropriate error logging. The comment about the former context manager approach provides helpful historical context.
101-127: Thorough target setup with reuse logic.The method correctly handles both existing and new target creation scenarios, which is important for driver restart scenarios. The error handling around existing target checks is appropriate.
130-145: Portal management with duplicate prevention.The network portal setup correctly checks for existing portals to avoid conflicts, which is essential for proper iSCSI operation.
146-151: Appropriate TPG attribute configuration.The TPG attributes are configured correctly for a test/development environment, disabling authentication and enabling automatic node ACL generation.
152-166: Clean start method with proper error wrapping.The start method provides a clean interface and properly wraps underlying exceptions in the custom ISCSIError type.
167-186: Comprehensive cleanup in stop method.The stop method properly removes all LUNs and cleans up the target. The TODO comment indicates awareness of potential policy decisions around LUN persistence.
187-213: Simple getter methods with proper export decoration.These accessor methods provide clean interfaces for retrieving server configuration details.
214-222: Proper input validation with clear error messages.The validation method correctly checks for duplicate names and validates the size parameter with appropriate error messages.
234-265: Comprehensive file storage object creation with size handling.The method handles all the complex scenarios around file creation, extension, and size validation. The logging provides good visibility into what operations are being performed.
266-305: Well-structured LUN addition with proper error handling.The add_lun method correctly validates inputs, handles both file and block device scenarios, and provides comprehensive error handling and logging.
306-333: Proper LUN removal with state validation.The remove_lun method correctly checks for server state and LUN existence before attempting removal, with appropriate cleanup of both LUN and storage objects.
334-353: Comprehensive LUN listing with detailed information.The list_luns method provides all relevant information about configured LUNs, including distinguishing between file and block storage types.
354-361: Proper cleanup method with error handling.The close method ensures the server is stopped during cleanup and properly calls the parent cleanup method.
packages/jumpstarter-driver-iscsi/README.md (1)
1-92: Excellent comprehensive documentation that addresses previous feedback.This README provides detailed documentation covering installation, configuration, API reference, and usage examples. It effectively addresses the previous review comment requesting detailed documentation and follows the pattern of other Jumpstarter driver documentation.
The documentation includes:
- Clear installation instructions with distribution-specific commands
- Comprehensive configuration parameter table
- API reference integration with Sphinx
- Practical usage examples with code blocks
- Proper warnings about the target vs. initiator distinction
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
packages/jumpstarter-driver-iscsi/jumpstarter_driver_iscsi/driver.py (1)
224-231: Remove unnecessary else clause after return.The
elseclause is unnecessary since the previous block returns early.def _get_full_path(self, file_path: str, is_block: bool) -> str: """Get the full path for the LUN file or block device""" if is_block: if not os.path.isabs(file_path): raise ISCSIError("For block devices, file_path must be an absolute path") return file_path - else: - full_path = os.path.join(self.root_dir, file_path) - os.makedirs(os.path.dirname(full_path), exist_ok=True) - return full_path + full_path = os.path.join(self.root_dir, file_path) + os.makedirs(os.path.dirname(full_path), exist_ok=True) + return full_pathpackages/jumpstarter-driver-iscsi/README.md (2)
8-8: Minor: Consider adding comma for clarity.For improved readability, consider adding a comma after "exported LUNs".
-> exported LUNs you still need a separate iSCSI **initiator** (client) on the +> exported LUNs, you still need a separate iSCSI **initiator** (client) on the
54-54: Minor: Use consistent emphasis style.Consider using underscores instead of asterisks for emphasis to maintain consistency with the rest of the documentation.
-| `host` | IP address to bind the target to. Empty string will auto-detect default IP. | str | no | *auto* | +| `host` | IP address to bind the target to. Empty string will auto-detect default IP. | str | no | _auto_ |
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
packages/jumpstarter-driver-iscsi/README.md(1 hunks)packages/jumpstarter-driver-iscsi/jumpstarter_driver_iscsi/driver.py(1 hunks)
🧰 Additional context used
🧠 Learnings (3)
📓 Common learnings
Learnt from: bennyz
PR: jumpstarter-dev/jumpstarter#241
File: packages/jumpstarter-driver-tftp/jumpstarter_driver_tftp/client.py:52-60
Timestamp: 2025-01-29T11:52:43.554Z
Learning: The TFTP driver (packages/jumpstarter-driver-tftp/jumpstarter_driver_tftp/driver.py) handles all low-level concerns like path validation, error handling, and checksum computation. The client (packages/jumpstarter-driver-tftp/jumpstarter_driver_tftp/client.py) should remain simple as it delegates these responsibilities to the driver.
Learnt from: bennyz
PR: jumpstarter-dev/jumpstarter#267
File: packages/jumpstarter-driver-tftp/jumpstarter_driver_tftp/driver.py:32-40
Timestamp: 2025-02-08T16:08:34.616Z
Learning: In the TFTP driver (jumpstarter_driver_tftp), the TFTP server component is read-only, while file management operations (upload, delete) are handled through the driver's client interface, not through the TFTP protocol itself.
Learnt from: NickCao
PR: jumpstarter-dev/jumpstarter#463
File: packages/jumpstarter-cli-admin/jumpstarter_cli_admin/get_test.py:270-270
Timestamp: 2025-05-28T15:09:35.768Z
Learning: The jumpstarter CLI is designed to match kubectl's behavior, including returning exit code 0 (success) when no resources are found, rather than exit code 1 (failure).
packages/jumpstarter-driver-iscsi/README.md (2)
Learnt from: bennyz
PR: jumpstarter-dev/jumpstarter#241
File: packages/jumpstarter-driver-tftp/jumpstarter_driver_tftp/client.py:52-60
Timestamp: 2025-01-29T11:52:43.554Z
Learning: The TFTP driver (packages/jumpstarter-driver-tftp/jumpstarter_driver_tftp/driver.py) handles all low-level concerns like path validation, error handling, and checksum computation. The client (packages/jumpstarter-driver-tftp/jumpstarter_driver_tftp/client.py) should remain simple as it delegates these responsibilities to the driver.
Learnt from: bennyz
PR: jumpstarter-dev/jumpstarter#267
File: packages/jumpstarter-driver-tftp/jumpstarter_driver_tftp/driver.py:32-40
Timestamp: 2025-02-08T16:08:34.616Z
Learning: In the TFTP driver (jumpstarter_driver_tftp), the TFTP server component is read-only, while file management operations (upload, delete) are handled through the driver's client interface, not through the TFTP protocol itself.
packages/jumpstarter-driver-iscsi/jumpstarter_driver_iscsi/driver.py (2)
Learnt from: bennyz
PR: jumpstarter-dev/jumpstarter#241
File: packages/jumpstarter-driver-tftp/jumpstarter_driver_tftp/client.py:52-60
Timestamp: 2025-01-29T11:52:43.554Z
Learning: The TFTP driver (packages/jumpstarter-driver-tftp/jumpstarter_driver_tftp/driver.py) handles all low-level concerns like path validation, error handling, and checksum computation. The client (packages/jumpstarter-driver-tftp/jumpstarter_driver_tftp/client.py) should remain simple as it delegates these responsibilities to the driver.
Learnt from: bennyz
PR: jumpstarter-dev/jumpstarter#267
File: packages/jumpstarter-driver-tftp/jumpstarter_driver_tftp/driver.py:32-40
Timestamp: 2025-02-08T16:08:34.616Z
Learning: In the TFTP driver (jumpstarter_driver_tftp), the TFTP server component is read-only, while file management operations (upload, delete) are handled through the driver's client interface, not through the TFTP protocol itself.
🪛 LanguageTool
packages/jumpstarter-driver-iscsi/README.md
[uncategorized] ~8-~8: Possible missing comma found.
Context: ..._** (server). To access the > exported LUNs you still need a separate iSCSI **initi...
(AI_HYDRA_LEO_MISSING_COMMA)
[uncategorized] ~15-~15: Possible missing comma found.
Context: ...-fb` relies on the in-kernel LIO target framework which is packaged differently by each d...
(AI_HYDRA_LEO_MISSING_COMMA)
🪛 markdownlint-cli2 (0.17.2)
packages/jumpstarter-driver-iscsi/README.md
22-22: Dollar signs used before commands without showing output
(MD014, commands-show-output)
54-54: Emphasis style
Expected: underscore; Actual: asterisk
(MD049, emphasis-style)
54-54: Emphasis style
Expected: underscore; Actual: asterisk
(MD049, emphasis-style)
🪛 Pylint (3.3.7)
packages/jumpstarter-driver-iscsi/jumpstarter_driver_iscsi/driver.py
[refactor] 31-31: Too many instance attributes (13/7)
(R0902)
[refactor] 224-231: Unnecessary "else" after "return", remove the "else" and de-indent the code inside it
(R1705)
⏰ Context from checks skipped due to timeout of 90000ms (9)
- GitHub Check: Redirect rules - jumpstarter-docs
- GitHub Check: Header rules - jumpstarter-docs
- GitHub Check: pytest-matrix (macos-15, 3.12)
- GitHub Check: pytest-matrix (ubuntu-24.04, 3.11)
- GitHub Check: pytest-matrix (macos-15, 3.13)
- GitHub Check: pytest-matrix (ubuntu-24.04, 3.12)
- GitHub Check: pytest-matrix (ubuntu-24.04, 3.13)
- GitHub Check: e2e
- GitHub Check: Pages changed - jumpstarter-docs
🔇 Additional comments (10)
packages/jumpstarter-driver-iscsi/jumpstarter_driver_iscsi/driver.py (9)
1-10: LGTM! Clean import structure.The imports are well-organized and all appear to be used appropriately in the implementation.
30-56: Well-designed dataclass structure with comprehensive configuration options.The driver configuration is well thought out with sensible defaults and proper field definitions. The private fields for internal state management are appropriately marked with
init=False.
57-69: Solid initialization logic with proper dependency setup.The
__post_init__method correctly handles directory creation, storage initialization, and automatic IP detection. The integration with the Opendal storage driver follows the established pattern from other Jumpstarter drivers.
71-79: Robust default IP detection with appropriate fallback.The method uses a clever technique to determine the default route interface IP and includes proper error handling with a sensible fallback to
0.0.0.0.
85-150: Comprehensive target configuration with proper error handling.The target setup logic is well-structured, handling both existing and new target creation scenarios. The separation into helper methods improves readability and maintainability.
174-175: Consider the TODO comment for LUN cleanup strategy.The TODO comment suggests uncertainty about whether LUNs should be automatically removed when stopping the server. This could impact data persistence expectations.
Should LUNs be preserved when stopping the server to maintain data persistence, or is automatic cleanup the desired behavior? Please clarify the intended lifecycle management strategy.
265-303: Excellent LUN management with comprehensive validation and error handling.The
add_lunmethod properly handles both file-backed and block device scenarios, with thorough input validation and appropriate error messages. The file creation and extension logic is particularly well-implemented.
305-331: Proper LUN removal with cleanup and validation.The
remove_lunmethod includes appropriate state validation and proper cleanup of both LUN and storage objects.
333-351: Comprehensive LUN information retrieval.The
list_lunsmethod provides all necessary information about configured LUNs, including proper type detection for block vs file-backed storage.packages/jumpstarter-driver-iscsi/README.md (1)
1-91: Excellent comprehensive documentation addressing previous feedback.This README significantly addresses the previous comment about needing detailed documentation. It provides:
- Clear installation instructions with distribution-specific examples
- Comprehensive configuration parameters table
- API reference with method documentation
- Practical usage examples with code samples
The documentation structure and content quality are excellent.
|
|
||
| ### Start a server and export a raw image | ||
| ```{testcode} | ||
| from jumpstarter_driver_iscsi.driver import ISCSI |
There was a problem hiding this comment.
I would not use the testcode as in the other patch.
| iscsi = instance.__enter__() | ||
|
|
||
| # Upload an existing QCOW2 image and expose it as LUN 0. | ||
| iscsi.upload_image("demo", "./disk.qcow2") |
There was a problem hiding this comment.
Why do we need the "demo" name? isn't it a parameter in the driver config?
|
|
||
| def _get_full_path(self, file_path: str, is_block: bool) -> str: | ||
| """Get the full path for the LUN file or block device""" | ||
| if is_block: |
There was a problem hiding this comment.
We should make sure that anything requested is always under the configured root directory, and that no path traversal is used "../../ etc"
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (5)
packages/jumpstarter-driver-iscsi/jumpstarter_driver_iscsi/driver.py (2)
85-99: Consider adding more specific error handling for target configuration.While the try-catch block provides general error handling, consider catching more specific exceptions from the rtslib_fb library operations to provide better error messages and recovery options.
def _configure_target(self): """Helper that configures the target; formerly a context-manager but the implicit enter/exit semantics were confusing and the driver never needed teardown at this point. """ try: self._rtsroot = RTSRoot() self._setup_target() self._setup_network_portal() self._configure_tpg_attributes() - except Exception as e: + except (OSError, RuntimeError, ValueError) as e: self.logger.error(f"Error in iSCSI target configuration: {e}") - raise + raise ISCSIError(f"Target configuration failed: {e}") from e + except Exception as e: + self.logger.error(f"Unexpected error in iSCSI target configuration: {e}") + raise ISCSIError(f"Target configuration failed: {e}") from e
174-175: Consider making LUN removal on stop configurable.The TODO comment suggests uncertainty about whether LUNs should be removed on server stop. Consider making this behavior configurable through a parameter like
remove_luns_on_stop: bool = True.@dataclass(kw_only=True) class ISCSI(Driver): # ... existing fields ... + remove_luns_on_stop: bool = TrueThen update the stop method:
def stop(self): """Stop the iSCSI target server""" try: - for name in list(self._luns.keys()): - # TODO: maybe leave? - self.remove_lun(name) + if self.remove_luns_on_stop: + for name in list(self._luns.keys()): + self.remove_lun(name)packages/jumpstarter-driver-iscsi/examples/exporter.yaml (1)
16-16: Fix YAML formatting issues.The static analysis tools identified trailing spaces and missing newline at the end of the file.
- port: 3260 + port: 3260 +packages/jumpstarter-driver-iscsi/README.md (1)
54-54: Minor markdown formatting consistency.The static analysis suggests using underscore emphasis style for consistency. This is a minor stylistic preference.
-| `host` | IP address to bind the target to. Empty string will auto-detect default IP. | str | no | *auto* | +| `host` | IP address to bind the target to. Empty string will auto-detect default IP. | str | no | _auto_ |docs/source/reference/package-apis/drivers/iscsi.md (1)
54-54: Minor markdown formatting consistency.Same formatting suggestion as in the README for consistency across documentation.
-| `host` | IP address to bind the target to. Empty string will auto-detect default IP. | str | no | *auto* | +| `host` | IP address to bind the target to. Empty string will auto-detect default IP. | str | no | _auto_ |
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
docs/source/reference/package-apis/drivers/index.md(1 hunks)docs/source/reference/package-apis/drivers/iscsi.md(1 hunks)packages/jumpstarter-driver-iscsi/README.md(1 hunks)packages/jumpstarter-driver-iscsi/examples/exporter.yaml(1 hunks)packages/jumpstarter-driver-iscsi/examples/iscsi.py(1 hunks)packages/jumpstarter-driver-iscsi/jumpstarter_driver_iscsi/client.py(1 hunks)packages/jumpstarter-driver-iscsi/jumpstarter_driver_iscsi/driver.py(1 hunks)packages/jumpstarter-driver-iscsi/pyproject.toml(1 hunks)pyproject.toml(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- docs/source/reference/package-apis/drivers/index.md
🚧 Files skipped from review as they are similar to previous changes (4)
- pyproject.toml
- packages/jumpstarter-driver-iscsi/pyproject.toml
- packages/jumpstarter-driver-iscsi/jumpstarter_driver_iscsi/client.py
- packages/jumpstarter-driver-iscsi/examples/iscsi.py
🧰 Additional context used
🧠 Learnings (4)
📓 Common learnings
Learnt from: bennyz
PR: jumpstarter-dev/jumpstarter#241
File: packages/jumpstarter-driver-tftp/jumpstarter_driver_tftp/client.py:52-60
Timestamp: 2025-01-29T11:52:43.554Z
Learning: The TFTP driver (packages/jumpstarter-driver-tftp/jumpstarter_driver_tftp/driver.py) handles all low-level concerns like path validation, error handling, and checksum computation. The client (packages/jumpstarter-driver-tftp/jumpstarter_driver_tftp/client.py) should remain simple as it delegates these responsibilities to the driver.
Learnt from: bennyz
PR: jumpstarter-dev/jumpstarter#267
File: packages/jumpstarter-driver-tftp/jumpstarter_driver_tftp/driver.py:32-40
Timestamp: 2025-02-08T16:08:34.616Z
Learning: In the TFTP driver (jumpstarter_driver_tftp), the TFTP server component is read-only, while file management operations (upload, delete) are handled through the driver's client interface, not through the TFTP protocol itself.
packages/jumpstarter-driver-iscsi/jumpstarter_driver_iscsi/driver.py (2)
Learnt from: bennyz
PR: jumpstarter-dev/jumpstarter#241
File: packages/jumpstarter-driver-tftp/jumpstarter_driver_tftp/client.py:52-60
Timestamp: 2025-01-29T11:52:43.554Z
Learning: The TFTP driver (packages/jumpstarter-driver-tftp/jumpstarter_driver_tftp/driver.py) handles all low-level concerns like path validation, error handling, and checksum computation. The client (packages/jumpstarter-driver-tftp/jumpstarter_driver_tftp/client.py) should remain simple as it delegates these responsibilities to the driver.
Learnt from: bennyz
PR: jumpstarter-dev/jumpstarter#267
File: packages/jumpstarter-driver-tftp/jumpstarter_driver_tftp/driver.py:32-40
Timestamp: 2025-02-08T16:08:34.616Z
Learning: In the TFTP driver (jumpstarter_driver_tftp), the TFTP server component is read-only, while file management operations (upload, delete) are handled through the driver's client interface, not through the TFTP protocol itself.
docs/source/reference/package-apis/drivers/iscsi.md (2)
Learnt from: bennyz
PR: jumpstarter-dev/jumpstarter#241
File: packages/jumpstarter-driver-tftp/jumpstarter_driver_tftp/client.py:52-60
Timestamp: 2025-01-29T11:52:43.554Z
Learning: The TFTP driver (packages/jumpstarter-driver-tftp/jumpstarter_driver_tftp/driver.py) handles all low-level concerns like path validation, error handling, and checksum computation. The client (packages/jumpstarter-driver-tftp/jumpstarter_driver_tftp/client.py) should remain simple as it delegates these responsibilities to the driver.
Learnt from: bennyz
PR: jumpstarter-dev/jumpstarter#267
File: packages/jumpstarter-driver-tftp/jumpstarter_driver_tftp/driver.py:32-40
Timestamp: 2025-02-08T16:08:34.616Z
Learning: In the TFTP driver (jumpstarter_driver_tftp), the TFTP server component is read-only, while file management operations (upload, delete) are handled through the driver's client interface, not through the TFTP protocol itself.
packages/jumpstarter-driver-iscsi/README.md (2)
Learnt from: bennyz
PR: jumpstarter-dev/jumpstarter#241
File: packages/jumpstarter-driver-tftp/jumpstarter_driver_tftp/client.py:52-60
Timestamp: 2025-01-29T11:52:43.554Z
Learning: The TFTP driver (packages/jumpstarter-driver-tftp/jumpstarter_driver_tftp/driver.py) handles all low-level concerns like path validation, error handling, and checksum computation. The client (packages/jumpstarter-driver-tftp/jumpstarter_driver_tftp/client.py) should remain simple as it delegates these responsibilities to the driver.
Learnt from: bennyz
PR: jumpstarter-dev/jumpstarter#267
File: packages/jumpstarter-driver-tftp/jumpstarter_driver_tftp/driver.py:32-40
Timestamp: 2025-02-08T16:08:34.616Z
Learning: In the TFTP driver (jumpstarter_driver_tftp), the TFTP server component is read-only, while file management operations (upload, delete) are handled through the driver's client interface, not through the TFTP protocol itself.
🪛 markdownlint-cli2 (0.17.2)
docs/source/reference/package-apis/drivers/iscsi.md
22-22: Dollar signs used before commands without showing output
(MD014, commands-show-output)
54-54: Emphasis style
Expected: underscore; Actual: asterisk
(MD049, emphasis-style)
54-54: Emphasis style
Expected: underscore; Actual: asterisk
(MD049, emphasis-style)
packages/jumpstarter-driver-iscsi/README.md
22-22: Dollar signs used before commands without showing output
(MD014, commands-show-output)
54-54: Emphasis style
Expected: underscore; Actual: asterisk
(MD049, emphasis-style)
54-54: Emphasis style
Expected: underscore; Actual: asterisk
(MD049, emphasis-style)
🪛 YAMLlint (1.37.1)
packages/jumpstarter-driver-iscsi/examples/exporter.yaml
[error] 16-16: no new line character at the end of file
(new-line-at-end-of-file)
[error] 16-16: trailing spaces
(trailing-spaces)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (10)
- GitHub Check: pytest-matrix (macos-15, 3.13)
- GitHub Check: pytest-matrix (ubuntu-24.04, 3.11)
- GitHub Check: pytest-matrix (macos-15, 3.12)
- GitHub Check: pytest-matrix (ubuntu-24.04, 3.13)
- GitHub Check: pytest-matrix (ubuntu-24.04, 3.12)
- GitHub Check: pytest-matrix (macos-15, 3.11)
- GitHub Check: e2e
- GitHub Check: Redirect rules - jumpstarter-docs
- GitHub Check: Header rules - jumpstarter-docs
- GitHub Check: Pages changed - jumpstarter-docs
🔇 Additional comments (6)
packages/jumpstarter-driver-iscsi/jumpstarter_driver_iscsi/driver.py (3)
1-28: Clean imports and well-structured exception hierarchy.The imports are now properly cleaned up (addressing the previous unused import issue), and the exception hierarchy provides good granularity for error handling.
224-241: Excellent path traversal protection implementation.The path validation logic properly addresses the security concern raised in past comments by:
- Validating that normalized paths don't start with '..' or aren't absolute
- Using
os.path.abspath()to resolve any remaining path traversal attempts- Ensuring resolved paths stay within the root directory boundary
- Handling edge cases like paths equal to root_path
This implementation follows security best practices for path validation.
243-273: Robust file storage object creation with proper size handling.The file creation logic handles multiple scenarios well:
- Creates new files with specified size
- Uses existing file size when size_mb is 0
- Extends files when current size is smaller than requested
- Provides appropriate warnings when files are larger than requested
The size calculations and file operations are handled correctly.
packages/jumpstarter-driver-iscsi/examples/exporter.yaml (1)
1-16: Well-structured exporter configuration example.The YAML configuration properly demonstrates:
- Correct CRD structure with appropriate metadata
- Proper driver type specification
- Comprehensive configuration parameters with sensible defaults
- Clear example for deployment scenarios
This provides a good reference for users deploying the iSCSI driver in Kubernetes.
packages/jumpstarter-driver-iscsi/README.md (1)
1-63: Comprehensive and well-structured documentation.This README successfully addresses the previous feedback about needing detailed documentation. It includes:
- Clear installation instructions with distribution-specific guidance
- Proper configuration examples with parameter descriptions
- Important warnings about target vs initiator distinction
- Complete API reference integration
The documentation follows good practices and provides users with all necessary information to get started.
docs/source/reference/package-apis/drivers/iscsi.md (1)
1-63: Excellent API documentation structure.The documentation provides comprehensive coverage of the iSCSI driver with:
- Clear explanation of the target implementation
- Detailed installation and configuration guidance
- Proper Sphinx integration with autoclass directives
- Good balance between technical detail and usability
The content appropriately mirrors the README while being formatted for the documentation system.
Signed-off-by: Benny Zlotnik <bzlotnik@protonmail.com> wip Signed-off-by: Benny Zlotnik <bzlotnik@protonmail.com> ruff Signed-off-by: Benny Zlotnik <bzlotnik@redhat.com> address coderabbit comments Signed-off-by: Benny Zlotnik <bzlotnik@redhat.com> add docs Signed-off-by: Benny Zlotnik <bzlotnik@redhat.com> address comments Signed-off-by: Benny Zlotnik <bzlotnik@redhat.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (4)
packages/jumpstarter-driver-iscsi/jumpstarter_driver_iscsi/driver.py (2)
71-80: Consider a more robust approach for determining the default IP.The current implementation relies on external connectivity to 8.8.8.8, which may fail in air-gapped environments. Additionally, falling back to
0.0.0.0binds to all interfaces, which might not be the intended behavior.Consider using a more robust approach that enumerates local network interfaces:
import netifaces def get_default_ip(self): """Get the IP address of the default route interface""" try: # Try the current approach first with socket.socket(socket.AF_INET, socket.SOCK_DGRAM) as s: s.connect(("8.8.8.8", 80)) return s.getsockname()[0] except Exception: # Fallback to checking network interfaces try: gws = netifaces.gateways() default_iface = gws['default'][netifaces.AF_INET][1] addrs = netifaces.ifaddresses(default_iface) return addrs[netifaces.AF_INET][0]['addr'] except Exception: self.logger.warning("Could not determine default IP address, falling back to 127.0.0.1") return "127.0.0.1" # localhost is safer than 0.0.0.0
173-175: Address the TODO about LUN removal behavior.The current implementation removes all LUNs when stopping. This behavior might not always be desired, especially if the target is meant to be restarted with the same configuration.
Would you like me to implement a configurable option to preserve LUNs on stop, or open an issue to track this enhancement?
packages/jumpstarter-driver-iscsi/examples/exporter.yaml (1)
16-16: Fix YAML formatting issues.Remove trailing spaces and add a newline at the end of the file.
- port: 3260 + port: 3260docs/source/reference/package-apis/drivers/iscsi.md (1)
22-22: Fix markdown formatting for consistency.Address the following formatting issues:
- Line 22: Remove dollar sign or show command output
- Line 54: Use underscores for emphasis instead of asterisks
For line 22:
-$ sudo dnf install targetcli python3-rtslib +sudo dnf install targetcli python3-rtslibFor line 54:
-| `host` | IP address to bind the target to. Empty string will auto-detect default IP. | str | no | *auto* | +| `host` | IP address to bind the target to. Empty string will auto-detect default IP. | str | no | _auto_ |Also applies to: 54-54
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
docs/source/reference/package-apis/drivers/index.md(2 hunks)docs/source/reference/package-apis/drivers/iscsi.md(1 hunks)packages/jumpstarter-driver-iscsi/README.md(1 hunks)packages/jumpstarter-driver-iscsi/examples/exporter.yaml(1 hunks)packages/jumpstarter-driver-iscsi/examples/iscsi.py(1 hunks)packages/jumpstarter-driver-iscsi/jumpstarter_driver_iscsi/client.py(1 hunks)packages/jumpstarter-driver-iscsi/jumpstarter_driver_iscsi/driver.py(1 hunks)packages/jumpstarter-driver-iscsi/pyproject.toml(1 hunks)pyproject.toml(1 hunks)
✅ Files skipped from review due to trivial changes (2)
- docs/source/reference/package-apis/drivers/index.md
- packages/jumpstarter-driver-iscsi/pyproject.toml
🚧 Files skipped from review as they are similar to previous changes (4)
- pyproject.toml
- packages/jumpstarter-driver-iscsi/README.md
- packages/jumpstarter-driver-iscsi/jumpstarter_driver_iscsi/client.py
- packages/jumpstarter-driver-iscsi/examples/iscsi.py
🧰 Additional context used
🧠 Learnings (3)
📓 Common learnings
Learnt from: bennyz
PR: jumpstarter-dev/jumpstarter#241
File: packages/jumpstarter-driver-tftp/jumpstarter_driver_tftp/client.py:52-60
Timestamp: 2025-01-29T11:52:43.554Z
Learning: The TFTP driver (packages/jumpstarter-driver-tftp/jumpstarter_driver_tftp/driver.py) handles all low-level concerns like path validation, error handling, and checksum computation. The client (packages/jumpstarter-driver-tftp/jumpstarter_driver_tftp/client.py) should remain simple as it delegates these responsibilities to the driver.
Learnt from: bennyz
PR: jumpstarter-dev/jumpstarter#267
File: packages/jumpstarter-driver-tftp/jumpstarter_driver_tftp/driver.py:32-40
Timestamp: 2025-02-08T16:08:34.616Z
Learning: In the TFTP driver (jumpstarter_driver_tftp), the TFTP server component is read-only, while file management operations (upload, delete) are handled through the driver's client interface, not through the TFTP protocol itself.
packages/jumpstarter-driver-iscsi/jumpstarter_driver_iscsi/driver.py (2)
Learnt from: bennyz
PR: jumpstarter-dev/jumpstarter#241
File: packages/jumpstarter-driver-tftp/jumpstarter_driver_tftp/client.py:52-60
Timestamp: 2025-01-29T11:52:43.554Z
Learning: The TFTP driver (packages/jumpstarter-driver-tftp/jumpstarter_driver_tftp/driver.py) handles all low-level concerns like path validation, error handling, and checksum computation. The client (packages/jumpstarter-driver-tftp/jumpstarter_driver_tftp/client.py) should remain simple as it delegates these responsibilities to the driver.
Learnt from: bennyz
PR: jumpstarter-dev/jumpstarter#267
File: packages/jumpstarter-driver-tftp/jumpstarter_driver_tftp/driver.py:32-40
Timestamp: 2025-02-08T16:08:34.616Z
Learning: In the TFTP driver (jumpstarter_driver_tftp), the TFTP server component is read-only, while file management operations (upload, delete) are handled through the driver's client interface, not through the TFTP protocol itself.
docs/source/reference/package-apis/drivers/iscsi.md (2)
Learnt from: bennyz
PR: jumpstarter-dev/jumpstarter#241
File: packages/jumpstarter-driver-tftp/jumpstarter_driver_tftp/client.py:52-60
Timestamp: 2025-01-29T11:52:43.554Z
Learning: The TFTP driver (packages/jumpstarter-driver-tftp/jumpstarter_driver_tftp/driver.py) handles all low-level concerns like path validation, error handling, and checksum computation. The client (packages/jumpstarter-driver-tftp/jumpstarter_driver_tftp/client.py) should remain simple as it delegates these responsibilities to the driver.
Learnt from: bennyz
PR: jumpstarter-dev/jumpstarter#267
File: packages/jumpstarter-driver-tftp/jumpstarter_driver_tftp/driver.py:32-40
Timestamp: 2025-02-08T16:08:34.616Z
Learning: In the TFTP driver (jumpstarter_driver_tftp), the TFTP server component is read-only, while file management operations (upload, delete) are handled through the driver's client interface, not through the TFTP protocol itself.
🧬 Code Graph Analysis (1)
packages/jumpstarter-driver-iscsi/jumpstarter_driver_iscsi/driver.py (3)
packages/jumpstarter-driver-opendal/jumpstarter_driver_opendal/driver.py (1)
Opendal(21-203)packages/jumpstarter/jumpstarter/driver/base.py (1)
Driver(56-269)packages/jumpstarter-driver-iscsi/jumpstarter_driver_iscsi/client.py (8)
start(20-27)stop(29-38)remove_lun(85-95)get_host(40-47)get_port(49-56)get_target_iqn(58-65)add_lun(67-83)list_luns(97-104)
🪛 markdownlint-cli2 (0.17.2)
docs/source/reference/package-apis/drivers/iscsi.md
22-22: Dollar signs used before commands without showing output
(MD014, commands-show-output)
54-54: Emphasis style
Expected: underscore; Actual: asterisk
(MD049, emphasis-style)
54-54: Emphasis style
Expected: underscore; Actual: asterisk
(MD049, emphasis-style)
🪛 YAMLlint (1.37.1)
packages/jumpstarter-driver-iscsi/examples/exporter.yaml
[error] 16-16: no new line character at the end of file
(new-line-at-end-of-file)
[error] 16-16: trailing spaces
(trailing-spaces)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (10)
- GitHub Check: Redirect rules - jumpstarter-docs
- GitHub Check: Header rules - jumpstarter-docs
- GitHub Check: Pages changed - jumpstarter-docs
- GitHub Check: e2e
- GitHub Check: pytest-matrix (macos-15, 3.13)
- GitHub Check: pytest-matrix (macos-15, 3.11)
- GitHub Check: pytest-matrix (ubuntu-24.04, 3.13)
- GitHub Check: pytest-matrix (ubuntu-24.04, 3.11)
- GitHub Check: pytest-matrix (macos-15, 3.12)
- GitHub Check: pytest-matrix (ubuntu-24.04, 3.12)
🔇 Additional comments (3)
packages/jumpstarter-driver-iscsi/jumpstarter_driver_iscsi/driver.py (2)
222-242: Excellent path traversal prevention implementation!The path validation logic properly addresses the security concern by:
- Normalizing paths and checking for
..prefix- Using absolute path resolution to ensure paths stay within
root_dir- Properly handling the edge case where resolved path equals root path
This implementation effectively prevents directory traversal attacks.
363-370: Good cleanup implementation.The
closemethod properly handles cleanup by:
- Attempting to stop the iSCSI target
- Catching and logging exceptions to ensure cleanup continues
- Always calling
super().close()for proper resource cleanupdocs/source/reference/package-apis/drivers/iscsi.md (1)
3-5: Verify the RFC-tgt framework link.The text mentions "RFC-tgt framework" but links to the tcmu-runner repository. While related, these are different components of the Linux SCSI target subsystem.
Please verify if this should link to the LIO/target documentation instead, or update the text to match the link.
github-project-automation
Bot
moved this from In Progress
to Done
in Jumpstarter planning
2 participants
Summary by CodeRabbit
New Features
Documentation
Chores