fix: address PR #642 review follow-ups for dut-network driver (#653)

Addresses review feedback from @bennyz and @raballew on #642:

- **dnsmasq.stop() missing state_dir**: Pass
`state_dir=self._state_path` so the pidfile is used as the authoritative
PID source, matching the pattern already used by `reload_config()`
- **NAT probe at import time**: Defer `_can_nat_between_namespaces()`
from module-level execution to a session-scoped `nat_available` pytest
fixture, avoiding namespace/veth/nft side effects during `pytest
--collect-only` or IDE test discovery
- **shell=True in test helpers**: Refactor `_run()` and `_popen()` from
string-based `shell=True` to list-based subprocess via `shlex.split()`,
consistent with production code
- **Broken README diagram**: Replace the misaligned hardware diagram
with architecture diagrams showing internal data flow (interface →
dnsmasq → ip_fwd → nftables → upstream) for both NAT and DHCP-only modes

Co-authored-by: Cursor <cursoragent@cursor.com>

Author: mangelajo

python/packages/jumpstarter-driver-dut-network/README.md

@@ -43,7 +43,7 @@ export:
   dut-network:
     type: jumpstarter_driver_dut_network.driver.DutNetwork
     config:
-      interface: "enx00e04c683af1"
+      interface: "eth2"
       subnet: "192.168.100.0/24"
       gateway_ip: "192.168.100.1"
       nat_mode: "masquerade"
@@ -66,7 +66,7 @@ export:
   dut-network:
     type: jumpstarter_driver_dut_network.driver.DutNetwork
     config:
-      interface: "enx00e04c683af1"
+      interface: "eth2"
       subnet: "192.168.100.0/24"
       gateway_ip: "192.168.100.1"
       upstream_interface: "enp2s0"
@@ -105,7 +105,7 @@ export:
   dut-network:
     type: jumpstarter_driver_dut_network.driver.DutNetwork
     config:
-      interface: "enx00e04c683af1"
+      interface: "eth2"
       nat_mode: "masquerade"
       dns_entries:
         - hostname: "controller.lab.local"
@@ -206,15 +206,47 @@ with env() as client:
 
 The driver uses a dedicated nftables table (named after the interface, e.g. `table ip jumpstarter_enx00e04c683af1`) that does not conflict with firewalld or other nftables users. Firewalld manages its own `firewalld` table and does not touch other tables, even during reloads.
 
-## Typical Hardware Setup
+## Architecture
+
+```text
+                     Exporter Host
+ ┌─────────┐        ┌──────────────────────────────────────┐          ┌─────────┐
+ │   DUT   │        │                                      │          │   LAN   │
+ │         │  eth   │  eth2               ┌──────────┐     │          │         │
+ │  DHCP   │◄──────►│  192.168.100.1/24   │ dnsmasq  │     │          │         │
+ │  client │        │  (gateway)          │ DHCP+DNS │     │          │         │
+ │         │        │       │             └──────────┘     │          │         │
+ │ 192.168.│        │       │  forwarding                  │  eth     │         │
+ │ 100.10  │        │       ▼             ┌──────────┐     │          │         │
+ │         │        │  ┌─────────┐        │ nftables │     │ enp2s0   │ 10.26.  │
+ └─────────┘        │  │ ip_fwd  │───────►│ NAT      │────►│◄──────►  │ 28.0/24 │
+                    │  └─────────┘        │          │     │(upstream)│         │
+                    │                     │masq/1:1  │     │          └─────────┘
+                    │                     └──────────┘     │
+                    └──────────────────────────────────────┘
+
+  ─── Masquerade: DUT traffic appears as exporter's upstream IP
+  ─── 1:1 NAT:    DUT gets a dedicated public IP on the upstream interface
+```
+
+### Disabled NAT (DHCP-only isolation)
 
 ```text
-┌──────────────┐     Ethernet      ┌──────────────────┐     LAN
-│   DUT        │────────────────────│   Exporter Host  │────────────
-│ (SA8775P)    │   (USB NIC)        │   (Sidekick)     │  (enp2s0)
-│ end0:        │                    │ enx00e04c683af1: │
-│ 192.168.100.10                    │   192.168.100.1   │
-└──────────────┘                    └──────────────────┘
+                     Exporter Host
+ ┌─────────┐        ┌──────────────────────────────┐
+ │   DUT   │        │                              │
+ │         │  eth   │  eth2          ┌──────────┐  │
+ │  DHCP   │◄──────►│  192.168.100.1 │ dnsmasq  │  │
+ │  client │        │  (gateway)     │ DHCP+DNS │  │
+ │         │        │                └──────────┘  │
+ │ 192.168.│        │                              │
+ │ 100.10  │        │  No forwarding, no NAT.      │
+ │         │        │  L2-isolated network only.   │
+ └─────────┘        └──────────────────────────────┘
+
+  The DUT can reach the exporter on 192.168.100.1 but has
+  no route to the LAN or internet. Useful for pure L2
+  isolation or when routing is handled externally.
 ```
 
 ## Troubleshooting
@@ -250,10 +282,10 @@ sysctl net.ipv4.conf.<upstream>.forwarding
 
 ## Running Tests
 
-Integration tests require root privileges:
+Integration tests require root privileges through passwordless sudo, or direct root access:
 
 ```shell
-sudo make pkg-test-dut-network
+make pkg-test-dut-network
 ```
 
 Tests use veth pairs and network namespaces to simulate the DUT without real hardware.

