-
-
Notifications
You must be signed in to change notification settings - Fork 247
build: Containerfile for the main PCP container using supported builds #2240
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
base: main
Are you sure you want to change the base?
Conversation
Removes hard-coding of x86_64 platform in Dockerfiles.
8f732fc
to
77a418e
Compare
build/containers/pcp/Containerfile
Outdated
@@ -0,0 +1,42 @@ | |||
FROM quay.io/centos/centos:stream10-minimal | |||
|
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.
container-entrypoint
script is missing in the built container. We need to copy it into the container.
COPY build/containers/pcp/root /
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.
@kurik I'm not following this - the COPY line is there but later in the file (under comment ## Setup). What was the nature of the failure there?
build/containers/pcp/Containerfile
Outdated
FROM quay.io/centos/centos:stream10-minimal | ||
|
||
RUN microdnf install -y --setopt=install_weak_deps=0 --setopt=tsflags=nodocs \ | ||
pcp-zeroconf |
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.
gettext
package, which is used by the container-entrypoint
script is missing in the centos:stream10-minimal
image. We need to install it:
RUN microdnf install -y --setopt=install_weak_deps=0 --setopt=tsflags=nodocs \
pcp-zeroconf gettext
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.
Should this be gettext-envsubst?
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.
Yes, gettext-envsubst
is sufficient to pull, we do not need the whole gettext
.
build/containers/pcp/Containerfile
Outdated
@@ -0,0 +1,42 @@ | |||
FROM quay.io/centos/centos:stream10-minimal | |||
|
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.
UID/GID of the pcp
user may vary with every build of the container image. This might lead to the situation that archives created by an older container will not be readable/writeable by a newer container build. To avoid this, the solution used in the Dockerfile
was to have fixed UID/GID specified directly in the Dockefile/Containerfile. I would recommend to use the same approach here as well:
RUN groupadd -g 1001 -r pcp
RUN useradd -u 1001 -g pcp -r -d /var/lib/pcp -s /sbin/nologin -c "Performance Co-Pilot" pcp
This might lead to split of the microdnf install
to two parts as we need to have groupadd
and useradd
installed (package shadow-utils
) before these are used in the Dockerfile/Containerfile , while pcp-zeroconf
needs to have the UID/GID already created.
The final code might looks like this:
RUN microdnf install -y --setopt=install_weak_deps=0 --setopt=tsflags=nodocs \
shadow-utils
RUN groupadd -g 1001 -r pcp
RUN useradd -u 1001 -g pcp -r -d /var/lib/pcp -s /sbin/nologin -c "Performance Co-Pilot" pcp
RUN microdnf install -y --setopt=install_weak_deps=0 --setopt=tsflags=nodocs \
pcp-zeroconf
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.
Its a shame this isn't handled by our systemd sysusers.d configuration, and I worry that that newer mechanism conflicts with this use of user/group-add commands (whoever goes first wins?). A potential splitting up of rpm installation makes a bad situation worse IMO. AIUI, in OpenShift and k8s it would seem none of this matters anyway since the base UIDs are assigned dynamically. Ugh, we need more experimentation here I think.
systemd-udevd-control.socket systemd-udevd-kernel.socket | ||
|
||
ENV NAME="pcp-latest" | ||
ENV VERSION="7" |
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.
Currently the centos-stream-9
as well as centos-stream-10
contain PCPv6, so I expect this is a preparation for the upcoming PCPv7 release.
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.
Yep.
No description provided.