-
Notifications
You must be signed in to change notification settings - Fork 388
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
Add e2e test for VLAN secondary network #5745
Conversation
"os" | ||
"testing" | ||
) | ||
|
||
// setupLogging creates a temporary directory to export the test logs if necessary. If a directory |
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.
Moved these funcs to entry.go
, so they can be used in e2e-secondary-network.
a5a5414
to
6a9d5f1
Compare
034ade1
to
40f1111
Compare
// Create Pods on the control plane Node, assuming a single Node cluster for the SR-IOV | ||
// test. | ||
controlPlaneNodeName := antreae2e.NodeName(0) |
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 there a strong reason to specify that this is the "control-plane" Node, instead of just using nodeName := antreae2e.NodeName(0)
?
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 was trying to keep the original code behavior. But is it clearer to call out the node is the control plane node?
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.
Usually you just need any Node, and the control-plane Node does not have any special meaning. In that case it's better to use nodeName(0)
, or even not specify the Node unless you are specifically testing for inter-Node traffic.
588cad9
to
6f15775
Compare
b8e8ff4
to
9994612
Compare
Extend secondary network e2e tests to support testing secondary VLAN networks. Add a script to run the test on a Kind cluster, and enable the test on Github CI. Signed-off-by: Jianjun Shen <shenj@vmware.com>
--secondary-bridge) | ||
SECONDARY_BRIDGE="$2" | ||
shift 2 | ||
;; | ||
--physical-interface) | ||
PHYSICAL_INTERFACE="$2" | ||
shift 2 |
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 see that you switched to using Helm directly in the end. Should these new flags be removed?
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 like to keep them as it is still simpler to run generate-manifest.sh in some cases.
/test-all |
/test-e2e |
Extend secondary network e2e tests to support testing secondary VLAN
networks. Add a script to run the test on a Kind cluster, and enable
the test on Github CI.
Issue: #5278