-
Notifications
You must be signed in to change notification settings - Fork 364
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
Support antrea-agent UBI8 based image #3273
Conversation
Codecov Report
@@ Coverage Diff @@
## main #3273 +/- ##
===========================================
- Coverage 60.12% 41.84% -18.28%
===========================================
Files 331 199 -132
Lines 28444 24106 -4338
===========================================
- Hits 17102 10088 -7014
- Misses 9483 12981 +3498
+ Partials 1859 1037 -822
Flags with carried forward coverage won't be shown. Click here to find out more.
|
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.
Overall, LGTM
Can you check the Docker image size and make sure it is reasonable?
UBI8 image creation will be automated by the operator build and will not
be created automatically in the Antrea project scope.
Is there a reason for that? Given that all the build scripts are in this repo, IMO it would make sense to have a Github workflow here to build & push the Docker image.
build/images/Dockerfile.build.ubi
Outdated
USER root | ||
|
||
COPY build/images/scripts/* /usr/local/bin/ | ||
COPY --from=antrea-build /antrea/bin/* /usr/local/bin/ |
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.
missing new line at the end of file
build/images/base/build.sh
Outdated
if [ "$DISTRO" == "ubuntu" ]; then | ||
docker push antrea/base-ubuntu:$OVS_VERSION | ||
elif [ "$DISTRO" == "ubi" ]; then | ||
docker push antrea/base-ubi:$OVS_VERSION | ||
fi |
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.
maybe docker push antrea/base-$DISTRO:$OVS_VERSION
?
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.
Makes sense. Thanks.
build/images/ovs/Dockerfile.ubi
Outdated
# https://docs.openvswitch.org/en/latest/intro/install/fedora | ||
FROM centos:centos7 as ovs-rpms | ||
|
||
# Some patches may not apply cleanly if another version is provided. |
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.
Comment doesn't make much sense as it is since there is no default version provided in the Dockerfile.
See build/images/ovs/Dockerfile
for the "correct" comment
build/images/ovs/Dockerfile.ubi
Outdated
|
||
# Change Repository from UBI8’s to CentOS because UBI8's repository does not contain | ||
# enough packages required by OVS installation. | ||
# Use the RHEL official repository is the best choice but it's not for free. |
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.
# Use the RHEL official repository is the best choice but it's not for free. | |
# Using the official RHEL repository would be the best choice but it's not publicly accessible. |
build/images/ovs/Dockerfile.ubi
Outdated
COPY CentOS.repo /tmp/CentOS.repo | ||
RUN rm -f /etc/yum.repos.d/* && mv /tmp/CentOS.repo /etc/yum.repos.d/CentOS.repo && \ | ||
curl https://www.centos.org/keys/RPM-GPG-KEY-CentOS-Official -o /etc/pki/rpm-gpg/RPM-GPG-KEY-centosofficial && \ | ||
subscription-manager config --rhsm.manage_repos=0 && \ | ||
yum clean all -y && yum update -y && yum reinstall yum -y | ||
|
||
COPY charon-logging.conf /tmp | ||
COPY --from=ovs-rpms /tmp/ovs-rpms/* /tmp/ovs-rpms/ | ||
# TODO: update the strongSwan logging config. | ||
RUN yum install /tmp/ovs-rpms/* -y && yum install epel-release -y && \ | ||
yum install iptables logrotate strongswan -y && \ | ||
mv /etc/logrotate.d/openvswitch /etc/logrotate.d/openvswitch-switch && \ | ||
sed -i "/rotate /a\ #size 100M" /etc/logrotate.d/openvswitch-switch && \ | ||
rm -rf /tmp/* |
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.
my recommendation would be to ensure that there is a single RUN
command in the final Docker image to help keep the image small. Also, you should ensure that the last yum
instruction is a yum clean all
. I don't know if there are other good practices for REHL / CentOS Docker images when one wants to keep the size small.
03390b4
to
53ccd3b
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.
PR description is no longer accurate
UBI8 image creation will be automated by the operator build and will not
be created automatically in the Antrea project scope.
build/images/base/Dockerfile.ubi
Outdated
FROM antrea/openvswitch-ubi:${OVS_VERSION} | ||
|
||
LABEL maintainer="Antrea <[email protected]>" | ||
LABEL description="Takes care of building the Antrea binaries as part of building the image." |
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.
This description is invalid. If you could fix the one in build/images/base/Dockerfile as well it would be great.
|
||
USER root | ||
|
||
RUN yum install ipset jq -y && yum clean all |
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.
We recently started installing iptables-wrapper
in the ubuntu base image to improve portability on different Node operating systems: #3276. You probably want to do the same 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.
Seems like there's a support issue with RHEL:
RHEL/CentOS 7 ship iptables 1.4, which does not support nft mode. RHEL/CentOS 8 ship a hacked version of iptables 1.8 that only supports nft mode. Therefore, neither can be used as a basis for a portable iptables-using container image.
https://github.com/kubernetes-sigs/iptables-wrappers/
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.
I guess this also applies to CentOS 8 stream?
It should not really matter anyway if the expectation is that the Nodes will also run CentOS 8 and support nft mode.
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.
LGTM
@ksamoray you need to ensure all commits are signed, the DCO check is failing (or you can squash and sign the resulting commit) |
5666e08
to
1e40c0a
Compare
/skip-all |
Makefile
Outdated
.PHONY: antctl-linux | ||
antctl-linux: |
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.
This change seems to be causing all the CI e2e tests to fail:
2022-02-15T10:45:29.5022438Z === CONT TestAntctl/testAntctlVerboseMode/RootNonVerbose
2022-02-15T10:45:29.5022857Z antctl_test.go:227:
2022-02-15T10:45:29.5023226Z Error Trace: antctl_test.go:227
2022-02-15T10:45:29.5025323Z Error: Expected nil, but got: &errors.errorString{s:"Internal error occurred: error executing command in container: failed to exec in container: failed to start exec \"301733dceb0111cd36999c637ffcbf2be462a6766c1e5d962c7a4aea3271bc6b\": OCI runtime exec failed: exec failed: container_linux.go:380: starting container process caused: exec: \"antctl\": executable file not found in $PATH: unknown"}
2022-02-15T10:45:29.5026450Z Test: TestAntctl/testAntctlVerboseMode/RootNonVerbose
2022-02-15T10:45:29.5027461Z Messages: antctl stdout:
Not sure exactly why but I would guess it's a conflict with the following Makefile target:
ANTCTL_BINARIES := antctl-darwin antctl-linux antctl-windows
$(ANTCTL_BINARIES): antctl-%:
@GOOS=$* $(GO) build -o $(BINDIR)/$@ $(GOFLAGS) -ldflags '$(LDFLAGS)' antrea.io/antrea/cmd/antctl
@if [[ $@ != *windows ]]; then \
chmod 0755 $(BINDIR)/$@; \
else \
mv $(BINDIR)/$@ $(BINDIR)/[email protected]; \
fi
With your change we end up with 2 different targets named antctl-linux
. I assume the second one takes precedence in this case, but it produces a binary called antctl-linux
and not a binary called antctl
.
My recommendation would be to remove this target (previously antctl-ubuntu
) altogether. And in the Dockerfile you can rename antctl-linux
to antctl
.
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.
Makes sense, thanks
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.
The Dockerfile needs to be updated as well, or the failure is the same.
I think this:
RUN make antrea-agent antrea-controller antrea-cni antctl
Needs to become this:
RUN make antrea-agent antrea-controller antrea-cni antctl-linux
RUN cd bin && ln -s antctl-linux antctl
or
RUN make antrea-agent antrea-controller antrea-cni antctl-linux
RUN mv bin/antctl-linux bin/antctl
Add the required code to build an Antrea image based on Red Hat UBI (Universal Base Image), which is in used by Red Hat platforms. Signed-off-by: Kobi Samoray <[email protected]>
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.
LGTM
This reverts commit 43d41bb.
…" (antrea-io#3364)" This reverts commit f4e10a9.
Add the required code to build an Antrea image based on Red Hat UBI (Universal Base Image), which is in used by Red Hat platforms. Signed-off-by: Kobi Samoray <[email protected]>
…ea-io#3364) This reverts commit 43d41bb.
Add the required code to build an Antrea image based on Red Hat UBI
(Universal Base Image), which is in used by Red Hat platforms.
Signed-off-by: Kobi Samoray [email protected]