Page MenuHomeSoftware Heritage

Instanciate the SMTP class only when needed
ClosedPublic

Authored by douardda on Aug 5 2022, 7:21 PM.

Details

Summary

instead of creating it in the VaultBackend constructor: when configured
(with host and port), SMTP.connect() is immediately called, which makes
it mandatory to have the smtp server up and running to be able to create
the VaultBackend object (which makes it hard to run properly in an elastic
environment like docker or k8s).

Event Timeline

Build is green

Patch application report for D8205 (id=29604)

Rebasing onto ca71a59eda...

Current branch diff-target is up to date.
Changes applied before test
commit 18c958a42ba69823e93788e3c81e07b66311dbf5
Author: David Douard <david.douard@sdfa3.org>
Date:   Fri Aug 5 19:16:21 2022 +0200

    Instanciate the SMTP class only when needed
    
    instead of creating it in the VaultBackend constructor: when configured
    (with host and port), SMTP.connect() is immediately called, which makes
    it mandatory to have the smtp server up and running to be able to create
    the VaultBackend object (which makes it hard to run properly in an elastic
    environment like docker or k8s).

See https://jenkins.softwareheritage.org/job/DVAU/job/tests-on-diff/250/ for more details.

vlorentz added inline comments.
swh/vault/backend.py
490–500

We can keep the previous behavior of leaving connections open.

Why "this is doomed"?

This revision now requires changes to proceed.Aug 10 2022, 10:03 AM
swh/vault/backend.py
490–500

We can keep the previous behavior of leaving connections open.

why would we want this? What are the pros of keeping it open?

Why "this is doomed"?

because there is no reason to believe there is someone listening on localhost:25 (especially in elastic envs etc).
Once again, what is the advantage of hardwriting this fallback?

Remove the fallback to 127.0.0.1:25 but log delivery failure instead

vlorentz added inline comments.
swh/vault/backend.py
490–500

why would we want this? What are the pros of keeping it open?

less latency, but it's not a big deal

Once again, what is the advantage of hardwriting this fallback?

I'm fine with removing the fallback, I just didn't understand the comment.

This revision is now accepted and ready to land.Sep 14 2022, 4:23 PM

Build is green

Patch application report for D8205 (id=30529)

Rebasing onto ee678042f9...

Current branch diff-target is up to date.
Changes applied before test
commit cab17e72537b4188a2df4e2bedc6afa92d4f8810
Author: David Douard <david.douard@sdfa3.org>
Date:   Fri Aug 5 19:16:21 2022 +0200

    Instanciate the SMTP class only when needed
    
    instead of creating it in the VaultBackend constructor: when configured
    (with host and port), SMTP.connect() is immediately called, which makes
    it mandatory to have the smtp server up and running to be able to create
    the VaultBackend object (which makes it hard to run properly in an elastic
    environment like docker or k8s).
    
    This also removes the fallback to hardcoded 'localhost:25' smtp server;
    if the smtp server is not configured or not reachable, the call to
    _smtp_send() will fail logging the failure (both in logging and sentry).

See https://jenkins.softwareheritage.org/job/DVAU/job/tests-on-diff/253/ for more details.