Skip to content

UI: remove confusing message related to podman#22024

Open
obnoxxx wants to merge 1 commit intokubernetes:masterfrom
obnoxxx:kicbase-simplify-image-load
Open

UI: remove confusing message related to podman#22024
obnoxxx wants to merge 1 commit intokubernetes:masterfrom
obnoxxx:kicbase-simplify-image-load

Conversation

@obnoxxx
Copy link
Copy Markdown
Contributor

@obnoxxx obnoxxx commented Dec 2, 2025

Image loading from chache is not implemented and minikube start -d podmanalways issued a confusing and unhelpful error message.

This change removes the confusing and unhelpful message, replacing it with a log message.

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

The outputs of minikube start -d podman on current Fedora Linux 43 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-1765275396-22083 ...
🔥  Creating podman container (CPUs=2, Memory=7900MB) ...
🐳  Preparing Kubernetes v1.34.2 on Docker 29.1.2 ...
🔗  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    0m30.899s
user    0m1.452s
sys     0m1.009s
$ echo $?
0
$ 

@k8s-ci-robot 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
Copy link
Copy Markdown
Contributor

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.

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.

@k8s-ci-robot 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 k8s-ci-robot requested a review from nirs December 2, 2025 12:33
@minikube-bot
Copy link
Copy Markdown
Collaborator

Can one of the admins verify this patch?

