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

Fix ovs group id conflict when dual stack is enabled #2317

Merged
merged 1 commit into from
Jun 29, 2021

Conversation

hongliangl
Copy link
Contributor

@hongliangl hongliangl commented Jun 28, 2021

When dual stack is enabled, there will be IPv4 and IPv6 AntreaProxy. The IPv4
AntreaProxy and IPv6 AntreaProxy don't share the same GroupCounter. When a
IPv6 Service is created by IPv6 AntreaProxy, the GroupID generated may be
duplicated with a GroupID used by IPv4 Service. GroupCounter of IPv6 AntreaProxy
can start with 0x10000000 to avoid above issue.

Signed-off-by: Hongliang Liu [email protected]

@hongliangl hongliangl force-pushed the ipv6-proxier-group branch 2 times, most recently from 89d1008 to 1da2453 Compare June 28, 2021 02:00
@codecov-commenter
Copy link

codecov-commenter commented Jun 28, 2021

Codecov Report

Merging #2317 (200258a) into main (2d19196) will increase coverage by 3.39%.
The diff coverage is 60.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #2317      +/-   ##
==========================================
+ Coverage   62.04%   65.44%   +3.39%     
==========================================
  Files         281      281              
  Lines       21746    21749       +3     
==========================================
+ Hits        13493    14234     +741     
+ Misses       6849     6061     -788     
- Partials     1404     1454      +50     
Flag Coverage Δ
e2e-tests 55.50% <60.00%> (?)
kind-e2e-tests 52.40% <60.00%> (-0.01%) ⬇️
unit-tests 41.61% <0.00%> (ø)

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

Impacted Files Coverage Δ
pkg/agent/proxy/types/groupcounter.go 87.50% <50.00%> (-7.74%) ⬇️
pkg/agent/proxy/proxier.go 63.89% <100.00%> (ø)
pkg/agent/flowexporter/connections/connections.go 80.48% <0.00%> (-7.32%) ⬇️
pkg/agent/flowexporter/exporter/exporter.go 78.66% <0.00%> (-0.92%) ⬇️
pkg/ovs/openflow/ofctrl_bridge.go 50.00% <0.00%> (+0.34%) ⬆️
pkg/agent/openflow/network_policy.go 76.41% <0.00%> (+0.59%) ⬆️
...ntroller/networkpolicy/networkpolicy_controller.go 70.09% <0.00%> (+0.64%) ⬆️
pkg/agent/cniserver/pod_configuration.go 54.68% <0.00%> (+0.78%) ⬆️
pkg/controller/egress/store/egressgroup.go 1.38% <0.00%> (+1.38%) ⬆️
pkg/ovs/openflow/ofctrl_flow.go 47.36% <0.00%> (+1.75%) ⬆️
... and 43 more

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.

I am ok with the approach, but also wonder how if we build a separate ID allocator, like idAllocator in Egress (and could NetworkPolicy, Egress, group share an ID allocator implementation?). @tnqn

antoninbas
antoninbas previously approved these changes Jun 28, 2021
Copy link
Contributor

@antoninbas antoninbas 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 when this is merged, please cherry-pick this patch (https://github.com/antrea-io/antrea/blob/main/docs/contributors/cherry-picks.md) to any impacted release branch starting from 0.13.

@tnqn
Copy link
Member

tnqn commented Jun 29, 2021

I am ok with the approach, but also wonder how if we build a separate ID allocator, like idAllocator in Egress (and could NetworkPolicy, Egress, group share an ID allocator implementation?). @tnqn

I think later we should probably have single group ID allocator which allocates ID and tracks used IDs only and let consumers to maintain their own resource and ID mapping as the group is likely to be used for other purposes (like multicast group) only, we need to ensure the ID is globally unique.

It's a good idea to share the core logic of IP allocation among the current allocators. I will consider refactoring them in the future. @jianjuns

@tnqn
Copy link
Member

tnqn commented Jun 29, 2021

@hongliangl Do the current CI tests cover this scenario?

@tnqn
Copy link
Member

tnqn commented Jun 29, 2021

/test-all

@tnqn
Copy link
Member

tnqn commented Jun 29, 2021

@hongliangl DCO verification failed

@hongliangl
Copy link
Contributor Author

@hongliangl DCO verification failed

Fixed, thanks.

@hongliangl
Copy link
Contributor Author

@hongliangl Do the current CI tests cover this scenario?

@tnqn In theory, current CI tests don't cover this scenario yet. I found this issue by enabling dual stack AntreaProxy on testbed.

@tnqn
Copy link
Member

tnqn commented Jun 29, 2021

/test-all

@hongliangl can you remove the ending period from the commit message? maybe the title could be shorter to make it look neat in git tools: "Fix ovs group id conflict when dual stack is enabled"

@tnqn
Copy link
Member

tnqn commented Jun 29, 2021

@hongliangl Do the current CI tests cover this scenario?

@tnqn In theory, current CI tests don't cover this scenario yet. I found this issue by enabling dual stack AntreaProxy on testbed.

Can running upstream conformance test in dual-stack cluster with AntreaProxy enabled catch it?

When dual stack is enabled, there will be IPv4 and IPv6 AntreaProxy. The IPv4
AntreaProxy and IPv6 AntreaProxy don't share the same GroupCounter. When a
IPv6 Service is created by IPv6 AntreaProxy, the GroupID generated may be
duplicated with a GroupID used by IPv4 Service. GroupCounter of IPv6
AntreaProxy can start with 0x10000000 to avoid above issue.

Signed-off-by: Hongliang Liu <[email protected]>
@hongliangl hongliangl changed the title Fix that proxy ovs group id may be duplicated when dual stack is enabled Fix ovs group id conflict when dual stack is enabled Jun 29, 2021
@tnqn
Copy link
Member

tnqn commented Jun 29, 2021

/test-all

@tnqn
Copy link
Member

tnqn commented Jun 29, 2021

/skip-e2e

e2e succeeded: https://jenkins.antrea-ci.rocks/job/antrea-e2e-for-pull-request/2776/. The latest update only changed commit message.

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.

5 participants