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

feat: Build and test with Python 3.8 and 3.11 for codejail. #167

Merged
merged 7 commits into from
Apr 29, 2024

Conversation

feanil
Copy link
Contributor

@feanil feanil commented Apr 23, 2024

This change uses deadksnakes to be able to install any
relevant python versions we might need. We also update the Dockerfile to
be able to build containers for both Python 3.8 and 3.11. Along the way
we also needed to make the following changes:

Makefile Changes:

  • Add make as an explicit package to install since we call it and
    previously it was coming in implicitly with git and sudo

  • Install pip and virtualenv differently.

  • Add build-essentials so we can compile numpy in python 3.11

Other Changes:

  • Create a new apparmor profiles that will work with multiple versions
    of Python and pulls in the latest content of python/abstractions
    in case they differ for different combinations of python/ubuntu.

  • Create multiple sudoers files and load the right one based on the
    python version.

  • Update the publish workflow to publish images for both python
    versions.

See individual commit messages for more details.

@feanil feanil requested a review from timmc-edx April 23, 2024 19:15
@feanil feanil force-pushed the feanil/build_multiple_images branch from 36e50ed to cf66b1e Compare April 23, 2024 19:48
@feanil feanil linked an issue Apr 24, 2024 that may be closed by this pull request
6 tasks
@feanil feanil mentioned this pull request Apr 24, 2024
6 tasks
@feanil feanil force-pushed the feanil/build_multiple_images branch from c8f45d9 to 5cf6d5f Compare April 25, 2024 04:06
feanil added 4 commits April 25, 2024 00:32
Before we can test on the newer versions of python we need to build the
new images.  This PR updates the Dockerfile to Ubuntu 22.04 instead of
20.04 and uses deadksnakes to be able to install any relevant python
versions we might need.

We need to update the Dockerfile to be able to build containers for both
Python 3.8 and 3.11.  Along the way we also needed to make the following
changes:

Makefile Changes:

* Add `make` as an explicit package to install since we call it and
  previously it was coming in implicitly with `git` and `sudo`

* Install pip and virtualenv differently.

* Add build-essentials so we can compile numpy in python 3.11

Other Changes:

* Create multiple apparmor profiles and load the correct one based on
  the python version.

* Create multiple sudoers files and load the right one based on the
  python version.

* Update the publish workflow to publish images for both python
  versions.
Update CI to run tests on both 3.8 and 3.11
The traceback has more details in the newer version of python including
the full filepath for the jailed_code file which includes the generated
temp path.  That path is random each time so we use a regex to deal with
it instead of hardcoding the test message.
Needed to fix Pylint errors in 3.11 but also it looked like a bunch of
other packages hadn't been updated in a while.
@feanil feanil force-pushed the feanil/build_multiple_images branch from 32970fb to d1df5cb Compare April 25, 2024 04:38
@feanil feanil requested a review from MoisesGSalas April 25, 2024 04:44
Copy link
Contributor

@MoisesGSalas MoisesGSalas left a comment

Choose a reason for hiding this comment

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

I left a few comments.

In addition: I tried running the tests manually (in a docker container, with the same commands as the workflow) and found some weird behavior.

The suite passes the first couple of times but after the third or so attempt it begins to fail. Deleting and creating the container again seems to fix it. It seems like there's a few lingering processes that accumulate between runs, but I didn't went too deep.

I guess revisiting those tests is outside the scope of this PR right?

Dockerfile Show resolved Hide resolved

profile apparmor_profile /home/sandbox/codejail_sandbox-python3.11/bin/python {
#include <abstractions/base>
#include <abstractions/python>
Copy link
Contributor

Choose a reason for hiding this comment

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

IIRC this inherited from the host, if the CI runner is on ubuntu-20.04 it might be missing some rules from python3.11.

I think deadsnakes doesn't update any of the base rules.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@MoisesGSalas good thought, looking at the 20.04 profiles, it does look like they are not going to set the rules correctly for Python 3.11

  /usr/lib{,32,64}/python{2.[4-7],3.[0-9]}/**.{pyc,so}           mr,
  /usr/lib{,32,64}/python{2.[4-7],3.[0-9]}/**.{egg,py,pth}       r,
  /usr/lib{,32,64}/python{2.[4-7],3.[0-9]}/{site,dist}-packages/ r,
  /usr/lib{,32,64}/python3.[0-9]/lib-dynload/*.so            mr,

  /usr/local/lib{,32,64}/python{2.[4-7],3,3.[0-9]}/**.{pyc,so}           mr,
  /usr/local/lib{,32,64}/python{2.[4-7],3,3.[0-9]}/**.{egg,py,pth}       r,
  /usr/local/lib{,32,64}/python{2.[4-7],3,3.[0-9]}/{site,dist}-packages/ r,
  /usr/local/lib{,32,64}/python3.[0-9]/lib-dynload/*.so            mr,

  # Site-wide configuration
  /etc/python{2.[4-7],3.[0-9]}/** r,

  # shared python paths
  /usr/share/{pyshared,pycentral,python-support}/**      r,
  /{var,usr}/lib/{pyshared,pycentral,python-support}/**  r,
  /usr/lib/{pyshared,pycentral,python-support}/**.so     mr,
  /var/lib/{pyshared,pycentral,python-support}/**.pyc    mr,
  /usr/lib/python3/dist-packages/**.so          mr,

  # wx paths
  /usr/lib/wx/python/*.pth r,

  # python build configuration and headers
  /usr/include/python{2.[4-7],3.[0-9]}*/pyconfig.h r,

Newer versions of this profile are more up-to-date and can handle older and newer versions of python, do you think it makes sense to drop the abstraction and just explicitly set the rules similar to what are in 22.04?

  /usr/{local/,}lib{,32,64}/python{2.[4-7],3,3.[0-9],3.1[0-9]}/**.{pyc,so,so.*[0-9]} mr,
  /usr/{local/,}lib{,32,64}/python{2.[4-7],3,3.[0-9],3.1[0-9]}/**.{egg,py,pth}       r,
  /usr/{local/,}lib{,32,64}/python{2.[4-7],3,3.[0-9],3.1[0-9]}/{site,dist}-packages/ r,
  /usr/{local/,}lib{,32,64}/python{2.[4-7],3,3.[0-9],3.1[0-9]}/{site,dist}-packages/**/ r,
  /usr/{local/,}lib{,32,64}/python{2.[4-7],3,3.[0-9],3.1[0-9]}/{site,dist}-packages/*.dist-info/{METADATA,namespace_packages.txt} r,
  /usr/{local/,}lib{,32,64}/python{2.[4-7],3,3.[0-9],3.1[0-9]}/{site,dist}-packages/*.VERSION r,
  /usr/{local/,}lib{,32,64}/python{2.[4-7],3,3.[0-9],3.1[0-9]}/{site,dist}-packages/*.egg-info/PKG-INFO r,
  /usr/{local/,}lib{,32,64}/python3.{1,}[0-9]/lib-dynload/*.so            mr,

  # Site-wide configuration
  /etc/python{2.[4-7],3.[0-9],3.1[0-9]}/** r,

  # shared python paths
  /usr/share/{pyshared,pycentral,python-support}/**      r,
  /{var,usr}/lib/{pyshared,pycentral,python-support}/**  r,
  /usr/lib/{pyshared,pycentral,python-support}/**.so     mr,
  /var/lib/{pyshared,pycentral,python-support}/**.pyc    mr,
  /usr/lib/python3/dist-packages/**.so          mr,

  # wx paths
  /usr/lib/wx/python/*.pth r,

  # python build configuration and headers
  /usr/include/python{2.[4-7],3.[0-9],3.1[0-9]}*/pyconfig.h r,

  # Include additions to the abstraction
  include if exists <abstractions/python.d>

Copy link
Contributor

Choose a reason for hiding this comment

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

We do something similar in the profile we use in the tutor-contrib-codejail plugin.

I'm in favor of in-lining those rules and also take the opportunity to update the runner and some of the actions that are throwing a few warnings about a deprecated node version.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@MoisesGSalas I simplified the apparmor profiles into a single profile that will work with python 3.0 -> 3.99, I continued to include the local abstraction if it exists, in case things significantly change and we miss something, we'll have that at least.

I did not put the runner updates in this PR because I don't want to conflate the two things but that can be a quick follow-up PR or you can make it and I can review/approve. Please take a look at these changes and if they look good to you, I can organize the commits and update the description before merging and releasing this code.

@feanil feanil requested a review from MoisesGSalas April 29, 2024 14:26
Copy link
Contributor

@MoisesGSalas MoisesGSalas left a comment

Choose a reason for hiding this comment

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

IMO this is good to go.

@feanil feanil changed the title feat: Build multipel test containers for codejail. feat: Build and test with Python 3.8 and 3.11 for codejail. Apr 29, 2024
@feanil feanil merged commit c499d69 into master Apr 29, 2024
4 checks passed
@feanil feanil deleted the feanil/build_multiple_images branch April 29, 2024 16:11
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.

Test codejail on Python 3.11
2 participants