Implement updated Writable spec #664

MikaelSmith · 2019-12-16T19:21:15Z

Implement the new Writable interface (defined to mirror the Readable interface). Implement Write on simple file interfaces: local, AWS S3, and Google Cloud Storage.

Resolves #620. Also fixes #656.

api/fs/root.go

MikaelSmith · 2019-12-16T19:25:27Z

This is still a work in progress, but I think the approach is about right. Includes a rewrite of fuse/file to broadly mimic https://github.com/bazil/bolt-mount/blob/master/file.go, as suggested in bazil/fuse#225.

Remaining work:

Make the local plugin a developer option that's enabled at compile-time. It was very useful for testing.
Implement Write on AWS S3 objects.
Add more tests for fuse/file.
Revisit TODOs on reading data when file is opened for writing. We don't have buffering for block reading, which would be useful here so we don't reread from APIs with $ costs. Writing a file to Google Cloud Storage is currently a little slow because we repeat the read, then write updated data. In many cases we'll receive a full write of the file as well, so we don't need to read any data; we could delay reading until we get a non-contiguous write. Requiring that we can Read on Opening a file for Writing also means that entries that implement Write but not Read will fail.
Implement Writable for external plugins.

ekinanp

Still reviewing, just left a few initial comments.

plugin/types.go

plugin/gcp/storageObject.go

cmd/internal/server/core.go

fuse/dir.go

plugin/gcp/pubsubTopic.go

api/fs/root.go

MikaelSmith · 2019-12-17T23:33:36Z

I think this is ready for review again.

fuse/file.go

ekinanp

Some initial comments, still reviewing

api/fs/root.go

fuse/file.go

This is a developer-centric option that has to be explicitly enabled with an environment variable. By default it mounts `/tmp`, can be configured with `local.basepath` to mount any other directory. Signed-off-by: Michael Smith <[email protected]>

Implement the new Writable interface (defined to mirror the Readable interface). Implement Write on a local file interface for testing. Also adds fuse/file tests. Supports puppetlabs-toy-chest#620. Signed-off-by: Michael Smith <[email protected]>

This makes editors that rely on size read the entire file. Saves size from the last open on the fs node so we can use it to serve attribute requests. Fixes puppetlabs-toy-chest#656. Signed-off-by: Michael Smith <[email protected]>

Also use a context for S3 get object requests. Signed-off-by: Michael Smith <[email protected]>

Signed-off-by: Michael Smith <[email protected]>

Add `sizeValid` to track whether we need to call `plugin.Size` before using `size`. This is only needed when opening a file WriteOnly to delay a Read - to ensure we have the entire file content before Write - so we only do it when necessary. Handle `io.EOF` when loading the file in case a call to `Setattr` increased it beyond the original size but didn't write to fill that space, and pad with null characters when that happens. Upgrade journal entries to warnings when they result in an error. FUSE doesn't relay the actual error messages. Delay releasing data and clearing entry cache until we release all writers. Tweak a few tests so we have a balance of ReadWrite tests that rely on attribute size or calling Read to determine size. Update TestTruncateAndWrite to ensure we don't call Read when we've called Setattr to truncate the file first. Add additional comments explaining how to use BlockReadable and Writable, and some code comments to clarify how Open works. Signed-off-by: Michael Smith <[email protected]>

ekinanp

Per the conversation on Slack, we want a way to distinguish between file-like and non-file-like entries.

File-like entries are entries with content, such that Read returns that content and Write updates it. Non-file-like entries are entries without content, so that "Read" data could represent one thing and the "Written" data could represent another.
For plugin author docs: file-like entries are entries with a Size attribute (implicitly entry is also Readable). So an entry has content iff it has a Size attribute. Thus, people should not set size for non-file-like entries.
- Reason: Size attribute means you can filter those things with find. When would filtering on Size make sense for Docker container logs, cloud function logs, etc?
  - We could always change these semantics later to a is-file-like key in the entry JSON on user demand
Implementation:
- File-like entries are Readable, Writable, and have a Size attribute.
  - Reason: Up to now, we tell people “set size if you know it” for Readable entries. We also do that for things like metadata.json entries, which are not file-like. These semantics ensure that those entries don't accidentally become file-like.
- For file-like entries, f.data represents the content. For non-file-like entries, f.data represents what's about to be written.
- For file-like entries, the first Write call loads the entire data (because we're going to overwrite it anyways on a Flush). f.data is then updated appropriately with the written block.
- Flush should just write whatever f.data is.
- For Read, f.data should only be returned for file-like entries (and if it exists). Otherwise, we should delegate to plugin.ReadWithAnalytics. That's because f.data represents the content for file-like entries while for other entries, it's separate from what could be read.

ekinanp · 2019-12-19T07:50:26Z

One example of a non-file like entry that may have Read/Write could be a GCP log, where read could return its most recent 1000 entries while write could write a new entry.

MikaelSmith · 2019-12-20T00:46:17Z

Updated.

Clarifies difference in how files with and without Size attributes work by defining them as file-like and non-file-like and describing what that means. Primarily enables interaction with files where read and write are not symmetric. Signed-off-by: Michael Smith <[email protected]>

ekinanp

Reviewing the tests next

CONTRIBUTING.md

api/fs/root.go

ekinanp · 2020-01-02T18:39:24Z

docs/docs/index.md

+### write
+The `write` action lets you write data to an entry. Thus, any command that writes a file also works with these entries.
+
+Note that Wash distinguishes between file-like and non-file-like entries. An entry is file-like if it's readable and writable and defines its size; you can edit it like a file. If it doesn't define a size then it's non-file-like, and trying to open it with a ReadWrite handle will error; reads from it may not return data you previously wrote to it.


I'd also add a note about checking docs for that entry's write semantics (non-file-like entries only)

docs/docs/index.md

docs/docs/external-plugins.md

fuse/file.go

plugin/externalPluginEntry.go

fuse/file_test.go

ekinanp · 2020-01-02T21:17:42Z

fuse/file.go

+	return &file{fuseNode: newFuseNode("f", p, e), writers: make(map[fuse.HandleID]struct{})}
+}
+
+func (f *file) isFileLike() bool {


Hmm, maybe isFileLike => isFileLikeEntry ? It's slightly weird seeing file#isFileLike, esp. if you're new to this code.

fuse/file.go

docs/docs/index.md

ekinanp · 2020-01-03T00:10:02Z

plugin/types.go

+interface allows sending data to the entry.
+
+Wash distinguishes between two different patterns for things you can read and write. It considers
+a "file-like" entry to be one with a defined size (so the `size` attribute is set when listing the


OK, I agree about using size alone for file-like things. But I think we should remove size from the metadata/data.json files because those aren't really "files" (editing them makes no sense) so they shouldn't be treated as such unless we have a good reason to do so in the future.

I think editing metadata does make sense for some things: the metadata is a reflection of a mix of state and configuration, and you may be able to make changes to that configuration. Editing those values to change the configuration would make sense. This is especially true for everything in Kubernetes.

Right, but what you write isn't going to strictly match what's read -- there isn't a 1:1 symmetry between the content because we output pretty-printed JSON. But I don't have strong feelings about keeping size around either so up to you.

Ok, that makes sense.

ekinanp · 2020-01-03T00:11:05Z

plugin/types.go

+// entry is only Writable, then only full writes (starting from offset 0) are
+// allowed, anything else initiated by the filesystem will result in an error.
+//
+// It's up to the implementer to decide how much data integrity to guarantee.


What do you mean by "data integrity"? Clarifying that would be useful.

Primarily around whether writing volumes makes sure an fsync happens so that data is serialized to persistent storage before returning. Not sure how to summarize that.

Ah ok. Sounds like an issue for file-like entries, which could be summed up as plugin.Write(data); data == plugin.Read (ensure that the next Read from the API will fetch the written data). Don't know if that implies "always call fsync" for volumes.

I ended up removing this because I don't think any plugins should explicitly implement something like calling sync after writing. It represents a possible loss of data, but the machine would have to die right after and we'd likely get a connection error if that happened. Things like vim and databases will call Fsync, and we're currently lying to them in some cases that it did something, but for now I'd suggest don't run a database on Wash's filesystem.

plugin/types.go

ekinanp · 2020-01-03T00:14:26Z

plugin/externalPluginEntry_test.go

+
+var _ = internal.Command(&mockCommand{})
+
+func (suite *ExternalPluginEntryTestSuite) TestWrite() {


Why does Write have so many tests? Seems like it should be symmetric with other InvokeAndWait tests.

I'll have to spend a little time on this tomorrow. I'm not sure the other tests are verifying all the assertions we expect. They also mock at a higher level (the script's InvokeAndWait call) than Write had available originally.

Address review comments to try and clarify code. Signed-off-by: Michael Smith <[email protected]>

Moves script `InvokeAndWait` behavior to a function on the invocation so we can setup more complicated invocations before running them. Keeps the script `InvokeAndWait` as a helper for the most common case. Signed-off-by: Michael Smith <[email protected]>

Don't set size on console output and metadata files. The plugin system knows how to get the size just as efficiently, and explicitly setting the size implies they'll behave like files (which may not be true). Signed-off-by: Michael Smith <[email protected]>

ekinanp reviewed Dec 16, 2019

View reviewed changes

api/fs/root.go Show resolved Hide resolved

MikaelSmith force-pushed the writes branch 2 times, most recently from ffc29d6 to 8458a1c Compare December 16, 2019 23:24

ekinanp reviewed Dec 16, 2019

View reviewed changes

plugin/types.go Outdated Show resolved Hide resolved

plugin/gcp/storageObject.go Show resolved Hide resolved

cmd/internal/server/core.go Outdated Show resolved Hide resolved

fuse/dir.go Outdated Show resolved Hide resolved

ekinanp reviewed Dec 16, 2019

View reviewed changes

plugin/gcp/pubsubTopic.go Show resolved Hide resolved

ekinanp reviewed Dec 16, 2019

View reviewed changes

api/fs/root.go Show resolved Hide resolved

MikaelSmith force-pushed the writes branch 3 times, most recently from 75c31ea to 03a397d Compare December 17, 2019 22:34

MikaelSmith marked this pull request as ready for review December 17, 2019 23:24

MikaelSmith requested a review from grimradical as a code owner December 17, 2019 23:24

MikaelSmith requested a review from a team December 17, 2019 23:24

ghost requested review from ekinanp and removed request for a team December 17, 2019 23:24

MikaelSmith force-pushed the writes branch from f74bad8 to adc0618 Compare December 17, 2019 23:33

MikaelSmith force-pushed the writes branch from adc0618 to 4ea52fa Compare December 17, 2019 23:38

MikaelSmith commented Dec 18, 2019

View reviewed changes

fuse/file.go Outdated Show resolved Hide resolved

ekinanp reviewed Dec 18, 2019

View reviewed changes

api/fs/root.go Outdated Show resolved Hide resolved

fuse/file.go Show resolved Hide resolved

fuse/file.go Outdated Show resolved Hide resolved

fuse/file.go Outdated Show resolved Hide resolved

fuse/file.go Outdated Show resolved Hide resolved

MikaelSmith added 6 commits December 18, 2019 14:06

Determine size on Open if unknown

3987b29

This makes editors that rely on size read the entire file. Saves size from the last open on the fs node so we can use it to serve attribute requests. Fixes puppetlabs-toy-chest#656. Signed-off-by: Michael Smith <[email protected]>

Implement Write for AWS S3 and Google Cloud Storage objects

76857f5

Also use a context for S3 get object requests. Signed-off-by: Michael Smith <[email protected]>

Implement Writable for external plugins

79cca5c

Signed-off-by: Michael Smith <[email protected]>

Update README based on features we've implemented recently

33e8f02

Signed-off-by: Michael Smith <[email protected]>

MikaelSmith force-pushed the writes branch from 47500f3 to 780630d Compare December 18, 2019 22:18

MikaelSmith force-pushed the writes branch from 780630d to 8a4d858 Compare December 18, 2019 22:43

ekinanp reviewed Dec 19, 2019

View reviewed changes

MikaelSmith force-pushed the writes branch from fa78390 to eaf8705 Compare December 20, 2019 00:47

MikaelSmith force-pushed the writes branch 2 times, most recently from dee1c8d to 8fc111e Compare December 20, 2019 19:26

ekinanp reviewed Jan 2, 2020

View reviewed changes

fuse/file.go Show resolved Hide resolved

ekinanp reviewed Jan 2, 2020

View reviewed changes

fuse/file.go Show resolved Hide resolved

MikaelSmith force-pushed the writes branch 3 times, most recently from 74e4ad5 to 7ba3196 Compare January 2, 2020 23:59

ekinanp reviewed Jan 3, 2020

View reviewed changes

docs/docs/index.md Outdated Show resolved Hide resolved

ekinanp reviewed Jan 3, 2020

View reviewed changes

plugin/types.go Show resolved Hide resolved

ekinanp reviewed Jan 3, 2020

View reviewed changes

MikaelSmith force-pushed the writes branch 2 times, most recently from 5531a82 to adc7bc3 Compare January 3, 2020 01:20

MikaelSmith added 3 commits January 3, 2020 11:46

Work on code comments and naming things

b56866e

Address review comments to try and clarify code. Signed-off-by: Michael Smith <[email protected]>

MikaelSmith force-pushed the writes branch from adc7bc3 to 5f0ad9a Compare January 3, 2020 19:46

ekinanp approved these changes Jan 3, 2020

View reviewed changes

MikaelSmith merged commit 2abcd30 into puppetlabs-toy-chest:master Jan 3, 2020

MikaelSmith deleted the writes branch January 3, 2020 19:50

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement updated Writable spec #664

Implement updated Writable spec #664

MikaelSmith commented Dec 16, 2019

MikaelSmith commented Dec 16, 2019 •

edited

Loading

ekinanp left a comment

MikaelSmith commented Dec 17, 2019

ekinanp left a comment

ekinanp left a comment •

edited

Loading

ekinanp commented Dec 19, 2019

MikaelSmith commented Dec 20, 2019

ekinanp left a comment

ekinanp Jan 2, 2020

ekinanp Jan 2, 2020 •

edited

Loading

ekinanp Jan 3, 2020 •

edited

Loading

MikaelSmith Jan 3, 2020

ekinanp Jan 3, 2020

MikaelSmith Jan 3, 2020

ekinanp Jan 3, 2020 •

edited

Loading

MikaelSmith Jan 3, 2020

ekinanp Jan 3, 2020

MikaelSmith Jan 3, 2020

ekinanp Jan 3, 2020

MikaelSmith Jan 3, 2020


		var _ = internal.Command(&mockCommand{})

		func (suite *ExternalPluginEntryTestSuite) TestWrite() {

Implement updated Writable spec #664

Implement updated Writable spec #664

Conversation

MikaelSmith commented Dec 16, 2019

MikaelSmith commented Dec 16, 2019 • edited Loading

ekinanp left a comment

Choose a reason for hiding this comment

MikaelSmith commented Dec 17, 2019

ekinanp left a comment

Choose a reason for hiding this comment

ekinanp left a comment • edited Loading

Choose a reason for hiding this comment

ekinanp commented Dec 19, 2019

MikaelSmith commented Dec 20, 2019

ekinanp left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ekinanp Jan 2, 2020 • edited Loading

Choose a reason for hiding this comment

ekinanp Jan 3, 2020 • 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

ekinanp Jan 3, 2020 • 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

MikaelSmith commented Dec 16, 2019 •

edited

Loading

ekinanp left a comment •

edited

Loading

ekinanp Jan 2, 2020 •

edited

Loading

ekinanp Jan 3, 2020 •

edited

Loading

ekinanp Jan 3, 2020 •

edited

Loading