optimize systemctl commands#531
Conversation
WalkthroughThe Go module version in Changes
Sequence Diagram(s)sequenceDiagram
participant Caller
participant HandlerSystemd
participant systemctl
Caller->>HandlerSystemd: CreateJob(job, schedules, permission)
HandlerSystemd->>HandlerSystemd: Get timer filename
alt Conflicting units exist
HandlerSystemd->>HandlerSystemd: disableJob(job, unitType, timerFile)
HandlerSystemd->>HandlerSystemd: removeJobFiles(job, unitType, timerFile, serviceFile)
end
HandlerSystemd->>HandlerSystemd: Write unit files
HandlerSystemd->>HandlerSystemd: runSystemctlOnUnit(timerName, "enable", unitType, silent, extraArgs)
HandlerSystemd->>HandlerSystemd: runSystemctlOnUnit(timerName, "start", unitType, silent, extraArgs)
alt reload flag set
HandlerSystemd->>systemctl: daemon-reload
end
Caller->>HandlerSystemd: RemoveJob(job, permission)
HandlerSystemd->>HandlerSystemd: disableJob(job, unitType, timerFile)
HandlerSystemd->>HandlerSystemd: removeJobFiles(job, unitType, timerFile, serviceFile)
alt reload flag set
HandlerSystemd->>systemctl: daemon-reload
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (3)
✅ Files skipped from review due to trivial changes (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (5)
🔇 Additional comments (1)
✨ Finishing Touches
🧪 Generate unit tests
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:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. 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)
schedule/handler_systemd.go (1)
234-261: Consider collecting all removal errors.The
removeJobFilesmethod handles file removal well, but currently only logs errors without returning them. Consider collecting all errors to provide comprehensive feedback.func (h *HandlerSystemd) removeJobFiles(job *Config, unitType systemd.UnitType, timerFile, serviceFile string) error { var err error + var errors []error unit := systemd.NewUnit(user.Current()) systemdPath := systemd.GetSystemDir() if unitType == systemd.UserUnit { systemdPath, err = unit.GetUserDir() if err != nil { return nil } } dropInDir := systemd.GetServiceFileDropInDir(job.ProfileName, job.CommandName) timerDropInDir := systemd.GetTimerFileDropInDir(job.ProfileName, job.CommandName) obsoletes := []string{ path.Join(systemdPath, timerFile), path.Join(systemdPath, serviceFile), path.Join(systemdPath, timerDropInDir), path.Join(systemdPath, dropInDir), } for _, pathToRemove := range obsoletes { if err = os.RemoveAll(pathToRemove); err != nil { clog.Errorf("failed removing %q, error: %s. Please remove this path", pathToRemove, err.Error()) + errors = append(errors, fmt.Errorf("failed to remove %s: %w", pathToRemove, err)) } } - return nil + if len(errors) > 0 { + return fmt.Errorf("failed to remove %d file(s)", len(errors)) + } + return nil }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
go.mod(1 hunks)schedule/handler_systemd.go(7 hunks)
🧰 Additional context used
🧠 Learnings (2)
go.mod (1)
Learnt from: creativeprojects
PR: #425
File: schedule/handler_windows.go:97-118
Timestamp: 2025-02-04T14:38:07.701Z
Learning: The shell.SplitArguments function in the resticprofile project returns only []string and does not return any error.
schedule/handler_systemd.go (1)
Learnt from: creativeprojects
PR: #425
File: systemd/read.go:87-91
Timestamp: 2025-02-04T21:32:13.942Z
Learning: In systemd package, error handling for integer conversion from service files is unnecessary since these files are generated programmatically by the software itself, ensuring clean data.
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: Build and test (1.24, macos-latest)
- GitHub Check: Build and test (1.24, windows-latest)
- GitHub Check: Build and test (1.24, ubuntu-latest)
- GitHub Check: Analyze (go)
🔇 Additional comments (7)
go.mod (1)
3-3: Go version confirmed as valid
The module specifiesgo 1.24.5, which is the latest stable release as of July 2025 and is recommended for production. No changes required.schedule/handler_systemd.go (6)
118-118: Good refactoring - early variable assignment improves readability.Moving the
timerFileassignment to the beginning of the method reduces repeated calls tosystemd.GetTimerFileand makes the code cleaner.
130-137: Well-structured separation of concerns.The refactoring to use
disableJobandremoveJobFileshelper methods improves modularity and makes the code more maintainable. The error messages clearly indicate the specific operation that failed.
173-179: Excellent optimisation - combining enable and start operations.The use of
--quietreduces verbose output and--nowefficiently combines the enable and start operations into a single systemctl call, aligning perfectly with the PR objectives.
191-222: Clean refactoring with proper systemd notification.The refactored
RemoveJobmethod now properly separates concerns and explicitly callsdaemon-reloadto ensure systemd is aware of the configuration changes. This is a significant improvement in reliability.
224-232: Efficient implementation of the disable operation.The
disableJobhelper method efficiently combines stop and disable operations using the--nowflag, reducing the number of systemctl calls as intended by the PR objectives.
388-421: Excellent refactoring of systemctl command execution.The renamed
runSystemctlOnUnitfunction with variadicextraArgsprovides a flexible and unified interface for all systemctl operations. This improves code consistency throughout the file.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #531 +/- ##
==========================================
- Coverage 79.29% 79.27% -0.02%
==========================================
Files 136 136
Lines 13264 13295 +31
==========================================
+ Hits 10517 10539 +22
- Misses 2326 2337 +11
+ Partials 421 419 -2
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
schedule/handler_systemd.go(6 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
schedule/handler_systemd.go (3)
systemd/generate.go (7)
GetTimerFile(279-281)SystemUnit(71-71)UserUnit(70-70)GetServiceFile(264-266)Config(93-113)UnitType(66-66)NewUnit(120-125)schedule/config.go (1)
Config(10-27)user/user_other.go (1)
Current(11-41)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: Build and test (1.24, ubuntu-latest)
- GitHub Check: Build and test (1.24, windows-latest)
- GitHub Check: Analyze (go)
- GitHub Check: build
🔇 Additional comments (7)
schedule/handler_systemd.go (7)
118-119: LGTM! Good caching optimisation.Caching the timer file name early avoids repeated calls to
systemd.GetTimerFile()throughout the method, improving readability and performance.
172-179: LGTM! Conditional reload implementation is correct.The conditional systemd reload based on the "reload" flag provides users with control over when daemon-reload is executed, which aligns with the PR objectives to make this operation optional.
180-194: Excellent consolidation of enable and start operations.The refactoring successfully combines the enable and start operations using the
--nowflag whenno-startis not set, reducing the number of systemctl invocations as intended by the PR objectives.
213-232: LGTM! Clean refactoring of RemoveJob method.The method now properly uses the helper methods
disableJobandremoveJobFiles, and includes conditional reload behaviour. The logic flow is clear and maintains proper error handling.
234-242: LGTM! Well-implemented disableJob helper method.The method correctly combines stop and disable operations using the
--nowflag with--quietfor cleaner output, aligning with the PR objectives.
244-271: LGTM! Proper file cleanup implementation.The
removeJobFilesmethod correctly handles the removal of timer files, service files, and drop-in directories. The error logging for failed removals is appropriate, allowing the operation to continue even if some files cannot be removed.
398-431: LGTM! Enhanced function signature with variadic arguments.The renamed
runSystemctlOnUnitfunction now accepts variadic extra arguments, providing the flexibility needed for different systemctl operations whilst maintaining backward compatibility with existing calls.
…andler - Implemented the --reload flag in createSchedule to set the reload flag for jobs. - Updated the systemd handler to manage reload hooks and execute daemon-reload when necessary. - Improved documentation for keyfile generation and schedule commands for clarity.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
schedule/handler_systemd.go (1)
236-273: Address inconsistent error handling in helper methods.The
removeJobFilesmethod logs errors but always returnsnil, whilstdisableJobpropagates errors. This inconsistency makes it difficult to diagnose failures during unit removal.Based on the past review feedback, apply these fixes:
-func (h *HandlerSystemd) removeJobFiles(job *Config, unitType systemd.UnitType, timerFile, serviceFile string) error { +func (h *HandlerSystemd) removeJobFiles(job *Config, unitType systemd.UnitType, timerFile, serviceFile string) error { // ... existing code ... for _, pathToRemove := range obsoletes { - if err = os.RemoveAll(pathToRemove); err != nil { - clog.Errorf("failed removing %q, error: %s. Please remove this path", pathToRemove, err.Error()) - } + if err := os.RemoveAll(pathToRemove); err != nil { + return fmt.Errorf("failed to remove %q: %w", pathToRemove, err) + } } return nil } func (h *HandlerSystemd) disableJob(job *Config, unitType systemd.UnitType, timerFile string) error { - err := runSystemctlOnUnit(timerFile, systemctlDisable, unitType, job.removeOnly, flagNow, flagQuiet) - if err != nil { - return err - } + if err := runSystemctlOnUnit(timerFile, systemctlDisable, unitType, job.removeOnly, flagNow, flagQuiet); err != nil { + return fmt.Errorf("failed to disable unit %q: %w", timerFile, err) + } return nil }
🧹 Nitpick comments (2)
docs/content/usage/keyfile.md (1)
10-14: Improve readability by varying sentence structure.The static analysis tool correctly identifies repetitive sentence beginnings with "On" which creates monotonous reading. Consider restructuring for better flow.
- On Linux, FreeBSD, Dragonfly, and Solaris, Reader uses `getrandom(2)`. - On legacy Linux (< 3.17), it uses `/dev/urandom`. - On macOS, and OpenBSD Reader, uses `arc4random_buf(3)`. - On NetBSD, Reader uses the kern.arandom sysctl. - On Windows, Reader uses the ProcessPrng API. + Linux, FreeBSD, Dragonfly, and Solaris use `getrandom(2)`. + Legacy Linux (< 3.17) uses `/dev/urandom`. + macOS and OpenBSD use `arc4random_buf(3)`. + NetBSD uses the `kern.arandom` sysctl. + Windows uses the ProcessPrng API.docs/content/schedules/commands.md (1)
14-14: Consider using "before" instead of "prior to" for conciseness.The static analysis tool suggests that "prior to" might be wordy.
- In versions prior to `0.29.0`, the `--name` flag for a group scheduled all profiles in the group individually, as if the schedule command was run on each profile. + In versions before `0.29.0`, the `--name` flag for a group scheduled all profiles in the group individually, as if the schedule command was run on each profile.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
commands_schedule.go(1 hunks)docs/content/schedules/commands.md(2 hunks)docs/content/usage/keyfile.md(1 hunks)schedule/handler_systemd.go(10 hunks)
🧰 Additional context used
🧠 Learnings (2)
schedule/handler_systemd.go (1)
Learnt from: creativeprojects
PR: #425
File: systemd/read.go:87-91
Timestamp: 2025-02-04T21:32:13.942Z
Learning: In systemd package, error handling for integer conversion from service files is unnecessary since these files are generated programmatically by the software itself, ensuring clean data.
docs/content/schedules/commands.md (1)
Learnt from: creativeprojects
PR: #459
File: schtasks/schtasks.go:29-30
Timestamp: 2025-02-14T22:53:42.689Z
Learning: In the schtasks package, tasksPath is defined as a constant with value \resticprofile backup\ in taskscheduler.go. It's used as a prefix for managing task paths in the Windows Task Scheduler.
🪛 LanguageTool
docs/content/schedules/commands.md
[style] ~14-~14: ‘prior to’ might be wordy. Consider a shorter alternative.
Context: ...otice style="warning" %}} In versions prior to 0.29.0, the --name flag for a group...
(EN_WORDINESS_PREMIUM_PRIOR_TO)
docs/content/usage/keyfile.md
[style] ~12-~12: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ...nux (< 3.17), it uses /dev/urandom. - On macOS, and OpenBSD Reader, uses `arc4ra...
(ENGLISH_WORD_REPEAT_BEGINNING_RULE)
[style] ~13-~13: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ...BSD Reader, uses arc4random_buf(3). - On NetBSD, Reader uses the kern.arandom sy...
(ENGLISH_WORD_REPEAT_BEGINNING_RULE)
[uncategorized] ~13-~13: Possible missing article found.
Context: ... uses arc4random_buf(3). - On NetBSD, Reader uses the kern.arandom sysctl. - On Wind...
(AI_HYDRA_LEO_MISSING_THE)
[style] ~14-~14: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ... Reader uses the kern.arandom sysctl. - On Windows, Reader uses the ProcessPrng AP...
(ENGLISH_WORD_REPEAT_BEGINNING_RULE)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (5)
- GitHub Check: Build and test (1.24, ubuntu-latest)
- GitHub Check: Build and test (1.24, macos-latest)
- GitHub Check: Analyze (go)
- GitHub Check: Build and test (1.24, windows-latest)
- GitHub Check: build
🔇 Additional comments (8)
docs/content/usage/keyfile.md (1)
8-8: Documentation clarity improved.The rephrasing from the previous verbose description to a more concise form improves readability whilst maintaining accuracy.
commands_schedule.go (1)
59-63: LGTM! Consistent implementation of the reload flag.The implementation correctly follows the established pattern used for the
--no-startflag, ensuring consistent behaviour across command-line arguments. The flag is properly propagated to all scheduled jobs.docs/content/schedules/commands.md (1)
38-38: Excellent documentation of the new reload flag.The addition clearly explains the purpose of the
--reloadflag, its availability from version 0.32.0, and the specific systemd issue it addresses. This will help users understand when to use this flag.schedule/handler_systemd.go (5)
74-83: Good implementation of deferred daemon-reload mechanism.The addition of the
Close()method with deferred daemon-reload hooks effectively addresses the PR objective of making daemon-reload conditional. This reduces unnecessary systemctl calls whilst ensuring reloads happen when needed.
188-197: Excellent consolidation of systemctl commands.The use of
--nowflag with enable/disable operations effectively achieves the PR objective of reducing systemctl command calls by combining start/stop with enable/disable operations.
180-186: Conditional reload implementation looks good.The conditional reload based on the job's "reload" flag is well-implemented and allows immediate reloading when explicitly requested, complementing the deferred reload mechanism.
406-439: Well-designed unified systemctl command execution.The
runSystemctlOnUnitfunction with variadicextraArgsparameter provides flexible command execution whilst maintaining consistency. This is a good abstraction that supports both the consolidated commands and various flag combinations.
358-362: Clean implementation of reload hook management.The
addReloadHookmethod correctly prevents duplicate entries usingslices.Contains, ensuring each unit type is only reloaded once during cleanup.
|
1 participant
Optimize systemd scheduler with deferred daemon-reload and consolidated commands
This PR optimizes systemd scheduling operations through several key improvements:
Performance Optimizations:
systemctl enable --nowandsystemctl disable --nowto combine enable/start and stop/disable operations into single commandssystemctl daemon-reloadwhen explicitly requested via--reloadflag or when removing jobs, avoiding unnecessary reloads during job creationEnhanced --reload Flag Support:
--reloadflag support toschedulecommandCode Structure Improvements:
disableJob()andremoveJobFiles()helper methods for better maintainabilityThese changes reduce the number of systemctl invocations and improve performance when managing systemd units, while maintaining backward compatibility and proper error handling.