-
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
Add addon enablement to start #6440
Conversation
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: tstromberg 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 |
LGTM, looks like there's a unit test failure:
|
looks good after unit tests fix |
travis caught another unit test:
and a lint error:
|
Codecov Report
@@ Coverage Diff @@
## master #6440 +/- ##
=========================================
Coverage ? 37.98%
=========================================
Files ? 128
Lines ? 8708
Branches ? 0
=========================================
Hits ? 3308
Misses ? 4974
Partials ? 426
|
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! Just a couple nits, but they can be addressed in a separate PR if you prefer.
/ok-to-test |
Error: running mkcmp: exit status 1 |
…aningful error messages and make them comparable
Error: running mkcmp: exit status 1 |
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.
nice!
All Times minikube: [ 96.361238 97.875949 96.325792] Average minikube: 96.854326 Averages Time Per Log
|
This was reported in [1] and fixed in [2] in 2019, but later it was broken by [3] in commit c3aafae. `listProfiles()` can return `os.ErrNotExist` from `os.ReadDir()` so it must be tested with `os.IsNotExist()`. [1] kubernetes#5898 [2] kubernetes#5955 [3] kubernetes#6440
This PR explicitly enables addons on startup, in lieu of there being an addon-manager to do so.
There is a new line emitted during startup:
Behavior changes
There is also a behavioral change: if an addon is enabled/disabled, and you attempt to enable it again, it will run the underlying kubectl command. It should still be idempotent.
I expect this will add ~1 second to our start time, but it does restore the previous behavior.
API changes
The addons package has a new function to handle startup logic:
addons.Start()
. This obsoletes what was instart.go:enableAddons()
andfiles.go:AddAddons()
To improve testing, the following public functions now take a profile name:
addon.IsEnabled()
cluster.IsHostRunning()
Fixes #6363