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

Promote feature CleanupStaleUDPSvcConntrack from Alpha to Beta #6372

Merged
merged 1 commit into from
Jun 13, 2024

Conversation

hongliangl
Copy link
Contributor

@hongliangl hongliangl commented May 27, 2024

In #5112, due to the limitations of the Go netlink library, AntreaProxy
would unconditionally delete conntrack entries added by kube-proxy in
conntrack zone 0. AntreaProxy was supposed to only delete its own entries
in conntrack zones 65520 or 65521. To address this, a feature was added
to isolate the relevant code.

After the merge of #6193, the netlink library was updated, allowing
AntreaProxy to precisely delete conntrack entries in zones 65520 or 65521.
It is now safe to enable the corresponding code by default.

Depends on #6379

@hongliangl hongliangl force-pushed the 20240527-proxy-udp-beta branch 3 times, most recently from 2159797 to 50c73bb Compare May 28, 2024 15:58
@hongliangl hongliangl changed the title [WIP] Promote CleanupStaleUDPSvcConntrack to Beta stage Promote feature CleanupStaleUDPSvcConntrack from Alpha to Beta May 28, 2024
@hongliangl hongliangl marked this pull request as ready for review May 28, 2024 15:59
@hongliangl hongliangl added the action/release-note Indicates a PR that should be included in release notes. label May 28, 2024
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.

Are we planning to bump this up to GA for v2.2?
With the current state of things, is there any situation where a user would want to be able to disable this functionality?

(You may to update some unit tests with this change)

@antoninbas antoninbas requested a review from tnqn May 28, 2024 20:02
@hongliangl
Copy link
Contributor Author

Are we planning to bump this up to GA for v2.2?

I think we can do it in v2.2. After all, this is a basic functionality. It was not supposed to add a feature gate when adding this. However, due to the limitation of netlink, we had to add a feature gate in #5112.

With the current state of things, is there any situation where a user would want to be able to disable this functionality?

Maybe there is no reason to disable the functionality since it is a very basic. You can find the corresponding e2e tests in this link https://github.com/kubernetes/kubernetes/master/test/e2e/network/conntrack.go.

(You may to update some unit tests with this change)

Yes, I'll add or update some unit tests to cover all code of the functionality.

@hongliangl hongliangl force-pushed the 20240527-proxy-udp-beta branch 2 times, most recently from 24a3f0c to a6fc051 Compare May 29, 2024 10:53
@tnqn
Copy link
Member

tnqn commented May 29, 2024

I see you already extracted virtualNodeIP related cleanup to #6379, which is the right thing. The PR should just focus on the promotion.

@hongliangl
Copy link
Contributor Author

I see you already extracted virtualNodeIP related cleanup to #6379, which is the right thing. The PR should just focus on the promotion.

I should mention that the fix #6372 should be merged first, and this pr can be merged then.

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.

could we extract the refactoring of pkg/agent/proxy/proxier_test.go from this graduation PR? Are the 2 changes inter-dependent?

@hongliangl
Copy link
Contributor Author

could we extract the refactoring of pkg/agent/proxy/proxier_test.go from this graduation PR?

Will do.

Are the 2 changes inter-dependent?

No, the refactoring of pkg/agent/proxy/proxier_test.go could be dependent. Though my original motivation for refactoring the file is to enable CleanupStaleUDPSvcConntrack in some tests.

In antrea-io#5112, due to the limitations of the Go netlink library, AntreaProxy
would unconditionally delete conntrack entries added by kube-proxy in
conntrack zone 0. AntreaProxy was supposed to only delete its own entries
in conntrack zones 65520 or 65521. To address this, a feature was added
to isolate the relevant code.

After the merge of antrea-io#6193, the netlink library was updated, allowing
AntreaProxy to precisely delete conntrack entries in zones 65520 or 65521.
It is now safe to enable the corresponding code by default.

Signed-off-by: Hongliang Liu <lhongliang@vmware.com>
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

@hongliangl
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

@tnqn tnqn merged commit 79a4693 into antrea-io:main Jun 13, 2024
52 of 55 checks passed
@hongliangl hongliangl deleted the 20240527-proxy-udp-beta branch June 14, 2024 00:04
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