Skip to content

Reduce unnecessary Disk IO#1966

Closed
georglauterbach wants to merge 4 commits intomasterfrom
disk-io
Closed

Reduce unnecessary Disk IO#1966
georglauterbach wants to merge 4 commits intomasterfrom
disk-io

Conversation

@georglauterbach
Copy link
Copy Markdown
Member

@georglauterbach georglauterbach commented May 12, 2021

Description

See #1958

@nukedupe please double-check this PR too.

Fixes #1958

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

@georglauterbach georglauterbach requested a review from a team May 12, 2021 09:59
@georglauterbach georglauterbach self-assigned this May 12, 2021
@georglauterbach georglauterbach added area/scripts kind/improvement Improve an existing feature, configuration file or the documentation pr/needs review priority/low labels May 12, 2021
@georglauterbach
Copy link
Copy Markdown
Member Author

georglauterbach commented May 12, 2021 •
edited
Loading

@nukedupe Please have a look at CI:

not ok 111 can detect changes
# (from function `assert_output' in file test/test_helper/bats-assert/src/assert.bash, line 231,
#  in test file test/mail_ssl_letsencrypt.bats, line 116)
#   `assert_output --partial "postfix: stopped"' failed
# 
# -- output does not contain substring --
# substring (1 lines):
#   postfix: stopped
# output (4 lines):
#   [[ TASKS ]]  2021-05-12 10:07:45  Start check-for-changes script.
#   [[  INF  ]]  2021-05-12 10:07:45  Using postmaster address [email protected]
#   Setting up watches.  Beware: since -r was given, this may take a while!
#   Watches established.
# --
#

Your changes do not actually achieve what you intended, or CI needs adjustments. The former is way more likely.

@georglauterbach
Copy link
Copy Markdown
Member Author

georglauterbach commented May 12, 2021

@docker-mailserver/maintainers This is ready for review :)

casperklein
casperklein reviewed May 12, 2021
View reviewed changes
Comment thread target/scripts/check-for-changes.sh Outdated
/etc/letsencrypt/acme.json \
/etc/letsencrypt/live
inotifywait -qq -r \
-e modify -e create -e delete -e moved \
Copy link
Copy Markdown
Member

@casperklein casperklein May 12, 2021

Choose a reason for hiding this comment

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

There is no "move" event: https://man.cx/inotifywait#heading7

Copy link
Copy Markdown
Member Author

@georglauterbach georglauterbach May 12, 2021

Choose a reason for hiding this comment

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

You mean, there is no "moved" , event? I will correct this, the event is called "move", without the "d". Is that what you mean?

Copy link
Copy Markdown
Member Author

@georglauterbach georglauterbach May 12, 2021

Choose a reason for hiding this comment

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

Interestingly, tests fail again. Oh nice.

Copy link
Copy Markdown

@nukedupe nukedupe May 12, 2021

Choose a reason for hiding this comment

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

My mistake, I haven't tested letsencrypt stuff. Seems like there is a problem with inotifywait on volume mounts.
echo "" >> /tmp/docker-mailserver/postfix-aliases.cf works
echo "" >> /etc/letsencrypt/acme.json works not

Does something speak against using sha512sum -c --status /tmp/docker-mailserver-config-chksum and check return value and not write to temp file and do cmp --silent ... ?

Copy link
Copy Markdown
Member Author

@georglauterbach georglauterbach May 12, 2021

Choose a reason for hiding this comment

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

My mistake, I haven't tested letsencrypt stuff. Seems like there is a problem with inotifywait on volume mounts.

echo "" >> /tmp/docker-mailserver/postfix-aliases.cf works

echo "" >> /etc/letsencrypt/acme.json works not

Does something speak against using sha512sum -c --status /tmp/docker-mailserver-config-chksum and check return value and not write to temp file and do cmp --silent ... ?

No, nothing speaks against it. Can you open a PR yourself? I would then close this PR.

Copy link
Copy Markdown

@nukedupe nukedupe May 12, 2021

Choose a reason for hiding this comment

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

Here is a better solution. But my PR is not in this PR... have to learn git... Can you fix it?

Copy link
Copy Markdown
Member Author

@georglauterbach georglauterbach May 13, 2021

Choose a reason for hiding this comment

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

Sorry, very low on time lately. Learning GIT is always beneficial, trust me :) I will close this - and if you have a working solution, create a pull request on GitHub.

PS: Have a look at our CONTRIBUTING.md, which explains how to do a pull request and add code on detail :)

@georglauterbach georglauterbach deleted the disk-io branch May 13, 2021 08:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@casperklein casperklein casperklein left review comments

+1 more reviewer

@nukedupe nukedupe nukedupe left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

@georglauterbach georglauterbach

Labels

area/scripts kind/improvement Improve an existing feature, configuration file or the documentation priority/low

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

[FR] Reduce unnecessary disk IO

3 participants

@georglauterbach @casperklein @nukedupe