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

Test kubernetes in CI #3482

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

Test kubernetes in CI #3482

wants to merge 35 commits into from

Conversation

benclifford
Copy link
Collaborator

@benclifford benclifford commented Jun 10, 2024

This PR is a codification of what I had to do to get pytest passing for myself while trying to review other kubernetes PRs.

The bulk of the modified files are additional test markings to label which tests require a shared filesystem (between all workers and the submit side) to store working files; and which tests require file staging (as the default file staging configuration is not valid for kubernetes, due to lack of shared file system).

There is also a bugfix for default pod names being generated as invalid format (which can fixed by users by specifying an explicit prefix)

This PR can probably be merged into master in a few stages.

@benclifford
Copy link
Collaborator Author

this is failing repeatedly on this test: (at 5c55fe6)

parsl/tests/test_regression/test_226.py F
=================================== FAILURES ===================================
__________________________________ test_no_eq __________________________________
    def test_no_eq():
>       res = get_foo_x('foo').result()

(most recently with a ManagerLost... before that, I wasn't capturing enough logging to see the whole exception.

@benclifford benclifford requested a review from rjmello June 10, 2024 15:46
@benclifford benclifford changed the title [not for merge] trying out kubernetes in github actions Test kubernetes in CI Jun 10, 2024
…read_namespaced_pod(), which is functionally the same but requires fewer permissions.
Copy link
Contributor

@rjmello rjmello left a comment

Choose a reason for hiding this comment

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

This looks good . It'd also be nice to organize the k8s-related CI files in .github/kubernetes/ or something similar.

Dockerfile Show resolved Hide resolved
Comment on lines +25 to +26
RUN apt-get update && apt-get install -y python3.12 python3.12-dev
RUN apt-get update && apt-get install -y python3.12-venv
Copy link
Contributor

Choose a reason for hiding this comment

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

I suggest we make the Python version configurable.

E.g.,

Suggested change
RUN apt-get update && apt-get install -y python3.12 python3.12-dev
RUN apt-get update && apt-get install -y python3.12-venv
ARG PYTHON_VERSION="3.12"
RUN apt-get install -y python${PYTHON_VERSION} python${PYTHON_VERSION}-dev
RUN apt-get-install -y python${PYTHON_VERSION}-venv

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

this is a bit weird (something to do with how python is packaged in trixie?). Normally in debian it looks like there's a single OS-level python3 available (which changes when there's a new code-named release) and it seems unusual that trixie happens to have two. certainly debian isn't traditionally set up to expect you to be able to choose a python version from the OS.

there's a couple of things that could happen: i) always use the OS-level default python3 or ii) use something like Conda to provide a much richer Python environment. Some dependencies like the ndcctools recommends being installed using conda anyway, and so maybe that's the way to go here. I don't think there's any particular reason to want to stick with the OS-level Python, as this is "an image where Parsl works" rather than "an image that looks like a particular debian version".

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.

None yet

3 participants