check-for-changes: performance improvements + wait for settle by NorseGaud · Pull Request #2104 · docker-mailserver/docker-mailserver

NorseGaud · 2021-07-29T13:38:47Z

Description

See: #2098

This PR will:

Have check-for-changes.sh wait for changes to "settle" (checksum comparison has to not have changed over two different checks with 5 second gaps between) before modifying and restarting anything
Added backgrounding of processes and using wait throughout instead of sleeps for more efficiency
Removed the while loop from addmailuser due to check-for-changes.sh logic changes

This is, in my opinion, critically needed for an API to be performing changes -- and lots of them all at once! See docker-mailserver/docker-mailserver-admin#3 for more discussion about this.

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

…formance improvements throughout + removed addmailuser while loops

NorseGaud · 2021-07-29T14:05:08Z

Forgot to draft this... again... Sorry... It's still a WIP

… various test fixes for new check-for-changes logic

…mailserver into check-for-changes-performance

NorseGaud · 2021-08-01T21:53:41Z

It looks like both the CI and my local machine are getting the following (running with new verbose flag):

✗ begin 186 checking tls: cipher list - rsa intermediate
   (from function `assert_success' in file test/test_helper/bats-assert/src/assert.bash, line 114,
    from function `collect_cipherlist_data' in file test/security_tls_cipherlists.bats, line 154,
    from function `check_ports' in file test/security_tls_cipherlists.bats, line 86,
    in test file test/security_tls_cipherlists.bats, line 47)
     `check_ports 'rsa' 'intermediate'' failed
   $ docker run -d --name tls_test_cipherlists --volume /Users/norsegaud/docker-mailserver/test/duplicate_configs/security_tls_cipherlists.bats/:/tmp/docker-mailserver/ --volume /Users/norsegaud/docker-mailserver/test/test-files/ssl/example.test/:/config/ssl/:ro --env DMS_DEBUG=0 --env ENABLE_POP3=1 --env SSL_TYPE=manual --env SSL_CERT_PATH=/config/ssl/cert.rsa.pem --env SSL_KEY_PATH=/config/ssl/key.rsa.pem --env TLS_LEVEL=intermediate --network test-network --network-alias example.test --hostname mail.example.test --tty mailserver-testing:ci
     f7d78f74fc3eedaa1277f63d9b7a2d06f22fbae7705f75b842b56bed71fce40b
   [ TASKLOG ]  mail.example.test is up and running
   $ docker run --rm --user 501:20 --network test-network --volume /Users/norsegaud/docker-mailserver/test/test-files/ssl/example.test/:/config/ssl/:ro --volume /var/folders/vb/pjyhgj4s491_k0t025_j56y00000gn/T//results/rsa/intermediate/:/output --workdir /output drwetter/testssl.sh:3.1dev --quiet --file /config/ssl/testssl.txt --mode parallel --overwrite --preference
     standard_init_linux.go:228: exec user process caused: permission denied
   
   -- command failed --
   status : 1
   output : standard_init_linux.go:228: exec user process caused: permission denied
   --

It seems like --user 501:20 is the issue as it runs without it. The comment says

--user "<uid>:<gid>" is a workaround: Avoids permission denied write errors for json output, uses id to match user uid & gid.

@polarathene, I'm going to see if anything else fails after removing it but I wanted to see if it's ok if it's removed (as long as everything else passes).

polarathene · 2021-08-01T22:19:01Z

--user 501:20 --network test-network --volume /Users/norsegaud/

That is odd looking uid and gid values, and with that volume is this Windows/WSL? That context seems to be missing in the CI tests (due to lack of verbose flag I guess?)

But this other volume looks more of an issue:

--volume /var/folders/vb/pjyhgj4s491_k0t025_j56y00000gn/T//results/rsa/intermediate/:/output

I'm not familiar with this mount, but that T//results looks a bit broken path wise? I don't think it should have two / in a row like that?

Chances are the --user may be misleading here as the cause, and rather the path to write to is invalid due to some change in this PR? I believe that is meant to be temporary output for the testssl container to write JSON to.

I think my test was one of the only ones to write to such a location which might be why it's failing in particular, if so it'd be better to fix whatever change has introduced that breakage, and not quick fix it by modifying the temporary files location used in the test.

polarathene · 2021-08-01T22:28:05Z

I believe whatever changes have been introduced have broken the functionality of ${BATS_TMPDIR} providing a valid path prefix here:

docker-mailserver/test/security_tls_cipherlists.bats

Lines 22 to 23 in f333740

    
           # `${BATS_TMPDIR}` maps to `/tmp` 
        
           export TLS_RESULTS_DIR="${BATS_TMPDIR}/results"

EDIT: If you are noticing any issues with Windows/WSL running the tests that are inconsistent with the CI, it might be another reason in favor of running bats from docker? I was looking into that at some point, not entirely sure if it'd resolve any issues with volume mounts though (a throwaway data volume that is a container instead of bind mount on the host filesystem might also work?).

NorseGaud · 2021-08-02T00:21:18Z

Thanks @polarathene , appreciate your insight. So, I rolled bats back to 1.3.0 and it works. It looks like 1.4.0 introduced a change to BATS_TMPDIR: bats-core/bats-core#410 (comment)

How the hell this didn't fail in #2095 is beyond me... Might be good to have @casperklein and @georglauterbach take a look if they get a few minutes. I'm unsure about how the CI is all set up TBH. IMO this shouldn't have passed...

NorseGaud · 2021-08-03T15:19:10Z

Please ignore the bats submodule for now. I'm using that to gain visibility into the tests and will revert it soon.

georglauterbach · 2021-08-13T08:34:01Z

Is this WIP or ready to be revied?

NorseGaud · 2021-08-13T11:48:29Z

@georglauterbach , no it's ready for review

georglauterbach

LGTM 👍🏼

georglauterbach · 2021-08-13T12:20:17Z

@NorseGaud when you push yourself, my review will be dismissed I think. But @docker-mailserver/maintainers can update the branch via the webUI which should not dismiss it (if that is somehow important) :D

I could do it myself, but I've become cautious with your PRs because I already messed up twice in other PRs 😆

georglauterbach · 2021-08-14T08:14:55Z

LGTM 👍

@casperklein or @wernerfred feel free to merge this PR if you approve it :)

