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

Prevent race condition on supervisorctl reload #2343

Merged
merged 2 commits into from
Dec 29, 2021

Conversation

casperklein
Copy link
Member

@casperklein casperklein commented Dec 25, 2021

Description

When setting SUPERVISOR_LOGLEVEL e.g. to info, supervisord will be restarted (don't get fooled by the term 'reload'):

function _setup_supervisor
{
if ! grep -q "loglevel = ${SUPERVISOR_LOGLEVEL}" /etc/supervisor/supervisord.conf
then
case "${SUPERVISOR_LOGLEVEL}" in
'critical' | 'error' | 'info' | 'debug' )
sed -i -E \
"s|(loglevel).*|\1 = ${SUPERVISOR_LOGLEVEL}|g" \
/etc/supervisor/supervisord.conf
supervisorctl reload
;;

The restart however, can take some time. While this time period, start-mailserver.sh continues to run until terminated by supervisord. This can lead to errors, if files are modified (by our numerous sed-operations).

When supervisord is completely restarted, also start-mailserver.sh is started again. Files that already have been modified by the first run, will now be reported as errors.

This is how a problematic log looks like (empty lines added for readability):

[ TASKLOG ]  Welcome to docker-mailserver 10.4.0
[ TASKLOG ]  Initializing setup
[ TASKLOG ]  Checking configuration
[ TASKLOG ]  Configuring mail server
Restarted supervisord

[ WARNING ]  No DKIM key provided. Check the documentation on how to get your keys.
[ WARNING ]  Spam messages WILL NOT BE DELIVERED, you will NOT be notified of ANY message bounced. Please define SPAMASSASSIN_SPAM_TO_INBOX explicitly.
[ WARNING ]  Clamav is disabled. You can enable it with 'ENABLE_CLAMAV=1'

2021-12-25 22:50:36,774 INFO Included extra file "/etc/supervisor/conf.d/saslauth.conf" during parsing
2021-12-25 22:50:36,774 INFO Included extra file "/etc/supervisor/conf.d/supervisor-app.conf" during parsing
2021-12-25 22:50:36,774 INFO Set uid to user 0 succeeded
2021-12-25 22:50:36,774 INFO RPC interface 'supervisor' initialized
2021-12-25 22:50:36,775 INFO supervisord started with pid 7
2021-12-25 22:50:37,779 INFO spawned: 'mailserver' with pid 2008

[ TASKLOG ]  Welcome to docker-mailserver 10.4.0
2021-12-25 22:50:37,802 INFO success: mailserver entered RUNNING state, process has stayed up for > than 0 seconds (startsecs)
[ TASKLOG ]  Initializing setup
[ TASKLOG ]  Checking configuration
[ TASKLOG ]  Configuring mail server
[ WARNING ]  No DKIM key provided. Check the documentation on how to get your keys.
Nameservers 127.0.0.11
Error: sed -i -r -e s|^(MinProtocol).*|\1 = TLSv1| -e s|^(CipherString).*|\1 = DEFAULT@SECLEVEL=1| /usr/lib/ssl/openssl.cnf
Error: sed -i -r s|^(smtpd_tls_chain_files =).*|\1 /etc/dms/tls/key /etc/dms/tls/cert| /etc/postfix/main.cf
Error: sed -i -r -e s|^(ssl_key =).*|\1 </etc/dms/tls/key| -e s|^(ssl_cert =).*|\1 </etc/dms/tls/cert| /etc/dovecot/conf.d/10-ssl.conf

1st part: start-mailserver.sh is running and restarts supervisord
2nd part: start-mailserver.sh keeps running for a bit
3rd/4th part: start-mailserver.sh started again (see the 'Welcome to docker-mailserver 10.4.0' message). Notice the sed errors, because sed was run a second time.

To prevent this, start-mailserver.sh should exit after calling supervisorctl reload.

After applying this fix, the log looks like this:

[ TASKLOG ]  Welcome to docker-mailserver 10.4.0
[ TASKLOG ]  Initializing setup
[ TASKLOG ]  Checking configuration
[ TASKLOG ]  Configuring mail server
Restarted supervisord

2021-12-25 23:37:51,922 INFO Included extra file "/etc/supervisor/conf.d/saslauth.conf" during parsing
2021-12-25 23:37:51,922 INFO Included extra file "/etc/supervisor/conf.d/supervisor-app.conf" during parsing
2021-12-25 23:37:51,922 INFO Set uid to user 0 succeeded
2021-12-25 23:37:51,923 INFO RPC interface 'supervisor' initialized
2021-12-25 23:37:51,923 INFO supervisord started with pid 7
2021-12-25 23:37:52,926 INFO spawned: 'mailserver' with pid 27
[ TASKLOG ]  Welcome to docker-mailserver 10.4.0
2021-12-25 23:37:52,946 INFO success: mailserver entered RUNNING state, process has stayed up for > than 0 seconds (startsecs)
[ TASKLOG ]  Initializing setup
[ TASKLOG ]  Checking configuration
[ TASKLOG ]  Configuring mail server
[ WARNING ]  No DKIM key provided. Check the documentation on how to get your keys.
[ WARNING ]  Spam messages WILL NOT BE DELIVERED, you will NOT be notified of ANY message bounced. Please define SPAMASSASSIN_SPAM_TO_INBOX explicitly.
[ WARNING ]  Clamav is disabled. You can enable it with 'ENABLE_CLAMAV=1'
[ TASKLOG ]  Applying user patches

Fixes #

Type of change

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Improvement (non-breaking change that does improve existing functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • This change requires a documentation update

Checklist:

  • My code follows the style guidelines of this project
  • I have performed a self-review of my own code
  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation (README.md or the documentation under docs/)
  • If necessary I have added tests that prove my fix is effective or that my feature works
  • New and existing unit tests pass locally with my changes

@casperklein casperklein self-assigned this Dec 25, 2021
@casperklein casperklein requested a review from a team December 25, 2021 23:47
@casperklein casperklein marked this pull request as ready for review December 26, 2021 00:39
Copy link
Member

@polarathene polarathene left a comment

Choose a reason for hiding this comment

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

Good catch! 👍

@georglauterbach
Copy link
Member

The more I think about it, and the more PRs out there are merged - especially considering the issue in #2348 - v10.5 seems more appropriate that v10.4.1. I'll add the correct milestone and this can be merged.

@georglauterbach georglauterbach added this to the v10.5.0 milestone Dec 29, 2021
@casperklein casperklein merged commit f7465a5 into docker-mailserver:master Dec 29, 2021
@casperklein casperklein deleted the supervisorctl branch December 29, 2021 16:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants