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

Improve antctl features in flow-aggregator #3642

Merged
merged 1 commit into from
Jun 28, 2022

Conversation

yuntanghsu
Copy link
Contributor

@yuntanghsu yuntanghsu commented Apr 14, 2022

Changing clickhouse/flow-collector related parameters at runtime and update configmap.

  • Antctl will mutate the ConfigMap directly, instead of calling a FA API in order to mutate the ConfigMap,
  • "antctl set" can only be used in FA pod
  • Watcher will watch mounted configMap.

Drawing


Enable ClickHouse
$ antctl set flow-aggregator clickHouse.enable=true
Update ClickHouse database
$ antctl set flow-aggregator clickHouse.database=name
Update ClickHouse databaseURL
$ antctl set flow-aggregator clickHouse.databaseURL=http://xxxxx
Update ClickHouse debug
$ antctl set flow-aggregator clickHouse.debug=true
Update ClickHouse compress
$ antctl set flow-aggregator clickHouse.compress=true
Update ClickHouse commitInterval
$ antctl set flow-aggregator clickHouse.commitInterval=10s
Update ClickHouse ttl (unit in second)
$ antctl set flow-aggregator clickHouse.ttl=100
Update IPFIX Flow Collector address
$ antctl set flow-aggregator flowCollector.address=:[:]
Enable IPFIX Flow Collector
$ antctl set flow-aggregator flowCollector.enable=true


Signed-off-by: Yun-Tang Hsu hsuy@vmware.com

@yuntanghsu yuntanghsu added the area/flow-visibility Issues or PRs related to flow visibility support in Antrea label Apr 14, 2022
@yuntanghsu yuntanghsu closed this Apr 14, 2022
@yuntanghsu yuntanghsu deleted the antctl_update_clickhouse branch April 14, 2022 18:43
@yuntanghsu yuntanghsu restored the antctl_update_clickhouse branch April 14, 2022 18:44
@yuntanghsu yuntanghsu reopened this Apr 15, 2022
@codecov-commenter
Copy link

codecov-commenter commented Apr 15, 2022

Codecov Report

Merging #3642 (d4b36e7) into main (a07959e) will decrease coverage by 3.34%.
The diff coverage is 0.00%.

❗ Current head d4b36e7 differs from pull request most recent head e9dad14. Consider uploading reports for the commit e9dad14 to get more accurate results

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #3642      +/-   ##
==========================================
- Coverage   49.03%   45.69%   -3.35%     
==========================================
  Files         261      256       -5     
  Lines       39245    38216    -1029     
==========================================
- Hits        19245    17462    -1783     
- Misses      18128    18994     +866     
+ Partials     1872     1760     -112     
Flag Coverage Δ
e2e-tests 45.69% <0.00%> (?)
kind-e2e-tests ?

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

Impacted Files Coverage Δ
pkg/config/flowaggregator/default.go 0.00% <0.00%> (ø)
pkg/util/flowexport/flowexport.go 0.00% <0.00%> (-34.62%) ⬇️
pkg/ipfix/ipfix_registry.go 0.00% <0.00%> (-100.00%) ⬇️
.../agent/flowexporter/priorityqueue/priorityqueue.go 0.00% <0.00%> (-92.41%) ⬇️
pkg/ipfix/ipfix_intermediate.go 0.00% <0.00%> (-90.91%) ⬇️
pkg/ipfix/ipfix_collector.go 0.00% <0.00%> (-83.34%) ⬇️
pkg/ipfix/ipfix_process.go 0.00% <0.00%> (-81.25%) ⬇️
...agent/flowexporter/connections/deny_connections.go 0.00% <0.00%> (-80.46%) ⬇️
pkg/agent/flowexporter/utils.go 0.00% <0.00%> (-76.60%) ⬇️
.../agent/flowexporter/connections/conntrack_linux.go 0.00% <0.00%> (-72.59%) ⬇️
... and 86 more

@yuntanghsu yuntanghsu force-pushed the antctl_update_clickhouse branch 4 times, most recently from 892893a to 03cc4c6 Compare April 15, 2022 04:45
@antrea-io antrea-io deleted a comment from lgtm-com bot Apr 15, 2022
@antrea-io antrea-io deleted a comment from lgtm-com bot Apr 15, 2022
@antrea-io antrea-io deleted a comment from lgtm-com bot Apr 15, 2022
@lgtm-com
Copy link

lgtm-com bot commented Apr 15, 2022

This pull request introduces 1 alert when merging a0b1803 into 2ab80d0 - view on LGTM.com

new alerts:

  • 1 for Database query built from user-controlled sources

@lgtm-com
Copy link

lgtm-com bot commented Apr 15, 2022

This pull request introduces 1 alert when merging 7eeb50b into 2ab80d0 - view on LGTM.com

new alerts:

  • 1 for Database query built from user-controlled sources

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.

IMO, there is a major issue with this PR: these commands are not troubleshooting commands, there are used to update the FlowAggregator configuration, yet changes made through antctl are not persistent (if the FlowAggregator Pod restarts, these changes will be lost) and they conflict with the "source of truth" that would be the ConfigMap. I don't think that's right, and we had the same issue in the past with the Antrea configuration.

pkg/antctl/antctl.go Outdated Show resolved Hide resolved
@yuntanghsu yuntanghsu force-pushed the antctl_update_clickhouse branch 3 times, most recently from 7f49f4f to 2492701 Compare April 16, 2022 05:02
@antrea-io antrea-io deleted a comment from lgtm-com bot Apr 16, 2022
@yuntanghsu yuntanghsu force-pushed the antctl_update_clickhouse branch 4 times, most recently from 3c2b293 to c257785 Compare April 18, 2022 23:41
pkg/antctl/antctl.go Outdated Show resolved Hide resolved
@yuntanghsu yuntanghsu closed this Apr 20, 2022
@yuntanghsu yuntanghsu force-pushed the antctl_update_clickhouse branch 13 times, most recently from 154f29d to 1317491 Compare June 11, 2022 07:03
pkg/flowaggregator/clickhouseclient/clickhouseclient.go Outdated Show resolved Hide resolved
pkg/flowaggregator/clickhouseclient/clickhouseclient.go Outdated Show resolved Hide resolved
pkg/flowaggregator/clickhouseclient/clickhouseclient.go Outdated Show resolved Hide resolved
pkg/flowaggregator/flowaggregator.go Outdated Show resolved Hide resolved
pkg/flowaggregator/flowaggregator.go Outdated Show resolved Hide resolved
pkg/flowaggregator/util/options.go Outdated Show resolved Hide resolved
pkg/flowaggregator/flowaggregator_test.go Show resolved Hide resolved
pkg/flowaggregator/flowaggregator_test.go Outdated Show resolved Hide resolved
pkg/flowaggregator/flowaggregator_test.go Outdated Show resolved Hide resolved
pkg/flowaggregator/flowaggregator_test.go Outdated Show resolved Hide resolved
@yuntanghsu yuntanghsu force-pushed the antctl_update_clickhouse branch 3 times, most recently from c3d9b00 to 85c1fd5 Compare June 27, 2022 16:42
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.

a couple more small comments

pkg/flowaggregator/flowaggregator_test.go Outdated Show resolved Hide resolved
pkg/flowaggregator/flowaggregator_test.go Outdated Show resolved Hide resolved
pkg/flowaggregator/util/options/options.go Outdated 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.

LGTM, please squash all your commits into a single one, with a helpful and well-formatted commit message.

1. Add “antctl set flow-aggregator” command for antctl in order to update clickhouse/flow-collector related parameters.
2. “antctl set flow-aggregator” will directly mutate the volume configMap of flow-aggregator
3. Mounted ConfigMap will be updated in 1min if there is a modification of volume configMap
4. Create watcher in flow-aggregator pod to watch mounted configMap  and let fa pod react to the changes
5. “antctl set flow-aggregator” can only be used in flow-aggregator pod currently.

Signed-off-by: Yun-Tang Hsu <hsuy@vmware.com>
@yuntanghsu
Copy link
Contributor Author

/test-conformance
/test-e2e
/test-networkpolicy

@yuntanghsu
Copy link
Contributor Author

/test-conformance

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/flow-visibility Issues or PRs related to flow visibility support in Antrea
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants