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

Ubuntu 24.04 LTS (noble) and Ubuntu 22.04 LTS (jammy) #2021

Closed
wants to merge 17 commits into from

Conversation

woju
Copy link
Member

@woju woju commented Oct 9, 2024

Description of the changes

This PR replaces #1904, which unfortunately got too many commits and too many revisions to be revievable-able.

How to test this PR?

CI

Special GitHub/CI considerations

After merging this PR, 20.04 pipelines need to be unmarked as required in GitHub, instead the new pipelines for 24.04 and 22.04 need to be set as required.

Fixes: #1893
Closes: #1904


This change is Reviewable

@woju woju added the jenkins-debugging triggers additional pipelines on Jenkins label Oct 9, 2024
@woju
Copy link
Member Author

woju commented Oct 9, 2024

Jenkins, test this please

1 similar comment
@woju
Copy link
Member Author

woju commented Oct 9, 2024

Jenkins, test this please

Copy link

@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 8 of 8 files at r1, 3 of 3 files at r2, 3 of 3 files at r3, 5 of 5 files at r4, 32 of 32 files at r5.
Reviewable status: 47 of 79 files reviewed, 5 unresolved discussions, not enough approvals from maintainers (2 more required), not enough approvals from different teams (1 more required, approved so far: ITL) (waiting on @woju)


-- commits line 5 at r1:
Removal of Ubuntu 20.04 and the removal of EPID support are two orthogonal things, so this commit message reads weird. But I won't block, just keeping my comment for history.


-- commits line 22 at r3:
Interestingly, we haven't used Fixes: tag in Gramine up until now.


-- commits line 27 at r4:
I would expect some explanation, why we do it and why we do it only now.

From what I recall, this is because of #1909 (comment). Could we add a comment, or is it too late?

Also, wouldn't this fix #1909? If yes, please add the PR description.


-- commits line 43 at r5:
I would prefer to not mention my name (who cares) and instead make it a streamlined text. We can fix it during final rebase.


-- commits line 48 at r6:
Blocking myself: stopped on rev6 ("Rework benchmark-http.sh" commit), must continue from here tomorrow.

Copy link

@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 3 of 3 files at r6, 4 of 4 files at r7, 3 of 3 files at r8, 1 of 1 files at r9, 1 of 1 files at r10, 1 of 1 files at r11, 1 of 1 files at r12, 2 of 2 files at r13, 2 of 2 files at r14, 1 of 1 files at r15, 2 of 2 files at r16, 14 of 14 files at r17, all commit messages.
Reviewable status: all files reviewed, 11 unresolved discussions, not enough approvals from maintainers (2 more required), not enough approvals from different teams (1 more required, approved so far: ITL) (waiting on @woju)


-- commits line 78 at r9:
Do we know why we needed this? Could we add a quick explanation in commit message body? But not blocking.


-- commits line 100 at r12:
Would be good to add a quick comment why it got broken. IIRC, it was something about difference in Clang versions (we run ASan only on Gramine built with Clang), which refused to print libos_call.c (i.e. proper source file name) for some reason.


-- commits line 112 at r14:
I would remove I guess is fine, it. So the sentence will become simply which means that ...


-- commits line 119 at r15:
Is this just to ease CI debugging? Could you add a short explanation in the commit body message?


-- commits line 123 at r17:
Could you explain again why this is needed?


.ci/ubuntu22.04.dockerfile line 56 at r8 (raw file):

# libomp-dev: needed for libos/test/regression/openmp.c
# libevent-dev: CI-Examples/memcached
# libmemcached-tools: CI-Examples/memcache

memcached (typo)


.ci/ubuntu24.04.dockerfile line 57 at r8 (raw file):

# libomp-dev: needed for libos/test/regression/openmp.c
# libevent-dev: CI-Examples/memcached
# libmemcached-tools: CI-Examples/memcache

ditto

@woju woju force-pushed the woju/ubuntu-noble-3 branch from bcbb172 to 38e6579 Compare October 11, 2024 10:40
Copy link
Member Author

