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

Look up Pods by name to fetch labels in Flow Aggregator #4942

Merged
merged 1 commit into from
May 16, 2023
Merged

Look up Pods by name to fetch labels in Flow Aggregator #4942

merged 1 commit into from
May 16, 2023

Conversation

dreamtalen
Copy link
Contributor

@dreamtalen dreamtalen commented May 5, 2023 •
edited
Loading

Currently Flow Aggregator looks up Pods by IP address to fetch labels, it is very possible to obtain incorrect Pods when Pod turnover quickly.
In this commit, we change to looking up Pods by Pod Name and Namespace to fetch labels in Flow Aggregator.

antoninbas
antoninbas reviewed May 9, 2023
View reviewed changes
pkg/flowaggregator/flowaggregator.go Outdated
@@ -482,18 +485,11 @@ func (fa *flowAggregator) fillK8sMetadata(key ipfixintermediate.FlowKey, record
}
}

func (fa *flowAggregator) fetchPodLabels(podAddress string) string {
pods, err := fa.podInformer.Informer().GetIndexer().ByIndex(podInfoIndex, podAddress)
func (fa *flowAggregator) fetchPodLabels(podName string, podNamespace string) string {
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: make namespace come first in parameter list

pkg/flowaggregator/flowaggregator.go Outdated
func (fa *flowAggregator) fetchPodLabels(podAddress string) string {
pods, err := fa.podInformer.Informer().GetIndexer().ByIndex(podInfoIndex, podAddress)
func (fa *flowAggregator) fetchPodLabels(podName string, podNamespace string) string {
pod, err := fa.podLister.Pods(podNamespace).Get(podName)
if err != nil {
klog.Warning(err)
Copy link
Contributor

Choose a reason for hiding this comment

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

for the functions you update, could you make sure all the klog calls are changed to use structured logging
In this case, use klog.InfoS or klog.ErrorS as you deem appropriate

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure, updated.

pkg/flowaggregator/flowaggregator.go Outdated
podLabelString = fa.fetchPodLabels(key.DestinationAddress)

destinationPodLabelString := ""
if destinationPodName, _, exist := record.GetInfoElementWithValue("destinationPodName"); exist {
Copy link
Contributor

Choose a reason for hiding this comment

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

let's clean up the code to avoid duplication between source & destination Pods (using a helper function, and maybe with an early return if some field is missing, to avoid too much if nesting)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure, I add a new parameter side to the fillPodLabels to avoid introducing a new helper function. Please let me know if you think a separate function looks better.

@antoninbas antoninbas added this to the Antrea v1.12 release milestone May 9, 2023
@antoninbas antoninbas added area/flow-visibility/aggregator Issues or PRs related to Flow Aggregator action/release-note Indicates a PR that should be included in release notes. labels May 9, 2023
jianjuns
jianjuns reviewed May 12, 2023
View reviewed changes
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.

In commit message:

we change to lookup Pods by name and namespace

lookup -> looking up

Namespace

@dreamtalen
Copy link
Contributor Author

In commit message:

we change to lookup Pods by name and namespace

lookup -> looking up

Namespace

Thanks, updated.

@dreamtalen dreamtalen changed the title Lookup Pods by name to fetch labels in Flow Aggregator Look up Pods by name to fetch labels in Flow Aggregator May 15, 2023
@dreamtalen dreamtalen marked this pull request as draft May 15, 2023 21:24
@dreamtalen dreamtalen marked this pull request as ready for review May 15, 2023 22:25
antoninbas
antoninbas reviewed May 15, 2023
View reviewed changes
pkg/flowaggregator/flowaggregator.go Outdated
if err != nil {
klog.Warningf("Add sourcePodLabels InfoElementWithValue failed: %v", err)
}
func (fa *flowAggregator) fillPodLabels(record ipfixentities.Record, side string) {
Copy link
Contributor

Choose a reason for hiding this comment

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

IMO, it would be better to keep func (fa *flowAggregator) fillPodLabels(record ipfixentities.Record) and introduce func (fa *flowAggregator) fillPodLabelsForSide(record ipfixentities.Record, podNamespaceIE, podNameIE, podLabelsIE string) error, so that the code looks like this:

func (fa *flowAggregator) fillPodLabels(record ipfixentities.Record) {
    if err := fillPodLabelsForSide(record, "sourcePodNamespace", "sourcePodName", "sourcePodLabels"); err != nil {
        klog.ErrorS(err, "Error when filling pod labels", "side", "source")
    }
    if err := fillPodLabelsForSide(record, "destinationPodNamespace", "destinationPodName", "destinationPodLabels"); err != nil {
        klog.ErrorS(err, "Error when filling pod labels", "side", "destination")
    }
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes it looks better in this way, thanks for the advice!

antoninbas
antoninbas previously approved these changes May 16, 2023
View reviewed changes
pkg/flowaggregator/flowaggregator.go Outdated
Comment on lines 504 to 510
if podName, _, exist := record.GetInfoElementWithValue(podNameIEName); exist {
podNameString := podName.GetStringValue()
if podNamespace, _, exist := record.GetInfoElementWithValue(podNamespaceIEName); exist {
podNamespaceString := podNamespace.GetStringValue()
if podNameString != "" && podNamespaceString != "" {
podLabelsString = fa.fetchPodLabels(podNamespaceString, podNameString)
}
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: the Go convention is to use ok (instead of exist) for that type of variable. The convention is followed in pkg/flowaggregator/flowrecord/record.go

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.

@dreamtalen
Look up Pods by name to fetch labels in Flow Aggregator
a1949bf 
Currently Flow Aggregator looks up Pods by IP address to fetch labels,
it is very possible to obtain incorrect Pods when Pod turnover quickly.
In this commit, we change to looking up Pods by Pod Name and Namespace
to fetch labels in Flow Aggregator.

Signed-off-by: Yongming Ding <dyongming@vmware.com>
@dreamtalen
Copy link
Contributor Author

/test-all

antoninbas
antoninbas approved these changes May 16, 2023
View reviewed changes
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
Copy link
Contributor Author

/test-conformance

@antoninbas antoninbas merged commit 197d282 into antrea-io:main May 16, 2023
ceclinux pushed a commit to ceclinux/antrea that referenced this pull request Jun 5, 2023
@dreamtalen @ceclinux
Look up Pods by name to fetch labels in Flow Aggregator (antrea-io#4942)
5b417e7 
Currently Flow Aggregator looks up Pods by IP address to fetch labels,
it is very possible to obtain incorrect Pods when Pod turnover is high.
In this commit, we instead to look up Pods by Pod Name and Namespace
to fetch labels in Flow Aggregator.

Signed-off-by: Yongming Ding <dyongming@vmware.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jianjuns jianjuns jianjuns left review comments

@antoninbas antoninbas antoninbas approved these changes

Assignees
No one assigned
Labels
action/release-note Indicates a PR that should be included in release notes. area/flow-visibility/aggregator Issues or PRs related to Flow Aggregator
Projects
None yet
Milestone
Antrea v1.12 release
Development

Successfully merging this pull request may close these issues.
3 participants
@dreamtalen @antoninbas @jianjuns