Fix minikube image load on windows (#20529)

[james-world]

fixes #20529

getWindowsVolumeNameCmd uses the deprecated wmic.exe, this breaks minikube image load on newer installs of windows. Whilst wmic.exe can be added, it will be removed from Windows 11 soon.

See here for details.

This fix replaces the use of wmic.exe with a call to the Powershell equivalent.

[linux-foundation-easycla]

The committers listed above are authorized under a signed CLA.

• ✅ login: james-world / name: James World (211296e, 8b7687e)

[k8s-ci-robot]

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: james-world
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:

• 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]

Welcome @james-world!

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

[k8s-ci-robot]

Hi @james-world. 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.

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?

[medyagh]

@james-world thank you for this PR do you mind putting the output of minikube Before/After this PR ? and also how about adding a Fail Over for users with Older Windows, so if this Method didnt work, minikube would use the older method?

and would this work in "git bash in windows" or only in powershell ?

also I think this is an opportunity to add unit test here

[james-world]

Hi @medyagh,

Addressing your points in turn:

1. Output before and after

Before (on any windows system where wmic.exe is not in path, whether run from Git Bash / MingW64 / Cmd / Powershell):

.\minikube.exe image load employee-api:latest

❌  Exiting due to GUEST_IMAGE_LOAD: Failed to load image: save to dir: caching images: caching image "C:\\Users\\james.world\\.minikube\\cache\\images\\amd64\\employee-api_latest": getting destination path: parsing docker archive dst ref: replace a Win drive letter to a volume name: exec: "wmic": executable file not found in %PATH%

╭───────────────────────────────────────────────────────────────────────────────────────────────────────────────╮
│                                                                                                               │
│    😿  If the above advice does not help, please let us know:                                                 │
│    👉  https://github.com/kubernetes/minikube/issues/new/choose                                               │
│                                                                                                               │
│    Please run `minikube logs --file=logs.txt` and attach logs.txt to the GitHub issue.                        │
│    Please also attach the following file to the GitHub issue:                                                 │
│    - C:\Users\JAMES~1.WOR\AppData\Local\Temp\minikube_image_1b5c8c33fb94831b9d273a970d695f2957a83a6b_0.log    │
│                                                                                                               │
╰───────────────────────────────────────────────────────────────────────────────────────────────────────────────╯

Without this fix, this will be the default experience on latest Windows builds now because wmic.exe is no longer included by default; e.g. developers on my team with new laptops have run into this, existing developers with us more than a month ago are OK.

Output after fix applied (again, on any windows system where wmic.exe is not in path, whether run from Git Bash / MingW64 / Cmd / Powershell):

There is no output! , as with the existing implementation the minikube image load command actually doesn't output anything when successful. This is a helper method executed by upstream functionality for image load - and that doesn't output anything on success bar the return code.

In terms of this specific function (getWindowsVolumeNameCmd), the output is of course identical to what it would have been using wmic.exe. Something like this:

\\?\Volume{ca87fc49-8366-4377-8c24-b7be4be5627b}\

2. Adding fallback behaviour

As for adding the failover - on the surface it sounds like a good idea. I can do it if you insist, however consider that as far as I know the Get-CimInstance was first added in Windows Powershell 3.0 and has been included in windows by default since Windows 8 and Windows Server 2012. Do we really need this fallback, do you think?

3. Would this work in "git bash in windows" or only in powershell ?

It works regardless of the windows shell from which minikube is executed - the go code is launching the Powershell process itself; it should always be in path. I've certainly tested from Git Bash, MingW64, Powershell and Cmd and all are fine.

4. I think this is an opportunity to add unit test here.

What would a unit test do beyond what's already in localpath_test.go? This feels more like integration testing territory to me? Because in this case, the function is dependent directly on Windows+Powershell in calling out to Powershell's Get-CimInstance command.

Honestly, I have very little go experience. It's been novel for me to build minikube on Windows; the docs aren't very informative for Windows platform builds. Because this is a drop in replacement for existing functionality, I had hoped the existing test coverage would be fine. If not, do you have a good example you can point in the codebase to for an existing integration test that is Windows specific that would serve as a good template for how to go about adding this idiomatically?

[medyagh]

thank you for the response, that sounds convincing, I think it wouldn't hurt to fall back to older vers way but I could be convinced either way.
@bobsira do you mind commenting on this, what do you think ?

[james-world]

On further consideration @medyagh, it's pretty trivial to add the fallback, so I have.

[james-world]

So I made it so if both the PowerShell and the WMIC methods both fail, both errors are reported, to aid troubleshooting:

❌  Exiting due to GUEST_IMAGE_LOAD: Failed to load image: save to dir: caching images: caching image "C:\\Users\\james.world\\.minikube\\cache\\images\\amd64\\employee-api_latest": getting destination path: parsing docker archive dst ref: replace a Win drive letter to a volume name: PowerShell failed (exit status 1), WMIC also failed: exec: "wmic": executable file not found in %PATH%

╭───────────────────────────────────────────────────────────────────────────────────────────────────────────────╮
│                                                                                                               │
│    😿  If the above advice does not help, please let us know:                                                 │
│    👉  https://github.com/kubernetes/minikube/issues/new/choose                                               │
│                                                                                                               │
│    Please run `minikube logs --file=logs.txt` and attach logs.txt to the GitHub issue.                        │
│    Please also attach the following file to the GitHub issue:                                                 │
│    - C:\Users\JAMES~1.WOR\AppData\Local\Temp\minikube_image_1b5c8c33fb94831b9d273a970d695f2957a83a6b_0.log    │
│                                                                                                               │
╰───────────────────────────────────────────────────────────────────────────────────────────────────────────────╯

[gabrielgbs97]

I believe the older method should not be used as a fallback. WMIC has been deprecated since the release of Windows 10 RTM, which was 10 years ago, and it is not included in the default installation of Windows 11 23H2. Therefore, it would not have been implemented using wmic.exe around 2016 - 2019 (?)

[james-world]

I'm comfortable either way; it does no harm and may help someone that has disabled PowerShell access in some way - thus avoiding a breaking change. If an approver can let me know which way to jump please?

[bobsira]

WMIC is deprecated so we can get rid of the older method fallback bit. Powershell is the way to go since it's recommended command-line utility on Windows

[bobsira]

https://github.com/minikube-machine/machine/blob/main/drivers/hyperv/powershell.go

This currently exist on how we discover Powershell and execute PS commands, you can borrow from this. Your change looks alright to me.

[gabrielgbs97]

If powershell.exe is not available in PATH (it should be...) it can be discovered via SystemRoot env variable:
'%SystemRoot%\system32\WindowsPowerShell\v1.0\powershell.exe