@k8s-ci-robot k8s-ci-robot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Dec 2, 2025
Comment thread pkg/minikube/node/cache.go Outdated
klog.Infof("successfully saved %s as a tarball", img)
}
if downloadOnly && err == nil {
if downloadOnly {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Copy Markdown
Contributor Author

@obnoxxx obnoxxx Dec 2, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Comment thread pkg/minikube/node/cache.go Outdated
return fmt.Errorf("not yet implemented, see issue #8426")
}
if driver.IsDocker(cc.Driver) && err == nil {
if driver.IsDocker(cc.Driver) {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Comment thread pkg/minikube/node/cache.go Outdated
klog.Infof("%s exists in daemon, skipping load", img)
finalImg = img
return nil
}
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).


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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

good point. I will change the patch accordingly.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done, fwiw.

@obnoxxx obnoxxx force-pushed the kicbase-simplify-image-load branch from 91b215e to 45d91f0 Compare December 2, 2025 14:22
@obnoxxx obnoxxx requested a review from nirs December 2, 2025 14:52
@obnoxxx 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 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 obnoxxx force-pushed the kicbase-simplify-image-load branch 2 times, most recently from bccc876 to 6dcfc40 Compare December 3, 2025 09:46
@obnoxxx
Copy link
Copy Markdown
Contributor Author

obnoxxx commented Dec 3, 2025

@medyagh , @afbjorklund FYI.

@afbjorklund
Copy link
Copy Markdown
Collaborator

afbjorklund commented Dec 3, 2025

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 obnoxxx force-pushed the kicbase-simplify-image-load branch from 6dcfc40 to 6097934 Compare December 3, 2025 11:52
@obnoxxx
Copy link
Copy Markdown
Contributor Author

obnoxxx commented Dec 3, 2025

@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
Copy link
Copy Markdown
Collaborator

afbjorklund commented Dec 3, 2025

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 obnoxxx force-pushed the kicbase-simplify-image-load branch from 6097934 to 92ddd47 Compare December 3, 2025 17:02
@obnoxxx
Copy link
Copy Markdown
Contributor Author

obnoxxx commented Dec 3, 2025

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

@obnoxxx 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 obnoxxx force-pushed the kicbase-simplify-image-load branch from 92ddd47 to e700ad4 Compare December 4, 2025 18:09
@k8s-ci-robot 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
@obnoxxx obnoxxx force-pushed the kicbase-simplify-image-load branch from e700ad4 to 0c31244 Compare December 8, 2025 10:45
@obnoxxx obnoxxx changed the title Kicbase simplify image load - remove confusing message related to podman UI: remove confusing message related to podman Dec 10, 2025
@obnoxxx obnoxxx force-pushed the kicbase-simplify-image-load branch from 0c31244 to 7a4216e Compare December 10, 2025 20:03
@k8s-ci-robot k8s-ci-robot removed the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Dec 10, 2025
@k8s-ci-robot k8s-ci-robot added the size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. label Dec 10, 2025
Comment thread pkg/minikube/node/cache.go Outdated

if cc.Driver == driver.Podman {
return fmt.Errorf("not yet implemented, see issue #8426")
klog.Infof("iskipping image load from cache: not implemented for podman. see issue #8426")
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

you can change this to warnningf

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

klog.Warningf is fine, but it should go only to the log (unless --alsologtostderr was used).

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@medyagh wrote:

you can change this to warnningf

done.

@obnoxxx obnoxxx force-pushed the kicbase-simplify-image-load branch from 7a4216e to 6b65410 Compare December 15, 2025 18:49
@obnoxxx obnoxxx requested a review from medyagh December 15, 2025 19:03
@obnoxxx
Copy link
Copy Markdown
Contributor Author

obnoxxx commented Dec 16, 2025

/assign @medyagh

Comment thread pkg/minikube/node/cache.go Outdated

if cc.Driver == driver.Podman {
return fmt.Errorf("not yet implemented, see issue #8426")
klog.Warningf("iskipping image load from cache: not implemented for podman. see issue #8426")
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

typo!

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for spotting! fixed.

https://help.github.com/en/packages/using-github-packages-with-your-projects-ecosystem/configuring-docker-for-use-with-github-packages#authenticating-to-github-packages
`)
}
if errors.Is(err, image.ErrGithubNeedsLogin) || errors.Is(err, image.ErrNeedsLogin) {
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

add an errors.Is here for podman and that specific error. and if that is the case only log it with warnning.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@medyagh wrote:

add an errors.Is here for podman and that specific error. and if that is the case only log it with warnning.

Thanks for the suggestion. looking into it.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@medyagh qrote:

add an errors.Is here for podman and that specific error. and if that is the case only log it with warnning.

I am afraid I did not quite get your request yet:
This a different function.

Are you saying to log only in this function (waitDownloadKicBaseImage) and not in the other function (beginDownloadKicBaseImage) ?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

clarification by @medyagh from chat:

show that you investigated the place that we checked the Value of returned error, and instead of returning nil we could check for the Type of Error there and Log it in the caller

minikube start -d podman issued a confusing error message that image loading
is unimplemented for the  podman driver

This change replaces  this error message with a warning message
 without changing anything else in the kicbase image loading

Signed-off-by: Michael Adam <obnox@samba.org>
@obnoxxx obnoxxx force-pushed the kicbase-simplify-image-load branch from 6b65410 to 3d43744 Compare January 8, 2026 14:42
@k8s-ci-robot
Copy link
Copy Markdown
Contributor

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: nirs, obnoxxx
Once this PR has been reviewed and has the lgtm label, please ask for approval from medyagh. 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:

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

@k8s-triage-robot
Copy link
Copy Markdown

The Kubernetes project currently lacks enough contributors to adequately respond to all PRs.

This bot triages PRs according to the following rules:

  • After 90d of inactivity, lifecycle/stale is applied
  • After 30d of inactivity since lifecycle/stale was applied, lifecycle/rotten is applied
  • After 30d of inactivity since lifecycle/rotten was applied, the PR is closed

You can:

  • Mark this PR as fresh with /remove-lifecycle stale
  • Close this PR with /close
  • Offer to help out with Issue Triage

Please send feedback to sig-contributor-experience at kubernetes/community.

/lifecycle stale

@k8s-ci-robot k8s-ci-robot added the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Apr 14, 2026
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. lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. size/XS Denotes a PR that changes 0-9 lines, ignoring generated files.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants