Use shell script instead of OS Agent for device wipe #3916
Conversation
Use simple shell script to perform device wipe instead of calling OS Agent to do that through the UDisks2 API. While it might have been a good idea to use high level interface for that back then, it turns out it causes more issues than the benefits it could bring. Main problem currently is that the OS Agent needs to read sysctl variables, but those are only set after mounting the overlay partition. But at the same time, the overlay partition can't be mounted if we want to wipe it - this creates a dependency cycle through the haos-agent.service. To get rid of the cycle and simplify things, use a shell script doing basically the same what the OS Agent does. Since the wipe functionality only makes sense to be implemented on HAOS targets (not on Supervised), there's little point of having it in higher layer of abstraction that OS Agent provides. It should be also checked if changes from #1291 are needed anymore, as the driving factor for those have been probably the wipe feature in OS Agent too, but at this point they seem to be harmless.
📝 WalkthroughWalkthroughThis pull request updates the behavior of the Changes
Sequence Diagram(s)sequenceDiagram
participant Sysd as Systemd
participant Service as haos-wipe.service
participant Script as haos-wipe script
participant Overlay as Overlay Partition
participant Data as Data Partition
participant Boot as /mnt/boot/cmdline.txt
Sysd->>Service: Start haos-wipe.service (after mnt-boot.mount)
Service->>Script: Execute /usr/libexec/haos-wipe
Script->>Overlay: Verify overlay partition (existence & unmounted)
alt Overlay check fails
Script-->>Service: Exit with error
else Overlay valid
Script->>Data: Verify data partition (existence & unmounted)
alt Data check fails
Script-->>Service: Exit with error
else Data valid
Script->>Data: Format data partition (ext4 options)
Script->>Overlay: Format overlay partition (ext4 options)
Script->>Boot: Remove wipe flag from cmdline.txt
Script-->>Service: Complete execution
end
end
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:
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: 0
🧹 Nitpick comments (1)
buildroot-external/rootfs-overlay/usr/libexec/haos-wipe (1)
30-30: Consider adding error handling for the cmdline.txt modification.The script assumes
/mnt/boot/cmdline.txtexists and is writable. Consider adding a check to verify the file exists before attempting to modify it.- /usr/bin/sed -i 's/\s*haos.wipe=1//g' /mnt/boot/cmdline.txt + if [ -f /mnt/boot/cmdline.txt ]; then + /usr/bin/sed -i 's/\s*haos.wipe=1//g' /mnt/boot/cmdline.txt + else + echo "[ERROR] Unable to find cmdline.txt in boot partition" + exit 1 + fi
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
buildroot-external/rootfs-overlay/usr/lib/systemd/system/haos-wipe.service(1 hunks)buildroot-external/rootfs-overlay/usr/libexec/haos-wipe(1 hunks)
🔇 Additional comments (6)
buildroot-external/rootfs-overlay/usr/libexec/haos-wipe (3)
1-31: The haos-wipe script looks robust and well-structured.The script follows good practices with proper error checking and clear messages. It verifies partitions exist and aren't mounted before wiping them, which is critical for safety.
7-21: Good safety checks before formatting partitions.The script correctly verifies both existence and mount status of partitions before attempting to format them, preventing potential data loss or system corruption.
24-27:❓ Verification inconclusive
Verify the formatting parameters align with system requirements.
The script uses specific formatting parameters like
-I 256for the overlay partition and optimization flags. Make sure these match the expected filesystem requirements for your system.
🏁 Script executed:
#!/bin/bash # Check existing partition settings to ensure new formatting matches requirements echo "Checking current overlay partition settings:" if [ -b /dev/disk/by-label/hassos-overlay ]; then dumpe2fs -h /dev/disk/by-label/hassos-overlay 2>/dev/null | grep -E "Inode size:|Filesystem features:" fi echo "Checking current data partition settings:" if [ -b /dev/disk/by-label/hassos-data ]; then dumpe2fs -h /dev/disk/by-label/hassos-data 2>/dev/null | grep -E "Inode size:|Filesystem features:" fiLength of output: 282
Action: Validate Filesystem Formatting Parameters
The mkfs.ext4 commands currently use parameters such as
-I 256(for the overlay partition) along with optimization flags. The verification script did not return partition details—most likely because neither/dev/disk/by-label/hassos-overlaynor/dev/disk/by-label/hassos-dataexists in the test environment. This means we couldn’t automatically confirm that the specified formatting options match your system's current filesystem requirements.
- Please manually verify in your production or staging environment that using
-I 256along with the lazy initialization flags is correct for the hassos-overlay partition.- Ensure that similar optimization flags for the hassos-data partition align with expected filesystem features.
- Revisit the conditions for the existence of these partitions in the script to avoid silent failures if they are missing.
buildroot-external/rootfs-overlay/usr/lib/systemd/system/haos-wipe.service (3)
7-7: Appropriate dependency change for the wipe service.Changing from
After=haos-agent.servicetoAfter=mnt-boot.mountcorrectly resolves the dependency cycle mentioned in the PR objectives. This ensures the service starts after the boot partition is mounted but doesn't require the agent.
13-13: Simplified ExecStart command improves maintainability.Replacing the busctl command with a direct script execution makes the service cleaner and easier to understand. This approach properly separates concerns by moving all wipe logic to the dedicated script.
1-17:❓ Verification inconclusive
Verify that all needed dependencies are still preserved.
The removal of the
Requires=haos-agent.servicedependency is intentional as part of migrating away from the OS Agent. However, ensure that the service still has all necessary dependencies for proper operation.
🏁 Script executed:
#!/bin/bash # Check dependencies and ordering in the new service setup echo "Verifying service ordering:" if systemctl cat mnt-boot.mount &>/dev/null; then echo "Boot mount service exists." else echo "Warning: mnt-boot.mount service not found in test environment." fi echo "Checking partition mounting services:" if systemctl cat mnt-data.mount mnt-overlay.mount &>/dev/null; then echo "Partition mount services exist." else echo "Warning: mount services not found in test environment." fi echo "Examining if wipe flag is used elsewhere:" grep -r "haos.wipe" --include="*.service" /usr/lib/systemd/system/ || echo "No other services reference the wipe flag"Length of output: 794
Dependency Verification for haos-wipe Service – Please Confirm Production Dependencies
The configuration intentionally removes the
Requires=haos-agent.servicedependency as part of migrating away from the OS Agent. However, the verification script indicated that the mounting services (mnt-boot.mount,mnt-data.mount, andmnt-overlay.mount) were not found in the test environment. This might be due to limitations in the test setup rather than an actual misconfiguration.
- Action Items:
- Confirm in your production (or packaging) environment that
mnt-boot.mountis available.- Verify that both
mnt-data.mountandmnt-overlay.mountare defined and activated.- Ensure that the
haos.wipe=1condition reliably triggers thehaos-wipeservice without unintended side effects.Please double-check these dependencies to ensure proper service operation.
Use simple shell script to perform device wipe instead of calling OS Agent to do that through the UDisks2 API. While it might have been a good idea to use high level interface for that back then, it turns out it causes more issues than the benefits it could bring.
Main problem currently is that the OS Agent needs to read sysctl variables, but those are only set after mounting the overlay partition. But at the same time, the overlay partition can't be mounted if we want to wipe it - this creates a dependency cycle through the haos-agent.service.
To get rid of the cycle and simplify things, use a shell script doing basically the same what the OS Agent does. Since the wipe functionality only makes sense to be implemented on HAOS targets (not on Supervised), there's little point of having it in higher layer of abstraction that OS Agent provides.
It should be also checked if changes from #1291 are needed anymore, as the driving factor for those have been probably the wipe feature in OS Agent too, but at this point they seem to be harmless.
Summary by CodeRabbit