From 48601d261da65bca60e6e317c5f7909350a81d9b Mon Sep 17 00:00:00 2001 From: Martin Atkins Date: Thu, 15 Nov 2018 17:01:19 -0800 Subject: [PATCH] states/statemgr: In Filesystem, back up output file, not input file The filesystem backend has the option of using a different file for its initial read. Previously we were incorrectly writing the contents of that file out into the backup file, rather than the prior contents of the output file. Now we will always read the output file in RefreshState in order to decide what we will back up but then we will optionally additionally read the input file and prefer its content as the "current" state snapshot. This is verified by command.TestMetaBackend_planLocalStatePath and TestMetaBackend_configureNew, which are both now passing. --- command/meta_backend_test.go | 2 - states/statemgr/filesystem.go | 115 +++++++++++++++++----------- states/statemgr/filesystem_test.go | 116 ++++++++++++++++++++++++++++- 3 files changed, 185 insertions(+), 48 deletions(-) diff --git a/command/meta_backend_test.go b/command/meta_backend_test.go index 2adfbabb6..15995de8f 100644 --- a/command/meta_backend_test.go +++ b/command/meta_backend_test.go @@ -238,7 +238,6 @@ func TestMetaBackend_configureInterpolation(t *testing.T) { // Newly configured backend func TestMetaBackend_configureNew(t *testing.T) { - // Create a temporary working directory that is empty td := tempDir(t) copy.CopyDir(testFixturePath("backend-new"), td) defer os.RemoveAll(td) @@ -1532,7 +1531,6 @@ func TestMetaBackend_planLocal(t *testing.T) { // A plan with a custom state save path func TestMetaBackend_planLocalStatePath(t *testing.T) { - // Create a temporary working directory that is empty td := tempDir(t) copy.CopyDir(testFixturePath("backend-plan-local"), td) defer os.RemoveAll(td) diff --git a/states/statemgr/filesystem.go b/states/statemgr/filesystem.go index 47ea17b25..3a8e79110 100644 --- a/states/statemgr/filesystem.go +++ b/states/statemgr/filesystem.go @@ -56,7 +56,6 @@ type Filesystem struct { file *statefile.File readFile *statefile.File backupFile *statefile.File - written bool writtenBackup bool } @@ -115,9 +114,6 @@ func (s *Filesystem) State() *states.State { if s.file == nil { return nil } - if s.backupPath != "" && s.backupFile == nil { - s.backupFile = s.file.DeepCopy() - } return s.file.DeepCopy().State } @@ -128,36 +124,19 @@ func (s *Filesystem) WriteState(state *states.State) error { // writing to a temp file on the same filesystem, and renaming the file over // the original. + defer s.mutex()() + if s.readFile == nil { - err := s.RefreshState() + err := s.refreshState() if err != nil { return err } } - defer s.mutex()() return s.writeState(state, nil) } func (s *Filesystem) writeState(state *states.State, meta *SnapshotMeta) error { - // We'll try to write our backup first, so we can be sure we've created - // it successfully before clobbering the original file it came from. - if !s.writtenBackup && s.backupFile != nil && s.backupPath != "" && !statefile.StatesMarshalEqual(state, s.backupFile.State) { - log.Printf("[TRACE] statemgr.Filesystem: creating backup snapshot at %s", s.backupPath) - bfh, err := os.Create(s.backupPath) - if err != nil { - return fmt.Errorf("failed to create local state backup file: %s", err) - } - defer bfh.Close() - - err = statefile.Write(s.backupFile, bfh) - if err != nil { - return fmt.Errorf("failed to write to local state backup file: %s", err) - } - - s.writtenBackup = true - } - if s.stateFileOut == nil { if err := s.createStateFiles(); err != nil { return nil @@ -165,13 +144,46 @@ func (s *Filesystem) writeState(state *states.State, meta *SnapshotMeta) error { } defer s.stateFileOut.Sync() + // We'll try to write our backup first, so we can be sure we've created + // it successfully before clobbering the original file it came from. + if !s.writtenBackup && s.backupFile != nil && s.backupPath != "" { + if !statefile.StatesMarshalEqual(state, s.backupFile.State) { + log.Printf("[TRACE] statemgr.Filesystem: creating backup snapshot at %s", s.backupPath) + bfh, err := os.Create(s.backupPath) + if err != nil { + return fmt.Errorf("failed to create local state backup file: %s", err) + } + defer bfh.Close() + + err = statefile.Write(s.backupFile, bfh) + if err != nil { + return fmt.Errorf("failed to write to local state backup file: %s", err) + } + + s.writtenBackup = true + } else { + log.Print("[TRACE] statemgr.Filesystem: not making a backup, because the new snapshot is identical to the old") + } + } else { + // This branch is all just logging, to help understand why we didn't make a backup. + switch { + case s.backupPath == "": + log.Print("[TRACE] statemgr.Filesystem: state file backups are disabled") + case s.writtenBackup: + log.Printf("[TRACE] statemgr.Filesystem: have already backed up original %s to %s on a previous write", s.path, s.backupPath) + case s.backupFile == nil: + log.Printf("[TRACE] statemgr.Filesystem: no original state snapshot to back up") + default: + log.Printf("[TRACE] statemgr.Filesystem: not creating a backup for an unknown reason") + } + } + s.file = s.file.DeepCopy() if s.file == nil { s.file = NewStateFile() } s.file.State = state.DeepCopy() - log.Print("[TRACE] statemgr.Filesystem: truncating the state file") if _, err := s.stateFileOut.Seek(0, os.SEEK_SET); err != nil { return err } @@ -204,7 +216,8 @@ func (s *Filesystem) writeState(state *states.State, meta *SnapshotMeta) error { return err } - s.written = true + // Any future reads must come from the file we've now updated + s.readPath = s.path return nil } @@ -217,7 +230,10 @@ func (s *Filesystem) PersistState() error { // RefreshState is an implementation of Refresher. func (s *Filesystem) RefreshState() error { defer s.mutex()() + return s.refreshState() +} +func (s *Filesystem) refreshState() error { var reader io.Reader // The s.readPath file is only OK to read if we have not written any state out @@ -227,10 +243,9 @@ func (s *Filesystem) RefreshState() error { // This is important for Windows, as if the input file is the same as the // output file, and the output file has been locked already, we can't open // the file again. - if !s.written && (s.stateFileOut == nil || s.readPath != s.path) { - log.Printf("[TRACE] statemgr.Filesystem: reading initial snapshot from %s", s.readPath) - + if s.stateFileOut == nil || s.readPath != s.path { // we haven't written a state file yet, so load from readPath + log.Printf("[TRACE] statemgr.Filesystem: reading initial snapshot from %s", s.readPath) f, err := os.Open(s.readPath) if err != nil { // It is okay if the file doesn't exist; we'll treat that as a nil state. @@ -247,11 +262,9 @@ func (s *Filesystem) RefreshState() error { reader = f } } else { - log.Printf("[TRACE] statemgr.Filesystem: reading snapshot from %s", s.path) - + log.Printf("[TRACE] statemgr.Filesystem: reading latest snapshot from %s", s.path) // no state to refresh if s.stateFileOut == nil { - log.Printf("[TRACE] statemgr.Filesystem: no state snapshot has been written yet") return nil } @@ -261,23 +274,20 @@ func (s *Filesystem) RefreshState() error { } f, err := statefile.Read(reader) - - // nothing to backup if there's no initial state - if f == nil { - log.Printf("[TRACE] statemgr.Filesystem: no initial state, so will skip writing a backup") - s.writtenBackup = true - } - - // if there's no state we just assign the nil return value - if err != nil && err != statefile.ErrNoState { - log.Printf("[TRACE] statemgr.Filesystem: state snapshot is nil") - return err + // if there's no state then a nil file is fine + if err != nil { + if err != statefile.ErrNoState { + return err + } + log.Printf("[TRACE] statemgr.Filesystem: snapshot file has nil snapshot, but that's okay") } s.file = f s.readFile = s.file.DeepCopy() if s.file != nil { log.Printf("[TRACE] statemgr.Filesystem: read snapshot with lineage %q serial %d", s.file.Lineage, s.file.Serial) + } else { + log.Print("[TRACE] statemgr.Filesystem: read nil snapshot") } return nil } @@ -378,13 +388,14 @@ func (s *Filesystem) StateForMigration() *statefile.File { // WriteStateForMigration is part of our implementation of Migrator. func (s *Filesystem) WriteStateForMigration(f *statefile.File, force bool) error { + defer s.mutex()() + if s.readFile == nil { - err := s.RefreshState() + err := s.refreshState() if err != nil { return err } } - defer s.mutex()() if !force { err := CheckValidImport(f, s.readFile) @@ -436,6 +447,20 @@ func (s *Filesystem) createStateFiles() error { } s.stateFileOut = f + + // If the file already existed with content then that'll be the content + // of our backup file if we write a change later. + s.backupFile, err = statefile.Read(s.stateFileOut) + if err != nil { + if err != statefile.ErrNoState { + return err + } + log.Printf("[TRACE] statemgr.Filesystem: no previously-stored snapshot exists") + } else { + log.Printf("[TRACE] statemgr.Filesystem: existing snapshot has lineage %q serial %d", s.backupFile.Lineage, s.backupFile.Serial) + } + + // Refresh now, to load in the snapshot if the file already existed return nil } diff --git a/states/statemgr/filesystem_test.go b/states/statemgr/filesystem_test.go index 7977a6f8d..c21acd34e 100644 --- a/states/statemgr/filesystem_test.go +++ b/states/statemgr/filesystem_test.go @@ -4,13 +4,17 @@ import ( "io/ioutil" "os" "os/exec" + "path/filepath" "strings" "sync" "testing" "github.com/go-test/deep" version "github.com/hashicorp/go-version" + "github.com/zclconf/go-cty/cty" + "github.com/hashicorp/terraform/addrs" + "github.com/hashicorp/terraform/states" "github.com/hashicorp/terraform/states/statefile" tfversion "github.com/hashicorp/terraform/version" ) @@ -57,7 +61,7 @@ func TestFilesystemLocks(t *testing.T) { t.Fatal("unexpected lock failure", err, string(out)) } - if string(out) != "lock failed" { + if !strings.Contains(string(out), "lock failed") { t.Fatal("expected 'locked failed', got", string(out)) } @@ -172,6 +176,116 @@ func TestFilesystem_backup(t *testing.T) { } } +// This test verifies a particularly tricky behavior where the input file +// is overridden and backups are enabled at the same time. This combination +// requires special care because we must ensure that when we create a backup +// it is of the original contents of the output file (which we're overwriting), +// not the contents of the input file (which is left unchanged). +func TestFilesystem_backupAndReadPath(t *testing.T) { + defer testOverrideVersion(t, "1.2.3")() + + workDir, err := ioutil.TempDir("", "tf") + if err != nil { + t.Fatalf("failed to create temporary directory: %s", err) + } + defer os.RemoveAll(workDir) + + markerOutput := addrs.OutputValue{Name: "foo"}.Absolute(addrs.RootModuleInstance) + + outState := states.BuildState(func(ss *states.SyncState) { + ss.SetOutputValue( + markerOutput, + cty.StringVal("from-output-state"), + false, // not sensitive + ) + }) + outFile, err := os.Create(filepath.Join(workDir, "output.tfstate")) + if err != nil { + t.Fatalf("failed to create temporary outFile %s", err) + } + defer outFile.Close() + err = statefile.Write(&statefile.File{ + Lineage: "-", + Serial: 0, + TerraformVersion: version.Must(version.NewVersion("1.2.3")), + State: outState, + }, outFile) + if err != nil { + t.Fatalf("failed to write initial outfile state to %s: %s", outFile.Name(), err) + } + + inState := states.BuildState(func(ss *states.SyncState) { + ss.SetOutputValue( + markerOutput, + cty.StringVal("from-input-state"), + false, // not sensitive + ) + }) + inFile, err := os.Create(filepath.Join(workDir, "input.tfstate")) + if err != nil { + t.Fatalf("failed to create temporary inFile %s", err) + } + defer inFile.Close() + err = statefile.Write(&statefile.File{ + Lineage: "-", + Serial: 0, + TerraformVersion: version.Must(version.NewVersion("1.2.3")), + State: inState, + }, inFile) + if err != nil { + t.Fatalf("failed to write initial infile state to %s: %s", inFile.Name(), err) + } + + backupPath := outFile.Name() + ".backup" + + ls := NewFilesystemBetweenPaths(inFile.Name(), outFile.Name()) + ls.SetBackupPath(backupPath) + + newState := states.BuildState(func(ss *states.SyncState) { + ss.SetOutputValue( + markerOutput, + cty.StringVal("from-new-state"), + false, // not sensitive + ) + }) + err = ls.WriteState(newState) + if err != nil { + t.Fatalf("failed to write new state: %s", err) + } + + // The backup functionality should've saved a copy of the original contents + // of the _output_ file, even though the first snapshot was read from + // the _input_ file. + t.Run("backup file", func (t *testing.T) { + bfh, err := os.Open(backupPath) + if err != nil { + t.Fatal(err) + } + bf, err := statefile.Read(bfh) + if err != nil { + t.Fatal(err) + } + os := bf.State.OutputValue(markerOutput) + if got, want := os.Value, cty.StringVal("from-output-state"); !want.RawEquals(got) { + t.Errorf("wrong marker value in backup state file\ngot: %#v\nwant: %#v", got, want) + } + }) + t.Run("output file", func (t *testing.T) { + ofh, err := os.Open(outFile.Name()) + if err != nil { + t.Fatal(err) + } + of, err := statefile.Read(ofh) + if err != nil { + t.Fatal(err) + } + os := of.State.OutputValue(markerOutput) + if got, want := os.Value, cty.StringVal("from-new-state"); !want.RawEquals(got) { + t.Errorf("wrong marker value in backup state file\ngot: %#v\nwant: %#v", got, want) + } + }) +} + func TestFilesystem_nonExist(t *testing.T) { defer testOverrideVersion(t, "1.2.3")() ls := NewFilesystem("ishouldntexist")