-
Notifications
You must be signed in to change notification settings - Fork 376
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
Support TopologyAwareHints in AntreaProxy #3515
Conversation
Codecov Report
@@ Coverage Diff @@
## main #3515 +/- ##
==========================================
- Coverage 64.00% 63.93% -0.08%
==========================================
Files 293 294 +1
Lines 43300 43406 +106
==========================================
+ Hits 27716 27750 +34
- Misses 13340 13417 +77
+ Partials 2244 2239 -5
Flags with carried forward coverage won't be shown. Click here to find out more.
|
88f1d40
to
0733796
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could I have a rough ideas what changes from copied from kube-proxy, what are Antrea specific?
# this flag will not take effect. | ||
# EndpointSlice: false | ||
|
||
# Enable TopologyAwareHints in AntreaProxy. This requires AntreaProxy and EndpointSlice to be | ||
# enabled, otherwise this flag will not take effect. | ||
# TopologyAwareHints: false |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So, this is a feature gate in K8s too? I am trying to understand do we need to add individual feature gates (for code isolation consideration), or just need a config flag to to enable/disable the feature, or nothing but enable it by default.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, it is also a feature gate in K8s. If we don't need this flag, I think we should also remove EndpointSlice feature gate in Antrea Agent since it is also a feature gate in K8s. I don't have a idea how to decide that if we need a feature gate in Antrea for the code that has a feature gate in K8s.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok. Probably let us keep it a feature gate for now.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Perhaps EndpointSlice should be a config as it's actually used to determine which API agent will use, and will be needed until we no longer support K8s versions that can disable EndpointSlice feature.
For TopologyAwareHints, I assume if the feature is disabled on K8s side, there will be no topology hint, right? If yes, we could just rely on the content of the endpoints. If there is hint, meaning feature is enabled, we implement it. If there isn't a hint, meaning feature is disabled, we don't do anything.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For TopologyAwareHints, I assume if the feature is disabled on K8s side, there will be no topology hint, right?
Yes
If there is hint, meaning feature is enabled, we implement it. If there isn't a hint, meaning feature is disabled, we don't do anything.
Yes, if there is no hint available, we don't do anything.
/test-all-features-conformance |
50515f9
to
cdae534
Compare
240c1af
to
ae92699
Compare
ae92699
to
dd6cbdf
Compare
dd6cbdf
to
b3eb306
Compare
b3eb306
to
f43a3a5
Compare
f43a3a5
to
9fa1ff7
Compare
09bac5c
to
9a9b30d
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. Have you tested the feature with upstream test about it?
|
b4ec8f3
to
ebb2f25
Compare
/test-all |
@tnqn added some unit tests and verified the feature manually, and it works as expected. |
ebb2f25
to
8564efc
Compare
Feature TopologyAwareHints is at Beta stage in Kubernetes v1.23, and it is enabled by default in Kubernetes v1.24 See more https://kubernetes.io/docs/concepts/services-networking/topology-aware-hints/. Signed-off-by: Hongliang Liu <[email protected]>
8564efc
to
1e95b9d
Compare
/test-all |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Feature TopologyAwareHints is at Beta stage in Kubernetes v1.23, and it is
enabled by default in Kubernetes v1.24 See more
https://kubernetes.io/docs/concepts/services-networking/topology-aware-hints/.
Signed-off-by: Hongliang Liu [email protected]