add experimental daemon plugin for 9P2000.L support #6612

djdv · 2019-08-29T16:41:23Z

Part of: ipfs-inactive/package-managers#74

Draft TODO:

Needs tests
- test the interfaces directly (go test)
- maybe blackbox tests (sharness, fsx, etc.)
go-ipfs/plugin should support plugin scoped configs (we can punt on this)
- We want to remove the go-ipfs-config dependency in our plugin, and instead, receive some context from go-ipfs/plugin
- e.g. var cfg Config.FileSystem = findCfgPathAndLoad(myNonStandardPath, &myWhateverFormat, ... could be avoided by changing Plugin.Init() to something like Plugin.Init(context, opts...)
- Resolved via plugins: add support for plugin configs #6613
~~Trying to enable the experiment through ipfs config --json Experimental.FileSystemEnabled true should work~~
- We can simply remove the plugin from the preload list, and document how to add it in experimental-features.md 🠗
Documentation
- package / code
- experimental-features.md

Other known issues:

~~go.mod has replace directives for this patchset, these need to point to the final dependency commits before merging~~
~~Directory reads in particular need to be fixed before merging. (currently reads entire directory every request and returns requested slice instead of using a forward-only stream)~~

Stebalien · 2019-09-03T01:08:33Z

plugin/plugins/filesystem/client/client.go

@@ -0,0 +1,168 @@
+package p9client


This is for testing, right?

This is intended to become a wrapper library for clients to use. So that they can dial our plugin's server easily. Longer term it should shape the VFS interface. Currently we just use 9P semantics, but we can define operations such as Readdir here to be more brief, much like how MFS does with operations like Mkdir.

e.g.
doing ents, err := client.Readdir(someFilesystem, "/some/path") rather than using the interface directly yourself.

However this is more related to ipfs-inactive/package-managers#71 and should be broken up. This PR should only relate to the server portion at the moment so I'll remove this from the patchset.

Stebalien · 2019-09-03T01:10:47Z

plugin/plugins/filesystem/filesystem.go

+
+	cfg := &Config{}
+	if env.Config != nil {
+		byteRep, err := json.Marshal(env.Config)


Hm. Maybe we should pass in a json.RawMessage? Or does that leak too much?

Are you saying env.Config would change to be of type json.RawMessage rather than ~~[]byte~~ interface{}?

That probably makes the most sense. I'm trying to think of a situation where you'd need raw bytes in the plugin config, that can't be fit inside formatted json, but nothing comes to immediate mind that wouldn't be better stored elsewhere.
(e.g. storing a CID to some binary in the config rather than encoding the (complete) binary itself)

So long as we discard the object once we've loaded and parsed, I'd imagine things wouldn't use any more resources than the current implementation. But I'm saying that without having doing any actual comparisons.

Alternatively, I'm assuming you could have meant ~~casting~~ asserting env.Config to as a RawMessage as well.

I went forward with the second one for now
b6d1506

but this isn't tested yet.

This #6612 (comment) likely needs to be resolved, and I'd like to see some method for Plugins to initiate a Save()/Store()/Whatever() for their section (only) of the config, from within go, if we possibly can.
e.g., I'd like to be able to load plugin config, modify some structure, and push it back to the config, but not have full config access.

plugin/plugins/filesystem/filesystem.go

plugin/plugins/filesystem/nodes/base.go

plugin/plugins/filesystem/nodes/ipfs.go

Stebalien · 2019-09-05T00:56:49Z

plugin/plugins/filesystem/nodes/ipfs.go

+	var err error
+	for _, name := range names {
+		//TODO: timerctx; don't want to hang forever
+		if walkedNode.Path, err = id.core.ResolvePath(id.Ctx, corepath.Join(walkedNode.Path, name)); err != nil {


Hm. This looks like something we might want to tweak in the CoreAPI. We probably want a version that returns a channel of Resolved{Component: string, To: Path} results.

TODO later: ipfs/interface-go-ipfs-core#42

plugin/plugins/filesystem/nodes/utils.go

Stebalien · 2019-09-05T01:26:32Z

plugin/plugins/filesystem/nodes/utils.go

+		//defer asyncContext.Cancel()
+		defer close(relayChan)
+
+		for i := 0; i != oStat.NumLinks; i++ {


This won't work with sharded directories.

Can't we just wait for Ls to close coreChan?

I changed how IPFS Readdir logic works, so that the system checks and stores the coreChan on Open(), and progresses on it during Readdir(), in a forward-only fashion.
https://github.com/ipfs/go-ipfs/blob/059c218a76149c270f6cd11d21a4a9dbd9172bae/plugin/plugins/filesystem/nodes/ipfs.go#L103

^ needs test: create sharded directory and read from it

plugin/plugins/filesystem/nodes/utils.go

Stebalien · 2019-09-05T01:29:38Z

Main things to address now:

Docs. This will make it easier to understand the architecture.
QIDs. We're going to have to find some way to make this work and I would really like to avoid putting anything on disk. Unfortunately, 1e6 files is going to cost us probably 100MiB of memory if we keep tables in memory.

lanzafame

A friendly reminder from someone trying to extract perf information out of ipfs to understand how gateways are performing, please pass contexts around everywhere before this gets merged. 😸

djdv · 2019-09-05T15:27:16Z

(Fixed building for linux,darwin 386 and linux arm,arm64. Now the tests will run, but they're broken at the moment)

djdv · 2019-10-23T15:48:54Z

I apologize to anyone monitoring this thread for using all of their RAM. There's a lot of comments in here!
I did an entire pass on them to make sure nothing gets lost.

Judgment needed on unresolved issues (ping @Stebalien)
~~#6612 (comment)~~
#6612 (comment)
#6612 (comment)
~~#6612 (comment)~~ -> moved to below
#6612 (comment)
~~#6612 (comment)~~
~~#6612 (comment)~~
└ updated ━ #6612 (comment)
~~#6612 (comment)~~

@ myself
Don't forget about these even if Github collapses them:
Document our root path thing
#6612 (comment)
basic: 0167805

Make sure to handle and test for sharded directories, HAMTs, etc.
#6612 (comment)
#6612 (comment)
-
#6612 (comment)
basic: 76d7d79

Do another pass on permissions
#6612 (comment)
#6612 (comment)
and attributes
#6612 (comment)
Looked at during restructure: 0b1fe20

Discuss how to handle overridding the config file's Multiaddr string / Environment variables
#6612 (comment)
#6612 (comment)

We've neglected to properly handle configuration in general, put thought into it and write a variety of tests
mimimum:

no config found, should initalize with defaults that persist on disk
config exists, validte that the parser for it is correct
make sure overridding the config with things like environment variables works as expected
add experimental daemon plugin for 9P2000.L support #6612 (comment)
add experimental daemon plugin for 9P2000.L support #6612 (comment)

Don't forget to filter metadata requests:
#6612 (comment)
We currently fetch data on demand instead of storing and filtering the structure with the reference. Returning only what was asked for.

Consistency check in docs and loggers:
The protocol is "9P", the library is "p9", Linux's mount uses 9p as an argument. (<- add this itself somewhere up front so documentation is somewhat more clear)
#6612 (comment)

We partially amended this:
but we currently swallow bytesRead instead returning 0. Don't do that.
#6612 (comment)
92ed82c
+
c294ac8

Reassess how/why we use loggers the way we do, and document it regardless:
#6612 (comment)

Make a pass on QID's in general:
Make sure we have some semblance of consistency.
#6612 (comment)
better: 0b1fe20

Read doesn't loop until full:
https://github.com/ipfs/go-ipfs/pull/6612/files#r338454668

djdv · 2019-10-23T17:49:43Z

We'll also want to make sure this change gets bubbled up for our Windows users: multiformats/go-multiaddr-net#60

plugin/plugins/filesystem/nodes/pinfs.go

plugin/plugins/filesystem/nodes/root.go

Stebalien

General advice:

Be careful about premature optimizations (or generalizations). The walking code is getting pretty complicated.
There are quite a few commented code chunks. When commenting out (then committing) code like this, explaining why it's commented helps.

Other than that, this looks like it's getting close.

Stebalien · 2019-10-24T07:50:51Z

plugin/plugins/filesystem/filesystem.go

+		// store error on the fs object then close our syncing channel (see use in `Close` below)
+		fs.serverErr = server.Serve(manet.NetListener(fs.listener))
+
+		if fs.closing { //[async] we expect `Accept` to fail while `Close` is in progress


👍 (nice comment)

Stebalien · 2019-10-24T07:52:57Z

plugin/plugins/filesystem/nodes/base.go

+	// Storage for file's metadata
+	Qid      *p9.QID
+	meta     *p9.Attr
+	metaMask *p9.AttrMask


We discussed this offline. Just storing the data probably isn't an issue.

This will cause issues when moving back and forth though (walking back to ..), so we should probably store metadata in an index on the FS instead.

Define "cause issues"? If this breaks .., we should definitely just pass these by value and optimize later.

Stebalien · 2019-10-24T07:53:16Z

plugin/plugins/filesystem/nodes/base.go

+	Qid      *p9.QID
+	meta     *p9.Attr
+	metaMask *p9.AttrMask
+	Logger   logging.EventLogger


plugin/plugins/filesystem/nodes/base.go

Stebalien · 2019-10-24T08:00:19Z

plugin/plugins/filesystem/utils/utils.go

+
+	It is valid to return a newly constructed reference or modify and return the existing reference
+	as long as `QID` is ready to be called on the resulting node
+	and resources are reaped where sensible within the fs implementation


This still needs to clarify who owns what. The interface contract needs to specify constraints on the inputs/outputs, describing what may/may not happen internally isn't all that important.

Something like "Step steps into the current directory, consuming the WalkRef. After calling Step, the original WalkRef must not be used."

Stebalien · 2019-10-24T08:55:48Z

plugin/plugins/filesystem/nodes/ipfs.go

+		select {
+		case entry, open := <-id.directory.entryChan:
+			if !open {
+				//id.operationsCancel()


Uncomment or document?

plugin/plugins/filesystem/nodes/ipfs.go

plugin/plugins/filesystem/nodes/keyfs.go

Stebalien · 2019-10-24T09:00:15Z

plugin/plugins/filesystem/nodes/root.go

+}
+
+// OverlayFileMeta holds data relevant to file system nodes themselves
+type OverlayFileMeta struct {


This seems reasonable but I'd definitely include that reasoning in the code docs.

Stebalien · 2019-10-24T09:03:16Z

plugin/plugins/filesystem/nodes/base.go

+
+/* general helpers */
+
+func (b *Base) ninePath() p9Path { return b.qid.Path }


This really doesn't need to exist.

djdv mentioned this pull request Aug 29, 2019

add an experimental version of mount to go-ipfs ipfs-inactive/package-managers#74

Closed

20 tasks

djdv force-pushed the feat/filesystem-plugin branch 4 times, most recently from 678a532 to 50c30a4 Compare September 4, 2019 12:24

Stebalien reviewed Sep 5, 2019

View reviewed changes

lanzafame reviewed Sep 5, 2019

View reviewed changes

djdv force-pushed the feat/filesystem-plugin branch 4 times, most recently from 24c4aca to b9a988f Compare September 5, 2019 15:20

djdv force-pushed the feat/filesystem-plugin branch 3 times, most recently from 2b9a5c2 to d922b96 Compare September 7, 2019 15:47

djdv added 14 commits September 9, 2019 11:11

plugin/fs: initial experiment

2a02a91

plugin/fs: Basic Readdir

ef75786

plugin/fs: Basic ReadAt

43a38b6

plugin/fs: Fix a bunch of path related issues

7a80532

plugin/fs: Fix a bunch of metadata related issues

2d99302

plugin/fs: linting

75618a5

plugin/fs: use config

07534a3

plugin/fs: Better init/config

0d28489

plugin/fs: Betterer init/config

7bb358f

plugin/fs: consistent protocol identifier

6a17332

plugin/fs: text secession

090f7cd

squashme-deps

19d5332

plugin/fs: client fixes

353007e

plugin/fs: add PinFS test

6e7ea19

djdv added 11 commits October 15, 2019 16:21

gomod deps

e029c7d

changes and tests around plugin close

8cee086

lol unsaved buffers

2a495c7

expose WalkRef and generalize Backtrack

161d86a

extract WalkRef and cleanup related use

9ef8e6a

unexport NinePath

545d666

inheritance changes related to the base clases

9024b95

small lint and docs

a0465f4

change attach clone method

2aead06

unbreak root attrs

ec9f8ea

cleanup attachers

c744a00

djdv mentioned this pull request Oct 22, 2019

experimental daemon plugin for 9P2000.L support (writable portions) #6720

Closed

djdv mentioned this pull request Oct 23, 2019

experimental daemon plugin for 9P2000.L support (writable portions) djdv/go-ipfs#1

Closed

djdv commented Oct 23, 2019

View reviewed changes

plugin/plugins/filesystem/nodes/pinfs.go Outdated Show resolved Hide resolved

djdv commented Oct 23, 2019

View reviewed changes

plugin/plugins/filesystem/nodes/root.go Outdated Show resolved Hide resolved

djdv added 6 commits October 23, 2019 19:31

slightly saner test strings

65b677f

test open

7ce0e9b

allow ipfs root to be read

4c6b8ee

fix context for proxy roots

6e654db

fix

3501592

doc pass WIP

07db2b9

Stebalien requested changes Oct 24, 2019

View reviewed changes

djdv added 5 commits October 25, 2019 09:34

make sure test defers gets called

984f155

try not to leak

0fa5983

change Plugin Close procedure

c9faaa7

change Plugin Close procedure part 2

c7cfd71

don't swallow Read return value

c294ac8

djdv closed this Nov 23, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

add experimental daemon plugin for 9P2000.L support #6612

add experimental daemon plugin for 9P2000.L support #6612

djdv commented Aug 29, 2019 •

edited

Loading

Stebalien Sep 3, 2019

djdv Sep 7, 2019

Stebalien Sep 3, 2019

djdv Sep 12, 2019 •

edited

Loading

djdv Sep 12, 2019 •

edited

Loading

djdv Sep 12, 2019 •

edited

Loading

Stebalien Sep 5, 2019

Stebalien Sep 5, 2019

Stebalien Sep 5, 2019

djdv Sep 9, 2019

djdv Sep 12, 2019

Stebalien commented Sep 5, 2019

lanzafame left a comment

djdv commented Sep 5, 2019

djdv commented Oct 23, 2019 •

edited

Loading

djdv commented Oct 23, 2019

Stebalien left a comment

Stebalien Oct 24, 2019

Stebalien Oct 24, 2019

Stebalien Oct 24, 2019

Stebalien Oct 24, 2019

Stebalien Oct 24, 2019

Stebalien Oct 24, 2019

Stebalien Oct 24, 2019

Stebalien Oct 24, 2019


		/* general helpers */

		func (b *Base) ninePath() p9Path { return b.qid.Path }

add experimental daemon plugin for 9P2000.L support #6612

add experimental daemon plugin for 9P2000.L support #6612

Conversation

djdv commented Aug 29, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

djdv Sep 12, 2019 • edited Loading

Choose a reason for hiding this comment

djdv Sep 12, 2019 • edited Loading

Choose a reason for hiding this comment

djdv Sep 12, 2019 • 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

Stebalien commented Sep 5, 2019

lanzafame left a comment

Choose a reason for hiding this comment

djdv commented Sep 5, 2019

djdv commented Oct 23, 2019 • edited Loading

djdv commented Oct 23, 2019

Stebalien left a 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

djdv commented Aug 29, 2019 •

edited

Loading

djdv Sep 12, 2019 •

edited

Loading

djdv Sep 12, 2019 •

edited

Loading

djdv Sep 12, 2019 •

edited

Loading

djdv commented Oct 23, 2019 •

edited

Loading