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 jammy and noble (22.04 LTS and 24.04 LTS) #1843

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

Conversation

woju
Copy link
Member

@woju woju commented Apr 11, 2024

Description of the changes

noble is scheduled to be released on 25.04.2024, about 2 weeks from now. From Gramine 1.7 we'll support noble and jammy so it's about time to have those in CI.

How to test this PR?

check CI


This change is Reviewable

@woju
Copy link
Member Author

woju commented Apr 12, 2024

Jenkins, test this please

1 similar comment
@woju
Copy link
Member Author

woju commented Apr 12, 2024

Jenkins, test this please

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: 0 of 17 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), "DO NOT MERGE" and "WIP" found in commit messages' one-liners (waiting on @woju)

a discussion (no related file):
I'm putting a blocking comment to not accidentally merge this PR (before the full cycle of validation is done, also from the Intel internal CI side). This PR will surely go in only after Gramine v1.7.


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: 0 of 18 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), "WIP" found in commit messages' one-liners (waiting on @dimakuv)

a discussion (no related file):

Previously, dimakuv (Dmitrii Kuvaiskii) wrote…

I'm putting a blocking comment to not accidentally merge this PR (before the full cycle of validation is done, also from the Intel internal CI side). This PR will surely go in only after Gramine v1.7.

It should be the other way around: we shouldn't release 1.7 without merging this PR. The PR (if ready), should get merged exactly on 25.04. (noble release). If we'll release v1.7 after 25.04, then we'll be bound by this project's policy to support 24.04 and 22.04, irrespective of Intel's internal CI and validation.


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: 0 of 18 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), "WIP" found in commit messages' one-liners (waiting on @woju)

a discussion (no related file):

If we'll release v1.7 after 25.04, then we'll be bound by this project's policy to support 24.04 and 22.04, irrespective of Intel's internal CI and validation.

I'm confused by this sentence. Why would merging this PR exactly on 25.04 change anything? This PR is about adding 22.04 and 24.04, so why can't this PR be merged after 25.04?


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: 0 of 18 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), "DO NOT MERGE" and "WIP" found in commit messages' one-liners (waiting on @dimakuv)

a discussion (no related file):

Previously, dimakuv (Dmitrii Kuvaiskii) wrote…

If we'll release v1.7 after 25.04, then we'll be bound by this project's policy to support 24.04 and 22.04, irrespective of Intel's internal CI and validation.

I'm confused by this sentence. Why would merging this PR exactly on 25.04 change anything? This PR is about adding 22.04 and 24.04, so why can't this PR be merged after 25.04?

Because Ubuntu 24.04 LTS is expected to be released on 25.04.2024: https://launchpad.net/ubuntu/noble, so we target the exact date. If we release 1.7 before 25.04.2024, the we're technically not bound to support noble until 1.8, but if we release 1.7 after 25.04.2024, then we release with noble support, so it would be unwise to release without passing CI.


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: 0 of 18 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), "DO NOT MERGE" and "WIP" found in commit messages' one-liners (waiting on @woju)

a discussion (no related file):

Previously, woju (Wojtek Porczyk) wrote…

Because Ubuntu 24.04 LTS is expected to be released on 25.04.2024: https://launchpad.net/ubuntu/noble, so we target the exact date. If we release 1.7 before 25.04.2024, the we're technically not bound to support noble until 1.8, but if we release 1.7 after 25.04.2024, then we release with noble support, so it would be unwise to release without passing CI.

Yes, all these things I understand. What I find confusing is your statement that this PR must be merged exactly on 25.04. Why merging this PR after 25.04 will be a problem?


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: 0 of 18 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), "DO NOT MERGE" and "WIP" found in commit messages' one-liners (waiting on @dimakuv)

a discussion (no related file):

Previously, dimakuv (Dmitrii Kuvaiskii) wrote…

Yes, all these things I understand. What I find confusing is your statement that this PR must be merged exactly on 25.04. Why merging this PR after 25.04 will be a problem?

I guess it will not be a problem if we do the actual merge slightly after 25.04., but before subsequent release (1.7 or 1.8, whichever it will be). In any case, we need to be ready for merging exactly 25.04, to be ready e.g. for scenario in which we release 1.7 on 26.04.


