New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

brfs: FUSE filesystem exposing the Store and remote CAS #5705

Merged
merged 27 commits into from Apr 25, 2018

Conversation

Projects
None yet
3 participants
@illicitonion
Copy link
Contributor

illicitonion commented Apr 16, 2018

Exposes

  • ${mount_point}/digest - contains a file named sha256-length for each
    file digest. Does not show anything in response to readdir, but
    opening any particular file (of a known digest) will work.
  • ${mount_point}/directory - contains a directory named sha256-length
    for each directory digest. Does not show anything in response to
    readdir, but readdir on any particular directory (of a known
    digest) will work.

Caveats

  • Not at all performance optimised. In particular, all filesystem
    operations happen single-threaded, including performing expensive I/O
    operations like fetching from a remote CAS or reading from LMDB.
    The underlying implementation is Futures-based, and the expensive
    operations are easily parallelisable, it just hasn't been done yet :)

This uses the low-level https://github.com/zargony/rust-fuse which
requires manual inode management.

There is also a higher-level https://github.com/wfraser/fuse-mt which I
discovered after putting this together, but which I'm consciously not
using. Its benefits would be:

  1. It operates on paths rather than inodes. This would be pretty
    convenient, as the inode cache in this implementation is a little
    annoying.
  2. It runs some operations in parallel by not having every function
    take a &mut self. I haven't looked at how parallel it ends up being.
  3. Its readdir API avoids needing to think about pagination. I ended up
    coincidentally re-inventing their API here :)

But its big drawback is that functions are required to return values
(e.g. from read) rather than call callbacks, which means that it's hard
for us to take advantage of our Futurey implementation in Store, as
we'd need to block in every call.

Both projects have talked about wanting to support Futures as a more
first-class concept at some point in a nebulous future, but neither has
a concrete plan for doing so.

Changes to our Cargo workspace

This excludes brfs from default workspace packages.
This is because not everyone has fuse installed, and we want things to
work out of the box.

To run all tests, including brfs, run cargo test --all. CI does this.
To run all tests, excluding brfs, run cargo test.
To run just the fs package's tests, run cargo test -p fs.

@illicitonion illicitonion force-pushed the twitter:dwagnerhall/brfs branch from 42a26b0 to ead8385 Apr 16, 2018

@dotordogh
Copy link
Contributor

dotordogh left a comment

Thank you for extracting the test data!! It makes writing tests so much easier to comprehend!

}

impl TestDirectory {
pub fn containing_roland() -> TestDirectory {

This comment has been minimized.

@dotordogh

dotordogh Apr 17, 2018

Contributor

I'm thinking doc strings that make the structure of each test directory would make understanding this way easier for someone new looking at the code

TestDirectory { directory }
}

pub fn directory(&self) -> &bazel_protos::remote_execution::Directory {

This comment has been minimized.

@dotordogh

dotordogh Apr 17, 2018

Contributor

This is all really cool! Thank you for doing it!

Arc::new(fs::ResettablePool::new("test-pool-".to_string())),
).expect("Error creating local store");

let test_bytes = TestData::roland();

This comment has been minimized.

@dotordogh

dotordogh Apr 17, 2018

Contributor

This is fantastic!!

@stuhood
Copy link
Member

stuhood left a comment

This is awesome, and I want to land it. There is one somewhat-fundamental question about the model to answer first I think.

let mut parts = str.split("-");
let fingerprint_str = parts
.next()
.ok_or_else(|| format!("Invalid digest: {}", str))?;

This comment has been minimized.

@stuhood

stuhood Apr 17, 2018

Member

Each of these Invalid digest: messages could be more specific.

This comment has been minimized.

@illicitonion

illicitonion Apr 24, 2018

Contributor

Fleshed out


struct BuildResultFS {
store: fs::Store,
inode_digest_cache: HashMap<Inode, InodeDetails>,

This comment has been minimized.

@stuhood

stuhood Apr 17, 2018

Member

A bit about strategy here... I think that one thing that might be possible would be to have a caching wrapper around Store, which caches Shared<Box<Future<T,E>>> store results... or to use Graph to do that memoization for you. If you were to use a Graph all from within the same context, the result would be a single root with all of the filesystem requests as direct leaf children... ie, ~no cost to cycle detect.

As mentioned in slack, the advantage of caching Futures is that you can prepopulate them from a background thread which would eagerly walk the Digest/Directory to warm it. This would allow for concurrent fetching/checkouts, even when the process consuming brfs is not reading files concurrently.

Depending on the memoization API, the "downside" would be that all inputs would be in memory. But honestly, I expect that that would be fine for a good long while (if not forever).

This comment has been minimized.

@illicitonion

illicitonion Apr 24, 2018

Contributor

I like the idea - I'm very happy to add caching and pre-loading in a follow-up PR, if that works for you? :)

name: OsString::from("directory"),
},
]),
// Skip digest contents because enumerating them takes forever.

This comment has been minimized.

@stuhood

stuhood Apr 17, 2018

Member

This would be "everything recursively visible within the Directory I have mounted"? One thing here is that we'll want to actually validate that a requested digests are members of the root Directory in order to prevent people escaping the sandbox that way. So eagerly listing the Directory might be a good idea anyway.

This comment has been minimized.

@illicitonion

illicitonion Apr 24, 2018

Contributor

Significantly expanded the comment. Is it more clear now?

This comment has been minimized.

@stuhood

stuhood Apr 24, 2018

Member

Well, it's clear what's happening, but I don't think it's addressed my comment.... I probably left it in the wrong place.

My point was that you can escape the sandbox by requesting an explicit digest with (DIGEST_ROOT, Some(digest_str)) that is not located under your root digest.

It's probably not a blocker, but it might guide future work here in the direction of eagerly listing the root.

This comment has been minimized.

@illicitonion

illicitonion Apr 24, 2018

Contributor

I wasn't putting this together with sandboxing in mind, just exposing files. I'm not sure what sandboxing use-cases we have, but we should for sure talk about that and design it when we need it :) I'm hoping that we can just reuse something like sandboxfs when it comes to that, though.

This comment has been minimized.

@stuhood

stuhood Apr 24, 2018

Member

Ok, I see what you mean. If a user process has access to this directory, then they have access to plenty of other things as well. But in the presence of a sandbox, you just wouldn't give them access to this directory.

illicitonion added some commits Apr 16, 2018

brfs: FUSE filesystem exposing the Store and remote CAS
Exposes:
 * ${mount_point}/digest - contains a file named sha256-length for each
   file digest. Does not show anything in response to `readdir`, but
   opening any particular file (of a known digest) will work.
 * ${mount_point}/directory - contains a directory named sha256-length
   for each directory digest. Does not show anything in response to
   `readdir`, but `readdir` on any particular directory (of a known
   digest) will work.

Caveats:
 * Not at all performance optimised. In particular, all filesystem
   operations happen single-threaded, including performing expensive I/O
   operations like fetching from a remote CAS or reading from LMDB.
   The underlying implementation is Futures-based, and the expensive
   operations are easily parallelisable, it just hasn't been done yet :)

This uses the low-level https://github.com/zargony/rust-fuse which
requires manual inode management.

There is also a higher-level https://github.com/wfraser/fuse-mt which I
discovered after putting this together, but which I'm consciously not
using. Its benefits would be:
 1. It operates on paths rather than inodes. This would be pretty
    convenient, as the inode cache in this implementation is a little
    annoying.
 2. It runs some operations in parallel by not having every function
    take a &mut self. I haven't looked at how parallel it ends up being.
 3. Its readdir API avoids needing to think about pagination. I ended up
    coincidentally re-inventing their API here :)

But its big drawback is that functions are required to return values
(e.g. from read) rather than call callbacks, which means that it's hard
for us to take advantage of our Futurey implementation in Store, as
we'd need to block in every call.

Both projects have talked about wanting to support Futures as a more
first-class concept at some point in a nebulous future, but neither has
a concrete plan for doing so.
Bound end of slice by size of file
OSX appears to not issue these requests, whereas Linux does

@illicitonion illicitonion force-pushed the twitter:dwagnerhall/brfs branch from d9187b1 to 435b929 Apr 24, 2018

illicitonion added some commits Apr 24, 2018

@stuhood
Copy link
Member

stuhood left a comment

Thanks... this is exciting stuff.

I'd like to see a bunch of explicit TODOs mentioning areas of improvement in here (and possibly a ticket or two for things you think we might want to work on soon).

illicitonion added some commits Apr 24, 2018

@stuhood
Copy link
Member

stuhood left a comment

Thanks!

As discussed offline, I'd like to see any tests that have a fuse dependency conditionally skipped if fuse isn't installed (or perhaps if an environment variable isn't set). The danger of course is that we forget to set the variable or install osxfuse, and then the tests bitrot. But it's important for tests to "just work" on someone's machine after checkout.


let _fs = mount(mount_dir.path(), store).expect("Mounting");

unsafe {

This comment has been minimized.

@stuhood

stuhood Apr 24, 2018

Member

There is more under this block than really needs to be... could do just the calls between open and read under the block.

This comment has been minimized.

@stuhood

stuhood Apr 24, 2018

Member

(this is a very nice test by the way.)

@illicitonion illicitonion changed the title WIP: brfs: FUSE filesystem exposing the Store and remote CAS brfs: FUSE filesystem exposing the Store and remote CAS Apr 24, 2018

illicitonion added some commits Apr 24, 2018

Exclude brfs from default workspace packages
This is because not everyone has fuse installed, and we want things to
work out of the box.

To run all tests, including brfs, run `cargo test --all`. CI does this.
To run all tests, excluding brfs, run `cargo test`.
To run just the fs package's tests, run `cargo test -p fs`.

@illicitonion illicitonion merged commit a93b45c into pantsbuild:master Apr 25, 2018

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

@illicitonion illicitonion deleted the twitter:dwagnerhall/brfs branch Apr 27, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment