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

Openflow1.5 integration #3770

Merged
merged 1 commit into from
Aug 8, 2022
Merged

Openflow1.5 integration #3770

merged 1 commit into from
Aug 8, 2022

Conversation

wenyingd
Copy link
Contributor

@wenyingd wenyingd commented May 11, 2022

  1. Use set_field action to replace the original "load" action, and use "copy_field" to replace "move"
  2. MatchField "NXM_NX_REGX" is removed from the Match in packetIn message, and message
    "OXM_CLASS_PACKET_REGS" is used in OpenFlow1.5 instead. OVS 64bit xreg is actually used in
    this mesage. A convension from OVS 32bit reg to 64bit xreg is needed when trying to identify
    the match settings.
  3. The type of "Data" field in PacketIn and PacketOut mesage in OpenFlow1.5 uses "Message" instead
    of the original "Ethernet". A type cast is needed to parse or set Ethernet packet in these two
    messages.
  4. Integration test is modified as the action changes.

Signed-off-by: wenyingd wenyingd@vmware.com

@wenyingd wenyingd force-pushed the openflow1.5 branch 2 times, most recently from 5710470 to 59fcd61 Compare May 12, 2022 12:21
@tnqn tnqn added the action/release-note Indicates a PR that should be included in release notes. label May 13, 2022
@tnqn tnqn added this to the Antrea v1.7 release milestone May 13, 2022
@wenyingd wenyingd removed this from the Antrea v1.7 release milestone May 13, 2022
@wenyingd wenyingd force-pushed the openflow1.5 branch 4 times, most recently from 693b0bd to 297bcd0 Compare May 25, 2022 13:53
@codecov-commenter
Copy link

codecov-commenter commented May 25, 2022

Codecov Report

Merging #3770 (5819054) into main (2a37aec) will increase coverage by 1.42%.
The diff coverage is 74.19%.

❗ Current head 5819054 differs from pull request most recent head e416bcf. Consider uploading reports for the commit e416bcf to get more accurate results

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #3770      +/-   ##
==========================================
+ Coverage   64.47%   65.89%   +1.42%     
==========================================
  Files         294      307      +13     
  Lines       44391    44515     +124     
==========================================
+ Hits        28622    29335     +713     
+ Misses      13441    12811     -630     
- Partials     2328     2369      +41     
Flag Coverage Δ *Carryforward flag
e2e-tests 59.15% <ø> (?)
kind-e2e-tests 50.91% <67.74%> (ø) Carriedforward from 2a37aec
unit-tests 44.35% <14.11%> (ø) Carriedforward from 2a37aec

*This pull request uses carry forward flags. Click here to find out more.

Impacted Files Coverage Δ
pkg/agent/controller/networkpolicy/packetin.go 73.94% <0.00%> (ø)
pkg/agent/openflow/multicast.go 0.00% <0.00%> (ø)
pkg/agent/openflow/packetin.go 63.63% <ø> (ø)
pkg/agent/openflow/pipeline.go 73.13% <ø> (ø)
pkg/ovs/openflow/ofctrl_flow.go 48.42% <25.00%> (ø)
pkg/ovs/openflow/ofctrl_meter.go 45.45% <28.57%> (ø)
pkg/agent/controller/traceflow/packetin.go 63.72% <40.00%> (ø)
pkg/agent/multicast/mcast_discovery.go 65.01% <40.00%> (ø)
pkg/ovs/openflow/ofctrl_builder.go 59.32% <44.44%> (ø)
pkg/agent/openflow/client.go 67.50% <50.00%> (ø)
... and 53 more

@wenyingd wenyingd force-pushed the openflow1.5 branch 2 times, most recently from 24f4364 to 3860144 Compare May 26, 2022 07:06
@wenyingd
Copy link
Contributor Author

/test-ipv6-only-e2e

@lgtm-com
Copy link

lgtm-com bot commented May 26, 2022

This pull request introduces 1 alert when merging 3860144 into 6d3036b - view on LGTM.com

new alerts:

  • 1 for Useless assignment to local variable

@wenyingd wenyingd force-pushed the openflow1.5 branch 2 times, most recently from 62b6b8b to dde9b04 Compare May 27, 2022 05:50
@wenyingd
Copy link
Contributor Author

/test-all
/test-windows-all
/test-ipv6-only-all
/test-ipv6-all

@XinShuYang
Copy link
Contributor

/test-ipv6-only-e2e
/test-ipv6-e2e

@wenyingd
Copy link
Contributor Author

/test-windows-all

@wenyingd
Copy link
Contributor Author

/test-all

@wenyingd
Copy link
Contributor Author

/test-windows-all

@wenyingd
Copy link
Contributor Author

/test-all
/test-windows-all
/test-ipv6-all
/test-ipv6-only-all

@wenyingd
Copy link
Contributor Author

wenyingd commented Jun 2, 2022

/test-all
/test-windows-all

@wenyingd
Copy link
Contributor Author

wenyingd commented Jun 6, 2022

/test-all
/test-windows-all
/test-ipv6-all
/test-ipv6-only-all

@wenyingd
Copy link
Contributor Author

wenyingd commented Jun 8, 2022

/test-ipv6-e2e
/test-flexible-ipam-e2e
/test-ipv6-only-e2e
/test-windows-proxyall-e2e

@wenyingd
Copy link
Contributor Author

wenyingd commented Aug 2, 2022

/test-all
/test-windows-all
/test-ipv6-only-all
/test-ipv6-all
/test-all-features-conformance
/test-flexible-ipam-e2e
/test-multicluster-e2e

@wenyingd wenyingd force-pushed the openflow1.5 branch 2 times, most recently from b310c36 to 8808794 Compare August 2, 2022 12:20
@wenyingd
Copy link
Contributor Author

wenyingd commented Aug 3, 2022

/test-all
/test-windows-all
/test-ipv6-only-all
/test-ipv6-all
/test-all-features-conformance
/test-flexible-ipam-e2e
/test-multicluster-e2e

@wenyingd
Copy link
Contributor Author

wenyingd commented Aug 3, 2022

/test-all
/test-windows-all
/test-ipv6-only-all
/test-ipv6-all
/test-all-features-conformance
/test-flexible-ipam-e2e
/test-multicluster-e2e

@wenyingd wenyingd requested a review from tnqn August 3, 2022 09:11
@wenyingd
Copy link
Contributor Author

wenyingd commented Aug 3, 2022

/test-all

@wenyingd
Copy link
Contributor Author

wenyingd commented Aug 4, 2022

/test-all
/test-windows-all
/test-ipv6-only-all
/test-ipv6-all
/test-all-features-conformance
/test-flexible-ipam-e2e
/test-multicluster-e2e

pkg/ovs/openflow/ofctrl_action.go Show resolved Hide resolved
Comment on lines 348 to 352
binary.BigEndian.PutUint32(maskBytes[:], mask)
pktMarkField.Mask = util.NewBuffer(maskBytes[:])
valueData = valueData << rng.Offset()
}
binary.BigEndian.PutUint32(valueBytes[:], valueData)
Copy link
Member

Choose a reason for hiding this comment

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

I meant why we create new slices with [:] so many times, but now I see valueBytes and maskBytes are arrays.
Since the method mainly use slices, should it just declare slices in the beginning, instead of declaring arrays and keep creating slices from it?

valueBytes := make([]byte, 4)
maskBytes := make([]byte, 4)

@@ -319,6 +332,9 @@ func (b *ofPacketOutBuilder) Done() *ofctrl.PacketOut {
klog.Errorf("Invalid PacketOutBuilder: IP header and IPv6 header are not allowed to exist at the same time")
return nil
}
if b.pktOut.InPort == 0 {
b.pktOut.InPort = openflow15.P_CONTROLLER
Copy link
Member

Choose a reason for hiding this comment

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

Doesn't ofnet set it when it's 0?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Originally ofnet does not have set default value in the case Inport is 0. So I added the logic in antrea code. But with the final code, ofnet uses openflow15.P_CONTROLLER in this case. I removed this code in antrea with my latest update.

1. Consume messages defined in OpenFlow1.5
2. Use set_field instead of load, and use copy_field instead of move
3. The type of PacketIn.Data and PacketOut.Data is changed to Buffer in
   libOpenflow, so a conversion from Buffer to Ethernet is needed in
   both logic and tests
4. Parse matches set NXM_NX_REGX via message OXM_CLASS_PACKET_REGS in
   packetIn message. xreg (64bit) is used in OpenFlow1.5 in PacketIn to
   provide OVS register settings, a convention from xreg to reg (32bit)
   is used in functions.
5. Meter works as an Action instead of Instruction in OpenFlow1.5. So
   Antrea needs to apply MeterAction if the meter id is expected to be
   consumed in flow actions.

Signed-off-by: wenyingd <wenyingd@vmware.com>
@wenyingd
Copy link
Contributor Author

wenyingd commented Aug 5, 2022

/test-all
/test-windows-all
/test-ipv6-only-all
/test-ipv6-all
/test-all-features-conformance
/test-flexible-ipam-e2e
/test-multicluster-e2e
/test-multicast-e2e

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

@wenyingd
Copy link
Contributor Author

wenyingd commented Aug 8, 2022

/test-flexible-ipam-e2e
/test-ipv6-e2e
/test-ipv6-only-all
/test-multicast-e2e

@wenyingd
Copy link
Contributor Author

wenyingd commented Aug 8, 2022

/test-integration

@wenyingd
Copy link
Contributor Author

wenyingd commented Aug 8, 2022

/test-ipv6-only-all

@tnqn
Copy link
Member

tnqn commented Aug 8, 2022

/skip-integration tested manually

@wenyingd
Copy link
Contributor Author

wenyingd commented Aug 8, 2022

/test-ipv6-e2e

@wenyingd
Copy link
Contributor Author

wenyingd commented Aug 8, 2022

/test-ipv6-only-all

@wenyingd
Copy link
Contributor Author

wenyingd commented Aug 8, 2022

/test-ipv6-only-conformance
/test-ipv6-only-e2e

@wenyingd
Copy link
Contributor Author

wenyingd commented Aug 8, 2022

/test-ipv6-only-e2e

1 similar comment
@wenyingd
Copy link
Contributor Author

wenyingd commented Aug 8, 2022

/test-ipv6-only-e2e

@tnqn tnqn merged commit 7ec0860 into antrea-io:main Aug 8, 2022
@wenyingd wenyingd deleted the openflow1.5 branch August 15, 2022 03:34
@antoninbas antoninbas mentioned this pull request Aug 31, 2022
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.

6 participants