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

Refactor docker setup #134

Draft
wants to merge 3 commits into
base: master
Choose a base branch
from
Draft

Refactor docker setup #134

wants to merge 3 commits into from

Conversation

LiquidPL
Copy link
Contributor

@LiquidPL LiquidPL commented Jan 15, 2023

Resolves #126.

This PR cleans up all the Docker and Docker Compose files used by oioioi.

Notable changes:

  • reworked Dockerfile to use multi-stage builds, now the resulting image doesn't have any leftovers from the build process,
  • the app is now running inside a virtualenv,
  • updated django-supervisor to work with Python 3 properly, and fixed a bug which would sometimes cause it to break in virtualenvs, currently it's hosted in my fork at https://github.com/LiquidPL/django-supervisor, however I can move it to the sio2project org if it's preferred that way,
  • it is now possible to build a smaller, production-ready image that doesn't include apt installed compilers,
  • removed sandboxes from the development image, as mounting a volume/directory to the already existing filetracker database in the image would lead to confusing behaviour (and additionally, the sandboxes would be pointlessly duplicated in every image),
  • added instructions on how to install the sandboxes on a machine without internet connection
  • various settings (database, rabbitmq, judging) that were previously hardcoded are now configurable via environment variables (with original hardcoded values being now configured in compose files)
  • removed duplicated docker container init scripts
  • moved all scripts that are not being run on the host machine to a separate scripts/ directory
  • removed duplicated configuration in docker-compose-selenium.yml
  • changed docker-compose configs to use an external sioworkers image

Some additional things to consider:

  • Is the selenium test suite actively maintained? I've made sure that it at least starts up properly, but from what I've seen all the tests fail and the suite hasn't been touched in a few years.
  • Why is the development server being started as a separate command instead of being managed in supervisor? I haven't seen anything that would suggest it can't be run that way, and this way we could simplify the docker-compose commands by removing extra/sample_configs/basic_settings_noserver.py.
  • This PR still lacks scripts and GitHub Actions workflows to build and publish the Docker images, as I'd prefer to discuss where to publish those images beforehand.
  • There's also two PRs for sioworkers at Fix Sio2Jail executor failing on Python 3 sioworkers#11, Add Dockerfile and CI workflows building Docker images on new releases sioworkers#12.
  • I'd also like to remove the local installation of sioworkers from the image (at least for production builds), however I'm not sure if it's being used anywhere.

@LiquidPL LiquidPL requested a review from twalen as a code owner January 15, 2023 14:26
@tudny

This comment was marked as resolved.

@LiquidPL
Copy link
Contributor Author

It fails because there's no sio2project/sioworkers image published yet.

@tudny
Copy link
Member

tudny commented Jan 15, 2023

Okay, my mistake. Sorry

@LiquidPL
Copy link
Contributor Author

I've pushed an image to liquidpl/sioworkers:test. You can change the tag in https://github.com/gs-hackathon-this-is-fine/oioioi-docker/blob/docker/docker-compose-dev.yml#L46 and run the tests again.

requirements.txt Outdated
@@ -2,7 +2,7 @@
# therefore they must be listed here. Moreover, they cannot be listed in
# setup.py, as pip is not able to install them.
http://github.com/Supervisor/supervisor/zipball/master#egg=supervisor==4.0.0.dev0
http://github.com/badochov/djsupervisor/zipball/master#egg=djsupervisor==0.4.0
http://github.com/LiquidPL/django-supervisor/zipball/master#egg=django-supervisor==0.4.0
Copy link
Contributor

Choose a reason for hiding this comment

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

I prefer to have this repo in sio2project organization

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've opened a pull request to bring in my changes: sio2project/django-supervisor#1.

Copy link
Contributor

Choose a reason for hiding this comment

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

I've merged your PR to django-supervisor

@LiquidPL
Copy link
Contributor Author

I've switched django-supervisor to use the sio2project fork.

I'm going to convert this PR to a draft for the time being, since at this moment it's not going to work, without the relevant sioworkers PR being merged, and the Docker images published.

@LiquidPL LiquidPL marked this pull request as draft January 16, 2023 14:52
@LiquidPL LiquidPL mentioned this pull request Mar 17, 2023
@LiquidPL
Copy link
Contributor Author

Apologies for not finishing up this pull request, I will get to it very soon. However, there's one problem, mainly that the sioworkers docker image seems to be not public, ie. the packages section shows one available package, but after clicking on it, the resulting search shows no results:

image

image

I'm guessing it's just a matter of switching the package to be publicly available.

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.

Provide officially distributed docker images for SIO2
3 participants