georglauterbach · 2021-08-16T07:19:42Z

I will go ahead and merge this :)

casperklein · 2021-08-16T10:44:07Z

I hadn't time to review this yet. Otherwise I had approved it..

wernerfred · 2021-08-16T11:17:41Z

Dito ⌛

casperklein · 2021-08-22T14:17:33Z

+service postfix start &
+wait


I think the sleep here was, because service postfix start exit (0) successfully pretty fast, however postfix needs some time to come up completely.

service postfix start & wait

is equal to just service postfix start ?

I am not sure if it's related, but I read an issue/PR this week, about our CI behaving "flacky" again. Maybe this is the reason?

Good points -- I'll play around with it and see what I find.

casperklein · 2021-08-22T14:31:18Z

+  CMP_RESULT=$(cmp -- "${CHKSUM_FILE}" "${CHKSUM_FILE}.new")
+  if [[ -n "${CMP_RESULT}" ]] # If there is a difference between checksum files


I know it was there before, but do some know what the -- are for? I couldn't find something in the man page.

A double-dash in a shell command signals the end of options and disables further option processing.

Commonly used for example when you're trying to grep something that starts with a double dash, you'd write

grep -E -- "--to-grep" $FILE

I knew that. I was wondering, if in that case, it has another purpose. Because it seems to me unnecessary here.

PS:

A double-dash in a shell command signals the end of options and disables further option processing.

I wouldn't rely on that, as not every command implements that logic 😞

I never use it personally :)

casperklein · 2021-08-22T20:38:05Z

 # wait until postfix is dead (triggered by trap)
 while kill -0 "$(< /var/spool/postfix/pid/master.pid)"
 do
-  sleep 5
+  sleep 1
 done


Inspired by your other changes, I wonder if this one liner would work too:

wait $(</var/spool/postfix/pid/master.pid)

Unfortunately, wait only works on child processes of the current shell:

# wait $(</var/spool/postfix/pid/master.pid) bash: wait: pid 3108 is not a child of this shell

However https://stackoverflow.com/a/41613532/568737 suggests tail instead (which works):

tail -f --pid="$(</var/spool/postfix/pid/master.pid)" /dev/null

We could replace the while loop with that.

Sounds good -- including in #2134

I am unsure. This could lead to problems on MacOS? I don't have a mac to test, but the stackoverflow posts suggest this only for linux.
On the other hand, we don't support MacOS officially. What do you think @docker-mailserver/maintainers?

The code doesn't run on mac, it'll run inside of the linux contianer. Shouldn't be an issue.

You are absolutely right.

…#2104)" This reverts commit 232d463.

* docker_container first, then fall back to docker_image + test changes to support + test change to wait for smtp port to fix flakey tests since #2104 * quick fix * Update setup.sh Co-authored-by: Georg Lauterbach <[email protected]> Co-authored-by: Casper <[email protected]>

NorseGaud added 6 commits July 24, 2021 09:47

added some helpful stuff to check for changes

47ec9bd

more tweaks

36586ea

test

f3994eb

check-for-changes.sh now makes sure that changes settle + various per…

dbf444b

…formance improvements throughout + removed addmailuser while loops

cleanup

9650792

Merge branch 'master' into check-for-changes-performance

4e5beb1

casperklein changed the title ~~check-for-changes performance improvements~~ WIP: check-for-changes performance improvements Jul 30, 2021

casperklein marked this pull request as draft July 30, 2021 12:07