@woju woju force-pushed the woju/ubuntu-noble branch 12 times, most recently from f87c7c5 to 7e2d16f Compare April 17, 2024 13:12
Copy link
Contributor

@jinengandhi-intel jinengandhi-intel 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 22 files reviewed, 2 unresolved discussions, not enough approvals from maintainers (1 more required), not enough approvals from different teams (1 more required, approved so far: ITL), "DO NOT MERGE" and "WIP" found in commit messages' one-liners (waiting on @dimakuv and @woju)

a discussion (no related file):
Woju as part of the PR you seem to be renaming/removing Ubuntu20.04 but as per Ubuntu (https://wiki.ubuntu.com/Releases) , 20.04 is supported at least till April 2025. So why are we removing support for Ubuntu 20.04?


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.

Reviewable status: 0 of 22 files reviewed, 2 unresolved discussions, not enough approvals from maintainers (1 more required), not enough approvals from different teams (1 more required, approved so far: ITL), "DO NOT MERGE" and "WIP" found in commit messages' one-liners (waiting on @dimakuv and @jinengandhi-intel)

a discussion (no related file):

20.04 is supported at least till April 2025

By Canonical, not by us.

See https://gramine.readthedocs.io/en/latest/devel/packaging.html#packaging-and-distributing. This has always been our policy.


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.

Reviewable status: 0 of 22 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), "DO NOT MERGE" and "WIP" found in commit messages' one-liners (waiting on @dimakuv, @jinengandhi-intel, and @woju)

a discussion (no related file):
Depends on #1865 ([glibc] Adjust CFLAGS should be dropped from here after merging that PR).


Copy link
Contributor

@jinengandhi-intel jinengandhi-intel 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 22 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), "DO NOT MERGE" and "WIP" found in commit messages' one-liners (waiting on @dimakuv and @woju)

a discussion (no related file):

Previously, mkow (Michał Kowalczyk) wrote…

20.04 is supported at least till April 2025

By Canonical, not by us.

See https://gramine.readthedocs.io/en/latest/devel/packaging.html#packaging-and-distributing. This has always been our policy.

Why do we still have 18.04 then in Ubuntu supported distros?


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.

Reviewable status: 0 of 22 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), "DO NOT MERGE" and "WIP" found in commit messages' one-liners (waiting on @dimakuv and @woju)

a discussion (no related file):

Previously, jinengandhi-intel wrote…

Why do we still have 18.04 then in Ubuntu supported distros?

We just forgot to update the list there, it's not supported since long time ago (as the previous sentence there indicates).
@woju: could you update packaging.rst in this PR, to only list 22.04 and 24.04?


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: 0 of 28 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, @jinengandhi-intel, and @mkow)

a discussion (no related file):

Previously, mkow (Michał Kowalczyk) wrote…

We just forgot to update the list there, it's not supported since long time ago (as the previous sentence there indicates).
@woju: could you update packaging.rst in this PR, to only list 22.04 and 24.04?

By all means.


a discussion (no related file):

Previously, mkow (Michał Kowalczyk) wrote…

Depends on #1865 ([glibc] Adjust CFLAGS should be dropped from here after merging that PR).

I've rebased on master and dropped the patch.


@woju
Copy link
Member Author

woju commented May 7, 2024

Jenkins, test Jenkins-SGX-24.04 please

@woju
Copy link
Member Author

woju commented May 7, 2024

Jenkins, test Jenkins-SGX-24.04 please

@woju
Copy link
Member Author

woju commented May 7, 2024

Jenkins, test Jenkins-SGX-22.04 please

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: 0 of 28 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 @mkow and @woju)

a discussion (no related file):
We want to discuss removing the support for Ubuntu 20.04 in the Gramine meetings (on Tuesday and Wednesday). Blocking until that discussion is resolved.


@woju woju marked this pull request as ready for review May 7, 2024 11:37
@woju woju changed the title WIP Ubuntu jammy and noble (22.04 LTS and 24.04 LTS) Ubuntu jammy and noble (22.04 LTS and 24.04 LTS) May 7, 2024
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: 0 of 28 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 @mkow)

a discussion (no related file):

Previously, dimakuv (Dmitrii Kuvaiskii) wrote…

We want to discuss removing the support for Ubuntu 20.04 in the Gramine meetings (on Tuesday and Wednesday). Blocking until that discussion is resolved.

