Skip to content

engine: tee archive correctly even when run step fails#1994

Merged
maiamcc merged 4 commits into
masterfrom
tee-correctly-when-run-fails
Aug 7, 2019
Merged

engine: tee archive correctly even when run step fails#1994
maiamcc merged 4 commits into
masterfrom
tee-correctly-when-run-fails

Conversation

@maiamcc

@maiamcc maiamcc commented Aug 6, 2019

Copy link
Copy Markdown
Contributor

Fixes observed bug:

  • introduce syntax error to same_img
  • first container fails to update with go build error (expected)
  • second container updates successfully (i.e. runs go build w/o error),
    and we fall back to image build b/c of "inconsistent state: one update
    succeeded, one failed"

The second go build succeeded b/c we weren't correctly syncing the change with the
syntax error to the second container -- early return from buildAndDeploy meant that
we didn't appropriately tee the tar reader.

@maiamcc
maiamcc requested review from jazzdan and nicks August 6, 2019 21:12
@jazzdan

jazzdan commented Aug 6, 2019

Copy link
Copy Markdown
Contributor

@maiamcc if you sync a large, multi-GB file, how does this affect memory usage?

@nicks

nicks commented Aug 6, 2019

Copy link
Copy Markdown
Contributor

ya, this feels like it's trying to optimize for the wrong thing. can we re-tar the files for each container?

@maiamcc

maiamcc commented Aug 6, 2019

Copy link
Copy Markdown
Contributor Author

Ugh you're both very correct.

}
}()
var archiveReader io.Reader = pr
tarBytes, err := ioutil.ReadAll(pr)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I think this is going to explode memory usage on large files. Try it out with a large, multi-GB file (dd if=/dev/zero of=file.txt count=1024 bs=1048576) and running tilt up. Then create a memory profile by pressing Ctrl+O in the terminal UI.

You can check out the profile with pprof by running go tool pprof tilt.heap_profile.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

That makes sense, and I don't have the internet bandwidth to test this right now asdfgh (like I literally can't even pull the golang docker image) so I pushed up a version where we recreate the tar archive every time, and if tests pass i'll land -- can verify performance later.

@maiamcc

maiamcc commented Aug 7, 2019

Copy link
Copy Markdown
Contributor Author

CI failed with:

=== Failed
=== FAIL: internal/watch TestWatchCountInnerFile (0.02s)
    notify_test.go:615: Got 1 events (expected 3): [{/tmp/TestWatchCountInnerFile218689039/watched231842338/a}] [/tmp/TestWatchCountInnerFile218689039/watched231842338/a /tmp/TestWatchCountInnerFile218689039/watched231842338/a/b /tmp/TestWatchCountInnerFile218689039/watched231842338/a/b/bigFile]

but passes locally??

$ go test github.com/windmilleng/tilt/internal/watch -run TestWatchCountInnerFile -count 1000
ok  	github.com/windmilleng/tilt/internal/watch	58.777s

Rerunning tests.

@maiamcc

maiamcc commented Aug 7, 2019

Copy link
Copy Markdown
Contributor Author

ping! ready!

@jazzdan

jazzdan commented Aug 7, 2019

Copy link
Copy Markdown
Contributor

I will create a memory profile

@maiamcc

maiamcc commented Aug 7, 2019

Copy link
Copy Markdown
Contributor Author

thanks, appreciate it <3

1 similar comment
@maiamcc

maiamcc commented Aug 7, 2019

Copy link
Copy Markdown
Contributor Author

thanks, appreciate it <3

@jazzdan

jazzdan commented Aug 7, 2019

Copy link
Copy Markdown
Contributor

The memory usage LGTM, didn't see it go over 200MB when transferring two 1 GB files

Not sure about the memory usage of the synclet, though. I wasn't able to finagle my network to be able to grab a profile on that end.

Screen Shot 2019-08-07 at 2 26 28 PM

@jazzdan jazzdan left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

LGTM!

@maiamcc
maiamcc merged commit 3aeaf6e into master Aug 7, 2019
@maiamcc
maiamcc deleted the tee-correctly-when-run-fails branch September 11, 2019 16:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants