Skip to content
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

[Flow-Aggregator] Implementation of flow aggregator build system and structure #1558

Merged

Conversation

dreamtalen
Copy link
Contributor

@dreamtalen dreamtalen commented Nov 13, 2020

This is the first PR of flow aggregator implementation, which only contains the build system and structure of it, including the yaml files, github workflow changes and command line tools. For the main logic of flow aggregator, please take a look at this PR (#1596).

@antrea-bot
Copy link
Collaborator

Thanks for your PR.
Unit tests and code linters are run automatically every time the PR is updated.
E2e, conformance and network policy tests can only be triggered by a member of the vmware-tanzu organization. Regular contributors to the project should join the org.

The following commands are available:

  • /test-e2e: to trigger e2e tests.
  • /skip-e2e: to skip e2e tests.
  • /test-conformance: to trigger conformance tests.
  • /skip-conformance: to skip conformance tests.
  • /test-all-features-conformance: to trigger conformance tests with all alpha features enabled.
  • /skip-all-features-conformance: to skip conformance tests with all alpha features enabled.
  • /test-whole-conformance: to trigger all conformance tests on linux.
  • /skip-whole-conformance: to skip all conformance tests on linux.
  • /test-networkpolicy: to trigger networkpolicy tests.
  • /skip-networkpolicy: to skip networkpolicy tests.
  • /test-windows-conformance: to trigger windows conformance tests.
  • /skip-windows-conformance: to skip windows conformance tests.
  • /test-windows-networkpolicy: to trigger windows networkpolicy tests.
  • /skip-windows-networkpolicy: to skip windows networkpolicy tests.
  • /test-hw-offload: to trigger ovs hardware offload test.
  • /skip-hw-offload: to skip ovs hardware offload test.
  • /test-all: to trigger all tests (except whole conformance).
  • /skip-all: to skip all tests (except whole conformance).

Copy link
Member

@srikartati srikartati left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the PR @dreamtalen
We should probably change the name of featureGate in Antrea agent as FlowVisbilty instead of FlowExporter to incorporate the flow-aggregator implementation in the same feature gate. What do you think?


USER root

COPY bin/flow-aggregator /usr/local/bin/
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we expect the consumer of this dockerfile to compile the flow-aggregator code as a pre-requisite?
Maybe it is better to include the compilation steps for flow-aggregator binary in the dockerfile itself as it is being done for Antrea image.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, added.

command:
- flow-aggregator
name: flow-aggregator
image: dreamtalen/flow-aggregator
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are you planning to use Antrea org in dockerhub registry to maintain the image?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, changed the image name to antrea/flow-aggregator

package main

import (
aggregator "github.com/vmware-tanzu/antrea/pkg/flowaggregator"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please use the convention followed in Antrea.
Can you request Antonin to enable the github workflows (golang linters, spell check, unit test etc.) for feature/flow-aggregator branch?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, done.


"github.com/spf13/cobra"
"github.com/vmware-tanzu/antrea/pkg/log"
"github.com/vmware-tanzu/antrea/pkg/version"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

here too and same everywhere.

}

// Convert the string input in net.Addr format
hostPortAddr := strSlice[0] + ":" + strSlice[1]
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Better to take care of flow collector having an IPv6 address like it is done here.
#1541
This PR will be checked in soon.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, I'm thinking about taking care it in next PR.

case <-stopCh:
break
case <-exportTicker.C:
curMsgs := cp.GetMessages()
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

could you move to go-ipfix v0.3.0 and use the message channel in collector instead?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure, this PR is based on a commit of go-ipfix before intermediate process merged. In the second PR which I'm working on now, I have moved to go-ipfix v0.3.0 and some other comments will be addressed in that PR too.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok. Will wait for other PR, but please try not to make PRs bulky. It may be hard for code-reviewers to review.

func (fae *flowAggregator) Run(stopCh <-chan struct{}) {
// Go-ipfix TCP collector doesn't work during my last test, fixed as udp temporarily.
collectAddr, _ := net.ResolveUDPAddr("udp", "0.0.0.0:4739")
cp, _ := collector.InitCollectingProcess(collectAddr, 1024, 0)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

how is the flow-aggregator collector address fed to the flow exporter at antrea-agent?

collectAddr, _ := net.ResolveUDPAddr("udp", "0.0.0.0:4739")
cp, _ := collector.InitCollectingProcess(collectAddr, 1024, 0)

ep, err := exporter.InitExportingProcess(fae.flowCollectorAddr, 1, 0)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what if exporting process loose connection to flow collector (provided in configMap) or flow collector is restarted?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+1 - can be in a future PR but we should handle these cases, just like the Agent should be robust to connection loss to the collector or aggregator

I believe this should be done in the go-ipfix library though, just like the gRPC library handles it behind the scene (https://godoc.org/google.golang.org/grpc).

# Provide the flow collector address as string with format <IP>:<port>[:<proto>], where proto is tcp or udp.
# If no L4 transport proto is given, we consider tcp as default.
# Defaults to "".
flowCollectorAddr: "10.96.28.149:4739:udp"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You should use default here and not your testing collector address.

setSent.AddRecord(record.GetInfoElements(), record.GetTemplateID())
}
bytesSent, err := ep.AddSetAndSendMsg(setType, setSent)
klog.Infof("Msg index: %d, sent bytes: %d, error: %v\n", i, bytesSent, err)
Copy link
Member

@srikartati srikartati Nov 15, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Increase the log level 4 or an appropriate level for debug messages and use error log if err is not nil.

@codecov-io
Copy link

codecov-io commented Nov 19, 2020

Codecov Report

❗ No coverage uploaded for pull request base (feature/flow-aggregator@08d1062). Click here to learn what that means.
The diff coverage is n/a.

Impacted file tree graph

@@                    Coverage Diff                     @@
##             feature/flow-aggregator    #1558   +/-   ##
==========================================================
  Coverage                           ?   61.92%           
==========================================================
  Files                              ?      181           
  Lines                              ?    15310           
  Branches                           ?        0           
==========================================================
  Hits                               ?     9480           
  Misses                             ?     4813           
  Partials                           ?     1017           
Flag Coverage Δ
kind-e2e-tests 52.20% <0.00%> (?)
unit-tests 40.39% <0.00%> (?)

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

Copy link
Member

@srikartati srikartati left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see the version of go-ipfix is v0.2.1. You should at least move to v0.2.4 which is on master. May be rebasing the feature/flow-aggregator with master helps.

selector:
app: flow-aggregator
type: NodePort
ports:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we should also expose TCP port. Using name field can be useful.

DOCKER_PASSWORD: ${{ secrets.DOCKER_PASSWORD }}
run: |
echo "$DOCKER_PASSWORD" | docker login -u "$DOCKER_USERNAME" --password-stdin
docker push antrea/flow-aggregator:latest
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

add a new line

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

seems like this wasn't addressed? same for build_tag.yml


USER root

COPY --from=flowaggregator-build /antrea/bin/flow-aggregator /usr/local/bin/
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

add a new line


RUN make flow-aggregator

FROM antrea/base-ubuntu:2.14.0
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

add metadata (see other Dockerfiles in the repo for examples)

@@ -0,0 +1,13 @@
FROM golang:1.15 as flowaggregator-build
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

maybe the directory should be called flow-aggregator to match the image name

Makefile Show resolved Hide resolved
.github/workflows/build.yml Show resolved Hide resolved
@@ -0,0 +1,80 @@
apiVersion: v1
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This manifest needs to be integrated with hack/generate-manifest.sh.

In particular generate-manifest.sh should support generating a release version of the manifest for which the docker image tag is the correct release tag. This manifest should be automatically uploaded to release assets when a Github release is created (so you need to edit upload_release_assets.yml).

Make sure that you double check everything and that you test hack/generate-manifest.sh locally, otherwise it breaks the release flow.

- name: flow-aggregator-config
configMap:
name: flow-aggregator-configmap

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

there is an extra empty line here

@antoninbas
Copy link
Contributor

The manifest needs to be integrated with hack/generate-manifest & kustomize. We need to have a dev version of the manifest and a release version of the manifest.

Changes to upload_release_assets.yml and build_tag.yml can be postponed to a future PR. It's important to get them right since these workflows are not exercised until we make an actual release.

// records to the flow collector.
// Flow export frequency should be greater than or equal to 1s(one second).
// Defaults to "60s". Follow the time units of duration.
FlowExportFrequency string `yaml:"flowExportFrequency,omitempty"`
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

FlowExportInterval is a better name here. Frequency is usually the inverse of time.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, addressed.

@dreamtalen dreamtalen changed the title Flow aggregator implementation with only collector and exporter [Flow Aggregator] Flow aggregator implementation with only collector and exporter Nov 30, 2020
@dreamtalen dreamtalen changed the title [Flow Aggregator] Flow aggregator implementation with only collector and exporter [Flow-Aggregator] Flow aggregator implementation with only collector and exporter Nov 30, 2020
@dreamtalen
Copy link
Contributor Author

The manifest needs to be integrated with hack/generate-manifest & kustomize. We need to have a dev version of the manifest and a release version of the manifest.

Changes to upload_release_assets.yml and build_tag.yml can be postponed to a future PR. It's important to get them right since these workflows are not exercised until we make an actual release.

Thanks, just addressed these comments.

app: flow-aggregator
namespace: flow-aggregator
apiVersion: kustomize.config.k8s.io/v1beta1
kind: Kustomization
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Better to have an extra empty line here.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, done.

# Flow export interval should be greater than or equal to 1s (one second).
# Valid time units are "ns", "us" (or "µs"), "ms", "s", "m", "h".
# Defaults to "60s".
flowExportInterval: 60s
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ditto.

@@ -4,10 +4,12 @@ on:
branches:
- master
- release-*
- feature/flow-aggregator
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

do we need go license workflow for feature branch?

Copy link
Contributor Author

@dreamtalen dreamtalen Dec 1, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, removed feature/flow-aggregator in go license and kind update workflow since they are not essential.


USER root

COPY --from=flow-aggregator-build /antrea/bin/flow-aggregator /usr/local/bin/
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

new line.

@dreamtalen dreamtalen force-pushed the local-flow-aggregator branch 2 times, most recently from 43dc23a to a188844 Compare December 2, 2020 00:39
Copy link
Member

@srikartati srikartati left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please look at the workflow failures corresponding to linters, compiling the binary and unit tests.

build/yamls/flow-aggregator.yml Show resolved Hide resolved
Copy link
Contributor

@antoninbas antoninbas left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

some comments, but I'm satisfied with the Github workflow changes and build system changes

.github/workflows/build.yml Outdated Show resolved Hide resolved
Comment on lines 3 to 4
LABEL maintainer="Antrea <[email protected]>"
LABEL description="The docker image for flow aggregator"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

these label statements should come after the last FROM statement if I recall correctly?

@@ -0,0 +1 @@
# placeholder
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe the convention is an empty file called .gitkeep

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok, I was following the style in build/yamls/octant/ or windows/, could you please check again?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I added the .gitignore file before I became aware of the .gitkeep "convention", but that's a pretty minor point anyway. Feel free to update all of them to .gitkeep as part of this PR or keep is as it is.

Comment on lines +22 to +27
// Provide flow export interval as a duration string. This determines how often the flow aggregator exports flow
// records to the flow collector.
// Flow export interval should be greater than or equal to 1s (one second).
// Valid time units are "ns", "us" (or "µs"), "ms", "s", "m", "h".
// Defaults to "60s".
FlowExportInterval string `yaml:"flowExportInterval,omitempty"`
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Probably a bit out of scope of this PR, but IIRC Rob Cowart recommended that we spread the export over the time interval (both in the agent and in the aggregator), to avoid all records arriving at the same time

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes using different export intervals for active and inactive flows need a proposal. We need to add the notion of inactive flows in the exported IPFIX fields.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this becomes especially important at the aggregator, which will generate more traffic than each individual agent presumably

}

_usage="Usage: $0 [--mode (dev|release)] [--keep] [--help|-h]
Generate a YAML manifest to run Antrea on Windows Nodes, using Kustomize, and print it to stdout.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Update the comment

defer exportTicker.Stop()
sentMsgNum := 0
go cp.Start()
for {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

so all the logic necessary to store connection state and reconcile flows from source and destination Nodes will go here in a future PR?

defer exportTicker.Stop()
sentMsgNum := 0
go cp.Start()
for {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

you may want to review whether all the log messages in this loop are necessary or have the correct verbosity?

func (fae *flowAggregator) Run(stopCh <-chan struct{}) {
// Go-ipfix TCP collector doesn't work during my last test, fixed as udp temporarily.
collectAddr, _ := net.ResolveUDPAddr("udp", "0.0.0.0:4739")
cp, _ := collector.InitCollectingProcess(collectAddr, 1024, 0)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why is the error ignored?

collectAddr, _ := net.ResolveUDPAddr("udp", "0.0.0.0:4739")
cp, _ := collector.InitCollectingProcess(collectAddr, 1024, 0)

ep, err := exporter.InitExportingProcess(fae.flowCollectorAddr, 1, 0)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+1 - can be in a future PR but we should handle these cases, just like the Agent should be robust to connection loss to the collector or aggregator

I believe this should be done in the go-ipfix library though, just like the gRPC library handles it behind the scene (https://godoc.org/google.golang.org/grpc).

collectAddr, _ := net.ResolveUDPAddr("udp", "0.0.0.0:4739")
cp, _ := collector.InitCollectingProcess(collectAddr, 1024, 0)

ep, err := exporter.InitExportingProcess(fae.flowCollectorAddr, 1, 0)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is there some symmetry in the go-ipfix implementation for the agent -> aggregator connection and the aggregator -> collector connection? @srikartati

Copy link
Member

@srikartati srikartati Dec 4, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@antoninbas Yes, both use the same exporting process from go-ipfix library, if you meant from the perspective of library functions used. Exporter code in Antrea for agent-aggregator and aggregator-collector will look similar, but the fields will be different. In the future, we could try to consolidate.

@dreamtalen
Copy link
Contributor Author

some comments, but I'm satisfied with the Github workflow changes and build system changes

Antonin, thanks for your comments. The logic in pkg/flowaggregator/flowaggregator.go has changed a lot since the go-ipfix version moved from v0.2.1 -> v0.3.1, so please check the second PR (#1596) about the main logic of flow aggregator. Other comments will be addressed soon.

Copy link
Member

@srikartati srikartati left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@antoninbas Yes there is dependency with go-ipfix versions. That is why there are some errors in workflows too.

@dreamtalen Actually, could you remove this file from the PR entirely and check-in as part of PR #1596? Or keep some minimal code.

collectAddr, _ := net.ResolveUDPAddr("udp", "0.0.0.0:4739")
cp, _ := collector.InitCollectingProcess(collectAddr, 1024, 0)

ep, err := exporter.InitExportingProcess(fae.flowCollectorAddr, 1, 0)
Copy link
Member

@srikartati srikartati Dec 4, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@antoninbas Yes, both use the same exporting process from go-ipfix library, if you meant from the perspective of library functions used. Exporter code in Antrea for agent-aggregator and aggregator-collector will look similar, but the fields will be different. In the future, we could try to consolidate.

build/yamls/flow-aggregator.yml Show resolved Hide resolved
@dreamtalen
Copy link
Contributor Author

@antoninbas Yes there is dependency with go-ipfix versions. That is why there are some errors in workflows too.

@dreamtalen Actually, could you remove this file from the PR entirely and check-in as part of PR #1596? Or keep some minimal code.

Thanks, I removed most logic in pkg/flowaggregator/flowaggregator.go and will update the description of this commit.

Copy link
Member

@srikartati srikartati left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM except for one comment on building the flow-aggregator image through make.

@@ -306,6 +312,16 @@ octant-antrea-ubuntu:
docker build --pull -t antrea/octant-antrea-ubuntu:$(DOCKER_IMG_VERSION) -f build/images/Dockerfile.octant.ubuntu .
docker tag antrea/octant-antrea-ubuntu:$(DOCKER_IMG_VERSION) antrea/octant-antrea-ubuntu

.PHONY: flow-aggregator-ubuntu
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should we add this target along with build-ubuntu to get executed when we do make? Or expect folks to build image by using the target directly? @antoninbas what do you think?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we can keep it as it is for now and make the change in the future if we feel like we need to

@dreamtalen dreamtalen changed the title [Flow-Aggregator] Flow aggregator implementation with only collector and exporter [Flow-Aggregator] Implementation of flow aggregator build system and structure Dec 8, 2020
@srikartati
Copy link
Member

/test-all

Copy link
Contributor

@antoninbas antoninbas left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

some small comments, otherwise LGTM

main comment on my side is that I don't think the flow aggregator needs to be a NodePort Service?

DOCKER_PASSWORD: ${{ secrets.DOCKER_PASSWORD }}
run: |
echo "$DOCKER_PASSWORD" | docker login -u "$DOCKER_USERNAME" --password-stdin
docker push antrea/flow-aggregator:latest
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

seems like this wasn't addressed? same for build_tag.yml

@@ -306,6 +312,16 @@ octant-antrea-ubuntu:
docker build --pull -t antrea/octant-antrea-ubuntu:$(DOCKER_IMG_VERSION) -f build/images/Dockerfile.octant.ubuntu .
docker tag antrea/octant-antrea-ubuntu:$(DOCKER_IMG_VERSION) antrea/octant-antrea-ubuntu

.PHONY: flow-aggregator-ubuntu
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we can keep it as it is for now and make the change in the future if we feel like we need to

FROM antrea/base-ubuntu:2.14.0

LABEL maintainer="Antrea <[email protected]>"
LABEL description="The docker image for flow aggregator"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

for the flow aggregator

spec:
selector:
app: flow-aggregator
type: NodePort
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

maybe I am missing something, but why is this a NodePort Service? a ClusterIP is enough for the agent to connect to the aggregator

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, I have removed the NodePort type declaration in the second PR, sorry for not merging it into this PR.

@@ -0,0 +1,11 @@
# Provide the flow collector address as string with format <IP>:<port>[:<proto>], where proto is tcp or udp.
# If no L4 transport proto is given, we consider tcp as default.
# Defaults to "".
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: don't think this line is needed

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry, do you mean these description lines are not needed for parameters in flow-aggregator.conf?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes, I don't believe we typically include them in the .conf file, since the default value is obvious from the commented-out line

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Got it, thanks.

# records to the flow collector.
# Flow export interval should be greater than or equal to 1s (one second).
# Valid time units are "ns", "us" (or "µs"), "ms", "s", "m", "h".
# Defaults to "60s".
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ditto

@@ -0,0 +1 @@
# placeholder
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I added the .gitignore file before I became aware of the .gitkeep "convention", but that's a pretty minor point anyway. Feel free to update all of them to .gitkeep as part of this PR or keep is as it is.

Comment on lines +22 to +27
// Provide flow export interval as a duration string. This determines how often the flow aggregator exports flow
// records to the flow collector.
// Flow export interval should be greater than or equal to 1s (one second).
// Valid time units are "ns", "us" (or "µs"), "ms", "s", "m", "h".
// Defaults to "60s".
FlowExportInterval string `yaml:"flowExportInterval,omitempty"`
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this becomes especially important at the aggregator, which will generate more traffic than each individual agent presumably

}
}

func (fae *flowAggregator) Run(stopCh <-chan struct{}) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: why fae instead of fa?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changed to fa, will also update the variable name in subsequent PRs.

}
if o.config.FlowCollectorAddr == "" {
return fmt.Errorf("IPFIX flow collector address should be provided")
} else {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if the address parsing logic can be unified with the agent, please consider doing it in a subsequent PR

@dreamtalen dreamtalen force-pushed the local-flow-aggregator branch 2 times, most recently from 1cb3a07 to 80e1ad7 Compare December 10, 2020 17:37
Copy link
Member

@srikartati srikartati left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM.
Seems like all comments are addressed. Requesting re-review of @antoninbas

@dreamtalen dreamtalen force-pushed the local-flow-aggregator branch 2 times, most recently from 7c5d6f0 to 8dce57a Compare December 10, 2020 18:54
This is the first PR of flow aggregator implementation, which only
contains the build system and main structure of it, including the yaml
files, github workflow changes and command linue tools.
Copy link
Contributor

@antoninbas antoninbas left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@srikartati srikartati merged commit 269eb5e into antrea-io:feature/flow-aggregator Dec 10, 2020
srikartati pushed a commit to srikartati/antrea that referenced this pull request Dec 10, 2020
…io#1558)

This is the first PR of flow aggregator implementation, which only
contains the build system and main structure of it, including the yaml
files, github workflow changes and command linue tools.
srikartati pushed a commit to srikartati/antrea that referenced this pull request Dec 10, 2020
…io#1558)

This is the first PR of flow aggregator implementation, which only
contains the build system and main structure of it, including the yaml
files, github workflow changes and command linue tools.
srikartati pushed a commit that referenced this pull request Dec 10, 2020
This is the first PR of flow aggregator implementation, which only
contains the build system and main structure of it, including the yaml
files, github workflow changes and command linue tools.
antoninbas pushed a commit that referenced this pull request Dec 11, 2020
This is the first PR of flow aggregator implementation, which only
contains the build system and main structure of it, including the yaml
files, github workflow changes and command linue tools.
antoninbas pushed a commit that referenced this pull request Dec 11, 2020
This is the first PR of flow aggregator implementation, which only
contains the build system and main structure of it, including the yaml
files, github workflow changes and command linue tools.
srikartati pushed a commit to srikartati/antrea that referenced this pull request Dec 17, 2020
…io#1558)

This is the first PR of flow aggregator implementation, which only
contains the build system and main structure of it, including the yaml
files, github workflow changes and command linue tools.
zyiou pushed a commit to zyiou/antrea that referenced this pull request Dec 17, 2020
…io#1558)

This is the first PR of flow aggregator implementation, which only
contains the build system and main structure of it, including the yaml
files, github workflow changes and command linue tools.
zyiou pushed a commit to zyiou/antrea that referenced this pull request Dec 17, 2020
srikartati pushed a commit to srikartati/antrea that referenced this pull request Dec 19, 2020
…io#1558)

This is the first PR of flow aggregator implementation, which only
contains the build system and main structure of it, including the yaml
files, github workflow changes and command linue tools.
srikartati pushed a commit to srikartati/antrea that referenced this pull request Dec 21, 2020
…io#1558)

This is the first PR of flow aggregator implementation, which only
contains the build system and main structure of it, including the yaml
files, github workflow changes and command linue tools.
srikartati pushed a commit to srikartati/antrea that referenced this pull request Dec 21, 2020
…io#1558)

This is the first PR of flow aggregator implementation, which only
contains the build system and main structure of it, including the yaml
files, github workflow changes and command linue tools.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants