Skip to content
This repository has been archived by the owner on Dec 21, 2021. It is now read-only.

Move restart policy to systemd #263

Merged
merged 14 commits into from
Aug 30, 2021
Merged

Move restart policy to systemd #263

merged 14 commits into from
Aug 30, 2021

Conversation

siegfriedweber
Copy link
Member

@siegfriedweber siegfriedweber commented Aug 17, 2021 •
edited by razvan
Loading

Description

Fixes #207
Tested by stackabletech/agent-integration-tests#61

Changed

  • Handling of service restarts moved from the Stackable agent to systemd.

Removed

  • Check removed if a service starts up correctly within 10 seconds. systemd manages restarts now and the Stackable agent cannot detect if a service is in a restart loop.

Review Checklist

  • Code contains useful comments
  • (Integration-)Test cases added (or not applicable)
  • Documentation added (or not applicable)
  • Changelog updated (or not applicable)

@siegfriedweber siegfriedweber requested a review from a team August 17, 2021 14:56
@siegfriedweber siegfriedweber self-assigned this Aug 17, 2021
@siegfriedweber siegfriedweber mentioned this pull request Aug 17, 2021
Merged
1 task
@siegfriedweber siegfriedweber force-pushed the move_restart_policy_to_systemd branch from 4310fa5 to 2fc6ef8 Compare August 19, 2021 07:21
soenkeliebau
soenkeliebau reviewed Aug 19, 2021
View reviewed changes
Copy link
Member

@soenkeliebau soenkeliebau left a comment

Choose a reason for hiding this comment

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

LGTM overall, I've left a few questions that I'd like to clarify, but most of those will probably simply be resolveable.

As far as I can tell this would not notice if a service enters into a permanent restart loop, right? It would update the restart count on the pod, but never set a failed condition or something similar?

src/provider/states/pod/starting.rs Show resolved Hide resolved
src/provider/states/pod/running.rs Show resolved Hide resolved
src/provider/states/pod/running.rs Show resolved Hide resolved
src/provider/systemdmanager/service.rs Show resolved Hide resolved
@siegfriedweber siegfriedweber force-pushed the move_restart_policy_to_systemd branch from 2fc6ef8 to e48e1ea Compare August 23, 2021 10:39
razvan
razvan previously approved these changes Aug 23, 2021
View reviewed changes
Copy link
Member

@razvan razvan left a comment

Choose a reason for hiding this comment

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

I didn't look closely at the code but the corresponding integration tests are green.

@siegfriedweber
Copy link
Member Author

As far as I can tell this would not notice if a service enters into a permanent restart loop, right? It would update the restart count on the pod, but never set a failed condition or something similar?

Right. If the restart policy of the pod is set to Always or OnFailure then this is the desired behavior, I think. The Stackable Agent also does not have a notion what a "restart loop" is. We regard a restart every 10 seconds as a restart loop but a restart every 10 minutes probably not.

@siegfriedweber siegfriedweber force-pushed the move_restart_policy_to_systemd branch 3 times, most recently from a205512 to abeac58 Compare August 26, 2021 14:25
soenkeliebau
soenkeliebau previously approved these changes Aug 30, 2021
View reviewed changes
Copy link
Member

@soenkeliebau soenkeliebau left a comment

Choose a reason for hiding this comment

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

LGTM

@siegfriedweber
Set TimeoutStopSec only once
49c9a4e
@siegfriedweber
Map restart policy to systemd service
bb03232
@siegfriedweber
Remove restart capability from pod state
89edf10 
The running state is no longer responsible for deciding if a service
should be restarted. This is done by systemd. Therefore the transition
from the running state to the starting state was removed.

The service_state function was added to the SystemdManager to check if a
service is running or terminated. This function is used for the
transition from the running to the terminated state and for patching the
container state.
@siegfriedweber
Service state "Created" added
71558d8
@siegfriedweber
Waiting after systemd service startup removed
fe40635 
When a systemd service is started then the agent does not wait anymore
ten seconds to check if it was successfully started because systemd
manages restarts now and the agent cannot detect if the service is in a
restart loop.
@siegfriedweber
Set RestartSec to 2 in unit files
b53d5f5
@siegfriedweber
Start rate limiting of systemd units disabled
1df17da
@siegfriedweber
Tests fixed
12676bc
@siegfriedweber
Changelog updated
39516c9
@siegfriedweber
Remove the waiting phase also from the docs
e5748b9
@siegfriedweber
Set restart count in the container status
72abc6f
@siegfriedweber
Add pull request to the changelog
68731bb
@siegfriedweber
Changelog updated
2b3a628
@siegfriedweber
Set correct systemd default target
3b67ff7
@siegfriedweber siegfriedweber force-pushed the move_restart_policy_to_systemd branch from abeac58 to 3b67ff7 Compare August 30, 2021 08:45
@siegfriedweber siegfriedweber merged commit 98bf547 into main Aug 30, 2021
@siegfriedweber siegfriedweber deleted the move_restart_policy_to_systemd branch August 30, 2021 11:24
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@lfrancke lfrancke lfrancke approved these changes

@razvan razvan razvan left review comments

@soenkeliebau soenkeliebau Awaiting requested review from soenkeliebau

Assignees

@siegfriedweber siegfriedweber

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Move restart behavior of services from agent to systemd
4 participants
@siegfriedweber @razvan @lfrancke @soenkeliebau