-
Notifications
You must be signed in to change notification settings - Fork 370
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -17,6 +17,7 @@ package proxy | |
import ( | ||
"fmt" | ||
"net" | ||
"sort" | ||
"strings" | ||
"sync" | ||
"time" | ||
|
@@ -25,6 +26,7 @@ import ( | |
"k8s.io/api/discovery/v1beta1" | ||
"k8s.io/apimachinery/pkg/runtime" | ||
k8sapitypes "k8s.io/apimachinery/pkg/types" | ||
"k8s.io/apimachinery/pkg/util/sets" | ||
"k8s.io/client-go/informers" | ||
"k8s.io/client-go/tools/record" | ||
"k8s.io/klog" | ||
|
@@ -42,6 +44,10 @@ import ( | |
const ( | ||
resyncPeriod = time.Minute | ||
componentName = "antrea-agent-proxy" | ||
// Due to the maximum message size in Openflow 1.3 and the implementation of Services in Antrea, the maximum number | ||
// of Endpoints that Antrea can support at the moment is 800. If the number of Endpoints for a given Service exceeds | ||
// 800, extra Endpoints will be dropped. | ||
maxEndpoints = 800 | ||
) | ||
|
||
// Proxier wraps proxy.Provider and adds extra methods. It is introduced for | ||
|
@@ -90,6 +96,8 @@ type proxier struct { | |
serviceStringMap map[string]k8sproxy.ServicePortName | ||
// serviceStringMapMutex protects serviceStringMap object. | ||
serviceStringMapMutex sync.Mutex | ||
// oversizeServiceSet records the Services that have more than 800 Endpoints. | ||
oversizeServiceSet sets.String | ||
|
||
runner *k8sproxy.BoundedFrequencyRunner | ||
stopChan <-chan struct{} | ||
|
@@ -117,6 +125,9 @@ func (p *proxier) removeStaleServices() { | |
} | ||
svcInfo := svcPort.(*types.ServiceInfo) | ||
klog.V(2).Infof("Removing stale Service: %s %s", svcPortName.Name, svcInfo.String()) | ||
if p.oversizeServiceSet.Has(svcPortName.String()) { | ||
p.oversizeServiceSet.Delete(svcPortName.String()) | ||
} | ||
if err := p.ofClient.UninstallServiceFlows(svcInfo.ClusterIP(), uint16(svcInfo.Port()), svcInfo.OFProtocol); err != nil { | ||
klog.Errorf("Failed to remove flows of Service %v: %v", svcPortName, err) | ||
continue | ||
|
@@ -258,12 +269,40 @@ func (p *proxier) installServices() { | |
} | ||
|
||
var endpointUpdateList []k8sproxy.Endpoint | ||
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 commentThe 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Make sense. I prefer to add a unit test. However, the |
||
if !p.oversizeServiceSet.Has(svcPortName.String()) { | ||
klog.Warningf("Since Endpoints of Service %s exceeds %d, extra Endpoints will be dropped", svcPortName.String(), maxEndpoints) | ||
p.oversizeServiceSet.Insert(svcPortName.String()) | ||
} | ||
// If the length of endpoints > maxEndpoints, endpoints should be cut. However, endpoints is a map. Therefore, | ||
// iterate the map and append every Endpoint to a slice endpointList. Since the iteration order of map in | ||
// Golang is random, if cut directly without any sorting, some Endpoints may not be installed. So cutting | ||
// slice endpointList after sorting can avoid this situation in some degree. | ||
var endpointList []k8sproxy.Endpoint | ||
for _, endpoint := range endpoints { | ||
endpointList = append(endpointList, endpoint) | ||
} | ||
sort.Sort(byEndpoint(endpointList)) | ||
endpointList = endpointList[:maxEndpoints] | ||
|
||
for _, endpoint := range endpointList { // 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 | ||
} | ||
endpointUpdateList = append(endpointUpdateList, endpoint) | ||
} | ||
} else { | ||
if p.oversizeServiceSet.Has(svcPortName.String()) { | ||
p.oversizeServiceSet.Delete(svcPortName.String()) | ||
} | ||
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 | ||
} | ||
endpointUpdateList = append(endpointUpdateList, endpoint) | ||
} | ||
endpointUpdateList = append(endpointUpdateList, endpoint) | ||
} | ||
|
||
if len(endpoints) < len(endpointsInstalled) { // There are Endpoints which expired. | ||
klog.V(2).Infof("Some Endpoints of Service %s removed, updating Endpoints", svcInfo.String()) | ||
needUpdateEndpoints = true | ||
|
@@ -605,6 +644,7 @@ func NewProxier( | |
endpointsMap: types.EndpointsMap{}, | ||
endpointReferenceCounter: map[string]int{}, | ||
serviceStringMap: map[string]k8sproxy.ServicePortName{}, | ||
oversizeServiceSet: sets.NewString(), | ||
groupCounter: types.NewGroupCounter(), | ||
ofClient: ofClient, | ||
isIPv6: isIPv6, | ||
|
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.