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.

Sign up for GitHub

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

Jump to bottom

Update generate-manifest-flow-aggregator.sh with new config #3526

Merged
merged 1 commit into from
Mar 28, 2022
Merged

Update generate-manifest-flow-aggregator.sh with new config #3526

merged 1 commit into from
Mar 28, 2022

Conversation

dreamtalen
Copy link
Contributor

In this commit, we updated the hack/generate-manifest-flow-aggregator.sh
with the new configuration of Flow Aggregator introduced in PR #3196 to
support both FlowCollector and Clickhouse sink.

Signed-off-by: Yongming Ding dyongming@vmware.com

@dreamtalen dreamtalen 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 Mar 25, 2022
@dreamtalen dreamtalen added this to the Antrea v1.6 release milestone Mar 25, 2022
heanlan
heanlan reviewed Mar 25, 2022
View reviewed changes
hack/generate-manifest-flow-aggregator.sh Outdated
sed -i.bak -E "s/^[[:space:]]*#[[:space:]]*address[[:space:]]*:[[:space:]]\"\"+[[:space:]]*$/ address: \"$FLOW_COLLECTOR\"/" flow-aggregator.conf
fi

if [[ $CLICKHOUSE != "" ]]; then
Copy link
Contributor

Choose a reason for hiding this comment

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

"if $CLICKHOUSE"?

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 force-pushed the fa-manifest-sh branch 2 times, most recently from b4f9cfb to a3c7374 Compare March 25, 2022 22:02
antoninbas
antoninbas reviewed Mar 25, 2022
View reviewed changes
hack/generate-manifest-flow-aggregator.sh
Comment on lines +147 to +152
perl -i -p0e 's/ # Enable is the switch to enable exporting flow records to external flow collector.\n #enable: false/ # Enable is the switch to enable exporting flow records to external flow collector.\n enable: true/' flow-aggregator.conf
sed -i.bak -E "s/^[[:space:]]*#[[:space:]]*address[[:space:]]*:[[:space:]]\"\"+[[:space:]]*$/ address: \"$FLOW_COLLECTOR\"/" flow-aggregator.conf
fi

if $CLICKHOUSE; then
perl -i -p0e 's/ # Enable is the switch to enable exporting flow records to ClickHouse.\n #enable: false/ # Enable is the switch to enable exporting flow records to ClickHouse.\n enable: true/' 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.

I hope the perl dependency will not be an issue. But I understand that multiline sed is not practical. We can improve later if 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.

Got it, thanks!

Copy link
Contributor

Choose a reason for hiding this comment

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

@dreamtalen Can you open a github issue for getting rid of the perl dependency? Just to make sure we will remember to do that before it becomes a problem!

hack/generate-manifest-flow-aggregator.sh Outdated
This option will work only with 'dev' mode.
--help, -h Print this message and exit
--mode (dev|release) Choose the configuration variant that you need (default is 'dev')
--flow-collector, -fc Flow collector is the flowCollector address configMap parameter
Copy link
Contributor

Choose a reason for hiding this comment

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

not introduced by this PR, but this should be --flow-collector, -fc <addr> to indicate that an arg is required (same for summary line above)

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 for pointing out, addressed.

antoninbas
antoninbas previously approved these changes Mar 26, 2022
View reviewed changes
@antoninbas
Copy link
Contributor

@dreamtalen what's the Jenkins job to run for this, could you start it?

@dreamtalen
Copy link
Contributor Author

dreamtalen commented Mar 26, 2022 •
edited

@dreamtalen what's the Jenkins job to run for this, could you start it?

Hi @antoninbas, currently only the daily-flow-visibility-validate Jenkins job will use the [-ch|--clickhouse] argument of generate-manifest-flow-aggregator.sh. So we have no Jenkins jobs that have to be triggered for this change.

Not for this PR, I noticed forgetting to add the description inside Jenkins readme.md of the daily-flow-visibility-validate Jenkins job in the last commit. Included that in this PR too.

jianjuns
jianjuns reviewed Mar 28, 2022
View reviewed changes
hack/generate-manifest-flow-aggregator.sh Outdated
--help, -h Print this message and exit
--mode (dev|release) Choose the configuration variant that you need (default is 'dev')
--flow-collector, -fc <addr> Flow collector is the flowCollector address configMap parameter
It should be given in format IP:port:proto. Example: 192.168.1.100:4739:udp
Copy link
Contributor

Choose a reason for hiding this comment

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

Add "." at the end of the sentence.

hack/generate-manifest-flow-aggregator.sh Outdated
This option will work only with 'dev' mode.
--help, -h Print this message and exit
--mode (dev|release) Choose the configuration variant that you need (default is 'dev')
--flow-collector, -fc <addr> Flow collector is the flowCollector address configMap parameter
Copy link
Contributor

Choose a reason for hiding this comment

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

Change to: "Specify the flowCollector address."?

Add "." at the end of the sentence.

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 Jianjun for the reviews. Addressed.

@dreamtalen
Update generate-manifest-flow-aggregator.sh with new config
77b396e 
In this commit, we updated the hack/generate-manifest-flow-aggregator.sh
with new configuration of Flow Aggregator to support both FlowCollector
and Clickhouse sink.

Signed-off-by: Yongming Ding <dyongming@vmware.com>
salv-orlando
salv-orlando approved these changes Mar 28, 2022
View reviewed changes
Copy link
Contributor

@salv-orlando salv-orlando left a comment

Choose a reason for hiding this comment

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

change LGTM

@dreamtalen dreamtalen mentioned this pull request Mar 28, 2022
Merged
@antoninbas
Copy link
Contributor

/skip-all

@antoninbas antoninbas merged commit 1515300 into antrea-io:main Mar 28, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@heanlan heanlan heanlan left review comments

@salv-orlando salv-orlando salv-orlando approved these changes

@jianjuns jianjuns jianjuns approved these changes

@wsquan171 wsquan171 Awaiting requested review from wsquan171

@yanjunz97 yanjunz97 Awaiting requested review from yanjunz97

@antoninbas antoninbas Awaiting requested review from antoninbas

Assignees
No one assigned
Labels
area/flow-visibility/aggregator Issues or PRs related to Flow Aggregator area/flow-visibility Issues or PRs related to flow visibility support in Antrea
Projects
None yet
Milestone
Antrea v1.6 release
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@dreamtalen @antoninbas @salv-orlando @jianjuns @heanlan