Fix and improve the docker image building process of Flow Aggregator #2016

dreamtalen · 2021-03-31T04:17:56Z

Fix the error of Flow Aggregator image after PR #2004 changed the base image to scratch.
Change the image name target "flow-aggregator-ubuntu" to "flow-aggregtor-image" in Makefile since ubuntu is not relevant.
Change the base image of Flow Aggregator code coverage image to ubuntu:20.04.

codecov-io · 2021-03-31T04:27:28Z

Codecov Report

Merging #2016 (d10d3c0) into main (08ea67c) will decrease coverage by 5.82%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##             main    #2016      +/-   ##
==========================================
- Coverage   65.12%   59.30%   -5.83%     
==========================================
  Files         197      197              
  Lines       17407    17507     +100     
==========================================
- Hits        11336    10382     -954     
- Misses       4883     6011    +1128     
+ Partials     1188     1114      -74

Flag	Coverage Δ
kind-e2e-tests	`44.59% <ø> (-11.57%)`	⬇️
unit-tests	`41.53% <ø> (-0.16%)`	⬇️

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.21%)`	⬇️
pkg/k8s/name.go	`33.33% <0.00%> (-66.67%)`	⬇️
pkg/agent/controller/networkpolicy/packetin.go	`9.93% <0.00%> (-59.01%)`	⬇️
pkg/apiserver/handlers/webhook/mutation_crd.go	`0.00% <0.00%> (-52.64%)`	⬇️
pkg/apiserver/handlers/webhook/validation_crd.go	`0.00% <0.00%> (-52.64%)`	⬇️
...ver/registry/controlplane/nodestatssummary/rest.go	`50.00% <0.00%> (-50.00%)`	⬇️
... and 43 more

srikartati

Thanks for fixing this.

srikartati · 2021-03-31T05:09:19Z

build/images/flow-aggregator/Dockerfile


 FROM scratch

 LABEL maintainer="Antrea <projectantrea-dev@googlegroups.com>"
 LABEL description="The docker image for the flow aggregator"

-USER root
+COPY --from=flow-aggregator-build /antrea/bin/flow-aggregator /
+COPY --from=flow-aggregator-build /new_tmp /tmp


What is the purpose of tmp?

It's needed for logs. Added a comment here.

sorry could you clarify? which logs?
The flow-aggregator should not emit logs to /tmp, everything should go to stdout / stderr

Sure, I was adding /tmp directory because got this error:

>>> kubectl logs flow-aggregator-78cc9f6c78-5jwpp -n flow-aggregator I0331 20:33:40.099906 1 log_file.go:99] Set log file max size to 104857600 I0331 20:33:40.099906 1 log_file.go:99] Set log file max size to 104857600 log: exiting because of error: log: cannot create log: open /tmp/flow-aggregator.flow-aggregator-78cc9f6c78-5jwpp.unknownuser.log.INFO.20210331-203340.1: no such file or directory

According to https://github.com/vmware-tanzu/antrea/blob/bb6cd1016e2d2863847ce13052b951e27a654b14/build/yamls/flow-aggregator/base/flow-aggregator.yml#L125, the logs should go to /var/log/flowaggregator, where does the /tmp come from?
The intent is to have this directory mounted from the host actually (see YAML for Antrea Controller) which is not currently the case for flow-aggregator.yml. This needs to be addressed as well, there is no need to create the directory in the container image (it will be created dynamically by the volume mount).
The unknownuser is not ideal, I think you can solve it by adding ENV USER root to the Dockerfile

Thanks, it seems the /tmp is used by klog. I solved it by adding --logtostderr to let logs go to stderr.

srikartati · 2021-03-31T05:20:07Z

Makefile

-.PHONY: flow-aggregator-ubuntu
-flow-aggregator-ubuntu:
+.PHONY: flow-aggregator-image
+flow-aggregator-image:
 	@echo "===> Building antrea/flow-aggregator Docker image <==="
 ifneq ($(NO_PULL),)
 	docker build -t antrea/flow-aggregator:$(DOCKER_IMG_VERSION) -f build/images/flow-aggregator/Dockerfile --build-arg OVS_VERSION=$(OVS_VERSION) .


OVS_VERSION can be removed. Same for code coverage image.

Thanks, addressed.

srikartati · 2021-03-31T05:22:30Z

build/images/flow-aggregator/Dockerfile

 FROM golang:1.15 as flow-aggregator-build

 WORKDIR /antrea

 COPY . /antrea

-RUN make flow-aggregator
+RUN CGO_ENABLED=0 make flow-aggregator


A comment would be informative here.

Thanks, addressed.

antoninbas · 2021-03-31T22:50:32Z

build/yamls/flow-aggregator/base/flow-aggregator.yml

-        - --logtostderr=false
-        - --log_dir=/var/log/flowaggregator
-        - --alsologtostderr
-        - --log_file_max_size=100
-        - --log_file_max_num=4
+        - --logtostderr
        - --v=0


This change may be orthogonal to this PR. What's your take on this @srikartati?
It's fine to keep logging to /var/log/flowaggregator as long we use a hostPath volume, unless there is a bug in klog an it tries to log to /tmp even when --log_dir is provided (which I would find surprising).

Yes, I think this change seems to be orthogonal.

https://github.com/vmware-tanzu/antrea/blob/main/cmd/flow-aggregator/main.go#L51
We expect log limits here. Not sure what default values or how does this function will react. @dreamtalen do you know what happens?

I quickly tried this patch by adding the missing hostPath volume. I did not get any error that logs are written to /tmp.

Here is my diff on top of this patch:

~/work/antrea_all/antrea$ git diff diff --git a/build/yamls/flow-aggregator/base/flow-aggregator.yml b/build/yamls/flow-aggregator/base/flow-aggregator.yml index 5dd33d93..7e7ac18d 100644 --- a/build/yamls/flow-aggregator/base/flow-aggregator.yml +++ b/build/yamls/flow-aggregator/base/flow-aggregator.yml @@ -124,7 +124,11 @@ spec: - args: - --config - /etc/flow-aggregator/flow-aggregator.conf - - --logtostderr + - --logtostderr=false + - --log_dir=/var/log/flow-aggregator + - --alsologtostderr + - --log_file_max_size=100 + - --log_file_max_num=4 - --v=0 name: flow-aggregator image: flow-aggregator @@ -135,9 +139,14 @@ spec: name: flow-aggregator-config readOnly: true subPath: flow-aggregator.conf + - mountPath: /var/log/flow-aggregator + name: host-var-log-flow-aggregator serviceAccountName: flow-aggregator volumes: - name: flow-aggregator-config configMap: name: flow-aggregator-configmap - + - name: host-var-log-flow-aggregator + hostPath: + path: /var/log/flow-aggregator + type: DirectoryOrCreate

Thanks for checking let's go with this

I think keeping the same behavior as antrea-controller for logging is better.
@antoninbas Do you know what is the benefit of logging this to a separate directory along with stderr? Where will the directory be used? I see that log collection in e2e tests is also using "kubectl logs.."

The idea was to have all the logs in one directory on the Node (/var/log/antrea). But it doesn't make too much sense for the Antrea Controller and the Flow Aggregator which are Deployments, and not DaemonSets. You can either match the Antrea Controller and we can revisit this decision later for both, or you can switch to only logging to stdout / stderr (remove file logging) for the Flow Aggregator (leave the Antrea Controller alone in this PR though). I don't have a strong preference. Maybe the second solution simplifies things and is the right way to go? Since we were not actually logging to the Node filesystem earlier (missing volume mount) there isn't any loss of functionality. What do you think?

Thanks, Antonin.
From Jianjun's PR, I see that it can be used for the log bundle. If that is the case, can we use /var/log/antrea instead of /var/log/flow-aggregator here as a solution? Earlier, the missing log functionality in the flow aggregator is overlooked. We can fix that here.

yes I think you can use /var/log/antrea

Thanks Srikar for helping me try adding the hostpath, updated PR with /var/log/antrea log path.

srikartati · 2021-04-01T17:25:56Z

/test-all
/test-ipv6-only-e2e
/test-ipv6-ds-e2e

srikartati · 2021-04-01T17:26:34Z

/test-ipv6-e2e

srikartati

LGTM

jianjuns · 2021-04-01T17:35:16Z

build/yamls/flow-aggregator.yml

      serviceAccountName: flow-aggregator
      volumes:
      - configMap:
          name: flow-aggregator-configmap-hf78268hm6
        name: flow-aggregator-config
+      - hostPath:
+          path: /var/log/antrea


Probably better to have a sub dir for flow aggregtor, as the same log dir can be used for other components too (agent/controller/ovs).

I have no strong preference here. The agent and controller logs are already in the same directory, and the component name is part of the log file name. I am fine with a sub directory though.

Yes, but today only agent/controller logs are in the top dir; OVS is not. I prefer a sub-dir for flow aggregator too.

Thanks, updated the log directory to /var/log/antrea/flow-aggregator

srikartati

Realized that code-coverage patch needs to be changed as well otherwise code-coverage manifestation will not work.
https://github.com/vmware-tanzu/antrea/blob/main/test/e2e/coverage/flow-aggregator-arg-file

needs more changes

Fix the error of Flow Aggregator image after PR antrea-io#2004 changed the base image to scratch. Change the image name target "flow-aggregator-ubuntu" to "flow-aggregtor-image" in Makefile since ubuntu is not relevant. Change the base image of Flow Aggregator code coverage image to ubuntu:20.04.

dreamtalen · 2021-04-01T20:45:47Z

Realized that code-coverage patch needs to be changed as well otherwise code-coverage manifestation will not work.
https://github.com/vmware-tanzu/antrea/blob/main/test/e2e/coverage/flow-aggregator-arg-file

Thanks, addressed

jianjuns

LGTM

antoninbas

LGTM

srikartati · 2021-04-01T20:57:32Z

/test-all
/test-ipv6-only-e2e
/test-ipv6-e2e

srikartati

LGTM

srikartati · 2021-04-01T21:58:18Z

/test-conformance
/test-networkpolicy

srikartati · 2021-04-01T23:35:32Z

Kind tests with Antrea network policies failed because e2e tests have timed out after 50 mins. This is unrelated to Flow Aggregator. All the other tests have passed. Merging it.

vmwclabot added the cla-not-required label Mar 31, 2021

srikartati reviewed Mar 31, 2021

View reviewed changes

srikartati added this to the Antrea v1.0 release milestone Mar 31, 2021

dreamtalen force-pushed the local-fix-dockerfile branch 2 times, most recently from 06a107f to e19bf9c Compare March 31, 2021 21:41

antoninbas reviewed Mar 31, 2021

View reviewed changes

dreamtalen force-pushed the local-fix-dockerfile branch from e19bf9c to d10d3c0 Compare April 1, 2021 06:23

srikartati previously approved these changes Apr 1, 2021

View reviewed changes

jianjuns reviewed Apr 1, 2021

View reviewed changes

srikartati reviewed Apr 1, 2021

View reviewed changes

dreamtalen force-pushed the local-fix-dockerfile branch from d10d3c0 to 8c9dcf0 Compare April 1, 2021 20:44

jianjuns reviewed Apr 1, 2021

View reviewed changes

antoninbas approved these changes Apr 1, 2021

View reviewed changes

srikartati approved these changes Apr 1, 2021

View reviewed changes

srikartati merged commit 117cd94 into antrea-io:main Apr 1, 2021

zyiou added area/flow-visibility Issues or PRs related to flow visibility support in Antrea area/flow-visibility/aggregator Issues or PRs related to Flow Aggregator labels Jun 9, 2021

Fix and improve the docker image building process of Flow Aggregator #2016

Fix and improve the docker image building process of Flow Aggregator #2016

Conversation

dreamtalen commented Mar 31, 2021

codecov-io commented Mar 31, 2021 • edited Loading

Codecov Report

srikartati 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

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

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

srikartati commented Apr 1, 2021

srikartati commented Apr 1, 2021

srikartati 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

srikartati left a comment

Choose a reason for hiding this comment

dreamtalen commented Apr 1, 2021

jianjuns left a comment

Choose a reason for hiding this comment

antoninbas left a comment

Choose a reason for hiding this comment

srikartati commented Apr 1, 2021

srikartati left a comment

Choose a reason for hiding this comment

srikartati commented Apr 1, 2021

srikartati commented Apr 1, 2021

codecov-io commented Mar 31, 2021 •

edited

Loading