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

Freeze dependencies on version they have at deployment #353

Merged
merged 2 commits into from
May 17, 2021

Conversation

DasSkelett
Copy link
Member

@DasSkelett DasSkelett commented May 15, 2021

Problem

Right now, every restart of the production instance (or alpha/beta as well) upgrades all Python dependencies to their very latest version, if unpinned (and most of them are).
On alpha, this lead to the backend not coming back up again recently, because it silently upgraded to Flask 2.0, but stayed at Werkzeug 0.16.1 because it is pinned, while Flask 2.0 needs Werkzeug >=2.0 to work.

This could be mitigated by upgrading the installed pip (20.0.2) to the latest version, since pip 20.3 gained a new, fixed dependency resolver, which now installs the latest Flask that still works with Werkzeug 0.16.1 (1.1.4).
If we didn't have Werkzeug pinned, alpha would've been broken as well:

Traceback (most recent call last):
  File "/usr/local/lib/python3.8/site-packages/flask/cli.py", line 240, in locate_app
    __import__(module_name)
  File "/opt/spacedock/KerbalStuff/app.py", line 62, in <module>
    from .middleware.profiler import ConditionalProfilerMiddleware
  File "/opt/spacedock/KerbalStuff/middleware/profiler.py", line 5, in <module>
    from werkzeug.contrib.profiler import ProfilerMiddleware
ModuleNotFoundError: No module named 'werkzeug.contrib'

Werkzeug moved profiler to werkzeug.middleware in 1.0.

Nothing prevents production to fail from similar package updates at any time.
for example, SqlAlchemy will drop soon-ish, and thanks to deprecation warnings I've turned on (see below), I know that our backend will break.

Considerations

We need some way to "pin" package versions on production.
Pinning them to a single version in requirements.txt would be very ugly and probably keep us from every updating any package again.
Bots like dependabot are really annoying.

Not running upgrading packages on production doesn't really work either, otherwise production will always stay at ancients versions, except if we start to specify minimum versions.

Limiting the maximum version isn't very safe either, nothing prevents dependencies from introducing breaking changes in minor version updates (even if they shouldn't).

If we deployed Docker images, this would be a solved problem. The image always keeps whatever dependencies were at when building and testing the image. They're basically baked in and never change, unless a new image gets built, tested and deployed.
Well, we don't deploy Docker containers. So here's my best attempt to mimic it:
Let's upgrade the packages whenever we deploy new changes, but not when simply restarting the server!

Changes

Let's freeze dependencies at whatever version they're at when deploying new production updates.
prepare.sh now checks the commit hash of the last time we restarted production (reading from .last_head), and compares it with the new/current one.

  • If they don't match, i.e. we're deploying new changes to production: install and upgrade all packages from requirements.txt, echo the output of pip freeze into a .frozen-requirements.txt file.
  • If they do match: don't upgrade, but install all packages from this .frozen-requirements.txt instead of requirements.txt.
    Afterwards, put the current commit hash into.last_head for the next time.

Additionally, we're also upgrading pip in prepare.sh, to make sure we benefit from any potential security or bug fixes. I really hope that pip itself doesn't change its basic commandline interface...

It might be enough to just leave out the --upgrade / -U in the second scenario, instead of saving and reading the package versions in a file. But for one I don't fully trust pip, and it would also lead to upgraded packages if we ever removed the venv library for some reason.

This should make sure that package versions on production at least somewhat resemble the state we've tested locally/on alpha/beta / in CI. Sure, there's not guarantee that a breaking update released in the time between we tested things and we deploy on production, or that we actually have the latest dependencies installed in our dev environments, but we're pretty limited in our possibilities here.
It's probably the closest we can get to an idempotent deployment.

Further changes

  • Unpin Werkzeug 0.16.1 – this was done due to an old Flask version being incompatible with Werkzeug 1.0, but newer Flask versions have been fixed (even in the 1.x series).

  • Pin Flask to <2.0, we should do some testing before letting anything upgrade to it.

  • Unping oauthlib 2.0.6 and requests-oauthlib 0.8.0, I have no idea why this was done, but it happened during Alista's great restructuring efforts a long time ago. I think we have all oauth functionality disabled anyway.

  • Pin SQLAlchemy to >=1.4,<2, 1.4 gives us all the deprecation warnings for 2.0, and 2.0 will drop soon-ish and would definitely break.

  • Change the import path of ProfilerMiddleware to fix the above import error

  • Turn on SqlAlchemy 2.0 deprecation warnings when running the Docker stack for development. Can be seen in the backend logs. Had to change the CMD to make the Python binary call the Flask module with -W default, running the Flask binary directly didn't output any warnings. It also ouputs deprecation warnings from other modules now.


Sorry, the PR description got kinda long. But I wanted to make sure that anybody who wants to can understand the problem, and that if we go down this route, future maintainers can re-read why we did what we did back then.

prepare.sh now saves the package versions at the state of deployment of a new commit to alpha/beta/production
into a .frozen-requirements.txt file. As long as the HEAD commit doesn't change, every restart
will install the exact same package versions, to prevent silent, unwanted, breaking upgrades to some degree.

Also pin Flask to <2 and Jinja2 <3 until we tested it.
Prevent SqlAlchemy from upgrading to 2.0 when it is released,
since we use several deprecated functions that will be removed.
Remove some older pins that got added as a result of upstream bugs that should be resolved now.

Change Werkzeug profiler import from `werkzeug.contrib.profiler` to `werkzeug.middleware.profiler`,
it has been moved in 1.0.
@DasSkelett DasSkelett added Type: Bug Area: Backend Related to the Python code that runs inside gunicorn Priority: Normal Status: Need Feedback labels May 15, 2021
@DasSkelett DasSkelett requested a review from HebaruSan May 15, 2021 18:12
prepare.sh Show resolved Hide resolved
Copy link
Contributor

@HebaruSan HebaruSan left a comment

Choose a reason for hiding this comment

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

This seems fine. Is there a way I can trigger it with start-server.sh? I'm not getting the new files generated and it would be nice to take a look at them.

@HebaruSan
Copy link
Contributor

If a production upgrade goes haywire, @V1TA5 can edit .frozen-requirements.txt to customize the pins as needed, right?

@DasSkelett
Copy link
Member Author

This seems fine. Is there a way I can trigger it with start-server.sh? I'm not getting the new files generated and it would be nice to take a look at them.

The Docker containers unfortunately don't use this script.
The way I've tested it was by changing all the bin/pip3 calls with pip3 (make sure you set up a venv in the SpaceDock repo directory if you don't have one yet, and activate it), and commenting out the ./build-frontend.sh line (if you have node installed, you could leave it, but it'll create the static folder with a bunch of files in it, and manipulate the package-lock.json). Then you can run the script right in the SD repo dir.
Alternatively it should also work inside the backend container, where you wouldn't have to create a venv first (but still change the calls).
Or you can use the virtualenv module instead of venv, like we do on the server: virtualenv -p python3 . . This way you could keep the bin/pip3 calls as is.

If a production upgrade goes haywire, @V1TA5 can edit .frozen-requirements.txt to customize the pins as needed, right?

For a quick fix if we need to force an older or newer version, yup. And if we ever want to force an upgrade to all dependencies for whatever reason, we can do that by removing one or both of the new files.

@HebaruSan
Copy link
Contributor

The Docker containers unfortunately don't use this script.

Alright, I'm comfortable just poking at it on alpha then.

@HebaruSan HebaruSan merged commit 88157d8 into KSP-SpaceDock:alpha May 17, 2021
@DasSkelett DasSkelett deleted the fix/pin-all-dependencies branch May 18, 2021 00:12
This was referenced May 21, 2021
@HebaruSan
Copy link
Contributor

HebaruSan commented Nov 16, 2021

  • Turn on SqlAlchemy 2.0 deprecation warnings when running the Docker stack for development. Can be seen in the backend logs.

I was going to ask how to view these logs, but apparently this is how:

docker logs spacedock_backend_1

(Thanks to tiangolo/uwsgi-nginx-flask-docker#6 (comment) for the help!)

@HebaruSan HebaruSan mentioned this pull request Nov 16, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area: Backend Related to the Python code that runs inside gunicorn Priority: Normal Status: Need Feedback Type: Bug
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants