Use FILE_SHARE_DELETE for log files on Windows. by cpuguy83 · Pull Request #39974 · moby/moby

cpuguy83 · 2019-09-23T18:39:53Z

This (hopefully! 🤞 ) fixes issues where one goroutine tries to delete or rename a file
while another goroutine has the file open (e.g. a log reader).

Closes #39856
Closes #36801
Fixes #39274

thaJeztah · 2019-09-23T18:41:45Z

@ddebroy @olljanat @vikramhh PTAL

olljanat · 2019-09-23T19:28:45Z

To be honest I don't understand how this would work even on theory?

Sharemode is hardcoded on https://github.com/golang/go/blob/4219aec60ab473fa00f1092034ca801218a5dbe9/src/syscall/syscall_windows.go#L297 and that why there is that very long discussion about changing that hardcoded flag on golang/go#32088

Btw. You can use my unit test for this one https://github.com/olljanat/go/commit/3828f1a5d0ebb69b4c459d5243799ded36ac1ee8 if that passes on Windows you are good to go.

cpuguy83 · 2019-09-23T23:42:31Z

@olljanat You're right.
So now I've copied syscall.Open with just the one modification for FILE_SHARE_DELETE.
Added a unit test for the functionality.

I had to pull in a few other functions from os to mimic os.OpenFile just in case there is some edge case... probably not even important.

This fixes issues where one goroutine tries to delete or rename a file while another goroutine has the file open (e.g. a log reader). Signed-off-by: Brian Goff <cpuguy83@gmail.com>

thaJeztah · 2019-09-24T08:37:27Z

daemon/logger/loggerutils/file_windows.go

+	"github.com/pkg/errors"
+)
+
+func openFile(name string, flag int, perm os.FileMode) (*os.File, error) {


Should we move this to pkg/system? In that package we also have some other windows-specific utilities (some of which) can be used as drop-in replacements for their default go implementations, e.g.

moby/pkg/system/filesys_windows.go

Lines 29 to 33 in e554ab5

// MkdirAll implementation that is volume path aware for Windows. It can be used

// as a drop-in replacement for os.MkdirAll()

func MkdirAll(path string, _ os.FileMode) error {

return mkdirall(path, false, "")

}

I'm intending this as a targeted workaround for a very specific and real issue rather than a generic solution.

It would be nice if go picked up some change to support this option.

I'm intending this as a targeted workaround for a very specific and real issue rather than a generic solution.

Gotcha. I was thinking if other places would also benefit from this, having it in that package would make it a slightly bit more reusable; not a blocker though

It would be nice if go picked up some change to support this option

Yes, definitely

olljanat · 2019-09-24T15:40:37Z

@cpuguy83 Now it it looks likely that it will use FILE_SHARE_DELETE share mode but note that it actually only solves a part of the problem. If you remove file which is created with that flag and try open new one without closing original open handle it will still fail.

Only way to prevent that is rename file for some name which is not used by any other process before removing it. So if I understand this right it should be added to just before os.Remove to:

moby/daemon/logger/loggerutils/logfile.go

Lines 218 to 222 in a36dfe7

    
           lastFile := fmt.Sprintf("%s.%d%s", name, maxFiles-1, extension) 
        
           err := os.Remove(lastFile) 
        
           if err != nil && !os.IsNotExist(err) { 
        
           	return errors.Wrap(err, "error removing oldest log file") 
        
           }

cpuguy83 · 2019-09-24T15:50:03Z

@olljanat Do you have some example code to reproduce this?
The simple one I came up with (call openFile again after the file is removed in the test I added TestOpenFileDelete ) does not repro that case.

olljanat · 2019-09-24T16:45:13Z

@cpuguy83 Hmm. It was mentioned on golang/go#32088 (comment) that defaults have been changed on latest versions of Windows and it really looks to be case at least on Windows 10, version 1903 so this simple PowerShell script works just fine on it but fails example on Windows Server 2019:

$file = [System.IO.File]::Open("c:\temp\test.txt", "Create", "ReadWrite", "Delete")
Remove-Item -Path c:\temp\test.txt
$file2 = [System.IO.File]::Open("c:\temp\test.txt", "Create", "ReadWrite", "Delete")

ddebroy · 2019-09-24T17:30:30Z

/cc @ameyag

thaJeztah

LGTM

andrewhsu · 2019-10-03T18:11:14Z

I'll see if this helps with master builds that fail on RS1 https://ci.docker.com/public/blue/organizations/jenkins/moby/activity/?branch=master

thaJeztah · 2019-10-03T18:11:40Z

We can cherry-pick once we see master is more stable with this 👍

…ows versions Signed-off-by: Olli Janatuinen <olli.janatuinen@gmail.com>

cpuguy83 requested review from kolyshkin, thaJeztah and tonistiigi September 23, 2019 18:39

thaJeztah added area/logging platform/windows status/2-code-review labels Sep 23, 2019

cpuguy83 force-pushed the fix_windows_file_handles branch from b749478 to b65a8b6 Compare September 23, 2019 23:40

Use FILE_SHARE_DELETE for log files on Windows.

a5f237c

This fixes issues where one goroutine tries to delete or rename a file while another goroutine has the file open (e.g. a log reader). Signed-off-by: Brian Goff <cpuguy83@gmail.com>

cpuguy83 force-pushed the fix_windows_file_handles branch from b65a8b6 to a5f237c Compare September 23, 2019 23:45

thaJeztah reviewed Sep 24, 2019

View reviewed changes

tonistiigi approved these changes Oct 3, 2019

View reviewed changes

thaJeztah approved these changes Oct 3, 2019

View reviewed changes

thaJeztah merged commit 37760f0 into moby:master Oct 3, 2019

thaJeztah added the process/cherry-pick label Oct 3, 2019

olljanat mentioned this pull request Oct 3, 2019

Docker json-file logging driver hangs when rotating files on Windows #39274

Closed

alexbrainman mentioned this pull request Oct 6, 2019

proposal: os: Create/Open/OpenFile() set FILE_SHARE_DELETE on windows golang/go#32088

Closed

cpuguy83 deleted the fix_windows_file_handles branch October 7, 2019 17:23

olljanat added a commit to olljanat/moby that referenced this pull request Oct 22, 2019

Successor for moby#39974 to handle log rotate correctly on older Wind…

92c7d80

…ows versions Signed-off-by: Olli Janatuinen <olli.janatuinen@gmail.com>

thaJeztah added this to the 20.03.0 milestone Feb 22, 2020

thaJeztah mentioned this pull request Feb 22, 2020

[19.03 backport] Use FILE_SHARE_DELETE for log files on Windows. #40563

Merged

thaJeztah added process/cherry-picked and removed process/cherry-pick labels Feb 22, 2020

cpuguy83 mentioned this pull request May 19, 2020

Windows log rotation errors with active readers #40999

Closed

	// MkdirAll implementation that is volume path aware for Windows. It can be used
	// as a drop-in replacement for os.MkdirAll()
	func MkdirAll(path string, _ os.FileMode) error {
	return mkdirall(path, false, "")
	}

Conversation

cpuguy83 commented Sep 23, 2019 • edited by thaJeztah Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

thaJeztah commented Sep 23, 2019

Uh oh!

olljanat commented Sep 23, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

cpuguy83 commented Sep 23, 2019

Uh oh!

thaJeztah Sep 24, 2019

Choose a reason for hiding this comment

Uh oh!

cpuguy83 Sep 24, 2019

Choose a reason for hiding this comment

Uh oh!

thaJeztah Sep 24, 2019

Choose a reason for hiding this comment

Uh oh!

olljanat commented Sep 24, 2019

Uh oh!

cpuguy83 commented Sep 24, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

olljanat commented Sep 24, 2019

Uh oh!

ddebroy commented Sep 24, 2019

Uh oh!

thaJeztah left a comment

Choose a reason for hiding this comment

Uh oh!

andrewhsu commented Oct 3, 2019

Uh oh!

thaJeztah commented Oct 3, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

cpuguy83 commented Sep 23, 2019 •

edited by thaJeztah

Loading

olljanat commented Sep 23, 2019 •

edited

Loading

cpuguy83 commented Sep 24, 2019 •

edited

Loading