-
Notifications
You must be signed in to change notification settings - Fork 366
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
whereabouts IPAM package support for Antrea #2185
whereabouts IPAM package support for Antrea #2185
Conversation
build/images/base/build.sh
Outdated
@@ -113,15 +114,24 @@ docker build $PLATFORM_ARG --target cni-binaries \ | |||
--build-arg CNI_BINARIES_VERSION=$CNI_BINARIES_VERSION \ | |||
--build-arg OVS_VERSION=$OVS_VERSION . | |||
|
|||
docker build $PLATFORM_ARG --target whereabouts-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.
Docker pull/push changes not verified yet (TODO: Discuss with Antonin).
did local build for antrea/base-ubuntu:2.14.0 and made sure that the docker image gets packaged with whereabouts IPAM bin at /opt/cni/bin/ location.
docker run antrea/base-ubuntu:latest ls -ltr /opt/cni/bin
total 52364
-rwxr-xr-x 1 root root 4246019 Aug 26 2020 bandwidth
-rwxr-xr-x 1 root root 3979034 Aug 26 2020 portmap
-rwxr-xr-x 1 root root 3566204 Aug 26 2020 loopback
-rwxr-xr-x 1 root root 3745368 Aug 26 2020 host-local
-rwxr-xr-x 1 root root 38074904 May 14 23:23 whereabouts
build/images/scripts/install_cni
Outdated
@@ -27,6 +27,9 @@ install -m 755 /opt/cni/bin/portmap /host/opt/cni/bin/portmap | |||
# Install bandwidth CNI binary file, It is required to support traffic shaping. |
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.
Not your code, but would you mind change "," to "." in your 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.
sure. took care.
build/images/scripts/install_cni
Outdated
@@ -27,6 +27,9 @@ install -m 755 /opt/cni/bin/portmap /host/opt/cni/bin/portmap | |||
# Install bandwidth CNI binary file, It is required to support traffic shaping. | |||
install -m 755 /opt/cni/bin/bandwidth /host/opt/cni/bin/bandwidth | |||
|
|||
# Install whereabouts IPAM binary file. Required for global IPAM support specific to CNF use cases |
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.
Nit: add a "." at end of line.
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.
done.
build/images/base/Dockerfile
Outdated
@@ -21,6 +21,23 @@ RUN set -eux; \ | |||
mkdir -p /opt/cni/bin; \ | |||
wget -q -O - https://github.com/containernetworking/plugins/releases/download/$CNI_BINARIES_VERSION/cni-plugins-linux-${pluginsArch}-$CNI_BINARIES_VERSION.tgz | tar xz -C /opt/cni/bin $CNI_PLUGINS | |||
|
|||
FROM golang:1.13 as whereabouts-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.
I am thinking should we just include whereabouts in the cni-binaries. Let us see what @antoninbas thinks.
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.
Originally I preferred the current solution because we need to build whereabouts which require Go. However, adding an intermediate build step makes things more complicated as it impacts other scripts.
So thinking about this more, I think that:
- Arun should talk to the whereabouts folks to see if they plan to upload binaries for their releases moving forward. IMO this is the right thing to do...
- If this is indeed the plan, we can use these binaries I built locally as a stop gap solution
This way we avoid making the build chain more complex and we have an approach which is likely more in line with the correct long term solution. If we need to update these binaries because of a new whereabouts release, I can take care of it.
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, that makes it much simpler. I had updated Dockerfile accordingly and pushed the changes.
I will work with whereabouts community to add binaries as part of their release cadence.
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.
After thinking about it a bit, I believe we should use pre-built binaries as I explained in more details in one of the comments.
Arun, it seems that the email address you use to commit doesn't match any address associated with your Github account, you may want to fix that.
Do you know the impact on the Docker image size when including the new binary?
build/images/base/Dockerfile
Outdated
RUN wget -q -O - https://golang.org/dl/go$GO_VERSION.linux-amd64.tar.gz | tar xz -C /usr/local/ && \ | ||
export PATH="/usr/local/go/bin:$PATH" && \ | ||
mkdir -p "$GOPATH/src" "$GOPATH/bin" && chmod -R 777 "$GOPATH" | ||
|
||
ENV PATH $GOPATH/bin:/usr/local/go/bin/:$PATH |
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.
why do you install Go? you are using the golang image (FROM golang:1.13
)
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.
Go install was required in docker scope to build whereabouts source code. But, that is not required any more, as we had decided to handle with pre-compiled bins.
build/images/base/Dockerfile
Outdated
RUN mkdir -p /whereabouts; \ | ||
wget -q -O - https://github.com/k8snetworkplumbingwg/whereabouts/archive/refs/tags/v0.4.tar.gz | tar xz -C /whereabouts/; \ | ||
cd /whereabouts/*/. ; \ | ||
go build -o bin/whereabouts cmd/whereabouts.go |
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.
is there any difference with using the hack/build-go.sh
as specified in the whereabouts README: https://github.com/k8snetworkplumbingwg/whereabouts#building
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.
no. But may need bit of change from where we copy the exec. But, whereabouts source compile is removed, as we use pre-compiled bin.
build/images/base/Dockerfile
Outdated
@@ -21,6 +21,23 @@ RUN set -eux; \ | |||
mkdir -p /opt/cni/bin; \ | |||
wget -q -O - https://github.com/containernetworking/plugins/releases/download/$CNI_BINARIES_VERSION/cni-plugins-linux-${pluginsArch}-$CNI_BINARIES_VERSION.tgz | tar xz -C /opt/cni/bin $CNI_PLUGINS | |||
|
|||
FROM golang:1.13 as whereabouts-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.
Originally I preferred the current solution because we need to build whereabouts which require Go. However, adding an intermediate build step makes things more complicated as it impacts other scripts.
So thinking about this more, I think that:
- Arun should talk to the whereabouts folks to see if they plan to upload binaries for their releases moving forward. IMO this is the right thing to do...
- If this is indeed the plan, we can use these binaries I built locally as a stop gap solution
This way we avoid making the build chain more complex and we have an approach which is likely more in line with the correct long term solution. If we need to update these binaries because of a new whereabouts release, I can take care of it.
47d028c
to
e5e510d
Compare
@antoninbas Docker Image size impact: ~38MB increase. |
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, can you mark the PR as ready, it still shows as a "Draft"? Also, the commit message was still done through an email address not associated with your Github account...
@jianjuns are you ok with the new approach?
Codecov Report
@@ Coverage Diff @@
## main #2185 +/- ##
==========================================
- Coverage 61.26% 55.35% -5.91%
==========================================
Files 270 274 +4
Lines 20486 20673 +187
==========================================
- Hits 12550 11444 -1106
- Misses 6636 8007 +1371
+ Partials 1300 1222 -78
Flags with carried forward coverage won't be shown. Click here to find out more.
|
41842cf
to
8b65380
Compare
It looks good, if no concern to provide download of whereabouts binaries from Antrea downloads. |
build/images/base/Dockerfile
Outdated
wget -q -O - https://github.com/containernetworking/plugins/releases/download/$CNI_BINARIES_VERSION/cni-plugins-linux-${pluginsArch}-$CNI_BINARIES_VERSION.tgz | tar xz -C /opt/cni/bin $CNI_PLUGINS | ||
|
||
wget -q -O - https://github.com/containernetworking/plugins/releases/download/$CNI_BINARIES_VERSION/cni-plugins-linux-${pluginsArch}-$CNI_BINARIES_VERSION.tgz | tar xz -C /opt/cni/bin $CNI_PLUGINS; \ | ||
wget -q -O /opt/cni/bin/whereabouts https://downloads.antrea.io/whereabouts/v0.4/whereabouts-linux-${pluginsArch} |
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.
@arunvelayutham sorry for yet another change but could you download https://downloads.antrea.io/whereabouts/v0.4/whereabouts-linux-${pluginsArch}.tgz
instead and untar it? I wanted to include a copy of the Apache 2 License. Once you untar, the binary's name is simply whereabouts
.
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.
@antoninbas sorry for the delay. missed to see this comment. sure, I will push it shortly
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 latest version doesn't look right, can you test it?
The url is https://downloads.antrea.io/whereabouts/v0.4/whereabouts-linux-${pluginsArch}.tgz
: it's a tarball file that you need to untar in order to retrieve the whereabouts
binary included in it
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. I didn't mean to push that change. accidently did that while rebase. I have the correct one pushed back. please chcek.
docker run d1c7228483ec ls -ltr /opt/cni/bin
total 50652
-rwxr-xr-x 1 root root 4246019 Aug 26 2020 bandwidth
-rwxr-xr-x 1 root root 3979034 Aug 26 2020 portmap
-rwxr-xr-x 1 root root 3566204 Aug 26 2020 loopback
-rwxr-xr-x 1 root root 3745368 Aug 26 2020 host-local
-rwxr-xr-x 1 502 staff 36322924 May 18 21:58 whereabouts
I believe you are using the same bin as built last week.
8b65380
to
3facbb3
Compare
Signed-off-by: arunvelayutham <arunkumar.velauytham@intel.com>
3facbb3
to
a34b895
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.
LGTM
/skip-all |
Whereabouts IPAM package support for Antrea