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

Remove polling from addon manager #6046

Merged
merged 14 commits into from
Jan 16, 2020

Conversation

priyawadhwa
Copy link

This PR removes polling from the addon manager to reduce CPU overhead.

Fixes #6034

Priya Wadhwa added 4 commits December 5, 2019 10:46
Add run kubectl apply command directly if addons are changed by the user if the addon manager is disabled.
@k8s-ci-robot k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Dec 10, 2019
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: priyawadhwa

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 approved Indicates a PR has been approved by an approver from all required OWNERS files. label Dec 10, 2019
@priyawadhwa
Copy link
Author

/ok-to-test

@k8s-ci-robot k8s-ci-robot added the ok-to-test Indicates a non-member PR verified by an org member that is safe to test. label Dec 10, 2019
@codecov-io
Copy link

codecov-io commented Dec 10, 2019

Codecov Report

Merging #6046 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #6046   +/-   ##
=======================================
  Coverage   38.26%   38.26%           
=======================================
  Files         124      124           
  Lines        8308     8308           
=======================================
  Hits         3179     3179           
  Misses       4715     4715           
  Partials      414      414
Impacted Files Coverage Δ
pkg/minikube/assets/addons.go 37.93% <ø> (ø) ⬆️

@minikube-bot
Copy link
Collaborator

All Times minikube: [ 149.435142 148.039775 147.346287]
All Times Minikube (PR 6046): [ 145.962436 166.321483 146.992477]

Average minikube: 148.273735
Average Minikube (PR 6046): 153.092132

Averages Time Per Log

+----------------------+-----------+--------------------+
|         LOG          | MINIKUBE  | MINIKUBE (PR 6046) |
+----------------------+-----------+--------------------+
| minikube v           |  0.368303 |           0.348200 |
| Creating kvm2        | 50.569101 |          48.700360 |
| Preparing Kubernetes | 55.841411 |          56.397759 |
| Pulling images       | 17.692755 |          18.617587 |
| Launching Kubernetes | 21.158082 |          25.636236 |
| Waiting for cluster  |  2.591127 |           2.721636 |
+----------------------+-----------+--------------------+

Copy link
Member

@medyagh medyagh left a comment

Choose a reason for hiding this comment

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

the integraiton tests need to be updated

@medyagh
Copy link
Member

medyagh commented Dec 10, 2019

KVM integration logs

    --- FAIL: TestFunctional/parallel (0.00s)
        --- FAIL: TestFunctional/parallel/AddonManager (301.18s)
            functional_test.go:174: (dbg) TestFunctional/parallel/AddonManager: waiting 5m0s for pods matching "component=kube-addon-manager" in namespace "kube-system" ...
            functional_test.go:174: ***** TestFunctional/parallel/AddonManager: pod "component=kube-addon-manager" failed to start within 5m0s: timed out waiting for the condition ****
            helpers.go:313: TestFunctional/parallel/AddonManager: showing logs for failed pods as of 2019-12-10 21:36:58.916534172 +0000 UTC m=+1206.295678758
            functional_test.go:175: wait: component=kube-addon-manager within 5m0s: timed out waiting for the condition

Address nits and remove integration test to make sure addon manager pod
is up and running since we will no longer enable it by default.
@k8s-ci-robot k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Dec 10, 2019
@minikube-bot
Copy link
Collaborator

All Times minikube: [ 147.350005 147.908974 142.661036]
All Times Minikube (PR 6046): [ 141.690715 147.685515 141.865801]

Average minikube: 145.973338
Average Minikube (PR 6046): 143.747343

Averages Time Per Log

+----------------------+-----------+--------------------+
|         LOG          | MINIKUBE  | MINIKUBE (PR 6046) |
+----------------------+-----------+--------------------+
| minikube v           |  0.353068 |           0.349495 |
| Creating kvm2        | 49.350351 |          48.941900 |
| Preparing Kubernetes | 54.057086 |          48.446381 |
| Pulling images       | 18.916024 |          17.233201 |
| Launching Kubernetes | 20.480491 |          25.987556 |
| Waiting for cluster  |  2.766275 |           2.738381 |
+----------------------+-----------+--------------------+

@k8s-ci-robot k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Dec 16, 2019
@minikube-bot
Copy link
Collaborator

All Times minikube: [ 129.876051 130.936441 129.269450]
All Times Minikube (PR 6046): [ 129.135321 131.112670 127.735381]

Average Minikube (PR 6046): 129.327791
Average minikube: 130.027314

Averages Time Per Log

+----------------------+-----------+--------------------+
|         LOG          | MINIKUBE  | MINIKUBE (PR 6046) |
+----------------------+-----------+--------------------+
| minikube v           |  0.387473 |           0.293237 |
| Creating kvm2        | 50.713939 |          50.054591 |
| Preparing Kubernetes | 51.309530 |          52.272401 |
| Pulling images       |  3.236393 |           3.013053 |
| Launching Kubernetes | 21.268650 |          20.865799 |
| Waiting for cluster  |  3.121994 |           2.777418 |
+----------------------+-----------+--------------------+

@minikube-bot
Copy link
Collaborator

All Times minikube: [ 123.453524 124.885183 122.262322]
All Times Minikube (PR 6046): [ 122.833123 121.276905 125.251210]

Average minikube: 123.533677
Average Minikube (PR 6046): 123.120413

Averages Time Per Log

+----------------------+-----------+--------------------+
|         LOG          | MINIKUBE  | MINIKUBE (PR 6046) |
+----------------------+-----------+--------------------+
| minikube v           |  0.233063 |           0.211863 |
| Creating kvm2        | 46.487048 |          45.979315 |
| Preparing Kubernetes | 50.724748 |          51.181762 |
| Pulling images       |  3.124036 |           3.621612 |
| Launching Kubernetes | 20.184663 |          19.521917 |
| Waiting for cluster  |  2.731010 |           2.554037 |
| Done                 |           |                    |
|                      |  0.049108 |           0.049905 |
+----------------------+-----------+--------------------+

@@ -75,7 +75,7 @@ var Addons = map[string]*Addon{
"addon-manager.yaml.tmpl",
"0640",
true),
}, true, "addon-manager"),
}, false, "addon-manager"),
Copy link
Contributor

Choose a reason for hiding this comment

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

Please follow this up with a PR that removes the addon-manager. I think it's continued existence will only add confusion.

If someone begs for the addon-manager to come back, we can reconsider.

Copy link
Author

Choose a reason for hiding this comment

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

sounds good

@priyawadhwa
Copy link
Author

HyperV Windows logs

Hyperkit MacOS logs

VirtualBox Linux logs

VirtualBox MacOS logs

None logs

@priyawadhwa priyawadhwa dismissed medyagh’s stale review January 16, 2020 19:32

All comments were addressed :)

@priyawadhwa priyawadhwa merged commit d310331 into kubernetes:master Jan 16, 2020
@priyawadhwa priyawadhwa deleted the remove-addon-manager branch January 16, 2020 19:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. ok-to-test Indicates a non-member PR verified by an org member that is safe to test. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Remove polling from the addon manager to reduce overhead
6 participants