From 625806f44854fa4abae0144538e2226e85be7f22 Mon Sep 17 00:00:00 2001 From: Paul Hinze Date: Wed, 18 May 2022 16:52:54 -0500 Subject: [PATCH 1/3] Don't delete machine datadir when SetId("") is called Legacy's `Machine#id=()` has an important side effect when a nil ID is specified - it clears the contents of the machine's DataDir. We mirrored this behavior over to gogo, with a subtle difference - we deleted the whole DataDir vs just its children. It turns out the Docker provider relies on the DataDir being cleared-but-not-removed by doing a SetId dance in its InitState action. (see 1e6259dd00d702f83048c75c5c229ce6494c4c6e). So here we need to mirror that behavior in order for the Docker provider to work properly. --- internal/core/machine_test.go | 16 ++++++++++++++++ internal/core/project.go | 4 ++-- internal/core/target.go | 15 ++++++++++++++- 3 files changed, 32 insertions(+), 3 deletions(-) diff --git a/internal/core/machine_test.go b/internal/core/machine_test.go index 9a136a9a5..5c6797e03 100644 --- a/internal/core/machine_test.go +++ b/internal/core/machine_test.go @@ -74,6 +74,22 @@ func TestMachineSetEmptyId(t *testing.T) { require.Error(t, err) } +func TestMachineSetIdBlankThenSomethingRecreatesDataDir(t *testing.T) { + tm, _ := TestMinimalMachine(t) + + // Set empty id, followed by a temp id. This is the same thing that happens + // in the Docker provider's InitState action + require.NoError(t, tm.SetID("")) + require.NoError(t, tm.SetID("preparing")) + + // The DataDir should still exist; the Docker provider relies on this + // behavior in order for its provisioning sentinel file handling to work + // properly. + dir, err := tm.DataDir() + require.NoError(t, err) + require.DirExists(t, dir.DataDir().String()) +} + func TestMachineGetNonExistentBox(t *testing.T) { tp := TestMinimalProject(t) tm, _ := TestMachine(t, tp, diff --git a/internal/core/project.go b/internal/core/project.go index c27474ac6..6cc55ff83 100644 --- a/internal/core/project.go +++ b/internal/core/project.go @@ -231,8 +231,8 @@ func (p *Project) JobInfo() *component.JobInfo { return p.jobInfo } -// Load a project within the current basis. If the project is not found, it -// will be created. +// LoadTarget loads a target within the current project. If the target is not +// found, it will be created. func (p *Project) LoadTarget(topts ...TargetOption) (t *Target, err error) { p.m.Lock() defer p.m.Unlock() diff --git a/internal/core/target.go b/internal/core/target.go index d41fd0dea..e0db5d089 100644 --- a/internal/core/target.go +++ b/internal/core/target.go @@ -4,6 +4,7 @@ import ( "context" "fmt" "os" + "path/filepath" "strings" "sync" "time" @@ -265,7 +266,19 @@ func (t *Target) Destroy() (err error) { _, err = t.Client().DeleteTarget(t.ctx, &vagrant_server.DeleteTargetRequest{ Target: t.Ref().(*vagrant_plugin_sdk.Ref_Target), }) - os.RemoveAll(t.dir.DataDir().String()) + + // Remove all the files inside the datadir without wiping the datadir itself + files, err := filepath.Glob(filepath.Join(t.dir.DataDir().String(), "*")) + if err != nil { + return err + } + for _, file := range files { + rerr := os.RemoveAll(file) + if rerr != nil { + err = multierror.Append(err, rerr) + } + } + return } From 5bd0ac371c68a53b1c78e01dbfc3473af0fc242a Mon Sep 17 00:00:00 2001 From: Paul Hinze Date: Wed, 18 May 2022 17:03:10 -0500 Subject: [PATCH 2/3] tests: Correct basis dir setup I noticed we were still catching the default locations for dirs in test and just appending a tempdir path inside of them. This fixes that and ensures all test files are contained within the tmpdir. --- internal/core/testing_basis.go | 17 +++++++++++++++-- 1 file changed, 15 insertions(+), 2 deletions(-) diff --git a/internal/core/testing_basis.go b/internal/core/testing_basis.go index f1052733b..a65f657e0 100644 --- a/internal/core/testing_basis.go +++ b/internal/core/testing_basis.go @@ -4,6 +4,7 @@ import ( "context" "io/ioutil" "os" + "path/filepath" "github.com/hashicorp/vagrant-plugin-sdk/core" coremocks "github.com/hashicorp/vagrant-plugin-sdk/core/mocks" @@ -79,8 +80,20 @@ func TestBasis(t testing.T, opts ...BasisOption) (b *Basis) { require.NoError(t, err) t.Cleanup(func() { os.RemoveAll(td) }) - projDir, err := datadir.NewBasis(td) - require.NoError(t, err) + mkSubdir := func(root, sub string) string { + sd := filepath.Join(root, sub) + require.NoError(t, os.Mkdir(sd, 0755)) + return sd + } + + projDir := &datadir.Basis{ + Dir: datadir.NewBasicDir( + mkSubdir(td, "config"), + mkSubdir(td, "cache"), + mkSubdir(td, "data"), + mkSubdir(td, "temp"), + ), + } defaultOpts := []BasisOption{ WithClient(singleprocess.TestServer(t)), From 88957f71fb3fea5b2e9eee9ceb0a4e5772f7436e Mon Sep 17 00:00:00 2001 From: Paul Hinze Date: Wed, 25 May 2022 12:52:55 -0500 Subject: [PATCH 3/3] tests: Add assertion to address review comment --- internal/core/machine_test.go | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/internal/core/machine_test.go b/internal/core/machine_test.go index 5c6797e03..7fcef998a 100644 --- a/internal/core/machine_test.go +++ b/internal/core/machine_test.go @@ -60,6 +60,11 @@ func TestMachineSetEmptyId(t *testing.T) { require.Nil(t, dbTarget) require.Error(t, err) + // Verify the DataDir still exists (see below test for more detail on why) + dir, err := tm.DataDir() + require.NoError(t, err) + require.DirExists(t, dir.DataDir().String()) + // Also check new id dbTarget, err = tm.Client().GetTarget(tm.ctx, &vagrant_server.GetTargetRequest{ @@ -74,7 +79,7 @@ func TestMachineSetEmptyId(t *testing.T) { require.Error(t, err) } -func TestMachineSetIdBlankThenSomethingRecreatesDataDir(t *testing.T) { +func TestMachineSetIdBlankThenSomethingPreservesDataDir(t *testing.T) { tm, _ := TestMinimalMachine(t) // Set empty id, followed by a temp id. This is the same thing that happens