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.

Sign up for GitHub

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

Jump to bottom

WIP: Run kubeadm init phase addon all to ensure kube-proxy is up in v1.13+ #4014

Closed
wants to merge 2 commits into from
Closed

WIP: Run kubeadm init phase addon all to ensure kube-proxy is up in v1.13+ #4014

wants to merge 2 commits into from

Conversation

liubog2008
Copy link

@liubog2008 liubog2008 commented Mar 28, 2019 •
edited
Loading

fix #3936 #3843

I find that kube-proxy is not created after minikube start(restart) so that waitForPods always waits until timeout.

kubeadm init phase addon all can generate kube-proxy before waitForPods.

How to reproduce:

  1. minikube start
  2. ctrl+c after vm is created
  3. minikube start again

@k8s-ci-robot
Copy link
Contributor

Thanks for your pull request. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please follow instructions at https://git.k8s.io/community/CLA.md#the-contributor-license-agreement to sign the CLA.

It may take a couple minutes for the CLA signature to be fully registered; after that, please reply here with a new comment and we'll verify. Thanks.

  • If you've already signed a CLA, it's possible we don't have your GitHub username or you're using a different email address. Check your existing CLA data and verify that your email is set on your git commits.
  • If you signed the CLA as a corporation, please sign in with your organization's credentials at https://identity.linuxfoundation.org/projects/cncf to be authorized.
  • If you have done the above and are still having issues with the CLA being reported as unsigned, please email the CNCF helpdesk: [email protected]

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. I understand the commands that are listed here.

@k8s-ci-robot k8s-ci-robot added cncf-cla: no Indicates the PR's author has not signed the CNCF CLA. size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. labels Mar 28, 2019
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is NOT APPROVED

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

If they are not already assigned, you can assign the PR to them by writing /assign @luxas 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

@minikube-bot
Copy link
Collaborator

Can one of the admins verify this patch?

@liubog2008
Copy link
Author

I signed it

@liubog2008
Copy link
Author

/check-cla

1 similar comment
@liubog2008
Copy link
Author

/check-cla

@k8s-ci-robot k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. and removed cncf-cla: no Indicates the PR's author has not signed the CNCF CLA. labels Mar 28, 2019
@neerdoc
Copy link

neerdoc commented Mar 28, 2019

Looks like a typo in the PR.
sudo kubeadm init phase addons all --config %s should be
sudo kubeadm init phase addon all --config %s,
i.e., addon not addons.

At least my kubeadm complains on it.

@liubog2008
Copy link
Author

thanks @neerdoc

@tstromberg
Copy link
Contributor

Thanks for the PR! Do you mind elaborating a bit on the "why" of this change in the PR description?

I'm not sure what this does compared to the existing commands, and I'm additionally unsure what the connection between this PR and #3936 is. Thank you!

@liubog2008
Copy link
Author

@tstromberg That's my first comment.
#3843 (comment)

@k8s-ci-robot k8s-ci-robot added size/S Denotes a PR that changes 10-29 lines, ignoring generated files. and removed size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. labels Mar 29, 2019
@liubog2008
Copy link
Author

@tstromberg

tstromberg
tstromberg reviewed Apr 10, 2019
View reviewed changes
pkg/minikube/bootstrapper/kubeadm/kubeadm.go
@@ -264,11 +264,11 @@ func addAddons(files *[]assets.CopyableFile, data interface{}) error {
}

// waitForPods waits until the important Kubernetes pods are in running state
func waitForPods(k8s config.KubernetesConfig, quiet bool) error {
func waitForPods(k8s config.KubernetesConfig, quiet bool, componentsOnly bool) error {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Having two bool arguments to a function is a bad code smell. Do you mind splitting this out into two functions? Perhaps something like:

waitForComponentPods and waitFork8sPods?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK

@tstromberg tstromberg mentioned this pull request Apr 11, 2019
Closed
@tstromberg tstromberg changed the title fix(kubeadm): fix missing addon phase of kubeadm after v1.13.0 Run kubeadm init phase addon all to ensure kube-proxy is up in v1.13+ Apr 11, 2019
@tstromberg tstromberg changed the title Run kubeadm init phase addon all to ensure kube-proxy is up in v1.13+ WIP: Run kubeadm init phase addon all to ensure kube-proxy is up in v1.13+ Apr 11, 2019
@k8s-ci-robot k8s-ci-robot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Apr 11, 2019
@tstromberg
Copy link
Contributor

Any luck splitting waitForPods into two functions?

@tstromberg tstromberg mentioned this pull request May 14, 2019
Closed
@tstromberg tstromberg mentioned this pull request May 16, 2019
Merged
@tstromberg
Copy link
Contributor

Since there haven't been any updates to this, I've proposed a simpler solution in #4276

Thank you @liubog2008 for pointing us in the right direction for solving this nefarious problem.

@tstromberg tstromberg closed this May 16, 2019
@liubog2008 liubog2008 deleted the fix-missing-proxy branch May 22, 2019 11:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@tstromberg tstromberg tstromberg left review comments

@priyawadhwa priyawadhwa Awaiting requested review from priyawadhwa

Assignees
No one assigned
Labels
cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. size/S Denotes a PR that changes 10-29 lines, ignoring generated files.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

waiting for k8s-app=kube-proxy: timed out waiting for the condition
5 participants
@liubog2008 @k8s-ci-robot @minikube-bot @neerdoc @tstromberg