feat(ceph): add crash auto-archive interval - #4349
Conversation
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughThe change adds resource requests and limits to the Nextcloud CronJob. It also configures the Ceph manager to archive crashes after one day. ChangesNextcloud resource configuration
Ceph crash archival configuration
Estimated code review effort: 1 (Trivial) | ~5 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
 |
|
Overall Grade |
Security Reliability Complexity Hygiene |
Code Review Summary
| Analyzer | Status | Updated (UTC) | Details |
|---|---|---|---|
| JavaScript |  |
Aug 5, 2026 10:12p.m. | Review ↗ |
| Shell | |
Aug 5, 2026 10:12p.m. | Review ↗ |
Important
AI Review is run only on demand for your team. We're only showing results of static analysis review right now. To trigger AI Review, comment @deepsourcebot review on this thread.
@@ spec.cephConfig.global @@
# ceph.rook.io/v1/CephCluster/rook-ceph/rook-ceph
! + one map entry added:
+ mgr/crash/warn_recent_interval: "86400"
|
@@ spec.values.cronjob @@
# helm.toolkit.fluxcd.io/v2/HelmRelease/default/nextcloud
! + one map entry added:
+ resources:
+ limits:
+ cpu: 1
+ memory: 2Gi
+ requests:
+ cpu: 100m
+ memory: 500Mi
@@ spec.values.cephClusterSpec.cephConfig.global @@
# helm.toolkit.fluxcd.io/v2/HelmRelease/rook-ceph/rook-ceph-cluster
! + one map entry added:
+ mgr/crash/warn_recent_interval: "86400"
|
…imit - Increase nginx.resources.limits.memory from 128Mi to 256Mi - Increase nginx.resources.requests.memory from 64Mi to 128Mi - Reduce PHP memory_limit from 8G to 512M - Add CronJob resource limits (requests: 100m/500Mi, limits: 1/2Gi) These changes address the OOMKill of nextcloud-nginx container by providing: 1. Adequate headroom for large file uploads (client_max_body_size 10G) 2. Sensible per-process PHP memory limits aligned with container constraints 3. Bounded resources for cron jobs to prevent BestEffort QoS issues
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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 `@kubernetes/apps/default/nextcloud/app/helmrelease.yaml`:
- Around line 441-447: Remove the unused resources block under the cronjob
configuration, preserving the existing cronjob.cronjob.resources settings and
using cronjob.sidecar.resources exclusively for sidecar resource limits and
requests.
In `@kubernetes/apps/rook-ceph/rook-ceph/cluster/helmrelease.yaml`:
- Around line 54-55: Correct the comment for the mgr/crash/warn_recent_interval
setting so it describes only the RECENT_CRASH warning interval, not automatic
crash archival. Do not claim this configuration archives crashes or resolves
issue `#18088`; either implement a mechanism that invokes ceph crash archive or
limit this change to the accurate warning-window description.
🪄 Autofix
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 Plus
Run ID: e5401140-ee04-442b-854a-68f14bf416c6
📒 Files selected for processing (2)
kubernetes/apps/default/nextcloud/app/helmrelease.yamlkubernetes/apps/rook-ceph/rook-ceph/cluster/helmrelease.yaml
🔗 Linked repositories identified
CodeRabbit considers these linked repositories for cross-repo context during reviews:
Tanguille/LLMKube(auto-detected)Tanguille/pr-reviewer-action(auto-detected)Tanguille/toolhive(auto-detected)Tanguille/renovate-presets(auto-detected)Tanguille/litellm-operator(auto-detected)Tanguille/2x-R9700-RDNA4-GFX1201-sglang-inference(auto-detected)
| resources: | ||
| requests: | ||
| cpu: 100m | ||
| memory: 500Mi | ||
| limits: | ||
| cpu: 1 | ||
| memory: 2Gi |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -eu
printf '%s\n' '--- target lines ---'
sed -n '425,485p' kubernetes/apps/default/nextcloud/app/helmrelease.yaml
printf '%s\n' '--- chart/version references ---'
rg -n -C 3 'nextcloud|9\.2\.5|cronjob:' kubernetes/apps/default/nextcloud/app/helmrelease.yaml kubernetes/apps/default/nextcloud -g '*.yaml' -g '*.yml' || true
printf '%s\n' '--- cronjob resource keys in repository ---'
rg -n -C 2 'cronjob(\.cronjob)?\.resources|resources:' kubernetes/apps/default/nextcloud/app/helmrelease.yamlRepository: Tanguille/cluster
Length of output: 40475
🏁 Script executed (no clone):
#!/bin/bash
set -eu
base='https://raw.githubusercontent.com/nextcloud/helm/nextcloud-9.2.5/charts/nextcloud'
printf '%s\n' '--- chart values ---'
curl -fsSL "$base/values.yaml" | sed -n '/^cronjob:/,/^[^ ]/p'
printf '%s\n' '--- chart templates containing cronjob resources ---'
for file in templates/cronjob.yaml templates/_helpers.tpl templates/deployment.yaml; do
content="$(curl -fsSL "$base/$file" || true)"
if [ -n "$content" ]; then
printf '%s\n' "--- $file ---"
printf '%s\n' "$content" | rg -n -C 4 'cronjob|resources' || true
fi
done
printf '%s\n' '--- repository chart source metadata ---'
curl -fsSL 'https://api.github.com/repos/nextcloud/helm/contents/charts/nextcloud?ref=nextcloud-9.2.5' |
jq -r '.[].name'
Length of output: 10902
🏁 Script executed:
#!/bin/bash
set -eu
python3 - <<'PY'
from pathlib import Path
from urllib.request import urlopen
release = Path("kubernetes/apps/default/nextcloud/app/helmrelease.yaml").read_text()
oci = Path("kubernetes/apps/default/nextcloud/app/ocirepository.yaml").read_text()
assert "tag: 9.2.5" in oci
base = "https://raw.githubusercontent.com/nextcloud/helm/nextcloud-9.2.5/charts/nextcloud/"
values = urlopen(base + "values.yaml").read().decode()
cronjob_template = urlopen(base + "templates/cronjob.yaml").read().decode()
deployment_template = urlopen(base + "templates/deployment.yaml").read().decode()
assert "cronjob:\n resources:" in release
assert "cronjob:\n failedJobsHistoryLimit:" in release
assert "resources:\n requests:" in release
# The CronJob template enters .Values.cronjob.cronjob before resolving .resources.
assert "{{- with .Values.cronjob.cronjob }}" in cronjob_template
assert "{{- toYaml .resources | nindent 16 }}" in cronjob_template
assert ".Values.cronjob.resources" not in cronjob_template
# The sidecar uses a different, explicit path.
assert "{{- with .Values.cronjob.sidecar }}" in deployment_template
assert "{{- toYaml .resources | nindent 12 }}" in deployment_template
print("chart=9.2.5")
print("cronjob_resources_path=cronjob.cronjob.resources")
print("sidecar_resources_path=cronjob.sidecar.resources")
print("top_level_cronjob_resources_consumed=false")
PYRepository: Tanguille/cluster
Length of output: 2441
Remove the unused cronjob.resources block.
Chart 9.2.5 reads CronJob resources from cronjob.cronjob.resources. The existing block already configures them. Use cronjob.sidecar.resources only for sidecar resources.
🤖 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 `@kubernetes/apps/default/nextcloud/app/helmrelease.yaml` around lines 441 -
447, Remove the unused resources block under the cronjob configuration,
preserving the existing cronjob.cronjob.resources settings and using
cronjob.sidecar.resources exclusively for sidecar resource limits and requests.
Source: MCP tools
| # Auto-archive Ceph crashes after 1 day (86400 seconds) | ||
| mgr/crash/warn_recent_interval: "86400" |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy lift
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -eu
printf '%s\n' '--- target file ---'
nl -ba kubernetes/apps/rook-ceph/rook-ceph/cluster/helmrelease.yaml | sed -n '35,70p'
printf '%s\n' '--- related crash settings and issue references ---'
rg -n -C 3 'warn_recent_interval|crash archive|crash.*archive|18088|archive.*crash' kubernetes .github README.md 2>/dev/null || true
printf '%s\n' '--- recent file history summary ---'
git log -5 --oneline -- kubernetes/apps/rook-ceph/rook-ceph/cluster/helmrelease.yamlRepository: Tanguille/cluster
Length of output: 1540
🌐 Web query:
Ceph mgr/crash/warn_recent_interval RECENT_CRASH archive semantics official documentation
💡 Result:
The Ceph configuration option mgr/crash/warn_recent_interval defines the time threshold used to determine whether a crash is considered "recent" [1][2]. By default, this interval is set to 2 weeks [1][3]. When a Ceph daemon crashes, the cluster reports a RECENT_CRASH health warning if there are unacknowledged crash reports that occurred within this interval [4][5]. To clear this warning and acknowledge a crash, an administrator can use the command ceph crash archive <crash-id> [5][3]. The semantics of archiving a crash report are as follows: - The crash report is no longer considered for the RECENT_CRASH health check [5][3]. - The crash report is removed from the list of new crashes (viewable via ceph crash ls-new) [5][3]. - The crash report remains in the system and is still accessible via the ceph crash ls command [5][3]. Crash reports are automatically purged from the cluster after the duration specified by the mgr/crash/retain_interval option, which defaults to 1 year [1][3].
Citations:
- 1: https://docs.ceph.com/en/latest/mgr/crash/
- 2: https://docs.ceph.com/en/latest/mgr/crash/?highlight=crash
- 3: https://docs.ceph.com/en/octopus/mgr/crash/
- 4: https://docs.redhat.com/en/documentation/red_hat_ceph_storage/9/html/troubleshooting_guide/health-messages-of-a-ceph-cluster_diag
- 5: https://docs.redhat.com/en/documentation/red_hat_ceph_storage/5/html/operations_guide/management-of-managers-using-the-ceph-orchestrator
🌐 Web query:
Rook issue 18088 automatic archive Ceph crashes
💡 Result:
The issue referred to as Rook PR #18011, titled "osd: archive crash on OSD removal," addresses a bug in the Rook-Ceph crash management logic where the automatic archiving of crash reports for removed OSDs was failing [1]. In Ceph, the RECENT_CRASH health warning persists until crash reports are acknowledged (archived), even if the daemon causing the crash has been removed [2]. Rook is designed to automatically archive these crashes when an OSD is removed to prevent stale health warnings [1][3]. The bug in issue #18011 involved an inverted guard condition in the code responsible for this process [1]: 1. The code incorrectly attempted to archive crashes only if the crash list was nil, which is the opposite of the intended logic [1]. 2. Because ceph crash ls typically returns a non-nil slice, the archiving loop was effectively unreachable, causing the RECENT_CRASH warning to persist indefinitely after OSD removals [1]. 3. The PR also addressed latent issues where attempts to archive an empty crash ID (or crashes that did not match the removed OSD) could lead to spurious errors from the Ceph manager [1]. To resolve this, the fix implemented a corrected guard (len(crash) == 0) and improved the robustness of the archive loop to ensure that all matching crash entries are correctly processed and acknowledged [1]. This ensures that once an OSD is removed, the associated RECENT_CRASH warning is cleared as expected [1].
Citations:
- 1: osd: archive crash on OSD removal rook/rook#18011
- 2: https://oneuptime.com/blog/post/2026-03-31-rook-fix-recent-crash-health-check-in-ceph/view
- 3: ceph: Add ceph archive cmd in osd removal rook/rook#7001
🏁 Script executed:
#!/bin/bash
set -eu
printf '%s\n' '--- target file ---'
awk '{printf "%5d %s\n", NR, $0}' kubernetes/apps/rook-ceph/rook-ceph/cluster/helmrelease.yaml | sed -n '35,75p'
printf '%s\n' '--- commit introducing the setting ---'
git show --format=fuller --stat --no-renames b7913a4
git show --format= --no-ext-diff b7913a4 -- kubernetes/apps/rook-ceph/rook-ceph/cluster/helmrelease.yaml
printf '%s\n' '--- Rook version and CephCluster references ---'
rg -n -C 3 'rook-ceph|v1\.20\.3|CephCluster|config:|mgr/crash' kubernetes/apps/rook-cephRepository: Tanguille/cluster
Length of output: 18961
🏁 Script executed (no clone):
#!/bin/bash
set -eu
for endpoint in \
'https://api.github.com/repos/rook/rook/issues/18088' \
'https://api.github.com/repos/rook/rook/pulls/18088' \
'https://api.github.com/repos/rook/rook/issues/18011' \
'https://api.github.com/repos/rook/rook/pulls/18011'
do
printf '\n--- %s ---\n' "$endpoint"
curl -fsSL "$endpoint" | jq '{number,title,state,html_url,body}'
done
Length of output: 7983
🏁 Script executed (no clone):
#!/bin/bash
set -eu
curl -fsSL 'https://docs.ceph.com/en/latest/mgr/crash/' |
sed -n '/warn_recent_interval/,+8p;/retain_interval/,+5p' |
head -80
Length of output: 1027
Do not describe this setting as automatic crash archival.
mgr/crash/warn_recent_interval only controls the RECENT_CRASH warning window. It does not invoke ceph crash archive or move crash reports to the archived set. This change does not implement issue #18088. Use a mechanism that invokes ceph crash archive, or update the comment and objective to describe the warning interval only.
🤖 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 `@kubernetes/apps/rook-ceph/rook-ceph/cluster/helmrelease.yaml` around lines 54
- 55, Correct the comment for the mgr/crash/warn_recent_interval setting so it
describes only the RECENT_CRASH warning interval, not automatic crash archival.
Do not claim this configuration archives crashes or resolves issue `#18088`;
either implement a mechanism that invokes ceph crash archive or limit this
change to the accurate warning-window description.
Source: MCP tools
AI Automated ReviewAnalysis engine: omniroute@http://litellm.ai.svc.cluster.local/v1 (openai) Recommendation: request_changes — the Ceph change itself is correct and correct, but the PR mixes an unrelated Nextcloud change into a feat(ceph) PR and adds a duplicate resources block. Change-by-change findingskubernetes/apps/rook-ceph/rook-ceph/cluster/helmrelease.yaml
kubernetes/apps/default/nextcloud/app/helmrelease.yaml
Standards Compliance
Tool Harness Findings
Unknowns / Needs Verification
Release notes
|
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
| cronjob: | ||
| enabled: true | ||
| type: cronjob | ||
| resources: |
There was a problem hiding this comment.
Automated finding from AI PR review.
| cronjob: | ||
| enabled: true | ||
| type: cronjob | ||
| resources: |
There was a problem hiding this comment.
Minor (style): The new cronjob.resources block duplicates the existing cronjob.cronjob.resources block with identical values; one of the two is dead config depending on the chart schema — reconcile to a single block and remove the dead one.
Automated finding from AI PR review.
PR #4349 emitted "Release notes: no release notes apply" and "Tool Harness Findings: the tool loop issued no tool calls" on a two-file Ceph change. The release-notes instruction fired on every PR regardless of whether anything was bumped; gate it on a version change and tell the reviewer to drop empty sections.
1 participant
Fix for rook/rook#18088
Summary by CodeRabbit