systemd: improve robustness of certificate renewals#55
Conversation
|
Warning Review limit reached
Next review available in: 5 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (4)
📝 WalkthroughWalkthroughSystemd unit files for dnstapir-renew are updated across deb and rpm packaging trees. Service files gain start-limit and restart-on-failure directives; timer files replace weekly calendar scheduling with a boot delay plus a fixed Monday 12:00 Europe/Stockholm schedule. ChangesSystemd service and timer configuration
Estimated code review effort: 1 (Trivial) | ~5 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
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 |
There was a problem hiding this comment.
🧹 Nitpick comments (2)
deb/usr/lib/systemd/system/dnstapir-renew.service (1)
4-5: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDocument the retry-budget rationale.
StartLimitBurst=72withRestartSec=1hyields ~72h of hourly retries inside the 75hStartLimitIntervalSecwindow — a reasonable "keep retrying for ~3 days" policy, but the numbers are non-obvious without context. A short comment above these directives would help future maintainers understand the intended retry budget instead of having to reverse-engineer it.Also applies to: 16-17
🤖 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 `@deb/usr/lib/systemd/system/dnstapir-renew.service` around lines 4 - 5, Add a brief inline comment near the dnstapir-renew.service restart-limit settings to document the intended retry budget: explain that StartLimitBurst=72 with hourly restarts gives roughly 72 hours of retries within the 75-hour StartLimitIntervalSec window. Place it above the related StartLimitIntervalSec/StartLimitBurst directives so maintainers can understand the rationale without reverse-engineering the values.deb/usr/lib/systemd/system/dnstapir-renew.timer (1)
9-10: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winConsider
Persistent=truefor missed-run recovery.The stated goal is robustness of renewals, but the timer still lacks
Persistent=true. If the host is powered off across the Monday 12:00 window and doesn't reboot in the interim, the weekly run is simply skipped until the next Monday.OnBootSec=5monly covers hosts that reboot; always-on hosts down during the exact window get no catch-up run.Proposed addition
[Timer] OnBootSec=5m OnCalendar=Mon *-*-* 12:00:00 Europe/Stockholm +Persistent=true AccuracySec=1h RandomizedDelaySec=100min🤖 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 `@deb/usr/lib/systemd/system/dnstapir-renew.timer` around lines 9 - 10, The timer definition for dnstapir-renew should be updated to handle missed weekly runs by enabling Persistent=true alongside the existing OnBootSec and OnCalendar settings. Locate the systemd timer unit and add the persistent missed-run recovery option so the renew job is triggered after downtime even if the Monday 12:00 window was missed.
🤖 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.
Nitpick comments:
In `@deb/usr/lib/systemd/system/dnstapir-renew.service`:
- Around line 4-5: Add a brief inline comment near the dnstapir-renew.service
restart-limit settings to document the intended retry budget: explain that
StartLimitBurst=72 with hourly restarts gives roughly 72 hours of retries within
the 75-hour StartLimitIntervalSec window. Place it above the related
StartLimitIntervalSec/StartLimitBurst directives so maintainers can understand
the rationale without reverse-engineering the values.
In `@deb/usr/lib/systemd/system/dnstapir-renew.timer`:
- Around line 9-10: The timer definition for dnstapir-renew should be updated to
handle missed weekly runs by enabling Persistent=true alongside the existing
OnBootSec and OnCalendar settings. Locate the systemd timer unit and add the
persistent missed-run recovery option so the renew job is triggered after
downtime even if the Monday 12:00 window was missed.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: a985e131-2541-402a-9e62-a58fe79319e7
📒 Files selected for processing (4)
deb/usr/lib/systemd/system/dnstapir-renew.servicedeb/usr/lib/systemd/system/dnstapir-renew.timerrpm/SOURCES/dnstapir-renew.servicerpm/SOURCES/dnstapir-renew.timer
dfe7990 to
adfb434
Compare
|
Switching to using |
For background see conversation in the chat
Summary by CodeRabbit