Kicbase simplify image load - remove confusing message related to podman #22024

obnoxxx · 2025-12-02T12:32:51Z

refactors image loading in beginDownloadKicBaseImage() a bit to simplify the logic
by combining redundant code blocks.

In the special case of podman, it leaves image loading from cache unimplemented unimplemented and removes the confusing and unhelpful message.

Thus is intended as a replacement for PR #21932 without pretending to implement the cache handling.

The otputs of minikube start -d podman are provided with and without this patch illustrate the changed behavior:

without the patch

$ # on current master:
$ ./out/minikube delete --all
🔥  Deleting "minikube" in podman ...
🔥  Removing /home/obnox/.minikube/machines/minikube ...
💀  Removed all traces of the "minikube" cluster.
🔥  Successfully deleted all profiles


$ time ./out/minikube start -d podman 
😄  minikube v1.37.0 on Fedora 43
✨  Using the podman driver based on user configuration
📌  Using Podman driver with root privileges
👍  Starting "minikube" primary control-plane node in "minikube" cluster
🚜  Pulling base image v0.0.48-1764169655-21974 ...
E1203 11:02:21.355429   86335 cache.go:238] Error downloading kic artifacts:  not yet implemented, see issue #8426
🔥  Creating podman container (CPUs=2, Memory=7900MB) ...
🐳  Preparing Kubernetes v1.34.2 on Docker 29.0.4 ...
🔗  Configuring bridge CNI (Container Networking Interface) ...
🔎  Verifying Kubernetes components...
    ▪ Using image gcr.io/k8s-minikube/storage-provisioner:v5
🌟  Enabled addons: storage-provisioner, default-storageclass
🏄  Done! kubectl is now configured to use "minikube" cluster and "default" namespace by default

real    0m29.764s
user    0m1.816s
sys     0m0.963s
$ echo $?
0
$

with the patch

$ # on obnoxxx/kicbase-simplify-image-load :
$ make clean && make
$ ./out/minikube  delete --all --purge

...

$ time ./out/minikube start -d podman
😄  minikube v1.37.0 on Fedora 43
✨  Using the podman driver based on user configuration
📌  Using Podman driver with root privileges
👍  Starting "minikube" primary control-plane node in "minikube" cluster
🚜  Pulling base image v0.0.48-1764169655-21974 ...
💾  Downloading Kubernetes v1.34.2 preload ...
    > preloaded-images-k8s-v18-v1...:  271.13 MiB / 271.13 MiB  100.00% 19.18 M
🔥  Creating podman container (CPUs=2, Memory=7900MB) ...
🐳  Preparing Kubernetes v1.34.2 on Docker 29.0.4 ...
🔗  Configuring bridge CNI (Container Networking Interface) ...
🔎  Verifying Kubernetes components...
    ▪ Using image gcr.io/k8s-minikube/storage-provisioner:v5
🌟  Enabled addons: storage-provisioner, default-storageclass
🏄  Done! kubectl is now configured to use "minikube" cluster and "default" namespace by default

real    0m45.507s
user    0m3.615s
sys     0m2.048s
$ echo $?
0
$

k8s-ci-robot · 2025-12-02T12:33:00Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: obnoxxx
Once this PR has been reviewed and has the lgtm label, please assign comradeprogrammer 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:

OWNERS

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 · 2025-12-02T12:33:00Z

Hi @obnoxxx. Thanks for your PR.

I'm waiting for a github.com 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-sigs/prow repository.

minikube-bot · 2025-12-02T12:33:03Z

Can one of the admins verify this patch?

nirs · 2025-12-02T13:46:31Z

pkg/minikube/node/cache.go

 				klog.Infof("successfully saved %s as a tarball", img)
 			}
-			if downloadOnly && err == nil {
+			if downloadOnly {


If we run with --download-only for downloading required resource, I think we expect to fail if the download fails. Here we ignore the download error and return success to caller.

The if downloadOnly block is now within a bigger if err == nil block.
So this should be OK, right?

nirs · 2025-12-02T13:47:14Z

pkg/minikube/node/cache.go

 				return fmt.Errorf("not yet implemented, see issue #8426")
 			}
-			if driver.IsDocker(cc.Driver) && err == nil {
+			if driver.IsDocker(cc.Driver) {


If the download failed, why are we loading the image from cache?

nirs · 2025-12-02T13:48:55Z

pkg/minikube/node/cache.go

+					klog.Infof("%s exists in daemon, skipping load", img)
+					finalImg = img
+					return nil
+				}


This is simplied since we already handled downloadOnly, but this handling seems wrong, so we need to revisit this change after error handling is fixed.

This is not new code. The diff just makes it look new, but it is presented this way as the convoluted if statement above is broken apart for greater clarity in the code.

fwiw, the downloadOnly case was treated here without this patch. Now it is treated further below (see line 169).

nirs · 2025-12-02T13:54:13Z

pkg/minikube/node/cache.go


-			if cc.Driver == driver.Podman {
-				return fmt.Errorf("not yet implemented, see issue #8426")
-			}


This is the only correct change - removing the bad fmt.Errorf. But it would be more correct to convert this to debug log like:

Skipping <operation description>: not implemented, see issue #8426

This will improve the user experience but make it easier to debug podman driver, in case skipping has negative effects. If this operation is not needed for podman removing the log is a better change, but I'm not sure what this code is doing and wha is the unimplemented operation that we skipped.

good point. I will change the patch accordingly.

done, fwiw.

obnoxxx · 2025-12-03T10:22:06Z

@medyagh , @afbjorklund FYI.

afbjorklund · 2025-12-03T11:06:06Z

You can just skip the message, there is no need for such error output among the emojis?

There is also no need to pull the kicbase to the cache, if it is not going to be used?

obnoxxx · 2025-12-03T11:56:27Z

@afbjorklund wrote:

You can just skip the message, there is no need for such error output among the emojis?

Good point. Removed.

There is also no need to pull the kicbase to the cache, if it is not going to be used?

With the patch, pulling to cache is only done for the docker driver anyways.

new output with patch (also updated the description):

$ make clean && make
$ ./out/minikube  delete --all
🔥  Deleting "minikube" in podman ...
🔥  Removing /home/obnox/.minikube/machines/minikube ...
💀  Removed all traces of the "minikube" cluster.
🔥  Successfully deleted all profiles
$ time ./out/minikube start -d podman 
😄  minikube v1.37.0 on Fedora 43
✨  Using the podman driver based on user configuration
📌  Using Podman driver with root privileges
👍  Starting "minikube" primary control-plane node in "minikube" cluster
🚜  Pulling base image v0.0.48-1764169655-21974 ...
❗  minikube cannot pull kicbase image from any docker registry, and is trying to download kicbase tarball from github release page via HTTP.
❗  It's very likely that you have an internet issue. Please ensure that you can access the internet at least via HTTP, directly or with proxy. Currently your proxy configuration is:

    > kicbase-v0.0.48-amd64.tar:  1.22 GiB / 1.22 GiB  100.00% 18.34 MiB p/s 1m
🔥  Creating podman container (CPUs=2, Memory=7900MB) ...
🐳  Preparing Kubernetes v1.34.2 on Docker 29.0.4 ...
🔗  Configuring bridge CNI (Container Networking Interface) ...
🔎  Verifying Kubernetes components...
    ▪ Using image gcr.io/k8s-minikube/storage-provisioner:v5
🌟  Enabled addons: storage-provisioner, default-storageclass
🏄  Done! kubectl is now configured to use "minikube" cluster and "default" namespace by default

real    1m42.655s
user    0m6.445s
sys     0m6.654s
$ echo $?
0
$

afbjorklund · 2025-12-03T12:11:04Z

If you don't pull anything to the cache, then there should not be any such output either.

🚜  Pulling base image v0.0.48-1764169655-21974 ...
❗  minikube cannot pull kicbase image from any docker registry, and is trying to download kicbase tarball from github release page via HTTP.
❗  It's very likely that you have an internet issue. Please ensure that you can access the internet at least via HTTP, directly or with proxy. Currently your proxy configuration is:

    > kicbase-v0.0.48-amd64.tar:  1.22 GiB / 1.22 GiB  100.00% 18.34 MiB p/s 1m

The first line could remain, as an information. But it will be podman doing the pulling.

obnoxxx · 2025-12-03T17:03:23Z

@afbjorklund wrote:

If you don't pull anything to the cache, then there should not be any such output either.

🚜  Pulling base image v0.0.48-1764169655-21974 ...
❗  minikube cannot pull kicbase image from any docker registry, and is trying to download kicbase tarball from github release page via HTTP.
❗  It's very likely that you have an internet issue. Please ensure that you can access the internet at least via HTTP, directly or with proxy. Currently your proxy configuration is:

    > kicbase-v0.0.48-amd64.tar:  1.22 GiB / 1.22 GiB  100.00% 18.34 MiB p/s 1m

The first line could remain, as an information. But it will be podman doing the pulling.

Good points, thanks!

Updated accordingly.

minikube start -d podman issued a confusing message that image loading is unimplemented for the podman driver This change removes this message without changing anythink else in the kicbase image loading Signed-off-by: Michael Adam <[email protected]>

This change refactors beginDownloadKicBaseImage() a bit to achieve a more streamlined flow of logic, combining redundant blocks. Signed-off-by: Michael Adam <[email protected]>

k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Dec 2, 2025

k8s-ci-robot requested a review from ComradeProgrammer December 2, 2025 12:32

k8s-ci-robot added the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label Dec 2, 2025

k8s-ci-robot requested a review from nirs December 2, 2025 12:33

k8s-ci-robot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Dec 2, 2025

obnoxxx mentioned this pull request Dec 2, 2025

wip: Fix image loading with podman driver #21932

Closed

nirs reviewed Dec 2, 2025

View reviewed changes

obnoxxx force-pushed the kicbase-simplify-image-load branch from 91b215e to 45d91f0 Compare December 2, 2025 14:22

obnoxxx requested a review from nirs December 2, 2025 14:52

obnoxxx changed the title ~~Kicbase simplify image load - remove podman case with confusing message~~ Kicbase simplify image load - rimprove confusing message related to podman Dec 2, 2025

obnoxxx changed the title ~~Kicbase simplify image load - rimprove confusing message related to podman~~ Kicbase simplify image load - improve confusing message related to podman Dec 2, 2025

obnoxxx force-pushed the kicbase-simplify-image-load branch 2 times, most recently from bccc876 to 6dcfc40 Compare December 3, 2025 09:46

obnoxxx force-pushed the kicbase-simplify-image-load branch from 6dcfc40 to 6097934 Compare December 3, 2025 11:52

obnoxxx force-pushed the kicbase-simplify-image-load branch from 6097934 to 92ddd47 Compare December 3, 2025 17:02

obnoxxx changed the title ~~Kicbase simplify image load - improve confusing message related to podman~~ Kicbase simplify image load - remove confusing message related to podman Dec 4, 2025

obnoxxx added 2 commits December 4, 2025 15:49

kicbase: simplify the image loading logic

e700ad4

This change refactors beginDownloadKicBaseImage() a bit to achieve a more streamlined flow of logic, combining redundant blocks. Signed-off-by: Michael Adam <[email protected]>

obnoxxx force-pushed the kicbase-simplify-image-load branch from 92ddd47 to e700ad4 Compare December 4, 2025 18:09

k8s-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Dec 4, 2025

Kicbase simplify image load - remove confusing message related to podman #22024

Are you sure you want to change the base?

Kicbase simplify image load - remove confusing message related to podman #22024

Conversation

obnoxxx commented Dec 2, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

without the patch

with the patch

Uh oh!

k8s-ci-robot commented Dec 2, 2025

Uh oh!

k8s-ci-robot commented Dec 2, 2025

Uh oh!

minikube-bot commented Dec 2, 2025

Uh oh!

nirs Dec 2, 2025

Choose a reason for hiding this comment

Uh oh!

obnoxxx Dec 2, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nirs Dec 2, 2025

Choose a reason for hiding this comment

Uh oh!

nirs Dec 2, 2025

Choose a reason for hiding this comment

Uh oh!

obnoxxx Dec 2, 2025

Choose a reason for hiding this comment

Uh oh!

nirs Dec 2, 2025

Choose a reason for hiding this comment

Uh oh!

obnoxxx Dec 2, 2025

Choose a reason for hiding this comment

Uh oh!

obnoxxx Dec 2, 2025

Choose a reason for hiding this comment

Uh oh!

obnoxxx commented Dec 3, 2025

Uh oh!

afbjorklund commented Dec 3, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

obnoxxx commented Dec 3, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

afbjorklund commented Dec 3, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

obnoxxx commented Dec 3, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

obnoxxx commented Dec 2, 2025 •

edited

Loading

obnoxxx Dec 2, 2025 •

edited

Loading

afbjorklund commented Dec 3, 2025 •

edited

Loading

obnoxxx commented Dec 3, 2025 •

edited

Loading

afbjorklund commented Dec 3, 2025 •

edited

Loading