Merge pull request #55034 from yujuhong/fix-fs

Automatic merge from submit-queue (batch tested with PRs 55034, 55068). If you want to cherry-pick this change to another branch, please follow the instructions <a href="https://github.com/kubernetes/community/blob/master/contributors/devel/cherry-picks.md">here</a>.

Close the file before renaming in FileStore

Also change the unit test to use a real file system to detect errors
like this.

pkg/kubelet/util/store/BUILD

@@ -23,6 +23,7 @@ go_test(
     deps = [
         "//pkg/util/filesystem:go_default_library",
         "//vendor/github.com/stretchr/testify/assert:go_default_library",
+        "//vendor/github.com/stretchr/testify/require:go_default_library",
     ],
 )
 

pkg/kubelet/util/store/filestore.go

@@ -117,14 +117,17 @@ func writeFile(fs utilfs.Filesystem, path string, data []byte) (retErr error) {
 	}
 
 	tmpPath := tmpFile.Name()
+	shouldClose := true
 
 	defer func() {
 		// Close the file.
-		if err := tmpFile.Close(); err != nil {
-			if retErr == nil {
-				retErr = err
-			} else {
-				retErr = fmt.Errorf("failed to close temp file after error %v; close error: %v", retErr, err)
+		if shouldClose {
+			if err := tmpFile.Close(); err != nil {
+				if retErr == nil {
+					retErr = fmt.Errorf("close error: %v", err)
+				} else {
+					retErr = fmt.Errorf("failed to close temp file after error %v; close error: %v", retErr, err)
+				}
 			}
 		}
 
@@ -146,6 +149,13 @@ func writeFile(fs utilfs.Filesystem, path string, data []byte) (retErr error) {
 		return err
 	}
 
+	// Closing the file before renaming.
+	err = tmpFile.Close()
+	shouldClose = false
+	if err != nil {
+		return err
+	}
+
 	return fs.Rename(tmpPath, path)
 }
 

pkg/kubelet/util/store/filestore_test.go

@@ -22,15 +22,25 @@ import (
 	"testing"
 
 	"github.com/stretchr/testify/assert"
+	"github.com/stretchr/testify/require"
 	"k8s.io/kubernetes/pkg/util/filesystem"
 )
 
 func TestFileStore(t *testing.T) {
 	path, err := ioutil.TempDir("", "FileStore")
 	assert.NoError(t, err)
-	store, err := NewFileStore(path, filesystem.NewFakeFs())
+	store, err := NewFileStore(path, filesystem.DefaultFs{})
 	assert.NoError(t, err)
+	testStore(t, store)
+}
 
+func TestFakeFileStore(t *testing.T) {
+	store, err := NewFileStore("/tmp/test-fake-file-store", filesystem.NewFakeFs())
+	assert.NoError(t, err)
+	testStore(t, store)
+}
+
+func testStore(t *testing.T, store Store) {
 	testCases := []struct {
 		key       string
 		data      string
@@ -70,20 +80,20 @@ func TestFileStore(t *testing.T) {
 
 	// Test add data.
 	for _, c := range testCases {
-		_, err = store.Read(c.key)
+		t.Log("test case: ", c)
+		_, err := store.Read(c.key)
 		assert.Error(t, err)
 
 		err = store.Write(c.key, []byte(c.data))
 		if c.expectErr {
 			assert.Error(t, err)
 			continue
-		} else {
-			assert.NoError(t, err)
 		}
 
+		require.NoError(t, err)
 		// Test read data by key.
 		data, err := store.Read(c.key)
-		assert.NoError(t, err)
+		require.NoError(t, err)
 		assert.Equal(t, string(data), c.data)
 	}
 
@@ -100,7 +110,7 @@ func TestFileStore(t *testing.T) {
 		}
 
 		err = store.Delete(c.key)
-		assert.NoError(t, err)
+		require.NoError(t, err)
 		_, err = store.Read(c.key)
 		assert.EqualValues(t, ErrKeyNotFound, err)
 	}
@@ -111,6 +121,6 @@ func TestFileStore(t *testing.T) {
 
 	// Test list keys.
 	keys, err = store.List()
-	assert.NoError(t, err)
+	require.NoError(t, err)
 	assert.Equal(t, len(keys), 0)
 }