Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Add atomic writes to local backend #528

Closed
wants to merge 2 commits into from
Closed

Add atomic writes to local backend #528

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
433e1aa
Add atomic writes to local backend. Fixes #527
mac-chaffee Apr 14, 2022
73827b8
Don't store test files above current dir
mac-chaffee Apr 14, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
38 changes: 36 additions & 2 deletions local.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,7 @@ import (
// LocalFilesystemBackend is a storage backend for local filesystem storage
type LocalFilesystemBackend struct {
RootDirectory string
TempDirectory string
}

// NewLocalFilesystemBackend creates a new instance of LocalFilesystemBackend
Expand All @@ -35,7 +36,20 @@ func NewLocalFilesystemBackend(rootDirectory string) *LocalFilesystemBackend {
if err != nil {
panic(err)
}
b := &LocalFilesystemBackend{RootDirectory: absPath}
// Create a temporary folder for partially-completed writes (if not present)
tempPath := filepath.Join(absPath, ".tmp")
Copy link
Collaborator

@cbuto cbuto Apr 27, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

this temp directory creation might be better suited in the PutObject function (like we do with the RootDirectory). This way the temp directory will always be recreated if it doesn't exist during a put, so if that directory was deleted while chartmuseum is running, uploads will continue to work without restarting chartmuseum.

Copy link
Author

Choose a reason for hiding this comment

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

That would require an extra stat per invocation of PutObject. Do we really want that performance overhead?

Copy link
Collaborator

Choose a reason for hiding this comment

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

that is definitely a consideration, but it seems like we should be treating the root dir and this new dir similarly. i.e. either both are created in NewLocalFilesystemBackend or both are created in PutObject. @jdolitsky @scbizu do you have any thoughts here?

Copy link
Collaborator

Choose a reason for hiding this comment

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

For @cbuto 's consideration , how about do anther os.MkdirAll() and ignore the dir already exist error in PutObject , I agree with cbuto's opinion and it is maybe reasonable even we do some overhead tradeoff here.

_, err = os.Stat(tempPath)
if err != nil {
if os.IsNotExist(err) {
err = os.MkdirAll(tempPath, 0774)
if err != nil {
panic(err)
}
} else {
panic(err)
}
}
b := &LocalFilesystemBackend{RootDirectory: absPath, TempDirectory: tempPath}
return b
}

Expand Down Expand Up @@ -99,7 +113,27 @@ func (b LocalFilesystemBackend) PutObject(path string, content []byte) error {
return err
}
}
err = ioutil.WriteFile(fullpath, content, 0644)

// Write to a temporary file first
tempFile, err := os.CreateTemp(b.TempDirectory, "partial-upload-")
if err != nil {
return err
}
defer os.Remove(tempFile.Name())
_, err = tempFile.Write(content)
if err != nil {
return err
}
tempFile.Close()

// Default permissions on a temp file are 600, so let's change that
err = os.Chmod(tempFile.Name(), 0774)
if err != nil {
return err
}

// Now that the file is written safely, atomically move it into place
err = os.Rename(tempFile.Name(), fullpath)
return err
}

Expand Down
15 changes: 9 additions & 6 deletions local_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,8 +17,8 @@ limitations under the License.
package storage

import (
"fmt"
"os"
"path/filepath"
"testing"
"time"

Expand All @@ -28,15 +28,16 @@ import (
type LocalTestSuite struct {
suite.Suite
LocalFilesystemBackend *LocalFilesystemBackend
BrokenTempDirectory string
TempRootDirectory string
}

func (suite *LocalTestSuite) SetupSuite() {
timestamp := time.Now().Format("20060102150405")
suite.BrokenTempDirectory = fmt.Sprintf("../../.test/storage-local/%s-broken", timestamp)
defer os.RemoveAll(suite.BrokenTempDirectory)
backend := NewLocalFilesystemBackend(suite.BrokenTempDirectory)
suite.TempRootDirectory = filepath.Join(".test", timestamp)
backend := NewLocalFilesystemBackend(suite.TempRootDirectory)
suite.LocalFilesystemBackend = backend
_, err := os.Stat(filepath.Join(suite.LocalFilesystemBackend.TempDirectory))
suite.Nil(err, "should have created a .tmp/ directory for partial uploads")
}

func (suite *LocalTestSuite) TestListObjects() {
Expand All @@ -55,5 +56,7 @@ func (suite *LocalTestSuite) TestPutObjectWithNonExistentPath() {
}

func TestLocalStorageTestSuite(t *testing.T) {
suite.Run(t, new(LocalTestSuite))
ts := new(LocalTestSuite)
defer os.RemoveAll(ts.TempRootDirectory)
suite.Run(t, ts)
}
3 changes: 2 additions & 1 deletion storage_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@ package storage
import (
"fmt"
"os"
"path/filepath"
"testing"
"time"

Expand All @@ -33,7 +34,7 @@ type StorageTestSuite struct {

func (suite *StorageTestSuite) setupStorageBackends() {
timestamp := time.Now().Format("20060102150405")
suite.TempDirectory = fmt.Sprintf("../../.test/storage-storage/%s", timestamp)
suite.TempDirectory = filepath.Join(".test", timestamp)
suite.StorageBackends = make(map[string]Backend)
suite.StorageBackends["LocalFilesystem"] = Backend(NewLocalFilesystemBackend(suite.TempDirectory))

Expand Down