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

Add clusterUUID column to S3Uploader and ClickHouseExporter #4214

Merged
merged 1 commit into from
Sep 13, 2022

Conversation

heanlan
Copy link
Contributor

@heanlan heanlan commented Sep 7, 2022

To support multi-cluster, we add a column "clusterUUID" to the S3Uploader and ClickHouseExporter flows table, as the cluster identifier.

Signed-off-by: heanlan [email protected]

@codecov
Copy link

codecov bot commented Sep 7, 2022

Codecov Report

Merging #4214 (fb2f81b) into main (2ce2ef0) will decrease coverage by 0.39%.
The diff coverage is 60.00%.

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

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #4214      +/-   ##
==========================================
- Coverage   60.97%   60.58%   -0.40%     
==========================================
  Files         384      385       +1     
  Lines       54378    54392      +14     
==========================================
- Hits        33159    32954     -205     
- Misses      18770    19000     +230     
+ Partials     2449     2438      -11     
Flag Coverage Δ
integration-tests 34.79% <ø> (+<0.01%) ⬆️
kind-e2e-tests 48.28% <49.09%> (+0.50%) ⬆️
unit-tests 42.02% <14.54%> (-0.90%) ⬇️
Impacted Files Coverage Δ
pkg/flowaggregator/exporter/s3.go 0.00% <0.00%> (-71.12%) ⬇️
pkg/flowaggregator/exporter/ipfix.go 63.11% <33.33%> (-0.63%) ⬇️
pkg/flowaggregator/exporter/clickhouse.go 52.00% <50.00%> (-26.27%) ⬇️
pkg/flowaggregator/flowaggregator.go 67.56% <62.50%> (ø)
pkg/flowaggregator/s3uploader/s3uploader.go 72.13% <66.66%> (-17.61%) ⬇️
pkg/flowaggregator/exporter/utils.go 72.72% <72.72%> (ø)
...lowaggregator/clickhouseclient/clickhouseclient.go 73.77% <100.00%> (-6.72%) ⬇️
pkg/agent/cniserver/ipam/antrea_ipam.go 52.81% <0.00%> (-22.95%) ⬇️
... and 34 more

@heanlan heanlan marked this pull request as draft September 7, 2022 20:44
@heanlan heanlan marked this pull request as ready for review September 8, 2022 16:45
pkg/flowaggregator/exporter/utils.go Outdated Show resolved Hide resolved
pkg/flowaggregator/exporter/utils.go Outdated Show resolved Hide resolved
pkg/flowaggregator/s3uploader/s3uploader_test.go Outdated Show resolved Hide resolved
)
clusterUUID, err := getClusterUUID(k8sClient)
if err != nil {
klog.InfoS("Error when retrieving cluster UUID; will generate a random observation domain ID", "error", err)
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 could switch to klog.ErrorS here

clusterUUID = clusterIdentity.UUID
return true, nil
}); err != nil {
return clusterUUID, fmt.Errorf("unable to retrieve cluster UUID, timeout: %v, ConfigMapNameSpace: %s, ConfigMapName: %s", timeout, defaultAntreaNamespace, clusteridentity.DefaultClusterIdentityConfigMapName)
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
return clusterUUID, fmt.Errorf("unable to retrieve cluster UUID, timeout: %v, ConfigMapNameSpace: %s, ConfigMapName: %s", timeout, defaultAntreaNamespace, clusteridentity.DefaultClusterIdentityConfigMapName)
return clusterUUID, fmt.Errorf("unable to retrieve cluster UUID from ConfigMap '%s/%s': timeout after %v", defaultAntreaNamespace, clusteridentity.DefaultClusterIdentityConfigMapName, timeout)

@heanlan heanlan force-pushed the add-clusterID branch 2 times, most recently from 8f74883 to ab03a81 Compare September 9, 2022 18:14
@heanlan heanlan marked this pull request as draft September 9, 2022 18:24
@heanlan heanlan marked this pull request as ready for review September 9, 2022 19:09
antoninbas
antoninbas previously approved these changes Sep 9, 2022
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

dreamtalen
dreamtalen previously approved these changes Sep 9, 2022
Copy link
Contributor

@dreamtalen dreamtalen left a comment

Choose a reason for hiding this comment

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

LGTM

@@ -81,9 +81,10 @@ const (
throughputFromSourceNode,
throughputFromDestinationNode,
reverseThroughputFromSourceNode,
reverseThroughputFromDestinationNode)
reverseThroughputFromDestinationNode,
clusterUUID)
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: some extra indents 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, mixed tabs and spaces

To support multi-cluster, we add a column "clusterUUID" to the
S3Uploader and ClickHouseExporter flows table, as the cluster
identifier.

Signed-off-by: heanlan <[email protected]>
@heanlan
Copy link
Contributor Author

heanlan commented Sep 9, 2022

/test-all

@heanlan
Copy link
Contributor Author

heanlan commented Sep 13, 2022

/test-all

@heanlan
Copy link
Contributor Author

heanlan commented Sep 13, 2022

Hi @antoninbas , shall we merge it? The successful run of jenkins e2e is here: http://10.176.27.169:8080/job/antrea-e2e-for-pull-request/1413/

@antoninbas antoninbas merged commit 2e37448 into antrea-io:main Sep 13, 2022
@heanlan heanlan deleted the add-clusterID branch September 13, 2022 20:39
heanlan added a commit to heanlan/antrea that referenced this pull request Mar 29, 2023
…o#4214)

To support multi-cluster, we add a column "clusterUUID" to the
S3Uploader and ClickHouseExporter flows table, as the cluster
identifier.

Signed-off-by: heanlan <[email protected]>
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.

3 participants