Skip to content

Use WMI to implement Volume API to reduce PowerShell overhead #360

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

laozc
Copy link
Contributor

@laozc laozc commented Oct 22, 2024

/kind feature

What this PR does / why we need it:
This PR leverages of WMI interfaces to replace the PowerShell functions for Windows related storage operations,
which improve the overall performance of csi-proxy.

Which issue(s) this PR fixes:
Fixes #193

Special notes for your reviewer:

Does this PR introduce a user-facing change?:

NONE

@k8s-ci-robot k8s-ci-robot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. kind/feature Categorizes issue or PR as related to a new feature. do-not-merge/release-note-label-needed Indicates that a PR should not merge because it's missing one of the release note labels. labels Oct 22, 2024
@k8s-ci-robot k8s-ci-robot requested review from humblec and pohly October 22, 2024 03:54
@k8s-ci-robot k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Oct 22, 2024
@k8s-ci-robot
Copy link
Contributor

Welcome @laozc!

It looks like this is your first PR to kubernetes-csi/csi-proxy 🎉. 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-csi/csi-proxy 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 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 Oct 22, 2024
@k8s-ci-robot
Copy link
Contributor

Hi @laozc. Thanks for your PR.

I'm waiting for a kubernetes-csi 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.

@k8s-ci-robot k8s-ci-robot added the size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. label Oct 22, 2024
@andyzhangx
Copy link
Member

thanks for the PR. @laozc have you tested the WMI based operation manually? does it work?

@laozc
Copy link
Contributor Author

laozc commented Oct 23, 2024

Yes. I verified it in my local environment and they work the same as PowerShell commands.
I'm still working on finalizing the PR with more testing and integrate it into CSI driver to get some result.

@mauriciopoppe
Copy link
Member

/cc @andyzhangx @mauriciopoppe
/uncc @humblec @pohly

@k8s-ci-robot k8s-ci-robot requested review from andyzhangx and mauriciopoppe and removed request for pohly and humblec October 23, 2024 15:02
@k8s-ci-robot k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Oct 28, 2024
@k8s-ci-robot k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Nov 1, 2024
@laozc laozc force-pushed the wmi branch 8 times, most recently from 7ee9449 to c55b906 Compare November 10, 2024 10:24
@laozc laozc marked this pull request as ready for review November 10, 2024 10:35
@k8s-ci-robot k8s-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Nov 10, 2024
@k8s-ci-robot k8s-ci-robot added release-note-none Denotes a PR that doesn't merit a release note. and removed do-not-merge/release-note-label-needed Indicates that a PR should not merge because it's missing one of the release note labels. labels Mar 13, 2025
@laozc laozc changed the title Use WMI instead of PowerShell for OS operations Use WMI to implement Volume API for better performance Mar 13, 2025
@laozc laozc changed the title Use WMI to implement Volume API for better performance Use WMI to implement Volume API to reduce PowerShell overhead Mar 13, 2025
@laozc
Copy link
Contributor Author

laozc commented Mar 13, 2025

The commit which contains all API changes in the same PR was 6986e59

@laozc
Copy link
Contributor Author

laozc commented Mar 13, 2025

I believe we could safely remove the constraint to stick with Go 1.22 with this change.

mountedFolder, err := isMountedFolder(candidatePath)

This method uses Windows API calls to determine whether the path is a mounted path instead of relying on the Go API behavior.

@laozc laozc force-pushed the wmi branch 4 times, most recently from 3024353 to 355a96a Compare March 13, 2025 12:00
@mauriciopoppe
Copy link
Member

/ok-to-test

@k8s-ci-robot k8s-ci-robot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Mar 13, 2025
Copy link
Member

@mauriciopoppe mauriciopoppe left a comment

Choose a reason for hiding this comment

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

I left some comments.

@laozc laozc force-pushed the wmi branch 4 times, most recently from d878563 to c5b7070 Compare March 14, 2025 17:16
@mauriciopoppe
Copy link
Member

Hey @andyzhangx, would it be possible for you to review the PR please? Maybe you have experience with the github.com/microsoft/wmi package and how to use it. From my end, I can only check that the integration tests are still passing.

"github.com/microsoft/wmi/pkg/base/query"
"github.com/microsoft/wmi/pkg/errors"
cim "github.com/microsoft/wmi/pkg/wmiinstance"
"github.com/microsoft/wmi/server2019/root/microsoft/windows/storage"
Copy link
Member

Choose a reason for hiding this comment

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

this relies on specific server version, does it work on server 2022 and 2025?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I always tested on Windows Server 22H2 with this API.
I don't think it would break as long as the new feature introduced in newer API is not used.

Copy link
Member

@andyzhangx andyzhangx left a comment

Choose a reason for hiding this comment

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

/hold
I will try this PR on Azure CSI driver first to make sure it works on common Windows server versions.

