Fix `minikube addons list` output for default addons #15762

shubhbapna · 2023-01-31T19:27:02Z

The cause of the issue was in pkg/node/start.go where it only updated the config if there were any existing addons defined which was not the case when install-addons was set to false.

It seems to working locally. Adding output here:

$ ./minikube-linux-amd64 start --install-addons=false
😄  minikube v1.29.0 on Fedora 36
🆕  Kubernetes 1.26.1 is now available. If you would like to upgrade, specify: --kubernetes-version=v1.26.1
✨  Using the docker driver based on existing profile
❗  docker is currently using the btrfs storage driver, consider switching to overlay2 for better performance
👍  Starting control plane node minikube in cluster minikube
🚜  Pulling base image ...
🔄  Restarting existing docker container for "minikube" ...
❗  This container is having trouble accessing https://k8s.gcr.io
💡  To pull new external images, you may need to configure a proxy: https://minikube.sigs.k8s.io/docs/reference/networking/proxy/
🐳  Preparing Kubernetes v1.23.8 on Docker 20.10.20 ...
🔎  Verifying Kubernetes components...
🏄  Done! kubectl is now configured to use "minikube" cluster and "default" namespace by default

# no storage provisioner
$ kubectl get pods -A
NAMESPACE      NAME                                        READY   STATUS             RESTARTS       AGE
kube-system    coredns-64897985d-dvsrj                     1/1     Running            9 (11m ago)    68d
kube-system    etcd-minikube                               1/1     Running            11 (68s ago)   68d
kube-system    kube-apiserver-minikube                     1/1     Running            10 (68s ago)   68d
kube-system    kube-controller-manager-minikube            1/1     Running            9 (68s ago)    68d
kube-system    kube-proxy-mb8r7                            1/1     Running            9 (68s ago)    68d
kube-system    kube-scheduler-minikube                     1/1     Running            10 (68s ago)   68d

# finally correct states for addons
$ ./minikube-linux-amd64 addons list
|-----------------------------|----------|--------------|--------------------------------|
|         ADDON NAME          | PROFILE  |    STATUS    |           MAINTAINER           |
|-----------------------------|----------|--------------|--------------------------------|
| ambassador                  | minikube | disabled     | 3rd party (Ambassador)         |
| auto-pause                  | minikube | disabled     | Google                         |
| cloud-spanner               | minikube | disabled     | Google                         |
| csi-hostpath-driver         | minikube | disabled     | Kubernetes                     |
| dashboard                   | minikube | disabled     | Kubernetes                     |
| default-storageclass        | minikube | disabled     | Kubernetes                     |
| efk                         | minikube | disabled     | 3rd party (Elastic)            |
| freshpod                    | minikube | disabled     | Google                         |
| gcp-auth                    | minikube | disabled     | Google                         |
| gvisor                      | minikube | disabled     | Google                         |
| headlamp                    | minikube | disabled     | 3rd party (kinvolk.io)         |
| helm-tiller                 | minikube | disabled     | 3rd party (Helm)               |
| inaccel                     | minikube | disabled     | 3rd party (InAccel             |
|                             |          |              | [[email protected]])            |
| ingress                     | minikube | disabled     | Kubernetes                     |
| ingress-dns                 | minikube | disabled     | Google                         |
| istio                       | minikube | disabled     | 3rd party (Istio)              |
| istio-provisioner           | minikube | disabled     | 3rd party (Istio)              |
| kong                        | minikube | disabled     | 3rd party (Kong HQ)            |
| kubevirt                    | minikube | disabled     | 3rd party (KubeVirt)           |
| logviewer                   | minikube | disabled     | 3rd party (unknown)            |
| metallb                     | minikube | disabled     | 3rd party (MetalLB)            |
| metrics-server              | minikube | disabled     | Kubernetes                     |
| nvidia-driver-installer     | minikube | disabled     | Google                         |
| nvidia-gpu-device-plugin    | minikube | disabled     | 3rd party (Nvidia)             |
| olm                         | minikube | disabled     | 3rd party (Operator Framework) |
| pod-security-policy         | minikube | disabled     | 3rd party (unknown)            |
| portainer                   | minikube | disabled     | 3rd party (Portainer.io)       |
| registry                    | minikube | disabled     | Google                         |
| registry-aliases            | minikube | disabled     | 3rd party (unknown)            |
| registry-creds              | minikube | disabled     | 3rd party (UPMC Enterprises)   |
| storage-provisioner         | minikube | disabled     | Google                         |
| storage-provisioner-gluster | minikube | disabled     | 3rd party (Gluster)            |
| volumesnapshots             | minikube | disabled     | Kubernetes                     |
|-----------------------------|----------|--------------|--------------------------------|

k8s-ci-robot · 2023-01-31T19:27:12Z

Hi @shubhbapna. Thanks for your PR.

I'm waiting for a kubernetes member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

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.

minikube-bot · 2023-01-31T19:30:05Z

Can one of the admins verify this patch?

pkg/addons/addons_test.go

Co-authored-by: Steven Powell <[email protected]>

spowelljr

Hi @shubhbapna, this works great for the existing addons, however if we were to add a new addon that is enabled by default this still outputs the addon as enabled even if it isn't.

$ minikube start --install-addons=false

# remove storage-provisioner from config.json to simulate new default addon being added
$ sed -i.bak 's/"storage-provisioner": false,//g' ~/.minikube/profiles/minikube/config.json

# storage-provisoner listed as enabled when it isn't
$ minikube addons list
|-----------------------------|----------|--------------|--------------------------------|
|         ADDON NAME          | PROFILE  |    STATUS    |           MAINTAINER           |
|-----------------------------|----------|--------------|--------------------------------|
| ambassador                  | minikube | disabled     | 3rd party (Ambassador)         |
| auto-pause                  | minikube | disabled     | Google                         |
| cloud-spanner               | minikube | disabled     | Google                         |
| csi-hostpath-driver         | minikube | disabled     | Kubernetes                     |
| dashboard                   | minikube | disabled     | Kubernetes                     |
| default-storageclass        | minikube | disabled     | Kubernetes                     |
| efk                         | minikube | disabled     | 3rd party (Elastic)            |
| freshpod                    | minikube | disabled     | Google                         |
| gcp-auth                    | minikube | disabled     | Google                         |
| gvisor                      | minikube | disabled     | Google                         |
| headlamp                    | minikube | disabled     | 3rd party (kinvolk.io)         |
| helm-tiller                 | minikube | disabled     | 3rd party (Helm)               |
| inaccel                     | minikube | disabled     | 3rd party (InAccel             |
|                             |          |              | [[email protected]])            |
| ingress                     | minikube | disabled     | Kubernetes                     |
| ingress-dns                 | minikube | disabled     | Google                         |
| istio                       | minikube | disabled     | 3rd party (Istio)              |
| istio-provisioner           | minikube | disabled     | 3rd party (Istio)              |
| kong                        | minikube | disabled     | 3rd party (Kong HQ)            |
| kubevirt                    | minikube | disabled     | 3rd party (KubeVirt)           |
| logviewer                   | minikube | disabled     | 3rd party (unknown)            |
| metallb                     | minikube | disabled     | 3rd party (MetalLB)            |
| metrics-server              | minikube | disabled     | Kubernetes                     |
| nvidia-driver-installer     | minikube | disabled     | Google                         |
| nvidia-gpu-device-plugin    | minikube | disabled     | 3rd party (Nvidia)             |
| olm                         | minikube | disabled     | 3rd party (Operator Framework) |
| pod-security-policy         | minikube | disabled     | 3rd party (unknown)            |
| portainer                   | minikube | disabled     | 3rd party (Portainer.io)       |
| registry                    | minikube | disabled     | Google                         |
| registry-aliases            | minikube | disabled     | 3rd party (unknown)            |
| registry-creds              | minikube | disabled     | 3rd party (UPMC Enterprises)   |
| storage-provisioner         | minikube | enabled ✅   | Google                         |
| storage-provisioner-gluster | minikube | disabled     | 3rd party (Gluster)            |
| volumesnapshots             | minikube | disabled     | Kubernetes                     |
|-----------------------------|----------|--------------|--------------------------------|

The cause of this whole issue is that if the addon is not found in the config list that it defaults to outputting it's state as it's default state instead of false.

minikube/pkg/minikube/assets/addons.go

Lines 77 to 85 in c80c885

    
           func (a *Addon) IsEnabled(cc *config.ClusterConfig) bool { 
        
           	status, ok := cc.Addons[a.Name()] 
        
           	if ok { 
        
           		return status 
        
           	} 
        
           	// Return the default unconfigured state of the addon 
        
           	return a.enabled 
        
           }

shubhbapna · 2023-02-06T21:44:36Z

@spowelljr sorry could you elaborate more on this case? (I haven't worked with addons before)

IMO adding a new addon would have already happened before we start minikube. I tried the same set of commands in this order and the state of addons is still correctly shown

# remove storage-provisioner from config.json to simulate new default addon being added
$ sed -i.bak 's/"storage-provisioner": false,//g' ~/.minikube/profiles/minikube/config.json

$ minikube start --install-addons=false

# storage-provisoner is listed as disabled even when the default is enabled
$ minikube addons list

spowelljr · 2023-02-07T17:47:27Z

If users are starting minikube for the first time it's fine, but many minikube users have their clusters existing for extended periods of time. For example the user starts minikube with v1.26.0, they upgrade their minikube binary to v1.29.0, but because the cluster was created with v1.26.0 the cluster is missing the new addon added in v1.29.0. That's what the sed statement simulates.

shubhbapna · 2023-02-07T19:22:03Z

I see in that case I think we will have to persist the fact that minikube was started with --install-addons=false. For that I am thinking we can add a field in the config.json like addonsDisabledOnStart. Wdyt? @spowelljr

shubhbapna · 2023-02-14T17:43:57Z

@spowelljr I am persisting whether install-addons was true or false in the config.json and using that to resolve the case you were talking about. Let me know if there is better approach I missed

spowelljr · 2023-02-16T17:29:56Z

I don't think saving install-addons to the config is the right thing to do here.

We can ignore my previous case of if a new default addon is added in the future as the chance of this is likely 0. But this will still display wrong if a user started their cluster with an older version of minikube with --install-addons=false, updates to a binary with this change, it will still display the wrong table as right now it's only fixing it on creation not in the display logic itself.

The cause of this problem is the use and implementation of the IsEnabled function I mentioned in the previous comment.

minikube/pkg/minikube/assets/addons.go

Lines 77 to 85 in c80c885

    
           func (a *Addon) IsEnabled(cc *config.ClusterConfig) bool { 
        
           	status, ok := cc.Addons[a.Name()] 
        
           	if ok { 
        
           		return status 
        
           	} 
        
           	// Return the default unconfigured state of the addon 
        
           	return a.enabled 
        
           }

This also stems from the Addon struct

minikube/pkg/minikube/assets/addons.go

Lines 37 to 49 in cde517c

    
           // Addon is a named list of assets, that can be enabled 
        
           type Addon struct { 
        
           	Assets             []*BinAsset 
        
           	enabled            bool 
        
           	addonName          string 
        
           	Maintainer         string 
        
           	VerifiedMaintainer string 
        
           	Docs               string 
        
           	Images             map[string]string 
        
           	// Registries currently only shows the default registry of images 
        
           	Registries map[string]string 
        
           }

The enabled field in the Addon struct is misleading as enabled is actually whether it's enabled by default, not whether it's currently enabled or disabled.

This takes us to the IsEnabled func, if there's any entry in the config for the addon we're checking it returns it's status in the config, but if it's not in the config it returns a.enabled, which as just mentioned actually means enabled by default.

The fix should be changing the fallback for IsEnabled to return false if it's not found in the config instead of a.enabled as that will make the IsEnabled function work as the name intends. However, this will break starting minikube because of the following:

minikube/pkg/addons/addons.go

Lines 538 to 543 in cde517c

    
           // Get the default values of any addons not saved to our config 
        
           for name, a := range assets.Addons { 
        
           	if _, exists := existing[name]; !exists { 
        
           		enable[name] = a.IsEnabled(cc) 
        
           	} 
        
           }

We rely on a.enabled as the backup value for this so we enable the default addons, if these were to return false no addons would be enabled by default.

What I think we really need is a new function like IsEnabled called something like IsEnabledOrDefault. The logic for IsEnabledOrDefault can be an exact copy paste of IsEnabled, and then IsEnabled can be updated to return false if the value isn't found in the config. And then

minikube/pkg/addons/addons.go

Lines 538 to 543 in cde517c

    
           // Get the default values of any addons not saved to our config 
        
           for name, a := range assets.Addons { 
        
           	if _, exists := existing[name]; !exists { 
        
           		enable[name] = a.IsEnabled(cc) 
        
           	} 
        
           }

Can we updated to use the new IsEnabledOrDefault so it still enables the default addons on a regular start.

This reverts commit 0f334db.

spowelljr · 2023-02-17T16:23:38Z

/ok-to-test

minikube-pr-bot · 2023-02-17T17:31:22Z

kvm2 driver with docker runtime

+----------------+----------+---------------------+
|    COMMAND     | MINIKUBE | MINIKUBE (PR 15762) |
+----------------+----------+---------------------+
| minikube start | 54.6s    | 55.2s               |
| enable ingress | 25.9s    | 26.5s               |
+----------------+----------+---------------------+

Times for minikube start: 54.9s 54.0s 56.1s 51.2s 56.7s
Times for minikube (PR 15762) start: 55.3s 55.4s 55.8s 55.0s 54.7s

Times for minikube ingress: 25.2s 25.8s 25.2s 28.7s 24.7s
Times for minikube (PR 15762) ingress: 26.8s 29.2s 26.2s 24.7s 25.8s

docker driver with docker runtime

+-------------------+----------+---------------------+
|      COMMAND      | MINIKUBE | MINIKUBE (PR 15762) |
+-------------------+----------+---------------------+
| minikube start    | 27.1s    | 26.6s               |
| ⚠️  enable ingress | 33.5s    | 39.4s ⚠️             |
+-------------------+----------+---------------------+

Times for minikube start: 27.6s 26.5s 25.8s 26.2s 29.2s
Times for minikube (PR 15762) start: 27.8s 27.0s 26.9s 25.2s 26.0s

Times for minikube ingress: 81.6s 20.1s 23.1s 21.1s 21.6s
Times for minikube (PR 15762) ingress: 21.6s 21.6s 20.6s 83.6s 49.6s

docker driver with containerd runtime

+----------------+----------+---------------------+
|    COMMAND     | MINIKUBE | MINIKUBE (PR 15762) |
+----------------+----------+---------------------+
| minikube start | 22.2s    | 22.6s               |
| enable ingress | 47.0s    | 47.8s               |
+----------------+----------+---------------------+

Times for minikube start: 21.5s 22.2s 21.6s 22.4s 23.2s
Times for minikube (PR 15762) start: 22.8s 22.0s 22.6s 23.1s 22.5s

Times for minikube (PR 15762) ingress: 79.6s 48.6s 31.6s 47.6s 31.6s
Times for minikube ingress: 79.6s 33.6s 21.6s 19.6s 80.6s

minikube-pr-bot · 2023-02-17T19:09:55Z

These are the flake rates of all failed tests.

Environment	Failed Tests	Flake Rate (%)
Docker_Windows	TestRunningBinaryUpgrade (gopogh)	0.00 (chart)
Docker_macOS	TestNetworkPlugins/group/flannel/Start (gopogh)	2.63 (chart)
Docker_macOS	TestCertOptions (gopogh)	4.40 (chart)
Docker_macOS	TestDockerFlags (gopogh)	4.40 (chart)
Docker_macOS	TestForceSystemdEnv (gopogh)	4.40 (chart)
Docker_macOS	TestForceSystemdFlag (gopogh)	4.40 (chart)
Docker_macOS	TestCertExpiration (gopogh)	4.43 (chart)
Docker_macOS	TestNoKubernetes/serial/StartWithK8s (gopogh)	4.46 (chart)
Docker_macOS	TestPause/serial/Start (gopogh)	4.46 (chart)
Docker_macOS	TestStoppedBinaryUpgrade/MinikubeLogs (gopogh)	5.06 (chart)
KVM_Linux	TestMultiNode/serial/StartAfterStop (gopogh)	9.04 (chart)
KVM_Linux	TestPause/serial/SecondStartNoReconfiguration (gopogh)	9.64 (chart)
Docker_Linux	TestMultiNode/serial/DeployApp2Nodes (gopogh)	11.31 (chart)
Docker_Linux	TestMultiNode/serial/PingHostFrom2Pods (gopogh)	11.31 (chart)
Docker_macOS	TestMultiNode/serial/DeployApp2Nodes (gopogh)	11.95 (chart)
Docker_macOS	TestMultiNode/serial/PingHostFrom2Pods (gopogh)	11.95 (chart)
Hyper-V_Windows	TestNoKubernetes/serial/StartWithK8s (gopogh)	94.44 (chart)
Hyper-V_Windows	TestStoppedBinaryUpgrade/Upgrade (gopogh)	98.61 (chart)
Docker_macOS	TestIngressAddonLegacy/serial/ValidateIngressAddonActivation (gopogh)	100.00 (chart)
Docker_macOS	TestIngressAddonLegacy/serial/ValidateIngressAddons (gopogh)	100.00 (chart)
Docker_macOS	TestIngressAddonLegacy/serial/ValidateIngressDNSAddonActivation (gopogh)	100.00 (chart)
Docker_macOS	TestIngressAddonLegacy/StartLegacyK8sCluster (gopogh)	100.00 (chart)
Docker_macOS	TestKubernetesUpgrade (gopogh)	100.00 (chart)
Docker_macOS	TestMissingContainerUpgrade (gopogh)	100.00 (chart)
Docker_macOS	TestRunningBinaryUpgrade (gopogh)	100.00 (chart)
Docker_macOS	TestStoppedBinaryUpgrade/Upgrade (gopogh)	100.00 (chart)
Docker_Windows	TestFunctional/parallel/ServiceCmd (gopogh)	100.00 (chart)
Hyper-V_Windows	TestMultiNode/serial/PingHostFrom2Pods (gopogh)	100.00 (chart)
Hyper-V_Windows	TestMultiNode/serial/RestartKeepsNodes (gopogh)	100.00 (chart)
Hyper-V_Windows	TestRunningBinaryUpgrade (gopogh)	100.00 (chart)
More tests...	Continued...

Too many tests failed - See test logs for more details.

To see the flake rates of all tests by environment, click here.

spowelljr

Looks good, thanks for the PR!

k8s-ci-robot · 2023-02-17T23:28:17Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: shubhbapna, spowelljr

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:

~~OWNERS~~ [spowelljr]

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

Shubh Bapna added 2 commits January 31, 2023 14:18

added function to disable all addons in config. updated test cases

e0016e6

disabled addons while starting node if existing addons in nil

b0513bc

k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Jan 31, 2023

k8s-ci-robot requested review from afbjorklund and medyagh January 31, 2023 19:27

k8s-ci-robot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Jan 31, 2023

shubhbapna mentioned this pull request Jan 31, 2023

made the default status of addons false #15722

Closed

spowelljr requested changes Feb 1, 2023

View reviewed changes

pkg/addons/addons_test.go Outdated Show resolved Hide resolved

Fix test error message

2d7df77

Co-authored-by: Steven Powell <[email protected]>

shubhbapna requested review from spowelljr and removed request for medyagh and afbjorklund February 1, 2023 19:29

spowelljr requested changes Feb 3, 2023

View reviewed changes

persist whether install-addons was true or false

0f334db

shubhbapna requested a review from spowelljr February 14, 2023 17:42

Shubh Bapna added 2 commits February 17, 2023 10:40

Revert "persist whether install-addons was true or false"

2ac1435

This reverts commit 0f334db.

added isEnabledOrDefault

f88c6ad

k8s-ci-robot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Feb 17, 2023

spowelljr approved these changes Feb 17, 2023

View reviewed changes

spowelljr merged commit e066375 into kubernetes:master Feb 17, 2023

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Feb 17, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix `minikube addons list` output for default addons #15762

Fix `minikube addons list` output for default addons #15762

shubhbapna commented Jan 31, 2023

k8s-ci-robot commented Jan 31, 2023

minikube-bot commented Jan 31, 2023

spowelljr left a comment

shubhbapna commented Feb 6, 2023 •

edited

Loading

spowelljr commented Feb 7, 2023

shubhbapna commented Feb 7, 2023 •

edited

Loading

shubhbapna commented Feb 14, 2023

spowelljr commented Feb 16, 2023

spowelljr commented Feb 17, 2023

minikube-pr-bot commented Feb 17, 2023

minikube-pr-bot commented Feb 17, 2023

spowelljr left a comment

k8s-ci-robot commented Feb 17, 2023

	func (a Addon) IsEnabled(cc config.ClusterConfig) bool {
	status, ok := cc.Addons[a.Name()]
	if ok {
	return status
	}

	// Return the default unconfigured state of the addon
	return a.enabled
	}

Fix minikube addons list output for default addons #15762

Fix minikube addons list output for default addons #15762

Conversation

shubhbapna commented Jan 31, 2023

k8s-ci-robot commented Jan 31, 2023

minikube-bot commented Jan 31, 2023

spowelljr left a comment

Choose a reason for hiding this comment

shubhbapna commented Feb 6, 2023 • edited Loading

spowelljr commented Feb 7, 2023

shubhbapna commented Feb 7, 2023 • edited Loading

shubhbapna commented Feb 14, 2023

spowelljr commented Feb 16, 2023

spowelljr commented Feb 17, 2023

minikube-pr-bot commented Feb 17, 2023

minikube-pr-bot commented Feb 17, 2023

spowelljr left a comment

Choose a reason for hiding this comment

k8s-ci-robot commented Feb 17, 2023

Fix `minikube addons list` output for default addons #15762

Fix `minikube addons list` output for default addons #15762

shubhbapna commented Feb 6, 2023 •

edited

Loading

shubhbapna commented Feb 7, 2023 •

edited

Loading