delmailuser Mailbox directory not existing shouldn't throw an error +…

c04aed9

… various test fixes for new check-for-changes logic

wernerfred added the meta/feature freeze On hold due to upcoming release process label Jul 30, 2021

wernerfred added this to the v10.1.1 milestone Jul 30, 2021

NorseGaud added 3 commits July 30, 2021 22:55

quick test fix

68727fa

sleep should only happen when there is a difference

954374e

Merge branch 'master' of https://github.com/docker-mailserver/docker-…

f5bcfd2

…mailserver into check-for-changes-performance

NorseGaud mentioned this pull request Aug 2, 2021

Bats 1.4.X fix: BATS_TMPDIR #2110

Closed

11 tasks

fix for path to use pwd instead fo CDIR

c494954

NorseGaud changed the title ~~WIP: check-for-changes performance improvements~~ check-for-changes: performance improvements + wait for settle Aug 3, 2021

NorseGaud marked this pull request as ready for review August 3, 2021 15:18

georglauterbach added area/scripts kind/improvement Improve an existing feature, configuration file or the documentation priority/low pr/needs review priority/medium and removed meta/feature freeze On hold due to upcoming release process labels Aug 10, 2021

georglauterbach assigned NorseGaud Aug 10, 2021

NorseGaud added 2 commits August 13, 2021 07:51

bats bump

52cb02e

Merge branch 'master' into check-for-changes-performance

3a54d7f

georglauterbach requested review from a team August 13, 2021 12:17

georglauterbach approved these changes Aug 13, 2021

View reviewed changes

williamdes reviewed Aug 13, 2021

View reviewed changes

Comment thread test/open_dkim.bats

Merge branch 'master' into check-for-changes-performance

89ca9d0

georglauterbach approved these changes Aug 14, 2021

View reviewed changes

georglauterbach merged commit 232d463 into docker-mailserver:master Aug 16, 2021

NorseGaud deleted the check-for-changes-performance branch August 16, 2021 11:19

casperklein reviewed Aug 17, 2021

View reviewed changes

Comment thread target/supervisor/conf.d/supervisor-app.conf

NorseGaud mentioned this pull request Aug 17, 2021

reverted stopwaitsecs for postfix #2137

Merged

11 tasks

casperklein reviewed Aug 22, 2021

View reviewed changes

This was referenced Aug 28, 2021

setup.sh: docker_container first, then fall back to docker_image #2134

Merged

[BUG] [10.1.1] High CPU usage caused by check-for-changes.sh #2154

Closed

NorseGaud added a commit that referenced this pull request Aug 28, 2021

Revert "check-for-changes: performance improvements + wait for settle (…

dff7e42

…#2104)" This reverts commit 232d463.

NorseGaud mentioned this pull request Aug 28, 2021

Revert "check-for-changes: performance improvements + wait for settle" #2155

Merged

casperklein mentioned this pull request Aug 28, 2021

v10.1.2 release #2156

Merged

5 tasks

NorseGaud mentioned this pull request Aug 28, 2021

check-for-changes: performance improvements + wait for settle v2 #2157

Closed

10 tasks

		CMP_RESULT=$(cmp -- "${CHKSUM_FILE}" "${CHKSUM_FILE}.new")
		if [[ -n "${CMP_RESULT}" ]] # If there is a difference between checksum files

Uh oh!

Conversation

NorseGaud commented Jul 29, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Type of change

Checklist:

Uh oh!

NorseGaud commented Jul 29, 2021

Uh oh!

NorseGaud commented Aug 1, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

polarathene commented Aug 1, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

polarathene commented Aug 1, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

NorseGaud commented Aug 2, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

NorseGaud commented Aug 3, 2021

Uh oh!

georglauterbach commented Aug 13, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

NorseGaud commented Aug 13, 2021

Uh oh!

georglauterbach left a comment

Choose a reason for hiding this comment

Uh oh!

georglauterbach commented Aug 13, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

georglauterbach commented Aug 14, 2021

Uh oh!

georglauterbach commented Aug 16, 2021

Uh oh!

casperklein commented Aug 16, 2021

Uh oh!

wernerfred commented Aug 16, 2021

Uh oh!

Uh oh!

casperklein Aug 22, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

casperklein Aug 22, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

casperklein Aug 22, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

NorseGaud commented Jul 29, 2021 •

edited

Loading

NorseGaud commented Aug 1, 2021 •

edited

Loading

polarathene commented Aug 1, 2021 •

edited

Loading

polarathene commented Aug 1, 2021 •

edited

Loading

NorseGaud commented Aug 2, 2021 •

edited

Loading

georglauterbach commented Aug 13, 2021 •

edited

Loading

georglauterbach commented Aug 13, 2021 •

edited

Loading

casperklein Aug 22, 2021 •

edited

Loading

casperklein Aug 22, 2021 •

edited

Loading

casperklein Aug 22, 2021 •

edited

Loading