-
Notifications
You must be signed in to change notification settings - Fork 359
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 antctl trace-packet arguments missing issue #5838
Fix antctl trace-packet arguments missing issue #5838
Conversation
Need to backport this to v1.14. |
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 fixing it. Could we add one e2e test to testAntctlAgentLocalAccess? Fixing related bug is a good chance to improve the test coverage.
t.Run("testAntctlAgentLocalAccess", func(t *testing.T) {
testAntctlAgentLocalAccess(t, data)
testAntctlAgentTracePacket(t, data)
})
2155824
to
30c7219
Compare
test/e2e/antctl_test.go
Outdated
@@ -159,6 +160,30 @@ func testAntctlAgentLocalAccess(t *testing.T, data *TestData) { | |||
} | |||
} | |||
|
|||
// testAntctlAgentTracePacket ensures antctl trace-packet is runnable in an agent Pod. | |||
func testAntctlAgentTracePacket(t *testing.T, data *TestData) { |
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.
This test is not meaningful right now. It does exactly the same thing as testAntctlAgentLocalAccess
, because trace-packet
is included in the return value of antctl.CommandList.GetDebugCommands(runtime.ModeAgent)
.
The reason why testAntctlAgentLocalAccess
was not failing is because antctl trace-packet
doesn't return an error code in case of syntax error:
root@kind-control-plane:/# antctl trace-packet
syntax error at br-int (or the bridge name was omitted)
ovs-appctl: /var/run/openvswitch/ovs-vswitchd.100.ctl: server returned an error
root@kind-control-plane:/# echo $?
0
So the new test doesn't validate anything new.
We probably need to fix antctl trace-packet
further, so that the command fails properly in case of an ovs-appctl
execution error. After that, we won't need to add this new e2e test case, unless you want to have a test that's more specific to trace-packet
, and that performs additional validation. It would be need to have a unit test to validate that antctl trace-packet
exits with an error code when the ovs-appctl
command fails.
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, I was suggesting an actual test to verify the normal operation, not the same "runable" check as testAntctlAgentLocalAccess
. Either e2e or unit test works for me.
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. I missed checking the testAntctlAgentLocalAccess and thought it's not included in the exiting tests. I will check and add a unit test for 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.
It turns out that the /ovstracing
API didn't return error code but respond with command error message directly when there is syntax error from OVS server, I refined the code with more unit tests.
30c7219
to
95a34de
Compare
95a34de
to
c2c435b
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
Fix the following error when running `antctl trace-packet`, which is caused by missing arguments for ovs-appctl command. ``` syntax error at br-int (or the bridge name was omitted) ovs-appctl: /var/run/openvswitch/ovs-vswitchd.103.ctl: server returned an error ``` Signed-off-by: Lan Luo <luola@vmware.com>
c2c435b
to
f679a9b
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-conformance |
Fix the following error when running
antctl trace-packet
, which is caused by missing arguments for ovs-appctl command.Fixes #5831