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

docker: add namespaced builds for spack/ubuntu #4577

Merged
merged 1 commit into from
Sep 19, 2022

Conversation

vsoch
Copy link
Member

@vsoch vsoch commented Sep 16, 2022

We currently build one set of containers that are spack focused, and ideally we should provide this along with m ore vanilla (e.g., ubuntu) builds. This update will move the spack builds into a spack subdirectory and add two ubuntu builds - one a base container, and the other that just runs a make check-prep to run some tests. This ubuntu base could be used for other contexts
of testing, and I think after the python work is integrated we would migrate the container there into this directory too. Some notes:

  • configure AC needs to work via a build argument for a FLUX_VERSION otherwise we get a wonky build error. It's hard coded now an eventually can come from a variable (e.g. release)
  • a lot of the supporting python scripts won't work in the container build unless they are executable and have a hash bang for python (they were trying to be run with /bin/sh, and then "Permission denied"
  • I think we can keep the PR trigger as long as paths are matched (added here)

Also just noticed ubuntu bases are a lot faster than spack!

Signed-off-by: vsoch vsoch@users.noreply.github.com

@vsoch vsoch force-pushed the add/test-container branch 5 times, most recently from 6b96f39 to f23d1ab Compare September 16, 2022 06:43
@grondo
Copy link
Contributor

grondo commented Sep 16, 2022

a lot of the supporting python scripts won't work in the container build unless they are executable and have a hash bang for python (they were trying to be run with /bin/sh, and then "Permission denied"

For better or worse we've mostly done this on purpose because those scripts are meant to be run via flux python <script>.py to ensure they use the python with which Flux was configured. So we leave off the executable bit and shebang to discourage running them incorrectly.

configure AC needs to work via a build argument for a FLUX_VERSION otherwise we get a wonky build error. It's hard coded now an eventually can come from a variable (e.g. release)

This should not be required if you pull down at least the latest tag during checkout, though I'm fine with also supporting a variable.

@vsoch
Copy link
Member Author

vsoch commented Sep 16, 2022

For better or worse we've mostly done this on purpose because those scripts are meant to be run via flux python <script>.py to ensure they use the python with which Flux was configured. So we leave off the executable bit and shebang to discourage running them incorrectly.

Gotcha. I'm not sure what to do here, because removing it, the containers will definitely error.

This should not be required if you pull down at least the latest tag during checkout, though I'm fine with also supporting a variable.

Cool, thanks! The use case is being able to build from any git hash.

Let me know what changes you'd like.

@grondo
Copy link
Contributor

grondo commented Sep 16, 2022

I'm traveling today, so I won't be able to finish review til Monday. It would be interesting to know why the container errors when running python commands the intended way. Maybe there's a better way we can be doing things!

@vsoch
Copy link
Member Author

vsoch commented Sep 16, 2022

Ah ok we can chat then! The short answer is I get a permission denied during the build and it appears to be trying to use a shell interpreter.

Bon voyage! Have fun! 🧳

@grondo
Copy link
Contributor

grondo commented Sep 19, 2022

The short answer is I get a permission denied during the build and it appears to be trying to use a shell interpreter.

But what is trying to run the python scripts directly - make check runs all the test scripts under t with flux python and python scripts under src/cmd should be run via the flux(1) command driver. So, it would appear something is being run incorrectly in the container.

@vsoch
Copy link
Member Author

vsoch commented Sep 19, 2022

The container is running those exact steps too (for install with autogen -> make -> make install)!

https://github.com/flux-framework/flux-core/pull/4577/files#diff-c0d983b7ccd4520581637309574db8af2c766127b1295704c81bc3ef8b35a6e4R48-R53

So is there some other environment variable or some difference about the container (which you can see the Dockerfile there) that could lead to this?

Also - I've run into this bug running the flux build for the gitlab CI recipe with Jacamar. When I'm building in different contexts with Flux it comes up fairly regularly, so (if a user could hit this too trying to install) I don't think we should ignore it. We maybe need a different strategy all together?

@vsoch
Copy link
Member Author

vsoch commented Sep 19, 2022

I have an idea - how about I'll build a variant of the container that doesn't build, and give instruction for how to shell in and reproduce? Then you can very likely see what is going on. Or if there is something you want me to test or try I can do that too!

@grondo
Copy link
Contributor

grondo commented Sep 19, 2022

Yeah, if you have instructions to reproduce that would be great! We run the build and make check in 4-6 different containers on every PR and when we build RPMs and haven't seen this issue so I'm clearly missing something basic here! Perhaps it has something to do with the Python used.

@vsoch
Copy link
Member Author

vsoch commented Sep 19, 2022

OK! I'm munching on 🥑 but I'll do this right after dinner!

@vsoch
Copy link
Member Author

vsoch commented Sep 19, 2022

Okay done (it didn't end well for the 🥑)! So here is what I did. I started with this Dockerfile:

FROM ubuntu:22.04

# docker build -t vanessa/flux-framework:grondo .

# used in configure.ac
ARG FLUX_VERSION=0.42.0
ENV FLUX_VERSION=${FLUX_VERSION}
ENV DEBIAN_FRONTEND=noninteractive

RUN apt-get update \
 && apt-get -qq install -y --no-install-recommends \
        automake \
        libsodium-dev \
        libzmq3-dev \
        libczmq-dev \
        libjansson-dev \
        libmunge-dev \
        libncursesw5-dev \
        lua5.4 \
        liblua5.4-dev \
        liblz4-dev \
        libsqlite3-dev \
        uuid-dev \
        libhwloc-dev \
        libmpich-dev \
        libs3-dev \
        libevent-dev \
        libarchive-dev \
        python3 \
        python3-dev \
        python3-pip \
        python3-sphinx \
        libtool \
        git \
        build-essential \
        # Flux security
        libjson-glib-1.0.0 \ 
        libjson-glib-dev \ 
        libpam-dev && \
        ldconfig && \
        rm -rf /var/lib/apt/lists/*

RUN git clone https://github.com/flux-framework/flux-core /code
WORKDIR /code

# This has added support for flux version from environment (above)
COPY ./configure.ac /code/configure.ac

RUN ./autogen.sh && \
    ./configure --prefix=/usr/local --without-python # && \
#    make && \
#    make install && \
#    ldconfig

# Ensure we can find Python
# RUN ln -s /usr/bin/python3 /usr/bin/python
ENTRYPOINT ["/bin/bash"]

And this is pretty much the same as the one here. Note that I've commented out the make commands (this will be something that you can run, and I'll show that after) and I've also disabled python --without-python (this was more trying to simplify things to debug, possibly related, if this flag isn't commonly used?). To build the container I made a directory at /tmp/test for it, and also wrote the configure.ac from this PR there for the container. The reason we need that is so the version can be derived from the environment variable. Then I built (you don't need to run this):

$ docker build -t vanessa/flux-framework:grondo .

Okay so here are... 🚧 Instructions for reproducing! 🚧

Pull the container:

$ docker pull vanessa/flux-framework:grondo

Shell inside:

$ docker run -it vanessa/flux-framework:grondo

And you'll be in the root of flux at /code! Now just run make:

$ make

And after some very good compiling (insert compiler noises here) it should reproduce:

make[1]: Entering directory '/code/etc'
  GEN      completions/flux
  GEN      flux/help.d/core.json
/bin/bash: line 2: ./gen-cmdhelp.py: Permission denied
make[1]: *** [Makefile:914: flux/help.d/core.json] Error 126
make[1]: Leaving directory '/code/etc'
make: *** [Makefile:591: all-recursive] Error 1
root@26a883a58e50:/code# 

@grondo
Copy link
Contributor

grondo commented Sep 19, 2022

Thanks!

and I've also disabled python --without-python (this was more trying to simplify things to debug, possibly related, if this flag isn't commonly used?).

Ah, yeah, I bet this is the problem. I don't think you can configure and use flux-core without python. We should probably remove that option! 🤦

@vsoch
Copy link
Member Author

vsoch commented Sep 19, 2022

Ok I'll prepare a branch to test this out!

@vsoch
Copy link
Member Author

vsoch commented Sep 19, 2022

This PR now is dependent on #4584. We would want to update the Python install there, then I'll come back here and remove the --without-python flux and undo some of the hashbang changes and (hopefully) get a successful container build!

@vsoch
Copy link
Member Author

vsoch commented Sep 19, 2022

Just testing these locally and will update the branch soon!

@vsoch
Copy link
Member Author

vsoch commented Sep 19, 2022

okay updated with changes! I don't know why cmdhelp is showing up as an empty file - I just removed the hashbang locally. We will see if it's just a GitHub floop.

@grondo
Copy link
Contributor

grondo commented Sep 19, 2022

I don't know why cmdhelp is showing up as an empty file - I just removed the hashbang locally. We will see if it's just a GitHub floop.

They show as empty because the permissions have changed. Since they are run as $(PYTHON) command.py in the Makefiles, the permissions probably don't need to be 0755, though I guess it doesn't hurt.

@vsoch
Copy link
Member Author

vsoch commented Sep 19, 2022

ah do you want me to change back? What should it be? chmod... ?

@grondo
Copy link
Contributor

grondo commented Sep 19, 2022

Sure, they were previously 0644. Also, s/adding/add/ in the comment message and PR title. Otherwise, looks great!

@vsoch vsoch changed the title docker: adding namespaced builds for spack/ubuntu docker: add namespaced builds for spack/ubuntu Sep 19, 2022
@vsoch
Copy link
Member Author

vsoch commented Sep 19, 2022

ok all set! Apparently I am an old dog 🐶 that does not learn new tricks - I made this "adding" mistake before, it's like in one 👂 and out the other! I hope I get it right next time.

@grondo
Copy link
Contributor

grondo commented Sep 19, 2022

made this "adding" mistake before, it's like in one  and out the other! I hope I get it right next time.

Haha, no problem! If it helps, even Git's own documentation requires present tense, imperative mood.

@vsoch
Copy link
Member Author

vsoch commented Sep 19, 2022

okay we are green! 🍏 📗 🟢 💚

I skipped the salad nobody wants the salad lol.

Copy link
Contributor

@grondo grondo left a comment

Choose a reason for hiding this comment

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

Yeah, who wants a big bowl of plant leafs?
LGTM!

@grondo
Copy link
Contributor

grondo commented Sep 19, 2022

I'll set the mwp flag!

@vsoch
Copy link
Member Author

vsoch commented Sep 19, 2022

It said it couldn't be embarked so I clicked the suggested button to update it.

@grondo
Copy link
Contributor

grondo commented Sep 19, 2022

It said it couldn't be embarked so I clicked the suggested button to update it.

That won't work because it creates a merge commit. You'll have to force-push your branch again rebased on current master. Sorry!

@vsoch
Copy link
Member Author

vsoch commented Sep 19, 2022

Why is that option enabled in settings if it results in an undesired outcome ? 🤔

@vsoch
Copy link
Member Author

vsoch commented Sep 19, 2022

ah because update with rebase is OK?

We currently build one set of containers that are
spack focused, and ideally we should provide this
along with more vanilla (e.g., ubuntu) builds.
This update will move the spack builds into a
spack subdirectory and add two ubuntu builds -
one a base container, and the other that just
runs a make check-prep to run some tests.
This ubuntu base could be used for other contexts
of testing, and I think after the python work is
integrated we would migrate the container there
into this directory too.

Signed-off-by: vsoch <vsoch@users.noreply.github.com>
@grondo
Copy link
Contributor

grondo commented Sep 19, 2022

ah because update with rebase is OK?

Yeah, I suppose. Though I don't think there was a way to disable it until recently (it isn't enabled in settings, it is just not disabled in settings 😆 )

@codecov
Copy link

codecov bot commented Sep 19, 2022

Codecov Report

Merging #4577 (da34a89) into master (da34a89) will not change coverage.
The diff coverage is n/a.

❗ Current head da34a89 differs from pull request most recent head b06fdb9. Consider uploading reports for the commit b06fdb9 to get more accurate results

@@           Coverage Diff           @@
##           master    #4577   +/-   ##
=======================================
  Coverage   83.36%   83.36%           
=======================================
  Files         409      409           
  Lines       68548    68548           
=======================================
  Hits        57148    57148           
  Misses      11400    11400           

@grondo
Copy link
Contributor

grondo commented Sep 19, 2022

Actually I didn't see a way to disable the "Update" button on quick perusal of settings. Do you know where it is?

@vsoch
Copy link
Member Author

vsoch commented Sep 19, 2022

It's usually this one:
image

@grondo
Copy link
Contributor

grondo commented Sep 19, 2022

Oh, we don't have that configured. 🤷

@vsoch
Copy link
Member Author

vsoch commented Sep 19, 2022

That’s super weird!!

@mergify mergify bot merged commit 3a14182 into flux-framework:master Sep 19, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants