Skip to content
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

[multicast] fix the issue multicast does not work if antreaPolicy is disabled #3807

Merged
merged 1 commit into from May 18, 2022

Conversation

liu4480
Copy link
Contributor

@liu4480 liu4480 commented May 18, 2022

Currently, packetIn reason only supports 0 and 1. There is no more available packetIn reasons
as traceflow and antreaPolicy use both. Multicast reuses packetIn reason with antreaPolicy, and
uses different register bit to indicate the real reason. There is no queue for packetIn reason
'PacketInReasonNP' if antreaPloicy is disabled. And multicast will not work then.

This patch fixes #3808

@liu4480 liu4480 requested review from tnqn and wenyingd May 18, 2022 11:34
wenyingd
wenyingd previously approved these changes May 18, 2022
Copy link
Contributor

@wenyingd wenyingd left a 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, please create an issue to link this fix.

@wenyingd
Copy link
Contributor

In the commit message, Multicast doesn't use a different register, but use a different register bit to mark the packet is for Multicast purpose.

@codecov-commenter
Copy link

codecov-commenter commented May 18, 2022

Codecov Report

Merging #3807 (189fe7d) into main (3a51abe) will increase coverage by 1.57%.
The diff coverage is 66.66%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #3807      +/-   ##
==========================================
+ Coverage   63.38%   64.96%   +1.57%     
==========================================
  Files         280      280              
  Lines       39750    39750              
==========================================
+ Hits        25197    25825     +628     
+ Misses      12593    11937     -656     
- Partials     1960     1988      +28     
Flag Coverage Δ
e2e-tests 46.02% <66.66%> (?)
kind-e2e-tests 51.77% <66.66%> (+0.61%) ⬆️
unit-tests 43.75% <0.00%> (+<0.01%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
pkg/agent/openflow/client.go 70.28% <ø> (+2.41%) ⬆️
pkg/agent/openflow/packetin.go 63.63% <66.66%> (ø)
pkg/agent/cniserver/ipam/antrea_ipam.go 74.02% <0.00%> (-1.74%) ⬇️
pkg/agent/controller/networkpolicy/priority.go 90.71% <0.00%> (-0.43%) ⬇️
pkg/agent/cniserver/server.go 67.46% <0.00%> (+0.20%) ⬆️
pkg/agent/nodeportlocal/k8s/npl_controller.go 61.44% <0.00%> (+0.23%) ⬆️
...ntroller/networkpolicy/networkpolicy_controller.go 70.17% <0.00%> (+0.43%) ⬆️
pkg/agent/cniserver/pod_configuration.go 55.43% <0.00%> (+0.83%) ⬆️
pkg/ovs/openflow/ofctrl_packetout.go 80.57% <0.00%> (+0.86%) ⬆️
pkg/controller/ipam/antrea_ipam_controller.go 76.41% <0.00%> (+1.31%) ⬆️
... and 22 more

@@ -638,7 +638,8 @@ func run(o *Options) error {
if features.DefaultFeatureGate.Enabled(features.Traceflow) {
packetInReasons = append(packetInReasons, uint8(openflow.PacketInReasonTF))
}
if features.DefaultFeatureGate.Enabled(features.AntreaPolicy) {
if features.DefaultFeatureGate.Enabled(features.AntreaPolicy) ||
features.DefaultFeatureGate.Enabled(features.Multicast) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think a more proper fix is to remove the first argument of StartPacketInHandler and just get the reasons from the keys of c.packetInHandlers to avoid all the redundancy and inconsistency.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@wenyingd @hongliangl does the above way work?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think a more proper fix is to remove the first argument of StartPacketInHandler and just get the reasons from the keys of c.packetInHandlers to avoid all the redundancy and inconsistency.

I prefer this way.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes, it looks better. updated

…disabled

Currently, packetIn reason only supports 0 and 1. There is no more available packetIn reasons
as traceflow and antreaPolicy use both. Multicast reuses packetIn reason with antreaPolicy, and
uses a different register bit to indicate the real reason. There is no queue for packetIn reason
'PacketInReasonNP' if antreaPloicy is disabled. And multicast will not work then.

This patch fixes antrea-io#3808

Signed-off-by: Bin Liu <biliu@vmware.com>
Copy link
Member

@tnqn tnqn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@tnqn
Copy link
Member

tnqn commented May 18, 2022

/test-all

@tnqn tnqn added the action/release-note Indicates a PR that should be included in release notes. label May 18, 2022
@tnqn tnqn added this to the Antrea v1.7 release milestone May 18, 2022
@tnqn tnqn merged commit 20663c2 into antrea-io:main May 18, 2022
@liu4480 liu4480 deleted the fix-multicast branch May 26, 2022 11:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
action/release-note Indicates a PR that should be included in release notes.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

multicast does not work if antreaPolicy is disabled
5 participants