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
interfaces/systemd: use batch systemd operations #11372
interfaces/systemd: use batch systemd operations #11372
Conversation
Signed-off-by: Ondrej Kubik <ondrej.kubik@canonical.com>
Signed-off-by: Ondrej Kubik <ondrej.kubik@canonical.com>
Drop the changes that require new API calls to DaemonReloadIfNeeded(), and only enable batched operations. Signed-off-by: Maciej Borzecki <maciej.zenon.borzecki@canonical.com>
Codecov Report
@@ Coverage Diff @@
## master #11372 +/- ##
==========================================
- Coverage 78.36% 78.35% -0.01%
==========================================
Files 931 931
Lines 106558 106569 +11
==========================================
+ Hits 83499 83507 +8
- Misses 17859 17861 +2
- Partials 5200 5201 +1
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report at Codecov.
|
interfaces/systemd/backend.go
Outdated
} | ||
} | ||
} | ||
if 0 < len(disableUnits) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(nitpick) this is slightly unusual, we usually have if len(disableUnits) > 0
(same below)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pushed a tweak, thanks!
Signed-off-by: Maciej Borzecki <maciej.zenon.borzecki@canonical.com>
…interface-backend-batch
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, only nitpicks about comments and error messages which should now be in plural form.
interfaces/systemd/backend.go
Outdated
if err := systemd.Enable(serviceUnits); err != nil { | ||
logger.Noticef("cannot enable service %q: %s", service, err) | ||
if len(changed) > 0 { | ||
// Ensure the service is running right now and on reboots |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nitpick: this comment should use the plural now.
interfaces/systemd/backend.go
Outdated
if len(removed) > 0 { | ||
logger.Noticef("systemd-backend: Disable: removed services: %q", removed) | ||
if err := systemd.Disable(removed); err != nil { | ||
logger.Noticef("cannot disable service %q: %s", removed, err) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nitpick: this message should now say "services"
interfaces/systemd/backend.go
Outdated
if err := systemd.Stop(serviceUnits, 5*time.Second); err != nil { | ||
logger.Noticef("cannot stop service %q: %s", service, err) | ||
if err := systemd.Stop(removed, 5*time.Second); err != nil { | ||
logger.Noticef("cannot stop service %q: %s", removed, err) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
same here
interfaces/systemd/backend.go
Outdated
} | ||
} | ||
} | ||
if len(disableUnits) > 0 { | ||
if err := systemd.Disable(disableUnits); err != nil { | ||
logger.Noticef("cannot disable service %q: %s", disableUnits, err) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
plural
interfaces/systemd/backend.go
Outdated
} | ||
if len(stopUnits) > 0 { | ||
if err := systemd.Stop(stopUnits, 5*time.Second); err != nil { | ||
logger.Noticef("cannot stop service %q: %s", stopUnits, err) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
plural
There was a problem hiding this 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>
Signed-off-by: Maciej Borzecki <maciej.zenon.borzecki@canonical.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you!
The PR cherry picks some of the changes from #10463, and some test tweaks from #11353. The main change calling systemd to operate on services in batch, rather than one by one.
cc @kubiko