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

Shift a couple of stages from Jenkinsfile into Dockerfile #364

Merged
merged 8 commits into from
Dec 4, 2024

Conversation

james-smith-za
Copy link
Contributor

The Jenkins job on our new deployment is running Docker containers with the option -u 1000:1000 instead of -u 0:0 which results in permissions issues when doing administrative things such as using apt-get to install stuff.

So this commit moves the install stages of the test run into a Dockerfile, so that the image which is all nicely configured can then be used to run the tests.

Relates to NGC-1447.

@james-smith-za james-smith-za requested a review from bmerry December 2, 2024 14:00
@james-smith-za james-smith-za marked this pull request as draft December 2, 2024 14:05
@james-smith-za james-smith-za force-pushed the ngc-1447-jenkins-dockerfile branch 4 times, most recently from 3bbead2 to 0736d98 Compare December 2, 2024 14:18
@bmerry bmerry removed their request for review December 2, 2024 14:19
@james-smith-za james-smith-za force-pushed the ngc-1447-jenkins-dockerfile branch from 0736d98 to 4041bfb Compare December 2, 2024 14:27
The Jenkins job on our new deployment is running Docker containers with
the option `-u 1000:1000` instead of `-u 0:0` which results in
permissions issues when doing administrative things such as using
`apt-get` to install stuff.

So this commit moves the install stages of the test run into a
Dockerfile, so that the image which is all nicely configured can then be
used to run the tests.

Relates to NGC-1447.
@james-smith-za james-smith-za force-pushed the ngc-1447-jenkins-dockerfile branch from 4041bfb to 890d049 Compare December 2, 2024 14:34
@james-smith-za
Copy link
Contributor Author

This job is working on the Jenkins test instance barring the fact that the agent doesn't have multicast loopback (which I haven't configured yet on the test agent), so I guess it's a good sign?

@james-smith-za james-smith-za marked this pull request as ready for review December 2, 2024 14:35
@james-smith-za james-smith-za requested a review from bmerry December 2, 2024 14:35
@james-smith-za
Copy link
Contributor Author

The test passes on old Jenkins though so that's helpful.

@bmerry
Copy link
Contributor

bmerry commented Dec 3, 2024

The linting checks are failing on Github Actions.

Dockerfile.CI Outdated
Comment on lines 4 to 5
# Copyright (c) 2024, National Research Foundation (SARAO)
#
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
# Copyright (c) 2024, National Research Foundation (SARAO)
#
# Copyright 2024 National Research Foundation (SARAO)

For consistency with other files.

Dockerfile.CI Outdated
Comment on lines 28 to 40
RUN apt-get update && apt-get -y --no-install-recommends install libpython3-dev python3-venv jq iproute2
RUN python3 -m venv /venv
RUN .ci/install-sys-pkgs.sh
RUN PATH="/venv/bin:$PATH" .ci/py-requirements.sh

# Install Python package
RUN PATH="/venv/bin:$PATH" pip install -v \
--config-settings=setup-args=--native-file=ci.ini \
--config-settings=setup-args=-Dibv=enabled \
--config-settings=setup-args=-Dibv_hw_rate_limit=enabled \
--config-settings=setup-args=-Dmlx5dv=enabled \
.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think I'd prefer to just use the Dockerfile for the stuff that needs to be done as root. I feel like one gets better debuggability if the steps that work on the code under test are done in Jenkinsfile stages so that you can get a clear stage view of what happened, how long each stage took etc, rather than just "the Dockerfile failed to build".

Suggested change
RUN apt-get update && apt-get -y --no-install-recommends install libpython3-dev python3-venv jq iproute2
RUN python3 -m venv /venv
RUN .ci/install-sys-pkgs.sh
RUN PATH="/venv/bin:$PATH" .ci/py-requirements.sh
# Install Python package
RUN PATH="/venv/bin:$PATH" pip install -v \
--config-settings=setup-args=--native-file=ci.ini \
--config-settings=setup-args=-Dibv=enabled \
--config-settings=setup-args=-Dibv_hw_rate_limit=enabled \
--config-settings=setup-args=-Dmlx5dv=enabled \
.
RUN apt-get update && apt-get -y --no-install-recommends install libpython3-dev python3-venv jq iproute2
RUN .ci/install-sys-pkgs.sh

