-
Notifications
You must be signed in to change notification settings - Fork 377
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
[tests/e2e]: Remove all references of busybox in e2e tests #6000
Conversation
Signed-off-by: Shikhar Soni <[email protected]>
TestConnectivity/testOVSRestartSameNode failed in kind e2e test. Additionally, I'm not sure if it's worth replacing all instances of busybox with toolbox in the e2e test. AFAIK the toolbox instance has larger image size(169MB compared to 1.24MB) and longer time for initialization. cc @antoninbas |
@XinShuYang The point of the change is to avoid pulling 2 different images for traffic test. Even busybox is smaller, it causes an extra pull if we keep using it. The linked issue #5991 explains it. |
This should be fixed. It may be because arping in busybox is different from the one in toolbox. |
@tnqn Thanks for the explanation. I am ok with this change on Linux testbed. However, since the toolbox image is currently not supported on Windows, the e2e test also needs to be refactored in this PR. |
It has Windows version: antrea-io/image-utils#27 |
That's great! I didn't notice the recent code changes. |
/test-windows-containerd-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.
one small comment , otherwise LGTM
@shikharish from the test logs:
As @tnqn pointed out, |
@antoninbas How can I run the e2e tests on an already running Kind cluster? Simply running the above command gives me an error.
I have a Kind cluster running with a custom config. |
Does |
Signed-off-by: Shikhar Soni <[email protected]>
Got it to work. Thank you! |
"E2e tests on a Kind cluster on Linux for Flow Visibility" failed:
It should be related to |
@tnqn Didn't know that it skipped those tests by default.
The command I ran was |
@shikharish that probably won't be very helpful, but I didn't manage to reproduce locally with the main branch when running antrea/test/e2e/flowaggregator_test.go Lines 1150 to 1154 in 23292b3
You may just need to explicitly add http:// in front of the IP address, so that's a pretty straightforward change.
|
Signed-off-by: Shikhar Soni <[email protected]>
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
thanks @shikharish
/test-e2e |
/test-windows-containerd-e2e |
I will merge this PR. The Windows job failure is unrelated to this PR: it is because Thanks for your contribution @shikharish |
@antoninbas Thank you! I would love to work on that issue too if it is possible. I am still exploring Antrea and want to contribute more to improve my understanding. |
@shikharish We appreciate your enthusiasm. But the Windows issue is a bit tricky and I have already started working on it. I'd like to restore the Windows CI ASAP. |
Fixes antrea-io#5991 Signed-off-by: Shikhar Soni <[email protected]>
Fixes antrea-io#5991 Signed-off-by: Shikhar Soni <[email protected]>
Fixes #5991 Signed-off-by: Shikhar Soni <[email protected]>
Closes #5991