Skip to content

Conversation

nirs
Copy link
Contributor

@nirs nirs commented Oct 2, 2025

Currently we use viper outside of cmd/minikube/cmd package in pkg/minikube/** and pkg/drivers/**. This introduce 2 issues:

  • Make the code hard to test by using global state. For example for testing interactive mode we must use viper.SetBool("interactive", false). This may break another test assuming interactive mode.
  • Error prone - since the flag names are private constants in the cmd/minikube/cmd package, we use strings (e.g. "interactive") in other package. A typo in the flag name will silently provide the wrong value, ignoring the user request.

This is the first part in removing viper calls outside of the cmd/minikube/cmd package.

Preparations

  • run: Introduce minikube/run and cmd/flags packages - the basic infrastructure for passing options from cmd/minikube/cmd to other packages.
  • run: Pass options to drivers - big change passing options from most commands that load or check driver status.
    • This is a pretty big commit but it is hard to split it.

Removals

  • vment: Remove viper interactive check - remove viper.GetBool("interactive") check in vment.ValidateHelper()
  • firewall: Remove viper interactive check - remove viper.GetBool("interactive") check in firewall.UnblockBootpd()
  • notify: Remove viper interactive checks - remove viper.GetBool("interactive") check in notify.shouldCheckURLVersion()
  • run: Remove viper.GetBool("download-only") checks in cmd and minikube packages

Part-of #21670

@k8s-ci-robot
Copy link
Contributor

Skipping CI for Draft Pull Request.
If you want CI signal for your change, please convert it to an actual PR.
You can still manually trigger a test run with /test all

@k8s-ci-robot k8s-ci-robot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Oct 2, 2025
@k8s-ci-robot k8s-ci-robot requested review from medyagh and prezha October 2, 2025 18:15
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: nirs
Once this PR has been reviewed and has the lgtm label, please assign spowelljr for approval. For more information see the Code Review Process.

The full list of commands accepted by this bot can be found 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 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 Oct 2, 2025
@nirs
Copy link
Contributor Author

nirs commented Oct 2, 2025

/cc @ComradeProgrammer
/cc @afbjorklund

@nirs
Copy link
Contributor Author

nirs commented Oct 2, 2025

/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 Oct 2, 2025
@nirs nirs force-pushed the remove-viper-checks branch from 1a26ae8 to cf26111 Compare October 2, 2025 19:25
@afbjorklund afbjorklund removed their request for review October 2, 2025 19:26
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.

thank you for doing this ! this is gonna make minikube development much safer (and less suprises)

@medyagh
Copy link
Member

medyagh commented Oct 2, 2025

/ok-to-test

@minikube-pr-bot

This comment has been minimized.

@nirs

This comment was marked as outdated.

@minikube-pr-bot

This comment has been minimized.

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.

we should make to document this in the contributors guide
to never make a viper call outside CMD packages

@nirs nirs force-pushed the remove-viper-checks branch from 9daa62a to ec1ad26 Compare October 9, 2025 18:17
@minikube-pr-bot

This comment has been minimized.

@minikube-pr-bot

This comment has been minimized.

@nirs nirs force-pushed the remove-viper-checks branch from ec1ad26 to 0c44d92 Compare October 18, 2025 13:14
@minikube-pr-bot

This comment has been minimized.

@minikube-pr-bot

This comment has been minimized.

@nirs nirs force-pushed the remove-viper-checks branch from 0c44d92 to 64e83d5 Compare October 18, 2025 20:57
@k8s-ci-robot k8s-ci-robot added size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Oct 18, 2025
@nirs nirs marked this pull request as draft October 18, 2025 21:01
@k8s-ci-robot k8s-ci-robot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Oct 18, 2025
@nirs nirs force-pushed the remove-viper-checks branch 3 times, most recently from d40b4ed to 33695fc Compare October 18, 2025 22:14
@minikube-pr-bot

This comment has been minimized.

@minikube-pr-bot

This comment has been minimized.

@minikube-pr-bot

This comment has been minimized.

@minikube-pr-bot

This comment has been minimized.

nirs and others added 6 commits October 19, 2025 17:51
This change introduce the basic infrastructure for passing command line
options from the cmd/minikube/cmd package to other packages.

The cmd/flags package provides the Options() function returning
run.Options initialized using viper. This package keeps the constants
for command line options (e.g. "interactive") that we want to share with
various packages without accessing global state via viper.

To use options in drivers code, include the options in the CommonDriver
struct. The options will be initialized from the command line options
when creating a driver.

The basic idea is to create options in the command:

    options := flags.Options()

And pass it to other packages, where code will use:

    if options.NonInteractive {

Instead of:

    if viper.GetBool("interactive") {

This is type safe and allows reliable parallel testing.
Some drivers need command line options since they need to pass command
line options back to minikube firewall package. The way to pass command
line options to the driver is via the NewDriver function, called by the
registry Loader function.

The registry Loader function is called by machine.LocalClient.Load,
which is part of the limachine API interface, which is not part of
minikube so we cannot change it. We pass the options to
machine.NewAPIClient(), so the client can pass the options to Load().

Some drivers need to validate vment helper in the registry StatusChecker
function, considering the --interactive and --download-only flags. So we
pas the options to the StatusChecker function.

This change create the options in most commands that call
machine.NewAPIClient or registry StatusChecker function and pass the
options down.
vment.ValidateHelper() accept now *run.Options and use
options.NonInteractive to check if interaction is allowed.  Update
callers to pass options from the minikube command.

Testing non-interactive mode:

    % sudo rm /etc/sudoers.d/vmnet-helper
    % sudo -k
    % out/minikube start -d krunkit --interactive=false
    😄  minikube v1.37.0 on Darwin 26.0.1 (arm64)
    ✨  Using the krunkit (experimental) driver based on user configuration

    🤷  Exiting due to PROVIDER_KRUNKIT_NOT_FOUND: The 'krunkit' provider was not found: exit status 1: sudo: a password is required
    💡  Suggestion: Install and configure vment-helper
    📘  Documentation: https://minikube.sigs.k8s.io/docs/reference/drivers/krunkit/

Testing interactive mode:

    % out/minikube start -d krunkit
    😄  minikube v1.37.0 on Darwin 26.0.1 (arm64)
    💡  Unable to run vmnet-helper without a password
        To configure vment-helper to run without a password, please check the documentation:
        https://github.com/nirs/vmnet-helper/#granting-permission-to-run-vmnet-helper
    Password:
    ✨  Using the krunkit (experimental) driver based on user configuration
    👍  Starting "minikube" primary control-plane node in "minikube" cluster
    🔥  Creating krunkit VM (CPUs=2, Memory=6144MB, Disk=20000MB) ...
    🐳  Preparing Kubernetes v1.34.1 on Docker 28.4.0 ...
    🔗  Configuring bridge CNI (Container Networking Interface) ...
    🔎  Verifying Kubernetes components...
        ▪ Using image gcr.io/k8s-minikube/storage-provisioner:v5
    🌟  Enabled addons: default-storageclass, storage-provisioner
    🏄  Done! kubectl is now configured to use "minikube" cluster and "default" namespace by default
filewall.UnblockBootpd() accepts now *run.Options and use it to check if
we can interact with the user. Update callers to pass options.
The notify helpers accept now *run.Options and use it to check if we can
interact with the user. Modify callers to pass options using
cmd/flags.Options().
Add run.Options.DownloadOnly option and replace
viper.GetBool("download-only") calls in from minikube packages.
@nirs nirs force-pushed the remove-viper-checks branch from 33695fc to 1b58c48 Compare October 19, 2025 16:00
@nirs nirs marked this pull request as ready for review October 19, 2025 16:13
@k8s-ci-robot k8s-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Oct 19, 2025
@k8s-ci-robot
Copy link
Contributor

k8s-ci-robot commented Oct 19, 2025

@nirs: The following test failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name Commit Details Required Rerun command
pull-minikube-integration 1b58c48 link true /test pull-minikube-integration

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.

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-sigs/prow repository. I understand the commands that are listed here.

@minikube-pr-bot
Copy link

kvm2 driver with docker runtime

┌────────────────┬──────────┬────────────────────────┐
│    COMMAND     │ MINIKUBE │ MINIKUBE  ( PR 21683 ) │
├────────────────┼──────────┼────────────────────────┤
│ minikube start │ 43.6s    │ 44.3s                  │
│ enable ingress │ 16.2s    │ 16.5s                  │
└────────────────┴──────────┴────────────────────────┘

Times for minikube start: 41.7s 42.1s 45.9s 43.6s 44.7s
Times for minikube (PR 21683) start: 45.3s 44.4s 44.3s 45.2s 42.2s

Times for minikube ingress: 15.8s 16.4s 16.9s 15.8s 15.9s
Times for minikube (PR 21683) ingress: 17.3s 16.4s 15.9s 16.8s 15.9s

docker driver with docker runtime

┌────────────────┬──────────┬────────────────────────┐
│    COMMAND     │ MINIKUBE │ MINIKUBE  ( PR 21683 ) │
├────────────────┼──────────┼────────────────────────┤
│ minikube start │ 23.4s    │ 23.6s                  │
│ enable ingress │ 11.9s    │ 12.5s                  │
└────────────────┴──────────┴────────────────────────┘

Times for minikube start: 23.4s 22.7s 22.8s 25.4s 22.7s
Times for minikube (PR 21683) start: 22.0s 26.7s 23.7s 23.6s 22.2s

Times for minikube ingress: 12.7s 11.7s 13.7s 10.7s 10.7s
Times for minikube (PR 21683) ingress: 12.7s 12.7s 11.7s 13.7s 11.6s

docker driver with containerd runtime

┌────────────────┬──────────┬────────────────────────┐
│    COMMAND     │ MINIKUBE │ MINIKUBE  ( PR 21683 ) │
├────────────────┼──────────┼────────────────────────┤
│ minikube start │ 21.6s    │ 21.7s                  │
│ enable ingress │ 21.0s    │ 20.4s                  │
└────────────────┴──────────┴────────────────────────┘

Times for minikube ingress: 20.2s 20.1s 22.2s 21.1s 21.2s
Times for minikube (PR 21683) ingress: 21.1s 20.1s 20.1s 20.2s 20.2s

Times for minikube start: 23.2s 20.2s 20.4s 24.2s 20.1s
Times for minikube (PR 21683) start: 20.6s 24.0s 20.5s 21.4s 21.7s

@minikube-pr-bot
Copy link

Here are the number of top 10 failed tests in each environments with lowest flake rate.

Environment Test Name Flake Rate
Docker_Linux_containerd (2 failed) TestFunctional/parallel/DashboardCmd(gopogh) 0.00% (chart)
Docker_Linux_containerd (2 failed) TestKubernetesUpgrade(gopogh) 0.00% (chart)
KVM_Linux_crio (4 failed) TestFunctional/parallel/ImageCommands/ImageRemove(gopogh) 2.50% (chart)

Besides the following environments also have failed tests:

  • Docker_Linux_crio_arm64: 37 failed (gopogh)

  • Docker_Linux_crio: 41 failed (gopogh)

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

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/XXL Denotes a PR that changes 1000+ lines, ignoring generated files.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants