Repository: kubernetes/kubernetes (language: go)

Issue:
Windows: Container log rotation may fail if the container logs are followed
### What happened?

The test ``k8s.io/kubernetes/pkg/kubelet/kuberuntime/logs.TestReadRotatedLog`` is failing on Windows [0]:

```
{Failed  === RUN   TestReadRotatedLog
    logs_test.go:276: 
        	Error Trace:	C:/kubernetes/pkg/kubelet/kuberuntime/logs/logs_test.go:276
        	Error:      	Received unexpected error:
        	            	rename C:\Users\azureuser\AppData\Local\Temp\TestReadRotatedLog1957435032\001\logfile2771403626 C:\Users\azureuser\AppData\Local\Temp\TestReadRotatedLog1957435032\001\logfile2771403626.2220240422-035910: The process cannot access the file because it is being used by another process.
        	Test:       	TestReadRotatedLog
        	Messages:   	failed to rotate log "C:\\Users\\azureuser\\AppData\\Local\\Temp\\TestReadRotatedLog1957435032\\001\\logfile2771403626" to "C:\\Users\\azureuser\\AppData\\Local\\Temp\\TestReadRotatedLog1957435032\\001\\logfile2771403626.2220240422-035910"
--- FAIL: TestReadRotatedLog (0.21s)
```

The idea of the test is to check that kubelet still follows and reads the Container Logs after a Log Rotation occured. However, because the followed file is open, it cannot be renamed, and thus, the container logs cannot be rotated.

This issue could be prevented if the file would be opened with the ``FILE_SHARE_DELETE`` flag set [1]: ``Delete access allows both delete and rename operations.``

[0] https://prow.k8s.io/view/gs/kubernetes-jenkins/logs/ci-kubernetes-unit-windows-master/1782243642258231296
[1] https://learn.microsoft.com/en-us/windows/win32/api/fileapi/nf-fileapi-createfilea

### What did you expect to happen?

Kubelet should not block container log rotation.

### How can we reproduce it (as minimally and precisely as possible)?

Run the test ``k8s.io/kubernetes/pkg/kubelet/kuberuntime/logs.TestReadRotatedLog`` is failing on Windows.

### Anything else we need to know?

/sig windows

/kind bug
/kind failing-test

### Kubernetes version

latest, master. This unit test has been failing for a while.


### Cloud provider

N/A


### OS version

Windows nodes

</details>


### Install tools

N/A


### Container runtime (CRI) and version (if applicable)

N/A


### Related plugins (CNI, CSI, ...) and versions (if applicable)

N/A



Discussion / hints:

Fix patch:
diff --git a/staging/src/k8s.io/cri-client/pkg/logs/logs.go b/staging/src/k8s.io/cri-client/pkg/logs/logs.go
index 8642f5a180a46..a2f07483d53a1 100644
--- a/staging/src/k8s.io/cri-client/pkg/logs/logs.go
+++ b/staging/src/k8s.io/cri-client/pkg/logs/logs.go
@@ -295,7 +295,7 @@ func ReadLogs(ctx context.Context, logger *klog.Logger, path, containerID string
 		return fmt.Errorf("failed to try resolving symlinks in path %q: %v", path, err)
 	}
 	path = evaluated
-	f, err := os.Open(path)
+	f, err := openFileShareDelete(path)
 	if err != nil {
 		return fmt.Errorf("failed to open log file %q: %v", path, err)
 	}
@@ -364,7 +364,7 @@ func ReadLogs(ctx context.Context, logger *klog.Logger, path, containerID string
 					return err
 				}
 				if recreated {
-					newF, err := os.Open(path)
+					newF, err := openFileShareDelete(path)
 					if err != nil {
 						if os.IsNotExist(err) {
 							continue
diff --git a/staging/src/k8s.io/cri-client/pkg/logs/logs_other.go b/staging/src/k8s.io/cri-client/pkg/logs/logs_other.go
new file mode 100644
index 0000000000000..4aa17b4a20746
--- /dev/null
+++ b/staging/src/k8s.io/cri-client/pkg/logs/logs_other.go
@@ -0,0 +1,29 @@
+//go:build !windows
+// +build !windows
+
+/*
+Copyright 2024 The Kubernetes Authors.
+
+Licensed under the Apache License, Version 2.0 (the "License");
+you may not use this file except in compliance with the License.
+You may obtain a copy of the License at
+
+    http://www.apache.org/licenses/LICENSE-2.0
+
+Unless required by applicable law or agreed to in writing, software
+distributed under the License is distributed on an "AS IS" BASIS,
+WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+See the License for the specific language governing permissions and
+limitations under the License.
+*/
+
+package logs
+
+import (
+	"os"
+)
+
+func openFileShareDelete(path string) (*os.File, error) {
+	// Noop. Only relevant for Windows.
+	return os.Open(path)
+}
diff --git a/staging/src/k8s.io/cri-client/pkg/logs/logs_windows.go b/staging/src/k8s.io/cri-client/pkg/logs/logs_windows.go
new file mode 100644
index 0000000000000..83d07d8df790e
--- /dev/null
+++ b/staging/src/k8s.io/cri-client/pkg/logs/logs_windows.go
@@ -0,0 +1,49 @@
+//go:build windows
+// +build windows
+
+/*
+Copyright 2024 The Kubernetes Authors.
+
+Licensed under the Apache License, Version 2.0 (the "License");
+you may not use this file except in compliance with the License.
+You may obtain a copy of the License at
+
+    http://www.apache.org/licenses/LICENSE-2.0
+
+Unless required by applicable law or agreed to in writing, software
+distributed under the License is distributed on an "AS IS" BASIS,
+WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+See the License for the specific language governing permissions and
+limitations under the License.
+*/
+
+package logs
+
+import (
+	"os"
+	"syscall"
+)
+
+// Based on Windows implementation of Windows' syscall.Open
+// https://cs.opensource.google/go/go/+/refs/tags/go1.22.2:src/syscall/syscall_windows.go;l=342
+// In addition to syscall.Open, this function also adds the syscall.FILE_SHARE_DELETE flag to sharemode,
+// which will allow us to read from the file without blocking the file from being deleted or renamed.
+// This is essential for Log Rotation which is done by renaming the open file. Without this, the file rename would fail.
+func openFileShareDelete(path string) (*os.File, error) {
+	pathp, err := syscall.UTF16PtrFromString(path)
+	if err != nil {
+		return nil, err
+	}
+
+	var access uint32 = syscall.GENERIC_READ
+	var sharemode uint32 = syscall.FILE_SHARE_READ | syscall.FILE_SHARE_WRITE | syscall.FILE_SHARE_DELETE
+	var createmode uint32 = syscall.OPEN_EXISTING
+	var attrs uint32 = syscall.FILE_ATTRIBUTE_NORMAL
+
+	handle, err := syscall.CreateFile(pathp, access, sharemode, nil, createmode, attrs, 0)
+	if err != nil {
+		return nil, err
+	}
+
+	return os.NewFile(uintptr(handle), path), nil
+}


Test patch:
diff --git a/staging/src/k8s.io/cri-client/pkg/logs/logs_test.go b/staging/src/k8s.io/cri-client/pkg/logs/logs_test.go
index 59febfc00ba2c..cf53d7a5f5fe7 100644
--- a/staging/src/k8s.io/cri-client/pkg/logs/logs_test.go
+++ b/staging/src/k8s.io/cri-client/pkg/logs/logs_test.go
@@ -24,7 +24,6 @@ import (
 	"io"
 	"os"
 	"path/filepath"
-	goruntime "runtime"
 	"testing"
 	"time"
 
@@ -212,10 +211,6 @@ func TestReadLogs(t *testing.T) {
 }
 
 func TestReadRotatedLog(t *testing.T) {
-	if goruntime.GOOS == "windows" {
-		// TODO: remove skip once the failing test has been fixed.
-		t.Skip("Skip failing test on Windows.")
-	}
 	tmpDir := t.TempDir()
 	file, err := os.CreateTemp(tmpDir, "logfile")
 	if err != nil {
@@ -288,6 +283,10 @@ func TestReadRotatedLog(t *testing.T) {
 		}
 	}
 
+	// Finished writing into the file, close it, so we can delete it later.
+	err = file.Close()
+	assert.NoErrorf(t, err, "could not close file.")
+
 	time.Sleep(20 * time.Millisecond)
 	// Make the function ReadLogs end.
 	fakeRuntimeService.Lock()