Also change COPY . . to just copy install-sys-pkgs.sh. At present any code change will invalidate the entire Dockerfile, so you're not going to get any caching.

Jenkinsfile Outdated
label 'spead2'
filename 'Dockerfile.CI'
Copy link
Contributor

Choose a reason for hiding this comment

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

If you take my suggestions above, the Dockerfile only needs the .ci directory, so it can move to .ci/Dockerfile and change this to

Suggested change
filename 'Dockerfile.CI'
dir '.ci'

Bruce makes the argument that the Dockerfile should only contain things
that need to be done as root.

I don't have a strong opinion on the topic so I am accommodating this.
Didn't need the directory because it's in the same folder as the file
it's copying.
The second one needed to be corrected too.
@james-smith-za james-smith-za force-pushed the ngc-1447-jenkins-dockerfile branch from e9d3190 to f97db81 Compare December 3, 2024 20:09
Making a venv at /venv requires root permissions as well, as it turns
out. Using it will be as well so we chown it.
@james-smith-za james-smith-za force-pushed the ngc-1447-jenkins-dockerfile branch from 1965cb0 to 2bf3f2e Compare December 3, 2024 20:18
@james-smith-za
Copy link
Contributor Author

Fair points all round, and I have addressed your feedback as far as I am able. I am now running into issues with files not being found in meson:

22:19:11    + meson setup /var/jenkins/workspace/ead2_ngc-1447-jenkins-dockerfile /var/jenkins/workspace/ead2_ngc-1447-jenkins-dockerfile/.mesonpy-4tmu33hl -Dbuildtype=release -Db_ndebug=if-release -Db_vscrt=md -Dtools=disabled -Dunit_test=disabled -Dcuda=disabled -Dgdrapi=disabled -Dcap=disabled -Dpython=true --native-file=ci.ini -Dibv=enabled -Dibv_hw_rate_limit=enabled -Dmlx5dv=enabled --native-file=/var/jenkins/workspace/ead2_ngc-1447-jenkins-dockerfile/.mesonpy-4tmu33hl/meson-python-native-file.ini
22:19:11    Could not find any valid candidate for native files: ci.ini
22:19:11  
22:19:11    ERROR: Cannot find specified native file: /var/jenkins/workspace/ead2_ngc-1447-jenkins-dockerfile/.mesonpy-4tmu33hl/meson-python-native-file.ini
22:19:11    error: subprocess-exited-with-error

see here.

I'm not quite clear as to which file is missing though: it looks as though it's complaining both about the ci.ini file and the /var/jenkins/..../meson-python-native-file.ini. The former looks to me as though it gets explicitly created somewhere but the latter I'm not sure about.

@bmerry
Copy link
Contributor

bmerry commented Dec 4, 2024

I'll investigate.

@bmerry
Copy link
Contributor

bmerry commented Dec 4, 2024

My guess would be that it's because install-sys-pkgs.sh is creating the ci.ini in $HOME, but $HOME during the docker build probably doesn't match the $HOME during the test.

I think let me see if I can rework things to write the ci.ini in the workspace rather than somewhere in $HOME, since I'm not sure we can rely on having a $HOME in the Jenkins case (particularly if the agent doesn't use UID 1000).

bmerry and others added 2 commits December 4, 2024 12:13
- Move venv creation back to Jenkinsfile, and create it inside the
  workspace
- Remove native.ini for Linux and use it on MacOS only
- Update Jenkins install step to use setup-flags.sh.
Rework for Jenkins testing
@james-smith-za james-smith-za requested a review from bmerry December 4, 2024 11:16
Copy link
Contributor

@bmerry bmerry left a comment

Choose a reason for hiding this comment

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

Looks good, just some corrections to the copyright block. Feel free to merge after fixing those.

.ci/Dockerfile Outdated Show resolved Hide resolved
.ci/Dockerfile Outdated Show resolved Hide resolved
.ci/Dockerfile Outdated Show resolved Hide resolved
@james-smith-za james-smith-za merged commit d7907b8 into master Dec 4, 2024
22 of 23 checks passed
@james-smith-za james-smith-za deleted the ngc-1447-jenkins-dockerfile branch December 4, 2024 11:58
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.

2 participants