-
Notifications
You must be signed in to change notification settings - Fork 33
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
Add support for CentOS Stream 9 #206
Add support for CentOS Stream 9 #206
Conversation
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 8 of 8 files at r1, all commit messages.
Reviewable status: all files reviewed, 21 unresolved discussions, not enough approvals from maintainers (3 more required), not enough approvals from different teams (2 more required, approved so far: ) (waiting on @adarshan-intel)
config.yaml.template
line 9 at r1 (raw file):
# - ubuntu:20.04, ubuntu:21.04, ubuntu:22.04, ubuntu:23.04 # - debian:10, debian:11, debian:12 # - centos:8, quay.io/centos/centos:stream9
It looks like there is no official CentOS Stream 9 on DockerHub? Interesting. Do you know the reason for this? (All other base OS images we pull from DockerHub.)
config.yaml.template
line 9 at r1 (raw file):
# - ubuntu:20.04, ubuntu:21.04, ubuntu:22.04, ubuntu:23.04 # - debian:10, debian:11, debian:12 # - centos:8, quay.io/centos/centos:stream9
I think these are sufficiently different CentOS distros, so we should split them into separate lines:
# - centos:8
# - centos:stream9 (pulled from quay.io/centos/centos)
gsc.py
line 263 at r1 (raw file):
distro = 'redhat/ubi8:' + version_id if (os_release['ID'] == 'centos' and version_id=='9'):
version_id == '9'
(add spaces around the equals sign)
templates/quay.io/centos/centos/Dockerfile.build.template
line 20 at r1 (raw file):
'tomli>=1.1.0' 'tomli-w>=0.4.0' \ && dnf repolist \ && dnf install -y \
Can you add the same comment as here:
# Install pyelftools and voluptuous after the installation of epel-release as it is provided by the |
templates/quay.io/centos/centos/Dockerfile.compile.template
line 5 at r1 (raw file):
{% block install %} RUN dnf distro-sync -y && dnf install 'dnf-command(config-manager)' -y
What is this, why is this needed? Could you share some references/links where I can read more? Also, can you add a short comment here?
templates/quay.io/centos/centos/Dockerfile.compile.template
line 7 at r1 (raw file):
RUN dnf distro-sync -y && dnf install 'dnf-command(config-manager)' -y RUN dnf config-manager --set-enabled -y crb && \
ditto, what is this?
templates/quay.io/centos/centos/Dockerfile.compile.template
line 7 at r1 (raw file):
RUN dnf distro-sync -y && dnf install 'dnf-command(config-manager)' -y RUN dnf config-manager --set-enabled -y crb && \
Please add this comment:
# NOTE: meson v1.2.* has a bug that leads to Gramine build failure because of not found `libcurl.a` |
templates/quay.io/centos/centos/Dockerfile.compile.template
line 9 at r1 (raw file):
RUN dnf config-manager --set-enabled -y crb && \ dnf install -y yum-utils && \ dnf install -y epel-release && \
Why do you need to have epel-release
installed separately from other packages? In the original CentOS Dockerfile, we installed it together with all other packages:
epel-release \ |
Update: you also install epel-release
below, in the list of packages... Please remove one of these lines.
templates/quay.io/centos/centos/Dockerfile.compile.template
line 10 at r1 (raw file):
dnf install -y yum-utils && \ dnf install -y epel-release && \ dnf install -y \
This should be un-intended by 4 spaces
templates/quay.io/centos/centos/Dockerfile.compile.template
line 12 at r1 (raw file):
dnf install -y \ autoconf \ bc \
You sure you need this package? It's not needed in CentOS 8:
bison \ |
templates/quay.io/centos/centos/Dockerfile.compile.template
line 14 at r1 (raw file):
bc \ binutils \ bison \
Why is there no curl
here? See
curl \ |
Update: probably this is replaced by libcurl-devel
, which I see below?
templates/quay.io/centos/centos/Dockerfile.compile.template
line 20 at r1 (raw file):
gawk \ gcc-c++ \ gdb \
Why do you need gdb? Please remove.
templates/quay.io/centos/centos/Dockerfile.compile.template
line 23 at r1 (raw file):
git \ glibc-static \ glibc.i686 \
You sure you need these two glibc*
packages?
templates/quay.io/centos/centos/Dockerfile.compile.template
line 26 at r1 (raw file):
httpd \ libcurl-devel \ libjpeg-turbo-devel \
I can't believe this is needed
templates/quay.io/centos/centos/Dockerfile.compile.template
line 27 at r1 (raw file):
libcurl-devel \ libjpeg-turbo-devel \ libmemcached \
Not needed?
templates/quay.io/centos/centos/Dockerfile.compile.template
line 28 at r1 (raw file):
libjpeg-turbo-devel \ libmemcached \ libunwind \
Not needed?
templates/quay.io/centos/centos/Dockerfile.compile.template
line 32 at r1 (raw file):
libXfixes \ libXrender \ libXtst \
Surely all these libX*
are not needed
templates/quay.io/centos/centos/Dockerfile.compile.template
line 33 at r1 (raw file):
libXrender \ libXtst \ lsof \
Not needed?
templates/quay.io/centos/centos/Dockerfile.compile.template
line 37 at r1 (raw file):
nasm \ ncurses-devel \ ncurses-libs \
Not needed? I think ncurses-devel
is enough.
templates/quay.io/centos/centos/Dockerfile.compile.template
line 40 at r1 (raw file):
ninja-build \ nss-mdns \ nss-myhostname \
Both these nss-*
packages not needed?
templates/quay.io/centos/centos/Dockerfile.compile.template
line 51 at r1 (raw file):
python3-click \ python3-cryptography \ python3-devel \
You sure this one is needed?
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
Yes, this is true. There is no official image for CentOS Stream 9, so I use the community-based image for CentOS Stream 9. |
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: 4 of 8 files reviewed, 21 unresolved discussions, not enough approvals from maintainers (3 more required), not enough approvals from different teams (2 more required, approved so far: ), "fixup! " found in commit messages' one-liners (waiting on @dimakuv)
config.yaml.template
line 9 at r1 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
I think these are sufficiently different CentOS distros, so we should split them into separate lines:
# - centos:8 # - centos:stream9 (pulled from quay.io/centos/centos)
Done.
gsc.py
line 263 at r1 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
version_id == '9'
(add spaces around the equals sign)
Done.
templates/quay.io/centos/centos/Dockerfile.build.template
line 20 at r1 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
Can you add the same comment as here:
# Install pyelftools and voluptuous after the installation of epel-release as it is provided by the
Done.
templates/quay.io/centos/centos/Dockerfile.compile.template
line 5 at r1 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
What is this, why is this needed? Could you share some references/links where I can read more? Also, can you add a short comment here?
The dnf-command(config-manager)
is a plugin for the DNF package manager in RPM-based Linux distributions. It provides a command-line interface for managing DNF repositories and DNF configuration options. With it, users can enable or disable repositories, add new repositories, and adjust various DNF settings without editing config files directly.
Chapter 10. Managing custom software repositories | Red Hat Product Documentation
This is needed to install packages listed in Dockerfile. Else we get this error
Error: Unable to find a match: glibc-static libmemcached libunwind nasm ninja-build nss-mdns protobuf-c-compiler protobuf-c-devel protobuf-compiler protobuf-devel python3-click
templates/quay.io/centos/centos/Dockerfile.compile.template
line 7 at r1 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
ditto, what is this?
The dnf config-manager --set-enabled -y crb
part uses the DNF package manager to enable the CodeReady Builder (CRB) repository which is needed to install certain packages.
templates/quay.io/centos/centos/Dockerfile.compile.template
line 7 at r1 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
Please add this comment:
# NOTE: meson v1.2.* has a bug that leads to Gramine build failure because of not found `libcurl.a`
Done.
templates/quay.io/centos/centos/Dockerfile.compile.template
line 9 at r1 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
Why do you need to have
epel-release
installed separately from other packages? In the original CentOS Dockerfile, we installed it together with all other packages:
epel-release \ Update: you also install
epel-release
below, in the list of packages... Please remove one of these lines.
Done.
templates/quay.io/centos/centos/Dockerfile.compile.template
line 12 at r1 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
You sure you need this package? It's not needed in CentOS 8:
bison \
Done.
templates/quay.io/centos/centos/Dockerfile.compile.template
line 14 at r1 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
Why is there no
curl
here? See
curl \ Update: probably this is replaced by
libcurl-devel
, which I see below?
There is already an existing installation of curl in CentOS Stream 9. Therefore, installing another curl package would lead to conflicts, so it not adding in the dockerfile.
templates/quay.io/centos/centos/Dockerfile.compile.template
line 20 at r1 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
Why do you need gdb? Please remove.
Done.
templates/quay.io/centos/centos/Dockerfile.compile.template
line 23 at r1 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
You sure you need these two
glibc*
packages?
Done.
templates/quay.io/centos/centos/Dockerfile.compile.template
line 26 at r1 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
I can't believe this is needed
Done.
templates/quay.io/centos/centos/Dockerfile.compile.template
line 27 at r1 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
Not needed?
Done.
templates/quay.io/centos/centos/Dockerfile.compile.template
line 28 at r1 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
Not needed?
Done.
templates/quay.io/centos/centos/Dockerfile.compile.template
line 32 at r1 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
Surely all these
libX*
are not needed
Done.
templates/quay.io/centos/centos/Dockerfile.compile.template
line 33 at r1 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
Not needed?
Done.
templates/quay.io/centos/centos/Dockerfile.compile.template
line 37 at r1 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
Not needed? I think
ncurses-devel
is enough.
Done.
templates/quay.io/centos/centos/Dockerfile.compile.template
line 40 at r1 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
Both these
nss-*
packages not needed?
Done.
templates/quay.io/centos/centos/Dockerfile.compile.template
line 51 at r1 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
You sure this one is needed?
Done.
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 4 files at r2, all commit messages.
Reviewable status: all files reviewed, 3 unresolved discussions, not enough approvals from maintainers (3 more required), not enough approvals from different teams (2 more required, approved so far: ), "fixup! " found in commit messages' one-liners (waiting on @adarshan-intel)
a discussion (no related file):
How could I test this? It seems to require RHEL subscription.
config.yaml.template
line 9 at r1 (raw file):
Previously, adarshan-intel (Adarsh Anand) wrote…
Yes, this is true. There is no official image for CentOS Stream 9, so I use the community-based image for CentOS Stream 9.
Yes, this seems to be so. Let's use quay.io
then.
templates/quay.io/centos/centos/Dockerfile.compile.template
line 5 at r1 (raw file):
Previously, adarshan-intel (Adarsh Anand) wrote…
The
dnf-command(config-manager)
is a plugin for the DNF package manager in RPM-based Linux distributions. It provides a command-line interface for managing DNF repositories and DNF configuration options. With it, users can enable or disable repositories, add new repositories, and adjust various DNF settings without editing config files directly.Chapter 10. Managing custom software repositories | Red Hat Product Documentation
This is needed to install packages listed in Dockerfile. Else we get this error
Error: Unable to find a match: glibc-static libmemcached libunwind nasm ninja-build nss-mdns protobuf-c-compiler protobuf-c-devel protobuf-compiler protobuf-devel python3-click
Thanks for the explanation
templates/quay.io/centos/centos/Dockerfile.compile.template
line 7 at r1 (raw file):
Previously, adarshan-intel (Adarsh Anand) wrote…
The
dnf config-manager --set-enabled -y crb
part uses the DNF package manager to enable the CodeReady Builder (CRB) repository which is needed to install certain packages.
Ok, I learnt about CRB: https://discussion.fedoraproject.org/t/epel-crb-it-is-the-same/44433/7
So, does this mean that in order to use this CentOS Stream 9 in GSC, the user needs to subscribe to Red Hat Enterprise Linux (RHEL)?
templates/quay.io/centos/centos/Dockerfile.compile.template
line 42 at r2 (raw file):
&& /usr/bin/python3 -B -m pip install 'tomli>=1.1.0' 'tomli-w>=0.4.0' 'meson>=0.56,!=1.2.*' {% endblock %}
Please add a newline symbol here
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: 7 of 8 files reviewed, 3 unresolved discussions, not enough approvals from maintainers (3 more required), not enough approvals from different teams (2 more required, approved so far: ), "fixup! " found in commit messages' one-liners (waiting on @dimakuv)
a discussion (no related file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
How could I test this? It seems to require RHEL subscription.
No any system should be able to run CentOS Stream 9, I have tested this on Ubuntu 24.04 Machine.
templates/quay.io/centos/centos/Dockerfile.compile.template
line 7 at r1 (raw file):
So, does this mean that in order to use this CentOS Stream 9 in GSC, the user needs to subscribe to Red Hat Enterprise Linux (RHEL)?
No any system should be able to run CentOS Stream 9, I have tested this on Ubuntu 24.04 Machine
templates/quay.io/centos/centos/Dockerfile.compile.template
line 10 at r1 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
This should be un-intended by 4 spaces
Done.
templates/quay.io/centos/centos/Dockerfile.compile.template
line 42 at r2 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
Please add a newline symbol here
Done.
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 1 of 1 files at r3, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion, not enough approvals from maintainers (3 more required), not enough approvals from different teams (2 more required, approved so far: ), "fixup! " found in commit messages' one-liners (waiting on @adarshan-intel)
a discussion (no related file):
Previously, adarshan-intel (Adarsh Anand) wrote…
No any system should be able to run CentOS Stream 9, I have tested this on Ubuntu 24.04 Machine.
Ok, I tested it on my local machine, using a trivial HelloWorld example. This is to reproduce my steps:
- Prepare the minimal Dockerfile and manifest:
$ cat test/centos9-hello-world.dockerfile
From quay.io/centos/centos:stream9
CMD ["echo", "\"Hello World!\""]
$ cat test/centos9-hello-world.manifest
# intentionally left empty so that GSC uses default manifest options
- Run GSC:
$ docker build --tag centos9-helloworld --file test/centos9-hello-world.dockerfile .
$ ./gsc build --insecure-args centos9-helloworld test/centos9-hello-world.manifest
$ ./gsc sign-image centos9-helloworld enclave-key.pem
- Run Docker:
$ docker run --device=/dev/sgx_enclave \
-v /var/run/aesmd/aesm.socket:/var/run/aesmd/aesm.socket \
gsc-centos9-helloworld
/gramine/meson_build_output/lib64/gramine/sgx/loader: error while loading shared libraries: libprotobuf-c.so.1: cannot open shared object file: No such file or directory
@adarshan-intel I get an error about missing file libprotobuf-c.so.1
. Could you test yourself? Looks like some package is missing.
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: 6 of 8 files reviewed, 1 unresolved discussion, 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)
a discussion (no related file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
Ok, I tested it on my local machine, using a trivial HelloWorld example. This is to reproduce my steps:
- Prepare the minimal Dockerfile and manifest:
$ cat test/centos9-hello-world.dockerfile From quay.io/centos/centos:stream9 CMD ["echo", "\"Hello World!\""] $ cat test/centos9-hello-world.manifest # intentionally left empty so that GSC uses default manifest options
- Run GSC:
$ docker build --tag centos9-helloworld --file test/centos9-hello-world.dockerfile . $ ./gsc build --insecure-args centos9-helloworld test/centos9-hello-world.manifest $ ./gsc sign-image centos9-helloworld enclave-key.pem
- Run Docker:
$ docker run --device=/dev/sgx_enclave \ -v /var/run/aesmd/aesm.socket:/var/run/aesmd/aesm.socket \ gsc-centos9-helloworld /gramine/meson_build_output/lib64/gramine/sgx/loader: error while loading shared libraries: libprotobuf-c.so.1: cannot open shared object file: No such file or directory
@adarshan-intel I get an error about missing file
libprotobuf-c.so.1
. Could you test yourself? Looks like some package is missing.
Yes, fixed the issue by installing protobuf-c-compiler, java-11-openjdk, java-11-openjdk-devel packages
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 2 of 2 files at r4, 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 @adarshan-intel)
templates/quay.io/centos/centos/Dockerfile.build.template
line 21 at r2 (raw file):
&& dnf repolist \ # Install pyelftools and voluptuous after the installation of epel-release as it is provided by the # EPEL repo
Why did you remove this comment?
templates/quay.io/centos/centos/Dockerfile.build.template
line 26 at r2 (raw file):
python3-voluptuous \ && dnf -y clean all
Please re-add the empty line, otherwise it's hard to read this code
templates/quay.io/centos/centos/Dockerfile.build.template
line 15 at r4 (raw file):
epel-release \ java-11-openjdk \ java-11-openjdk-devel \
Why would you need Java 11 installed here?
templates/quay.io/centos/centos/Dockerfile.build.template
line 23 at r4 (raw file):
python3-protobuf \ && /usr/bin/python3 -B -m pip install click jinja2 protobuf \ 'tomli>=1.1.0' 'tomli-w>=0.4.0' 'meson>=0.56,!=1.2.*' \
I don't think you need Meson in this .build.template
file
templates/quay.io/centos/centos/Dockerfile.compile.template
line 10 at r4 (raw file):
RUN dnf config-manager --set-enabled -y crb && \ dnf install -y \
In the .build.template
file, you have this code slightly differently:
RUN dnf update -y \
&& dnf distro-sync -y && dnf install 'dnf-command(config-manager)' -y \
&& dnf config-manager --set-enabled -y crb \
&& dnf install -y \
...
Can you use the same style in this .compile.template
file?
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: 2 of 8 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 @dimakuv)
templates/quay.io/centos/centos/Dockerfile.build.template
line 21 at r2 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
Why did you remove this comment?
Done.
templates/quay.io/centos/centos/Dockerfile.build.template
line 26 at r2 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
Please re-add the empty line, otherwise it's hard to read this code
Done.
templates/quay.io/centos/centos/Dockerfile.build.template
line 15 at r4 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
Why would you need Java 11 installed here?
If I don't add it, it shows this error
[P1:T1:] error: libos_init() failed in init_exec_handle: No such file or directory (ENOENT)
So I have removed java-11-openjdk, and keeping java-11-openjdk-devel package
templates/quay.io/centos/centos/Dockerfile.build.template
line 23 at r4 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
I don't think you need Meson in this
.build.template
file
Done.
templates/quay.io/centos/centos/Dockerfile.compile.template
line 10 at r4 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
In the
.build.template
file, you have this code slightly differently:RUN dnf update -y \ && dnf distro-sync -y && dnf install 'dnf-command(config-manager)' -y \ && dnf config-manager --set-enabled -y crb \ && dnf install -y \ ...
Can you use the same style in this
.compile.template
file?
Done.
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 6 of 6 files at r5, all commit messages.
Reviewable status: all files reviewed, 4 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 @adarshan-intel)
gsc.py
line 196 at r5 (raw file):
version_id = 8 elif distro in {"redhat/ubi9", "redhat/ubi9-minimal"}: version_id = 9
Now this PR has unrelated changes from #207 (it is based on that PR).
I'm putting a blocking comment so that we first merge #207, and then we rebase this PR on top of that one.
templates/quay.io/centos/centos/Dockerfile.build.template
line 15 at r4 (raw file):
Previously, adarshan-intel (Adarsh Anand) wrote…
If I don't add it, it shows this error
[P1:T1:] error: libos_init() failed in init_exec_handle: No such file or directory (ENOENT)
So I have removed java-11-openjdk, and keeping java-11-openjdk-devel package
But that's because you run some Java workload, and you seem to not have a proper Docker image for that Java workload?
Gramine has absolutely nothing to do with Java. This should be removed, and your issue must be solved at the application Docker image level.
templates/quay.io/centos/centos/Dockerfile.compile.template
line 10 at r4 (raw file):
Previously, adarshan-intel (Adarsh Anand) wrote…
Done.
That's not exactly what I wanted, sorry for confusing. I wanted that both files use this "one RUN command" style. Could you change?
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, 4 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)
gsc.py
line 196 at r5 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
Now this PR has unrelated changes from #207 (it is based on that PR).
I'm putting a blocking comment so that we first merge #207, and then we rebase this PR on top of that one.
Yes, those are unrelated changes. Are there any changes that I need to make from my side, or is the code OK?
templates/quay.io/centos/centos/Dockerfile.build.template
line 15 at r4 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
But that's because you run some Java workload, and you seem to not have a proper Docker image for that Java workload?
Gramine has absolutely nothing to do with Java. This should be removed, and your issue must be solved at the application Docker image level.
Can you suggest what can I do here, since removing this package shows this error?
Not sure how to fix it
templates/quay.io/centos/centos/Dockerfile.compile.template
line 10 at r4 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
That's not exactly what I wanted, sorry for confusing. I wanted that both files use this "one RUN command" style. Could you change?
Done.
32b4a96
to
c9db246
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: 2 of 8 files reviewed, 4 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)
gsc.py
line 196 at r5 (raw file):
Now this PR has unrelated changes from
Removed the unrelated changes and only kept changes related to centos stream 9
Previously, adarshan-intel (Adarsh Anand) wrote…
If I install the "which" package, the build will be successful. As earlier it was not able to execute the command
|
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 5 of 6 files at r7, 1 of 1 files at r8, all commit messages.
Reviewable status: all files reviewed, 2 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 @adarshan-intel)
templates/quay.io/centos/centos/Dockerfile.compile.template
line 10 at r4 (raw file):
Previously, adarshan-intel (Adarsh Anand) wrote…
Done.
Not done? What I wanted is to have only one RUN
command and concatenate all dnf ...
commands under this single RUN.
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: 6 of 8 files reviewed, 2 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)
templates/quay.io/centos/centos/Dockerfile.compile.template
line 10 at r4 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
Not done? What I wanted is to have only one
RUN
command and concatenate alldnf ...
commands under this single RUN.
Done in this commit - 37e0cd4
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 2 of 2 files at r9, 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 (2 more required, approved so far: ), "fixup! " found in commit messages' one-liners
Previously, adarshan-intel (Adarsh Anand) wrote…
@dimakuv Is this still blocking? I have installed the which, protobuf-c-compiler package and removed the Java packages. |
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 (2 more required, approved so far: ), "fixup! " found in commit messages' one-liners (waiting on @anjalirai-intel and @jkr0103)
a discussion (no related file):
Previously, adarshan-intel (Adarsh Anand) wrote…
@dimakuv Is this still blocking? I have installed the which, protobuf-c-compiler package and removed the Java packages.
I'm hitting these errors in my Docker now: docker/docker-py#3256
I'll deal with this later. @jkr0103 @anjalirai-intel Did you test this PR?
@dimakuv We did not face any such issue. The CI setup we have for running gsc workloads is stable. We have tested the helloworld, bash example with distro as "auto", "distro specified" in config.yaml and also tested "gsc build-gramine" feature. No issues seen |
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 all commit messages.
Reviewable status: 7 of 8 files reviewed, 3 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 @adarshan-intel, @dimakuv, and @jkr0103)
gsc.py
line 241 at r10 (raw file):
Previously, adarshan-intel (Adarsh Anand) wrote…
Done. Yes once #207 is merged, I will again re-request a review for this and fix the merge conflicts
Why did you say "done" then? It's not done.
gsc.py
line 268 at r10 (raw file):
Previously, adarshan-intel (Adarsh Anand) wrote…
Done.
Not done.
Previously, mkow (Michał Kowalczyk) wrote…
Done. Sorry my bad. |
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: 7 of 8 files reviewed, 3 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, @jkr0103, and @mkow)
gsc.py
line 241 at r10 (raw file):
Previously, mkow (Michał Kowalczyk) wrote…
Why did you say "done" then? It's not done.
Yes I mean this is the only thing pending here.
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 all commit messages.
Reviewable status: 7 of 8 files reviewed, 3 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 @adarshan-intel, @dimakuv, and @jkr0103)
config.yaml.template
line 10 at r10 (raw file):
Previously, adarshan-intel (Adarsh Anand) wrote…
When specifying a custom distribution name in config.yaml (not auto), it's crucial to use the full path
quay.io/centos/centos:stream9
.Merely using
centos:stream9
as distro in the config.yaml file will not function correctly.I attempted to modify the code so that entering
centos:stream9
would default to pulling fromquay.io/centos/centos:stream9
, but this did not work when building the base Gramine docker image.Therefore, this ensures users input the complete path. Although the process works correctly when 'auto' is entered as a distro as the logic is implemented to pull from quay.io
If specifying centos:stream9
doesn't work then we should just list quay.io/centos/centos:stream9
instead, not centos:stream9
.
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: 6 of 8 files reviewed, 3 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, @jkr0103, and @mkow)
config.yaml.template
line 10 at r10 (raw file):
Previously, mkow (Michał Kowalczyk) wrote…
If specifying
centos:stream9
doesn't work then we should just listquay.io/centos/centos:stream9
instead, notcentos:stream9
.
Done. 7e2438e
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 1 of 1 files at r12, 1 of 1 files at r13, all commit messages.
Reviewable status: all 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: Intel), "fixup! " found in commit messages' one-liners (waiting on @jkr0103 and @mkow)
gsc.py
line 241 at r10 (raw file):
Previously, adarshan-intel (Adarsh Anand) wrote…
Yes I mean this is the only thing pending here.
#207 -- it was merged, @adarshan-intel please rebase.
df76f12
to
c0b7b56
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: 5 of 8 files reviewed, 3 unresolved discussions, not enough approvals from maintainers (2 more required), not enough approvals from different teams (2 more required, approved so far: ) (waiting on @dimakuv, @jkr0103, and @mkow)
gsc.py
line 241 at r10 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
#207 -- it was merged, @adarshan-intel please rebase.
Rebased #206, please check
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 3 of 6 files at r7, 2 of 2 files at r9, 3 of 3 files at r14, all commit messages.
Reviewable status: all files reviewed, 2 unresolved discussions, not enough approvals from maintainers (2 more required), not enough approvals from different teams (2 more required, approved so far: ) (waiting on @adarshan-intel and @jkr0103)
gsc.py
line 248 at r14 (raw file):
def template_path(distro): if distro.startswith("quay.io/centos/centos"):
Why not ==
? What values are you expecting here?
Suggestion:
if distro.startswith("quay.io/centos/centos:"):
gsc.py
line 249 at r14 (raw file):
def template_path(distro): if distro.startswith("quay.io/centos/centos"): return "centos/stream"
ditto for the whole PR
Suggestion:
return 'centos/stream'
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, 2 unresolved discussions, not enough approvals from maintainers (2 more required), not enough approvals from different teams (2 more required, approved so far: ) (waiting on @jkr0103 and @mkow)
gsc.py
line 248 at r14 (raw file):
Previously, mkow (Michał Kowalczyk) wrote…
Why not
==
? What values are you expecting here?
Done.
gsc.py
line 249 at r14 (raw file):
Previously, mkow (Michał Kowalczyk) wrote…
ditto for the whole PR
Done.
Previously, adarshan-intel (Adarsh Anand) wrote…
The colon ( |
1f66728
to
b754f3b
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.
@dimakuv @mkow Can you review this again ?
Reviewable status: 7 of 8 files reviewed, 2 unresolved discussions, not enough approvals from maintainers (2 more required), not enough approvals from different teams (2 more required, approved so far: ) (waiting on @jkr0103 and @mkow)
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 1 of 1 files at r15, 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 (2 more required, approved so far: ) (waiting on @adarshan-intel and @jkr0103)
gsc.py
line 248 at r14 (raw file):
Previously, adarshan-intel (Adarsh Anand) wrote…
The colon (
:
) will not be used because in the functiontemplate_path
, we are splitting the distro based on:
infetch_and_validate_distro_support
. Therefore, using colon in thetemplate_path
function as you suggested would result in an error as distro becomes exactlyquay.io/centos/centos
Ok, right, but what about the rest of my comment?
Signed-off-by: adarshan-intel <adarsh.anand@intel.com>
b754f3b
to
89565e3
Compare
Previously, mkow (Michał Kowalczyk) wrote…
Done, made |
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 1 of 1 files at r16, all commit messages.
Reviewable status: all files reviewed, all discussions resolved, not enough approvals from maintainers (1 more required), not enough approvals from different teams (1 more required, approved so far: ITL) (waiting on @jkr0103)
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 3 of 6 files at r7, 2 of 2 files at r9, 2 of 3 files at r14, 1 of 1 files at r16, all commit messages.
Reviewable status:complete! all files reviewed, all discussions resolved (waiting on @jkr0103)
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 2 of 3 files at r14, 1 of 1 files at r16, all commit messages.
Reviewable status:complete! all files reviewed, all discussions resolved (waiting on @jkr0103)
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 3 of 6 files at r7, 2 of 2 files at r9, 2 of 3 files at r14, 1 of 1 files at r16, all commit messages.
Reviewable status:complete! all files reviewed, all discussions resolved
Description of the changes
Add support for CentOS Stream 9 using the image - quay.io/centos/centos:stream9 to GSC.
How to test this PR?
To test this PR, set follow similar procedure as followed with Ubuntu and do gsc build and signing commands.
This change is![Reviewable](https://camo.githubusercontent.com/23b05f5fb48215c989e92cc44cf6512512d083132bd3daf689867c8d9d386888/68747470733a2f2f72657669657761626c652e696f2f7265766965775f627574746f6e2e737667)