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 test scripts #1914

Open
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

oshogbo
Copy link
Contributor

@oshogbo oshogbo commented Jun 18, 2024

Description of the changes

This pull request is related to #1913 and it adds the scripts for automatically testing Docker images before releasing them to Docker Hub.

How to test this PR?

GRAMINE_URL=https://github.com/oshogbo/gramine-wips.git ./test.sh ubuntu20

This change is Reviewable

Signed-off-by: Mariusz Zaborski <[email protected]>
Copy link
Member

@mkow mkow left a comment

Choose a reason for hiding this comment

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

Reviewed all commit messages.
Reviewable status: 0 of 2 files reviewed, 2 unresolved discussions, not enough approvals from maintainers (3 more required), not enough approvals from different teams (2 more required, approved so far: ) (waiting on @oshogbo)


packaging/docker/Dockerfile.test line 25 at r1 (raw file):

    meson setup \
        build/ \
	--prefix=/usr \

tabs -> spaces
plz fix your editor config ;)


packaging/docker/test.sh line 19 at r1 (raw file):

        codename="focal"
        ;;
    ubuntu22)

no 24.04?

Copy link
Contributor Author

@oshogbo oshogbo left a comment

Choose a reason for hiding this comment

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

Reviewable status: 0 of 2 files reviewed, 2 unresolved discussions, not enough approvals from maintainers (3 more required), not enough approvals from different teams (2 more required, approved so far: ) (waiting on @mkow)


packaging/docker/Dockerfile.test line 25 at r1 (raw file):

Previously, mkow (Michał Kowalczyk) wrote…

tabs -> spaces
plz fix your editor config ;)

Half of it I developed on sdp12 and moved between machines to commit. On that machine, I don't have the right config.
NVM.
Sorry about that ;/


packaging/docker/test.sh line 19 at r1 (raw file):

Previously, mkow (Michał Kowalczyk) wrote…

no 24.04?

These docker images are built based on the official Gramine packages (they do apt install from the official server).
As we don't have ubuntu24 images (AFIK) there is no way to test it.
When the new version is released (v1.8) I will add the support for ubuntu24 here and in the Docker build script.

Copy link
Contributor

@dimakuv dimakuv left a comment

Choose a reason for hiding this comment

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

Reviewed 1 of 2 files at r1, 1 of 1 files at r2, all commit messages.
Reviewable status: all files reviewed, 14 unresolved discussions, not enough approvals from maintainers (2 more required), not enough approvals from different teams (2 more required, approved so far: ), "fixup! " found in commit messages' one-liners (waiting on @mkow and @oshogbo)


-- commits line 7 at r2:
@oshogbo Could you add the Signed-off-by line for fixup commits also? I understand that it's useless, but GitHub's DCO checker fails because of this. And it's annoying to see a presumptive failure reported by GitHub...


packaging/docker/Dockerfile.test line 1 at r2 (raw file):

ARG GRAMINE_IMAGE=gramineproject/gramine:stable-focal

Is it reasonable to hard-code a particular version here? Wouldn't it be better to force the user to specify it explicitly (like it is done in the Bash script below)?


packaging/docker/Dockerfile.test line 13 at r2 (raw file):

    autoconf bison gawk git meson nasm libunwind-dev ninja-build pkg-config python3 \
    python3-click python3-jinja2 python3-pip python3-pyelftools python3-pytest \
    wget && \

This list will be annoying to modify. Can we split into one package per line (like we do in other cases, see e.g. CI dockerfiles)? Then it will be trivial to delete/add/sort.


packaging/docker/Dockerfile.test line 14 at r2 (raw file):

    python3-click python3-jinja2 python3-pip python3-pyelftools python3-pytest \
    wget && \
    python3 -m pip install 'meson>=0.56' 'tomli>=1.1.0' 'tomli-w>=0.4.0' && \

ditto (one package per line)


packaging/docker/Dockerfile.test line 25 at r2 (raw file):

    meson setup \
        build/ \
        --prefix=/usr \

Is there a specific reason why you choose the (non-default) /usr here? I mean, Ubuntu searches in both /usr and /usr/local (default in meson), so why not just use the default?


packaging/docker/Dockerfile.test line 27 at r2 (raw file):

        --prefix=/usr \
        -Ddirect=disabled \
        -Dsgx=disabled \

Doesn't it mean that this PR is blocked on #1913? Without that prerequisite PR, this meson compile will fail, isn't it?


packaging/docker/test.sh line 12 at r2 (raw file):

fi

image=""

Useless variable?


packaging/docker/test.sh line 38 at r2 (raw file):

    -t "${tag}" \
    -f Dockerfile.test \
    . || exit 1

Why this exit 1? Why not set -e?


packaging/docker/test.sh line 42 at r2 (raw file):

docker run \
    --rm \
    -ti \

I guess -t is needed to show PyTest output on the terminal, but do you really need -i? There's nothing interactive here.


packaging/docker/test.sh line 44 at r2 (raw file):

    -ti \
    --device /dev/sgx_enclave \
    --volume /var/run/aesmd/aesm.socket:/var/run/aesmd/aesm.socket \

You're testing gramine-direct (not SGX), so you don't need these two lines


packaging/docker/test.sh line 46 at r2 (raw file):

    --volume /var/run/aesmd/aesm.socket:/var/run/aesmd/aesm.socket \
    --cap-add SYS_ADMIN \
    --cap-add SYS_PTRACE \

These two capabilities should be explained in a comment. I guess ADMIN and PTRACE are for GDB tests?

Hm, but you don't specify these capabilities in the next docker-run (and that one also uses GDB tests), so ADMIN and PTRACE are required for something else...


packaging/docker/test.sh line 53 at r2 (raw file):

docker run \
    --rm \
    -ti \

ditto

Copy link
Contributor

@dimakuv dimakuv left a comment

Choose a reason for hiding this comment

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

Reviewable status: all files reviewed, 14 unresolved discussions, not enough approvals from maintainers (2 more required), not enough approvals from different teams (2 more required, approved so far: ), "fixup! " found in commit messages' one-liners (waiting on @mkow and @oshogbo)


packaging/docker/Dockerfile.test line 25 at r2 (raw file):
I think I found the reason, explained by @oshogbo himself:

If your --prefix differs from the actual installed gramine, then gramine-test won't be able to find libpal.

Taken from top-level discussion in #1913.

Signed-off-by: Mariusz Zaborski <[email protected]>
Copy link
Contributor Author

@oshogbo oshogbo left a comment

Choose a reason for hiding this comment

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

Reviewable status: 0 of 2 files reviewed, 14 unresolved discussions, not enough approvals from maintainers (2 more required), not enough approvals from different teams (2 more required, approved so far: ), "fixup! " found in commit messages' one-liners (waiting on @dimakuv and @mkow)


-- commits line 7 at r2:

Previously, dimakuv (Dmitrii Kuvaiskii) wrote…

@oshogbo Could you add the Signed-off-by line for fixup commits also? I understand that it's useless, but GitHub's DCO checker fails because of this. And it's annoying to see a presumptive failure reported by GitHub...

Sure. I won't force-push it. I guess it was a comment for the feature.


packaging/docker/Dockerfile.test line 1 at r2 (raw file):

Previously, dimakuv (Dmitrii Kuvaiskii) wrote…

Is it reasonable to hard-code a particular version here? Wouldn't it be better to force the user to specify it explicitly (like it is done in the Bash script below)?

These Docker files are not really meant to be used manually I guess.
It also kinda documents what you are expecting, but I guess we can put a comment.
I have used the same scheme as in Dockerfile, so I don't really care if it's defined on not, I would just like to have the same in both files.


packaging/docker/Dockerfile.test line 13 at r2 (raw file):

Previously, dimakuv (Dmitrii Kuvaiskii) wrote…

This list will be annoying to modify. Can we split into one package per line (like we do in other cases, see e.g. CI dockerfiles)? Then it will be trivial to delete/add/sort.

Done.


packaging/docker/Dockerfile.test line 14 at r2 (raw file):

Previously, dimakuv (Dmitrii Kuvaiskii) wrote…

ditto (one package per line)

Done.


packaging/docker/Dockerfile.test line 25 at r2 (raw file):

Previously, dimakuv (Dmitrii Kuvaiskii) wrote…

I think I found the reason, explained by @oshogbo himself:

If your --prefix differs from the actual installed gramine, then gramine-test won't be able to find libpal.

Taken from top-level discussion in #1913.

Yes exactly.


packaging/docker/Dockerfile.test line 27 at r2 (raw file):

Previously, dimakuv (Dmitrii Kuvaiskii) wrote…

Doesn't it mean that this PR is blocked on #1913? Without that prerequisite PR, this meson compile will fail, isn't it?

Yes, it will.
Althouth, I have mentioned an alternative way of testing this PR:

GRAMINE_URL=https://github.com/oshogbo/gramine-wips.git ./test.sh ubuntu20

But for an upstream, yes it will fail.


packaging/docker/test.sh line 12 at r2 (raw file):

Previously, dimakuv (Dmitrii Kuvaiskii) wrote…

Useless variable?

Done.


packaging/docker/test.sh line 38 at r2 (raw file):

Previously, dimakuv (Dmitrii Kuvaiskii) wrote…

Why this exit 1? Why not set -e?

My idea was that you want to get the status of all test sgx and direct in one run.
Otherwise, you will end on the first run.


packaging/docker/test.sh line 42 at r2 (raw file):

Previously, dimakuv (Dmitrii Kuvaiskii) wrote…

I guess -t is needed to show PyTest output on the terminal, but do you really need -i? There's nothing interactive here.

Done.


packaging/docker/test.sh line 44 at r2 (raw file):

Previously, dimakuv (Dmitrii Kuvaiskii) wrote…

You're testing gramine-direct (not SGX), so you don't need these two lines

Done.


packaging/docker/test.sh line 46 at r2 (raw file):

Previously, dimakuv (Dmitrii Kuvaiskii) wrote…

These two capabilities should be explained in a comment. I guess ADMIN and PTRACE are for GDB tests?

Hm, but you don't specify these capabilities in the next docker-run (and that one also uses GDB tests), so ADMIN and PTRACE are required for something else...

Good catch, it seems that non of them are required any longer.


packaging/docker/test.sh line 53 at r2 (raw file):

Previously, dimakuv (Dmitrii Kuvaiskii) wrote…

ditto

Done.

Signed-off-by: Mariusz Zaborski <[email protected]>
Signed-off-by: Mariusz Zaborski <[email protected]>
Copy link
Contributor

@dimakuv dimakuv left a comment

Choose a reason for hiding this comment

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

Reviewed 2 of 2 files at r3, all commit messages.
Reviewable status: all files reviewed, 6 unresolved discussions, not enough approvals from maintainers (2 more required), not enough approvals from different teams (2 more required, approved so far: ), "fixup! " found in commit messages' one-liners (waiting on @mkow and @oshogbo)


-- commits line 7 at r2:

Previously, oshogbo (Mariusz Zaborski) wrote…

Sure. I won't force-push it. I guess it was a comment for the feature.

Yes, it's a comment for future.


packaging/docker/Dockerfile.test line 1 at r2 (raw file):

Previously, oshogbo (Mariusz Zaborski) wrote…

These Docker files are not really meant to be used manually I guess.
It also kinda documents what you are expecting, but I guess we can put a comment.
I have used the same scheme as in Dockerfile, so I don't really care if it's defined on not, I would just like to have the same in both files.

Ah, I see now. @oshogbo talks about this file: https://github.com/gramineproject/gramine/blob/master/packaging/docker/Dockerfile

Ok, resolving.


packaging/docker/Dockerfile.test line 25 at r2 (raw file):

Previously, oshogbo (Mariusz Zaborski) wrote…

Yes exactly.

Please add a comment before this RUN command about this


packaging/docker/Dockerfile.test line 27 at r2 (raw file):

Previously, oshogbo (Mariusz Zaborski) wrote…

Yes, it will.
Althouth, I have mentioned an alternative way of testing this PR:

GRAMINE_URL=https://github.com/oshogbo/gramine-wips.git ./test.sh ubuntu20

But for an upstream, yes it will fail.

I'll keep this blocking comment then, until #1913 is merged


packaging/docker/test.sh line 38 at r2 (raw file):

Previously, oshogbo (Mariusz Zaborski) wrote…

My idea was that you want to get the status of all test sgx and direct in one run.
Otherwise, you will end on the first run.

Good point, but can you add it as a top-level comment? Explaining that we don't want to set -e, because for actual runs of gramine-direct and gramine-sgx tests, we want to see them both (even if one of them fails).


packaging/docker/test.sh line 3 at r3 (raw file):

#!/usr/bin/env bash

Accidental empty line?

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.

3 participants