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

Upgrade base image to Debian bookworm #22044

Merged
merged 4 commits into from
Mar 25, 2024
Merged

Conversation

diox
Copy link
Member

@diox diox commented Mar 20, 2024

Fixes mozilla/addons#9507

Description

This PR updates the base docker image addons-server from Debian buster to bookworm

Context

Debian buster is in long-term support mode, but that will end soon. On top of this, the python docker images for buster are not longer receiving updates. We could switch to bullseye but bookworm is the current stable version and will be supported for longer

Testing

  • Run make clean_docker or equivalent to make sure you're rebuilding your deps
  • Build the image with make build_docker_image
  • If it builds correctly:
    • Stop your containers
    • Tag the newly built image to replace mozilla/addons-server
    • Restart the containers to pick it up
  • Test your env by clicking around, running tests etc.
  • make shell followed by cat /etc/debian_version should return 12.5

@diox
Copy link
Member Author

diox commented Mar 20, 2024

Build failed

Weird, it built just fine locally...

@KevinMind
Copy link
Contributor

I ran into this error previously due to the way we are installing the mysql packages.

@KevinMind
Copy link
Contributor

Exception: Can not find valid pkg-config name.
mozilla/addons-server#21 7.197       Specify MYSQLCLIENT_CFLAGS and MYSQLCLIENT_LDFLAGS env vars manually

This was solved on Mac by changing to -default for the mysql packages. Might also have to manually install pkg-config? We should check but this seems like a platform issue.

@diox
Copy link
Member Author

diox commented Mar 20, 2024

I expected pkg-config to be installed by default, but I guess it's not... I've added it in a27fbf9. I suspect because of the platform it's somehow not needed on my machine but might here, as a fallback of some kind ?

@diox diox marked this pull request as draft March 20, 2024 10:50
@KevinMind
Copy link
Contributor

Interestingly enough, pkg-config is not installed in either buster or bookworm. I determined this by running

container-diff analyze python:3.10-slim-buster --type=apt

If you change the image to mozilla/addons-server:latest you'll see it's installed.

Tried it for both, and did not find it. So it must be getting installed via one of the other packages we install. You've changed the mysql package target.

@diox
Copy link
Member Author

diox commented Mar 20, 2024

Since uwsgi is broken locally with this I wanted to look into adding a system check for it (which might prove tricky as I don't really want to launch the command... but anyway) and noticed the smoke test step in the verify docker image looks weird: why is it displaying pip/npm installs ? nvm, we're doing a make update_deps so that makes sense. It just makes the whole thing quite noisy.

@diox
Copy link
Member Author

diox commented Mar 20, 2024

We'll need to add a test to our verify docker image step that tries to run uwsgi (possibly celery as well), since that's currently failing locally...

pip install -I uwsgi --no-cache fixes it (without --no-cache it doesn't!) but I'm not yet sure what's going on.

@diox
Copy link
Member Author

diox commented Mar 20, 2024

Our theory is that locally, we're using dependencies from /deps mounted from the local volume and so, if you don't explicitly rebuild deps, you might end up with old ones (on top of having a potentially broken pip cache). We'll need to find a way to address this that doesn't completely negate the benefits of caching...

@KevinMind
Copy link
Contributor

Proposal:

initialize_docker should have a dependent target clean_docker

This should

  • remove ./deps/*
  • prune docker layer/volume/image cache
  • remove ./docker-cache/*

This means if you run initialize_docker or clean_docker you kill your cache and the next run should be totally fresh.

@diox
Copy link
Member Author

diox commented Mar 20, 2024

I agree, but that wouldn't have been enough here, I wanted to keep my database contents so I wouldn't have run make initialize_docker

@KevinMind
Copy link
Contributor

We could have the docker clean part skip the database? You could also say that the expectation is you try update_docker, if that doesn't work you can try nuking with initialize_docker.

I would actually have expected initialize_docker to run "fresh" by default.

@diox
Copy link
Member Author

diox commented Mar 20, 2024

Yeah, something along those lines sound good. As a rule anyway, when we upgrade the base image, we should clean deps completely... I am not sure this is a use-case worth automating though.

@KevinMind
Copy link
Contributor

That's why I think it should just be a command you can run arbitrarily. If you run clean_docker it will blow up everything, but for now at least we can not automatically trigger it.

@KevinMind
Copy link
Contributor

@diox something like this? #22050

@diox
Copy link
Member Author

diox commented Mar 21, 2024

@diox something like this? #22050

Yes, that sounds great, I'll try this. Meanwhile I've added a simple system check that we're able to run uwsgi in this PR: 146fc15

I still need to file an issue for the debug-toolbar bug but it's a regression from #21795, not this PR, so I won't address it here.

@diox diox marked this pull request as ready for review March 21, 2024 10:31
@diox diox requested review from KevinMind and eviljeff March 21, 2024 11:56
@KevinMind
Copy link
Contributor

Can you rebase.

@diox diox force-pushed the debian-bookworm branch from 146fc15 to 5c102a0 Compare March 22, 2024 09:53
@diox
Copy link
Member Author

diox commented Mar 22, 2024

Rebased (now contains mozilla/addons#9512 debug-toolbar fix as well)

@eviljeff eviljeff removed their request for review March 25, 2024 09:46
@diox diox force-pushed the debian-bookworm branch from 5c102a0 to bc2f0c0 Compare March 25, 2024 09:59
@KevinMind
Copy link
Contributor

@diox can you include how we should consider verifying this locally? I'm sure you have established a set of steps after going over and over yourself. How do I know this is working?

custom_setup = 'setup'


@register(CustomTags.custom_setup)
Copy link
Contributor

Choose a reason for hiding this comment

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

Just to clarify, this check is going to be executed when we run

./manage.py check

Probably worth specifying that in the PR description as it is less than obvious, and I'm not 100% sure it is true even now.

Copy link
Member Author

Choose a reason for hiding this comment

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

I added a docstring in c8c6a3f - the test I had added before should prove it's ran as part of manage.py check, which we trigger in the verify docker image GitHub Action. I've also made sure the check does fail if uwsgi isn't rebuilt.

@diox
Copy link
Member Author

diox commented Mar 25, 2024

Added instructions. One thing I noticed is that we should also upgrade CircleCI base image to be closer to debian bookworm just in case. Actually, CircleCI is already running cimg/python3.10-node which is based on Ubuntu 22.04, which is the closest thing to debian bookworm we have as far as CircleCI python convenience images go

@diox diox requested a review from KevinMind March 25, 2024 14:19
Copy link
Contributor

@KevinMind KevinMind left a comment

Choose a reason for hiding this comment

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

Verified locally.

@diox diox merged commit 0f034f4 into mozilla:master Mar 25, 2024
14 checks passed
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.

Upgrade base image from debian buster to bullseye or bookworm
2 participants