This provides best user experience - we will see the right default in --help. But this is not consistent with other code, and it read the config twice.

Minikube configures viper to read minikube config, so we can use here viper.GetBool(mdns) to get the default.

The issue is that it does not work now, since minikube call viper.ReadConfig() too late - after the flags were created. This should be fixed to create read config before flags are created.

So we have 2 options - keep the "false" hardcoded value, and replace with viper.GetBool(mdns) when viper integration is fixed, or use now viper.GetBool(mdns) and fix the viper issue properly. I'm fine with both options.

Kept the hardcoded false default for now. Will swap to viper.GetBool(mdns) once the viper config read ordering is fixed upstream.

Adding special helper to read the config for every flag does not scale and not efficient. If we do this for every flag we will read the config many times during start.

Agreed, removed the per-flag config reader. Using hardcoded false now.

This works and allow changing the flag when starting an existing machine:

minikube start                    # mdns disabled
minikube stop
minikube start --mdns             # mdns enabled
minikube stop
minikube start                    # mdns enabled
minikube stop
minikube start --mdns=false       # mdns disable

This is helps the user to fix mistakes 0 they forgot to enable mdns when the cluster was created.

But it is not fully baked - this will not work:

minikube start --mdns             # mdns enabled
minikube start --mdns=false       # mdns disabled in the running vm
minikube stop
minikube start                    # mdns disabled

I think the callback you add in the first version to warn that the user needs to restart is the way to handle this case, but this is a very poor user exprience.

The right way to support enable and disabling the flag dynamically is to disable mdns when on a running vm. But for this we need to keep the value in the config as a pointer so it can present:

• nil: mdns was not specified
• true: mdns enabled
• false: mdns disabled

But we don't have the infra to do this - the current code supports only bool values.

So we have 2 options:

1. Support partial working flag that does not work running cluster
   • Requires adding test coverage for dynamic changes - we need to cover disabling mdns on a cluster created with --mdns and enabling it back - that's 2 more start and stop test steps.
2. Disable support for dynamic modifications - make --mdns create time flag. You need to recreate the cluster if you want to change it
   • Current test are good enough of this.

I suggest we remove the update here to make the flag create time flag to keep the tests simpler. We can add dynamic enable/disable and support changing running vms later if users will request this feature. I think this is lower priority before we have a request, and we have much more important work.

BTW, rosetta and ventOffloading options are bugs - they do not work every for starting a stopped vm, since these options use ~/.minikube/machines/NAME/config.json and not ~/.minikube/profiles/NAME/config.json - which is the file modified in this function.

Went with option 2, --mdns is create-time only. No update in updateExistingConfigFromFlags, users recreate to change it.

It will help to add a comment about eth0, or add a constant. AI agents complain that this depends the distro. This is right since we use buildroot. The comment can explain why and help to fix this is we switch to another distro later.

A more robust way would be to use a constant from the code assuming buildroot, so it is clear that code depend on the distro. I'm not sure we have a good place of this, so adding a constant here should be good enough for now.

Extracted eth0 into a primaryInterface constant with a comment noting it's buildroot-specific.

The test looks correct now, we run the entire flow so we will not have false negative like the other tests using sub tests. We will also get good logs when the test fails since we see the entire flow in test. This is another issue with tests trying to use very small sub tests.

One issue, we don't don't test that starting without --mdns does not enable mdns - if the code is broken and we always enable it the tests will succeed.

If we add a test it may fail if systemd changes the default and enable mdns by default, but in this case we want to detect the issue quickly and simplify the code.

Adding a test for disable --mdns should be skipped if minikube is configured to enable mdns by default.

So we are missing another test that start minikube without the flag and test that mdns is not set. This can be a test that run in parallel with this test. We can create 2 sub tests with MaybeParallel() so they we wait only for the slower test.

This again can be adde to another test that start a cluster, but it does not make sense to split tests like this. We want to have clean tests that are easy to modify and debug, and handle performance issue by running them in parallel.

We need to use t.Context() and not context.Background(). This context is canceled when we reach the global test timeout.

This should run only "minkube start -p profile" no other argument is needed.

We can cover the config here by checking if minikube is configured to enable mdns by default - in this case we don't need to add "--mdns" to the command and the test should succeed.

Lets add getMDNS() helper like in #23069, so we filter non vm drivers (!driver.IsVM(), driver.IsSSH()) and warn that the flag will be ignored if the flag changed.
a2544ac

This will allow simplifying the code later, since cfg.MDNS will be set only if we need to use it, and user will get better feedback if they use an option that does not work with the current driver.

Good, I need to adopt this in 781cab1#diff-be4fac8e21b0e76d789b6aad73dceccc3118e0cbc88c1345e8aab848722e5e14R56

Check how we use "eth0" in other code, maybe we can have this constant in a common location, maybe part of the buildroot provisioner.

Lets keep this style to make the code easier to follow and change.

cmd := exec.Command("sudo", "resolvectl", "mdns", primaryInterface, "yes")
if _, err := runner.RunCmd(cmd); err != nil {
    out.WarningT("Failed to enable mDNS on {{.iface}}: {{.error}}", out.V{"iface": primaryInterface, "error": err})
}

This is easier to read and easier to modify, for example if we want to set cmd fields later.

If we do the check early in start_flags.go we can use here:

configureMDNS(runner, cfg.MDNS)

Same as #23069

Lets pass the cfg.MDNS value and skip if it is not enabled. Keeping the logic of when to configure here keeps the caller code clean and make future changes easier. For exmple if we move to three-sate (true, false, nil) we need to change only this function.