Skip to content
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

Do not wait for k8s-app pods when starting with CNI #3896

Merged
merged 1 commit into from
Mar 21, 2019

Conversation

brb
Copy link
Contributor

@brb brb commented Mar 18, 2019

Previously, minikube was waiting for some pods labeled with "k8s-app" (e.g. "kube-dns") to be ready before indicating that "minikube start" was successful. However, in the case of
"network-plugin=cni", network of such pods is managed by a CNI plugin which is usually started after "minikube start" has returned making the waiting for the pods to fail with:

Error starting cluster: wait: waiting for k8s-app=kube-dns: timed out
waiting for the condition

This commit omits waiting for "k8s-app" pods if minikube is started with "network-plugin=cni".

Original bug report: cilium/cilium#7383

Previously, minikube was waiting for some pods labeled with
"k8s-app" (e.g. "kube-dns") to be ready before indicating that
"minikube start" was successful. However, in the case of
"network-plugin=cni", network of such pods is managed by a CNI
plugin which is usually started after "minikube start" has returned
making the waiting for the pods to fail with:

    Error starting cluster: wait: waiting for k8s-app=kube-dns: timed out
    waiting for the condition

This commit omits waiting for "k8s-app" pods if minikube is started
with "network-plugin=cni".

Reported-by: Michal Rostecki <[email protected]>
Signed-off-by: Martynas Pumputis <[email protected]>
@k8s-ci-robot k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Mar 18, 2019
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: brb
To fully approve this pull request, please assign additional approvers.
We suggest the following additional approver: balopat

If they are not already assigned, you can assign the PR to them by writing /assign @balopat in a comment when ready.

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@k8s-ci-robot k8s-ci-robot added the size/S Denotes a PR that changes 10-29 lines, ignoring generated files. label Mar 18, 2019
@minikube-bot
Copy link
Collaborator

Can one of the admins verify this patch?

@tstromberg
Copy link
Contributor

@minikube-bot OK to test

@tstromberg
Copy link
Contributor

tstromberg commented Mar 20, 2019

Excellent change - thank you for cleaning up after my mess.

So that we don't break this again, could you also add an integration test for CNI support. There is a handy list of 'start' commands we test here:

https://github.com/kubernetes/minikube/blob/master/test/integration/start_stop_delete_test.go#L39

Alternatively - is it viable to wait for CNI first?

@tstromberg tstromberg added this to the v1.0.0 milestone Mar 20, 2019
@tstromberg
Copy link
Contributor

PR looks good. Can merge once the example CNI command-line is added to start_stop_delete_test.go.

Thank you again!

@tstromberg tstromberg merged commit c4ae58f into kubernetes:master Mar 21, 2019
@brb
Copy link
Contributor Author

brb commented Mar 22, 2019

@tstromberg Thanks for reviewing and merging. I will try to add the integration test next week.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/S Denotes a PR that changes 10-29 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants