Add a transaction-overlayfs helper script to the Docker images#706
Add a transaction-overlayfs helper script to the Docker images#706
transaction-overlayfs helper script to the Docker images#706Conversation
Signed-off-by: Juan Cruz Viotti <jv@jviotti.com>
🤖 Augment PR SummarySummary: This PR ships a new Docker helper, Changes:
Technical Notes: The script uses kernel overlayfs ( 🤖 Was this summary useful? React with 👍 or 👎 |
| mkdir -p "$OVERLAY_UPPER" "$OVERLAY_WORK" "$OVERLAY_MERGED" | ||
|
|
||
| TIME_START=$(date +%s%3N) | ||
| mount -t overlay overlay \ |
There was a problem hiding this comment.
With set -o errexit, failures after a successful mount (e.g., umount busy, cp error, etc.) can exit the script without cleaning up $OVERLAY_DIR (and potentially without unmounting). Consider a trap to always attempt umount and remove the temp directory on any exit path.
Severity: medium
🤖 Was this useful? React with 👍 or 👎, or 🚀 if it prevented an incident/outage.
| fi | ||
|
|
||
| TIME_START=$(date +%s%3N) | ||
| cp -a "$OVERLAY_UPPER"/. "$OUTPUT_DIR/" |
There was a problem hiding this comment.
The “merge” step cp -a "$OVERLAY_UPPER"/. "$OUTPUT_DIR"/ won’t apply overlayfs deletion/whiteout semantics, so files deleted during the transaction may remain in $OUTPUT_DIR (or whiteout entries may be copied through). That can break the intended transactional behavior for deletes/renames.
Severity: high
🤖 Was this useful? React with 👍 or 👎, or 🚀 if it prevented an incident/outage.
There was a problem hiding this comment.
3 issues found across 3 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="docker/transaction-overlayfs.sh">
<violation number="1" location="docker/transaction-overlayfs.sh:33">
P2: With `set -o errexit` enabled, if any command fails after the overlayfs is mounted (e.g., `umount` returns busy, `cp` encounters an error), the script will exit immediately without cleaning up `$OVERLAY_DIR` or unmounting the filesystem. Add a `trap` handler (e.g., `trap 'umount "$OVERLAY_MERGED" 2>/dev/null; rm -rf "$OVERLAY_DIR"' EXIT`) after the mount succeeds to ensure cleanup on all exit paths.</violation>
<violation number="2" location="docker/transaction-overlayfs.sh:50">
P1: The argument-rewrite loop reorders arguments by appending each processed argument to the end of the remaining list, so the command receives a different argument order than the caller provided.</violation>
<violation number="3" location="docker/transaction-overlayfs.sh:76">
P2: Copying only the upper layer does not propagate deletions from the overlay, so removed files remain in the output directory after a successful transaction.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
| shift | ||
| if [ "$CURRENT" = "$OUTPUT_DIR" ] | ||
| then | ||
| set -- "$@" "$OVERLAY_MERGED" |
There was a problem hiding this comment.
P1: The argument-rewrite loop reorders arguments by appending each processed argument to the end of the remaining list, so the command receives a different argument order than the caller provided.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At docker/transaction-overlayfs.sh, line 50:
<comment>The argument-rewrite loop reorders arguments by appending each processed argument to the end of the remaining list, so the command receives a different argument order than the caller provided.</comment>
<file context>
@@ -0,0 +1,83 @@
+ shift
+ if [ "$CURRENT" = "$OUTPUT_DIR" ]
+ then
+ set -- "$@" "$OVERLAY_MERGED"
+ else
+ set -- "$@" "$CURRENT"
</file context>
There was a problem hiding this comment.
1 issue found across 2 files (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="docker/transaction-overlayfs.sh">
<violation number="1" location="docker/transaction-overlayfs.sh:91">
P1: `find -type c` matches **all** character devices, not just overlayfs whiteouts (major:minor `0:0`). If the wrapped command creates a legitimate character device, it will be mistakenly deleted from both the upper layer and `OUTPUT_DIR`. Filter by device number to match only actual whiteouts.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
| find "$OVERLAY_UPPER" -type c | while read -r WHITEOUT_PATH | ||
| do | ||
| RELATIVE_PATH="${WHITEOUT_PATH#$OVERLAY_UPPER/}" | ||
| rm -rf "${OUTPUT_DIR:?}/$RELATIVE_PATH" | ||
| rm "$WHITEOUT_PATH" | ||
| done |
There was a problem hiding this comment.
P1: find -type c matches all character devices, not just overlayfs whiteouts (major:minor 0:0). If the wrapped command creates a legitimate character device, it will be mistakenly deleted from both the upper layer and OUTPUT_DIR. Filter by device number to match only actual whiteouts.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At docker/transaction-overlayfs.sh, line 91:
<comment>`find -type c` matches **all** character devices, not just overlayfs whiteouts (major:minor `0:0`). If the wrapped command creates a legitimate character device, it will be mistakenly deleted from both the upper layer and `OUTPUT_DIR`. Filter by device number to match only actual whiteouts.</comment>
<file context>
@@ -60,24 +75,26 @@ echo "transaction(overlayfs): command took $((TIME_END - TIME_START))ms" >&2
+
+# Process overlayfs whiteouts (file deletions) in the upper layer.
+# Whiteouts are character devices with major:minor 0:0
+find "$OVERLAY_UPPER" -type c | while read -r WHITEOUT_PATH
+do
+ RELATIVE_PATH="${WHITEOUT_PATH#$OVERLAY_UPPER/}"
</file context>
| find "$OVERLAY_UPPER" -type c | while read -r WHITEOUT_PATH | |
| do | |
| RELATIVE_PATH="${WHITEOUT_PATH#$OVERLAY_UPPER/}" | |
| rm -rf "${OUTPUT_DIR:?}/$RELATIVE_PATH" | |
| rm "$WHITEOUT_PATH" | |
| done | |
| find "$OVERLAY_UPPER" -type c | while read -r WHITEOUT_PATH | |
| do | |
| # Verify this is actually a whiteout (major:minor 0:0) | |
| MAJOR_MINOR=$(stat -c '%t:%T' "$WHITEOUT_PATH" 2>/dev/null) || continue | |
| [ "$MAJOR_MINOR" = "0:0" ] || continue | |
| RELATIVE_PATH="${WHITEOUT_PATH#$OVERLAY_UPPER/}" | |
| rm -rf "${OUTPUT_DIR:?}/$RELATIVE_PATH" | |
| rm "$WHITEOUT_PATH" | |
| done |
There was a problem hiding this comment.
Benchmark Index (community)
Details
| Benchmark suite | Current: 9e3009e | Previous: 2899731 | Ratio |
|---|---|---|---|
Add one schema (0 existing) |
41 ms |
51 ms |
0.80 |
Add one schema (100 existing) |
401 ms |
438 ms |
0.92 |
Add one schema (1000 existing) |
4155 ms |
4472 ms |
0.93 |
This comment was automatically generated by workflow using github-action-benchmark.
c9dbc8d to
9e3009e
Compare
There was a problem hiding this comment.
Benchmark Index (enterprise)
Details
| Benchmark suite | Current: 9e3009e | Previous: 2899731 | Ratio |
|---|---|---|---|
Add one schema (0 existing) |
44 ms |
50 ms |
0.88 |
Add one schema (100 existing) |
413 ms |
415 ms |
1.00 |
Add one schema (1000 existing) |
4129 ms |
4460 ms |
0.93 |
This comment was automatically generated by workflow using github-action-benchmark.
Signed-off-by: Juan Cruz Viotti jv@jviotti.com