fix: synchronize with udev via BSD flock after losetup to avoid race condition#330
fix: synchronize with udev via BSD flock after losetup to avoid race condition#330
Conversation
… condition After `losetup --find --show --partscan`, the kernel creates partition device nodes (e.g. /dev/loop7p1, /dev/loop7p2) asynchronously via udev. If code tries to mount a partition immediately after attaching, it may fail with "special device /dev/loopNpM does not exist". Fix this by adding `_wait_for_loopdev_partitions()` which: 1. Calls `udevadm settle` to flush all pending udev events 2. Polls for each expected partition device node with a configurable timeout 3. Raises ImageError if nodes don't appear within the timeout The wait is called from: - `Image.attach_loopdev()` in imagecraft/pack/image.py - `ImageService.attach_images()` in imagecraft/services/image.py (fresh attaches only) Adds unit tests for the new helper function and the integration points. Agent-Logs-Url: https://github.com/canonical/imagecraft/sessions/dfb57e1c-b428-4e70-b2b1-83a7428353fc Co-authored-by: lengau <4305943+lengau@users.noreply.github.com>
…rtition number helper - Replace broad `except Exception` with `except (subprocess.CalledProcessError, OSError)` to be precise about what failures are expected from udevadm settle - Extract duplicated partition-number calculation into `_volume_partition_nums()` helper function and import it into services/image.py to avoid code duplication Agent-Logs-Url: https://github.com/canonical/imagecraft/sessions/dfb57e1c-b428-4e70-b2b1-83a7428353fc Co-authored-by: lengau <4305943+lengau@users.noreply.github.com>
|
This misattributes the reason for the issue and thus is not the correct fix. It's not a race with udev triggers, it's a race with udev's inotify watch. As explained in https://systemd.io/BLOCK_DEVICE_LOCKING/ the correct fix is to flock th device containing the partition while doing anything (such as mounting) the partition itself. |
| emit.debug( | ||
| f"Attached image {self.disk_path} as loop device {self.loop_device}" | ||
| ) | ||
| _wait_for_loopdev_partitions( |
There was a problem hiding this comment.
@copilot We should wait for the partitions before logging that we've attached the device.
There was a problem hiding this comment.
Fixed in f7b72b2 — the wait is now called before emit.debug(...) so the log only fires once all partition nodes are confirmed to exist.
| ] | ||
|
|
||
|
|
||
| def _wait_for_loopdev_partitions( |
There was a problem hiding this comment.
@copilot since this is being used in another file it can't be private.
There was a problem hiding this comment.
Fixed in f7b72b2 — renamed both _wait_for_loopdev_partitions → wait_for_loopdev_partitions and _volume_partition_nums → volume_partition_nums and updated all call sites.
…ug log - Rename _wait_for_loopdev_partitions -> wait_for_loopdev_partitions - Rename _volume_partition_nums -> volume_partition_nums (both are imported by services/image.py so they can't be private) - In Image.attach_loopdev(), call wait_for_loopdev_partitions() before emit.debug() so the log only fires once the partitions are actually ready - Update all references in services/image.py and both test files Agent-Logs-Url: https://github.com/canonical/imagecraft/sessions/065c4f4e-7f04-47a7-9ac7-f5d74ae39dfc Co-authored-by: lengau <4305943+lengau@users.noreply.github.com>
|
@copilot please address mwhudson's feedback too |
Per systemd BLOCK_DEVICE_LOCKING, the correct fix for the partition-node
race is to hold a shared BSD flock on the loop device while using its
partitions. udev holds an exclusive lock while processing; a shared lock
blocks until udev is done and then keeps it held during our operations.
- imagecraft/pack/image.py:
- Remove volume_partition_nums() and wait_for_loopdev_partitions()
- Remove _UDEVADM_BIN, _PARTITION_WAIT_TIMEOUT, time, subprocess imports
- In Image.attach_loopdev(): open loop device and acquire LOCK_SH for
the duration of the context manager
- imagecraft/services/image.py:
- Remove import of removed helpers
- Add _loop_fds list to track open file handles holding flocks
- In attach_images(): open device and acquire LOCK_SH after attaching
(both fresh attach and reuse); store fd in _loop_fds
- In detach_images(): close all fds (releasing flocks) before detaching
- tests: remove TestWaitForLoopdevPartitions; update all tests to mock
fcntl.flock and builtins.open; add test for flock release on detach
Agent-Logs-Url: https://github.com/canonical/imagecraft/sessions/1d13f3fb-01b2-4424-b470-cbc2611b70e0
Co-authored-by: lengau <4305943+lengau@users.noreply.github.com>
- Wrap fcntl.flock in try/except in attach_images() to close the fd if flock raises, preventing a resource leak - Move inline `import fcntl` statements to module level in both test files Agent-Logs-Url: https://github.com/canonical/imagecraft/sessions/1d13f3fb-01b2-4424-b470-cbc2611b70e0 Co-authored-by: lengau <4305943+lengau@users.noreply.github.com>
Done in 3814238. Replaced the udevadm settle + polling approach with proper BSD flock synchronization as described in https://systemd.io/BLOCK_DEVICE_LOCKING/:
Warning Firewall rules blocked me from connecting to one or more addresses (expand for details)I tried to connect to the following addresses, but was blocked by firewall rules:
If you need me to access, download, or install something from one of these locations, you can either:
|
| # exclusive lock while it processes the new device; taking a shared | ||
| # lock here blocks until udev is done and then holds it for the | ||
| # duration of the context so udev does not interfere with our use. | ||
| with open(self.loop_device, "rb") as loop_fd: |
There was a problem hiding this comment.
@copilot i think this is going to hold the lock for longer than we want and block udev from running on the device at all, which might not be what is wanted. In my experience it has been better to lock the device only while the partition is being mounted.
| # exclusive lock while it processes the device; a shared lock here | ||
| # blocks until udev is done and then holds it while we use the | ||
| # device's partitions, preventing udev from interfering. | ||
| loop_fd = open(attached_device, "rb") # noqa: SIM115 (held deliberately) |
There was a problem hiding this comment.
@copilot again i think this holds the lock for too long. we should only hold the lock while mounting or formatting the partition
After
losetup --find --show --partscan, udev holds an exclusive BSD flock on the whole-disk device while processing it. Mounting immediately after attach can fail withspecial device /dev/loopNpM does not existbecause udev's inotify watch hasn't finished. The correct fix per systemd BLOCK_DEVICE_LOCKING is to hold a shared flock on the loop device while performing partition operations.Changes
imagecraft/pack/image.pyImage.attach_loopdev()now opens the loop device and acquiresfcntl.LOCK_SHafter attaching — this blocks until udev releases its exclusive lock, then holds the shared lock for the full duration of the context manager so udev cannot interfere with partition operations.imagecraft/services/image.pyImageService.attach_images()opens each loop device and acquiresLOCK_SHafter attaching (for both fresh attaches and reused devices). File handles are stored in_loop_fdsand closed (releasing the lock) indetach_images(). The fd is closed onflockfailure to prevent resource leaks.Tests
fcntl.flockandbuiltins.opentest_attach_images_flock_released_on_detachto verify the shared lock is released when images are detached