Remove the limit on the number of Endpoints in AntreaProxy #4167
Conversation
Codecov Report
@@ Coverage Diff @@
## main #4167 +/- ##
==========================================
+ Coverage 62.26% 66.26% +3.99%
==========================================
Files 385 304 -81
Lines 54501 46637 -7864
==========================================
- Hits 33933 30902 -3031
+ Misses 18069 13326 -4743
+ Partials 2499 2409 -90
|
hongliangl
changed the title
hongliangl
changed the title
hongliangl
force-pushed
the
20220826-openflow1.5
branch
2 times, most recently
from
ce5d9d5
to
610a9b2
Compare
hongliangl
force-pushed
the
20220826-openflow1.5
branch
3 times, most recently
from
c6b43cb
to
13085d9
Compare
hongliangl
added
area/proxy
hongliangl
force-pushed
the
20220826-openflow1.5
branch
3 times, most recently
from
7efd76f
to
fc18daf
Compare
hongliangl
force-pushed
the
20220826-openflow1.5
branch
2 times, most recently
from
6ff9140
to
c524be5
Compare
| } | ||
| return true | ||
| return b.ofSwitch.DeleteGroup(uint32(id)) |
There was a problem hiding this comment.
Why needs this b.ofSwitch.DeleteGroup(uint32(id))?
g.Delete() should already delete the group on OVS, and ofGroup.Add() does not add any object into ofctrl cache.
There was a problem hiding this comment.
We need b.ofSwitch.DeleteGroup(uint32(id)) to delete the group id cache in OFSwitch. We can see that when creating a new group:
// Create a new group. return an error if it already exists
func (self *OFSwitch) NewGroup(groupId uint32, groupType GroupType) (*Group, error) {
// check if the group already exists
if self.groupDb[groupId] != nil {
return nil, errors.New("group already exists")
}
// Create a new group
group := newGroup(groupId, groupType, self)
// Save it in the DB
self.groupDb[groupId] = group
return group, nil
}g.Add() and g.Delete() don't change the group id cache in OFSwitch since they are sent to OVS by bundle:
func (g *ofGroup) Add() error {
return g.bridge.AddOFEntriesInBundle([]OFEntry{g}, nil, nil)
}
func (g *ofGroup) Modify() error {
return g.bridge.AddOFEntriesInBundle(nil, []OFEntry{g}, nil)
}
func (g *ofGroup) Delete() error {
return g.bridge.AddOFEntriesInBundle(nil, nil, []OFEntry{g})
}As a result, when deleting a group, we need also to delete the group id from OFSwitch cache.
hongliangl
force-pushed
the
20220826-openflow1.5
branch
from
c524be5
to
078f046
Compare
pkg/ovs/openflow/ofctrl_group.go
Outdated
| for i, done := 0, false; done != true; i++ { | ||
| // Get the range of buckets to generate a temp group. | ||
| firstBucketIdx := i * MaxBucketsPerMessage | ||
| lastBucketIdx := (i + 1) * MaxBucketsPerMessage | ||
| if lastBucketIdx > len(g.ofctrl.Buckets) { | ||
| lastBucketIdx = len(g.ofctrl.Buckets) | ||
| done = true | ||
| } | ||
| // Generate a temp group to get an OVS message. Note that, the original group should not be modified since it is | ||
| // also stored in group cache, and the group cache is used when replying groups. | ||
| groupMessage := &ofctrl.Group{ | ||
| ID: g.ofctrl.ID, | ||
| GroupType: g.ofctrl.GroupType, | ||
| Buckets: g.ofctrl.Buckets[firstBucketIdx:lastBucketIdx], | ||
| } |
There was a problem hiding this comment.
what if number of buckets equals MaxBucketsPerMessage? Is it legal to insert empty buckets?
I think the code could be simplified:
for start := 0; start < len(g.ofctrl.Buckets); start += MaxBucketsPerMessage {
// Get the range of buckets to generate a temp group.
end := start + MaxBucketsPerMessage
if end > len(g.ofctrl.Buckets) {
end = len(g.ofctrl.Buckets)
}
// Generate a temp group to get an OVS message. Note that, the original group should not be modified since it is
// also stored in group cache, and the group cache is used when replying groups.
groupMessage := &ofctrl.Group{
ID: g.ofctrl.ID,
GroupType: g.ofctrl.GroupType,
Buckets: g.ofctrl.Buckets[start:end],
}
// For the message which is not the first, insert_buckets is used to add buckets to the group on OVS.
if start != 0 {
operation = openflow15.OFPGC_INSERT_BUCKET
}
messages = append(messages, groupMessage.GetBundleMessage(operation))
}
There was a problem hiding this comment.
Looks better, and I changed a little to support a message when there is no bucket in the group.
hongliangl
force-pushed
the
20220826-openflow1.5
branch
from
078f046
to
4772049
Compare
hongliangl
force-pushed
the
20220826-openflow1.5
branch
from
4772049
to
195403f
Compare
hongliangl
force-pushed
the
20220826-openflow1.5
branch
from
195403f
to
a8efa49
Compare
|
/test-all |
|
can we increase code coverage of the modified code? |
maybe the low coverage is because the commit's parent patch is not the latest, could you rebase on main? |
hongliangl
force-pushed
the
20220826-openflow1.5
branch
from
a8efa49
to
21ecb63
Compare
|
/test-all |
hongliangl
force-pushed
the
20220826-openflow1.5
branch
from
21ecb63
to
b65c3b4
Compare
|
The Codecov patch report seems inaccurate, and someone has reported the same issue. |
The result still looks confusing. The patch doesn't touch packetin.go and clickhouse.go but their coverage change. |
The |
|
/test-all |
Fixes: antrea-io#2092 Due to the message size of Openflow, the maximum number of Endpoints for a Service in AntreaProxy is 800. Since Openflow 1.5 is used in Antrea, the operation `insert_buckets` introduced in Openflow 1.5 can be used to create a Service with more than 800 Endpoints. To sync Service with more than 800 Endpoints to OVS, multiple Openflow messages will be sent to OVS in a bundle (the first message uses `add` to sync the OVS group and its corresponding buckets to OVS, other messages use `insert_buckets` to sync other buckets to OVS). Signed-off-by: Hongliang Liu <[email protected]>
hongliangl
force-pushed
the
20220826-openflow1.5
branch
from
b65c3b4
to
dab08c7
Compare
|
/test-all |
|
Could we merge the PR now @tnqn ? |
|
Thanks for merging @tnqn |
…#4167) Fixes: antrea-io#2092 Due to the message size of Openflow, the maximum number of Endpoints for a Service in AntreaProxy is 800. Since Openflow 1.5 is used in Antrea, the operation `insert_buckets` introduced in Openflow 1.5 can be used to create a Service with more than 800 Endpoints. To sync Service with more than 800 Endpoints to OVS, multiple Openflow messages will be sent to OVS in a bundle (the first message uses `add` to sync the OVS group and its corresponding buckets to OVS, other messages use `insert_buckets` to sync other buckets to OVS). Signed-off-by: Hongliang Liu <[email protected]>
Fixes: #2092
Due to the message size of Openflow, the maximum number of Endpoints for
a Service in AntreaProxy is 800. Since Openflow 1.5 is used in Antrea,
the operation
insert_bucketsintroduced in Openflow 1.5 can be used tocreate a Service with more than 800 Endpoints. To sync Service with more
than 800 Endpoints to OVS, multiple Openflow messages will be sent to
OVS in a bundle (the first message uses
addto sync the OVS group andits corresponding buckets to OVS, other messages use
insert_bucketstosync other buckets to OVS).
Signed-off-by: Hongliang Liu [email protected]