-
Notifications
You must be signed in to change notification settings - Fork 366
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
[IPv6] Support no-encap mode in dual-stack setup #2436
Conversation
Codecov Report
@@ Coverage Diff @@
## main #2436 +/- ##
===========================================
+ Coverage 42.51% 60.26% +17.74%
===========================================
Files 148 283 +135
Lines 18247 22544 +4297
===========================================
+ Hits 7758 13586 +5828
+ Misses 9789 7514 -2275
- Partials 700 1444 +744
Flags with carried forward coverage won't be shown. Click here to find out more.
|
6754390
to
35a88dc
Compare
4760ac3
to
5094e9d
Compare
/test-all /test-ipv6-e2e /test-ipv6-only-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.
Could you add a short description in the commit message on what are added or what were missing to support no-encap mode in dual stack?
and link to the appropriate issue |
pkg/agent/util/net.go
Outdated
for _, ip := range localIPs { | ||
if ipNet.IP.Equal(ip) { | ||
iface = &linkList[i] | ||
if ip.To4() == nil { | ||
v6IPNet = ipNet | ||
} else { | ||
v4IPNet = ipNet | ||
} | ||
} |
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 possible for the 2 different IPs for the Node (v4 / v6) to correspond to 2 different interfaces?
the function comment talks about "associated devices from IP", but we only return a single interface it seems
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.
Discussed with Wenying, we think that maybe it is unnecessary to consider such case. In such case, user should consider have 2 clusters with such interfaces.
The comment is updated.
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 restriction makes sense. Especially in a scenario where we may need to move the uplink to the OVS bridge. However, do you think we can log a clean error in that case and fail agent initialization?
92118f6
to
34602a4
Compare
/test-all /test-ipv6-all /test-ipv6-only-all |
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.
Two comments, otherwise LGTM
pkg/agent/memberlist/cluster.go
Outdated
if nodeAddrs.IPv4 == nil { | ||
return "", fmt.Errorf("there is no IPv4 address from K8s Node") | ||
} | ||
member := fmt.Sprintf("%s:%d", nodeAddrs.IPv4, c.bindPort) |
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 this PR can keep egress feature work in IPv6 cluster easily if it just handles the case like ipsec:
nodeAddr := nodeAddrs.IPv4
if nodeAddr == nil {
nodeAddr = nodeAddrs.IPv6
}
member := fmt.Sprintf("%s:%d", nodeAddr, c.bindPort)
@tnqn for the comment in
Please correct me if wrong. |
Thanks for bringing this issue. I think you are right, but I wonder how it worked when Wenqi tested it on IPv6. @wenqiq can you confirm whether "[]" is needed? Did you use dual-stack or pure IPv6 cluster when testing this? |
Given the single IPv6 stack case may be not working before this PR, I won't block it because of this. @lzhecheng Please resolve the conflict then we can merge this. |
* Extend single IP address to IPv4 and IPv6 ones for NodeAddr and NodeTransportAddr in nodeConfig * Refactor occurrences of Node IP address to adjust to 2 IP: agent initialization, node route * Still use one IP for IPv6/dual-stack not supported cases: Windows, NPL * Update tests Signed-off-by: Zhecheng Li <lzhecheng@vmware.com> Related: antrea-io#2426
@tnqn @lzhecheng I think you are right. It has been tested in pure IPv4 and dual-stack cluster but not pure IPv6 cluster.I'm still adding some test cases about IPv6. I think just
is OK. As the bindPort has been seted and port will added when joinning memberlist.
|
/test-all /test-ipv6-all /test-ipv6-only-all |
@wenqiq the test code you refered have "[]" for IPv6 address while |
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-e2e /test-ipv6-only-conformance /test-ipv6-only-e2e /test-windows-conformance /test-windows-networkpolicy |
@tnqn There're two failed checks. I considered the test results of this PR and others, I think Windows-networkpolicy is flaky and this PR has little to do with it. The failure case in ipv6-only-e2e is I think we can get this PR merged. Please share your idea. |
@lzhecheng OK, we can merge this. Please file an issue for FQDN failure, @GraysonWu may take a look. |
@lzhecheng what about TestFlowAggregator failures? |
@tnqn It didn't fail. It is the no. 475 of ipv6-only-e2e test right? There's only 1 failure.
|
You are right. I was looking at wrong build. Then obviously FQDN IPv6 test failed on all PRs, meaning it's not related to this PR. I'm merging this. |
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 nodeConfig.NodeIPv4Addr
is nil in pure IPv6 cluster which would cause panic in agent.
panic: runtime error: invalid memory address or nil pointer dereference
[signal SIGSEGV: segmentation violation code=0x1 addr=0x0 pc=0x1b9b101]
goroutine 1 [running]:
main.run(0xc00036a060)
/home/ubuntu/go/src/antrea/cmd/antrea-agent/agent.go:233 +0x1841
main.newAgentCommand.func1(0xc0001fa840, {0xc0001ae800, 0x0, 0x8})
/home/ubuntu/go/src/antrea/cmd/antrea-agent/main.go:57 +0x1ef
github.com/spf13/cobra.(*Command).execute(0xc0001fa840, {0xc000132010, 0x8, 0x8})
/home/ubuntu/go/src/antrea/vendor/github.com/spf13/cobra/command.go:854 +0x5f8
github.com/spf13/cobra.(*Command).ExecuteC(0xc0001fa840)
/home/ubuntu/go/src/antrea/vendor/github.com/spf13/cobra/command.go:958 +0x3ad
github.com/spf13/cobra.(*Command).Execute(...)
/home/ubuntu/go/src/antrea/vendor/github.com/spf13/cobra/command.go:895
main.main()
/home/ubuntu/go/src/antrea/cmd/antrea-agent/main.go:37 +0x4a
Fixed in #2655
…antrea-io#2196 Fixed agent start fail nil panic in pure IPv6 cluster. Related antrea-io#2436 Signed-off-by: Wenqi Qiu <wenqiq@vmware.com>
…antrea-io#2196 Fixed agent start fail nil panic in pure IPv6 cluster. Related antrea-io#2436 Signed-off-by: Wenqi Qiu <wenqiq@vmware.com>
…antrea-io#2196 Fixed agent start fail nil panic in pure IPv6 cluster. Related antrea-io#2436 Signed-off-by: Wenqi Qiu <wenqiq@vmware.com>
Signed-off-by: Zhecheng Li lzhecheng@vmware.com
Related: #2426