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

systemd: add NeedDaemonReload to the unit state #11241

Merged
merged 5 commits into from
Jan 18, 2022

Conversation

kubiko
Copy link
Contributor

@kubiko kubiko commented Jan 12, 2022

Adding NeedDaemonReload to the unit state, as foundation for removing unnecessary daemon-reload calls int he future.

Signed-off-by: Ondrej Kubik <ondrej.kubik@canonical.com>
… parameter

Signed-off-by: Ondrej Kubik <ondrej.kubik@canonical.com>
Signed-off-by: Ondrej Kubik <ondrej.kubik@canonical.com>
@codecov-commenter
Copy link

Codecov Report

Merging #11241 (f464291) into master (8592e45) will increase coverage by 0.00%.
The diff coverage is 100.00%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master   #11241   +/-   ##
=======================================
  Coverage   78.35%   78.36%           
=======================================
  Files         923      923           
  Lines      105342   105344    +2     
=======================================
+ Hits        82543    82551    +8     
+ Misses      17659    17654    -5     
+ Partials     5140     5139    -1     
Flag Coverage Δ
unittests 78.36% <100.00%> (+<0.01%) ⬆️

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

Impacted Files Coverage Δ
systemd/systemd.go 85.83% <100.00%> (+0.03%) ⬆️
store/cache.go 69.23% <0.00%> (-1.93%) ⬇️
cmd/snap/cmd_debug_state.go 70.64% <0.00%> (+0.45%) ⬆️
overlord/ifacestate/helpers.go 77.45% <0.00%> (+0.48%) ⬆️
osutil/synctree.go 79.24% <0.00%> (+2.83%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 8592e45...f464291. Read the comment docs.

@bboozzoo
Copy link
Collaborator

For reference, the implementation of the property NeedDaemonReload on systemd side is right here: https://github.com/systemd/systemd/blob/8585b7ca65e5c5d101a935b41cd081963f790946/src/core/unit.c#L3694-L3720

Copy link
Collaborator

@bboozzoo bboozzoo left a comment

Choose a reason for hiding this comment

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

LGTM, thanks!

systemd/systemd.go Show resolved Hide resolved
Copy link
Contributor

@MiguelPires MiguelPires left a comment

Choose a reason for hiding this comment

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

LGTM

Signed-off-by: Maciej Borzecki <maciej.zenon.borzecki@canonical.com>
@bboozzoo
Copy link
Collaborator

@mvo5 this is ready, test failure are unrelated though. Can you land it?

@anonymouse64 anonymouse64 self-requested a review January 16, 2022 18:47
Copy link
Contributor

@stolowski stolowski left a comment

Choose a reason for hiding this comment

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

LGTM, thanks

@pedronis pedronis merged commit 9fde45f into snapcore:master Jan 18, 2022
Copy link
Member

@anonymouse64 anonymouse64 left a comment

Choose a reason for hiding this comment

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

post-merge lgtm, thanks for splitting this out

@kubiko kubiko deleted the systemd-add-NeedDaemonReload branch June 16, 2023 12:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
7 participants