antrea-io / antrea

Kubernetes networking based on Open vSwitch
https://antrea.io
Apache License 2.0
1.66k stars 367 forks source link

Install-OVS.ps1 fails on systems localized to anything but English #6562

Open mkaring opened 3 months ago

mkaring commented 3 months ago

Describe the bug

The getInstalledOVSDrivers part of the https://github.com/antrea-io/antrea/blob/6e4ff875fc8b575010daa21648aae725972d49f5/hack/windows/Install-OVS.ps1#L309 script fails on system not running the English localization of Windows. The problem is caused by the pnputil.exe application. The output of this command is localized.

On my server running in German the output looks like this:

>pnputil.exe /enum-drivers
Microsoft-PnP-Hilfsprogramm

Veröffentlichter Name:     oem72.inf
Originalname:      accessorycenterhsadriver.inf
Anbietername:      Microsoft
Klassenname:         Erweiterungen
Klassen-GUID:         {e2f84ce7-8efa-411c-aa69-97454ca4cb57}
Erweiterungs-ID:       {38c7d909-96db-411a-b5a2-d9ec748851d6}
Treiberversion:     09/19/2022 4.410.137.0
Name des Signaturgebers:        Microsoft Windows Hardware Compatibility Publisher

Veröffentlichter Name:     oem74.inf
Originalname:      acpivpc.inf
Anbietername:      Lenovo
Klassenname:         Systemgeräte
Klassen-GUID:         {4d36e97d-e325-11ce-bfc1-08002be10318}
Treiberversion:     07/22/2021 15.11.29.65
Name des Signaturgebers:        Microsoft Windows Hardware Compatibility Publisher

…

In the end this causes the detection if the driver is up to date or not to fail.

To Reproduce

On a server running a language other than English, run the Install-OVS.ps1 script twice. You'll find that the detection of the already installed driver fails at the second run.

Expected

The script either warns about potential problem if it's running on a system with another language, or it works correctly.

Actual behavior

Parsing the existing drivers completely fails with all fields in the driver custom objects set to $null. This causes the script to consider any installed driver to be out of date.

Versions:

The affected version of the script is linked in the description.

Suggested solution:

Using the powershell commands Get-PnpDevice and Get-PnpDeviceProperty it should be possible to get all the required information without relying on localization of the operating system.

luolanzone commented 3 months ago

@wenyingd @XinShuYang could you take a look?

XinShuYang commented 3 months ago

Thanks for reporting the issue. The problem described in the issue indeed exists in non-English environments. AFAIK we currently do not claim to support multiple languages. It would require a significant effort and updated CI for verification. Regarding future plans, maybe @antoninbas @tnqn can share more thoughts?

wenyingd commented 3 months ago

@mkaring Thanks for reporting the issue. We have tried your suggested utility (get-pnpDevice and get-pnpDeviceProperty), unfortunately, we don't think the two are suitable for our case. It is because the two are used to list Windows devices, but for OVSext, it is driver, and no system device is created until the ovsext driver is enabled on a vmswitch.

Until now, antrea doesn't have the logic to support localization, we may need the maintainer's input to decide if the requirement is acceptable.

tnqn commented 3 months ago

AFAIK we currently do not claim to support multiple languages. It would require a significant effort and updated CI for verification. Regarding future plans, maybe @antoninbas @tnqn can share more thoughts?

I think we don't claim to not support multiple languages either. I feel it's a bit of common to assume environment language is not a problem. To avoid such problems, code/script should be implemented by relying return code, instead of matching human readable texts. Would it be possible to make the change, or there is no good way to do that with this windows utility? Let's see what @jianjuns and @antoninbas would say too.

github-actions[bot] commented 1 day ago

This issue is stale because it has been open 90 days with no activity. Remove stale label or comment, or this will be closed in 90 days