Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Remove supervisor #22577

Closed
wants to merge 5 commits into from
Closed

Remove supervisor #22577

wants to merge 5 commits into from

Conversation

KevinMind
Copy link
Contributor

@KevinMind KevinMind commented Aug 19, 2024

Relates to: mozilla/addons#14972

Todo:

  • investigate how to more thoroughly test
  • update docs about the container startup and reloading.

Description

Remove supervisor. Logging is handled by docker compose natively "docker compose logs web" Restarting for web is handled using existing uwsgi reload behaviour and celery is handled via django management command reload hook. Running the services is handled directly in docker compose.

Context

Remove supervisor from our development workflow. We use supervisor to manage our web and worker containers. Our current use covers a) running the service command b) running a watcher to restart the service on certain files changing c) routing logs and handling process level information.

Bottom line, we can achieve all 3 of these features with docker compose and existing libraries and tools. Removing supervisor simplifies container startup by removing an unnecessary chain in the startup flow and by reducing the number of dependencies we need to manage.

Testing

make up should work, tests should run, application is renderable. Should also be able to run celery jobs.

Checklist

  • Add #ISSUENUM at the top of your PR to an existing open issue in the mozilla/addons repository.
  • Successfully verified the change locally.
  • The change is covered by automated tests, or otherwise indicated why doing so is unnecessary/impossible.
  • Add or update relevant docs reflecting the changes made.

@KevinMind KevinMind force-pushed the docker-watch branch 7 times, most recently from 1d65e93 to f40d52e Compare August 19, 2024 14:32
@KevinMind KevinMind requested review from a team and diox and removed request for a team August 19, 2024 17:33
@KevinMind KevinMind marked this pull request as ready for review August 19, 2024 17:33
import os

from django.core.management.base import BaseCommand
from django.utils import autoreload
Copy link
Member

Choose a reason for hiding this comment

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

If we're going to use django's autoreload then we need to include pywatchman dependency and set it up. The default implementation in Django when pywatchman isn't present is incredibly inefficient, especially for a large number of files: it basically does a stat() on every file every second. This causes a lot of CPU usage, lags behind when you switch between branches or make several changes in quick succession, it's really terrible in general.

watchmedo, pywatchman and uwsgi autoreload use signals sent by the OS kernel instead (which work even inside docker - at least on Linux). IIRC watchman requires starting the service separately though, so you'd need to handle that.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Found a way to use uwsgi. Means we can continue avoiding the extra dependency for running the servers and not sacrifice those precious cycles.

Copy link
Member

Choose a reason for hiding this comment

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

Using uwsgi to run celery is extremely weird and confusing though. It's not meant for that at all... If this is just for autoreloading, why not keep watchmedo for that ? Or there might be other lightweight alternatives too

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The goal was to not require any additional dependencies.

Why? One of the biggest pain points of getting our local/ci setup to run is we need dev dependencies to even get the server up. Being able to run the server without extra dependencies and letting the dev deps just "enhance" simplifies the process A LOT.

I understand it's not totally orthodox, but is there a specific thing you can see wrong? It's really just a process runner.

  1. fewer dependencies to update
  2. smaller image
  3. easier to startup the server.

I see your point in the abstract, but practically speaking, I'm seeing a lot more upside than downside.

Copy link
Member

Choose a reason for hiding this comment

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

uwsgi is on maintenance mode these days, it has fairly obscure bugs and behaviors that sometimes go unfixed for months (some we experienced ourselves over the past few years). I would rather we start moving away from it (would unblock mozilla/addons#1994 (comment) if we were to switch to https://github.com/nginx/unit/ for instance) than depend even more on it for something that it was never meant to do anyway.

We have watchmedo already, I don't see the benefit in swapping it with uwsgi if we just care about the autoreload.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

watchmedo is a dev dependency, uwsgi is a prod dependency. That is the whole point. If we used nginx/unit would it be a dev or prod dependency?

I'm totally fine moving away from uwsgi in the future, even soon. But for this PR. We already use uwsgi for the main server so we are not exposing ourselves to any additional set of bugs we didn't already have. Also unit doesn't seem to handle the autoreloading problem at all (haven't looked deeply but it is not clearly advertised at least)

Is there a possibility to include watchdog in prod dependencies? That could simplify this quite a bit as we could use watchdog from docker compose directly.

Copy link
Member

Choose a reason for hiding this comment

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

We could add watchdog[watchmedo] to prod deps, yes. It doesn't run anything by default, is not imported anywhere, so it should be fine.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

👍

@KevinMind KevinMind requested a review from diox August 22, 2024 11:31
@KevinMind
Copy link
Contributor Author

Close in favor of #22592

@KevinMind KevinMind closed this Aug 23, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants