-
Notifications
You must be signed in to change notification settings - Fork 363
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
Fix missing call of removing groupDb cache when deleting OVS group #4592
Conversation
effaf08
to
fb1e07e
Compare
/test-multicast-e2e |
Codecov Report
@@ Coverage Diff @@
## main #4592 +/- ##
==========================================
+ Coverage 69.75% 69.88% +0.12%
==========================================
Files 400 403 +3
Lines 59450 60115 +665
==========================================
+ Hits 41472 42010 +538
- Misses 15178 15283 +105
- Partials 2800 2822 +22
*This pull request uses carry forward flags. Click here to find out more.
|
fb1e07e
to
15fd036
Compare
/test-multicast-e2e |
@@ -229,10 +229,6 @@ func (b *OFBridge) DeleteGroup(id GroupIDType) error { | |||
if ofctrlGroup == nil { | |||
return nil | |||
} | |||
g := &ofGroup{bridge: b, ofctrl: ofctrlGroup} |
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.
Why remove line 232 to 235, but not directly call OFBridge.DeleteGroup(gID) in the caller?
This change might cause group still exists in OVS when calling OFBridge.DeleteGroup but not sending another separate OF message to remove group.
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.
OFBridge.DeleteGroup
is not called here directly because DeleteOFEntries
will callmetrics.OVSFlowOpsErrorCount.WithLabelValues
metrics.OVSFlowOpsCount.WithLabelValues
for prometheus integration while DeleteGroup don't(and it shouldn't). I have changed DeleteGroup
to allow users to decide whether to do flow deletion and call DeleteOFEntries
and DeleteGroup
both here.
15fd036
to
f8faceb
Compare
/test-multicast-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.
Code change looks good to me, but it looks the UT coverage does not satisfy the requirement.
778bfc4
to
bb275da
Compare
Updated. UT coverage requirement satisfied @wenyingd |
pkg/agent/openflow/client.go
Outdated
return fmt.Errorf("error when deleting Service Endpoints Group %d: %w", groupID, err) | ||
return fmt.Errorf("error when deleting Openflow entries for Service Endpoints Group %d: %w", groupID, err) | ||
} | ||
if err := c.featureService.bridge.DeleteGroup(groupID, false); err != nil { |
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 err := c.featureService.bridge.DeleteGroup(groupID, false); err != nil { | |
if err := c.bridge.DeleteGroup(groupID, false); err != nil { |
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. Thanks
pkg/agent/openflow/client.go
Outdated
return fmt.Errorf("error when deleting Multicast receiver Group %d: %w", groupID, err) | ||
return fmt.Errorf("error when deleting Openflow entries for Multicast receiver Group %d: %w", groupID, err) | ||
} | ||
if err := c.featureMulticast.bridge.DeleteGroup(groupID, false); err != nil { |
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 err := c.featureMulticast.bridge.DeleteGroup(groupID, false); err != nil { | |
if err := c.bridge.DeleteGroup(groupID, false); err != nil { |
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. Thanks
bb275da
to
a9a1292
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
/test-all |
pkg/ovs/openflow/ofctrl_bridge.go
Outdated
@@ -224,14 +224,16 @@ func (b *OFBridge) createGroupWithType(id GroupIDType, groupType ofctrl.GroupTyp | |||
return g | |||
} | |||
|
|||
func (b *OFBridge) DeleteGroup(id GroupIDType) error { | |||
func (b *OFBridge) DeleteGroup(id GroupIDType, deleteFlows bool) error { |
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.
Is it used anywhere that deleteFlows
is true? Another question: why name it deleteFlows
? I didn't see that this is related to any flows.
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 is related to flows. In deleteFlows,
g.Delete()
is called, which calls AddOFEntriesInBundle.
deleteFlows = true is not used in other places. Keeping it as a parameter because I think it is reasonable that flow transactions should be kept at this level. Users can disable it explicitly, not implicitly if the flow transaction is not wanted.
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 'deleteGroupOnOVS'? This parameter is used to decide whether to delete the group on OVS.
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
pkg/ovs/openflow/ofctrl_bridge.go
Outdated
g := &ofGroup{bridge: b, ofctrl: ofctrlGroup} | ||
if err := g.Delete(); err != nil { | ||
return fmt.Errorf("failed to delete the group: %w", err) | ||
if deleteFlows { |
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 you add more comments or PR description? I didn't get that why the issue can be fixed by bypassing this part.
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
I synced with @wenyingd offline about the issue #4575. The root cause is that:
I'm ok with this fix, but could you add more comments about adding parameter |
a9a1292
to
ea6d558
Compare
Thanks for the review. Your understanding is correct. The root cause for the contrived code fix is coming from Prometheus metrics count. Please check #4592 (comment) and the updated comment for detail to see if it is reasonable to you. |
ea6d558
to
3d4e1d5
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.
The seems a serious issue when Multicast is enabled. Please backport it after merged.
pkg/ovs/openflow/ofctrl_bridge.go
Outdated
// DeleteGroup deletes a specified group in groupDb. If deleteGroupOnOVS sets to true, the group deletion transaction | ||
// will be synced to OVS. Note that to record OVS operation in Prometheus, openFlowClient.DeleteOFEntries([]binding.OFEntry{groupCache}) | ||
// should be called in advance, and deleteGroupOnOVS is unnecessary. | ||
func (b *OFBridge) DeleteGroup(id GroupIDType, deleteGroupOnOVS bool) error { |
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 the methods is a little confusing as CreateGroup
only construct group in memory while DeleteGroup
could delete the group in OVS. Methods should be symmetrical so clients can use similar methods to manage the same resource.
Since there isn't even an actual case needing deleteGroupOnOVS
to be true, I think we should just make CreateGroup and DeleteGroup symmetrical.
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
3d4e1d5
to
5c3a140
Compare
pkg/ovs/openflow/ofctrl_bridge.go
Outdated
@@ -224,15 +224,13 @@ func (b *OFBridge) createGroupWithType(id GroupIDType, groupType ofctrl.GroupTyp | |||
return g | |||
} | |||
|
|||
// DeleteGroup deletes a specified group in groupDb. Note that to record OVS operations in Prometheus, openFlowClient.DeleteOFEntries([]binding.OFEntry{groupCache}) |
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.
update comment
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.
name: "delete existed group without flow", | ||
existedGroupID: 20, |
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.
name: "delete existed group without flow", | |
existedGroupID: 20, | |
name: "delete existing group without flow", | |
existingGroupID: 20, |
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
err: nil, | ||
}, | ||
{ | ||
name: "delete non-existed group", |
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.
ditto
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
} { | ||
t.Run(m.name, func(t *testing.T) { | ||
b.ofSwitch = newFakeOFSwitch(b) | ||
b.ofSwitch.NewGroup(uint32(m.existedGroupID), ofctrl.GroupAll) |
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.
why it doesn't use b.CreateGroup
to be symmetrical
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
912b9b5
to
e514b98
Compare
The old group will be reused unexpectedly if groupDb cache managed by OFBridge is not cleared for that group, which causes a new group claims to have a different group type acquiring the old group type. Fixes antrea-io#4575 Signed-off-by: ceclinux <src655@gmail.com>
e514b98
to
4a094af
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
/test-all |
@ceclinux could you backport the PR to applicable releases up to 1.7. |
…ntrea-io#4592) The old group will be reused unexpectedly if groupDb cache managed by OFBridge is not cleared for that group, which causes a new group claims to have a different group type acquiring the old group type. Fixes antrea-io#4575 Signed-off-by: ceclinux <src655@gmail.com>
Fix missing call of removing groupDb cache when deleting OVS group
The old group will be reused unexpectedly if groupDb cache managed
by OFBridge is not cleared for that group, which causes a new group
claims to have a different group type acquiring the old group type.
Fixes #4575