public inbox for pmg-devel@lists.proxmox.com
 help / color / mirror / Atom feed
From: Stoiko Ivanov <s.ivanov@proxmox.com>
To: Dominik Csapak <d.csapak@proxmox.com>
Cc: pmg-devel@lists.proxmox.com
Subject: Re: [pmg-devel] [PATCH pmg-api/docs] make filter timeout configurable
Date: Fri, 12 Jan 2024 20:59:40 +0100	[thread overview]
Message-ID: <20240112205940.71d776f5@rosa.proxmox.com> (raw)
In-Reply-To: <eda519de-d269-4e94-9ba6-0ab4ca7567ed@proxmox.com>

Thank you very much for the review, testing and the discussion!

Just summing up some points from the talks:

On Fri, 12 Jan 2024 09:35:09 +0100
Dominik Csapak <d.csapak@proxmox.com> wrote:

> gave this a look & spin, and it works as intended
> 
> writing here what we discussed off-list:
> 
> there is still the same race when processing + the actual action
> takes longer than the time out, but that is no trivially fixable
One option to handle this would be to 'dry-run' the (final) actions
for each target-group, assuming that they work, i.e.:
that the mail is considered 'delivered' for accept and quarantine, and
'blocked' for block. with that information pmg-smtp-filter could
answer to the sending postfix, and then start processing the actions.
It would only need to generate a bounce if an accepted mail is not
successfully sent to the postfix on 10025.

But this entails refactoring PMG::SMTP and pmg-smtp-filter -
pmg-smtp-filter would need to return after processing who,what,when
and then SMTP would need to call a second function which deals with
running the actions, writing statistics, ...)
> 
> for "normal" timeouts (e.g. the default of 600s) this should not
> matter much, except for pathological cases where e.g. writing
> to the quarantine takes an absurd amount of time

In case a quarantine insert takes so long (and the processing of a mail
took 599s before) - the issue of getting a mail multiple times is probably
not your largest problem.

> 
> secondly, we probably should adapt the timeouts for virus+custom check scripts
> to the configured one too (or e.g. half) but that can be done afterwards
Thinking about it - I think min($filter_timeout, 5*60) (5 minutes is the
current timeout for virus+custom-checks, might make for a robust change.
splitting the timeout seems odd (just because clamAV needs more than
half the timeout, does not mean that custom checks + avast (if configured
at all) or spamassassin won't finish in a very short time.

as an alternative (which I'd only consider if someone actually runs into
such an issue) - we might introduce timeouts for each of the potentially
long-running things.


> and does not impact the actual issue here
> (except that probably the command runs unnecessarily long)
> 
> IMHO we should still mark the increased timeout for before queue filter
> in the next release notes, since that can be a bit unexpected
Good point - definitely worth mentioning

> 
> so from my side, this series is:
> 
> Reviewed-by: Dominik Csapak <d.csapak@proxmox.com>
> Tested-by: Dominik Csapak <d.csapak@proxmox.com>
> 





      reply	other threads:[~2024-01-12 20:00 UTC|newest]

Thread overview: 10+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2023-09-11 14:23 Stoiko Ivanov
2023-09-11 14:23 ` [pmg-devel] [PATCH pmg-api 1/4] templates: postfix: set same timeouts for before and after-queue Stoiko Ivanov
2023-09-11 14:23 ` [pmg-devel] [PATCH pmg-api 2/4] pmg-smtp-filter: refactor use of gettimeofday Stoiko Ivanov
2023-09-11 14:23 ` [pmg-devel] [PATCH pmg-api 3/4] config: postfix: make smtp-filter-timeout configurable Stoiko Ivanov
2024-01-12  9:15   ` Thomas Lamprecht
2023-09-11 14:23 ` [pmg-devel] [PATCH pmg-api 4/4] pmg-smtp-filter: die if processing took longer than the timeout Stoiko Ivanov
2024-01-12  9:19   ` Thomas Lamprecht
2023-09-11 14:23 ` [pmg-devel] [PATCH pmg-docs 1/1] doc-generator: add new option filter_timeout Stoiko Ivanov
2024-01-12  8:35 ` [pmg-devel] [PATCH pmg-api/docs] make filter timeout configurable Dominik Csapak
2024-01-12 19:59   ` Stoiko Ivanov [this message]

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20240112205940.71d776f5@rosa.proxmox.com \
    --to=s.ivanov@proxmox.com \
    --cc=d.csapak@proxmox.com \
    --cc=pmg-devel@lists.proxmox.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox
Service provided by Proxmox Server Solutions GmbH | Privacy | Legal