python/packages/jumpstarter-driver-dut-network/jumpstarter_driver_dut_network/driver.py

@@ -184,7 +184,7 @@ def cleanup(self) -> None:
         self.logger.info("Cleaning up DUT network configuration")
 
         if self._dnsmasq_process:
-            dnsmasq.stop(process=self._dnsmasq_process)
+            dnsmasq.stop(process=self._dnsmasq_process, state_dir=self._state_path)
             self._dnsmasq_process = None
 
         nftables.flush_rules(self._table_name)

python/packages/jumpstarter-driver-dut-network/jumpstarter_driver_dut_network/driver_test.py

@@ -6,6 +6,7 @@
 
 import os
 import re
+import shlex
 import shutil
 import subprocess
 import sys
@@ -27,25 +28,27 @@
 requires_dig = pytest.mark.skipif(not shutil.which("dig"), reason="dig not found")
 requires_dhclient = pytest.mark.skipif(not shutil.which("dhclient"), reason="dhclient not found")
 
-_SUDO_PREFIX = "" if os.getuid() == 0 else "sudo "
+_SUDO_CMD: list[str] = ["sudo"] if os.getuid() != 0 else []
 
 
 def _run(cmd: str, check: bool = True, ns: str | None = None) -> subprocess.CompletedProcess:
-    """Run a shell command with sudo when not root, optionally inside a network namespace."""
+    """Run a command with sudo when not root, optionally inside a network namespace."""
+    args = shlex.split(cmd)
     if ns:
-        cmd = f"{_SUDO_PREFIX}ip netns exec {ns} {cmd}"
+        args = [*_SUDO_CMD, "ip", "netns", "exec", ns, *args]
     else:
-        cmd = f"{_SUDO_PREFIX}{cmd}"
-    return subprocess.run(cmd, shell=True, capture_output=True, text=True, check=check)
+        args = [*_SUDO_CMD, *args]
+    return subprocess.run(args, capture_output=True, text=True, check=check)
 
 
 def _popen(cmd: str, ns: str | None = None) -> subprocess.Popen:
     """Start a background process, optionally inside a network namespace."""
+    args = shlex.split(cmd)
     if ns:
-        cmd = f"{_SUDO_PREFIX}ip netns exec {ns} {cmd}"
+        args = [*_SUDO_CMD, "ip", "netns", "exec", ns, *args]
     else:
-        cmd = f"{_SUDO_PREFIX}{cmd}"
-    return subprocess.Popen(cmd, shell=True, stdout=subprocess.PIPE, stderr=subprocess.PIPE)
+        args = [*_SUDO_CMD, *args]
+    return subprocess.Popen(args, stdout=subprocess.PIPE, stderr=subprocess.PIPE)
 
 
 def _can_nat_between_namespaces() -> bool:
@@ -144,10 +147,21 @@ def _can_nat_between_namespaces() -> bool:
         _run(f"ip netns del {dst_ns}", check=False)
 
 
-_nat_works = _can_nat_between_namespaces()
-requires_nat = pytest.mark.skipif(
-    not _nat_works, reason="nftables NAT between namespaces not available"
-)
+_nat_works: bool | None = None
+
+
+def _nat_probe() -> bool:
+    global _nat_works
+    if _nat_works is None:
+        _nat_works = _can_nat_between_namespaces()
+    return bool(_nat_works)
+
+
+@pytest.fixture(scope="session")
+def nat_available() -> None:
+    """Run the NAT probe once per session (at test time, not import time)."""
+    if not _nat_probe():
+        pytest.skip("nftables NAT between namespaces not available")  # type: ignore[misc]
 
 
 class NetworkTestEnv:
@@ -310,7 +324,7 @@ def test_dnsmasq_stopped_on_cleanup(self, net_env: NetworkTestEnv):
 @requires_privileges
 @requires_nft
 @requires_dnsmasq
-@requires_nat
+@pytest.mark.usefixtures("nat_available")
 class TestMasqueradeNat:
     """Test masquerade NAT rules."""
 
@@ -653,7 +667,7 @@ def test_remove_dns_entry_via_rpc(self, net_env: NetworkTestEnv):
 @requires_privileges
 @requires_nft
 @requires_dnsmasq
-@requires_nat
+@pytest.mark.usefixtures("nat_available")
 class TestOneToOneNatDataPlane:
     """Test actual data-plane connectivity through 1:1 NAT (not just rule presence)."""
 
@@ -725,7 +739,7 @@ def test_exporter_can_reach_dut_via_public_ip(self, net_env: NetworkTestEnv):
 @requires_privileges
 @requires_nft
 @requires_dnsmasq
-@requires_nat
+@pytest.mark.usefixtures("nat_available")
 class TestDisabledNatIsolation:
     """Test that disabled NAT prevents routing while still allowing local access."""
 
@@ -772,7 +786,7 @@ def test_dut_can_still_reach_gateway(self, net_env: NetworkTestEnv):
 @requires_privileges
 @requires_nft
 @requires_dnsmasq
-@requires_nat
+@pytest.mark.usefixtures("nat_available")
 class TestTcpConnectivity:
     """Test TCP connectivity through masquerade NAT (not just ICMP ping)."""