Add DbusNetwork driver #245
Conversation
WalkthroughThe pull request introduces a new Changes
Sequence DiagramsequenceDiagram
participant DN as DbusNetwork
participant Env as Environment
participant Bus as D-Bus Service
DN->>Env: Retrieve bus address
alt System Bus
DN->>Env: Get DBUS_SYSTEM_BUS_ADDRESS
else Session Bus
DN->>Env: Get DBUS_SESSION_BUS_ADDRESS
end
DN->>DN: Parse bus address
DN->>Bus: Establish Connection
alt Connection Successful
Bus-->>DN: Connected
else Connection Failed
DN->>DN: Raise ValueError
end
Poem
✨ Finishing Touches
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
packages/jumpstarter-driver-network/jumpstarter_driver_network/driver.py (1)
66-69: Add docstring forDbusNetworkclass.
It would be beneficial to add a docstring to briefly explain how the bus type is determined (system vs session) and how environment variables are leveraged. This helps future maintainers understand the intended usage.packages/jumpstarter-driver-network/jumpstarter_driver_network/driver_test.py (1)
113-116: Ensure tests run in a controlled environment.
Skipping the test if the DBus system bus isn't present is appropriate. Consider adding a fallback or mocking approach if you want coverage in CI environments that lack system D-Bus.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
packages/jumpstarter-driver-network/jumpstarter_driver_network/driver.py(2 hunks)packages/jumpstarter-driver-network/jumpstarter_driver_network/driver_test.py(3 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (5)
- GitHub Check: pytest-matrix (3.13)
- GitHub Check: pytest-matrix (3.11)
- GitHub Check: e2e
- GitHub Check: build-and-push-image (jumpstarter-dev/jumpstarter-devspace .devfile/Containerfile.client)
- GitHub Check: build-and-push-image (jumpstarter-dev/jumpstarter Dockerfile)
🔇 Additional comments (3)
packages/jumpstarter-driver-network/jumpstarter_driver_network/driver.py (1)
4-5: Use caution when mixinggetenvand direct environment variable references.
While it's perfectly fine to import and usegetenvandgetuidhere, consider providing more context in docstrings or in your class docstring to clarify how you expect environment variables to be set for bus addresses.
[approve]packages/jumpstarter-driver-network/jumpstarter_driver_network/driver_test.py (2)
11-11: Imports look good.
All essential classes are imported. No issues.
118-128: Confirm output validation.
Currently, the test usessubprocess.runwithcheck=Truebut doesn't assert anything about the output. Confirm if you need to verify the output content to ensure correctness.
| @exportstream | ||
| @asynccontextmanager | ||
| async def connect(self): # noqa: C901 | ||
| match self.kind: | ||
| case "system": | ||
| bus = getenv("DBUS_SYSTEM_BUS_ADDRESS", "unix:path=/run/dbus/system_bus_socket") | ||
| case "session": | ||
| bus = getenv("DBUS_SESSION_BUS_ADDRESS", f"unix:path=/run/user/{getuid()}/bus") | ||
| case _: | ||
| raise ValueError(f"invalid bus type: {self.kind}") | ||
|
|
||
| scheme, sep, rem = bus.partition(":") | ||
| if not sep: | ||
| raise ValueError(f"invalid bus addr: {bus}") | ||
|
|
||
| args = {} | ||
| for part in rem.split(","): | ||
| key, sep, value = part.partition("=") | ||
| if not sep: | ||
| raise ValueError(f"invalid bus addr: {bus}, missing separator in arguments") | ||
| args[key] = value | ||
|
|
||
| match scheme: | ||
| case "unix": | ||
| if "path" not in args: | ||
| raise ValueError(f"invalid bus addr: {bus}, missing path argument") | ||
|
|
||
| self.logger.debug("Connecting UDS path=%s", args["path"]) | ||
| async with await connect_unix(path=args["path"]) as stream: | ||
| yield stream | ||
| case "tcp": | ||
| if "host" not in args: | ||
| raise ValueError(f"invalid bus addr: {bus}, missing host argument") | ||
| if "port" not in args: | ||
| raise ValueError(f"invalid bus addr: {bus}, missing port argument") | ||
|
|
||
| try: | ||
| port = int(args["port"]) | ||
| except ValueError as e: | ||
| raise ValueError(f"invalid bus addr: {bus}, invalid port argument") from e | ||
|
|
||
| self.logger.debug("Connecting TCP host=%s port=%d", args["host"], port) | ||
| async with await connect_tcp(remote_host=args["host"], remote_port=port) as stream: | ||
| yield stream | ||
| case _: | ||
| raise ValueError(f"invalid bus scheme: {scheme}") | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider breaking down the logic to reduce complexity.
The match statement plus bus parsing in connect is somewhat lengthy. Extracting the bus parsing into a helper function or methods for each scheme (unix vs tcp) might improve readability and maintainability.
async def connect(self): # noqa: C901
bus = self._get_bus_address() # extracted logic
scheme, sep, rem = bus.partition(":")
...
+
+def _get_bus_address(self) -> str:
+ match self.kind:
+ case "system":
+ return getenv("DBUS_SYSTEM_BUS_ADDRESS", "unix:path=/run/dbus/system_bus_socket")
+ case "session":
+ return getenv("DBUS_SESSION_BUS_ADDRESS", f"unix:path=/run/user/{getuid()}/bus")
+ case _:
+ raise ValueError(f"invalid bus type: {self.kind}")📝 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.
| @exportstream | |
| @asynccontextmanager | |
| async def connect(self): # noqa: C901 | |
| match self.kind: | |
| case "system": | |
| bus = getenv("DBUS_SYSTEM_BUS_ADDRESS", "unix:path=/run/dbus/system_bus_socket") | |
| case "session": | |
| bus = getenv("DBUS_SESSION_BUS_ADDRESS", f"unix:path=/run/user/{getuid()}/bus") | |
| case _: | |
| raise ValueError(f"invalid bus type: {self.kind}") | |
| scheme, sep, rem = bus.partition(":") | |
| if not sep: | |
| raise ValueError(f"invalid bus addr: {bus}") | |
| args = {} | |
| for part in rem.split(","): | |
| key, sep, value = part.partition("=") | |
| if not sep: | |
| raise ValueError(f"invalid bus addr: {bus}, missing separator in arguments") | |
| args[key] = value | |
| match scheme: | |
| case "unix": | |
| if "path" not in args: | |
| raise ValueError(f"invalid bus addr: {bus}, missing path argument") | |
| self.logger.debug("Connecting UDS path=%s", args["path"]) | |
| async with await connect_unix(path=args["path"]) as stream: | |
| yield stream | |
| case "tcp": | |
| if "host" not in args: | |
| raise ValueError(f"invalid bus addr: {bus}, missing host argument") | |
| if "port" not in args: | |
| raise ValueError(f"invalid bus addr: {bus}, missing port argument") | |
| try: | |
| port = int(args["port"]) | |
| except ValueError as e: | |
| raise ValueError(f"invalid bus addr: {bus}, invalid port argument") from e | |
| self.logger.debug("Connecting TCP host=%s port=%d", args["host"], port) | |
| async with await connect_tcp(remote_host=args["host"], remote_port=port) as stream: | |
| yield stream | |
| case _: | |
| raise ValueError(f"invalid bus scheme: {scheme}") | |
| @exportstream | |
| @asynccontextmanager | |
| async def connect(self): # noqa: C901 | |
| bus = self._get_bus_address() | |
| scheme, sep, rem = bus.partition(":") | |
| if not sep: | |
| raise ValueError(f"invalid bus addr: {bus}") | |
| args = {} | |
| for part in rem.split(","): | |
| key, sep, value = part.partition("=") | |
| if not sep: | |
| raise ValueError(f"invalid bus addr: {bus}, missing separator in arguments") | |
| args[key] = value | |
| match scheme: | |
| case "unix": | |
| if "path" not in args: | |
| raise ValueError(f"invalid bus addr: {bus}, missing path argument") | |
| self.logger.debug("Connecting UDS path=%s", args["path"]) | |
| async with await connect_unix(path=args["path"]) as stream: | |
| yield stream | |
| case "tcp": | |
| if "host" not in args: | |
| raise ValueError(f"invalid bus addr: {bus}, missing host argument") | |
| if "port" not in args: | |
| raise ValueError(f"invalid bus addr: {bus}, missing port argument") | |
| try: | |
| port = int(args["port"]) | |
| except ValueError as e: | |
| raise ValueError(f"invalid bus addr: {bus}, invalid port argument") from e | |
| self.logger.debug("Connecting TCP host=%s port=%d", args["host"], port) | |
| async with await connect_tcp(remote_host=args["host"], remote_port=port) as stream: | |
| yield stream | |
| case _: | |
| raise ValueError(f"invalid bus scheme: {scheme}") | |
| def _get_bus_address(self) -> str: | |
| match self.kind: | |
| case "system": | |
| return getenv("DBUS_SYSTEM_BUS_ADDRESS", "unix:path=/run/dbus/system_bus_socket") | |
| case "session": | |
| return getenv("DBUS_SESSION_BUS_ADDRESS", f"unix:path=/run/user/{getuid()}/bus") | |
| case _: | |
| raise ValueError(f"invalid bus type: {self.kind}") |
packages/jumpstarter-driver-network/jumpstarter_driver_network/driver_test.py
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
packages/jumpstarter-driver-network/jumpstarter_driver_network/driver_test.py (1)
113-144: Consider enhancing test coverage.While the basic connectivity tests are good, consider adding:
- Docstrings explaining the purpose of each test
- Verification of the
busctloutput to ensure expected services are available- Error scenario tests (e.g., invalid bus kind, connection failures)
- Tests for actual D-Bus service functionality (e.g., calling a method on a known service)
Example enhancement for system bus test:
@pytest.mark.skipif( os.getenv("DBUS_SYSTEM_BUS_ADDRESS") is None and not os.path.exists("/run/dbus/system_bus_socket"), reason="dbus system bus not available", ) @pytest.mark.skipif(which("busctl") is None, reason="busctl not available") def test_dbus_network_system(monkeypatch): + """ + Test D-Bus system bus connectivity and basic functionality. + + Verifies: + 1. Connection to system bus can be established + 2. Basic D-Bus operations work through the forwarded connection + """ with serve(DbusNetwork(kind="system")) as client: with PortforwardAdapter(client=client) as addr: - subprocess.run( + result = subprocess.run( ["busctl", "list", "--system", "--no-pager"], env=os.environ | {"DBUS_SYSTEM_BUS_ADDRESS": f"tcp:host={addr[0]},port={addr[1]}"}, check=True, stdout=subprocess.PIPE, stderr=subprocess.PIPE, ) + # Verify expected system services are present + output = result.stdout.decode() + assert "org.freedesktop.DBus" in output, "D-Bus service not found"
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/jumpstarter-driver-network/jumpstarter_driver_network/driver_test.py(3 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (7)
- GitHub Check: pytest-matrix (3.13)
- GitHub Check: pytest-matrix (3.12)
- GitHub Check: e2e
- GitHub Check: pytest-matrix (3.11)
- GitHub Check: build-and-push-image (jumpstarter-dev/jumpstarter-devspace .devfile/Containerfile.client)
- GitHub Check: build-and-push-image (jumpstarter-dev/jumpstarter-dev .devfile/Containerfile)
- GitHub Check: build-and-push-image (jumpstarter-dev/jumpstarter Dockerfile)
🔇 Additional comments (3)
packages/jumpstarter-driver-network/jumpstarter_driver_network/driver_test.py (3)
11-11: LGTM!The import of
DbusNetworkis correctly placed alongside other network-related imports.
113-128: LGTM! Well-structured test for D-Bus system bus.The test correctly verifies basic connectivity to the D-Bus system bus with appropriate skip conditions and environment configuration.
130-144: LGTM! Well-structured test for D-Bus session bus.The test correctly verifies basic connectivity to the D-Bus session bus with appropriate skip conditions and environment configuration.
It would not close those issues, but make it much easier to develop. To close those issues we may have to offer extensive documentation and wrappers on top to let do the specific actions on the client side. |
|
Please add documentation with examples of usage, exporter configuration, etc... |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
packages/jumpstarter-driver-network/jumpstarter_driver_network/adapters/portforward.py (1)
22-33: Consider adding validation for local_host and local_port.While the implementation is correct, it would be beneficial to add input validation for
local_hostandlocal_portto prevent potential runtime errors.@dataclass(kw_only=True) class TcpPortforwardAdapter(PortforwardAdapter): local_host: str = "127.0.0.1" local_port: int = 0 + + def __post_init__(self): + if not isinstance(self.local_port, int) or self.local_port < 0: + raise ValueError(f"Invalid port number: {self.local_port}") + if not self.local_host: + raise ValueError("local_host cannot be empty")packages/jumpstarter-driver-network/jumpstarter_driver_network/driver_test.py (1)
124-139: Consider adding more test cases for D-Bus system bus.While the basic functionality is tested, consider adding test cases for:
- Error cases (invalid bus address)
- Connection failures
- Different message types
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
packages/jumpstarter-driver-network/jumpstarter_driver_network/adapters/__init__.py(1 hunks)packages/jumpstarter-driver-network/jumpstarter_driver_network/adapters/fabric.py(1 hunks)packages/jumpstarter-driver-network/jumpstarter_driver_network/adapters/novnc.py(1 hunks)packages/jumpstarter-driver-network/jumpstarter_driver_network/adapters/pexpect.py(1 hunks)packages/jumpstarter-driver-network/jumpstarter_driver_network/adapters/portforward.py(2 hunks)packages/jumpstarter-driver-network/jumpstarter_driver_network/driver.py(3 hunks)packages/jumpstarter-driver-network/jumpstarter_driver_network/driver_test.py(5 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (7)
- GitHub Check: pytest-matrix (3.13)
- GitHub Check: pytest-matrix (3.12)
- GitHub Check: e2e
- GitHub Check: build-and-push-image (jumpstarter-dev/jumpstarter-devspace .devfile/Containerfile.client)
- GitHub Check: pytest-matrix (3.11)
- GitHub Check: build-and-push-image (jumpstarter-dev/jumpstarter-dev .devfile/Containerfile)
- GitHub Check: build-and-push-image (jumpstarter-dev/jumpstarter Dockerfile)
🔇 Additional comments (9)
packages/jumpstarter-driver-network/jumpstarter_driver_network/adapters/__init__.py (1)
4-6: LGTM! Clean refactoring of port forwarding adapters.The changes properly expose the new specialized TCP and Unix socket forwarding adapters while maintaining backward compatibility through the
__all__declaration.packages/jumpstarter-driver-network/jumpstarter_driver_network/adapters/pexpect.py (1)
6-6: LGTM! Appropriate use of TCP-specific adapter.The change to inherit from
TcpPortforwardAdapteraligns with the class's use of TCP sockets (AF_INET) for communication.Also applies to: 10-10
packages/jumpstarter-driver-network/jumpstarter_driver_network/adapters/fabric.py (1)
7-7: LGTM! Appropriate use of TCP-specific adapter.The change to inherit from
TcpPortforwardAdapteraligns with the class's use of TCP-specific connection parameters (host and port).Also applies to: 11-11
packages/jumpstarter-driver-network/jumpstarter_driver_network/adapters/novnc.py (1)
5-5: LGTM! Appropriate use of TCP-specific adapter.The change to inherit from
TcpPortforwardAdapteraligns with:
- The class's use of TCP-specific connection parameters (host and port)
- WebSocket protocol operating over TCP
Also applies to: 10-10
packages/jumpstarter-driver-network/jumpstarter_driver_network/adapters/portforward.py (2)
8-20: LGTM! Well-structured base adapter implementation.The
PortforwardAdapterprovides a clean base implementation with proper connection lifecycle management and stream forwarding.
35-40: LGTM! Clean Unix adapter implementation.The
UnixPortforwardAdapterprovides a straightforward implementation for Unix socket forwarding.packages/jumpstarter-driver-network/jumpstarter_driver_network/driver.py (2)
67-70: LGTM! Well-defined class with proper type constraints.The use of
Literaltype forkindensures type safety at compile time.
71-118: Consider breaking down the complex connection logic.The connection method is quite complex with multiple responsibilities. Consider extracting the bus address parsing and connection logic into separate methods.
+ def _parse_bus_address(self) -> tuple[str, dict[str, str]]: + match self.kind: + case "system": + bus = getenv("DBUS_SYSTEM_BUS_ADDRESS", "unix:path=/run/dbus/system_bus_socket") + case "session": + bus = getenv("DBUS_SESSION_BUS_ADDRESS", f"unix:path=/run/user/{getuid()}/bus") + case _: + raise ValueError(f"invalid bus type: {self.kind}") + + scheme, sep, rem = bus.partition(":") + if not sep: + raise ValueError(f"invalid bus addr: {bus}") + + args = {} + for part in rem.split(","): + key, sep, value = part.partition("=") + if not sep: + raise ValueError(f"invalid bus addr: {bus}, missing separator in arguments") + args[key] = value + + return scheme, args + async def _connect_unix(self, path: str) -> MetadataStream: + self.logger.debug("Connecting UDS path=%s", path) + stream = await connect_unix(path=path) + return MetadataStream(stream=stream, metadata={"kind": self.kind}) + async def _connect_tcp(self, host: str, port: int) -> MetadataStream: + self.logger.debug("Connecting TCP host=%s port=%d", host, port) + stream = await connect_tcp(remote_host=host, remote_port=port) + return MetadataStream(stream=stream, metadata={"kind": self.kind}) @exportstream @asynccontextmanager async def connect(self): # noqa: C901 - match self.kind: - case "system": - bus = getenv("DBUS_SYSTEM_BUS_ADDRESS", "unix:path=/run/dbus/system_bus_socket") - case "session": - bus = getenv("DBUS_SESSION_BUS_ADDRESS", f"unix:path=/run/user/{getuid()}/bus") - case _: - raise ValueError(f"invalid bus type: {self.kind}") - - scheme, sep, rem = bus.partition(":") - if not sep: - raise ValueError(f"invalid bus addr: {bus}") - - args = {} - for part in rem.split(","): - key, sep, value = part.partition("=") - if not sep: - raise ValueError(f"invalid bus addr: {bus}, missing separator in arguments") - args[key] = value + scheme, args = self._parse_bus_address() match scheme: case "unix": if "path" not in args: raise ValueError(f"invalid bus addr: {bus}, missing path argument") - - self.logger.debug("Connecting UDS path=%s", args["path"]) - async with await connect_unix(path=args["path"]) as stream: - async with MetadataStream(stream=stream, metadata={"kind": self.kind}) as mstream: + async with await self._connect_unix(args["path"]) as mstream: yield mstream case "tcp": if "host" not in args: raise ValueError(f"invalid bus addr: {bus}, missing host argument") if "port" not in args: raise ValueError(f"invalid bus addr: {bus}, missing port argument") try: port = int(args["port"]) except ValueError as e: raise ValueError(f"invalid bus addr: {bus}, invalid port argument") from e - self.logger.debug("Connecting TCP host=%s port=%d", args["host"], port) - async with await connect_tcp(remote_host=args["host"], remote_port=port) as stream: - async with MetadataStream(stream=stream, metadata={"kind": self.kind}) as mstream: + async with await self._connect_tcp(args["host"], port) as mstream: yield mstream case _: raise ValueError(f"invalid bus scheme: {scheme}")packages/jumpstarter-driver-network/jumpstarter_driver_network/driver_test.py (1)
141-155: Consider adding more test cases for D-Bus session bus.Similar to the system bus tests, consider adding more test cases for comprehensive coverage.
Additionally, verify that the test environment is properly cleaned up:
|
Blocked by our inability to pass labels from drivers to clients: we need a mechanism to pass the type of the dbus bus (session or system) to the client side, so it knows which environment variable to set. |
What do you mean? |
So the driver can proxy one of two different kinds of dbus: session bus or system bus, and they are represented by two different environment variables |
Right now the driver proxies the dbus socket directly without filtering. We should incorporate https://github.com/flatpak/xdg-dbus-proxy for that functionality.
Summary by CodeRabbit
New Features
Tests