@woju woju 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, 11 unresolved discussions, not enough approvals from maintainers (2 more required), not enough approvals from different teams (1 more required, approved so far: ITL) (waiting on @dimakuv)


-- commits line 5 at r1:

Previously, dimakuv (Dmitrii Kuvaiskii) wrote…

Removal of Ubuntu 20.04 and the removal of EPID support are two orthogonal things, so this commit message reads weird. But I won't block, just keeping my comment for history.

I've rewritten the commit message.


-- commits line 22 at r3:

Previously, dimakuv (Dmitrii Kuvaiskii) wrote…

Interestingly, we haven't used Fixes: tag in Gramine up until now.

It's kernel's convention: https://kernel.org/doc/html/latest/process/submitting-patches.html#using-reported-by-tested-by-reviewed-by-suggested-by-and-fixes


-- commits line 27 at r4:

Previously, dimakuv (Dmitrii Kuvaiskii) wrote…

I would expect some explanation, why we do it and why we do it only now.

From what I recall, this is because of #1909 (comment). Could we add a comment, or is it too late?

Also, wouldn't this fix #1909? If yes, please add the PR description.

Done. Added commit message.


-- commits line 43 at r5:

Previously, dimakuv (Dmitrii Kuvaiskii) wrote…

I would prefer to not mention my name (who cares) and instead make it a streamlined text. We can fix it during final rebase.

Well, that was your explanation, so normally I shouldn't appropriate your work, but if you don't want to be credited, I can remove the mention.

Pray people won't quote your full emails in commit messages: https://gitlab.com/libvirt/libvirt-python/-/commit/8c800b0adf4849aeb94cb712192dba82fa4720d3


-- commits line 78 at r9:

Previously, dimakuv (Dmitrii Kuvaiskii) wrote…

Do we know why we needed this? Could we add a quick explanation in commit message body? But not blocking.

I'm not sure, it just timed out several times.


-- commits line 100 at r12:

Previously, dimakuv (Dmitrii Kuvaiskii) wrote…

Would be good to add a quick comment why it got broken. IIRC, it was something about difference in Clang versions (we run ASan only on Gramine built with Clang), which refused to print libos_call.c (i.e. proper source file name) for some reason.

Added the comment in the file itself, plz see below.


-- commits line 112 at r14:

Previously, dimakuv (Dmitrii Kuvaiskii) wrote…

I would remove I guess is fine, it. So the sentence will become simply which means that ...

Done.


-- commits line 119 at r15:

Previously, dimakuv (Dmitrii Kuvaiskii) wrote…

Is this just to ease CI debugging? Could you add a short explanation in the commit body message?

Done.


-- commits line 123 at r17:

Previously, dimakuv (Dmitrii Kuvaiskii) wrote…

Could you explain again why this is needed?

To make it so that full stack runs on this particular distro (both the VM and the docker).


.ci/ubuntu22.04.dockerfile line 56 at r8 (raw file):

Previously, dimakuv (Dmitrii Kuvaiskii) wrote…

memcached (typo)

Done.


.ci/ubuntu24.04.dockerfile line 57 at r8 (raw file):

Previously, dimakuv (Dmitrii Kuvaiskii) wrote…

ditto

Done.

Copy link

@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 71 of 71 files at r18, 3 of 3 files at r19, 2 of 3 files at r20, 2 of 5 files at r21, 32 of 32 files at r22, 2 of 3 files at r23, 4 of 4 files at r24, 3 of 3 files at r25, 1 of 1 files at r27, 1 of 1 files at r28, 2 of 2 files at r30, 2 of 2 files at r31, 1 of 1 files at r32, 2 of 2 files at r33, 14 of 14 files at r34, all commit messages.
Reviewable status: all files reviewed, all discussions resolved, not enough approvals from maintainers (1 more required)


-- commits line 5 at r1:

Previously, woju (Wojtek Porczyk) wrote…

I've rewritten the commit message.

Thanks, exactly what I expected


-- commits line 22 at r3:

Previously, woju (Wojtek Porczyk) wrote…

It's kernel's convention: https://kernel.org/doc/html/latest/process/submitting-patches.html#using-reported-by-tested-by-reviewed-by-suggested-by-and-fixes

I know, just wondered that it would be good to introduce this convention to Gramine too.


-- commits line 43 at r5:

Previously, woju (Wojtek Porczyk) wrote…

Well, that was your explanation, so normally I shouldn't appropriate your work, but if you don't want to be credited, I can remove the mention.

Pray people won't quote your full emails in commit messages: https://gitlab.com/libvirt/libvirt-python/-/commit/8c800b0adf4849aeb94cb712192dba82fa4720d3

Ok, whatever, I'll unblock here.

@woju
Copy link
Member Author

woju commented Oct 11, 2024

Jenkins, test Jenkins-Direct-22.04-Sanitizers please

dimakuv
dimakuv previously approved these changes Oct 12, 2024
Copy link

@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, all discussions resolved, not enough approvals from maintainers (1 more required)

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 8 of 8 files at r1, 71 of 71 files at r18.
Reviewable status: all files reviewed, 3 unresolved discussions, not enough approvals from maintainers (1 more required) (waiting on @woju)


.ci/linux-direct-sanitizers.jenkinsfile line 1 at r18 (raw file):

linux-sgx-ubuntu20.04-gcc-release.jenkinsfile

Why do you keep this file, while the commit message says it's dropping 20.04?


.ci/linux-direct-ubuntu20.04-gcc-debug.jenkinsfile line 1 at r18 (raw file):

linux-sgx-ubuntu20.04-gcc-release.jenkinsfile

ditto - Why do you keep this file, while the commit message says it's dropping 20.04?


.ci/linux-direct-ubuntu20.04-gcc-release.jenkinsfile line 1 at r18 (raw file):

linux-sgx-ubuntu20.04-gcc-release.jenkinsfile

ditto - and basically, ditto for this whole commit

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 3 of 3 files at r19, 3 of 3 files at r20, 5 of 5 files at r21, 32 of 32 files at r22, 3 of 3 files at r23, 4 of 4 files at r24, 3 of 3 files at r25, 1 of 1 files at r26, 1 of 1 files at r27, 1 of 1 files at r28, 1 of 1 files at r29, 2 of 2 files at r30, 2 of 2 files at r31, 1 of 1 files at r32, 2 of 2 files at r33, 14 of 14 files at r34, all commit messages.
Reviewable status: all files reviewed, 8 unresolved discussions, not enough approvals from maintainers (1 more required) (waiting on @woju)


-- commits line 98 at r27:
Please quote it here, we want the git repo to be standalone and not rely on GitHub existence.


.ci/ubuntu22.04.dockerfile line 76 at r23 (raw file):

    && cd wrk2 \
    && git checkout 44a94c17d8e6a0bac8559b53da76848e430cb7a7 \
    && make \

Let's save some CI time (or maybe even use nproc?)

Suggestion:

make -j8 \

CI-Examples/ra-tls-nginx/nginx.manifest.template line 50 at r22 (raw file):

sys.enable_sigterm_injection = true

sgx.max_threads = {{ 1 if env.get("EDMM", "0") | int > 0 else 8 }}

Why writing this differently than in all other files? (sgx.max_threads = {{ '1' if env.get('EDMM', '0') == '1' else '8' }})


libos/test/regression/device_ioctl_fail.manifest.template line 13 at r22 (raw file):

]

sgx.max_threads = {{ '1' if env.get('EDMM', '0') == '1' else '8' }}

Why is this change needed in all the tests? It seems unrelated to the PR.


libos/test/regression/test_libos.py line 1466 at r30 (raw file):

    # There's a bug in gdb introduced somewhere between versions 12 and 13 (and
    # still present in 15.x at the time of this writing): When using set
    # detach-on-fork off and set schedule-multiple on (which our gramine.gdb

Please backtick both commands, otherwise it's not obvious which part is the English sentence and which are the commands (especially when commands include proper English words like "on").

woju added 17 commits October 14, 2024 15:27
This coincides with IAS EOL, which happened on 1 October 2024. Our CI
runners for Focal used to use EPID developer key, and those were
disabled. As of this writing, IAS anwers 401 to our key.

See also:
https://www.intel.com/content/www/us/en/developer/articles/technical/software-security-guidance/resources/sgx-ias-using-epid-eol-timeline.html

Moving those runners to DCAP environment would require some work for
very little value. Focal is the third newest LTS at the time of this
writing, so not supported anyway according to our policy. Therefore we
just disable Ubuntu 20.04 LTS altogether.

Signed-off-by: Wojtek Porczyk <[email protected]>
This is a leftover from changing the default driver to upstream.

Fixes: 12e5d9a ("[CI] Change default SGX driver to upstream")
Signed-off-by: Wojtek Porczyk <[email protected]>
Busybox we were using had problem compiling on newer kernels for
undeclared `TCA_CBQ_MAX`. Switch to system busybox, as the one provided
by distribution is already compiled, and all we are exercising is
`busybox echo "Hello, world!"`, so compatibility between busybox
versions is not really a concern.

Fixes: #1909
Signed-off-by: Wojtek Porczyk <[email protected]>
Dmitrii Kuvaiskii explained it this way:

    3 threads is nothing, given that:
    - There is one main app thread
    - There is one IPC helper thread
    - There is one Async helper thread
    - When a pipe is created, another helper thread is created for a brief moment

    [...] technically 4 is a tight bound. There can't be more than that
    (at least in the current state of Gramine, until we add more helper
    threads).

Signed-off-by: Wojtek Porczyk <[email protected]>
- Make wrk2 dependency optional. The wrk2 tool adds -R option to wrk
  tool, however, wrk2 is not packaged for Debian/Ubuntu, but wrk is. If
  wrk2 is not available, then we can just use vanilla wrk tool.

- Convert bc arithmetic to python3 -c. This removes bc dependency in
  favour of python3, which is always available, because it's
  a dependency of both Gramine tooling and build system.

  (POSIX shell arithmetic substitution does not support decimal, so it's
  not suitable).

Signed-off-by: Wojtek Porczyk <[email protected]>
And add bookworm (Debian 12) and noble (Ubuntu 24.04 LTS) to
check-python-platlib.

Signed-off-by: Wojtek Porczyk <[email protected]>
Based on this suggestion by Dmitrii Kuvaiskii:

> @fwoodruff This is a known issue in some versions of `libomp` (the
> LLVM implementation of OpenMP), and I think also in `libiomp` (Intel
> implementation of OpenMP).
>
> You can read about this bug here:
>
>     * [llvm/llvm-project@dafebd5](llvm/llvm-project@dafebd5)
>     * [llvm/llvm-project@102d864](llvm/llvm-project@102d864)
>
> IIUC, it was completely fixed in LLVM OpenMP v18.1.0. I don't know
> whether and how it was fixed in `libiomp`.
>
> I would dissuade you from trying @kailun-qin workaround of enabling
> `untrusted_shm`. Instead, you can do this simple workaround:
>
> ```
> fs.mounts = [
>     ...,
>     { type = "tmpfs", path = "/dev/shm" },
> ]
> ```
>
> This way you are guaranteed that whatever files the OpenMP library
> creates in `/dev/shm/`, these files will **not** be visible on the
> host. So security of your application remains intact.

(#1780 (comment))

Signed-off-by: Wojtek Porczyk <[email protected]>
- epoll01:          300 s -> skip
- pipe:              30 s -> 60 s
- clock_gettime04: disable test 1

Signed-off-by: Wojtek Porczyk <[email protected]>
For some reason memcached-tool sometimes outputs more than 2 lines,
which still means that there is data in the server, so it's working.

Signed-off-by: Wojtek Porczyk <[email protected]>
Piping straight to OUTPUT hid the error from the log, which made
diagnostics unwieldy in our CI.

Signed-off-by: Wojtek Porczyk <[email protected]>
Fixes: 03a42b0 ("[CI] Add pipeline for noble (Ubuntu 24.04 LTS)")
Fixes: 9d53dde ("[CI] Add pipeline for jammy (Ubuntu 22.04 LTS)")
Signed-off-by: Wojtek Porczyk <[email protected]>
@woju woju force-pushed the woju/ubuntu-noble-3 branch from 38e6579 to b6d29c3 Compare October 14, 2024 13:31
Copy link
Member Author

@woju woju 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 8 files at r1.
Reviewable status: 0 of 79 files reviewed, 8 unresolved discussions, not enough approvals from maintainers (1 more required), not enough approvals from different teams (1 more required, approved so far: ITL) (waiting on @dimakuv and @mkow)


-- commits line 98 at r27:

Previously, mkow (Michał Kowalczyk) wrote…

Please quote it here, we want the git repo to be standalone and not rely on GitHub existence.

Done.


.ci/linux-direct-sanitizers.jenkinsfile line 1 at r18 (raw file):

Previously, mkow (Michał Kowalczyk) wrote…

Why do you keep this file, while the commit message says it's dropping 20.04?

Fixed per private channel. This will result in required GH statuses failing , to be disabled before merging this PR.


.ci/linux-direct-ubuntu20.04-gcc-debug.jenkinsfile line 1 at r18 (raw file):

Previously, mkow (Michał Kowalczyk) wrote…

ditto - Why do you keep this file, while the commit message says it's dropping 20.04?

Done.


.ci/linux-direct-ubuntu20.04-gcc-release.jenkinsfile line 1 at r18 (raw file):

Previously, mkow (Michał Kowalczyk) wrote…

ditto - and basically, ditto for this whole commit

Done.


.ci/ubuntu22.04.dockerfile line 76 at r23 (raw file):

Previously, mkow (Michał Kowalczyk) wrote…

Let's save some CI time (or maybe even use nproc?)

This is docker build step, it usually uses 0 time, because it is cached:

13:10:26  Step 13/16 : RUN git clone https://github.com/giltene/wrk2.git     && cd wrk2     && git checkout 44a94c17d8e6a0bac8559b53da76848e430cb7a7     && make     && cp wrk /usr/local/bin     && cd ..     && rm -rf wrk2
13:10:26   ---> Using cache
13:10:26   ---> e1b36cd7d214

I don't want to check if their Makefile survives -j8.


CI-Examples/ra-tls-nginx/nginx.manifest.template line 50 at r22 (raw file):

Previously, mkow (Michał Kowalczyk) wrote…

Why writing this differently than in all other files? (sgx.max_threads = {{ '1' if env.get('EDMM', '0') == '1' else '8' }})

Because this file apparently uses different convention (see 3 lines above).


libos/test/regression/device_ioctl_fail.manifest.template line 13 at r22 (raw file):

Previously, mkow (Michał Kowalczyk) wrote…

Why is this change needed in all the tests? It seems unrelated to the PR.

I'm not sure, maybe there's some difference between oot and upstream/dcap driver and/or there is some race between internal threads that is usually won on bare metal and more likely to be lost on virtual machines, which have more jittery scheduling? Note this PR not only changes the distros but also driver and the setup of CI nodes.


libos/test/regression/test_libos.py line 1466 at r30 (raw file):

Previously, mkow (Michał Kowalczyk) wrote…

Please backtick both commands, otherwise it's not obvious which part is the English sentence and which are the commands (especially when commands include proper English words like "on").

Done.

@woju
Copy link
Member Author

woju commented Oct 16, 2024

Superseded by #2026 to #2031.

@woju woju closed this Oct 16, 2024
@woju woju deleted the woju/ubuntu-noble-3 branch October 16, 2024 12:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
jenkins-debugging triggers additional pipelines on Jenkins
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[CI] Don't forget to move all tests to Ubuntu 22.04 and 24.04 when removing 20.04 support
3 participants