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

Remove unused OF tables and flows on Windows #3138

Merged
merged 1 commit into from Feb 24, 2022

Conversation

hongliangl
Copy link
Contributor

@hongliangl hongliangl commented Dec 14, 2021

On Windows Node:

  • Since all traffic from uplink interface is output to bridge
    local interface directly instead of being resubmitted to
    UplinkTable, flows on UplinkTables are no longer needed.
  • Since SNAT is performed on Windows host, flows in
    SNATTable which are used to perform SNAT are no longer needed.

Another update is to add function CleanOFTableCache to reset
ofTableCache and this function is only used in integration tests.

Signed-off-by: Hongliang Liu lhongliang@vmware.com

@codecov-commenter
Copy link

codecov-commenter commented Dec 14, 2021

Codecov Report

Merging #3138 (b90d195) into main (1b08a61) will decrease coverage by 10.40%.
The diff coverage is n/a.

❗ Current head b90d195 differs from pull request most recent head b15dfbb. Consider uploading reports for the commit b15dfbb to get more accurate results
Impacted file tree graph

@@             Coverage Diff             @@
##             main    #3138       +/-   ##
===========================================
- Coverage   59.79%   49.39%   -10.41%     
===========================================
  Files         306      302        -4     
  Lines       26178    36319    +10141     
===========================================
+ Hits        15654    17940     +2286     
- Misses       8807    16676     +7869     
+ Partials     1717     1703       -14     
Flag Coverage Δ
e2e-tests 49.39% <ø> (?)
kind-e2e-tests ?
unit-tests ?

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

Impacted Files Coverage Δ
pkg/agent/agent.go 50.00% <ø> (-1.35%) ⬇️
pkg/agent/apiserver/apiserver.go 66.30% <ø> (-3.27%) ⬇️
pkg/agent/client.go 76.74% <ø> (-0.68%) ⬇️
pkg/agent/config/node_config.go 88.88% <ø> (-11.12%) ⬇️
pkg/agent/consistenthash/consistenthash.go 55.00% <ø> (ø)
pkg/agent/controller/networkpolicy/fqdn.go 71.42% <ø> (-4.56%) ⬇️
...ntroller/networkpolicy/networkpolicy_controller.go 65.78% <ø> (-5.86%) ⬇️
pkg/agent/controller/networkpolicy/reconciler.go 64.39% <ø> (-12.60%) ⬇️
...gent/controller/noderoute/node_route_controller.go 51.42% <ø> (-5.41%) ⬇️
...g/agent/controller/serviceexternalip/controller.go 0.00% <ø> (ø)
... and 318 more

pkg/agent/openflow/client.go Outdated Show resolved Hide resolved
pkg/agent/openflow/client.go Outdated Show resolved Hide resolved
Copy link
Contributor

@jianjuns jianjuns left a comment

Choose a reason for hiding this comment

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

LGTM

@hongliangl hongliangl changed the title Remove unused OF tables and flow on Windows Remove unused OF tables and flows on Windows Dec 15, 2021
@XinShuYang
Copy link
Contributor

/test-windows-e2e

@hongliangl hongliangl force-pushed the remove-unused-tables branch 2 times, most recently from 14e78a0 to 76bc57f Compare January 11, 2022 12:03
pkg/agent/openflow/client.go Outdated Show resolved Hide resolved
pkg/agent/openflow/client.go Show resolved Hide resolved
pkg/agent/openflow/pipeline.go Outdated Show resolved Hide resolved
pkg/agent/openflow/pipeline.go Outdated Show resolved Hide resolved
pkg/agent/openflow/pipeline.go Outdated Show resolved Hide resolved
pkg/agent/openflow/pipeline.go Outdated Show resolved Hide resolved
jianjuns
jianjuns previously approved these changes Jan 12, 2022
@hongliangl
Copy link
Contributor Author

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

@hongliangl
Copy link
Contributor Author

/test-e2e to

@hongliangl
Copy link
Contributor Author

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

@hongliangl
Copy link
Contributor Author

