Atomic Snapshotting / Sticky Volume Migration #3563

schmichael · 2017-11-17T01:50:17Z

Includes #3539

Makes sticky migrations atomic: either all data is copied or none is. Previously partial data could be copied.

chelseakomlo · 2017-11-28T18:37:43Z

client/alloc_watcher_test.go

+		Typeflag: tar.TypeReg,
+	}
+	err = tw.WriteHeader(&fooHdr)
+	if err != nil {


Use assert library.

chelseakomlo · 2017-11-28T18:38:58Z

command/agent/alloc_endpoint_test.go

+		// Now write to its shared dir
+		allocDirI, err := s.client.GetAllocFS(alloc.ID)
+		if err != nil {
+			t.Fatalf("unable to find alloc dir: %v", err)


Use assert library.

chelseakomlo · 2017-11-28T18:39:38Z

command/agent/alloc_endpoint_test.go

+		if err := allocDir.Snapshot(ioutil.Discard); err == nil {
+			t.Errorf("expected Snapshot() to fail but it did not")
+		} else {
+			s.logger.Printf("[DEBUG] agent.test: snapshot returned error: %v", err)


Isn't err nil in the else case?

I flipped the normal if/else order and handled the nil case first. Will fip to the usual order.

chelseakomlo · 2017-11-28T21:02:55Z

client/alloc_watcher_test.go

+	t.Parallel()
+	dest, err := ioutil.TempDir("", "nomadtest-")
+	if err != nil {
+		t.Fatalf("err: %v", err)


Use assert test package- same for all other tests.

With that said, the tests look great!

I actually prefer this style as assert uses t.Error not t.Fatal so your test continues and usually panics due to the non-error value being nil.

...but maybe I'm the only one who gets confused by this? 😕

chelseakomlo · 2017-11-28T21:03:42Z

client/alloc_runner.go

@@ -841,6 +841,14 @@ func (r *AllocRunner) Run() {

 		// Soft-fail on migration errors
 		r.logger.Printf("[WARN] client: alloc %q error while migrating data from previous alloc: %v", r.allocID, err)
+
+		// Recreate alloc dir to ensure a clean slate
+		r.allocDir.Destroy()


Can allocDir ever be nil?

No, allocDir is initialized in NewAllocRunner and RestoreState includes (hopefully sufficient!) checks to prevent restoring a nil alloc dir. (Good question as I believe this did cause issues in the past!)

chelseakomlo · 2017-11-28T21:05:54Z

client/alloc_watcher.go

@@ -463,6 +463,9 @@ func (p *remotePrevAlloc) streamAllocDir(ctx context.Context, resp io.ReadCloser
 		}
 	}

+	// if we see this file, there was an error on the remote side
+	errorFilename := allocdir.SnapshotErrorFilename(p.prevAllocID)


I might be missing something obvious, but I can't find where allocdir is initialized. Can it ever be nil?

allocdir is sneakily referring to a package here... kind of unfortunate I use allocDir a lot as a variable and allocdir a lot as a package... 😬

chelseakomlo · 2017-11-28T21:08:19Z

client/allocdir/alloc_dir.go

+		Typeflag:   tar.TypeReg,
+	}
+
+	if err := tw.WriteHeader(&hdr); err != nil {


Can tw ever be nil?

No, it's always initialized in the single caller to writeError.

chelseakomlo · 2017-11-28T21:09:14Z

client/allocdir/alloc_dir.go

+// writeError writes a special file to a tar archive with the error encountered
+// during snapshotting. See Snapshot().
+func writeError(tw *tar.Writer, allocID string, err error) error {
+	contents := []byte(fmt.Sprintf("Error snapshotting: %v", err))


Does this need to be at the beginning of the function? Maybe put it closer to where it is actually used.

Need to put its length into the Header, so it has to stay here.

chelseakomlo · 2017-11-28T21:10:22Z

client/allocdir/alloc_dir.go

@@ -242,6 +265,7 @@ func (d *AllocDir) Destroy() error {
 		mErr.Errors = append(mErr.Errors, fmt.Errorf("failed to remove alloc dir %q: %v", d.AllocDir, err))
 	}

+	d.built = false


Maybe an explanatory comment for why this flag is changed?

If an alloc dir is being GC'd (removed) during snapshotting the walk func will be passed an error. Previously we didn't check for an error so a panic would occur when we'd try to use a nil `fileInfo`.

Test that snapshot errors don't return a valid tar currently fails.

as per PR comments

dadgar · 2017-12-04T23:53:14Z

website/source/docs/job-specification/ephemeral_disk.html.md

@@ -41,7 +41,9 @@ job "docs" {
 - `migrate` `(bool: false)` - When `sticky` is true, this specifies that the
  Nomad client should make a best-effort attempt to migrate the data from a
  remote machine if placement cannot be made on the original node. During data
-  migration, the task will block starting until the data migration has completed.
+  migration, the task will block starting until the data migration has


Document the API as well

dadgar · 2017-12-04T23:54:38Z

client/allocdir/alloc_dir.go

@@ -24,6 +24,10 @@ const (
 )

 var (
+	// SnapshotErrorTime is the sentinel time that will be used on the
+	// error file written by Snapshot when it encounters as error.
+	SnapshotErrorTime = time.Date(2000, 0, 0, 0, 0, 0, 0, time.UTC)


Probably should have a IsSnapshotErrorFile method that checks this as well as the file name. Right now this is unused on the validation path

dadgar · 2017-12-04T23:55:16Z

client/alloc_runner.go

@@ -841,6 +841,14 @@ func (r *AllocRunner) Run() {

 		// Soft-fail on migration errors
 		r.logger.Printf("[WARN] client: alloc %q error while migrating data from previous alloc: %v", r.allocID, err)
+
+		// Recreate alloc dir to ensure a clean slate
+		r.allocDir.Destroy()


Check for error here. Probably can't continue if the destroy fails as well!

github-actions · 2023-03-17T02:13:31Z

I'm going to lock this pull request because it has been closed for 120 days ⏳. This helps our maintainers find and focus on the active contributions.
If you have found a problem that seems related to this change, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further.

schmichael mentioned this pull request Nov 17, 2017

[WIP] Handle errors during snapshotting #3539

Closed

schmichael force-pushed the b-snapshot-atomic branch from e2972ce to e7191e2 Compare November 17, 2017 23:22

schmichael changed the title ~~[WIP] Atomic Snapshotting~~ Atomic Snapshotting / Sticky Volume Migration Nov 17, 2017

chelseakomlo suggested changes Nov 28, 2017

View reviewed changes

schmichael added 5 commits November 29, 2017 17:26

Handle errors during snapshotting

4a49375

If an alloc dir is being GC'd (removed) during snapshotting the walk func will be passed an error. Previously we didn't check for an error so a panic would occur when we'd try to use a nil `fileInfo`.

Destroy partially migrated alloc dirs

a05862d

Test that snapshot errors don't return a valid tar currently fails.

Check for error file when receiving snapshots

e3256ec

Add comment and normalize err check ordering

50b335f

as per PR comments

Add atomic migrations to changelog and docs

1ffb189

schmichael force-pushed the b-snapshot-atomic branch from a0ebf56 to 1ffb189 Compare November 30, 2017 01:27

chelseakomlo approved these changes Dec 1, 2017

View reviewed changes

dadgar approved these changes Dec 4, 2017

View reviewed changes

schmichael merged commit b53f24b into master Dec 5, 2017

schmichael deleted the b-snapshot-atomic branch December 5, 2017 17:16

github-actions bot locked as resolved and limited conversation to collaborators Mar 17, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Atomic Snapshotting / Sticky Volume Migration #3563

Atomic Snapshotting / Sticky Volume Migration #3563

schmichael commented Nov 17, 2017 •

edited

Loading

chelseakomlo Nov 28, 2017

chelseakomlo Nov 28, 2017

chelseakomlo Nov 28, 2017

schmichael Nov 30, 2017

chelseakomlo Nov 28, 2017

chelseakomlo Nov 28, 2017

schmichael Nov 30, 2017

chelseakomlo Nov 28, 2017

schmichael Nov 30, 2017

chelseakomlo Nov 28, 2017

schmichael Nov 30, 2017

chelseakomlo Nov 28, 2017

schmichael Nov 30, 2017

chelseakomlo Nov 28, 2017

schmichael Nov 30, 2017

chelseakomlo Nov 28, 2017

dadgar Dec 4, 2017

dadgar Dec 4, 2017

dadgar Dec 4, 2017

github-actions bot commented Mar 17, 2023

Atomic Snapshotting / Sticky Volume Migration #3563

Atomic Snapshotting / Sticky Volume Migration #3563

Conversation

schmichael commented Nov 17, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented Mar 17, 2023

schmichael commented Nov 17, 2017 •

edited

Loading