From 831b439136842751325870faae47c852036b88cc Mon Sep 17 00:00:00 2001 From: Tom Wieczorek Date: Thu, 22 Sep 2022 17:38:19 +0200 Subject: [PATCH] Write manifests atomically Use a temporary-write-and-rename approach to write manifests to disk, so that the appliers never see partially written files. The stragegy is as follows: Create a temporary file in the same directory as the target, write to it, sync and close the file handle, do a chmod and a rename. As a result, there's no more need to ignore chmod events in the stack applier. Those events actually *can* matter, when files become readable or unreadable for the k0s process. Signed-off-by: Tom Wieczorek --- go.mod | 2 +- internal/pkg/file/file.go | 82 ++++++++++++++++++- internal/pkg/file/file_test.go | 3 +- internal/pkg/templatewriter/templatewriter.go | 11 +-- pkg/applier/applier.go | 7 +- pkg/applier/stackapplier.go | 10 +-- pkg/component/controller/manifests.go | 13 +-- 7 files changed, 101 insertions(+), 27 deletions(-) diff --git a/go.mod b/go.mod index 565def8c7b1d..b8c640788e55 100644 --- a/go.mod +++ b/go.mod @@ -45,6 +45,7 @@ require ( go.etcd.io/etcd/client/pkg/v3 v3.5.4 go.etcd.io/etcd/client/v3 v3.5.4 go.etcd.io/etcd/etcdutl/v3 v3.5.4 + go.uber.org/multierr v1.8.0 go.uber.org/zap v1.23.0 golang.org/x/crypto v0.0.0-20220824171710-5757bc0c5503 golang.org/x/exp v0.0.0-20220827204233-334a2380cb91 @@ -258,7 +259,6 @@ require ( go.opentelemetry.io/proto/otlp v0.11.0 // indirect go.starlark.net v0.0.0-20200306205701-8dd3e2ee1dd5 // indirect go.uber.org/atomic v1.9.0 // indirect - go.uber.org/multierr v1.8.0 // indirect golang.org/x/mod v0.6.0-dev.0.20220419223038-86c51ed26bb4 // indirect golang.org/x/net v0.0.0-20220722155237-a158d28d115b // indirect golang.org/x/oauth2 v0.0.0-20220223155221-ee480838109b // indirect diff --git a/internal/pkg/file/file.go b/internal/pkg/file/file.go index 1304f368ebb5..7838c0330cd9 100644 --- a/internal/pkg/file/file.go +++ b/internal/pkg/file/file.go @@ -18,9 +18,13 @@ package file import ( "fmt" + "io" "os" + "path/filepath" "github.com/k0sproject/k0s/internal/pkg/users" + + "go.uber.org/multierr" ) // Exists checks if a file exists and is not a directory before we @@ -71,15 +75,89 @@ func Copy(src, dst string) error { return nil } +// WriteAtomically will atomically create or replace a file. The contents of the +// file will be those that the write callback writes to the Writer that gets +// passed in. The Writer will be unbuffered. WriteAtomically will buffer the +// contents in a hidden (i.e. its name will start with a dot), temporary file +// (it will have a .tmp extension). When write returns without an error, the +// temporary file will be renamed to fileName, otherwise it will be deleted +// without touching the target file. +// +// Note that this function is only best-effort on Windows: +// https://github.com/golang/go/issues/22397#issuecomment-498856679 +func WriteAtomically(fileName string, perm os.FileMode, write func(file io.Writer) error) error { + var fd *os.File + fd, err := os.CreateTemp(filepath.Dir(fileName), fmt.Sprintf(".%s.*.tmp", filepath.Base(fileName))) + if err != nil { + return err + } + + tmpFileName := fd.Name() + close := true + defer func() { + remove := err != nil + if close { + err = multierr.Append(err, fd.Close()) + } + if remove { + err = multierr.Append(err, os.Remove(tmpFileName)) + } + }() + + err = write(fd) + if err != nil { + return err + } + + // https://github.com/google/renameio/blob/v2.0.0/tempfile.go#L150-L157 + err = fd.Sync() + if err != nil { + return err + } + + err = fd.Close() + close = false + if err != nil { + return err + } + + err = os.Chmod(tmpFileName, perm) + if err != nil { + return err + } + + err = os.Rename(tmpFileName, fileName) + if err != nil { + return err + } + + return nil +} + +// WriteContentAtomically will atomically create or replace a file with the +// given content. WriteContentAtomically will create a hidden (i.e. its name +// will start with a dot), temporary file (it will have a .tmp extension) with +// the given content. Afterwards, the temporary file will be renamed to +// fileName, otherwise it will be deleted without touching the target file. +// +// Note that this function is only best-effort on Windows: +// https://github.com/golang/go/issues/22397#issuecomment-498856679 +func WriteContentAtomically(fileName string, content []byte, perm os.FileMode) error { + return WriteAtomically(fileName, perm, func(file io.Writer) error { + _, err := file.Write(content) + return err + }) +} + func WriteTmpFile(data string, prefix string) (path string, err error) { tmpFile, err := os.CreateTemp("", prefix) if err != nil { - return "", fmt.Errorf("cannot create temporary file: %v", err) + return "", fmt.Errorf("cannot create temporary file: %w", err) } text := []byte(data) if _, err = tmpFile.Write(text); err != nil { - return "", fmt.Errorf("failed to write to temporary file: %v", err) + return "", fmt.Errorf("failed to write to temporary file: %w", err) } return tmpFile.Name(), nil diff --git a/internal/pkg/file/file_test.go b/internal/pkg/file/file_test.go index 6aa21f96866e..c653e9c9fdbe 100644 --- a/internal/pkg/file/file_test.go +++ b/internal/pkg/file/file_test.go @@ -18,7 +18,6 @@ package file import ( "os" - "path" "path/filepath" "testing" @@ -36,7 +35,7 @@ func TestExists(t *testing.T) { t.Errorf("test non-existing: got %t, wanted %t", got, want) } - existingFileName := path.Join(dir, "existing") + existingFileName := filepath.Join(dir, "existing") require.NoError(t, os.WriteFile(existingFileName, []byte{}, 0644)) // test existing diff --git a/internal/pkg/templatewriter/templatewriter.go b/internal/pkg/templatewriter/templatewriter.go index 299704140ebb..ba11626ab68d 100644 --- a/internal/pkg/templatewriter/templatewriter.go +++ b/internal/pkg/templatewriter/templatewriter.go @@ -19,11 +19,11 @@ package templatewriter import ( "fmt" "io" - "os" "text/template" "github.com/Masterminds/sprig" + "github.com/k0sproject/k0s/internal/pkg/file" "github.com/k0sproject/k0s/pkg/constant" ) @@ -35,14 +35,9 @@ type TemplateWriter struct { Path string } -// Write writes executes the template and writes the results on disk +// Write executes the template and writes the results on disk func (p *TemplateWriter) Write() error { - podFile, err := os.OpenFile(p.Path, os.O_RDWR|os.O_CREATE|os.O_TRUNC, constant.CertMode) - if err != nil { - return fmt.Errorf("failed to open pod file for %s: %w", p.Name, err) - } - defer podFile.Close() - return p.WriteToBuffer(podFile) + return file.WriteAtomically(p.Path, constant.CertMode, p.WriteToBuffer) } // WriteToBuffer writes executed template tot he given writer diff --git a/pkg/applier/applier.go b/pkg/applier/applier.go index 09a43dc7964f..8d1602a263ee 100644 --- a/pkg/applier/applier.go +++ b/pkg/applier/applier.go @@ -24,7 +24,8 @@ import ( "path" "path/filepath" - "github.com/sirupsen/logrus" + "github.com/k0sproject/k0s/pkg/kubernetes" + "k8s.io/apimachinery/pkg/api/meta" "k8s.io/apimachinery/pkg/apis/meta/v1/unstructured" "k8s.io/cli-runtime/pkg/resource" @@ -33,7 +34,7 @@ import ( "k8s.io/client-go/rest" "k8s.io/client-go/restmapper" - "github.com/k0sproject/k0s/pkg/kubernetes" + "github.com/sirupsen/logrus" ) // manifestFilePattern is the glob pattern that all applicable manifest files need to match. @@ -105,10 +106,12 @@ func (a *Applier) Apply(ctx context.Context) error { if err != nil { return err } + files, err := filepath.Glob(path.Join(a.Dir, manifestFilePattern)) if err != nil { return err } + resources, err := a.parseFiles(files) if err != nil { return err diff --git a/pkg/applier/stackapplier.go b/pkg/applier/stackapplier.go index 0a9f02bec8bb..e0528bbfa4c7 100644 --- a/pkg/applier/stackapplier.go +++ b/pkg/applier/stackapplier.go @@ -108,19 +108,17 @@ func (s *StackApplier) Run(ctx context.Context) error { } func (*StackApplier) triggersApply(event fsnotify.Event) bool { - // always let the initial apply happen + // Always let the initial apply happen if event == (fsnotify.Event{}) { return true } - // ignore chmods (3845479a0) - if event.Op == fsnotify.Chmod { + // Only consider events on manifest files + if match, _ := filepath.Match(manifestFilePattern, filepath.Base(event.Name)); !match { return false } - // Only consider events on manifest files - match, _ := filepath.Match(manifestFilePattern, filepath.Base(event.Name)) - return match + return true } func (s *StackApplier) apply(ctx context.Context) { diff --git a/pkg/component/controller/manifests.go b/pkg/component/controller/manifests.go index 4cd968491188..76a153f90c4c 100644 --- a/pkg/component/controller/manifests.go +++ b/pkg/component/controller/manifests.go @@ -19,11 +19,10 @@ package controller import ( "crypto/md5" "fmt" - "os" - "path" "path/filepath" "github.com/k0sproject/k0s/internal/pkg/dir" + "github.com/k0sproject/k0s/internal/pkg/file" "github.com/k0sproject/k0s/pkg/constant" "github.com/sirupsen/logrus" ) @@ -36,10 +35,12 @@ type FsManifestsSaver struct { // Save saves given manifest under the given path func (f FsManifestsSaver) Save(dst string, content []byte) error { target := filepath.Join(f.dir, dst) - if err := os.WriteFile(target, content, constant.ManifestsDirMode); err != nil { - return fmt.Errorf("can't write manifest %s: %v", target, err) + + if err := file.WriteContentAtomically(target, content, constant.CertMode); err != nil { + return err } - logrus.WithField("component", "manifest-saver").Debugf("succesfully wrote %s:%s", target, hash(content)) + + logrus.WithField("component", "manifest-saver").Debugf("Successfully wrote %s:%s", target, hash(content)) return nil } @@ -49,7 +50,7 @@ func hash(data []byte) string { // NewManifestsSaver builds new filesystem manifests saver func NewManifestsSaver(manifest string, dataDir string) (*FsManifestsSaver, error) { - manifestDir := path.Join(dataDir, "manifests", manifest) + manifestDir := filepath.Join(dataDir, "manifests", manifest) err := dir.Init(manifestDir, constant.ManifestsDirMode) if err != nil { return nil, err