-
Notifications
You must be signed in to change notification settings - Fork 4.9k
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 network retry check for subnet already in use for podman #17779
Fix network retry check for subnet already in use for podman #17779
Conversation
Welcome @thomasjm! |
Hi @thomasjm. Thanks for your PR. I'm waiting for a kubernetes member to verify that this patch is reasonable to test. If it is, they should reply with Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: thomasjm The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
Can one of the admins verify this patch? |
The Kubernetes project currently lacks enough contributors to adequately respond to all PRs. This bot triages PRs according to the following rules:
You can:
Please send feedback to sig-contributor-experience at kubernetes/community. /lifecycle stale |
/remove-lifecycle stale |
@@ -156,6 +156,9 @@ func tryCreateDockerNetwork(ociBin string, subnet *network.Parameters, mtu int, | |||
if strings.Contains(rr.Output(), "is being used by a network interface") { | |||
return nil, ErrNetworkGatewayTaken | |||
} | |||
if strings.Contains(rr.Output(), "is already used on the host or by another config") { |
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.
WYT of changing from is already used on the host
to is already in use on the host
?
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.
Huh? This needs to match the output of Podman, it's not up to us to change.
/ok-to-test |
kvm2 driver with docker runtime
Times for minikube start: 48.4s 45.2s 45.7s 52.7s 49.7s Times for minikube ingress: 23.4s 23.9s 24.5s 23.9s 25.5s docker driver with docker runtime
Times for minikube start: 24.7s 24.4s 23.9s 21.7s 23.7s Times for minikube ingress: 21.2s 25.3s 21.3s 22.3s 21.8s docker driver with containerd runtime
Times for minikube start: 22.2s 19.9s 20.0s 20.4s 19.8s Times for minikube ingress: 32.2s 32.3s 31.8s 31.7s 32.3s |
These are the flake rates of all failed tests.
To see the flake rates of all tests by environment, click here. |
/ok-to-test |
@thomasjm thank you for improving the experience for the podman users ! btw our functional tests for podman has been failing for while (on github actions) I would love to see an extra eyes on that if you are interested, thank you for your contributions |
The code in the
tryCreateDockerNetwork
function (which is also used for Podman) tries to detect when network creation fails because the requested subnet is already in use, so that the caller can retry. It does this by checking for certain strings in the output of thepodman/docker network create
command. In the case of Podman, it fails to detect this correctly because the string Podman uses was not included.(This manifested as the inability to create two Minikube clusters simultaneously on the same machine, because the second one would fail with an error about network creation.)
To see this, here's the error message when I use my machine's rootless
podman
(version 4.5.0) and try to create the same subnet twice:This PR just adds an additional string to check in
tryCreateDockerNetwork
.