Close Open disk file handles by wongma7 · Pull Request #126 · kubernetes-csi/csi-proxy

wongma7 · 2021-04-14T19:47:02Z

What type of PR is this?
/kind bug

What this PR does / why we need it:

If csi-proxy keeps these files open it blocks EBS detach from succeeding.

I tested this fix and it works for me.

Full story:

While debugging my EBS CSI implementation I found everything worked fine except for volume Detach. EBS support said OS was reporting something holding on to the disk (despite me trying removing all accesspoints, taking disk offline, etc.). Eventually after much guessing and testing, narrowed it to csi-proxy, i.e. as soon as I stopped it Detach would proceed. (I'm no Windows expert so possibly there was an easier way to find out what was holding onto device, like via process explorer, but yeah...)

Which issue(s) this PR fixes:

Fixes #

Special notes for your reviewer:

Does this PR introduce a user-facing change?:

Fix GetDiskNumberByName and ListDiskIDs keeping file handles open on disks

k8s-ci-robot · 2021-04-14T19:47:10Z

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

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/test-infra repository.

ddebroy · 2021-04-19T22:35:52Z

/ok-to-test

jingxu97 · 2021-05-19T04:36:06Z

@wongma7 could you address the comment?

jingxu97 · 2021-05-21T18:54:46Z

/lgtm

wongma7 · 2021-05-21T22:14:19Z

/assign @msau42

please approve! 2 liner PR! many thanks!

msau42 · 2021-05-26T22:12:23Z


 	for i := range disks {
 		h, err := syscall.Open(disks[i].Path, syscall.O_RDONLY, 0)
+		defer syscall.Close(h)


What's the maximum number of disks a node can have? Would it be worth enclosing this loop in a code section so that it gets closed at the end of each loop?

for ec2 depends on instance type but typically like 20-30
https://docs.aws.amazon.com/AWSEC2/latest/WindowsGuide/volume_limits.html#instance-type-volume-limits (EKS only supports windows nitro instances so the "AWS PV" driver in the other section don't apply)

do you mean to wrap it like this? I don't think it's worth the loss in readability

func() (uint32, error) { ... syscall.Open defer syscall.Close(h) }()

Or a separate function could also work.

The default fd limit per process is 1024 for Linux. I don't know about Windows. Does Windows also have a maximum disk limit? cc @ddebroy @jingxu97

here it says that it's 16M https://docs.microsoft.com/en-us/archive/blogs/markrussinovich/pushing-the-limits-of-windows-handles#maximum-number-of-handles

did some refactoring so we always open and close 1 handle at a time

k8s-ci-robot · 2021-05-27T20:42:35Z

New changes are detected. LGTM label has been removed.

k8s-ci-robot · 2021-05-27T20:42:39Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: wongma7
To complete the pull request process, please ask for approval from jingxu97 after the PR has been reviewed.

The full list of commands accepted by this bot can be found here.

Details

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

wongma7 · 2021-05-27T20:43:41Z

/hold

need to retest on my setup

jingxu97 · 2021-05-31T21:29:24Z

@wongma7 thanks for this PR. Pls let me know after you test it.

wongma7 · 2021-06-01T19:52:06Z

/hold cancel

together with #145 I tested that attach, mount, unmount, detach works with ebs

jingxu97 · 2021-06-01T20:18:09Z


+var ErrNotFound = errors.New("not found")
+
+func (imp APIImplementor) getDiskNumberByID(path string, page83ID string) (uint32, error) {


I am kind of confused by these two functions getDiskNumberAndIDByPath and getDiskNumberByID.

Is there a way to kind of combine those?

okay, let me combine.

(the PR started as 2 lines, bit by bit it has ballooned, so I am getting confused too 😂)

jingxu97 · 2021-06-01T20:19:17Z

@mauriciopoppe is working on disk API part #140
wondering whether you want to wait for that PR merge since there might be some name changes?

wongma7 · 2021-06-01T20:26:48Z

@jingxu97 I'm going to revert to the original PR where I just call syscall.Close(h) at end of the loop. I don't want to refactor and then rebase if beta3 API will make it obsolete

k8s-ci-robot added kind/bug Categorizes issue or PR as related to a bug. do-not-merge/release-note-label-needed Indicates that a PR should not merge because it's missing one of the release note labels. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels Apr 14, 2021

k8s-ci-robot requested review from PatrickLang and msau42 April 14, 2021 19:47

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 Apr 14, 2021

wongma7 mentioned this pull request Apr 14, 2021

Implement Windows NodePublish/Unpublish kubernetes-sigs/aws-ebs-csi-driver#823

Merged

2 tasks

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 Apr 19, 2021

andyzhangx reviewed Apr 30, 2021

View reviewed changes

Comment thread internal/os/disk/api.go Outdated

wongma7 force-pushed the filehandle branch from 8109053 to 67e522d Compare May 21, 2021 18:33

k8s-ci-robot added size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. and removed size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels May 21, 2021

k8s-ci-robot assigned jingxu97 May 21, 2021

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label May 21, 2021

k8s-ci-robot assigned msau42 May 21, 2021

msau42 reviewed May 26, 2021

View reviewed changes

wongma7 force-pushed the filehandle branch from 67e522d to 334f9a5 Compare May 27, 2021 20:42

k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label May 27, 2021

k8s-ci-robot removed the size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. label May 27, 2021

k8s-ci-robot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label May 27, 2021

k8s-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label May 27, 2021

k8s-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jun 1, 2021

jingxu97 reviewed Jun 1, 2021

View reviewed changes

Comment thread internal/os/disk/api.go Outdated

jingxu97 reviewed Jun 1, 2021

View reviewed changes

Comment thread internal/os/disk/api.go Outdated

Close Open disk file handles

172c835

wongma7 force-pushed the filehandle branch from 334f9a5 to 172c835 Compare June 1, 2021 19:59

jingxu97 reviewed Jun 1, 2021

View reviewed changes

wongma7 force-pushed the filehandle branch from 172c835 to 5320b77 Compare June 1, 2021 20:30

k8s-ci-robot added size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Jun 1, 2021

wongma7 force-pushed the filehandle branch from 5320b77 to 172c835 Compare June 1, 2021 20:31

k8s-ci-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. labels Jun 1, 2021

wongma7 closed this Jun 1, 2021

wongma7 mentioned this pull request Jun 3, 2021

Close Open disk file handles #147

Merged


		var ErrNotFound = errors.New("not found")

		func (imp APIImplementor) getDiskNumberByID(path string, page83ID string) (uint32, error) {

Conversation

wongma7 commented Apr 14, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

k8s-ci-robot commented Apr 14, 2021

Uh oh!

ddebroy commented Apr 19, 2021

Uh oh!

Uh oh!

jingxu97 commented May 19, 2021

Uh oh!

jingxu97 commented May 21, 2021

Uh oh!

wongma7 commented May 21, 2021

Uh oh!

msau42 May 26, 2021

Choose a reason for hiding this comment

Uh oh!

wongma7 May 26, 2021

Choose a reason for hiding this comment

Uh oh!

msau42 May 26, 2021

Choose a reason for hiding this comment

Uh oh!

mauriciopoppe May 27, 2021

Choose a reason for hiding this comment

Uh oh!

wongma7 May 27, 2021

Choose a reason for hiding this comment

Uh oh!

k8s-ci-robot commented May 27, 2021

Uh oh!

k8s-ci-robot commented May 27, 2021

Uh oh!

wongma7 commented May 27, 2021

Uh oh!

jingxu97 commented May 31, 2021

Uh oh!

wongma7 commented Jun 1, 2021

Uh oh!

Uh oh!

Uh oh!

jingxu97 Jun 1, 2021

Choose a reason for hiding this comment

Uh oh!

wongma7 Jun 1, 2021

Choose a reason for hiding this comment

Uh oh!

jingxu97 commented Jun 1, 2021

Uh oh!

wongma7 commented Jun 1, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

wongma7 commented Apr 14, 2021 •

edited

Loading