-
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
Support dumping OVS flows for a Service with antctl #1877
Conversation
jianjuns
commented
Feb 17, 2021
// modifying the proxy.Provider interface. | ||
type Proxier interface { | ||
// GetProxyProvider returns the real proxy Provider. | ||
GetProxyProvider() k8sproxy.Provider |
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 we could just extend the k8sproxy.Provider
instead of having this method.
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 did not know if you changed k8sproxy.Provider already or not (that is why I hope you can describe all the changes made the kube-proxy pkgs somewhere). However, I feel always better to reduce the changes to copied code to make code sync easier.
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.
Yes, the changes are described at the top of the files in third-party.
The change I actually suggested from this comment is like
type Proxier interface {
k8sproxy.Provider
}
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.
Are you suggesting:
type Proxier interface {
k8sproxy.Provider
GetServiceFlowKeys(serviceName, namespace string) ([]string, []binding.GroupIDType)
}
I thought about it, but not sure if mockgen works or not. Do you know?
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 just tried and the mock contains all defined methods.
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.
Thanks. But I recalled the problem is for NewDualStackProxier(). How would you return an instance that implements k8sproxy.Provider then?
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 we should then use our own interface instead of the Provider
.
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 walked through the code and found there is no usage of Provider in the third party package.
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.
That means we need to rewrite metaProxier too? I still hope to reduce changes to the copied code.
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.
Make sense, I missed that part. Then current implementation looks good.
|
||
p.serviceEndpointsMapsMutex.Lock() |
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 sugges to using a function to wrap this snippet.
func() {
lock()
defer unlock()
...
}()
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.
Make sense. In this case, as really not much after the lock protection in this func, I just change to put "defer p.serviceEndpointsMapsMutex.Unlock()" after "p.serviceEndpointsMapsMutex.Lock()". Let me know what you think.
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.
Sounds good.
@@ -480,6 +488,17 @@ func (c *client) UninstallLoadBalancerServiceFromOutsideFlows(svcIP net.IP, svcP | |||
return c.deleteFlows(c.serviceFlowCache, cacheKey) |
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.
@weiqiangt : BTW, I wonder why in InstallEndpointFlows() we pass the extra isIPv6 argument, but not just let net.ParseIP() return a net.IP and check it is v4 or v6 when necessary. Is it for some efficiency consideration?
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.
Yes, for efficiency consideration, otherwise we will create a small bytes slice each time we check. cc @ksamoray.
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.
But in this case, the IP is checked at most twice: one in endpointDNATFlow() and one in hairpinSNATFlow, and the latter is only for local Endpoint. Is it worth to optimize?
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.
Considering parameters passing is simple I thought it is OK. But a second thought is that the cost here is not that critical, I'm fine with the change also.
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.
As endpointDNATFlow() performs To4() anyway for v4, this argument has no importance as To4() is later applied again here
https://github.com/vmware-tanzu/antrea/blob/main/pkg/agent/openflow/pipeline.go#L1830
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.
Thanks for the info guys! So, let us remove the argument then.
36d3029
to
a0e91fb
Compare
return fmt.Sprintf("Service_%s_%d_%s", svcIP, svcPort, protocol) | ||
} | ||
|
||
func (c *client) InstallEndpointFlows(protocol binding.Protocol, endpoints []proxy.Endpoint) error { |
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 removed the isIPv6 argument, as I could not figure out why it is needed. Let me know if I am wrong. @weiqiangt
25d7800
to
b0ea83a
Compare
Codecov Report
@@ Coverage Diff @@
## main #1877 +/- ##
=======================================
Coverage ? 58.79%
=======================================
Files ? 201
Lines ? 17374
Branches ? 0
=======================================
Hits ? 10215
Misses ? 6088
Partials ? 1071
Flags with carried forward coverage won't be shown. Click here to find out more. |
b0ea83a
to
d6fdc09
Compare
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
9464b73
to
d32c258
Compare
/test-windows-conformance |
/test-all |
1 similar comment
/test-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.
LGTM
pkg/agent/proxy/proxier.go
Outdated
if v4Flows == nil { | ||
if v6Flows == nil { | ||
return nil, nil | ||
} | ||
// Not to return nil, even v6Flows is empty. | ||
v4Flows = make([]string, 0, len(v6Flows)) | ||
} |
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 feel like a third parameter would have been clearer, i.e. the function could return (found bool, flowKeys []string, groupIDs []binding.GroupIDType)
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.
Sure. Change to what you suggested.
pkg/ovs/ovsctl/ofctl.go
Outdated
@@ -63,6 +64,27 @@ func (c *ovsCtlClient) DumpTableFlows(table uint8) ([]string, error) { | |||
return c.DumpFlows(fmt.Sprintf("table=%d", table)) | |||
} | |||
|
|||
func (c *ovsCtlClient) DumpGroup(groupID int) (string, error) { | |||
// There seems a bug in ovs-ofctl that dump-groups always returns all | |||
// the groups when using Openflow13, even the group ID is provided. As |
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.
s/even the group ID is provided/even when the group ID is provided
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.
Done.
@@ -63,6 +64,27 @@ func (c *ovsCtlClient) DumpTableFlows(table uint8) ([]string, error) { | |||
return c.DumpFlows(fmt.Sprintf("table=%d", table)) | |||
} | |||
|
|||
func (c *ovsCtlClient) DumpGroup(groupID int) (string, error) { | |||
// There seems a bug in ovs-ofctl that dump-groups always returns 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.
out-of-curiosity did you report the bug?
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.
Not yet. Will do.
d32c258
to
34808a3
Compare
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-all |
/test-all-features-conformance |
Extend Agent /ovsflows API handler to support returning OVS flows for a Service. Extend agent/proxy/proxier to query OVS flow keys and group IDs for a Service. Extend ovs/ovsctl/ofctl to support dumping a specific OVS group. Extend "antctl get ovsflows" command to dump OVS flows for a Service.
34808a3
to
caf5845
Compare
/test-all-features-conformance |
1 similar comment
/test-all-features-conformance |
/test-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.
Approving again after rebase
Extend Agent /ovsflows API handler to support returning OVS flows for a Service. Extend agent/proxy/proxier to query OVS flow keys and group IDs for a Service. Extend ovs/ovsctl/ofctl to support dumping a specific OVS group. Extend "antctl get ovsflows" command to dump OVS flows for a Service.