fix(changedetector): Use service reload commands instead of supervisorctl restart <service>#2947
Merged
polarathene merged 6 commits intodocker-mailserver:masterfrom Dec 23, 2022
Conversation
polarathene
added
service/dovecot
service/postfix
area/scripts
kind/improvement
casperklein
previously approved these changes
Dec 22, 2022
georglauterbach
requested changes
Dec 22, 2022
Member
georglauterbach
left a comment
georglauterbach
left a comment
There was a problem hiding this comment.
LGTM 👍🏼 Just the one thing about the return code / value.
Member
Author
These seem to appear in the main logs via I guess this PR is fine now (assuming the CI is a tad slower to avoid race condition). |
Reloading is faster than restarting the processes. Restarting is a bit heavy handed here and may no longer be necessary for general usage?
Change detection is too fast now (0-1 seconds vs 5+). Directory being waited on here was created near the end of a change event, reducing that time to detect a change by the utility method further. We can instead check that the directory exists after the change detection event is completed.
We don't presently use remote storage in tests, but it might be relevant in future when testing NFS. This at least avoids any confusing failure happening when that scenario is tested.
`-ne 0` wasn't doing what I thought apparently. Review feedback shows that this is the correct way to handle the conditional.
polarathene
force-pushed
the
chore/change-detection-reload-instead-of-restart
branch
from
1e9e1f7 to
18ddcd3
Compare
polarathene
changed the title
reload Dovecot and Postfix instead of restartreload commands instead of supervisorctl restart <service>
casperklein
approved these changes
Dec 23, 2022
This was referenced Dec 28, 2022
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
With
reloada change detection event during local testing can be processed in less than a second according to logs. Previously this was 5+ seconds (plus additional downtime for Postfix/Dovecot to become available again).In the past it was apparently an issue to use
<service> reloaddue to a concern with the PID for wrapper scripts thatsupervisorctlmanaged, thussupervisorctl <service> restarthad been used. Past discussions with maintainers suggest this is not likely an issue anymore, andreloadshould be fine to switch to now 👍NOTE: It may not be an issue in the CI, but on local systems running tests may risk failure in
setup-cli.batsfrom a false positive due to 1 second polling window of the test helper method, and a change event being possible to occur entirely between the two checks undetected by the current approach.If this is a problem, we may need to think of a better way to catch the change. The
letsencrypttest counts how many change events are expected to have been processed, and this could technically be leveraged by the test helper too.NOTE: These two lines (with regex pattern for postfix) are output in the terminal when using the services respective
reloadcommands:I wasn't sure how to match them as they did not appear in the
changedetectorlog (EDIT: they appear in the main log output, egdocker logs <container name>).Instead I've just monitored the
changedetectorlog messages, which should be ok for logic that previously needed to ensure Dovecot / Postfix was back up after therestartwas issued.This was a change I wanted to get into a major release at some point. I don't have the usual extensive details / references to support the change however.
This should help reduce downtime during change events, especially in situations like reported here?:
check-for-changes.sh): Drop redundant guards #2623 (comment)Type of change
Checklist: