-
Notifications
You must be signed in to change notification settings - Fork 193
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
) part 2
#1904
base: master
Are you sure you want to change the base?
Conversation
Jenkins, test this please |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewed 4 of 8 files at r1, 16 of 16 files at r2, 1 of 1 files at r3, 1 of 1 files at r4, 1 of 1 files at r5, 1 of 1 files at r6, 1 of 1 files at r7, 1 of 1 files at r8, 1 of 1 files at r9, 1 of 1 files at r10, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion, not enough approvals from maintainers (2 more required), not enough approvals from different teams (1 more required, approved so far: ITL) (waiting on @woju)
.ci/linux-sgx-ubuntu24.04-edmm.jenkinsfile
line 12 at r6 (raw file):
env.DOCKER_ARGS_SGX += ''' --add-host host.docker.internal:host-gateway
Is this something new? Why it is needed?
There was a problem hiding this 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, 1 unresolved discussion, not enough approvals from maintainers (2 more required), not enough approvals from different teams (1 more required, approved so far: ITL) (waiting on @dimakuv)
.ci/linux-sgx-ubuntu24.04-edmm.jenkinsfile
line 12 at r6 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
Is this something new? Why it is needed?
This is copied from 20.04 EDMM pipeline. I don't know why it's there, git blame
says it was added by Borys.
There was a problem hiding this 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, 1 unresolved discussion, not enough approvals from maintainers (2 more required), not enough approvals from different teams (1 more required, approved so far: ITL) (waiting on @woju)
.ci/linux-sgx-ubuntu24.04-edmm.jenkinsfile
line 12 at r6 (raw file):
Previously, woju (Wojtek Porczyk) wrote…
This is copied from 20.04 EDMM pipeline. I don't know why it's there,
git blame
says it was added by Borys.
So it was added in this commit: 1b1242f
I'm fairly certain that it's because that particular host (where this pipeline ran) had a PCCS daemon running on the host, and this needed this Docker-host-gateway trick. See the .ci/ubuntu20.04.dockerfile
in that commit.
I don't see that you install or enable PCCS in any way in the Ubuntu 24.04 Dockerfile: https://github.com/gramineproject/gramine/blob/woju/ubuntu-noble-2/.ci/ubuntu24.04.dockerfile. So I guess the tests that try DCAP-based SGX remote attestation fail right now?
f502926
to
176ff98
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewable status: 18 of 30 files reviewed, 3 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 @woju)
-- commits
line 2 at r21:
Could you move this out into a separate PR? It's a trivial refactoring and we could merge it right away.
-- commits
line 61 at r21:
Please squash all these commits in the final version of the PR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewable status: 18 of 30 files reviewed, 3 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, @mkow, and @woju)
Previously, mkow (Michał Kowalczyk) wrote…
Could you move this out into a separate PR? It's a trivial refactoring and we could merge it right away.
#1920, feel free to merge ad libitum, however once you merge, I'll have to rebase this PR on top of new master.
Previously, mkow (Michał Kowalczyk) wrote…
Please squash all these commits in the final version of the PR.
Will do, after I finish all the stuff here, for now it's more convenient for me to keep separate commits for now. Please keep blocking.
.ci/linux-sgx-ubuntu24.04-edmm.jenkinsfile
line 12 at r6 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
So it was added in this commit: 1b1242f
I'm fairly certain that it's because that particular host (where this pipeline ran) had a PCCS daemon running on the host, and this needed this Docker-host-gateway trick. See the
.ci/ubuntu20.04.dockerfile
in that commit.I don't see that you install or enable PCCS in any way in the Ubuntu 24.04 Dockerfile: https://github.com/gramineproject/gramine/blob/woju/ubuntu-noble-2/.ci/ubuntu24.04.dockerfile. So I guess the tests that try DCAP-based SGX remote attestation fail right now?
That's right, they fail with error 44. I need to fix that stuff before we merge.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewable status: 18 of 30 files reviewed, 4 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 @mkow and @woju)
a discussion (no related file):
This PR looks like it's still work in progress. I won't review it now then.
Previously, woju (Wojtek Porczyk) wrote…
#1920, feel free to merge ad libitum, however once you merge, I'll have to rebase this PR on top of new master.
#1920 was merged
176ff98
to
fb6d7a5
Compare
93a7f58
to
0e4d6da
Compare
Jenkins, test this please |
1 similar comment
Jenkins, test this please |
013b71e
to
b6ec216
Compare
f0a85c4
to
82a8448
Compare
6adc582
to
7f273a6
Compare
Newer Clang versions (v17 and newer) added more UBSan checks for C code that started triggering UBSan on PAL regression tests. In particular, UBSan added the check "Indirect call of a function through a function pointer of the wrong type". UBSan expects all functions which can be indirectly called to be instrumented with two magic metadata values, located right-before the function in address space. The `memory` test however does *not* add this required metadata to a dummy generated-on-the-fly function (it simply allocates some pages and copies the asm code at the beginning of the first page). Instead of adding UBSan-required metadata, we simply disable this particular check (it's a test after all, not core Gramine functionality). Signed-off-by: Dmitrii Kuvaiskii <[email protected]>
UBSan detects an uninitialized boolean variable on a DCAP SGX machine on the remote attestation example, failing with the message: error: ubsan: load of invalid value for bool or enum: 100 error: ubsan: ../pal/src/host/linux-sgx/pal_misc.c:724:43 This happens because `bool linkable` variable is assigned only in the EPID attestation scheme, but left unassigned in the DCAP scheme. This commit fixes this (though it's not a bug since DCAP never uses `linkable` anyway). Hence, UBSan doesn't complain anymore. Signed-off-by: Dmitrii Kuvaiskii <[email protected]>
Currently used version of LTP has a bug that results in the following build error, e.g. when using Clang v18 on Ubuntu 24.04: clone301.c:136:8: error: call to undeclared function 'pidfd_send_signal' This is because Clang promoted "implicit function declaration" check from a warning to an error starting from Clang v16. This commit adds `-Wno-implicit-function-declaration` flag to CFLAGS during LTP build. Newer LTP releases fixed this bug, so in the future, when Gramine uses newer LTP, this commit should be reverted. Signed-off-by: Dmitrii Kuvaiskii <[email protected]>
833d9bf
to
c492ee8
Compare
Jenkins, test Jenkins-SGX-20.04-apps please |
Jenkins, test Jenkins-SGX-24.04-apps please |
1436039
to
f27a8b3
Compare
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]>
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]>
…ython-platlib Signed-off-by: Wojtek Porczyk <[email protected]>
Signed-off-by: Wojtek Porczyk <[email protected]>
Signed-off-by: Wojtek Porczyk <[email protected]>
- epoll01 300 -> 600 s - pipe 30 -> 60 s Signed-off-by: Wojtek Porczyk <[email protected]>
For some reason memcached-tool sometimes outputs more than 2 lines, which I guess is fine, it means that there is data in the server, so it's working. Signed-off-by: Wojtek Porczyk <[email protected]>
Signed-off-by: Wojtek Porczyk <[email protected]>
Signed-off-by: Wojtek Porczyk <[email protected]>
Signed-off-by: Wojtek Porczyk <[email protected]>
Signed-off-by: Wojtek Porczyk <[email protected]>
Signed-off-by: Wojtek Porczyk <[email protected]>
Signed-off-by: Wojtek Porczyk <[email protected]>
Signed-off-by: Wojtek Porczyk <[email protected]>
f27a8b3
to
0e91e6e
Compare
Description of the changes
Fixes: #1893
How to test this PR?
CI
This change is