/test-e2e

@hongliangl
Copy link
Contributor Author

/test-windows-conformance
/test-windows-e2e
/test-windows-networkpolicy
/test-windows-proxyall-e2e

@hongliangl
Copy link
Contributor Author

/test-integration

@hongliangl
Copy link
Contributor Author

/test-e2e

@hongliangl
Copy link
Contributor Author

/test-integration

@hongliangl
Copy link
Contributor Author

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

@hongliangl
Copy link
Contributor Author

/test-all-features-conformance

@hongliangl
Copy link
Contributor Author

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

@hongliangl
Copy link
Contributor Author

@jianjuns @tnqn @wenyingd Do you have any more comments about this PR? Thanks.

@hongliangl
Copy link
Contributor Author

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

jianjuns
jianjuns previously approved these changes Feb 14, 2022
Copy link
Contributor

@jianjuns jianjuns left a comment

Choose a reason for hiding this comment

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

No further comment from me.

@hongliangl
Copy link
Contributor Author

No further comment from me.

Thanks!

@hongliangl
Copy link
Contributor Author

@tnqn @wenyingd Do you have any further comments? Thanks.

@hongliangl
Copy link
Contributor Author

/test-windows-all

@hongliangl
Copy link
Contributor Author

/test-ipv6-conformance
/test-ipv6-e2e
/test-ipv6-networkpolicy
/test-ipv6-only-conformance
/test-ipv6-only-e2e
/test-ipv6-only-networkpolicy
/test-flexible-ipam-e2e

pkg/agent/openflow/client.go Show resolved Hide resolved
pkg/agent/openflow/client.go Show resolved Hide resolved
pkg/agent/openflow/client.go Outdated Show resolved Hide resolved
pkg/agent/openflow/pipeline.go Show resolved Hide resolved
@hongliangl
Copy link
Contributor Author

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

@hongliangl
Copy link
Contributor Author

/test-windows-proxyall-e2e

@hongliangl
Copy link
Contributor Author

@tnqn Do you have any further comments? Thanks.

Cookie(c.cookieAllocator.Request(category).Raw()).
Done())
for _, localSubnet := range localSubnets {
flows = append(flows, ClassifierTable.BuildFlow(priorityHigh).MatchProtocol(binding.ProtocolIP).
Copy link
Member

Choose a reason for hiding this comment

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

This is wrong. localSubnets includes one IPv4 subnet and one IPv6 subnet, setting protocol to binding.ProtocolIP. for both subnets will not work. I don't think windows or flexiable IPAM supports IPv6, why don't just add the IPv4 check back and keep the IPv6 TODO as is?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks, updated.

On Windows Node:
  - Since all traffic from uplink interface is output to bridge
local interface directly instead of being resubmitted to
UplinkTable, flows on UplinkTables are no longer needed.
  - Since SNAT is performed on Windows host, flows in
SNATTable which are used to perform SNAT are no longer needed.

Another update is to add function CleanOFTableCache to reset
ofTableCache and this function is only used in integration tests.

Signed-off-by: Hongliang Liu <lhongliang@vmware.com>
@hongliangl
Copy link
Contributor Author

/test-windows-all

@hongliangl
Copy link
Contributor Author

/test-all
/test-windows-all

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 Feb 24, 2022

/test-integration

@tnqn tnqn merged commit ceae206 into antrea-io:main Feb 24, 2022
bangqipropel pushed a commit to bangqipropel/antrea that referenced this pull request Mar 2, 2022
On Windows Node:
  - Since all traffic from uplink interface is output to bridge
local interface directly instead of being resubmitted to
UplinkTable, flows on UplinkTables are no longer needed.
  - Since SNAT is performed on Windows host, flows in
SNATTable which are used to perform SNAT are no longer needed.

Another update is to add function CleanOFTableCache to reset
ofTableCache and this function is only used in integration tests.

Signed-off-by: Hongliang Liu <lhongliang@vmware.com>
@hongliangl hongliangl deleted the remove-unused-tables branch April 28, 2022 11:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants