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

[FlowAggregator] Add clusterId to aggregated records #6769

Merged
merged 3 commits into from
Jan 8, 2025

Conversation

antoninbas
Copy link
Contributor

In order to allow external flow collectors to easily identify which cluster the record is coming from (rather than relying on the observationDomainId or the exporter's IP address).

We use the cluster UUID (generated by Antrea) to populate this IE.

@antoninbas antoninbas force-pushed the fa-add-clusterId-ie branch 2 times, most recently from 49d1dcc to 0c3d2d8 Compare January 3, 2025 23:50
@antoninbas antoninbas marked this pull request as ready for review January 4, 2025 00:28
@antoninbas antoninbas added the action/release-note Indicates a PR that should be included in release notes. label Jan 4, 2025
@heanlan
Copy link
Contributor

heanlan commented Jan 6, 2025

Will it introduce fields redundancy for s3uploader and clickhouseexporter? as we previously added the field through #4214? Is it the same thing?

@antoninbas
Copy link
Contributor Author

Will it introduce fields redundancy for s3uploader and clickhouseexporter? as we previously added the field through #4214? Is it the same thing?

If you mean redundancy, as we are adding information we don't need for these exporters, then the answer is yes. These exporters add the field directly to their output, and they don't need this information.
However, there is no redundancy in the sense that these exporters' output will have duplicate data. These exporters will just ignore this IE value.

There is no straightforward solution to improve this IMO, as the IPFIX exporter cannot modify records, so it has to be done at "aggregation" time.

heanlan
heanlan previously approved these changes Jan 7, 2025
Copy link
Contributor

@heanlan heanlan left a comment

Choose a reason for hiding this comment

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

LGTM

In order to allow external flow collectors to easily identify which
cluster the record is coming from (rather than relying on the
observationDomainId or the exporter's IP address).

We use the cluster UUID (generated by Antrea) to populate this IE.

Signed-off-by: Antonin Bas <antonin.bas@broadcom.com>
Signed-off-by: Antonin Bas <antonin.bas@broadcom.com>
Signed-off-by: Antonin Bas <antonin.bas@broadcom.com>
@antoninbas
Copy link
Contributor Author

/test-all

Copy link
Member

@tnqn tnqn left a comment

Choose a reason for hiding this comment

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

LGTM

@antoninbas antoninbas merged commit f991c42 into antrea-io:main Jan 8, 2025
57 of 62 checks passed
@antoninbas antoninbas deleted the fa-add-clusterId-ie branch January 8, 2025 19:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
action/release-note Indicates a PR that should be included in release notes.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants