Remove broken node start --delete-on-failure flag - #23382
Conversation
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: sammaji The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
|
Welcome @sammaji! |
|
Hi @sammaji. 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 Regular contributors should join the org to skip this step. Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. DetailsInstructions 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-sigs/prow repository. |
|
Can one of the admins verify this patch? |
|
@sammaji This is too much - we need do separate PRs for these changes. Removing the cache command is a big change and we need to discuss it first. We can remove now the --delete-on-failure flag - small and simple change, and never works so basically no change in behavior. |
|
|
|
@nirs I have scoped the changes to --delete-on-failure only. |
@nirs agreed, I will create a PR around it, we can remove this deprecated command with a major release. |
This is not relevant to this PR. Did you reproduce the issue? Please show example runs before and after this fix. I'm not sure this will be easy to show - likely require looking in minikube log and may require adding new log to inspect the value. |
nirs
left a comment
There was a problem hiding this comment.
Change looks good - but we need to verify the bug report first to make sure the assumptions are correct.
The issue was based on code inspection for this command, and validated for the cache add command, which has the same issue in --all flag.
|
/ok-to-test |
| ``` | ||
| --delete-on-failure If set, delete the current cluster if start fails and try again. Defaults to false. | ||
| ``` | ||
|
|
There was a problem hiding this comment.
I think that this is auto-generated so we don't need to include it in the PR. When the PR is merged, an update workflow will create a pr like this #23303 will handle the docs change.
|
@sammaji: The following tests failed, say
Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR. DetailsInstructions 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-sigs/prow repository. I understand the commands that are listed here. |
|
kvm2 driver with docker runtime DetailsTimes for minikube (PR 23382) start: 36.2s 35.9s 35.8s 35.4s 36.2s Times for minikube ingress: 14.7s 15.2s 18.7s 15.2s 14.7s docker driver with docker runtime DetailsTimes for minikube start: 18.0s 18.4s 19.4s 20.9s 19.1s Times for minikube ingress: 18.6s 10.6s 12.6s 10.1s 12.6s docker driver with containerd runtime DetailsTimes for minikube (PR 23382) start: 19.5s 20.1s 17.5s 17.1s 15.7s Times for minikube ingress: 25.2s 24.7s 25.2s 25.2s 25.2s |
Viper has a single global flag namespace, but cobra registers flags per-command. A local flag only becomes visible to
viper.Get*()if it is explicitly bound withviper.BindPFlags()for the command being run. Onlyroot,start, anddeletedo this binding, so any other command reading a local flag viaviper.Get*()is silently broken.Two commands were affected:
minikube cache add --all— the flag was registered onaddCacheCmdbut never bound to viper, so it was always read asfalseand images were only cached to the current profile, never to all running profiles.minikube node start --delete-on-failure— same issue:viper.GetBool(deleteOnFailure)always returnedfalse, so a failed node start was never deleted and retried.BindPFlagscan't simply be added for these commands because flag names collide across commands (e.g.--allis also used bycache deleteandstop), and since every command'sinit()runs at startup, binding one command's flags to the global viper namespace would clobber bindings for other commands using the same name.Fix
minikube node start --delete-on-failurenever worked and nobody has reported relying on it, so the flag is removed rather than wired up.minikube start --delete-on-failureis unaffected. (minikube cacheis kept as it is).Docs were regenerated (
make generate-docs) to drop the removed command and flag.Fixes #23347