feat(k3s): harden Kyber storage lifecycle - #2134
Conversation
|
|
You do not have enough credits to review this pull request. Please purchase more credits to continue. |
|
Bugbot is not enabled for your account, so this pull request was not reviewed. Enable Bugbot in the Cursor dashboard to get automatic reviews on future PRs. |
📝 WalkthroughSummary by CodeRabbit
WalkthroughKyber containerd storage now uses a pinned filesystem UUID. Kubelet and journald limits are added, while SMART and periodic host-health monitoring are packaged, installed, enabled, and tested through Home Manager. ChangesKyber storage and reliability
Estimated code review effort: 4 (Complex) | ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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. Comment |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces host reliability monitoring, log limits, and storage validation for the Kyber k3s environment. It transitions the containerd SSD mount from label-based to UUID-based identification, adds scripts and systemd services for health checks and SMART disk monitoring, and configures native log rotation and journald limits. The review feedback highlights critical improvements to ensure script robustness under set -e, specifically recommending the use of findmnt instead of blkid to avoid permission issues during unprivileged Home Manager activation, and adding a guard for /proc/pressure/io to prevent premature script termination.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| mounted_source="$(@findmnt@ --noheadings --output SOURCE --target "$MOUNT_POINT")" | ||
| mounted_uuid="$(@blkid@ --match-tag UUID --output value "$mounted_source")" |
There was a problem hiding this comment.
Running blkid on a block device as a non-root user during Home Manager activation will fail or return an empty string due to permission restrictions on raw block devices. Since Home Manager activation runs as the unprivileged user, this will cause the activation script to abort or fail with an unexpected UUID error.
Using findmnt to retrieve the UUID directly is safe, does not require root privileges, and avoids the dependency on blkid entirely.
| mounted_source="$(@findmnt@ --noheadings --output SOURCE --target "$MOUNT_POINT")" | |
| mounted_uuid="$(@blkid@ --match-tag UUID --output value "$mounted_source")" | |
| mounted_uuid="$(@findmnt@ --noheadings --output UUID --target "$MOUNT_POINT")" |
| awk = "${pkgs.gawk}/bin/awk"; | ||
| blkid = "${pkgs.util-linux}/bin/blkid"; |
There was a problem hiding this comment.
Since we can retrieve the UUID directly using findmnt in activate.sh without requiring root privileges, the blkid dependency is no longer needed here.
awk = "${pkgs.gawk}/bin/awk";
| check_io_pressure() { | ||
| local some_avg300 full_avg300 |
There was a problem hiding this comment.
If /proc/pressure/io does not exist (e.g., on kernels where PSI is disabled or unsupported), the awk command will fail. Because set -e is enabled, this failure will prematurely abort the entire health check script, preventing subsequent checks from running.
Adding a guard to check for the existence of /proc/pressure/io ensures the script handles this state gracefully.
| check_io_pressure() { | |
| local some_avg300 full_avg300 | |
| check_io_pressure() { | |
| if [ ! -f /proc/pressure/io ]; then | |
| return 0 | |
| fi | |
| local some_avg300 full_avg300 |
References
- When 'set -e' is enabled in Bash scripts, ensure that individual command failures do not prematurely abort the entire script. Handle potential failures gracefully.
| mounted_source="$(findmnt --noheadings --output SOURCE --target "$CONTAINERD_MOUNT")" | ||
| mounted_uuid="$(blkid --match-tag UUID --output value "$mounted_source")" | ||
| if [ "$mounted_uuid" != "$EXPECTED_CONTAINERD_UUID" ]; then | ||
| set_alert "image-filesystem" "$CONTAINERD_MOUNT has UUID $mounted_uuid, expected $EXPECTED_CONTAINERD_UUID" | ||
| return | ||
| fi |
There was a problem hiding this comment.
Using blkid to query the UUID of the mounted source device can be fragile if the device is temporarily inaccessible or if the blkid cache is stale.
We can retrieve the UUID directly and robustly using findmnt --output UUID, which also handles potential command failures gracefully with || true to prevent set -e from aborting the script.
| mounted_source="$(findmnt --noheadings --output SOURCE --target "$CONTAINERD_MOUNT")" | |
| mounted_uuid="$(blkid --match-tag UUID --output value "$mounted_source")" | |
| if [ "$mounted_uuid" != "$EXPECTED_CONTAINERD_UUID" ]; then | |
| set_alert "image-filesystem" "$CONTAINERD_MOUNT has UUID $mounted_uuid, expected $EXPECTED_CONTAINERD_UUID" | |
| return | |
| fi | |
| mounted_uuid="$(findmnt --noheadings --output UUID --target "$CONTAINERD_MOUNT" 2>/dev/null || true)" | |
| if [ -z "$mounted_uuid" ] || [ "$mounted_uuid" != "$EXPECTED_CONTAINERD_UUID" ]; then | |
| set_alert "image-filesystem" "$CONTAINERD_MOUNT has UUID ${mounted_uuid:-unknown}, expected $EXPECTED_CONTAINERD_UUID" | |
| return | |
| fi |
References
- When 'set -e' is enabled in Bash scripts, ensure that individual command failures do not prematurely abort the entire script. Handle potential failures gracefully.
![indent-zero[bot]](https://avatars.githubusercontent.com/in/1426746?s=60&v=4){kind=small}
|
|
||
| if @findmnt@ --mountpoint "$MOUNT_POINT" >/dev/null 2>&1; then | ||
| mounted_source="$(@findmnt@ --noheadings --output SOURCE --target "$MOUNT_POINT")" | ||
| mounted_uuid="$(@blkid@ --match-tag UUID --output value "$mounted_source")" |
There was a problem hiding this comment.
Bare blkid will read empty for non-root users on Ubuntu, tripping the guard.
On Ubuntu the containerd block device (/dev/sda*) is brw-rw---- root disk, and blkid only returns cached info for devices readable by the invoking user. Since home-manager activation runs as the user, this call typically returns an empty string, and the check on line 113 then compares "" != "90f29a7b-..." — which is true — so activation aborts with unexpected containerd filesystem UUID: (empty), even when the disk is correctly mounted.
The same script already reaches for run_sudo @tune2fs@ -l "$root_source" in configure_root_ext4_reserve for exactly this reason, and prepare-containerd-disk.sh uses sudo blkid for the equivalent probe. Suggest:
if @findmnt@ --mountpoint "$MOUNT_POINT" >/dev/null 2>&1; then
mounted_source="$(@findmnt@ --noheadings --output SOURCE --target "$MOUNT_POINT")"
require_sudo || exit 0
mounted_uuid="$(run_sudo @blkid@ --match-tag UUID --output value "$mounted_source")"
...
fiThe existing spec assertion grep -q 'mounted_uuid=.*@blkid@' in spec/k3s_service_activate_spec.sh still matches after the change.
| fi | ||
|
|
||
| if [ "$d_state_count" -ge "$D_STATE_THRESHOLD" ]; then | ||
| samples=$((previous_samples + 1)) |
There was a problem hiding this comment.
Latent: corrupted state file crashes the health check.
previous_samples is read from /run/kyber-host-health/d-state.samples and fed directly into $((previous_samples + 1)). If that file ever contains non-numeric content (a partial write, a manual edit, or a truncated line), arithmetic evaluation fails and, under set -euo pipefail at the top of the file, the whole script exits non-zero — no subsequent check runs, and since the monitor is Type=oneshot the timer simply logs a failed unit and moves on. On tmpfs the file is normally rewritten on every run, so this is dormant today, but any interrupted write leaves the reliability monitor silently disabled until reboot.
A one-line guard makes this resilient:
if [ -r "$count_file" ]; then
read -r previous_samples <"$count_file" || previous_samples=0
[[ "$previous_samples" =~ ^[0-9]+$ ]] || previous_samples=0
fi
Mesa DescriptionTL;DRHardens Kyber host storage lifecycle and reliability by pinning the dedicated containerd storage device to its UUID and implementing proactive host health monitoring, SMART disk daemon configs, journald log limits, and tuned Kubelet garbage collection/eviction thresholds. What changed?
Description generated by Mesa. Update settings |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@config/k3s/kyber-host-health.sh`:
- Around line 38-44: Update check_io_pressure to test whether /proc/pressure/io
is readable before invoking either awk command; if it is unavailable, return
successfully from the function so set -e does not terminate the script and
subsequent health checks still run.
- Around line 82-84: Update the mounted_uuid assignment in the health-check
script to tolerate blkid failure by appending the requested non-failing
fallback. Preserve an empty mounted_uuid so the existing comparison against
EXPECTED_CONTAINERD_UUID triggers the mismatch alert instead of terminating
under set -e.
In `@home-manager/services/k3s/activate.sh`:
- Around line 53-66: Update sync_root_file so both run_sudo mkdir and run_sudo
cp explicitly append “|| return 1”, ensuring command failures propagate even
when the function is used as an if condition; retain the existing successful
return 0 behavior.
- Around line 141-156: Initialize systemd_changed to 0 before the
systemd_file_pair loop. Keep sync_root_file setting it to 1 when any systemd
file changes, so the daemon-reload condition remains unchanged and idempotent
runs do not fail under set -u.
- Around line 110-119: Update the mounted filesystem UUID lookup in the
mount-check block to invoke blkid through the existing run_sudo helper, ensuring
it runs with root privileges while preserving the current UUID comparison and
refusal behavior.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: d998676f-d9b4-415f-8294-f21e8ab22814
📒 Files selected for processing (16)
config/k3s/containerd.mountconfig/k3s/default.nixconfig/k3s/journald.confconfig/k3s/kubelet.confconfig/k3s/kyber-host-alert.shconfig/k3s/kyber-host-health.serviceconfig/k3s/kyber-host-health.shconfig/k3s/kyber-host-health.timerconfig/k3s/kyber-smartd.confconfig/k3s/kyber-smartd.servicehome-manager/services/k3s/activate.shhome-manager/services/k3s/default.nixnamed-hosts/kyber/README.mdnamed-hosts/kyber/prepare-containerd-disk.shspec/coverage_spec.shspec/k3s_service_activate_spec.sh
| check_io_pressure() { | ||
| local some_avg300 full_avg300 | ||
|
|
||
| # shellcheck disable=SC2016 | ||
| some_avg300="$(awk '$1 == "some" { for (i = 1; i <= NF; i++) if ($i ~ /^avg300=/) { sub(/^avg300=/, "", $i); print $i } }' /proc/pressure/io)" | ||
| # shellcheck disable=SC2016 | ||
| full_avg300="$(awk '$1 == "full" { for (i = 1; i <= NF; i++) if ($i ~ /^avg300=/) { sub(/^avg300=/, "", $i); print $i } }' /proc/pressure/io)" |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
Gracefully handle missing PSI file to prevent premature script failure.
If the system lacks PSI capabilities or /proc/pressure/io is not readable, awk will throw a fatal error. Due to set -e, this will cause the entire health check script to crash, preventing subsequent checks (like check_d_state and check_image_filesystem) from running.
Add a readability check to return early if the file does not exist.
🛠️ Proposed fix
check_io_pressure() {
+ if [ ! -r /proc/pressure/io ]; then
+ return 0
+ fi
local some_avg300 full_avg300
# shellcheck disable=SC2016📝 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.
| check_io_pressure() { | |
| local some_avg300 full_avg300 | |
| # shellcheck disable=SC2016 | |
| some_avg300="$(awk '$1 == "some" { for (i = 1; i <= NF; i++) if ($i ~ /^avg300=/) { sub(/^avg300=/, "", $i); print $i } }' /proc/pressure/io)" | |
| # shellcheck disable=SC2016 | |
| full_avg300="$(awk '$1 == "full" { for (i = 1; i <= NF; i++) if ($i ~ /^avg300=/) { sub(/^avg300=/, "", $i); print $i } }' /proc/pressure/io)" | |
| check_io_pressure() { | |
| if [ ! -r /proc/pressure/io ]; then | |
| return 0 | |
| fi | |
| local some_avg300 full_avg300 | |
| # shellcheck disable=SC2016 | |
| some_avg300="$(awk '$1 == "some" { for (i = 1; i <= NF; i++) if ($i ~ /^avg300=/) { sub(/^avg300=/, "", $i); print $i } }' /proc/pressure/io)" | |
| # shellcheck disable=SC2016 | |
| full_avg300="$(awk '$1 == "full" { for (i = 1; i <= NF; i++) if ($i ~ /^avg300=/) { sub(/^avg300=/, "", $i); print $i } }' /proc/pressure/io)" |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@config/k3s/kyber-host-health.sh` around lines 38 - 44, Update
check_io_pressure to test whether /proc/pressure/io is readable before invoking
either awk command; if it is unavailable, return successfully from the function
so set -e does not terminate the script and subsequent health checks still run.
| mounted_source="$(findmnt --noheadings --output SOURCE --target "$CONTAINERD_MOUNT")" | ||
| mounted_uuid="$(blkid --match-tag UUID --output value "$mounted_source")" | ||
| if [ "$mounted_uuid" != "$EXPECTED_CONTAINERD_UUID" ]; then |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
Prevent script termination if the filesystem lacks a UUID.
If the mounted device lacks a UUID (e.g., due to corruption or unexpected filesystem type), blkid will exit with status 2. Under set -e, this will crash the script before it can trigger the alert or proceed to other health checks.
Append || true so that an empty mounted_uuid correctly triggers the mismatch alert.
🛠️ Proposed fix
mounted_source="$(findmnt --noheadings --output SOURCE --target "$CONTAINERD_MOUNT")"
- mounted_uuid="$(blkid --match-tag UUID --output value "$mounted_source")"
+ mounted_uuid="$(blkid --match-tag UUID --output value "$mounted_source" || true)"
if [ "$mounted_uuid" != "$EXPECTED_CONTAINERD_UUID" ]; then📝 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.
| mounted_source="$(findmnt --noheadings --output SOURCE --target "$CONTAINERD_MOUNT")" | |
| mounted_uuid="$(blkid --match-tag UUID --output value "$mounted_source")" | |
| if [ "$mounted_uuid" != "$EXPECTED_CONTAINERD_UUID" ]; then | |
| mounted_source="$(findmnt --noheadings --output SOURCE --target "$CONTAINERD_MOUNT")" | |
| mounted_uuid="$(blkid --match-tag UUID --output value "$mounted_source" || true)" | |
| if [ "$mounted_uuid" != "$EXPECTED_CONTAINERD_UUID" ]; then |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@config/k3s/kyber-host-health.sh` around lines 82 - 84, Update the
mounted_uuid assignment in the health-check script to tolerate blkid failure by
appending the requested non-failing fallback. Preserve an empty mounted_uuid so
the existing comparison against EXPECTED_CONTAINERD_UUID triggers the mismatch
alert instead of terminating under set -e.
| sync_root_file() { | ||
| local source="$1" | ||
| local target="$2" | ||
|
|
||
| if [ ! -f "$source" ] || @diff@ -q "$source" "$target" >/dev/null 2>&1; then | ||
| return 1 | ||
| fi | ||
|
|
||
| require_sudo || return 1 | ||
| run_sudo mkdir -p "$(dirname "$target")" | ||
| run_sudo cp -f "$source" "$target" | ||
| return 0 | ||
| } | ||
|
|
There was a problem hiding this comment.
🎯 Functional Correctness | 🔴 Critical | ⚡ Quick win
Propagate command failures in sync_root_file.
Because sync_root_file is evaluated as the condition of an if statement later in the script (e.g. if sync_root_file ...; then), Bash temporarily suspends set -e execution inside the function body. If run_sudo mkdir or run_sudo cp fails, the script will not abort and will silently proceed to return 0, falsely reporting a successful synchronization.
Append || return 1 to ensure failures are correctly propagated.
🐛 Proposed fix
require_sudo || return 1
- run_sudo mkdir -p "$(dirname "$target")"
- run_sudo cp -f "$source" "$target"
- return 0
+ run_sudo mkdir -p "$(dirname "$target")" || return 1
+ run_sudo cp -f "$source" "$target" || return 1
+ return 0
}📝 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.
| sync_root_file() { | |
| local source="$1" | |
| local target="$2" | |
| if [ ! -f "$source" ] || @diff@ -q "$source" "$target" >/dev/null 2>&1; then | |
| return 1 | |
| fi | |
| require_sudo || return 1 | |
| run_sudo mkdir -p "$(dirname "$target")" | |
| run_sudo cp -f "$source" "$target" | |
| return 0 | |
| } | |
| sync_root_file() { | |
| local source="$1" | |
| local target="$2" | |
| if [ ! -f "$source" ] || `@diff`@ -q "$source" "$target" >/dev/null 2>&1; then | |
| return 1 | |
| fi | |
| require_sudo || return 1 | |
| run_sudo mkdir -p "$(dirname "$target")" || return 1 | |
| run_sudo cp -f "$source" "$target" || return 1 | |
| return 0 | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@home-manager/services/k3s/activate.sh` around lines 53 - 66, Update
sync_root_file so both run_sudo mkdir and run_sudo cp explicitly append “||
return 1”, ensuring command failures propagate even when the function is used as
an if condition; retain the existing successful return 0 behavior.
| if @findmnt@ --mountpoint "$MOUNT_POINT" >/dev/null 2>&1; then | ||
| mounted_source="$(@findmnt@ --noheadings --output SOURCE --target "$MOUNT_POINT")" | ||
| mounted_uuid="$(@blkid@ --match-tag UUID --output value "$mounted_source")" | ||
| if [ "$mounted_uuid" != "$EXPECTED_CONTAINERD_UUID" ]; then | ||
| echo "Refusing to run k3s with unexpected containerd filesystem UUID: $mounted_uuid" >&2 | ||
| echo "Expected $EXPECTED_CONTAINERD_UUID at $MOUNT_POINT" >&2 | ||
| exit 1 | ||
| fi | ||
| fi | ||
|
|
There was a problem hiding this comment.
🎯 Functional Correctness | 🔴 Critical | ⚡ Quick win
Run blkid with root privileges.
By default, reading block device attributes with blkid requires root privileges. Because this script executes as a normal user during Home Manager activation, blkid will fail with an exit code of 2 and produce empty output. The set -e policy will immediately catch the failure inside the command substitution and abort the activation.
🐛 Proposed fix to use `run_sudo`
if `@findmnt`@ --mountpoint "$MOUNT_POINT" >/dev/null 2>&1; then
mounted_source="$(`@findmnt`@ --noheadings --output SOURCE --target "$MOUNT_POINT")"
- mounted_uuid="$(`@blkid`@ --match-tag UUID --output value "$mounted_source")"
+ mounted_uuid="$(run_sudo `@blkid`@ --match-tag UUID --output value "$mounted_source")"
if [ "$mounted_uuid" != "$EXPECTED_CONTAINERD_UUID" ]; then📝 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.
| if @findmnt@ --mountpoint "$MOUNT_POINT" >/dev/null 2>&1; then | |
| mounted_source="$(@findmnt@ --noheadings --output SOURCE --target "$MOUNT_POINT")" | |
| mounted_uuid="$(@blkid@ --match-tag UUID --output value "$mounted_source")" | |
| if [ "$mounted_uuid" != "$EXPECTED_CONTAINERD_UUID" ]; then | |
| echo "Refusing to run k3s with unexpected containerd filesystem UUID: $mounted_uuid" >&2 | |
| echo "Expected $EXPECTED_CONTAINERD_UUID at $MOUNT_POINT" >&2 | |
| exit 1 | |
| fi | |
| fi | |
| if `@findmnt`@ --mountpoint "$MOUNT_POINT" >/dev/null 2>&1; then | |
| mounted_source="$(`@findmnt`@ --noheadings --output SOURCE --target "$MOUNT_POINT")" | |
| mounted_uuid="$(run_sudo `@blkid`@ --match-tag UUID --output value "$mounted_source")" | |
| if [ "$mounted_uuid" != "$EXPECTED_CONTAINERD_UUID" ]; then | |
| echo "Refusing to run k3s with unexpected containerd filesystem UUID: $mounted_uuid" >&2 | |
| echo "Expected $EXPECTED_CONTAINERD_UUID at $MOUNT_POINT" >&2 | |
| exit 1 | |
| fi | |
| fi |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@home-manager/services/k3s/activate.sh` around lines 110 - 119, Update the
mounted filesystem UUID lookup in the mount-check block to invoke blkid through
the existing run_sudo helper, ensuring it runs with root privileges while
preserving the current UUID comparison and refusal behavior.
| for systemd_file_pair in \ | ||
| "$MOUNT_FILE:$SYSTEM_MOUNT" \ | ||
| "$SERVICE_FILE:$SYSTEM_SERVICE" \ | ||
| "$HEALTH_SERVICE_FILE:$SYSTEM_HEALTH_SERVICE" \ | ||
| "$HEALTH_TIMER_FILE:$SYSTEM_HEALTH_TIMER" \ | ||
| "$SMARTD_SERVICE_FILE:$SYSTEM_SMARTD_SERVICE"; do | ||
| source_file="${systemd_file_pair%%:*}" | ||
| target_file="${systemd_file_pair#*:}" | ||
| if sync_root_file "$source_file" "$target_file"; then | ||
| systemd_changed=1 | ||
| fi | ||
| done | ||
|
|
||
| if [ "$systemd_changed" -eq 1 ]; then | ||
| run_sudo @systemctl@ daemon-reload | ||
| fi |
There was a problem hiding this comment.
🎯 Functional Correctness | 🔴 Critical | ⚡ Quick win
Initialize systemd_changed to avoid an unbound variable crash.
If none of the systemd files require synchronization, the for loop completes without assigning a value to systemd_changed. Evaluating [ "$systemd_changed" -eq 1 ] will then trigger an "unbound variable" error because of the set -u policy, crashing the script during steady-state (idempotent) runs.
🐛 Proposed fix
+systemd_changed=0
for systemd_file_pair in \
"$MOUNT_FILE:$SYSTEM_MOUNT" \📝 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.
| for systemd_file_pair in \ | |
| "$MOUNT_FILE:$SYSTEM_MOUNT" \ | |
| "$SERVICE_FILE:$SYSTEM_SERVICE" \ | |
| "$HEALTH_SERVICE_FILE:$SYSTEM_HEALTH_SERVICE" \ | |
| "$HEALTH_TIMER_FILE:$SYSTEM_HEALTH_TIMER" \ | |
| "$SMARTD_SERVICE_FILE:$SYSTEM_SMARTD_SERVICE"; do | |
| source_file="${systemd_file_pair%%:*}" | |
| target_file="${systemd_file_pair#*:}" | |
| if sync_root_file "$source_file" "$target_file"; then | |
| systemd_changed=1 | |
| fi | |
| done | |
| if [ "$systemd_changed" -eq 1 ]; then | |
| run_sudo @systemctl@ daemon-reload | |
| fi | |
| systemd_changed=0 | |
| for systemd_file_pair in \ | |
| "$MOUNT_FILE:$SYSTEM_MOUNT" \ | |
| "$SERVICE_FILE:$SYSTEM_SERVICE" \ | |
| "$HEALTH_SERVICE_FILE:$SYSTEM_HEALTH_SERVICE" \ | |
| "$HEALTH_TIMER_FILE:$SYSTEM_HEALTH_TIMER" \ | |
| "$SMARTD_SERVICE_FILE:$SYSTEM_SMARTD_SERVICE"; do | |
| source_file="${systemd_file_pair%%:*}" | |
| target_file="${systemd_file_pair#*:}" | |
| if sync_root_file "$source_file" "$target_file"; then | |
| systemd_changed=1 | |
| fi | |
| done | |
| if [ "$systemd_changed" -eq 1 ]; then | |
| run_sudo `@systemctl`@ daemon-reload | |
| fi |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@home-manager/services/k3s/activate.sh` around lines 141 - 156, Initialize
systemd_changed to 0 before the systemd_file_pair loop. Keep sync_root_file
setting it to 1 when any systemd file changes, so the daemon-reload condition
remains unchanged and idempotent runs do not fail under set -u.
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
7 issues found across 16 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="home-manager/services/k3s/activate.sh">
<violation number="1" location="home-manager/services/k3s/activate.sh:62">
P1: A failed privileged copy is reported as success, so activation can reload and enable stale or missing K3s/health/SMART units after a disk-full or permission failure. Make each install command terminate the activation on failure rather than falling through to `return 0`.</violation>
<violation number="2" location="home-manager/services/k3s/activate.sh:112">
P1: This UUID probe uses `@blkid@` before privilege escalation. During user-scoped activation, `blkid` can return no data or non-zero, which can trigger `set -e` or an empty-value mismatch and fail activation even when the mount is correct. Running this probe via `run_sudo` after `require_sudo` avoids that failure mode.</violation>
<violation number="3" location="home-manager/services/k3s/activate.sh:171">
P2: Updated SMART monitoring configuration never takes effect while `kyber-smartd.service` is already active: `enable --now` starts inactive units but does not restart the running smartd process. Track a changed smartd unit and restart it after `daemon-reload` (while preserving first-install start behavior).</violation>
</file>
<file name="config/k3s/kyber-host-health.sh">
<violation number="1" location="config/k3s/kyber-host-health.sh:42">
P2: Guard `/proc/pressure/io` before running `awk`. On hosts without PSI support, this command exits non-zero and `set -e` stops the health script before the remaining checks run.</violation>
<violation number="2" location="config/k3s/kyber-host-health.sh:63">
P3: Validate `previous_samples` before arithmetic. A malformed state file can break this calculation and terminate the health run early under `set -e`.</violation>
<violation number="3" location="config/k3s/kyber-host-health.sh:83">
P2: An unreadable or unsupported mounted source makes `blkid` terminate this script instead of producing the image-filesystem alert. Treat an empty UUID as a mismatch so the failed identity check remains visible and later health checks still run.</violation>
<violation number="4" location="config/k3s/kyber-host-health.sh:101">
P2: A hung `k3s crictl` that ignores or blocks TERM can keep this health service active past its intended 15-second bound, suppressing subsequent timer probes. Add a short kill-after grace period to enforce a real upper limit.</violation>
</file>
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
| fi | ||
|
|
||
| require_sudo || return 1 | ||
| run_sudo mkdir -p "$(dirname "$target")" |
There was a problem hiding this comment.
P1: A failed privileged copy is reported as success, so activation can reload and enable stale or missing K3s/health/SMART units after a disk-full or permission failure. Make each install command terminate the activation on failure rather than falling through to return 0.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At home-manager/services/k3s/activate.sh, line 62:
<comment>A failed privileged copy is reported as success, so activation can reload and enable stale or missing K3s/health/SMART units after a disk-full or permission failure. Make each install command terminate the activation on failure rather than falling through to `return 0`.</comment>
<file context>
@@ -41,6 +50,20 @@ require_sudo() {
+ fi
+
+ require_sudo || return 1
+ run_sudo mkdir -p "$(dirname "$target")"
+ run_sudo cp -f "$source" "$target"
+ return 0
</file context>
|
|
||
| if @findmnt@ --mountpoint "$MOUNT_POINT" >/dev/null 2>&1; then | ||
| mounted_source="$(@findmnt@ --noheadings --output SOURCE --target "$MOUNT_POINT")" | ||
| mounted_uuid="$(@blkid@ --match-tag UUID --output value "$mounted_source")" |
There was a problem hiding this comment.
P1: This UUID probe uses @blkid@ before privilege escalation. During user-scoped activation, blkid can return no data or non-zero, which can trigger set -e or an empty-value mismatch and fail activation even when the mount is correct. Running this probe via run_sudo after require_sudo avoids that failure mode.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At home-manager/services/k3s/activate.sh, line 112:
<comment>This UUID probe uses `@blkid@` before privilege escalation. During user-scoped activation, `blkid` can return no data or non-zero, which can trigger `set -e` or an empty-value mismatch and fail activation even when the mount is correct. Running this probe via `run_sudo` after `require_sudo` avoids that failure mode.</comment>
<file context>
@@ -84,6 +107,16 @@ configure_root_ext4_reserve() {
+if @findmnt@ --mountpoint "$MOUNT_POINT" >/dev/null 2>&1; then
+ mounted_source="$(@findmnt@ --noheadings --output SOURCE --target "$MOUNT_POINT")"
+ mounted_uuid="$(@blkid@ --match-tag UUID --output value "$mounted_source")"
+ if [ "$mounted_uuid" != "$EXPECTED_CONTAINERD_UUID" ]; then
+ echo "Refusing to run k3s with unexpected containerd filesystem UUID: $mounted_uuid" >&2
</file context>
| mounted_uuid="$(@blkid@ --match-tag UUID --output value "$mounted_source")" | |
| require_sudo || exit 0 | |
| mounted_uuid="$(run_sudo @blkid@ --match-tag UUID --output value "$mounted_source")" |
| run_sudo @systemctl@ enable --now k3s | ||
|
|
||
| if [ -f "$SMARTD_SERVICE_FILE" ]; then | ||
| run_sudo @systemctl@ enable --now kyber-smartd.service |
There was a problem hiding this comment.
P2: Updated SMART monitoring configuration never takes effect while kyber-smartd.service is already active: enable --now starts inactive units but does not restart the running smartd process. Track a changed smartd unit and restart it after daemon-reload (while preserving first-install start behavior).
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At home-manager/services/k3s/activate.sh, line 171:
<comment>Updated SMART monitoring configuration never takes effect while `kyber-smartd.service` is already active: `enable --now` starts inactive units but does not restart the running smartd process. Track a changed smartd unit and restart it after `daemon-reload` (while preserving first-install start behavior).</comment>
<file context>
@@ -129,6 +167,14 @@ fi
run_sudo @systemctl@ enable --now k3s
+if [ -f "$SMARTD_SERVICE_FILE" ]; then
+ run_sudo @systemctl@ enable --now kyber-smartd.service
+fi
+
</file context>
| fi | ||
|
|
||
| mounted_source="$(findmnt --noheadings --output SOURCE --target "$CONTAINERD_MOUNT")" | ||
| mounted_uuid="$(blkid --match-tag UUID --output value "$mounted_source")" |
There was a problem hiding this comment.
P2: An unreadable or unsupported mounted source makes blkid terminate this script instead of producing the image-filesystem alert. Treat an empty UUID as a mismatch so the failed identity check remains visible and later health checks still run.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At config/k3s/kyber-host-health.sh, line 83:
<comment>An unreadable or unsupported mounted source makes `blkid` terminate this script instead of producing the image-filesystem alert. Treat an empty UUID as a mismatch so the failed identity check remains visible and later health checks still run.</comment>
<file context>
@@ -0,0 +1,121 @@
+ fi
+
+ mounted_source="$(findmnt --noheadings --output SOURCE --target "$CONTAINERD_MOUNT")"
+ mounted_uuid="$(blkid --match-tag UUID --output value "$mounted_source")"
+ if [ "$mounted_uuid" != "$EXPECTED_CONTAINERD_UUID" ]; then
+ set_alert "image-filesystem" "$CONTAINERD_MOUNT has UUID $mounted_uuid, expected $EXPECTED_CONTAINERD_UUID"
</file context>
| mounted_uuid="$(blkid --match-tag UUID --output value "$mounted_source")" | |
| mounted_uuid="$(blkid --match-tag UUID --output value "$mounted_source" 2>/dev/null || true)" |
| local started_at finished_at latency_seconds error_count | ||
|
|
||
| started_at="$(date +%s)" | ||
| if ! timeout 15 k3s crictl info >/dev/null 2>&1; then |
There was a problem hiding this comment.
P2: A hung k3s crictl that ignores or blocks TERM can keep this health service active past its intended 15-second bound, suppressing subsequent timer probes. Add a short kill-after grace period to enforce a real upper limit.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At config/k3s/kyber-host-health.sh, line 101:
<comment>A hung `k3s crictl` that ignores or blocks TERM can keep this health service active past its intended 15-second bound, suppressing subsequent timer probes. Add a short kill-after grace period to enforce a real upper limit.</comment>
<file context>
@@ -0,0 +1,121 @@
+ local started_at finished_at latency_seconds error_count
+
+ started_at="$(date +%s)"
+ if ! timeout 15 k3s crictl info >/dev/null 2>&1; then
+ set_alert "cri-health" "k3s crictl info failed or exceeded 15 seconds"
+ return
</file context>
| if ! timeout 15 k3s crictl info >/dev/null 2>&1; then | |
| if ! timeout -k 1s 15s k3s crictl info >/dev/null 2>&1; then |
| local some_avg300 full_avg300 | ||
|
|
||
| # shellcheck disable=SC2016 | ||
| some_avg300="$(awk '$1 == "some" { for (i = 1; i <= NF; i++) if ($i ~ /^avg300=/) { sub(/^avg300=/, "", $i); print $i } }' /proc/pressure/io)" |
There was a problem hiding this comment.
P2: Guard /proc/pressure/io before running awk. On hosts without PSI support, this command exits non-zero and set -e stops the health script before the remaining checks run.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At config/k3s/kyber-host-health.sh, line 42:
<comment>Guard `/proc/pressure/io` before running `awk`. On hosts without PSI support, this command exits non-zero and `set -e` stops the health script before the remaining checks run.</comment>
<file context>
@@ -0,0 +1,121 @@
+ local some_avg300 full_avg300
+
+ # shellcheck disable=SC2016
+ some_avg300="$(awk '$1 == "some" { for (i = 1; i <= NF; i++) if ($i ~ /^avg300=/) { sub(/^avg300=/, "", $i); print $i } }' /proc/pressure/io)"
+ # shellcheck disable=SC2016
+ full_avg300="$(awk '$1 == "full" { for (i = 1; i <= NF; i++) if ($i ~ /^avg300=/) { sub(/^avg300=/, "", $i); print $i } }' /proc/pressure/io)"
</file context>
| fi | ||
|
|
||
| if [ "$d_state_count" -ge "$D_STATE_THRESHOLD" ]; then | ||
| samples=$((previous_samples + 1)) |
There was a problem hiding this comment.
P3: Validate previous_samples before arithmetic. A malformed state file can break this calculation and terminate the health run early under set -e.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At config/k3s/kyber-host-health.sh, line 63:
<comment>Validate `previous_samples` before arithmetic. A malformed state file can break this calculation and terminate the health run early under `set -e`.</comment>
<file context>
@@ -0,0 +1,121 @@
+ fi
+
+ if [ "$d_state_count" -ge "$D_STATE_THRESHOLD" ]; then
+ samples=$((previous_samples + 1))
+ fi
+ printf '%s\n' "$samples" >"$count_file"
</file context>
1 participant
Summary
Validation
git diff --checkNotes
No live host switch is included. Full ShellSpec has one unrelated sandbox-only failure in the existing security-sync test when it attempts to rewrite
config/shared/hooks/security.sh. Full Kyber activation evaluation is also blocked on this aarch64-darwin host by the existing x86_64-linux Obsidian headless IFD; the focused K3s activation expression evaluates successfully.Summary by cubic
Hardens
k3son Kyber by pinning the containerd SSD to a specific filesystem UUID, tightening image GC/eviction policy, and adding read‑only host reliability checks and log caps. The node now fails closed on storage identity issues and preserves headroom to prevent CRI stalls.New Features
containerdimage storage by UUID and refuses to runk3sif the mounted UUID is missing or mismatched.kubelet: serializes image pulls, sets image GC to 70/60, enforces 20% free onnodefsandimagefs, and rotates container logs (10Mi, 3 files).kubeletremains the sole CRI GC owner.smartdfor SMART self-tests and a minute-by-minutekyber-host-healthcheck for I/O PSI, sustained D-state, imagefs usage/UUID, and CRI latency/errors. Alerts go to the journal andwall; checks are read-only.Migration
named-hosts/kyber/prepare-containerd-disk.shbefore switching, then verify withfindmnt -n -o UUID /var/lib/rancher/k3s/agent/containerd.k3swill not start until corrected.containerdruntime data lives on the SSD.Written for commit 2717a48. Summary will update on new commits.