-
Notifications
You must be signed in to change notification settings - Fork 19
Add jumpstarter-driver-ssh-mount package for remote filesystem mounting #434
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from 6 commits
2f57133
102d337
b9d8473
133978b
b221596
e1d1d66
9f20741
058cf54
9feb62e
35ddb82
4d3f152
0bea7c6
5d9038e
9eb2c0d
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1 @@ | ||
| ../../../../../packages/jumpstarter-driver-ssh-mount/README.md | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,3 @@ | ||
| __pycache__/ | ||
| .coverage | ||
| coverage.xml |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,69 @@ | ||
| # SSHMount Driver | ||
|
|
||
| `jumpstarter-driver-ssh-mount` provides remote filesystem mounting via sshfs. It allows you to mount remote directories from a target device to your local machine using SSHFS (SSH Filesystem). | ||
|
|
||
| ## Installation | ||
|
|
||
| ```shell | ||
| pip3 install --extra-index-url https://pkg.jumpstarter.dev/simple/ jumpstarter-driver-ssh-mount | ||
| ``` | ||
|
|
||
| You also need `sshfs` installed on the client machine: | ||
|
|
||
| - **Fedora/RHEL**: `sudo dnf install fuse-sshfs` | ||
| - **Debian/Ubuntu**: `sudo apt-get install sshfs` | ||
| - **macOS**: Install macFUSE from https://macfuse.github.io/ and then install | ||
| sshfs from source, as Homebrew has removed sshfs support. | ||
|
|
||
| ## Configuration | ||
|
|
||
| The SSHMount driver references an existing SSH driver to inherit credentials | ||
| (username, identity key) and TCP connectivity. No duplicate configuration is needed. | ||
|
|
||
| Example exporter configuration: | ||
|
|
||
| ```yaml | ||
| export: | ||
| ssh: | ||
| type: jumpstarter_driver_ssh.driver.SSHWrapper | ||
| config: | ||
| default_username: "root" | ||
| # ssh_identity_file: "/path/to/ssh/key" | ||
| children: | ||
| tcp: | ||
| type: jumpstarter_driver_network.driver.TcpNetwork | ||
| config: | ||
| host: "192.168.1.100" | ||
| port: 22 | ||
| mount: | ||
| type: jumpstarter_driver_ssh_mount.driver.SSHMount | ||
| children: | ||
| ssh: | ||
| ref: "ssh" | ||
| ``` | ||
|
|
||
| ## CLI Usage | ||
|
|
||
| Inside a `jmp shell` session: | ||
|
|
||
| ```shell | ||
| # Mount remote filesystem | ||
| j mount /local/mountpoint | ||
| j mount /local/mountpoint -r /remote/path | ||
| j mount /local/mountpoint --direct | ||
|
|
||
| # Unmount | ||
| j mount --umount /local/mountpoint | ||
| j mount --umount /local/mountpoint --lazy | ||
| ``` | ||
mangelajo marked this conversation as resolved.
Show resolved
Hide resolved
Comment on lines
+46
to
+64
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [LOW] The CLI section in the README omits the Suggestion: add an example showing AI-generated, human reviewed
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Added in 5d9038e -- README CLI Usage section now includes an example of passing extra sshfs options via -o. |
||
|
|
||
| ## API Reference | ||
|
|
||
| ### SSHMountClient | ||
|
|
||
| - `mount(mountpoint, *, remote_path="/", direct=False, extra_args=None)` - Mount remote filesystem locally via sshfs | ||
| - `umount(mountpoint, *, lazy=False)` - Unmount a previously mounted sshfs filesystem | ||
|
|
||
| ### CLI | ||
|
|
||
| The driver registers as `mount` in the exporter config. When used in a `jmp shell` session, the CLI is a single command with a `--umount` flag for unmounting. | ||
|
Comment on lines
+77
to
+92
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [LOW] The API Reference section lacks a structured "Required Children" section. The Suggestion: add a "Required Children" subsection with the child name and type. AI-generated, human reviewed
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Added in 5d9038e -- README API Reference section now includes a structured "Required Children" table listing the ssh child name, type, and description. |
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,24 @@ | ||
| apiVersion: jumpstarter.dev/v1alpha1 | ||
| kind: ExporterConfig | ||
| metadata: | ||
| namespace: default | ||
| name: demo | ||
| endpoint: grpc.jumpstarter.192.168.0.203.nip.io:8082 | ||
| token: "<token>" | ||
| export: | ||
| ssh: | ||
| type: jumpstarter_driver_ssh.driver.SSHWrapper | ||
| config: | ||
| default_username: "root" | ||
| # ssh_identity_file: "/path/to/key" | ||
| children: | ||
| tcp: | ||
| type: jumpstarter_driver_network.driver.TcpNetwork | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think it's better to use here an SSH driver instead, in that way the ssh_identity file and other settings are already provided and we don't need to do that here as well.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Done -- updated the exporter config to reference the SSH driver directly via |
||
| config: | ||
| host: "192.168.1.100" | ||
| port: 22 | ||
| mount: | ||
| type: jumpstarter_driver_ssh_mount.driver.SSHMount | ||
| children: | ||
| ssh: | ||
| ref: "ssh" | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1 @@ | ||
|
|
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,277 @@ | ||
| from __future__ import annotations | ||
|
|
||
| import os | ||
| import subprocess | ||
| import tempfile | ||
| import threading | ||
| from dataclasses import dataclass, field | ||
| from typing import Any | ||
| from urllib.parse import urlparse | ||
|
|
||
| import click | ||
| from jumpstarter_driver_composite.client import CompositeClient | ||
| from jumpstarter_driver_network.adapters import TcpPortforwardAdapter | ||
|
|
||
| from jumpstarter.client.core import DriverMethodNotImplemented | ||
| from jumpstarter.client.decorators import driver_click_command | ||
|
|
||
| # Timeout in seconds for subprocess calls (sshfs mount/umount) | ||
| SUBPROCESS_TIMEOUT = 120 | ||
|
|
||
|
|
||
| @dataclass | ||
| class _MountInfo: | ||
| """Tracks state associated with an active sshfs mount.""" | ||
| mountpoint: str | ||
| identity_file: str | None = None | ||
| port_forward: Any | None = None | ||
| port_forward_thread: threading.Thread | None = None | ||
|
|
||
|
|
||
| @dataclass(kw_only=True) | ||
| class SSHMountClient(CompositeClient): | ||
| _active_mounts: dict[str, _MountInfo] = field(default_factory=dict, init=False, repr=False) | ||
|
|
||
| def cli(self): | ||
| @driver_click_command(self) | ||
| @click.argument("mountpoint", type=click.Path()) | ||
| @click.option("--umount", "-u", is_flag=True, help="Unmount instead of mount") | ||
| @click.option("--remote-path", "-r", default="/", help="Remote path to mount (default: /)") | ||
| @click.option("--direct", is_flag=True, help="Use direct TCP address") | ||
| @click.option("--lazy", "-l", is_flag=True, help="Lazy unmount (detach filesystem now, clean up later)") | ||
| @click.option("--extra-args", "-o", multiple=True, help="Extra arguments to pass to sshfs") | ||
| def mount(mountpoint, umount, remote_path, direct, lazy, extra_args): | ||
| """Mount or unmount remote filesystem via sshfs""" | ||
| if umount: | ||
| self.umount(mountpoint, lazy=lazy) | ||
| else: | ||
| self.mount(mountpoint, remote_path=remote_path, direct=direct, extra_args=list(extra_args)) | ||
|
|
||
| return mount | ||
|
|
||
| @property | ||
| def identity(self) -> str | None: | ||
| return self.ssh.identity | ||
|
|
||
| @property | ||
| def username(self) -> str: | ||
| return self.ssh.username | ||
|
|
||
| def mount(self, mountpoint, *, remote_path="/", direct=False, extra_args=None): | ||
| """Mount remote filesystem locally via sshfs""" | ||
| # Verify sshfs is available | ||
| sshfs_path = self._find_executable("sshfs") | ||
| if not sshfs_path: | ||
| raise click.ClickException( | ||
| "sshfs is not installed. Please install it:\n" | ||
| " Fedora/RHEL: sudo dnf install fuse-sshfs\n" | ||
| " Debian/Ubuntu: sudo apt-get install sshfs\n" | ||
| " macOS: Install macFUSE from https://macfuse.github.io/ and then install\n" | ||
| " sshfs from source, as Homebrew has removed sshfs support." | ||
| ) | ||
|
|
||
| # Resolve to absolute path for consistent tracking | ||
| mountpoint = os.path.realpath(mountpoint) | ||
|
|
||
| # Create mountpoint directory if it doesn't exist | ||
| os.makedirs(mountpoint, exist_ok=True) | ||
raballew marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
|
||
| if direct: | ||
| try: | ||
| address = self.ssh.tcp.address() | ||
| parsed = urlparse(address) | ||
| host = parsed.hostname | ||
| port = parsed.port | ||
| if not host or not port: | ||
| raise ValueError(f"Invalid address format: {address}") | ||
| self.logger.debug("Using direct TCP connection for sshfs - host: %s, port: %s", host, port) | ||
| self._run_sshfs(host, port, mountpoint, remote_path, extra_args, port_forward=None) | ||
| except (DriverMethodNotImplemented, ValueError) as e: | ||
| self.logger.error( | ||
| "Direct address connection failed (%s), falling back to port forwarding", e | ||
| ) | ||
| self.mount(mountpoint, remote_path=remote_path, direct=False, extra_args=extra_args) | ||
| else: | ||
| self.logger.debug("Using SSH port forwarding for sshfs connection") | ||
| # Create port forward adapter and keep it alive for the duration of the mount. | ||
| # We enter the context manager manually and only exit it on umount. | ||
| adapter = TcpPortforwardAdapter(client=self.ssh.tcp) | ||
| host, port = adapter.__enter__() | ||
| self.logger.debug("SSH port forward established - host: %s, port: %s", host, port) | ||
| try: | ||
| self._run_sshfs(host, port, mountpoint, remote_path, extra_args, port_forward=adapter) | ||
| except Exception: | ||
| # If sshfs failed, tear down the port forward immediately | ||
| adapter.__exit__(None, None, None) | ||
|
||
| raise | ||
|
||
|
|
||
| def _run_sshfs(self, host, port, mountpoint, remote_path, extra_args, *, port_forward): | ||
| identity_file = self._create_temp_identity_file() | ||
|
|
||
| try: | ||
| sshfs_args = self._build_sshfs_args(host, port, mountpoint, remote_path, identity_file, extra_args) | ||
| self.logger.debug("Running sshfs command: %s", sshfs_args) | ||
|
|
||
| result = subprocess.run(sshfs_args, capture_output=True, text=True, timeout=SUBPROCESS_TIMEOUT) | ||
| result = self._retry_sshfs_without_allow_other(result, sshfs_args) | ||
|
|
||
| if result.returncode != 0: | ||
| stderr = result.stderr.strip() | ||
| raise click.ClickException( | ||
| f"sshfs mount failed (exit code {result.returncode}): {stderr}" | ||
| ) | ||
|
|
||
| # Track this mount so we can clean up on umount | ||
| self._active_mounts[mountpoint] = _MountInfo( | ||
| mountpoint=mountpoint, | ||
| identity_file=identity_file, | ||
| port_forward=port_forward, | ||
| ) | ||
|
|
||
| default_username = self.username | ||
| user_prefix = f"{default_username}@" if default_username else "" | ||
| remote_spec = f"{user_prefix}{host}:{remote_path}" | ||
| click.echo(f"Mounted {remote_spec} on {mountpoint}") | ||
| click.echo(f"To unmount: j mount --umount {mountpoint}") | ||
| except click.ClickException: | ||
| # Clean up identity file on failure | ||
| self._cleanup_identity_file(identity_file) | ||
|
Comment on lines
+107
to
+148
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [HIGH] The sshfs process cleanup code (lines 138-148) is inside the Suggestion: move sshfs termination and finally:
if sshfs_proc is not None and sshfs_proc.poll() is None:
sshfs_proc.terminate()
try:
sshfs_proc.wait(timeout=10)
except subprocess.TimeoutExpired:
sshfs_proc.kill()
sshfs_proc.wait()
self._force_umount(mountpoint)
self._cleanup_identity_file(identity_file)AI-generated, human reviewed
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Fixed in 5d9038e -- moved sshfs process termination and
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Fixed in 5d9038e -- moved sshfs process termination and _force_umount() into the finally block of _run_sshfs. Now cleanup happens even if _run_subshell() or sshfs_proc.wait() raises an exception, including KeyboardInterrupt. |
||
| raise | ||
| except Exception as e: | ||
| self._cleanup_identity_file(identity_file) | ||
| raise click.ClickException(f"Failed to mount: {e}") from e | ||
|
|
||
| def _build_sshfs_args(self, host, port, mountpoint, remote_path, identity_file, extra_args): | ||
| default_username = self.username | ||
| user_prefix = f"{default_username}@" if default_username else "" | ||
| remote_spec = f"{user_prefix}{host}:{remote_path}" | ||
|
|
||
| sshfs_args = ["sshfs", remote_spec, mountpoint] | ||
|
|
||
| ssh_opts = [ | ||
| "StrictHostKeyChecking=no", | ||
| "UserKnownHostsFile=/dev/null", | ||
| "LogLevel=ERROR", | ||
| ] | ||
|
Comment on lines
+243
to
+247
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [LOW] Suggestion: document the override in the README. AI-generated, human reviewed
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Added in 5d9038e -- the README now documents the -o / --extra-args option with examples, and notes that it can be used to override defaults like StrictHostKeyChecking. |
||
|
|
||
| if port and port != 22: | ||
| sshfs_args.extend(["-p", str(port)]) | ||
|
Comment on lines
+249
to
+250
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [LOW] The port 22 conditional skip (not adding Suggestion: add a test asserting AI-generated, human reviewed
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Added in 5d9038e -- test_mount_port_22_omits_p_flag verifies that when the forwarded port is 22, the -p flag is not included in the sshfs args. |
||
|
|
||
| if identity_file: | ||
| ssh_opts.append(f"IdentityFile={identity_file}") | ||
|
|
||
| ssh_opts.append("allow_other") | ||
|
|
||
| for opt in ssh_opts: | ||
| sshfs_args.extend(["-o", opt]) | ||
|
|
||
| if extra_args: | ||
| sshfs_args.extend(extra_args) | ||
|
Comment on lines
+260
to
+261
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [LOW] Suggestion: document that AI-generated, human reviewed
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Agreed -- extra_args comes from the CLI (user-provided) or programmatic callers, both of which are trusted contexts. Added a note to the README documenting that extra_args are forwarded directly to sshfs. |
||
|
|
||
| return sshfs_args | ||
|
|
||
| def _retry_sshfs_without_allow_other(self, result, sshfs_args): | ||
| """Retry sshfs without allow_other if it failed due to that option""" | ||
| if result.returncode != 0 and "allow_other" in result.stderr: | ||
| self.logger.debug("Retrying sshfs without allow_other option") | ||
| # Remove both the "-o" flag and the "allow_other" value together | ||
| filtered = [] | ||
| skip_next = False | ||
| for i, arg in enumerate(sshfs_args): | ||
| if skip_next: | ||
| skip_next = False | ||
| continue | ||
| if arg == "-o" and i + 1 < len(sshfs_args) and sshfs_args[i + 1] == "allow_other": | ||
| skip_next = True | ||
| continue | ||
| filtered.append(arg) | ||
| return subprocess.run(filtered, capture_output=True, text=True, timeout=SUBPROCESS_TIMEOUT) | ||
| return result | ||
|
|
||
| def _create_temp_identity_file(self): | ||
| """Create a temporary file with the SSH identity key, if configured.""" | ||
|
||
| ssh_identity = self.identity | ||
| if not ssh_identity: | ||
| return None | ||
|
|
||
| temp_file = None | ||
| try: | ||
| temp_file = tempfile.NamedTemporaryFile(mode='w', delete=False, suffix='_ssh_key') | ||
| temp_file.write(ssh_identity) | ||
| temp_file.close() | ||
| os.chmod(temp_file.name, 0o600) | ||
| self.logger.debug("Created temporary identity file: %s", temp_file.name) | ||
| return temp_file.name | ||
| except Exception as e: | ||
| self.logger.error("Failed to create temporary identity file: %s", e) | ||
| if temp_file: | ||
| try: | ||
| os.unlink(temp_file.name) | ||
| except Exception: | ||
| pass | ||
| raise | ||
|
Comment on lines
+270
to
+289
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [LOW] In Suggestion: close the file handle before unlinking in the error path, or use a AI-generated, human reviewed
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Fixed in 5d9038e -- the error path now explicitly closes the file handle before unlinking, preventing the fd leak when write() raises. |
||
|
|
||
| def _cleanup_identity_file(self, identity_file): | ||
| """Remove a temporary identity file if it exists.""" | ||
| if identity_file: | ||
| try: | ||
| os.unlink(identity_file) | ||
| self.logger.debug("Cleaned up temporary identity file: %s", identity_file) | ||
| except Exception as e: | ||
| self.logger.warning("Failed to clean up identity file %s: %s", identity_file, e) | ||
|
|
||
| def umount(self, mountpoint=None, *, lazy=False): | ||
|
Check failure on line 223 in python/packages/jumpstarter-driver-ssh-mount/jumpstarter_driver_ssh_mount/client.py
|
||
| """Unmount a previously mounted sshfs filesystem. | ||
|
|
||
| If mountpoint is None, unmounts all active mounts from this session. | ||
| """ | ||
| if mountpoint is None: | ||
| # Unmount everything from this session | ||
| if not self._active_mounts: | ||
| click.echo("No active mounts to unmount.") | ||
| return | ||
| # Copy keys to avoid mutation during iteration | ||
| for mp in list(self._active_mounts.keys()): | ||
| self.umount(mp, lazy=lazy) | ||
| return | ||
|
|
||
| mountpoint = os.path.realpath(mountpoint) | ||
|
|
||
| # Try fusermount first (Linux), fall back to umount (macOS) | ||
| fusermount = self._find_executable("fusermount3") or self._find_executable("fusermount") | ||
| if fusermount: | ||
| cmd = [fusermount, "-u"] | ||
| if lazy: | ||
| cmd.append("-z") | ||
| cmd.append(mountpoint) | ||
| else: | ||
| cmd = ["umount"] | ||
| if lazy: | ||
| cmd.append("-l") | ||
|
||
| cmd.append(mountpoint) | ||
|
|
||
| self.logger.debug("Running unmount command: %s", cmd) | ||
| result = subprocess.run(cmd, capture_output=True, text=True, timeout=SUBPROCESS_TIMEOUT) | ||
|
|
||
| if result.returncode != 0: | ||
| stderr = result.stderr.strip() | ||
| raise click.ClickException(f"Unmount failed (exit code {result.returncode}): {stderr}") | ||
|
|
||
| # Clean up tracked resources for this mount | ||
| mount_info = self._active_mounts.pop(mountpoint, None) | ||
| if mount_info: | ||
| self._cleanup_identity_file(mount_info.identity_file) | ||
| if mount_info.port_forward: | ||
| try: | ||
| mount_info.port_forward.__exit__(None, None, None) | ||
| self.logger.debug("Closed port forward for %s", mountpoint) | ||
| except Exception as e: | ||
| self.logger.warning("Failed to close port forward for %s: %s", mountpoint, e) | ||
|
|
||
| click.echo(f"Unmounted {mountpoint}") | ||
|
|
||
| @staticmethod | ||
| def _find_executable(name): | ||
| """Find an executable in PATH, return full path or None""" | ||
| import shutil | ||
| return shutil.which(name) | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
you need to add this one to the driver index or docs compilation will fail.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done -- added
ssh-mount.mdto both the Utility Drivers listing and thetoctreedirective in the drivers index.