whereabouts IPAM package support for Antrea #2185

arunvelayutham · 2021-05-15T03:29:43Z

Whereabouts IPAM package support for Antrea

Required for CNF use case support.

arunvelayutham · 2021-05-15T03:35:55Z

build/images/base/build.sh

@@ -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 \


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

jianjuns · 2021-05-17T17:52:21Z

build/images/scripts/install_cni

@@ -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.


Not your code, but would you mind change "," to "." in your PR?

sure. took care.

jianjuns · 2021-05-17T17:52:42Z

build/images/scripts/install_cni

@@ -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


Nit: add a "." at end of line.

jianjuns · 2021-05-17T17:59:02Z

build/images/base/Dockerfile

@@ -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


I am thinking should we just include whereabouts in the cni-binaries. Let us see what @antoninbas thinks.

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

https://downloads.antrea.io/whereabouts/v0.4/whereabouts-linux-amd64

https://downloads.antrea.io/whereabouts/v0.4/whereabouts-linux-arm64

https://downloads.antrea.io/whereabouts/v0.4/whereabouts-linux-arm

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.

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.

antoninbas

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?

antoninbas · 2021-05-18T21:45:58Z

build/images/base/Dockerfile

+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


why do you install Go? you are using the golang image (FROM golang:1.13)

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.

antoninbas · 2021-05-18T21:46:58Z

build/images/base/Dockerfile

+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 


is there any difference with using the hack/build-go.sh as specified in the whereabouts README: https://github.com/k8snetworkplumbingwg/whereabouts#building

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.

antoninbas · 2021-05-18T21:47:53Z

build/images/base/Dockerfile

@@ -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


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

https://downloads.antrea.io/whereabouts/v0.4/whereabouts-linux-amd64

https://downloads.antrea.io/whereabouts/v0.4/whereabouts-linux-arm64

https://downloads.antrea.io/whereabouts/v0.4/whereabouts-linux-arm

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.

arunvelayutham · 2021-05-20T05:03:47Z

@antoninbas Docker Image size impact: ~38MB increase.

antoninbas

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-commenter · 2021-05-20T18:16:58Z

Codecov Report

Merging #2185 (a34b895) into main (3134f7e) will decrease coverage by 5.90%.
The diff coverage is n/a.

@@            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

Flag	Coverage Δ
kind-e2e-tests	`40.82% <ø> (-11.25%)`	⬇️
unit-tests	`41.10% <ø> (-0.28%)`	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
pkg/apis/controlplane/helper.go	`25.00% <0.00%> (-75.00%)`	⬇️
pkg/apis/controlplane/v1beta2/helper.go	`25.00% <0.00%> (-75.00%)`	⬇️
pkg/controller/networkpolicy/mutate.go	`0.00% <0.00%> (-71.77%)`	⬇️
pkg/controller/networkpolicy/store/group.go	`5.00% <0.00%> (-70.00%)`	⬇️
pkg/controller/networkpolicy/validate.go	`0.00% <0.00%> (-69.47%)`	⬇️
pkg/agent/controller/networkpolicy/packetin.go	`6.20% <0.00%> (-65.52%)`	⬇️
...formers/externalversions/security/v1alpha1/tier.go	`0.00% <0.00%> (-64.29%)`	⬇️
...ers/externalversions/core/v1alpha2/clustergroup.go	`0.00% <0.00%> (-64.29%)`	⬇️
...s/externalversions/core/v1alpha2/externalentity.go	`0.00% <0.00%> (-64.29%)`	⬇️
...xternalversions/security/v1alpha1/networkpolicy.go	`0.00% <0.00%> (-64.29%)`	⬇️
... and 78 more

jianjuns · 2021-05-20T21:58:13Z

@jianjuns are you ok with the new approach?

It looks good, if no concern to provide download of whereabouts binaries from Antrea downloads.

antoninbas · 2021-05-20T23:06:20Z

build/images/base/Dockerfile

-    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}


@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.

@antoninbas sorry for the delay. missed to see this comment. sure, I will push it shortly

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

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.

Signed-off-by: arunvelayutham <[email protected]>

antoninbas

LGTM

antoninbas · 2021-05-25T17:49:51Z

/skip-all

arunvelayutham commented May 15, 2021

View reviewed changes

arunvelayutham marked this pull request as draft May 15, 2021 03:42

jianjuns reviewed May 17, 2021

View reviewed changes

jianjuns requested a review from antoninbas May 17, 2021 17:59

antoninbas reviewed May 18, 2021

View reviewed changes

arunvelayutham force-pushed the antrea-whereabouts-support branch from 47d028c to e5e510d Compare May 20, 2021 04:38

antoninbas reviewed May 20, 2021

View reviewed changes

arunvelayutham marked this pull request as ready for review May 20, 2021 19:43

arunvelayutham force-pushed the antrea-whereabouts-support branch 2 times, most recently from 41842cf to 8b65380 Compare May 20, 2021 20:20

antoninbas reviewed May 20, 2021

View reviewed changes

arunvelayutham force-pushed the antrea-whereabouts-support branch from 8b65380 to 3facbb3 Compare May 24, 2021 21:25

Package whereabouts IPAM into Antrea docker image(CNF use case).

a34b895

Signed-off-by: arunvelayutham <[email protected]>

arunvelayutham force-pushed the antrea-whereabouts-support branch from 3facbb3 to a34b895 Compare May 25, 2021 16:57

antoninbas approved these changes May 25, 2021

View reviewed changes

antoninbas merged commit cddfccd into antrea-io:main May 25, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

whereabouts IPAM package support for Antrea #2185

whereabouts IPAM package support for Antrea #2185

arunvelayutham commented May 15, 2021

arunvelayutham May 15, 2021

jianjuns May 17, 2021

arunvelayutham May 20, 2021

jianjuns May 17, 2021

arunvelayutham May 20, 2021

jianjuns May 17, 2021

antoninbas May 18, 2021

arunvelayutham May 20, 2021

antoninbas left a comment

antoninbas May 18, 2021

arunvelayutham May 20, 2021

antoninbas May 18, 2021

arunvelayutham May 20, 2021

antoninbas May 18, 2021

arunvelayutham commented May 20, 2021

antoninbas left a comment

codecov-commenter commented May 20, 2021 •

edited

Loading

jianjuns commented May 20, 2021

antoninbas May 20, 2021

arunvelayutham May 24, 2021

antoninbas May 25, 2021

arunvelayutham May 25, 2021 •

edited

Loading

antoninbas left a comment

antoninbas commented May 25, 2021

		@@ -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.

whereabouts IPAM package support for Antrea #2185

whereabouts IPAM package support for Antrea #2185

Conversation

arunvelayutham commented May 15, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

antoninbas left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

arunvelayutham commented May 20, 2021

antoninbas left a comment

Choose a reason for hiding this comment

codecov-commenter commented May 20, 2021 • edited Loading

Codecov Report

jianjuns commented May 20, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

arunvelayutham May 25, 2021 • edited Loading

Choose a reason for hiding this comment

docker run d1c7228483ec ls -ltr /opt/cni/bin

antoninbas left a comment

Choose a reason for hiding this comment

antoninbas commented May 25, 2021

codecov-commenter commented May 20, 2021 •

edited

Loading

arunvelayutham May 25, 2021 •

edited

Loading