diff --git a/cmd/minikube/cmd/config/addons_list.go b/cmd/minikube/cmd/config/addons_list.go index 5ed95a850b..521182f226 100644 --- a/cmd/minikube/cmd/config/addons_list.go +++ b/cmd/minikube/cmd/config/addons_list.go @@ -29,6 +29,7 @@ import ( "k8s.io/minikube/pkg/minikube/assets" "k8s.io/minikube/pkg/minikube/config" "k8s.io/minikube/pkg/minikube/exit" + "k8s.io/minikube/pkg/minikube/mustload" "k8s.io/minikube/pkg/minikube/out" ) @@ -49,11 +50,12 @@ var addonsListCmd = &cobra.Command{ exit.UsageT("usage: minikube addons list") } + _, cc := mustload.Partial(ClusterFlagValue()) switch strings.ToLower(addonListOutput) { case "list": - printAddonsList() + printAddonsList(cc) case "json": - printAddonsJSON() + printAddonsJSON(cc) default: exit.WithCodeT(exit.BadUsage, fmt.Sprintf("invalid output format: %s. Valid values: 'list', 'json'", addonListOutput)) } @@ -85,27 +87,24 @@ var stringFromStatus = func(addonStatus bool) string { return "disabled" } -var printAddonsList = func() { +var printAddonsList = func(cc *config.ClusterConfig) { addonNames := make([]string, 0, len(assets.Addons)) for addonName := range assets.Addons { addonNames = append(addonNames, addonName) } sort.Strings(addonNames) + var tData [][]string table := tablewriter.NewWriter(os.Stdout) table.SetHeader([]string{"Addon Name", "Profile", "Status"}) table.SetAutoFormatHeaders(true) table.SetBorders(tablewriter.Border{Left: true, Top: true, Right: true, Bottom: true}) table.SetCenterSeparator("|") - pName := ClusterFlagValue() for _, addonName := range addonNames { addonBundle := assets.Addons[addonName] - addonStatus, err := addonBundle.IsEnabled(pName) - if err != nil { - out.WarningT("Unable to get addon status for {{.name}}: {{.error}}", out.V{"name": addonName, "error": err}) - } - tData = append(tData, []string{addonName, pName, fmt.Sprintf("%s %s", stringFromStatus(addonStatus), iconFromStatus(addonStatus))}) + enabled := addonBundle.IsEnabled(cc) + tData = append(tData, []string{addonName, cc.Name, fmt.Sprintf("%s %s", stringFromStatus(enabled), iconFromStatus(enabled))}) } table.AppendBulk(tData) @@ -120,9 +119,8 @@ var printAddonsList = func() { } } -var printAddonsJSON = func() { +var printAddonsJSON = func(cc *config.ClusterConfig) { addonNames := make([]string, 0, len(assets.Addons)) - pName := ClusterFlagValue() for addonName := range assets.Addons { addonNames = append(addonNames, addonName) } @@ -132,16 +130,11 @@ var printAddonsJSON = func() { for _, addonName := range addonNames { addonBundle := assets.Addons[addonName] - - addonStatus, err := addonBundle.IsEnabled(pName) - if err != nil { - glog.Errorf("Unable to get addon status for %s: %v", addonName, err) - continue - } + enabled := addonBundle.IsEnabled(cc) addonsMap[addonName] = map[string]interface{}{ - "Status": stringFromStatus(addonStatus), - "Profile": pName, + "Status": stringFromStatus(enabled), + "Profile": cc.Name, } } jsonString, _ := json.Marshal(addonsMap) diff --git a/cmd/minikube/cmd/config/disable.go b/cmd/minikube/cmd/config/disable.go index 552eaf7108..a77f44092d 100644 --- a/cmd/minikube/cmd/config/disable.go +++ b/cmd/minikube/cmd/config/disable.go @@ -33,7 +33,7 @@ var addonsDisableCmd = &cobra.Command{ } addon := args[0] - err := addons.Set(addon, "false", ClusterFlagValue()) + err := addons.SetAndSave(ClusterFlagValue(), addon, "false") if err != nil { exit.WithError("disable failed", err) } diff --git a/cmd/minikube/cmd/config/enable.go b/cmd/minikube/cmd/config/enable.go index 99cbb3bb88..f05daadfd6 100644 --- a/cmd/minikube/cmd/config/enable.go +++ b/cmd/minikube/cmd/config/enable.go @@ -32,7 +32,7 @@ var addonsEnableCmd = &cobra.Command{ exit.UsageT("usage: minikube addons enable ADDON_NAME") } addon := args[0] - err := addons.Set(addon, "true", ClusterFlagValue()) + err := addons.SetAndSave(ClusterFlagValue(), addon, "true") if err != nil { exit.WithError("enable failed", err) } diff --git a/cmd/minikube/cmd/config/open.go b/cmd/minikube/cmd/config/open.go index 93bca9f735..b49dce72fe 100644 --- a/cmd/minikube/cmd/config/open.go +++ b/cmd/minikube/cmd/config/open.go @@ -66,11 +66,9 @@ var addonsOpenCmd = &cobra.Command{ To see the list of available addons run: minikube addons list`, out.V{"name": addonName}) } - ok, err := addon.IsEnabled(cname) - if err != nil { - exit.WithError("IsEnabled failed", err) - } - if !ok { + + enabled := addon.IsEnabled(co.Config) + if !enabled { exit.WithCodeT(exit.Unavailable, `addon '{{.name}}' is currently not enabled. To enable this addon run: minikube addons enable {{.name}}`, out.V{"name": addonName}) diff --git a/cmd/minikube/cmd/dashboard.go b/cmd/minikube/cmd/dashboard.go index e3c9e93b6b..51957e4b58 100644 --- a/cmd/minikube/cmd/dashboard.go +++ b/cmd/minikube/cmd/dashboard.go @@ -67,13 +67,14 @@ var dashboardCmd = &cobra.Command{ var err error // Check dashboard status before enabling it - dashboardAddon := assets.Addons["dashboard"] - dashboardStatus, _ := dashboardAddon.IsEnabled(cname) - if !dashboardStatus { + addon := assets.Addons["dashboard"] + enabled := addon.IsEnabled(co.Config) + + if !enabled { // Send status messages to stderr for folks re-using this output. out.ErrT(out.Enabling, "Enabling dashboard ...") // Enable the dashboard add-on - err = addons.Set("dashboard", "true", cname) + err = addons.SetAndSave(cname, "dashboard", "true") if err != nil { exit.WithError("Unable to enable dashboard", err) } diff --git a/pkg/addons/addons.go b/pkg/addons/addons.go index 63ae1507df..3edc54b61b 100644 --- a/pkg/addons/addons.go +++ b/pkg/addons/addons.go @@ -23,6 +23,7 @@ import ( "sort" "strconv" "strings" + "sync" "time" "github.com/golang/glog" @@ -41,32 +42,49 @@ import ( // defaultStorageClassProvisioner is the name of the default storage class provisioner const defaultStorageClassProvisioner = "standard" -// Set sets a value -func Set(name, value, profile string) error { - glog.Infof("Setting %s=%s in profile %q", name, value, profile) +// RunCallbacks runs all actions associated to an addon, but does not set it (thread-safe) +func RunCallbacks(cc *config.ClusterConfig, name string, value string) error { + glog.Infof("Setting %s=%s in profile %q", name, value, cc.Name) a, valid := isAddonValid(name) if !valid { return errors.Errorf("%s is not a valid addon", name) } - cc, err := config.Load(profile) - if err != nil { - return errors.Wrap(err, "loading profile") - } - // Run any additional validations for this property if err := run(cc, name, value, a.validations); err != nil { return errors.Wrap(err, "running validations") } - if err := a.set(cc, name, value); err != nil { - return errors.Wrap(err, "setting new value of addon") - } - // Run any callbacks for this property if err := run(cc, name, value, a.callbacks); err != nil { return errors.Wrap(err, "running callbacks") } + return nil +} + +// Set sets a value in the config (not threadsafe) +func Set(cc *config.ClusterConfig, name string, value string) error { + a, valid := isAddonValid(name) + if !valid { + return errors.Errorf("%s is not a valid addon", name) + } + return a.set(cc, name, value) +} + +// SetAndSave sets a value and saves the config +func SetAndSave(profile string, name string, value string) error { + cc, err := config.Load(profile) + if err != nil { + return errors.Wrap(err, "loading profile") + } + + if err := RunCallbacks(cc, name, value); err != nil { + return errors.Wrap(err, "run callbacks") + } + + if err := Set(cc, name, value); err != nil { + return errors.Wrap(err, "set") + } glog.Infof("Writing out %q config to set %s=%v...", profile, name, value) return config.Write(profile, cc) @@ -87,7 +105,7 @@ func run(cc *config.ClusterConfig, name string, value string, fns []setFn) error return nil } -// SetBool sets a bool value +// SetBool sets a bool value in the config (not threadsafe) func SetBool(cc *config.ClusterConfig, name string, val string) error { b, err := strconv.ParseBool(val) if err != nil { @@ -110,13 +128,7 @@ func enableOrDisableAddon(cc *config.ClusterConfig, name string, val string) err addon := assets.Addons[name] // check addon status before enabling/disabling it - alreadySet, err := isAddonAlreadySet(addon, enable, cc.Name) - if err != nil { - out.ErrT(out.Conflict, "{{.error}}", out.V{"error": err}) - return err - } - - if alreadySet { + if isAddonAlreadySet(cc, addon, enable) { glog.Warningf("addon %s should already be in state %v", name, val) if !enable { return nil @@ -160,7 +172,7 @@ https://github.com/kubernetes/minikube/issues/7332`, out.V{"driver_name": cc.Dri mName := driver.MachineName(*cc, cp) host, err := machine.LoadHost(api, mName) if err != nil || !machine.IsRunning(api, mName) { - glog.Warningf("%q is not running, writing %s=%v to disk and skipping enablement (err=%v)", mName, addon.Name(), enable, err) + glog.Warningf("%q is not running, setting %s=%v and skipping enablement (err=%v)", mName, addon.Name(), enable, err) return nil } @@ -173,19 +185,17 @@ https://github.com/kubernetes/minikube/issues/7332`, out.V{"driver_name": cc.Dri return enableOrDisableAddonInternal(cc, addon, cmd, data, enable) } -func isAddonAlreadySet(addon *assets.Addon, enable bool, profile string) (bool, error) { - addonStatus, err := addon.IsEnabled(profile) - if err != nil { - return false, errors.Wrap(err, "is enabled") +func isAddonAlreadySet(cc *config.ClusterConfig, addon *assets.Addon, enable bool) bool { + enabled := addon.IsEnabled(cc) + if enabled && enable { + return true } - if addonStatus && enable { - return true, nil - } else if !addonStatus && !enable { - return true, nil + if !enabled && !enable { + return true } - return false, nil + return false } func enableOrDisableAddonInternal(cc *config.ClusterConfig, addon *assets.Addon, cmd command.Runner, data interface{}, enable bool) error { @@ -287,7 +297,10 @@ func enableOrDisableStorageClasses(cc *config.ClusterConfig, name string, val st } // Start enables the default addons for a profile, plus any additional -func Start(profile string, toEnable map[string]bool, additional []string) { +func Start(wg *sync.WaitGroup, cc *config.ClusterConfig, toEnable map[string]bool, additional []string) { + wg.Add(1) + defer wg.Done() + start := time.Now() glog.Infof("enableAddons start: toEnable=%v, additional=%s", toEnable, additional) defer func() { @@ -296,11 +309,7 @@ func Start(profile string, toEnable map[string]bool, additional []string) { // Get the default values of any addons not saved to our config for name, a := range assets.Addons { - defaultVal, err := a.IsEnabled(profile) - if err != nil { - glog.Errorf("is-enabled failed for %q: %v", a.Name(), err) - continue - } + defaultVal := a.IsEnabled(cc) _, exists := toEnable[name] if !exists { @@ -321,12 +330,25 @@ func Start(profile string, toEnable map[string]bool, additional []string) { } sort.Strings(toEnableList) + var awg sync.WaitGroup + out.T(out.AddonEnable, "Enabling addons: {{.addons}}", out.V{"addons": strings.Join(toEnableList, ", ")}) for _, a := range toEnableList { - err := Set(a, "true", profile) - if err != nil { - // Intentionally non-fatal - out.WarningT("Enabling '{{.name}}' returned an error: {{.error}}", out.V{"name": a, "error": err}) + awg.Add(1) + go func(name string) { + err := RunCallbacks(cc, name, "true") + if err != nil { + out.WarningT("Enabling '{{.name}}' returned an error: {{.error}}", out.V{"name": name, "error": err}) + } + awg.Done() + }(a) + } + + // Wait until all of the addons are enabled before updating the config (not thread safe) + awg.Wait() + for _, a := range toEnableList { + if err := Set(cc, a, "true"); err != nil { + glog.Errorf("store failed: %v", err) } } } diff --git a/pkg/addons/addons_test.go b/pkg/addons/addons_test.go index 6862aaf542..004d0f5047 100644 --- a/pkg/addons/addons_test.go +++ b/pkg/addons/addons_test.go @@ -20,6 +20,7 @@ import ( "io/ioutil" "os" "path/filepath" + "sync" "testing" "k8s.io/minikube/pkg/minikube/assets" @@ -59,47 +60,42 @@ func createTestProfile(t *testing.T) string { } func TestIsAddonAlreadySet(t *testing.T) { - profile := createTestProfile(t) - if err := Set("registry", "true", profile); err != nil { + cc := &config.ClusterConfig{Name: "test"} + + if err := Set(cc, "registry", "true"); err != nil { t.Errorf("unable to set registry true: %v", err) } - enabled, err := assets.Addons["registry"].IsEnabled(profile) - if err != nil { - t.Errorf("registry: %v", err) - } - if !enabled { + if !assets.Addons["registry"].IsEnabled(cc) { t.Errorf("expected registry to be enabled") } - enabled, err = assets.Addons["ingress"].IsEnabled(profile) - if err != nil { - t.Errorf("ingress: %v", err) - } - if enabled { + if assets.Addons["ingress"].IsEnabled(cc) { t.Errorf("expected ingress to not be enabled") } } func TestDisableUnknownAddon(t *testing.T) { - profile := createTestProfile(t) - if err := Set("InvalidAddon", "false", profile); err == nil { + cc := &config.ClusterConfig{Name: "test"} + + if err := Set(cc, "InvalidAddon", "false"); err == nil { t.Fatalf("Disable did not return error for unknown addon") } } func TestEnableUnknownAddon(t *testing.T) { - profile := createTestProfile(t) - if err := Set("InvalidAddon", "true", profile); err == nil { + cc := &config.ClusterConfig{Name: "test"} + + if err := Set(cc, "InvalidAddon", "true"); err == nil { t.Fatalf("Enable did not return error for unknown addon") } } -func TestEnableAndDisableAddon(t *testing.T) { +func TestSetAndSave(t *testing.T) { profile := createTestProfile(t) // enable - if err := Set("dashboard", "true", profile); err != nil { + if err := SetAndSave(profile, "dashboard", "true"); err != nil { t.Errorf("Disable returned unexpected error: " + err.Error()) } @@ -112,7 +108,7 @@ func TestEnableAndDisableAddon(t *testing.T) { } // disable - if err := Set("dashboard", "false", profile); err != nil { + if err := SetAndSave(profile, "dashboard", "false"); err != nil { t.Errorf("Disable returned unexpected error: " + err.Error()) } @@ -126,14 +122,18 @@ func TestEnableAndDisableAddon(t *testing.T) { } func TestStart(t *testing.T) { - profile := createTestProfile(t) - Start(profile, map[string]bool{}, []string{"dashboard"}) - - enabled, err := assets.Addons["dashboard"].IsEnabled(profile) - if err != nil { - t.Errorf("dashboard: %v", err) + cc := &config.ClusterConfig{ + Name: "start", + CPUs: 2, + Memory: 2500, + KubernetesConfig: config.KubernetesConfig{}, } - if !enabled { + + var wg sync.WaitGroup + Start(&wg, cc, map[string]bool{}, []string{"dashboard"}) + wg.Wait() + + if !assets.Addons["dashboard"].IsEnabled(cc) { t.Errorf("expected dashboard to be enabled") } } diff --git a/pkg/minikube/assets/addons.go b/pkg/minikube/assets/addons.go index 852eb27252..6d8d3f9ef6 100644 --- a/pkg/minikube/assets/addons.go +++ b/pkg/minikube/assets/addons.go @@ -19,8 +19,6 @@ package assets import ( "runtime" - "github.com/golang/glog" - "github.com/pkg/errors" "k8s.io/minikube/pkg/minikube/config" "k8s.io/minikube/pkg/minikube/constants" "k8s.io/minikube/pkg/minikube/vmpath" @@ -49,21 +47,14 @@ func (a *Addon) Name() string { } // IsEnabled checks if an Addon is enabled for the given profile -func (a *Addon) IsEnabled(profile string) (bool, error) { - c, err := config.Load(profile) - if err != nil { - return false, errors.Wrap(err, "load") - } - - // Is this addon explicitly listed in their configuration? - status, ok := c.Addons[a.Name()] - glog.V(1).Infof("IsEnabled %q = %v (listed in config=%v)", a.Name(), status, ok) +func (a *Addon) IsEnabled(cc *config.ClusterConfig) bool { + status, ok := cc.Addons[a.Name()] if ok { - return status, nil + return status } // Return the default unconfigured state of the addon - return a.enabled, nil + return a.enabled } // Addons is the list of addons diff --git a/pkg/minikube/exit/exit.go b/pkg/minikube/exit/exit.go index cba71f73a3..d9e4403e52 100644 --- a/pkg/minikube/exit/exit.go +++ b/pkg/minikube/exit/exit.go @@ -20,6 +20,7 @@ package exit import ( "os" "runtime" + "runtime/debug" "k8s.io/minikube/pkg/minikube/out" "k8s.io/minikube/pkg/minikube/problem" @@ -53,6 +54,7 @@ func WithCodeT(code int, format string, a ...out.V) { // WithError outputs an error and exits. func WithError(msg string, err error) { + glog.Infof("WithError(%s)=%v called from:\n%s", msg, err, debug.Stack()) p := problem.FromError(err, runtime.GOOS) if p != nil { WithProblem(msg, err, p) diff --git a/pkg/minikube/node/config.go b/pkg/minikube/node/config.go index ef0f66dc12..5b646d24d1 100644 --- a/pkg/minikube/node/config.go +++ b/pkg/minikube/node/config.go @@ -22,6 +22,7 @@ import ( "os/exec" "path/filepath" "strconv" + "sync" "github.com/golang/glog" "github.com/spf13/viper" @@ -46,7 +47,10 @@ func showVersionInfo(k8sVersion string, cr cruntime.Manager) { } // configureMounts configures any requested filesystem mounts -func configureMounts() { +func configureMounts(wg *sync.WaitGroup) { + wg.Add(1) + defer wg.Done() + if !viper.GetBool(createMount) { return } diff --git a/pkg/minikube/node/start.go b/pkg/minikube/node/start.go index 0fda6de595..6ef365e8da 100644 --- a/pkg/minikube/node/start.go +++ b/pkg/minikube/node/start.go @@ -134,15 +134,20 @@ func Start(starter Starter, apiServer bool) (*kubeconfig.Settings, error) { } } - configureMounts() + var wg sync.WaitGroup + go configureMounts(&wg) - if err := CacheAndLoadImagesInConfig(); err != nil { - out.FailureT("Unable to load cached images from config file.") - } + wg.Add(1) + go func() { + if err := CacheAndLoadImagesInConfig(); err != nil { + out.FailureT("Unable to load cached images from config file: {{error}}", out.V{"error": err}) + } + wg.Done() + }() // enable addons, both old and new! if starter.ExistingAddons != nil { - addons.Start(viper.GetString(config.ProfileName), starter.ExistingAddons, config.AddonList) + go addons.Start(&wg, &cc, starter.ExistingAddons, config.AddonList) } if apiServer { @@ -182,7 +187,10 @@ func Start(starter Starter, apiServer bool) (*kubeconfig.Settings, error) { } } - return kcs, nil + wg.Wait() + + // Write enabled addons to the config before completion + return kcs, config.Write(viper.GetString(config.ProfileName), &cc) } // Provision provisions the machine/container for the node