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

Adding pod store for flow-exporter and flow-aggregator to query pod information #5185

Merged
merged 1 commit into from
Jul 26, 2023

Conversation

yuntanghsu
Copy link
Contributor

@yuntanghsu yuntanghsu commented Jun 28, 2023

In this PR, we are adding a Pod store, which can be used to fetch Pod information by using ip and the connection start time.
The Pod store has the information for existing pods and the information for Pods deleted within 5 minutes.

Pod store is used to increase the accuracy for the records sent from flow-exporter to flow-aggregator.

Motivation:
Since connections are polled from conntrack table in flow-exporter every 5s and are sent to flow-aggregator every 5s, there will be at least 5s delay between the time the connection is established and the time we fetch Pod information in flow-exporter or flow-aggregator (we fill pod information using srcIP or dstIP in flow-exporter and flow-aggregator)

In theory, it is possible for the mapping (ip to pod) to be incorrect due to this 5s delay if there is very high Pod churn and an IP address is reused very quickly:

  1. The initial Pod may have been deleted → Not able to fetch Pod information in the exporter.
  2. The IP address may be in use by a new Pod. → Pod information will be updated incorrectly.

@yuntanghsu yuntanghsu marked this pull request as draft June 28, 2023 22:23
@yuntanghsu yuntanghsu force-pushed the pod_store branch 6 times, most recently from ed5a7fe to b4bb920 Compare July 3, 2023 19:29
@yuntanghsu yuntanghsu changed the title [WIP] Adding pod store for flow-exporter and flow-aggregator to query pod information Adding pod store for flow-exporter and flow-aggregator to query pod information Jul 3, 2023
@yuntanghsu yuntanghsu marked this pull request as ready for review July 3, 2023 20:50
cmd/antrea-agent/agent.go Outdated Show resolved Hide resolved
cmd/flow-aggregator/flow-aggregator.go Outdated Show resolved Hide resolved
pkg/agent/flowexporter/connections/connections_test.go Outdated Show resolved Hide resolved
pkg/util/podstore/podstore.go Outdated Show resolved Hide resolved
pkg/util/podstore/podstore.go Outdated Show resolved Hide resolved
pkg/util/podstore/podstore.go Outdated Show resolved Hide resolved
pkg/util/podstore/podstore.go Outdated Show resolved Hide resolved
type PodStorage struct {
curPod cache.Indexer
prevPod cache.Indexer
// Could be other lighter implementations
Copy link
Contributor

Choose a reason for hiding this comment

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

this is not a super useful comment as it is. Maybe we could add a benchmark in podtsore_test.go and see if the current implementation scales well to a very large number of Pods.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've removed it. I also add a benchmark for func BenchmarkGetPodByIPAndTime. Do you think it is sufficient if I only test this function?

Copy link
Contributor

Choose a reason for hiding this comment

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

GetPodByIPAndTime does not use the workqueue, so that benchmark doesn't seem relevant to the choice of workqueue.DelayingInterface.
If we wanted a valid benchmark, it would have to generate a lot of Pod Add & Delete events.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Do you mean we should generate a lot of Pod Add & Delete events in BenchmarkGetPodByIPAndTime ? I'm not quite sure how to test workqueue.DelayingInterface as currently it's only called when we have Delete events. And do you know how many events we should generate based on the number of Pods?

Copy link
Contributor

Choose a reason for hiding this comment

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

you could create 1,000 Pods in a fake apiserver, then delete all of them at once.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've updated the benchmark test. But I'm still thinking that even I generate lots of Pod Add/Delete events, I still can't tell whether workqueue.DelayingInterface plays a role here? When we have Delete events, the workqueue is only used by s.podsToDelete.AddAfter(pod, DelayTime) in the DeleteFunc. Is that because the workqueue can occupy some resource when we have Delete events?
Or should I have another benchmark for workqueue.DelayingInterface?

@yuntanghsu yuntanghsu force-pushed the pod_store branch 3 times, most recently from 3541f81 to 5a37928 Compare July 11, 2023 05:57
pkg/util/podstore/podstore.go Outdated Show resolved Hide resolved
pkg/util/podstore/podstore.go Outdated Show resolved Hide resolved
pkg/util/podstore/podstore.go Outdated Show resolved Hide resolved
pkg/util/podstore/podstore.go Outdated Show resolved Hide resolved
pkg/util/podstore/podstore.go Outdated Show resolved Hide resolved
pkg/util/podstore/podstore.go Outdated Show resolved Hide resolved
pkg/util/podstore/podstore.go Outdated Show resolved Hide resolved
pkg/util/podstore/podstore.go Outdated Show resolved Hide resolved
pkg/util/podstore/podstore.go Outdated Show resolved Hide resolved
pkg/util/podstore/podstore.go Outdated Show resolved Hide resolved
pkg/util/podstore/podstore.go Outdated Show resolved Hide resolved
pkg/util/podstore/podstore_test.go Outdated Show resolved Hide resolved
pkg/util/podstore/podstore_test.go Outdated Show resolved Hide resolved
pkg/util/podstore/podstore_test.go Outdated Show resolved Hide resolved
pkg/util/podstore/podstore_test.go Outdated Show resolved Hide resolved
pkg/util/podstore/podstore_test.go Outdated Show resolved Hide resolved
pkg/util/podstore/podstore_test.go Outdated Show resolved Hide resolved
pkg/util/podstore/podstore_test.go Outdated Show resolved Hide resolved
pkg/util/podstore/podstore_test.go Outdated Show resolved Hide resolved
@yuntanghsu yuntanghsu force-pushed the pod_store branch 4 times, most recently from e675cbd to 6126121 Compare July 22, 2023 05:18
@elton-furtado
Copy link

/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 overall

cmd/flow-aggregator/flow-aggregator.go Outdated Show resolved Hide resolved
pkg/flowaggregator/flowaggregator.go Show resolved Hide resolved
pkg/util/podstore/podstore.go Outdated Show resolved Hide resolved
pkg/util/podstore/podstore.go Outdated Show resolved Hide resolved
pkg/util/podstore/podstore.go Outdated Show resolved Hide resolved
Copy link
Contributor

@jianjuns jianjuns left a comment

Choose a reason for hiding this comment

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

Please change "pod" to "Pod" in commit description and message.

pkg/util/podstore/podstore.go Outdated Show resolved Hide resolved
pkg/util/podstore/podstore.go Outdated Show resolved Hide resolved
pkg/util/podstore/podstore_test.go Outdated Show resolved Hide resolved
pkg/util/podstore/podstore_test.go Outdated Show resolved Hide resolved
pkg/util/podstore/podstore_test.go Outdated Show resolved Hide resolved
pkg/util/podstore/podstore_test.go Outdated Show resolved Hide resolved
pkg/util/podstore/podstore_test.go Outdated Show resolved Hide resolved
pkg/util/podstore/podstore_test.go Outdated Show resolved Hide resolved
pkg/util/podstore/podstore_test.go Outdated Show resolved Hide resolved
pkg/util/podstore/podstore_test.go Outdated Show resolved Hide resolved
@yuntanghsu yuntanghsu force-pushed the pod_store branch 3 times, most recently from 40dfbdb to c22aae4 Compare July 25, 2023 04:27
tnqn
tnqn previously approved these changes Jul 25, 2023
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

@yuntanghsu
Copy link
Contributor Author

/test-all

1. Add Pod store which can be used to store current and previous Pods information.
2. Modify unit test

Signed-off-by: Yun-Tang Hsu <[email protected]>
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

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

@tnqn
Copy link
Member

tnqn commented Jul 26, 2023

/test-all

@tnqn tnqn merged commit 661a6a2 into antrea-io:main Jul 26, 2023
41 of 43 checks passed
@tnqn tnqn added the action/release-note Indicates a PR that should be included in release notes. label Jul 26, 2023
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.

5 participants