Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Setup systemd priority #409

Merged
merged 5 commits into from
Oct 5, 2024
Merged

Setup systemd priority #409

merged 5 commits into from
Oct 5, 2024

Conversation

creativeprojects
Copy link
Owner

Set Nice and IONice in systemd service unit from global config

Fixes #229

Copy link

coderabbitai bot commented Oct 4, 2024

Walkthrough

The pull request introduces several modifications, including an update to the .gitignore file to ignore all text files in the root directory, the addition of a new deploy-current target in the Makefile for deploying binaries, and enhancements to the Global struct in config/global.go to clarify field descriptions. A new test function, TestGeneratePriorityFields, is added in systemd/generate_test.go to validate priority configurations, while documentation updates in priority.md and systemd.md provide clearer usage instructions and new features related to resticprofile.

Changes

File Change Summary
.gitignore Added entry to ignore all text files (/*.txt).
Makefile Introduced new phony target deploy-current for building and deploying binaries.
config/global.go Updated descriptions for IONice, IONiceClass, IONiceLevel, ResticLockRetryAfter, and ResticStaleLockAge.
systemd/generate_test.go Added TestGeneratePriorityFields to validate Generate function with various priority configurations.
docs/content/configuration/priority.md Clarified that both string and numeric values are valid for the priority parameter.
docs/content/schedules/systemd.md Updated to include new features related to priority and CPU scheduling in resticprofile.

Assessment against linked issues

Objective Addressed Explanation
Ensure Nice value is correctly passed to systemd unit (229)

Possibly related PRs

  • add base url on json schema #408: The changes in the Makefile related to the generate-jsonschema target may connect with the main PR's modifications to the .gitignore file, as both involve file management and configuration aspects within the project.

Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media?

❤️ Share
🪧 Tips

Chat

There are 3 ways to chat with CodeRabbit:

  • Review comments: Directly reply to a review comment made by CodeRabbit. Example:
    • I pushed a fix in commit <commit_id>, please review it.
    • Generate unit testing code for this file.
    • Open a follow-up GitHub issue for this discussion.
  • Files and specific lines of code (under the "Files changed" tab): Tag @coderabbitai in a new review comment at the desired location with your query. Examples:
    • @coderabbitai generate unit testing code for this file.
    • @coderabbitai modularize this function.
  • PR comments: Tag @coderabbitai in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
    • @coderabbitai gather interesting stats about this repository and render them as a table. Additionally, render a pie chart showing the language distribution in the codebase.
    • @coderabbitai read src/utils.ts and generate unit testing code.
    • @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.
    • @coderabbitai help me debug CodeRabbit configuration file.

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)

  • @coderabbitai pause to pause the reviews on a PR.
  • @coderabbitai resume to resume the paused reviews.
  • @coderabbitai review to trigger an incremental review. This is useful when automatic reviews are disabled for the repository.
  • @coderabbitai full review to do a full review from scratch and review all the files again.
  • @coderabbitai summary to regenerate the summary of the PR.
  • @coderabbitai resolve resolve all the CodeRabbit review comments.
  • @coderabbitai configuration to show the current CodeRabbit configuration for the repository.
  • @coderabbitai help to get help.

Other keywords and placeholders

  • Add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.
  • Add @coderabbitai summary to generate the high-level summary at a specific location in the PR description.
  • Add @coderabbitai anywhere in the PR title to generate the title automatically.

CodeRabbit Configuration File (.coderabbit.yaml)

  • You can programmatically configure CodeRabbit by adding a .coderabbit.yaml file to the root of your repository.
  • Please see the configuration documentation for more information.
  • If your editor has YAML language server enabled, you can add the path at the top of this file to enable auto-completion and validation: # yaml-language-server: $schema=https://coderabbit.ai/integrations/schema.v2.json

Documentation and Community

  • Visit our Documentation for detailed information on how to use CodeRabbit.
  • Join our Discord Community to get help, request features, and share feedback.
  • Follow us on X/Twitter for updates and announcements.

Copy link

codecov bot commented Oct 4, 2024

Codecov Report

Attention: Patch coverage is 63.04348% with 17 lines in your changes missing coverage. Please review.

Project coverage is 72.33%. Comparing base (da03de7) to head (2c6b1e3).
Report is 1 commits behind head on master.

Files with missing lines Patch % Lines
schedule/handler_systemd.go 0.00% 17 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master     #409      +/-   ##
==========================================
- Coverage   73.09%   72.33%   -0.77%     
==========================================
  Files         121      127       +6     
  Lines       12559    12955     +396     
==========================================
+ Hits         9180     9370     +190     
- Misses       2976     3160     +184     
- Partials      403      425      +22     
Flag Coverage Δ
unittests 72.33% <63.04%> (-0.77%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 7

🧹 Outside diff range and nitpick comments (7)
docs/content/schedules/systemd.md (4)

Line range hint 66-139: Excellent addition of systemd drop-in files documentation.

The new section on systemd drop-in files is a valuable addition to the documentation. It provides clear examples in multiple configuration formats, which is helpful for users with different preferences. This feature allows for more flexible and maintainable systemd configurations.

To further improve clarity, consider adding a brief explanation of why a user might want to use drop-in files. For example:

"Drop-in files allow you to customize systemd unit configurations without modifying the main unit files. This is particularly useful for applying specific overrides or adding extra configuration options whilst maintaining the ability to easily revert changes."


226-233: Excellent implementation of Nice and IO scheduling options.

The additions of Nice, CPUSchedulingPolicy, IOSchedulingClass, and IOSchedulingPriority to the default unit file template effectively address the issue described in #229. These changes allow for proper prioritisation of the service as intended by the user's configuration.

To enhance completeness, consider adding a brief comment in the documentation explaining these new options and how they relate to the global configuration. For example:

"The Nice, CPUSchedulingPolicy, IOSchedulingClass, and IOSchedulingPriority options in the unit file template allow for fine-grained control over process scheduling and I/O priorities. These values are derived from the global configuration, ensuring that user-specified priorities are correctly applied to the systemd service."


Line range hint 252-262: Helpful addition of template variables documentation.

The new 'Template variables' section is a valuable addition to the documentation. It provides a clear list of all available variables for both unit and timer templates, which is essential for users creating custom templates.

To further improve this section, consider adding a brief description of each variable's purpose. For example:

* JobDescription   string   // Description of the backup job
* TimerDescription string   // Description of the timer
* WorkingDirectory string   // Directory where the backup command will be executed
* CommandLine      string   // The full command line to execute the backup
* OnCalendar       []string // Array of systemd calendar specifications for scheduling
* SystemdProfile   string   // Name of the systemd profile
* Nice             int      // Nice value for process priority
* Environment      []string // Array of environment variables to set

This would provide users with more context about each variable's role in the template.


Line range hint 1-262: Documentation updates align well with PR objectives.

The changes made to this document effectively address the PR objectives and the issue described in #229. The addition of Nice, CPUSchedulingPolicy, IOSchedulingClass, and IOSchedulingPriority options in the unit file template ensures that these values can be correctly applied from the global configuration.

To further clarify the connection between these changes and the PR objectives, consider adding a brief note in the 'First time schedule' or 'Template variables' section explaining that these additions allow for proper prioritisation of the backup service based on the user's configuration.

For example:

"The addition of Nice and IO scheduling options in the unit file template allows resticprofile to accurately set process and I/O priorities based on the global configuration, addressing previous issues where these settings were not correctly applied to the systemd unit."

This addition would provide context for users and explicitly tie the documentation changes to the PR's purpose.

config/global.go (2)

13-13: Approve the updated description with a minor suggestion.

The updated description for IONiceClass provides clearer information about the possible values and their meanings, which is beneficial for users. This aligns well with the PR objectives of setting IONice values.

Consider adding a brief explanation of what "ionice-class" means for users who might not be familiar with the concept. For example:

-	IONiceClass          int                 `mapstructure:"ionice-class" default:"2" range:"[1:3]" description:"Sets the unix \"ionice-class\" to apply when \"ionice\" is enabled (1=realtime, 2=best-effort, 3=idle)"`
+	IONiceClass          int                 `mapstructure:"ionice-class" default:"2" range:"[1:3]" description:"Sets the unix \"ionice-class\" (I/O scheduling class) to apply when \"ionice\" is enabled (1=realtime, 2=best-effort, 3=idle)"`

14-14: Approve the updated description with a minor suggestion.

The updated description for IONiceLevel provides clearer information about the possible values and their meanings, which is beneficial for users. This aligns well with the PR objectives of setting IONice values.

Consider adding a brief explanation of what "ionice-level" means for users who might not be familiar with the concept. For example:

-	IONiceLevel          int                 `mapstructure:"ionice-level" default:"0" range:"[0:7]" description:"Sets the unix \"ionice-level\" to apply when \"ionice\" is enabled (0=highest priority, 7=lowest priority)"`
+	IONiceLevel          int                 `mapstructure:"ionice-level" default:"0" range:"[0:7]" description:"Sets the unix \"ionice-level\" (I/O priority within the scheduling class) to apply when \"ionice\" is enabled (0=highest priority, 7=lowest priority)"`
schedule/scheduler_config.go (1)

94-94: Avoid variable shadowing of 'scheduler' for clarity

The variable scheduler is re-declared within the case constants.SchedulerSystemd: block, which shadows the scheduler variable declared earlier in the function. To enhance code clarity and prevent potential confusion, consider using a different variable name or simply assigning to the existing variable without re-declaring it.

Apply this diff to modify the variable assignment:

-		scheduler := SchedulerSystemd{
+		scheduler = SchedulerSystemd{
			UnitTemplate:  global.SystemdUnitTemplate,
			TimerTemplate: global.SystemdTimerTemplate,
			Nice:          global.Nice,
		}
📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL

📥 Commits

Files that changed from the base of the PR and between da03de7 and 2641dc9.

📒 Files selected for processing (7)
  • config/global.go (2 hunks)
  • crond/crontab_test.go (1 hunks)
  • docs/content/schedules/systemd.md (1 hunks)
  • schedule/handler_systemd.go (1 hunks)
  • schedule/scheduler_config.go (2 hunks)
  • systemd/generate.go (3 hunks)
  • systemd/generate_test.go (1 hunks)
🧰 Additional context used
🪛 GitHub Check: codecov/patch
schedule/handler_systemd.go

[warning] 110-126: schedule/handler_systemd.go#L110-L126
Added lines #L110 - L126 were not covered by tests

schedule/scheduler_config.go

[warning] 100-102: schedule/scheduler_config.go#L100-L102
Added lines #L100 - L102 were not covered by tests

🔇 Additional comments (11)
docs/content/schedules/systemd.md (1)

Line range hint 1-262: Well-structured and formatted documentation.

The overall structure and formatting of the document are excellent. The use of Hugo shortcodes for tabs allows users to easily switch between different configuration formats, which is particularly helpful. The logical flow of information and clear headings make it easy for users to navigate and find relevant details.

The mix of Markdown and Hugo shortcodes is well-balanced, providing both readability and advanced formatting capabilities. The consistent use of code blocks for examples enhances the document's clarity.

config/global.go (3)

23-23: Approve the reformatted description.

The reformatting of the ResticLockRetryAfter field description improves readability without changing the content. This change, while not directly related to the PR objectives, contributes to overall code clarity.


24-24: Approve the reformatted description.

The reformatting of the ResticStaleLockAge field description improves readability without changing the content. This change, while not directly related to the PR objectives, contributes to overall code clarity.


15-15: Verify if changes to the Nice field are necessary.

The PR objectives mention setting the Nice value in the systemd service unit. However, the Nice field in the Global struct has not been modified. Please verify if any changes to this field or its description are necessary to fully address the PR objectives.

To check if the Nice value is being used correctly in the systemd unit template, you can run the following script:

This will help ensure that the Nice value from the global configuration is being correctly applied to the systemd unit template.

crond/crontab_test.go (1)

291-291: Approved change, but clarification needed

The addition of the -buildvcs=false flag to the go build command is a valid improvement. It can make the build process faster and more consistent, especially in CI/CD environments, by preventing the inclusion of version control information in the build output.

However, this change doesn't seem to be directly related to the PR objectives of setting Nice and IONice values in the systemd service unit. Could you please clarify why this change is included in this PR?

Additionally, I suggest adding a brief comment explaining the reason for this flag, to improve code maintainability. For example:

// Use -buildvcs=false to exclude version control info, ensuring consistent builds
cmd := exec.Command("go", "build", "-buildvcs=false", "-o", binary, "./mock")
systemd/generate.go (3)

76-87: Confirmation: Addition of new scheduling fields to templateInfo struct

The new fields CPUSchedulingPolicy, IOSchedulingClass, and IOSchedulingPriority are appropriately added to the templateInfo struct, enhancing the configurability of scheduling parameters for systemd units.


92-109: Confirmation: Extension of Config struct with scheduling parameters

Including Nice, CPUSchedulingPolicy, IOSchedulingClass, and IOSchedulingPriority in the Config struct correctly enables users to specify these scheduling options. The implementation aligns with the intended functionality.


146-158: Confirmation: Proper initialisation of templateInfo with new fields

The templateInfo instance is correctly initialised with values from config, including the new scheduling parameters. This ensures that all necessary data is passed to the template.

schedule/handler_systemd.go (2)

110-126: Ensure backward compatibility with existing configurations

The addition of new parameters to systemd.Generate may affect existing configurations. Please verify that these changes are backwards-compatible and that default values are appropriately handled when the new configuration options are not set.

Would you like assistance in testing with existing configurations to confirm compatibility?

🧰 Tools
🪛 GitHub Check: codecov/patch

[warning] 110-126: schedule/handler_systemd.go#L110-L126
Added lines #L110 - L126 were not covered by tests


124-126: ⚠️ Potential issue

Validate scheduling parameter values

Ensure that the values for h.config.Nice, h.config.IONiceClass, and h.config.IONiceLevel are within acceptable ranges before passing them to systemd.Generate. This will prevent potential runtime errors or misconfigurations when generating the systemd unit.

Consider adding validation checks like:

if h.config.Nice < -20 || h.config.Nice > 19 {
    return fmt.Errorf("Nice value %d is out of valid range (-20 to 19)", h.config.Nice)
}
🧰 Tools
🪛 GitHub Check: codecov/patch

[warning] 110-126: schedule/handler_systemd.go#L110-L126
Added lines #L110 - L126 were not covered by tests

systemd/generate_test.go (1)

438-543: Well-written test for priority field configurations

The new test function TestGeneratePriorityFields effectively validates the Generate function with various priority settings. It covers different scenarios and ensures correct behaviour.

schedule/scheduler_config.go Outdated Show resolved Hide resolved
schedule/scheduler_config.go Show resolved Hide resolved
systemd/generate.go Show resolved Hide resolved
systemd/generate.go Show resolved Hide resolved
schedule/handler_systemd.go Outdated Show resolved Hide resolved
schedule/handler_systemd.go Outdated Show resolved Hide resolved
systemd/generate_test.go Show resolved Hide resolved
Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 2

🧹 Outside diff range and nitpick comments (6)
examples/sample.service (2)

11-12: Good addition of CPUSchedulingPolicy parameter.

The conditional inclusion of the CPUSchedulingPolicy parameter enhances the service unit's configurability, aligning well with the PR's objective of improving systemd priority settings.

Consider updating the documentation to reflect this new configuration option and provide guidance on its usage and potential values (e.g., other, batch, idle, fifo, rr).


13-14: Excellent addition of IOSchedulingClass parameter.

The inclusion of the IOSchedulingClass parameter is a valuable addition, complementing the Nice parameter by allowing I/O priority configuration. This aligns well with the PR objective of setting IONice values.

Consider updating the documentation to explain the available IOSchedulingClass options (e.g., none, realtime, best-effort, idle) and their implications on system performance.

schedule/scheduler_config_test.go (2)

27-69: Well-structured test cases, good coverage

The table-driven test approach is a significant improvement. It provides better coverage for various scenarios and enhances readability. The test cases appropriately address the issue mentioned in the PR objectives regarding the Nice value.

A minor suggestion to further improve the tests:

Consider adding a test case where IONice is false but IONiceClass and IONiceLevel are set, to ensure they are not included in the output when IONice is disabled. For example:

{
    global: &config.Global{
        Scheduler:            constants.SchedulerSystemd,
        SystemdTimerTemplate: "timer.tpl",
        SystemdUnitTemplate:  "unit.tpl",
        IONice:               false,
        IONiceClass:          3,
        IONiceLevel:          5,
    },
    expected: SchedulerSystemd{TimerTemplate: "timer.tpl", UnitTemplate: "unit.tpl"},
},

This would help verify that IONiceClass and IONiceLevel are only included when IONice is true.


71-72: Consider enhancing test output for easier debugging

The loop efficiently tests all scenarios. However, when a test fails, it might be challenging to identify which specific case failed.

To improve debuggability, consider using a subtest for each case:

for i, tc := range testCases {
    t.Run(fmt.Sprintf("Case %d", i), func(t *testing.T) {
        result := NewSchedulerConfig(tc.global)
        assert.Equal(t, tc.expected, result, "Mismatch in case %d", i)
    })
}

This approach will provide more detailed output when a test fails, making it easier to identify and debug specific failing cases.

systemd/generate_test.go (1)

438-559: Well-structured and comprehensive test function.

The TestGeneratePriorityFields function is well-implemented, using a table-driven approach to test various scenarios for priority-related fields in systemd service units. It covers a good range of cases, including empty, standard, and background priorities, as well as specific Nice and IO scheduling values.

However, consider the following suggestions to enhance the test coverage:

  1. Add a test case for invalid priority values or combinations to ensure proper error handling.
  2. Verify the file permissions of the generated service file to ensure they are set correctly.
  3. Check the correct placement of priority-related fields within the service file structure, as their order might be important for systemd.

Would you like assistance in implementing these additional test cases?

schedule/scheduler_config.go (1)

111-122: Add documentation comment to getSchedulerSystemdDefaultConfig function

Including a comment explaining the purpose and usage of getSchedulerSystemdDefaultConfig would improve code readability and maintainability.

Apply this diff to add a function comment:

+// getSchedulerSystemdDefaultConfig initialises a SchedulerSystemd instance using global configuration values.
 func getSchedulerSystemdDefaultConfig(global *config.Global) SchedulerSystemd {
     scheduler := SchedulerSystemd{
         UnitTemplate:  global.SystemdUnitTemplate,
         TimerTemplate: global.SystemdTimerTemplate,
         Nice:          global.Nice,
     }
     if global.IONice {
         scheduler.IONiceClass = global.IONiceClass
         scheduler.IONiceLevel = global.IONiceLevel
     }
     return scheduler
 }
📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL

📥 Commits

Files that changed from the base of the PR and between 2641dc9 and 6288717.

📒 Files selected for processing (6)
  • examples/linux.yaml (2 hunks)
  • examples/sample.service (1 hunks)
  • schedule/handler_systemd.go (1 hunks)
  • schedule/scheduler_config.go (3 hunks)
  • schedule/scheduler_config_test.go (1 hunks)
  • systemd/generate_test.go (1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
  • schedule/handler_systemd.go
🔇 Additional comments (10)
examples/sample.service (3)

9-16: Summary: Excellent improvements to systemd service unit configuration

These changes significantly enhance the configurability of the systemd service unit, addressing the issues raised in #229 and meeting the PR objectives. The conditional parameters for Nice, CPUSchedulingPolicy, IOSchedulingClass, and IOSchedulingPriority provide fine-grained control over process and I/O priorities.

To ensure these changes are fully effective:

  1. Update the documentation to explain these new configuration options and their usage.
  2. Run the suggested verification scripts to confirm that all priority settings are correctly applied in various scenarios.
  3. Consider adding examples in the documentation to demonstrate how these parameters interact and affect service behaviour.

Overall, this is a valuable improvement that should resolve the reported issues and provide users with more control over their service priorities.


15-16: Great addition of IOSchedulingPriority parameter.

The inclusion of the IOSchedulingPriority parameter completes the IONice configuration mentioned in the PR objectives. This allows for fine-grained control over I/O priority within the chosen IOSchedulingClass.

Please update the documentation to explain the valid range of IOSchedulingPriority values (typically 0-7, with 0 being the highest priority) and how they interact with the IOSchedulingClass.

To ensure the IONice settings are correctly applied, please run the following verification:

#!/bin/bash
# Description: Verify that IOSchedulingClass and IOSchedulingPriority are correctly set in the generated service file

# Test: Generate the service file with different IO scheduling settings
for class in none realtime best-effort idle; do
    for priority in 0 4 7; do
        echo "Testing with IOSchedulingClass=$class and IOSchedulingPriority=$priority"
        # Assuming there's a command to generate the service file, replace it with the actual command
        # ./generate_service_file --io-class $class --io-priority $priority > test_service_io_${class}_${priority}.service
        # grep "^IOScheduling" test_service_io_${class}_${priority}.service
    done
done

This script will help verify that different IOSchedulingClass and IOSchedulingPriority combinations are correctly reflected in the generated service files.


9-10: Excellent fix for the Nice parameter issue.

This change correctly implements the conditional setting of the Nice parameter, addressing the problem described in issue #229. The Nice value will now be set based on the global configuration as intended.

To ensure this fix resolves the issue completely, please run the following verification:

This script will help verify that different Nice values are correctly reflected in the generated service files.

schedule/scheduler_config_test.go (1)

27-72: Excellent improvements to test coverage and structure

The changes to TestSystemdConfig significantly enhance the test coverage and structure. The new table-driven approach allows for comprehensive testing of various scenarios, including the Nice and IONice configurations mentioned in the PR objectives.

These modifications effectively address the issue described in #229, where the Nice value was not being correctly set in the systemd unit. The test cases now verify that Nice, IONiceClass, and IONiceLevel are correctly handled based on the global configuration.

The new structure also facilitates easy addition of more test cases in the future, improving the maintainability of the test suite.

Overall, these changes represent a substantial improvement in the quality and reliability of the testing for the scheduler configuration.

examples/linux.yaml (3)

Line range hint 1-156: Summary of changes and recommendations

The modifications to this configuration file generally align with the PR objectives, particularly in setting the Nice and IONice values. The changes appear to address the issue described in #229, although there are a few points that require clarification:

  1. Verify if the 'nice' value of 19 is intentional, as it differs from the value of 17 mentioned in the linked issue.
  2. Consider the performance implications of the combined ionice and nice settings.
  3. Clarify the rationale behind changing the schedule-permission from 'system' to 'user' in the test2 profile.

Overall, the changes seem to improve the configuration's ability to manage system resources. However, it's crucial to ensure that these modifications don't negatively impact the backup process's effectiveness or system security.


64-64: Clarify the change in schedule-permission

The schedule-permission for both backup and check operations in the test2 profile has been changed from 'system' to 'user'. This modification wasn't explicitly mentioned in the PR objectives. Could you please clarify:

  1. The rationale behind this change?
  2. Any potential impact on the execution of scheduled tasks?
  3. Whether this change is related to the main objective of setting Nice and IONice values?

This alteration could have implications for how the tasks are executed and the permissions they run under. It's important to ensure that this change doesn't inadvertently affect the system's security or the backup process's effectiveness.

To confirm the impact of this change, you might want to run:

systemctl show [email protected] | grep User

This will display the user under which the service runs, helping to verify the effect of the schedule-permission change.

Also applies to: 69-69


11-14: Verify the 'nice' value and consider performance implications

The addition of ionice and nice parameters aligns well with the PR objective. However, there are a couple of points to consider:

  1. The 'nice' value is set to 19, which differs from the value of 17 mentioned in the linked issue Nice in systemd unit is always 5 #229. Please verify if this is intentional or if it should be adjusted to match the issue description.

  2. The combination of ionice (class 3, level 7) and nice (19) settings results in the lowest possible priority. Whilst this may be suitable for non-critical background tasks, it could potentially lead to significant delays in backup operations during high system load.

To ensure these settings are applied correctly, you may want to run the following command after deploying this change:

This will display the relevant lines from the systemd service file, allowing you to verify that the values are correctly propagated.

systemd/generate_test.go (1)

438-559: Summary: New test function addresses PR objectives

The new TestGeneratePriorityFields function effectively addresses the PR objective of ensuring that Nice and IONice values in the systemd service unit are set based on the global configuration. This comprehensive test suite covers various priority settings scenarios, including empty, standard, and background priorities, as well as specific Nice and IO scheduling values.

The implementation aligns well with the goal of resolving the issue where the nice value in the systemd unit was consistently set to 5, regardless of the intended configuration. By testing different priority configurations, this function helps verify that the systemd unit template now correctly reflects the values defined in the global configuration.

While the overall implementation is solid, the suggested improvements (adding test cases for invalid priorities, checking file permissions, verifying field placement, and enhancing error messages) will further strengthen the test coverage and maintainability of the code.

schedule/scheduler_config.go (2)

97-98: Refactor: Centralise SchedulerSystemd initialisation

The introduction of getSchedulerSystemdDefaultConfig improves code maintainability by centralising the initialisation logic for SchedulerSystemd. This enhances consistency and reduces duplication.


105-105: Consistent use of helper function for default scheduler configuration

Using getSchedulerSystemdDefaultConfig(global) in the default case ensures consistent initialisation of SchedulerSystemd across different scheduler types.

systemd/generate_test.go Show resolved Hide resolved
systemd/generate_test.go Outdated Show resolved Hide resolved
Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL

📥 Commits

Files that changed from the base of the PR and between 6288717 and c1f3247.

📒 Files selected for processing (2)
  • config/global.go (2 hunks)
  • systemd/generate_test.go (1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
  • config/global.go

systemd/generate_test.go Show resolved Hide resolved
Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 0

🧹 Outside diff range and nitpick comments (2)
docs/content/configuration/priority.md (2)

13-13: Excellent clarification on accepted value types.

The addition of "string or numeric values are both valid" provides valuable information to users and aligns well with the PR objectives. This change will help prevent confusion and potential errors when setting the priority parameter.

To further enhance clarity, consider adding an example of a numeric value in the table below. This would illustrate how both string and numeric values correspond to the "nice" equivalents.


Line range hint 18-18: Consider enhancing the visibility of the Windows 11 note.

The note about Windows 11 issues with the "Background" priority level is crucial information for users. To improve its visibility and ensure users don't miss this important detail, consider formatting it as a notice or warning block, similar to the IO Nice information block in the document.

Here's a suggested format:

{{% notice style="warning" %}}
The implementation of the background mode is broken in Windows 11. It is widely reported that the process has a limit of 32MB of memory, even though this is undocumented. Please use `Idle` or `Low` on Windows 11 instead.
{{% /notice %}}
📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL

📥 Commits

Files that changed from the base of the PR and between c1f3247 and 2c6b1e3.

📒 Files selected for processing (5)
  • .gitignore (1 hunks)
  • Makefile (1 hunks)
  • config/global.go (2 hunks)
  • docs/content/configuration/priority.md (1 hunks)
  • docs/content/schedules/systemd.md (4 hunks)
✅ Files skipped from review due to trivial changes (1)
  • .gitignore
🚧 Files skipped from review as they are similar to previous changes (2)
  • config/global.go
  • docs/content/schedules/systemd.md
🔇 Additional comments (2)
docs/content/configuration/priority.md (1)

Line range hint 1-114: Overall, excellent documentation update.

The changes made to this file effectively address the PR objectives by clarifying the accepted values for the priority parameter. The documentation provides comprehensive information about priority settings, including Nice and IO Nice, with clear examples in various configuration formats.

The addition of the Windows 11 note is particularly valuable for users. The suggested enhancements to improve the visibility of this note and to add a numeric example in the priority table will further increase the clarity and usefulness of this documentation.

Great job on maintaining and improving this crucial piece of documentation!

Makefile (1)

312-325: ⚠️ Potential issue

New deployment target doesn't address PR objectives

The new deploy-current target automates the deployment process, which is a useful addition. However, it doesn't align with the stated PR objectives of setting the Nice and IONice values in the systemd service unit based on the global configuration.

Additionally, there are some concerns:

  1. The use of sudo -S in the SSH commands could be a security risk if not handled properly, as it might expose sudo passwords.
  2. There's no error handling or logging for failed deployments, which could lead to incomplete or inconsistent deployments across servers.
  3. The target doesn't implement any changes related to the nice value issue mentioned in the linked issue Nice in systemd unit is always 5 #229.

Consider the following improvements:

  1. Implement the changes related to setting Nice and IONice values as per the PR objectives.
  2. Replace sudo -S with a more secure method of privilege escalation, such as using SSH keys with sudo permissions or a dedicated deployment user.
  3. Add error handling and logging to ensure all deployments are successful.
  4. Include a step to update or restart the systemd service with the new configuration.

Example of a more secure deployment command:

-		ssh $$server "sudo -S install $(BINARY_LINUX_AMD64) /usr/local/bin/resticprofile" ; \
+		ssh $$server "sudo install $(BINARY_LINUX_AMD64) /usr/local/bin/resticprofile && sudo systemctl restart resticprofile.service" ; \

To ensure the deployment process is secure and aligns with best practices, please run the following commands:

@creativeprojects creativeprojects merged commit 830d0fd into master Oct 5, 2024
8 of 9 checks passed
@creativeprojects creativeprojects deleted the systemd-priority branch October 5, 2024 17:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Nice in systemd unit is always 5
1 participant