This PR as it currently stands does not remove 20.04 pipelines, so I think it should be moved forward.


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 1 of 19 files at r1, 2 of 3 files at r3, 3 of 5 files at r5, 22 of 22 files at r6, all commit messages.
Reviewable status: all files reviewed, 6 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 32 at r6:
TODO: review the commit split before merging
btw. in the future, @woju, plz make PRs more atomic, this one mixes a lot of stuff and is annoying to review.


.ci/ubuntu24.04.dockerfile line 1 at r6 (raw file):

FROM ubuntu:noble

Suggestion:

FROM ubuntu:24.04

.ci/lib/config-docker.jenkinsfile line 6 at r6 (raw file):

env.DOCKER_ARGS_COMMON = """
    --env=PATH=${env.PREFIX}/bin:${env.PATH}
    --security-opt seccomp=${env.WORKSPACE}/scripts/docker_seccomp.json

Now docker_seccomp.json is not tested in CI at all. Can you move this change out of this PR so we can discuss it separately?


python/gramine-sgx-sign line 135 at r6 (raw file):

        expanded = manifest.expand_all_trusted_files(chroot=chroot)
    except FileNotFoundError as err:
        ctx.fail(f'Missing trusted file: {err.filename!r}')

How is this related to the PR? Can you move it to a separate PR?

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 19 files at r1, 2 of 3 files at r3, 3 of 5 files at r5, 22 of 22 files at r6, all commit messages.
Reviewable status: all files reviewed, 11 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):

Previously, woju (Wojtek Porczyk) wrote…

I guess it will not be a problem if we do the actual merge slightly after 25.04., but before subsequent release (1.7 or 1.8, whichever it will be). In any case, we need to be ready for merging exactly 25.04, to be ready e.g. for scenario in which we release 1.7 on 26.04.

This discussion is not relevant anymore. Gramine v1.7 was released before Ubuntu 24.04 release date, so v1.7 didn't have any support for 24.04.

The next release of Gramine (v1.8) will have support for 24.04.


a discussion (no related file):

Previously, woju (Wojtek Porczyk) wrote…

This PR as it currently stands does not remove 20.04 pipelines, so I think it should be moved forward.

Yes, sounds good.


a discussion (no related file):
Don't you want to add:

  • check-python-platlib-ubuntu24.04.dockerfile
  • check-python-platlib-debian12.dockerfile

a discussion (no related file):
I guess moving some test configurations from 20.04 to e.g. 24.04 will be done in some future PR?

For example, we have:

  • linux-sgx-ubuntu20.04-musl.jenkinsfile which tests Musl, and it's the only such pipeline
  • linux-direct-ubuntu20.04-gcc-debug.jenkinsfile which tests debug build, and it's the only such pipeline
  • linux-sgx-ubuntu20.04-gcc-release-apps.jenkinsfile which tests CI-Examples applications under SGX, and it's the only such pipeline


.ci/ubuntu24.04.dockerfile line 3 at r6 (raw file):

FROM ubuntu:noble

ENV DEBIAN_FRONTEND=noninteractive

Maybe we could do the same in other Dockerfiles? I see that Ubuntu 22.04 and 20.04 don't have this envvar, but instead embed it directly in commands.


.ci/ubuntu24.04.dockerfile line 18 at r6 (raw file):

# in dockerfile.
RUN mkdir /debian
COPY debian/control /debian

This is smart.

I guess you don't want to bother doing the same for Ubuntu 20.04 Dockerfile, but what about Ubuntu 22.04? Don't you want to unify the commands in Dockerfiles?


.ci/ubuntu24.04.dockerfile line 34 at r6 (raw file):

    'python3-pyelftools' \
    'python3-tomli (>= 1.1.0)' \
    'python3-tomli-w (>= 0.4.0)'

Where is voluptuous?

python3-voluptuous,


.ci/lib/config-docker.jenkinsfile line 6 at r6 (raw file):

Previously, mkow (Michał Kowalczyk) wrote…

Now docker_seccomp.json is not tested in CI at all. Can you move this change out of this PR so we can discuss it separately?

+1, it would be nice to test this in CI. Did you experience any problems with our current seccomp filter? If yes, we should fix/update it, not remove the test. If you didn't experience any issues, then we can keep it + have your new comment, but in the comment we say that "could run with unconfined but for testing purposes, we use an explicit seccomp config"


Documentation/devel/packaging.rst line 10 at r6 (raw file):

Currently officially supported distributions:

- Ubuntu (24.04 LTS, 22.04 LTS);

Based on our discussions (to really remove support for Ubuntu 20.04 in v1.9) I think we should still list it as officially supported.


python/gramine-sgx-sign line 135 at r6 (raw file):

Previously, mkow (Michał Kowalczyk) wrote…

How is this related to the PR? Can you move it to a separate PR?

+1, this looks totally unrelated to CI changes.

woju added 2 commits May 10, 2024 17:32
Signed-off-by: Wojtek Porczyk <woju@invisiblethingslab.com>
Signed-off-by: Wojtek Porczyk <woju@invisiblethingslab.com>
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: 24 of 29 files reviewed, 11 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 @oshogbo)

a discussion (no related file):

Previously, dimakuv (Dmitrii Kuvaiskii) wrote…

This discussion is not relevant anymore. Gramine v1.7 was released before Ubuntu 24.04 release date, so v1.7 didn't have any support for 24.04.

The next release of Gramine (v1.8) will have support for 24.04.

Done.


a discussion (no related file):

Previously, dimakuv (Dmitrii Kuvaiskii) wrote…

I guess moving some test configurations from 20.04 to e.g. 24.04 will be done in some future PR?

For example, we have:

  • linux-sgx-ubuntu20.04-musl.jenkinsfile which tests Musl, and it's the only such pipeline
  • linux-direct-ubuntu20.04-gcc-debug.jenkinsfile which tests debug build, and it's the only such pipeline
  • linux-sgx-ubuntu20.04-gcc-release-apps.jenkinsfile which tests CI-Examples applications under SGX, and it's the only such pipeline

Yes. For now I wanted to have just those two pipelines, to check compilation, to limit the scope of the PR. I expect most of the problems will be related to compilation, not to tests (two problems were already found here and fixed by @oshogbo in #1865)


a discussion (no related file):

Previously, dimakuv (Dmitrii Kuvaiskii) wrote…

Don't you want to add:

  • check-python-platlib-ubuntu24.04.dockerfile
  • check-python-platlib-debian12.dockerfile

Not in this PR I think.



-- commits line 32 at r6:

Previously, mkow (Michał Kowalczyk) wrote…

TODO: review the commit split before merging
btw. in the future, @woju, plz make PRs more atomic, this one mixes a lot of stuff and is annoying to review.

Even more annoying is waiting for review in this project, so I think I'll be submitting PRs that have related stuff (e.g. all the stuff that is needed to run a pipeline on a new distro), just properly split into atomic commits. You can always switch reviewable to review commit by commit.


.ci/ubuntu24.04.dockerfile line 1 at r6 (raw file):

FROM ubuntu:noble

Done.


.ci/ubuntu24.04.dockerfile line 3 at r6 (raw file):

Previously, dimakuv (Dmitrii Kuvaiskii) wrote…

Maybe we could do the same in other Dockerfiles? I see that Ubuntu 22.04 and 20.04 don't have this envvar, but instead embed it directly in commands.

Done for 22.04. I don't want to touch 20.04.


.ci/ubuntu24.04.dockerfile line 18 at r6 (raw file):

Previously, dimakuv (Dmitrii Kuvaiskii) wrote…

This is smart.

I guess you don't want to bother doing the same for Ubuntu 20.04 Dockerfile, but what about Ubuntu 22.04? Don't you want to unify the commands in Dockerfiles?

I think it's impossible on Ubuntu 20.04, because apt-get build-dep will fail for dependencies that are not available in focal. I did a version for 22.04.


.ci/ubuntu24.04.dockerfile line 34 at r6 (raw file):

Previously, dimakuv (Dmitrii Kuvaiskii) wrote…

Where is voluptuous?

python3-voluptuous,

Missed, sorry. It's in debian/control in Build-Depends, and got installed by apt-get build-dep above.


.ci/lib/config-docker.jenkinsfile line 6 at r6 (raw file):

Previously, dimakuv (Dmitrii Kuvaiskii) wrote…

+1, it would be nice to test this in CI. Did you experience any problems with our current seccomp filter? If yes, we should fix/update it, not remove the test. If you didn't experience any issues, then we can keep it + have your new comment, but in the comment we say that "could run with unconfined but for testing purposes, we use an explicit seccomp config"

Yes, there was a problem. Let's see what happens if I switch to newer version.


Documentation/devel/packaging.rst line 10 at r6 (raw file):

Previously, dimakuv (Dmitrii Kuvaiskii) wrote…

Based on our discussions (to really remove support for Ubuntu 20.04 in v1.9) I think we should still list it as officially supported.

Done. I think I'll remove it when I'll remove 20.04 pipelines, which may be before 1.9 (or even before 1.8), but not right now.


python/gramine-sgx-sign line 135 at r6 (raw file):

Previously, dimakuv (Dmitrii Kuvaiskii) wrote…

+1, this looks totally unrelated to CI changes.

#1880

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 5 of 5 files at r7, all commit messages.
Reviewable status: all files reviewed, 7 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)


.ci/ubuntu22.04.dockerfile line 9 at r7 (raw file):

# Intel's RSA-2048 key signing the intel-sgx/sgx_repo repository. Expires 2027-03-20.
# https://download.01.org/intel-sgx/sgx_repo/ubuntu/intel-sgx-deb.key
# TODO after Intel releases for noble: fix mantic to noble

This TODO is not applicable here.


.ci/ubuntu22.04.dockerfile line 11 at r7 (raw file):

# TODO after Intel releases for noble: fix mantic to noble
COPY .ci/intel-sgx-deb.key /etc/apt/keyrings/intel-sgx-deb.asc
RUN echo deb [arch=amd64 signed-by=/etc/apt/keyrings/intel-sgx-deb.asc] https://download.01.org/intel-sgx/sgx_repo/ubuntu mantic main > /etc/apt/sources.list.d/intel-sgx.list

I think you want jammy instead of mantic


.ci/ubuntu22.04.dockerfile line 43 at r7 (raw file):

# shellcheck: .ci/run-shellcheck
# cargo: CI-Examples/rust
# clang: FIXME

What is this FIXME? Clang is used for AddressSanitizer builds.

woju added 5 commits May 14, 2024 13:14
Signed-off-by: Wojtek Porczyk <woju@invisiblethingslab.com>
Signed-off-by: Wojtek Porczyk <woju@invisiblethingslab.com>
Signed-off-by: Wojtek Porczyk <woju@invisiblethingslab.com>
Signed-off-by: Wojtek Porczyk <woju@invisiblethingslab.com>
Signed-off-by: Wojtek Porczyk <woju@invisiblethingslab.com>
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: 24 of 29 files reviewed, 7 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)


.ci/ubuntu22.04.dockerfile line 9 at r7 (raw file):

Previously, dimakuv (Dmitrii Kuvaiskii) wrote…

This TODO is not applicable here.

Done.


.ci/ubuntu22.04.dockerfile line 11 at r7 (raw file):

Previously, dimakuv (Dmitrii Kuvaiskii) wrote…

I think you want jammy instead of mantic

Done.


.ci/ubuntu22.04.dockerfile line 43 at r7 (raw file):

Previously, dimakuv (Dmitrii Kuvaiskii) wrote…

What is this FIXME? Clang is used for AddressSanitizer builds.

Done.

dimakuv
dimakuv previously approved these changes May 14, 2024
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 5 of 5 files at r8, all commit messages.
Reviewable status: all files reviewed, 4 unresolved discussions (waiting on @mkow)

Signed-off-by: Wojtek Porczyk <woju@invisiblethingslab.com>
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: 28 of 29 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 @dimakuv and @mkow)


.ci/lib/config-docker.jenkinsfile line 6 at r6 (raw file):

Previously, woju (Wojtek Porczyk) wrote…

Yes, there was a problem. Let's see what happens if I switch to newer version.

So eventually I just pass seccomp argument to docker().inside(). Rationale is that it depends on node() specifier, so I think it's fair to have it variable per-node.

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: 28 of 29 files reviewed, 5 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)


.ci/ubuntu24.04.dockerfile line 9 at r9 (raw file):

# Intel's RSA-2048 key signing the intel-sgx/sgx_repo repository. Expires 2027-03-20.
# https://download.01.org/intel-sgx/sgx_repo/ubuntu/intel-sgx-deb.key
# TODO after Intel releases for noble: fix mantic to noble

I'm putting a blocking comment for now: we need to discuss how it affects users. Technically, we should at least warn users that when they git-clone the master branch on Ubuntu 24.04, and they want to use RA-TLS (or whatever with attestation), which requires Intel SGX PSW/DCAP packages/libraries. We currently silently use 23.10-based packages for Intel SGX PSW/DCAP, but (1) end users won't know this trick, and (2) technically it's wrong to use packages from another OS distro version.

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: 28 of 29 files reviewed, 5 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)


.ci/ubuntu24.04.dockerfile line 9 at r9 (raw file):

Previously, dimakuv (Dmitrii Kuvaiskii) wrote…

I'm putting a blocking comment for now: we need to discuss how it affects users. Technically, we should at least warn users that when they git-clone the master branch on Ubuntu 24.04, and they want to use RA-TLS (or whatever with attestation), which requires Intel SGX PSW/DCAP packages/libraries. We currently silently use 23.10-based packages for Intel SGX PSW/DCAP, but (1) end users won't know this trick, and (2) technically it's wrong to use packages from another OS distro version.

(2) is true for RPMs, but false for DEBs: on debian/ubuntu the packages themselves may be OK to use on different distro versions and even for different distros (like using debian packages on ubuntu and vice versa). What matters is how the packages are linked, IOW if the distro ships a package of a library with matching SONAME, incl. version if the soname is versioned. ISTM that those specific mantic packages are OK to use on noble.

(1) might be true or not, but it's not our problem that users don't know something.

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: 28 of 29 files reviewed, 5 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)


.ci/ubuntu24.04.dockerfile line 9 at r9 (raw file):

Previously, woju (Wojtek Porczyk) wrote…

(2) is true for RPMs, but false for DEBs: on debian/ubuntu the packages themselves may be OK to use on different distro versions and even for different distros (like using debian packages on ubuntu and vice versa). What matters is how the packages are linked, IOW if the distro ships a package of a library with matching SONAME, incl. version if the soname is versioned. ISTM that those specific mantic packages are OK to use on noble.

(1) might be true or not, but it's not our problem that users don't know something.

Oh, and this is CI, not anything that we support for users. FWIW users who don't want to use mantic repos can recompile those libraries themselves.

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 1 files at r9, all commit messages.
Reviewable status: all files reviewed, 4 unresolved discussions (waiting on @mkow)


.ci/ubuntu24.04.dockerfile line 9 at r9 (raw file):

Previously, woju (Wojtek Porczyk) wrote…

Oh, and this is CI, not anything that we support for users. FWIW users who don't want to use mantic repos can recompile those libraries themselves.

Ok, I'm unblocking this. I added a note in our roadmap here on GitHub, that if PSW/DCAP packages are not yet released for 24.04 at the time we release Gramine v1.8, we'll need to put a note/warning in our Release Notes. I think this is a fair compromise.

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 5 files at r7, 4 of 5 files at r8, 1 of 1 files at r9, all commit messages.
Reviewable status: all files reviewed, 4 unresolved discussions (waiting on @woju)

a discussion (no related file):

Previously, woju (Wojtek Porczyk) wrote…

Done.

@woju: Please update the description of the PR accordingly, so it's not confusing for the people reading the merge log later.



-- commits line 32 at r6:

You can always switch reviewable to review commit by commit.

I can't, only the first reviewer can decide about that.

Even more annoying is waiting for review in this project

It would have gone much faster if you had split it into smaller PRs, because reviewing most of them would be trivial and we could have merged them right away (also, they wouldn't be blocked on discussions about other changes).


.ci/ubuntu22.04.dockerfile line 36 at r9 (raw file):

    'python3-voluptuous'

# dependencies for various tests, CI-Examples, etc.

Why is this list so different from 24.04? Aren't we running all these tests on both?


.ci/lib/config-docker.jenkinsfile line 6 at r6 (raw file):

Previously, woju (Wojtek Porczyk) wrote…

So eventually I just pass seccomp argument to docker().inside(). Rationale is that it depends on node() specifier, so I think it's fair to have it variable per-node.

Ok, but the original issue still stands - docker_seccomp.json is not tested anymore.

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.

None yet

4 participants