Fix minikube image load exits code when guest-side load fails

[Ankit4921]

[23471.log](https://github.com/user-attachments/files/31120500/23471.log)

Fixes #23471

[kubernetes-prow]

[APPROVALNOTIFIER] This PR is NOT APPROVED

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

The full list of commands accepted by this bot can be found here.

Details

Needs approval from an approver in each of these files:

• OWNERS

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

[linux-foundation-easycla]

The committers listed above are authorized under a signed CLA.

• ✅ login: Ankit4921 / name: Ankit4921 (518358c, dc4e44a)

[kubernetes-prow]

Welcome @Ankit4921!

It looks like this is your first PR to kubernetes/minikube 🎉. Please refer to our pull request process documentation to help your PR have a smooth ride to approval.

You will be prompted by a bot to use commands during the review process. Do not be afraid to follow the prompts! It is okay to experiment. Here is the bot commands documentation.

You can also check if kubernetes/minikube has its own contribution guidelines.

You may want to refer to our testing guide if you run into trouble with your tests not passing.

If you are having difficulty getting your pull request seen, please follow the recommended escalation practices. Also, for tips and tricks in the contribution process you may want to read the Kubernetes contributor cheat sheet. We want to make sure your contribution gets all the attention it needs!

Thank you, and welcome to Kubernetes. 😃

[kubernetes-prow]

Hi @Ankit4921. 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.

Details

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.

[minikube-bot]

Can one of the admins verify this patch?

[ravi-arnan]

Thanks for picking this up, and @nirs thanks for the ping. Reviewed by reading the callers on master and running the test locally. The one-line change in DoLoadImages is the right fix and it does what #23471 asked for. Five notes, the first two are worth acting on before merge.

1. The new test passes without exercising the bug. Running TestDoLoadImages_ReturnsError on this branch, the error it observes never comes from the image load:

E cache_images.go:219] Failed to load profile "exitcode": cluster "exitcode" does not exist
I cache_images.go:265] failed pushing to: exitcode
--- PASS: TestDoLoadImages_ReturnsError (0.14s)

config.Load(pName) fails first, so the loop continues before any node is touched and the bogus tar is never transferred. Replacing []string{bad} with an empty []string{} and running it again still passes, which confirms the tar plays no part. The profile is created via createHost but never written where config.Load looks for it. As it stands the test pins the profile-load branch, which is the one branch the code deliberately treats as non-fatal (note 2).

2. This makes a documented non-fatal race fatal. failed collects three different things, and only the third is a real load failure:

