chore: Remove delay starting the change detection service#3064
Conversation
There should be no concern with conflicts as any writes should have already been done by the time this daemon service is started.
The change event for adding a user can be processed much sooner now, which means Amavis may not yet be ready. Added extra condition to wait on at least the Amavis port being reachable, and some failure asserts with the mail queue to better catch / debug when this problem occurs.
polarathene
added
area/scripts
kind/improvement
polarathene
changed the title
|
|
||
| _wait_for_smtp_port_in_container | ||
| # Even if the Amavis port is reachable at this point, it may still refuse connections? | ||
| _wait_for_tcp_port_in_container 10024 |
There was a problem hiding this comment.
Do we want to wait for port 10025 too?
There was a problem hiding this comment.
I'm not actually sure when these are used 😅
Perhaps I'm listening on the wrong port since 10024 appears to be for sending and 10025 for receiving? I just know that when I queried the mail queue, sometimes a connection refused error mentioned port 10024.
You could do me a favour and run the tests locally a few times so we can be more sure about not having introduced a new race condition here.
I ran it many times actually and wasn't able to produce it locally. I had this change ready around a month ago, but back then it occurred more often. I had a solution to delay sending mail a little bit with sleep 1:
# NOTE: If change detection begins too early, it can cause connection issues with Amavis.
# `sleep 1` can be added before or after to prevent that from happening.
sleep 1I am pretty sure that the issue is more to do with Amavis not being ready quick enough, as before with the 10 sec change event delay, there was plenty of time. There might be a small window after the change event with the reloads on amavis and postfix (plus dovecot for delivery I guess).
I'll be getting some rest, but will address this concern when I'm back 👍
There was a problem hiding this comment.
I'll be getting some rest, but will address this concern when I'm back 👍
Alright! I too think this is about a Amavis-timing-issue. I'd add the check for port 10025 and I'm not against a small sleep as mentioned in your comment (with additional info on why it's needed, just like in your comment).
There was a problem hiding this comment.
I'd add the check for port 10025
Will this be added or should I approve now?
There was a problem hiding this comment.
I'll update it, sorry been a bit busy and since CI is broken presently for tests it seems I should wait until we've resolved that 😅
There was a problem hiding this comment.
I'd add the check for port 10025
Will this be added or should I approve now? 😆
There was a problem hiding this comment.
Oh sorry, I don't believe there needs to be a check for port 10025. AFAIK this is a port on Postfix to receive mail back from Amavis after it's done processing. I am doubtful it'll make a difference when there's a connection error logged about port 10024. The sleep should give Amavis enough time.
|
@casperklein ready to approve 😅 |
If you're really short on time, just let us briefly know :) Maybe we can then go ahead and merge a few PRs with only reviewer :) |
|
🤦🏼 I did not see auto-merge was enabled... The merge happened because I changed the required reviewers for the time being to 1. Rationale: This project is maintained by volunteers, and we cannot ask people to spend time on it when they do not have time for it. On the other hand, we have at least 1 reviewer available at the moment, so I think for the time being, it is safe to merge some PRs with only 1 approval, especially when PRs are waiting for a longer time to get merged (hindering more work to happen ATM). Best case would be to have 2 frequent reviewers again. @casperklein just keep us updated on your current status, I can change the required approvals back at any time :) I also wanted to ask @polarathene if you're fine with this being merged before actually merging it, but the auto-merge was just too fast.. :D |
I think that this is the right thing to do. We have not been getting new maintainers joining, and many of us are strained for time. I know I've over-committed myself already and need to focus on other tasks outside of DMS. As much as I'd like to contribute more, it's a huge time sink that's difficult to justify the amount of free time I invest into it 😅 |
|
No worries, you have already contributed so much and you added so much value to this project ❤️ I know from experience that maintaing takes a lot of time, absolutely right :D I think what would be most valuable is to still have a few people around for reviewing some of the proposed changes - one does not need to invest heavily and doing changes themselves; reviewing is very important in the end :) |
I am still here. I just had some two extreme busy weeks and had no time for reviews. If PRs are not related to the current test improvements, I'll review them in more depth. So I don't approve them in hurry to get them merged asap (without reviewing them properly). I will take a look at the merged PRs, in case something important was missed. |
I hope I did not offend you in any way :) I am going to be on vacation and I'll be very busy the next weeks, so I wanted to get some PRs out of the way, to have peace of mind during my vacation :D
👍🏼 |
No you didn't. In march I'll be more active again. |
3 participants
Description
There does not seem to be a need anymore for delaying the service availability by 10 seconds when starting a container.
AFAIK, with the current start-up scripts the change detector service should not have an issue with conflicting writes:
History + References
sleep 5.sleep 10and re-located to sleep at a later point.check-for-changes.sh): Drop redundant guards #2623Change Detector start-up flow (preparing initial checksum)
_setup=>_prepare_for_change_detection=>_monitored_files_checksums >"${CHKSUM_FILE}"docker-mailserver/target/scripts/start-mailserver.sh
Lines 228 to 242 in 00b1d88
docker-mailserver/target/scripts/startup/setup-stack.sh
Lines 3 to 13 in 00b1d88
docker-mailserver/target/scripts/helpers/change-detection.sh
Lines 9 to 21 in 00b1d88
Type of change
Checklist: