From 0f1611286da14ecb6b03b2016c6f61fcec6e772a Mon Sep 17 00:00:00 2001 From: Alex Vandiver Date: Tue, 11 May 2021 01:32:31 +0000 Subject: [PATCH] management: Rename the deliver_email command to deliver_scheduled_email. This makes it parallel with deliver_scheduled_messages, and clarifies that it is not used for simply sending outgoing emails (e.g. the `email_senders` queue). This also renames the supervisor job to match. --- docs/subsystems/email.md | 2 +- .../check_email_deliverer_process | 2 +- .../supervisor/zulip.conf.template.erb | 10 +++++----- zerver/lib/send_email.py | 2 +- ...er_email.py => deliver_scheduled_emails.py} | 18 ++++++++++-------- .../commands/print_email_delivery_backlog.py | 2 +- zerver/tests/test_signup.py | 4 ++-- zerver/tests/test_users.py | 14 +++++++++----- zproject/computed_settings.py | 2 +- zproject/default_settings.py | 2 +- 10 files changed, 32 insertions(+), 26 deletions(-) rename zerver/management/commands/{deliver_email.py => deliver_scheduled_emails.py} (78%) diff --git a/docs/subsystems/email.md b/docs/subsystems/email.md index c076689cd4..bc615c842f 100644 --- a/docs/subsystems/email.md +++ b/docs/subsystems/email.md @@ -23,7 +23,7 @@ with only a few things you need to know to get started. are several other functions in `zerver.lib.send_email`, but all of them eventually call the `send_email` function. The most interesting one is `send_future_email`. The `ScheduledEmail` entries are eventually processed - by a supervisor job that runs `zerver/management/commands/deliver_email.py`. + by a supervisor job that runs `zerver/management/commands/deliver_scheduled_emails.py`. * Always use `user_profile.delivery_email`, not `user_profile.email`, when passing data into the `send_email` library. The `user_profile.email` field may not always be valid. diff --git a/puppet/zulip/files/nagios_plugins/zulip_app_frontend/check_email_deliverer_process b/puppet/zulip/files/nagios_plugins/zulip_app_frontend/check_email_deliverer_process index 6fcb996a20..6754519337 100755 --- a/puppet/zulip/files/nagios_plugins/zulip_app_frontend/check_email_deliverer_process +++ b/puppet/zulip/files/nagios_plugins/zulip_app_frontend/check_email_deliverer_process @@ -5,7 +5,7 @@ #"CRITICAL": 2 #"UNKNOWN": 3 -SUPERVISOR_STATUS=$(supervisorctl status zulip-workers:zulip_deliver_enqueued_emails 2>&1) +SUPERVISOR_STATUS=$(supervisorctl status zulip-workers:zulip_deliver_scheduled_emails 2>&1) STATUS=$(echo "$SUPERVISOR_STATUS" | awk '{ print $2 }') case "$STATUS" in diff --git a/puppet/zulip/templates/supervisor/zulip.conf.template.erb b/puppet/zulip/templates/supervisor/zulip.conf.template.erb index b9a35f2282..ace66a3057 100644 --- a/puppet/zulip/templates/supervisor/zulip.conf.template.erb +++ b/puppet/zulip/templates/supervisor/zulip.conf.template.erb @@ -94,8 +94,8 @@ stopasgroup=true ; Without this, we leak processes every restart killasgroup=true ; Without this, we leak processes every restart <% end %> -[program:zulip_deliver_enqueued_emails] -command=nice -n15 /home/zulip/deployments/current/manage.py deliver_email +[program:zulip_deliver_scheduled_emails] +command=nice -n15 /home/zulip/deployments/current/manage.py deliver_scheduled_emails environment=HTTP_proxy="<%= @proxy %>",HTTPS_proxy="<%= @proxy %>" priority=350 ; the relative start priority (default 999) autostart=true ; start at supervisord start (default: true) @@ -104,7 +104,7 @@ stopsignal=TERM ; signal used to kill process (default TERM) topwaitsecs=30 ; max num secs to wait b4 SIGKILL (default 10) user=zulip ; setuid to this UNIX account to run the program redirect_stderr=true ; redirect proc stderr to stdout (default false) -stdout_logfile=/var/log/zulip/events_deliver_enqueued_emails.log ; stdout log path, NONE for none; default AUTO +stdout_logfile=/var/log/zulip/events_deliver_scheduled_emails.log ; stdout log path, NONE for none; default AUTO stdout_logfile_maxbytes=20MB ; max # logfile bytes b4 rotation (default 50MB) stdout_logfile_backups=3 ; # of stdout logfile backups (default 10) directory=/home/zulip/deployments/current/ @@ -131,9 +131,9 @@ directory=/home/zulip/deployments/current/ [group:zulip-workers] <% if @queues_multiprocess %> ; each refers to 'x' in [program:x] definitions -programs=zulip_deliver_enqueued_emails, zulip_deliver_scheduled_messages, <% @queues.each_with_index do |queue, i| -%>zulip_events_<%= queue %><%= ',' if i < (@queues.size - 1) %> <% end -%> +programs=zulip_deliver_scheduled_emails, zulip_deliver_scheduled_messages, <% @queues.each_with_index do |queue, i| -%>zulip_events_<%= queue %><%= ',' if i < (@queues.size - 1) %> <% end -%> <% else %> -programs=zulip_deliver_enqueued_emails, zulip_events, zulip_deliver_scheduled_messages +programs=zulip_deliver_scheduled_emails, zulip_events, zulip_deliver_scheduled_messages <% end %> ; The [include] section can just contain the "files" setting. This diff --git a/zerver/lib/send_email.py b/zerver/lib/send_email.py index a5de64c66c..2da7139b17 100644 --- a/zerver/lib/send_email.py +++ b/zerver/lib/send_email.py @@ -415,7 +415,7 @@ def handle_send_email_format_changes(job: Dict[str, Any]) -> None: del job["to_user_id"] -def deliver_email(email: ScheduledEmail) -> None: +def deliver_scheduled_emails(email: ScheduledEmail) -> None: data = orjson.loads(email.data) user_ids = list(email.users.values_list("id", flat=True)) if not user_ids and not email.address: diff --git a/zerver/management/commands/deliver_email.py b/zerver/management/commands/deliver_scheduled_emails.py similarity index 78% rename from zerver/management/commands/deliver_email.py rename to zerver/management/commands/deliver_scheduled_emails.py index c7571dc617..0be5607435 100644 --- a/zerver/management/commands/deliver_email.py +++ b/zerver/management/commands/deliver_scheduled_emails.py @@ -1,11 +1,13 @@ """\ -Deliver email messages that have been queued by various things -(at this time invitation reminders and day1/day2 followup emails). +Send email messages that have been queued for later delivery by +various things (at this time invitation reminders and day1/day2 +followup emails). This management command is run via supervisor. Do not run on multiple machines, as you may encounter multiple sends in a specific race condition. (Alternatively, you can set `EMAIL_DELIVERER_DISABLED=True` on all but one machine to make the command have no effect.) + """ import logging import time @@ -17,7 +19,7 @@ from django.utils.timezone import now as timezone_now from zerver.lib.logging_util import log_to_file from zerver.lib.management import sleep_forever -from zerver.lib.send_email import EmailNotDeliveredException, deliver_email +from zerver.lib.send_email import EmailNotDeliveredException, deliver_scheduled_emails from zerver.models import ScheduledEmail ## Setup ## @@ -26,12 +28,12 @@ log_to_file(logger, settings.EMAIL_DELIVERER_LOG_PATH) class Command(BaseCommand): - help = """Deliver emails queued by various parts of Zulip -(either for immediate sending or sending at a specified time). + help = """Send emails queued by various parts of Zulip +for later delivery. -Run this command under supervisor. This is for SMTP email delivery. +Run this command under supervisor. -Usage: ./manage.py deliver_email +Usage: ./manage.py deliver_scheduled_emails """ def handle(self, *args: Any, **options: Any) -> None: @@ -46,7 +48,7 @@ Usage: ./manage.py deliver_email if email_jobs_to_deliver: for job in email_jobs_to_deliver: try: - deliver_email(job) + deliver_scheduled_emails(job) except EmailNotDeliveredException: logger.warning("%r not delivered", job) time.sleep(10) diff --git a/zerver/management/commands/print_email_delivery_backlog.py b/zerver/management/commands/print_email_delivery_backlog.py index a60070e8b4..e5312aa16a 100644 --- a/zerver/management/commands/print_email_delivery_backlog.py +++ b/zerver/management/commands/print_email_delivery_backlog.py @@ -14,7 +14,7 @@ class Command(BaseCommand): help = """Shows backlog count of ScheduledEmail (The number of currently overdue (by at least a minute) email jobs) -This is run as part of the nagios health check for the deliver_email command. +This is run as part of the nagios health check for the deliver_scheduled_emails command. Usage: ./manage.py print_email_delivery_backlog """ diff --git a/zerver/tests/test_signup.py b/zerver/tests/test_signup.py index 9345d2610d..2747b3bd2e 100644 --- a/zerver/tests/test_signup.py +++ b/zerver/tests/test_signup.py @@ -63,7 +63,7 @@ from zerver.lib.rate_limiter import add_ratelimit_rule, remove_ratelimit_rule from zerver.lib.send_email import ( EmailNotDeliveredException, FromAddress, - deliver_email, + deliver_scheduled_emails, send_future_email, ) from zerver.lib.stream_subscription import get_stream_subscriptions_for_user @@ -1814,7 +1814,7 @@ so we didn't send them an invitation. We did send invitations to everyone else!" self.assertEqual(len(email_jobs_to_deliver), 1) email_count = len(outbox) for job in email_jobs_to_deliver: - deliver_email(job) + deliver_scheduled_emails(job) self.assertEqual(len(outbox), email_count + 1) self.assertEqual(self.email_envelope_from(outbox[-1]), settings.NOREPLY_EMAIL_ADDRESS) self.assertIn(FromAddress.NOREPLY, self.email_display_from(outbox[-1])) diff --git a/zerver/tests/test_users.py b/zerver/tests/test_users.py index 571a1a01d8..f76469eff7 100644 --- a/zerver/tests/test_users.py +++ b/zerver/tests/test_users.py @@ -28,7 +28,11 @@ from zerver.lib.avatar import avatar_url, get_gravatar_url from zerver.lib.create_user import copy_user_settings from zerver.lib.events import do_events_register from zerver.lib.exceptions import JsonableError -from zerver.lib.send_email import clear_scheduled_emails, deliver_email, send_future_email +from zerver.lib.send_email import ( + clear_scheduled_emails, + deliver_scheduled_emails, + send_future_email, +) from zerver.lib.stream_topic import StreamTopicTarget from zerver.lib.test_classes import ZulipTestCase from zerver.lib.test_helpers import ( @@ -1420,7 +1424,7 @@ class ActivateTest(ZulipTestCase): self.assertEqual(ScheduledEmail.objects.filter(users=hamlet).count(), 0) self.assertEqual(ScheduledEmail.objects.filter(users=iago).count(), 1) - def test_deliver_email(self) -> None: + def test_deliver_scheduled_emails(self) -> None: iago = self.example_user("iago") hamlet = self.example_user("hamlet") send_future_email( @@ -1431,7 +1435,7 @@ class ActivateTest(ZulipTestCase): ) self.assertEqual(ScheduledEmail.objects.count(), 1) email = ScheduledEmail.objects.all().first() - deliver_email(email) + deliver_scheduled_emails(email) from django.core.mail import outbox self.assertEqual(len(outbox), 1) @@ -1445,7 +1449,7 @@ class ActivateTest(ZulipTestCase): ) self.assertEqual(ScheduledEmail.objects.count(), 0) - def test_deliver_email_no_addressees(self) -> None: + def test_deliver_scheduled_emails_no_addressees(self) -> None: iago = self.example_user("iago") hamlet = self.example_user("hamlet") to_user_ids = [hamlet.id, iago.id] @@ -1460,7 +1464,7 @@ class ActivateTest(ZulipTestCase): email.users.remove(*to_user_ids) with self.assertLogs("zulip.send_email", level="INFO") as info_log: - deliver_email(email) + deliver_scheduled_emails(email) from django.core.mail import outbox self.assertEqual(len(outbox), 0) diff --git a/zproject/computed_settings.py b/zproject/computed_settings.py index 96164546f0..e5f16e0e54 100644 --- a/zproject/computed_settings.py +++ b/zproject/computed_settings.py @@ -911,7 +911,7 @@ LOGGING: Dict[str, Any] = { "zerver.lib.digest": { "level": "DEBUG", }, - "zerver.management.commands.deliver_email": { + "zerver.management.commands.deliver_scheduled_emails": { "level": "DEBUG", }, "zerver.management.commands.enqueue_digest_emails": { diff --git a/zproject/default_settings.py b/zproject/default_settings.py index a7f380caf6..a8d726d8d5 100644 --- a/zproject/default_settings.py +++ b/zproject/default_settings.py @@ -325,7 +325,7 @@ REGISTER_LINK_DISABLED: Optional[bool] = None LOGIN_LINK_DISABLED = False FIND_TEAM_LINK_DISABLED = True -# Controls if the server should run certain jobs like deliver_email or +# Controls if the server should run certain jobs like deliver_scheduled_emails or # deliver_scheduled_messages. This setting in long term is meant for # handling jobs for which we don't have a means of establishing a locking # mechanism that works with multiple servers running these jobs.