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

Cherry pick of #5592: Remove redundant log in fillPodInfo/fillServiceInfo#5731: Use labels to enhance records filtering in FA e2e tests#5770: Improve flow-visibility e2e test #5886

Conversation

yuntanghsu
Copy link
Contributor

@yuntanghsu yuntanghsu commented Jan 17, 2024

Cherry pick of #5592 #5731 #5770 on release-1.14.

#5592: Remove redundant log in fillPodInfo/fillServiceInfo
#5731: Use labels to enhance records filtering in FA e2e tests
#5770: Improve flow-visibility e2e test

For details on the cherry pick process, see the cherry pick requests page.

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

@antoninbas
Copy link
Contributor

/test-all

@yuntanghsu yuntanghsu force-pushed the automated-cherry-pick-of-#5592-#5731-#5770-upstream-release-1.14 branch from 28f89a2 to b621ce0 Compare January 18, 2024 01:55
@antoninbas
Copy link
Contributor

@yuntanghsu why did you have to update the PR?

@yuntanghsu
Copy link
Contributor Author

yuntanghsu commented Jan 18, 2024

@yuntanghsu why did you have to update the PR?

I squashed the commits.

There were some commits I think they were not squashed well before.

@antoninbas
Copy link
Contributor

@yuntanghsu why did you have to update the PR?

I squashed the commits.

There were some commits I think they were not squashed well before.

@tnqn Do we generally squash the commits manually for individual PRs when cherry-picking multiple PRs at once? I'm talking about individual PRs which may have been merged with "Squash and merge" but originally contained multiple commits (see #5592) for an example.

@tnqn
Copy link
Member

tnqn commented Jan 22, 2024

@tnqn Do we generally squash the commits manually for individual PRs when cherry-picking multiple PRs at once? I'm talking about individual PRs which may have been merged with "Squash and merge" but originally contained multiple commits (see #5592) for an example.

I always rebase and merge to keep commits consistent, to make it easy to search whether a commit is in a branch. I don't recall I have cherry-picked PRs containing multiple commits (as critical bugfixes don't usually contain multiple commits), but if I were doing it, I would cherry-pick the merge commit manually (d6766cc in this case).

@antoninbas
Copy link
Contributor

I think that makes sense.
I often merge PRs with multiple commits with the "Squash and merge" option, but I guess it's not very compatible with the cherry-pick-pull.sh script, and it becomes messy when trying to cherry-pick multiple PRs at once, with some of these PRs having multiple commits.

@yuntanghsu you may want to do a manual cherry-pick here, as suggested by Quan.

…onnection store. (antrea-io#5592)

1. Remove redundant logs in fillPodInfo/fillServiceInfo to avoid Flow
   Exporter from flooding logs.
2. Add Mark field for deny connections. We were missing this field
   previously, resulting in trying to fill service information for
   non-service IPs.
3. Update the DestinationServiceAddress for deny connections when we can
   find this information in PacketIn.
4. Update e2e/unit tests to verify that the Service-relateds field are
   filled correctly.

Fixes antrea-io#5573

Signed-off-by: Yun-Tang Hsu <hsuy@vmware.com>
In this commit, we:

1. Add a label to Perftest Pods before initiating traffic in each subtest to filter records in both the IPFIX collector Pod and the ClickHouse.
2. Remove the --since-time flag used during log retrieval from the IPFIX collector Pod in the e2e test.
3. Stop relying on timestamps for record filtering due to potential time discrepancies between the testbed and Kubernetes nodes, which might hinder the retrieval of desired records.

Signed-off-by: Yun-Tang Hsu <hsuy@vmware.com>
1. Changed the order in which we append expired records before exporting
   them from our exporter. For inter-Node traffic with egress/ingress NP
   with action drop, we will receive records from both PacketIn and the
   conntrack table. For the egress case, we need to ensure that the
   PacketIn record arrives at the FA first, or we will be stuck waiting
   for correlation.

2. Add checks to verify that records can successfully be sent by the
   Flow Exporter before sending traffic in e2e tests.

3. Add labels to External subtest to filter useless logs from the IPFIX
   collector Pod.

4. Confirm the correct addition of a label to a specific Pod after
   updating the Pod.

5. Remove the octetDeltaCount check and, instead, filter out all records
   with octetDeltaCount=0 when retrieving records from the IPFIX
   collector Pod and ClickHouse.

6. Use new version of go-ipfix test collector. The new collector no
    longer prints all received records. Instead records are saved
    in-memory, and can be queried over HTTP. The list of records can
    also be reset with an HTTP call. In this way, we can drastically
    reduce the time to retrieve records in tests.

Signed-off-by: Yun-Tang Hsu <hsuy@vmware.com>
@yuntanghsu yuntanghsu force-pushed the automated-cherry-pick-of-#5592-#5731-#5770-upstream-release-1.14 branch from b621ce0 to 2fe5d84 Compare January 22, 2024 19:09
@yuntanghsu yuntanghsu changed the title Automated cherry pick of #5592: Remove redundant log in fillPodInfo/fillServiceInfo #5731: Enhance Records Filtering: Utilizing Labels In The E2E Test #5770: Improve flow-visibility e2e test Automated cherry pick of #5592: Remove redundant log in fillPodInfo/fillServiceInfo#5731: Use labels to enhance records filtering in FA e2e tests#5770: Improve flow-visibility e2e test Jan 22, 2024
@yuntanghsu
Copy link
Contributor Author

@antoninbas I've manually cherry-picked these 3 PRs. Please take a look to see if it is correct.

@antoninbas
Copy link
Contributor

It's a bit misleading to reuse the same PR if you didn't use the script.
The cherry-pick is not "automated" since you did it manually.

@yuntanghsu yuntanghsu changed the title Automated cherry pick of #5592: Remove redundant log in fillPodInfo/fillServiceInfo#5731: Use labels to enhance records filtering in FA e2e tests#5770: Improve flow-visibility e2e test Cherry pick of #5592: Remove redundant log in fillPodInfo/fillServiceInfo#5731: Use labels to enhance records filtering in FA e2e tests#5770: Improve flow-visibility e2e test Jan 22, 2024
@yuntanghsu
Copy link
Contributor Author

yuntanghsu commented Jan 22, 2024

It's a bit misleading to reuse the same PR if you didn't use the script.

What do you mean here? Shouldn't I manually cherry-pick these PRs? Or should I manually do it for the first PR only?

@antoninbas
Copy link
Contributor

@yuntanghsu I thought you were going to do it manually for all PRs:

git checkout -b cherry-pick-of-...
git cherry-pick d6766cc
...

@yuntanghsu
Copy link
Contributor Author

I just realize you were saying I should create another PR to replace this one instead of referring to the PRs in this PR.

@yuntanghsu
Copy link
Contributor Author

replaced by #5900

@yuntanghsu yuntanghsu closed this Jan 22, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/cherry-pick Categorizes issue or PR as related to the cherry-pick of a bug fix from the main branch to a release
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants