-
Notifications
You must be signed in to change notification settings - Fork 364
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
Extends the Endpoints support from 500 to 800, extra ones will be dropped in AntreaProxy #2101
Conversation
df057cc
to
b320a4d
Compare
275ef43
to
22b0306
Compare
Codecov Report
@@ Coverage Diff @@
## main #2101 +/- ##
==========================================
+ Coverage 61.23% 65.26% +4.02%
==========================================
Files 270 269 -1
Lines 20376 20812 +436
==========================================
+ Hits 12478 13583 +1105
+ Misses 6609 5842 -767
- Partials 1289 1387 +98
Flags with carried forward coverage won't be shown. Click here to find out more.
|
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.
@hongliangl : I would prefer to add descriptions about the actual changes in the commit title and message. We can list the issue number in the commit message too.
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 overall, would like to see @jianjuns @antoninbas's opinions.
@hongliangl please update the PR's title as well, otherwise it's not clear what this is about from the PR list. |
And typo in the commit message: "AtreaProxy" |
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.
we are not really fixing the issue IMO. We should either keep it open or open a new one.
pkg/agent/openflow/client.go
Outdated
maxRetryForOFSwitch = 5 | ||
// Due to the message size of openflow 1.3 and implementation of Service in Antrea, the maximum Endpoint that Antrea | ||
// can support now is 800. If the Endpoints of Service exceed 800, the exceeding Endpoints will be ignored. | ||
maxEndpoints = 800 |
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 we went from 500 to 800? This is a bit underwhelming to be honest, albeit still an improvement
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.
Yeah, still an improvement, may a bit, too small.
Thanks for reminding, updated. |
pkg/agent/proxy/proxier.go
Outdated
for _, endpoint := range endpointUpdateList { | ||
if _, ok := endpointsInstalled[endpoint.String()]; !ok { | ||
needUpdateEndpoints = true | ||
break | ||
} | ||
} |
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.
The extra endpoints won't be in "endpointsInstalled" and "needUpdateEndpoints" will always be true regardless of how many times it has been reconciled.
Sorting endpointUpdateList
is to arrange all Endpoints in order and then cut them. If the reserved
Endpoints are all installed, needUpdateEndpoints
will not true.
More detailed, needUpdateEndpoints
is recalculated with the cut endpointUpdateList
and endpointsInstalled
. needUpdateEndpoints
will not true if the reserved Endpoints in the cut endpointUpdateList
are all installed. This part causes extra overhead, but only when the service is oversize.
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.
I think L274 will set needUpdateEndpoints
to true.
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.
If a Service is oversize, L274 will always set needUpdateEndpoints
to true as extra Endpoints are not installed.
L290 resets needUpdateEndpoints
to false as endpointUpdateList
is cut and it's unknown that if all reserved Endpoints in endpointUpdateList
are installed, so L294~L299 checks that if there are any Endpoints in endpointUpdateList
not install.
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.
I missed L290. However, it still seems wrong to reset needUpdateEndpoints
as there are other conditions that could set it to true, for example, needUpdateEndpoints = pSvcInfo.SessionAffinityType() != svcInfo.SessionAffinityType()
. And it's redundant to "Check if the cut endpointUpdateList are all installed" twice when it exceeds the size. Could we cut the endpoints in the below place? https://github.com/vmware-tanzu/antrea/blob/eadf0921f88712b373df9f00c778ef81642def2f/pkg/agent/proxy/proxier.go#L244-L246
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.
it still seems wrong to reset needUpdateEndpoints as there are other conditions that could set it to true, for example,
needUpdateEndpoints = pSvcInfo.SessionAffinityType() != svcInfo.SessionAffinityType()
.
needUpdateEndpoints = pSvcInfo.SessionAffinityType() != svcInfo.SessionAffinityType()
should be considered.
Could we cut the endpoints in the below place?
Variable endpoints
is a map, IMO, we can't cut the endpoints below the code above.
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.
It needs to convert it to a list sooner or later, no harm to do it earlier? It can save the repeated calculation of needUpdateEndpoints
in your current code L273-L278 and L294-303, and reduce the code complexity in some way I think.
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.
Updated.
10b3620
to
6ad0b0c
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
/test-windows-networkpolicy |
/test-windows-e2e @jianjuns @antoninbas would you take another look at this PR? |
/test-windows-e2e |
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.
A suggestion on the feature gate doc.
@@ -51,7 +51,11 @@ example, to enable `AntreaProxy` on Linux, edit the Agent configuration in the | |||
`AntreaProxy` implements Service load-balancing for ClusterIP Services as part | |||
of the OVS pipeline, as opposed to relying on kube-proxy. This only applies to | |||
traffic originating from Pods, and destined to ClusterIP Services. In | |||
particular, it does not apply to NodePort Services. | |||
particular, it does not apply to NodePort Services. Please note that due to | |||
some restrictions on the implementation of Services in Antrea, the maximum |
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.
How about:
Please note that in the current AntreaProxy implementation there is a restriction that the maximum number...
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.
Unless you feel strongly about it, I suggest merging as it is. We have been postponing v1.0.1 for a while now, and this is pretty much the last change we are waiting for.
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.
That works for me.
for _, endpoint := range endpoints { // Check if there is any installed Endpoint which is not expected anymore. | ||
if _, ok := endpointsInstalled[endpoint.String()]; !ok { // There is an expected Endpoint which is not installed. | ||
needUpdateEndpoints = true | ||
if len(endpoints) > maxEndpoints { |
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.
personally I would have liked to see a unit test for this (with maxEndpoints
set to small value). However, this is strictly better than what we had before, and there is little risk in merging this.
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.
Make sense. I prefer to add a unit test. However, the maxEndpoints
is a constant variable, IMO, I didn't come up with any good idea to test this. BTW, thanks for merging this.
…pped in AntreaProxy (antrea-io#2101) For antrea-io#2092 Due to the message size and the implementation of Service in AntreaProxy, the maximum number of Endpoints that AntreaProxy can support now is 800. If the the number of Endpoints in given Service exceeds 800, the extra Endpoints will be dropped and a warning will be logged. In AntreaProxy, OVS group is the key part of Service implementation. For now, Antrea is using Openflow 1.3 to communicate with OVS. In previous design, every bucket of a OVS group has five actions. Two actions for loading Endpoint IP and port to registers and resubmit action must be preserved.The other two actions for loading values to register can be moved to flows (in current patch, they are moved to table 41), and then one message can hold more bucket items. As a result, the maximum Endpoint has changed from 511 to 800. Unfortunately, to ensure AntreaProxy running correctly, the extra Endpoints will be dropped.
…pped in AntreaProxy (#2101) For #2092 Due to the message size and the implementation of Service in AntreaProxy, the maximum number of Endpoints that AntreaProxy can support now is 800. If the the number of Endpoints in given Service exceeds 800, the extra Endpoints will be dropped and a warning will be logged. In AntreaProxy, OVS group is the key part of Service implementation. For now, Antrea is using Openflow 1.3 to communicate with OVS. In previous design, every bucket of a OVS group has five actions. Two actions for loading Endpoint IP and port to registers and resubmit action must be preserved.The other two actions for loading values to register can be moved to flows (in current patch, they are moved to table 41), and then one message can hold more bucket items. As a result, the maximum Endpoint has changed from 511 to 800. Unfortunately, to ensure AntreaProxy running correctly, the extra Endpoints will be dropped.
…pped in AntreaProxy (antrea-io#2101) For antrea-io#2092 Due to the message size and the implementation of Service in AntreaProxy, the maximum number of Endpoints that AntreaProxy can support now is 800. If the the number of Endpoints in given Service exceeds 800, the extra Endpoints will be dropped and a warning will be logged. In AntreaProxy, OVS group is the key part of Service implementation. For now, Antrea is using Openflow 1.3 to communicate with OVS. In previous design, every bucket of a OVS group has five actions. Two actions for loading Endpoint IP and port to registers and resubmit action must be preserved.The other two actions for loading values to register can be moved to flows (in current patch, they are moved to table 41), and then one message can hold more bucket items. As a result, the maximum Endpoint has changed from 511 to 800. Unfortunately, to ensure AntreaProxy running correctly, the extra Endpoints will be dropped.
…pped in AntreaProxy (antrea-io#2101) For antrea-io#2092 Due to the message size and the implementation of Service in AntreaProxy, the maximum number of Endpoints that AntreaProxy can support now is 800. If the the number of Endpoints in given Service exceeds 800, the extra Endpoints will be dropped and a warning will be logged. In AntreaProxy, OVS group is the key part of Service implementation. For now, Antrea is using Openflow 1.3 to communicate with OVS. In previous design, every bucket of a OVS group has five actions. Two actions for loading Endpoint IP and port to registers and resubmit action must be preserved.The other two actions for loading values to register can be moved to flows (in current patch, they are moved to table 41), and then one message can hold more bucket items. As a result, the maximum Endpoint has changed from 511 to 800. Unfortunately, to ensure AntreaProxy running correctly, the extra Endpoints will be dropped.
…pped in AntreaProxy (antrea-io#2101) For antrea-io#2092 Due to the message size and the implementation of Service in AntreaProxy, the maximum number of Endpoints that AntreaProxy can support now is 800. If the the number of Endpoints in given Service exceeds 800, the extra Endpoints will be dropped and a warning will be logged. In AntreaProxy, OVS group is the key part of Service implementation. For now, Antrea is using Openflow 1.3 to communicate with OVS. In previous design, every bucket of a OVS group has five actions. Two actions for loading Endpoint IP and port to registers and resubmit action must be preserved.The other two actions for loading values to register can be moved to flows (in current patch, they are moved to table 41), and then one message can hold more bucket items. As a result, the maximum Endpoint has changed from 511 to 800. Unfortunately, to ensure AntreaProxy running correctly, the extra Endpoints will be dropped.
…pped in AntreaProxy (#2101) For #2092 Due to the message size and the implementation of Service in AntreaProxy, the maximum number of Endpoints that AntreaProxy can support now is 800. If the the number of Endpoints in given Service exceeds 800, the extra Endpoints will be dropped and a warning will be logged. In AntreaProxy, OVS group is the key part of Service implementation. For now, Antrea is using Openflow 1.3 to communicate with OVS. In previous design, every bucket of a OVS group has five actions. Two actions for loading Endpoint IP and port to registers and resubmit action must be preserved.The other two actions for loading values to register can be moved to flows (in current patch, they are moved to table 41), and then one message can hold more bucket items. As a result, the maximum Endpoint has changed from 511 to 800. Unfortunately, to ensure AntreaProxy running correctly, the extra Endpoints will be dropped.
…pped in AntreaProxy (antrea-io#2101) For antrea-io#2092 Due to the message size and the implementation of Service in AntreaProxy, the maximum number of Endpoints that AntreaProxy can support now is 800. If the the number of Endpoints in given Service exceeds 800, the extra Endpoints will be dropped and a warning will be logged. In AntreaProxy, OVS group is the key part of Service implementation. For now, Antrea is using Openflow 1.3 to communicate with OVS. In previous design, every bucket of a OVS group has five actions. Two actions for loading Endpoint IP and port to registers and resubmit action must be preserved.The other two actions for loading values to register can be moved to flows (in current patch, they are moved to table 41), and then one message can hold more bucket items. As a result, the maximum Endpoint has changed from 511 to 800. Unfortunately, to ensure AntreaProxy running correctly, the extra Endpoints will be dropped.
…pped in AntreaProxy (#2101) For #2092 Due to the message size and the implementation of Service in AntreaProxy, the maximum number of Endpoints that AntreaProxy can support now is 800. If the the number of Endpoints in given Service exceeds 800, the extra Endpoints will be dropped and a warning will be logged. In AntreaProxy, OVS group is the key part of Service implementation. For now, Antrea is using Openflow 1.3 to communicate with OVS. In previous design, every bucket of a OVS group has five actions. Two actions for loading Endpoint IP and port to registers and resubmit action must be preserved.The other two actions for loading values to register can be moved to flows (in current patch, they are moved to table 41), and then one message can hold more bucket items. As a result, the maximum Endpoint has changed from 511 to 800. Unfortunately, to ensure AntreaProxy running correctly, the extra Endpoints will be dropped.
…pped in AntreaProxy (antrea-io#2101) For antrea-io#2092 Due to the message size and the implementation of Service in AntreaProxy, the maximum number of Endpoints that AntreaProxy can support now is 800. If the the number of Endpoints in given Service exceeds 800, the extra Endpoints will be dropped and a warning will be logged. In AntreaProxy, OVS group is the key part of Service implementation. For now, Antrea is using Openflow 1.3 to communicate with OVS. In previous design, every bucket of a OVS group has five actions. Two actions for loading Endpoint IP and port to registers and resubmit action must be preserved.The other two actions for loading values to register can be moved to flows (in current patch, they are moved to table 41), and then one message can hold more bucket items. As a result, the maximum Endpoint has changed from 511 to 800. Unfortunately, to ensure AntreaProxy running correctly, the extra Endpoints will be dropped.
…pped in AntreaProxy (antrea-io#2101) For antrea-io#2092 Due to the message size and the implementation of Service in AntreaProxy, the maximum number of Endpoints that AntreaProxy can support now is 800. If the the number of Endpoints in given Service exceeds 800, the extra Endpoints will be dropped and a warning will be logged. In AntreaProxy, OVS group is the key part of Service implementation. For now, Antrea is using Openflow 1.3 to communicate with OVS. In previous design, every bucket of a OVS group has five actions. Two actions for loading Endpoint IP and port to registers and resubmit action must be preserved.The other two actions for loading values to register can be moved to flows (in current patch, they are moved to table 41), and then one message can hold more bucket items. As a result, the maximum Endpoint has changed from 511 to 800. Unfortunately, to ensure AntreaProxy running correctly, the extra Endpoints will be dropped.
…pped in AntreaProxy (#2101) For #2092 Due to the message size and the implementation of Service in AntreaProxy, the maximum number of Endpoints that AntreaProxy can support now is 800. If the the number of Endpoints in given Service exceeds 800, the extra Endpoints will be dropped and a warning will be logged. In AntreaProxy, OVS group is the key part of Service implementation. For now, Antrea is using Openflow 1.3 to communicate with OVS. In previous design, every bucket of a OVS group has five actions. Two actions for loading Endpoint IP and port to registers and resubmit action must be preserved.The other two actions for loading values to register can be moved to flows (in current patch, they are moved to table 41), and then one message can hold more bucket items. As a result, the maximum Endpoint has changed from 511 to 800. Unfortunately, to ensure AntreaProxy running correctly, the extra Endpoints will be dropped.
The current implementation limits the maximum number of buckets in an OVS group add/insert_bucket message to 800. This constraint is based on the fact that each bucket has 3 actions, such as `set_field:0xa0a0007->reg0`, `set_field:0x50/0xffff->reg4`, and `resubmit(,EndpointDNAT)`. However, an update in antrea-io#5205 introduced a new action, `set_field:0x4000000/0x4000000->reg4`, for remote Endpoints, making it impossible to accommodate 800 buckets with 4 actions in an OVS group add/insert_bucket message. Another case is that the message cannot hold 800 buckets with 3 actions, such as `set_field:0xa0a0007->xxreg0`, `set_field:0x50/0xffff->reg4` and `resubmit(,EndpointDNAT)`, for IPv6 Endpoints. To address this limitation, we have the following changes in this patch: - The action for loading `EpToLearnRegMark` or `EpSelectedRegMark` in table `ServiceLB` flows is moved back to OVS group bucket action. This original change was introduced in antrea-io#2101, which is a workaround to accommodate as many as more Endpoints in an OVS group add message in Openflow 1.3, where an OVS group can be only created by an add message and cannot be updated. Now we use Openflow 1.5, where an insert_bucket message can be used to append buckets to an existing OVS group. Moving the action for loading `EpToLearnRegMark` or `EpSelectedRegMark` back to OVS group bucket action is more logical as such action is loaded after Service Endpoint selection, rather than being set earlier before the selection in table ServiceLB. - Set the maximum number of buckets to 400. is derived from the worst-case scenario, where each bucket includes 4 actions like: `set_field:0xa0a0007->xxreg0`, `set_field:0x50/0xffff->reg4`, `set_field:0x100000/0x100000->reg4`, `load:0x2->NXM_NX_REG4[16..18]` and `resubmit(,EndpointDNAT)`. We can use the following command to verify this: ```bash ovs-ofctl mod-group br-int group_id=100,type=select,$(for i in {0..400}; do echo -n "bucket=bucket_id:$i,weight:100,actions=set_field:0xa0a0007->xxreg0,set_field:0x50/0xffff->reg4,set_field:0/0x100000->reg4,load:0x2->NXM_NX_REG4[16..18],resubmit(,EndpointDNAT),"; done) ``` Signed-off-by: Hongliang Liu <[email protected]>
The current implementation limits the maximum number of buckets in an OVS group add/insert_bucket message to 800. This constraint is based on the fact that each bucket has 3 actions, such as `set_field:0xa0a0007->reg0`, `set_field:0x50/0xffff->reg4`, and `resubmit(,EndpointDNAT)`. However, an update in antrea-io#5205 introduced a new action, `set_field:0x4000000/0x4000000->reg4`, for remote Endpoints, making it impossible to accommodate 800 buckets with 4 actions in an OVS group add/insert_bucket message. Another case is that the message cannot hold 800 buckets with 3 actions, such as `set_field:0xa0a0007->xxreg0`, `set_field:0x50/0xffff->reg4` and `resubmit(,EndpointDNAT)`, for IPv6 Endpoints. To address this limitation, we have the following changes in this patch: - The action for loading `EpToLearnRegMark` or `EpSelectedRegMark` in table `ServiceLB` flows is moved back to OVS group bucket action. This original change was introduced in antrea-io#2101, which is a workaround to accommodate as many as more Endpoints in an OVS group add message in Openflow 1.3, where an OVS group can be only created by an add message and cannot be updated. Now we use Openflow 1.5, where an insert_bucket message can be used to append buckets to an existing OVS group. Moving the action for loading `EpToLearnRegMark` or `EpSelectedRegMark` back to OVS group bucket action is more logical as such action is loaded after Service Endpoint selection, rather than being set earlier before the selection in table ServiceLB. - Set the maximum number of buckets to 400. is derived from the worst-case scenario, where each bucket includes 4 actions like: `set_field:0xa0a0007->xxreg0`, `set_field:0x50/0xffff->reg4`, `set_field:0x100000/0x100000->reg4`, `load:0x2->NXM_NX_REG4[16..18]` and `resubmit(,EndpointDNAT)`. We can use the following command to verify this: ```bash ovs-ofctl mod-group br-int group_id=100,type=select,$(for i in {0..400}; do echo -n "bucket=bucket_id:$i,weight:100,actions=set_field:0xa0a0007->xxreg0,set_field:0x50/0xffff->reg4,set_field:0/0x100000->reg4,load:0x2->NXM_NX_REG4[16..18],resubmit(,EndpointDNAT),"; done) ``` Signed-off-by: Hongliang Liu <[email protected]>
For #2092
Due to the message size and the implementation of Service in AtreaProxy, the maximum Endpoint that AntreaProxy can support now is 800. If the Endpoints of Service exceed 800, the exceeding Endpoints will be ignored.
In AntreaProxy, OVS group is the key part of Service implementation. For now, Antrea is using Openflow 1.3 to communicate with OVS. In previous design, every bucket of a OVS group has five actions. Two actions for loading Endpoint IP and port to registers and resubmit action must be reserved.The other two actions for loading values to register can be moved to flows (in current patch, they are moved to table 41), and then one message can hold more bucket items. As a result, the maximum Endpoint has changed from 511 to 800. Unfortunately, to ensure AntreaProxy running correctly, the exceeding Endpoints will be ignored.