fix(changedetector): Use service reload commands instead of supervisorctl restart <service> (#2947)

With `reload` a 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> reload` due to a concern with the PID for wrapper scripts that `supervisorctl` managed, thus `supervisorctl <service> restart` had been used. Past discussions with maintainers suggest this is not likely an issue anymore, and `reload` should 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.bats` from 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 `letsencrypt` test 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 `reload` commands:

```
postfix/master.*: reload -- version .*, configuration /etc/postfix
dovecot: master: Warning: SIGHUP received - reloading configuration
```

I wasn't sure how to match them as they did not appear in the `changedetector` log (_**EDIT:** they appear in the main log output, eg `docker logs <container name>`_).

Instead I've just monitored the `changedetector` log messages, which should be ok for logic that previously needed to ensure Dovecot / Postfix was back up after the `restart` was issued.

---

Commit history:

* chore: Change events `reload` Dovecot and Postfix instead of `restart`

Reloading is faster than restarting the processes.

Restarting is a bit heavy handed here and may no longer be necessary for general usage?

* tests: Adapt tests to support service `reload` instead of `restart`

* chore: Additional logging for debugging change event logs

* fix: Wait on change detection, then verify directory created

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.

* chore: Keep using the maildir polling check

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.
This commit is contained in:
Brennan Kinney 2022-12-24 01:57:24 +13:00 committed by GitHub
parent fe21fe78e2
commit b58165762a
No known key found for this signature in database
GPG key ID: 4AEE18F83AFDEB23
6 changed files with 38 additions and 45 deletions

View file

@ -181,18 +181,15 @@ function wait_until_change_detection_event_completes() {
[[ $(__change_event_status) == "${CHANGE_EVENT_END}" ]]
}
if [[ ! $(__is_changedetector_processing) ]]
# A new change event is expected,
# If the last event status is not yet `CHANGE_EVENT_START`, wait until it is:
if ! __is_changedetector_processing
then
# A new change event is expected, wait for it:
repeat_until_success_or_timeout 60 __is_changedetector_processing
fi
# Change event is in progress, wait until it finishes:
repeat_until_success_or_timeout 60 __is_changedetector_finished
# NOTE: Although the change event has completed, services like Postfix and Dovecot
# may still be in the process of restarting.
# You may still want to wait longer if depending on those to be ready.
}
# An account added to `postfix-accounts.cf` must wait for the `changedetector` service