@k8s-ci-robot k8s-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Apr 15, 2025
@andyzhangx
Copy link
Member

@laozc I tries to copy the cim folder into my Azure Disk CSI driver repo (for test): https://github.com/kubernetes-sigs/azuredisk-csi-driver/compare/master...andyzhangx:azuredisk-csi-driver:cim?expand=1, but I got following error, have you hit similar error in your CSI driver?

https://github.com/andyzhangx/azuredisk-csi-driver/actions/runs/14463170910/job/40559504163

# sigs.k8s.io/azuredisk-csi-driver/pkg/os/cim
FAIL	sigs.k8s.io/azuredisk-csi-driver/pkg/os/cim [setup failed]
package sigs.k8s.io/azuredisk-csi-driver/pkg/os/cim
	imports github.com/microsoft/wmi/pkg/wmiinstance
	imports golang.org/x/sys/windows: build constraints exclude all Go files in /home/runner/work/azuredisk-csi-driver/azuredisk-csi-driver/vendor/golang.org/x/sys/windows
# sigs.k[8](https://github.com/andyzhangx/azuredisk-csi-driver/actions/runs/14463170910/job/40559504163#step:4:9)s.io/azuredisk-csi-driver/pkg/os/cim
package sigs.k8s.io/azuredisk-csi-driver/pkg/os/cim
	imports github.com/microsoft/wmi/server201[9](https://github.com/andyzhangx/azuredisk-csi-driver/actions/runs/14463170910/job/40559504163#step:4:10)/root/microsoft/windows/storage
	imports github.com/microsoft/wmi/pkg/base/instance
	imports github.com/microsoft/wmi/pkg/base/session: build constraints exclude all Go files in /home/runner/work/azuredisk-csi-driver/azuredisk-csi-driver/vendor/github.com/microsoft/wmi/pkg/base/session

@laozc
Copy link
Contributor Author

laozc commented Apr 15, 2025

@laozc I tries to copy the cim folder into my Azure Disk CSI driver repo (for test): https://github.com/kubernetes-sigs/azuredisk-csi-driver/compare/master...andyzhangx:azuredisk-csi-driver:cim?expand=1, but I got following error, have you hit similar error in your CSI driver?

https://github.com/andyzhangx/azuredisk-csi-driver/actions/runs/14463170910/job/40559504163

# sigs.k8s.io/azuredisk-csi-driver/pkg/os/cim
FAIL	sigs.k8s.io/azuredisk-csi-driver/pkg/os/cim [setup failed]
package sigs.k8s.io/azuredisk-csi-driver/pkg/os/cim
	imports github.com/microsoft/wmi/pkg/wmiinstance
	imports golang.org/x/sys/windows: build constraints exclude all Go files in /home/runner/work/azuredisk-csi-driver/azuredisk-csi-driver/vendor/golang.org/x/sys/windows
# sigs.k[8](https://github.com/andyzhangx/azuredisk-csi-driver/actions/runs/14463170910/job/40559504163#step:4:9)s.io/azuredisk-csi-driver/pkg/os/cim
package sigs.k8s.io/azuredisk-csi-driver/pkg/os/cim
	imports github.com/microsoft/wmi/server201[9](https://github.com/andyzhangx/azuredisk-csi-driver/actions/runs/14463170910/job/40559504163#step:4:10)/root/microsoft/windows/storage
	imports github.com/microsoft/wmi/pkg/base/instance
	imports github.com/microsoft/wmi/pkg/base/session: build constraints exclude all Go files in /home/runner/work/azuredisk-csi-driver/azuredisk-csi-driver/vendor/github.com/microsoft/wmi/pkg/base/session

Hi, these Go files in github.com/microsoft/wmi/pkg/base/instance and golang.org/x/sys/windows are set with a build flat to only work under GOOS=windows so they should be excluded from UT.
I don't think the code to call WMI pkg/os/cim could be tested in UT stage as they're wrapping WMI.
The E2E cases should cover the functionality.

You may either
go -covermode=atomic -coverprofile=profile.cov $(go list ./... | grep -v sigs.k8s.io/azuredisk-csi-driver/pkg/os/cim) to exclude these package
or either add

//go:build windows
// +build windows

to each file in this package so the test coverage is skipped for these files on Linux.

@laozc
Copy link
Contributor Author

laozc commented Apr 15, 2025

I could add build constraints to these codes if the downstream projects need.

//go:build windows
// +build windows

@andyzhangx
Copy link
Member

//go:build windows
// +build windows

yes, that's necessary, and pls also add header, you could refer to
kubernetes-sigs/azuredisk-csi-driver@d442711

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. do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. kind/feature Categorizes issue or PR as related to a new feature. ok-to-test Indicates a non-member PR verified by an org member that is safe to test. release-note-none Denotes a PR that doesn't merit a release note. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

High cpu usage of powershell processes triggered by csi-proxy
4 participants