c, err := config.Load(pName)
if err != nil {
    // Non-fatal because it may race with profile deletion
    klog.Errorf("Failed to load profile %q: %v", pName, err)
    failed = append(failed, pName)

plus the Status(api, m) error just below it. With this change, a profile deleted concurrently now fails the whole command, contradicting that comment. minikube cache add iterates every profile from cacheAddProfiles(), so one profile disappearing mid-run would fail a command that succeeded for every other profile. Worth either restricting the returned error to the LoadCachedImages / LoadLocalImages failures, or dropping that comment deliberately.

3. Blast radius, for the record. DoLoadImages is reached from CacheAndLoadImages, so this changes three commands, not one:

• minikube start: node/start.go:200 only does out.FailureT("Unable to push cached images"), no exit.Error. So start does not begin failing, it starts printing what it used to swallow. This is the good case and it is why the change is safe.
• minikube cache add and minikube cache reload: cmd/cache.go:55 and :109 both call exit.Error, so these start exiting non-zero. Probably wanted, but it is a user visible change beyond image load and deserves a line in the PR description.

4. The user still does not learn why it failed. The issue's complaint was a silent no-op, and the new message names the machine but not the cause:

return fmt.Errorf("failed to load images to: %s", strings.Join(failed, " "))

The actual docker load error is still klog-only, so at default verbosity the user gets "failed to load images to: exitcode" and has to rerun with -v=3 to see the tar was corrupt. Collecting the per-machine errors and wrapping them, or errors.Join, would finish the fix.

5. Nit: t.Parallel() with os.Setenv should be t.Setenv. MINIKUBE_HOME is process global, so setting it from a parallel test leaks into whatever else is running at that moment. Nothing in the package reads it in a parallel test today, so this does not break anything now, it is a trap for the next parallel test that resolves localpath.MiniPath(). t.Setenv restores automatically and deliberately refuses to run inside a parallel test for exactly this reason, and cache_binaries_test.go in this same package already uses it.

For what it is worth on the convention question, I have applied nirs's image rm guidance to #23448 so the two commands land on the same rule: fail when the operation did not achieve what was asked, stay quiet when the end state is already what the user wanted.

[nirs]

// Non-fatal because it may race with profile deletion

This does not make sense. If you delete a profile while running other commands on the profile the other command should fail. Even if we improve this later to lock profiles during commands one of the commands will fail.

[ravi-arnan]

Thanks for the quick turnaround on dc4e44a1. Notes 3, 4 and 5 are handled: the error now carries the cause, and the t.Parallel() / os.Setenv pair is gone. Note 1 still stands, and note 2 has an open question that is really for @nirs.

Note 1: the new test still passes without the fix.

TestDoLoadImages_ReturnsError now calls LoadLocalImages, which this PR does not modify, so it never reaches DoLoadImages. Two measurements against this branch's base, ec11ed6a:

1. ec11ed6a checked out with only cache_image_test.go taken from dc4e44a1, so the production change is absent:

--- PASS: TestDoLoadImages_ReturnsError (0.00s)
ok  	k8s.io/minikube/pkg/minikube/machine	0.032s

2. Same run with the this is not a tar archive at all payload replaced by a valid tar archive. Still passes. The file content plays no part, because NewFakeCommandRunner() has no commands registered and fake_runner.go:61 therefore fails every command it is asked to run, systemctl --version included. The error the test asserts on comes from the fake runner, not from an image load.

A test that does pin the change. Status() returns state.None with no error for a machine that does not exist (host.go:35), so the cheapest reachable failure is a machine record that exists but cannot be loaded. No docker and no cluster needed:

func TestDoLoadImagesReturnsError(t *testing.T) {
	home := filepath.Join(t.TempDir(), ".minikube")
	t.Setenv("MINIKUBE_HOME", home)

	cc := config.ClusterConfig{
		Name:             "pinprofile",
		KubernetesConfig: config.KubernetesConfig{ContainerRuntime: "docker"},
		Nodes:            []config.Node{{Name: "", ControlPlane: true}},
	}
	if err := config.SaveProfile("pinprofile", &cc, home); err != nil {
		t.Fatalf("SaveProfile: %v", err)
	}

	// A machine record that exists but cannot be loaded, so Status() errors.
	md := filepath.Join(home, "machines", "pinprofile")
	if err := os.MkdirAll(md, 0755); err != nil {
		t.Fatal(err)
	}
	if err := os.WriteFile(filepath.Join(md, "config.json"), []byte("{not json"), 0644); err != nil {
		t.Fatal(err)
	}

	err := DoLoadImages([]string{filepath.Join(home, "nope.tar")},
		[]*config.Profile{{Name: "pinprofile", Config: &cc}},
		"", false, &run.CommandOptions{})
	if err == nil {
		t.Fatal("expected DoLoadImages to return an error when a machine could not be reached, got nil")
	}
}

On ec11ed6a it fails, DoLoadImages returns <nil>. On dc4e44a1 it passes:

DoLoadImages returned: failed to load images to: pinprofile: load: filestore "pinprofile":
Error getting migrated host: invalid character 'n' looking for beginning of object key string

That also exercises note 4, since the cause is now carried in the message.

Note 2: I withdraw my own version of it, but this revision moved away from what nirs asked for. He wrote above that a profile deleted mid-run should fail the command. dc4e44a1 goes the other way: the config.Load branch no longer appends to failed, so that case is now skipped silently. Worth settling before merge, since it is the one behavioural question left in the diff.

Correcting my own note 2 on scope while I am here: every caller except one passes exactly one profile (node/start.go:200, cmd/image.go:142 and :148). The multi-profile loop is only reachable through cacheAddProfiles() taking its allFlag branch, so making that branch fatal touches minikube cache add and minikube cache reload and nothing else. Smaller blast radius than I implied.

Minor: failed and failures are now appended at the same three sites, so failed only feeds the klog line. One slice would do.

[Ankit4921]

Thanks for the detailed review. I've addressed Note 1 by rewriting TestDoLoadImages_ReturnsError with the exact pattern you proposed:

1. Creates a profile that exists in MINIKUBE_HOME
2. Writes a machine directory with invalid config.json
3. Calls DoLoadImages with that profile
4. Asserts that it returns an error

The test now passes with the fix and fails without it. The error message correctly shows: failed to load images to: pinprofile: load: filestore "pinprofile": Error getting migrated host: invalid character 'n' looking for beginning of object key string

This truly exercises the production code path in DoLoadImages as you intended.

Note 2 remains as a behavioral decision: whether a profile deleted mid-run should fail the command or remain non-fatal. As you noted, this is a question for @nirs and should be settled before merge. I'm ready to adjust the config.Load error handling once that decision is made. Currently it treats that race as non-fatal (continues with a warning).

Updated test:-
23471.log

[ravi-arnan]

Verified the rewritten test locally, both directions.

• Checked out the PR base ec11ed6a4 in a worktree and took only cache_image_test.go from a87d75467, leaving cache_images.go at base: FAIL, expected DoLoadImages to return an error when a machine could not be reached, got nil. It reaches Status() and logs error getting status for pinprofile: load: filestore "pinprofile": Error getting migrated host: invalid character 'n' ..., then base returns nil.
• Same worktree with the production change applied: PASS in 0.00s. Whole pkg/minikube/machine package green (ok k8s.io/minikube/pkg/minikube/machine 5.167s), gofmt -l clean, go vet clean.

I also ran the substitution the other way round, since a test that still passes without its input is what note 1 was about. Dropping the corrupt machines/pinprofile/config.json and changing nothing else makes the test fail even with the fix applied: Status() on a missing machine returns state.None with no error, so nothing lands in failed. The corrupt machine record is load bearing, which is the point.

One thing worth writing down so the test name is not read too widely later: the images argument is inert here. Replacing []string{filepath.Join(home, "nope.tar")} with []string{} still passes, because the node never reaches Running and LoadLocalImages is never called. That is fine for what this test pins, a per-machine failure surfacing as a returned error. It just does not cover the transfer path.

Minor and non blocking: failed and failures now grow together at all three sites, so their lengths can never differ, and failed only feeds the klog line and the len() > 0 check. Not worth a push on its own.

Note 2 is still for you and @nirs to settle, agreed. Nothing further from me.