-
Notifications
You must be signed in to change notification settings - Fork 1.6k
gofer: add pluggable provider interface for custom filesystem backends #12950
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
base: master
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Hmm IMO we move all this into runsc/fsgofer. Since this is very specific to filesystem gofers (as opposed to network gofers). fsgofer and gofer as sibling packages seems a bit odd. You can have it under We have a similar concept in our shim, where we allow shim extensions: pkg/shim/v1/extension/extension.go. Maybe you can call it GoferRegistry? Up to you.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yeah, def fair points. I had On the name, agreed |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,4 @@ | ||
| package( | ||
| default_applicable_licenses = ["//:license"], | ||
| licenses = ["notice"], | ||
| ) |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,31 @@ | ||
| load("//tools:defs.bzl", "go_library", "go_test") | ||
|
|
||
| package( | ||
| default_applicable_licenses = ["//:license"], | ||
| licenses = ["notice"], | ||
| ) | ||
|
|
||
| go_library( | ||
| name = "provider", | ||
| srcs = ["provider.go"], | ||
| visibility = ["//runsc:__subpackages__"], | ||
| deps = [ | ||
| "//pkg/lisafs", | ||
| "//pkg/seccomp", | ||
| "//runsc/specutils", | ||
| "@com_github_opencontainers_runtime_spec//specs-go:go_default_library", | ||
| ], | ||
| ) | ||
|
|
||
| go_test( | ||
| name = "provider_test", | ||
| size = "small", | ||
| srcs = ["provider_test.go"], | ||
| library = ":provider", | ||
| deps = [ | ||
| "//pkg/lisafs", | ||
| "//pkg/seccomp", | ||
| "//runsc/specutils", | ||
| "@com_github_opencontainers_runtime_spec//specs-go:go_default_library", | ||
| ], | ||
| ) |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,60 @@ | ||
| // Copyright 2026 The gVisor Authors. | ||
| // | ||
| // Licensed under the Apache License, Version 2.0 (the "License"); | ||
| // you may not use this file except in compliance with the License. | ||
| // You may obtain a copy of the License at | ||
| // | ||
| // http://www.apache.org/licenses/LICENSE-2.0 | ||
| // | ||
| // Unless required by applicable law or agreed to in writing, software | ||
| // distributed under the License is distributed on an "AS IS" BASIS, | ||
| // WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. | ||
| // See the License for the specific language governing permissions and | ||
| // limitations under the License. | ||
|
|
||
| // Package provider defines an interface for pluggable gofer filesystem | ||
| // backends. The stock fsgofer handles any mount no Provider claims. | ||
| package provider | ||
|
|
||
| import ( | ||
| specs "github.com/opencontainers/runtime-spec/specs-go" | ||
| "gvisor.dev/gvisor/pkg/lisafs" | ||
| "gvisor.dev/gvisor/pkg/seccomp" | ||
| "gvisor.dev/gvisor/runsc/specutils" | ||
| ) | ||
|
|
||
| // Provider is implemented by alternative LisaFS backends. It follows the | ||
| // same pattern as socket.Provider: the first registered Provider whose | ||
| // NewServer returns a non-nil server handles the mount. | ||
| type Provider interface { | ||
| // Name identifies the provider in log messages. | ||
| Name() string | ||
|
|
||
| // NewServer returns a LisaFS server for the given mount, or | ||
| // (nil, nil) if this provider does not handle it. A non-nil error | ||
| // means the provider claims the mount but failed to initialize. | ||
| // | ||
| // mount is nil for the root filesystem (root is not present in | ||
| // spec.Mounts). Per-sandbox config may be read from spec.Annotations. | ||
| // | ||
| // A Provider may return the same *lisafs.Server across calls to | ||
| // multiplex mounts onto one server; the caller deduplicates. | ||
| NewServer(spec *specs.Spec, mount *specs.Mount, mountPath string, conf specutils.GoferMountConf, readonly bool) (*lisafs.Server, error) | ||
|
|
||
| // SeccompRules returns additional rules to merge into the stock | ||
| // gofer's seccomp allowlist. | ||
| SeccompRules() seccomp.SyscallRules | ||
| } | ||
|
|
||
| var registered []Provider | ||
|
|
||
| // Register adds p to the provider list. Must be called during init or | ||
| // early in main, before Registered is iterated. | ||
| func Register(p Provider) { | ||
| registered = append(registered, p) | ||
| } | ||
|
|
||
| // Registered returns all registered providers in registration order. | ||
| func Registered() []Provider { | ||
| return registered | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seems like an anti pattern that we create a new server for each connection. The model we try to have is that we create one gofer server which can handle multiple connections. The server embeds
lisafs.Server. That struct does a bunch of work, including providing concurrency guarantees across RPCs. See usages of the renameMu and also the entire filesystem tree it maintains. Each node has anopsMuwhich synchronizes RPCs on the same file.I think what we should do is only 1 gofer server. And allow lisafs connections to define custom behavior. I think what you want to do is hijack the fsgofer Server
Mountimplementation:gvisor/runsc/fsgofer/lisafs.go
Lines 116 to 117 in 7b19947
Notice that you have the Connection struct there. You also have the full
mountPaththere. If that is not sufficient to make the decision to pivot into your custom gofer, you can add whatever optional fields in lisafs.Connection to help you make that decision at Mount time. This RPC is called only once per connection.In that method, you return your custom implementation of
lisafs.ControlFDImpl. And that should be all. Every RPC on that connection will then be routed to yourlisafs.ControlFDImplimplementation.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for the detailed read.
Yeah, some of this is influenced by that long-lived sentry hardened design :D. The shape in this PR comes from the customer gofer, which implements
lisafs.ServerImpldirectly because it needs values forSupportedMessagesandServerOptsthat diverge from fsgofer's, along with its own per-implementation lifecycle for things like a background uploader and lease heartbeat. For example, it advertisesFlushso the sentry sends us theFlushRPC onclose(2), which is how I learn about writes that bypassed the gofer through the donated host FD, and it runs withServerOpts{}because the custom gofer's storage model cannot honorOpenOnDeletedorSetAttrOnDeleted. Those values are configured per-ServerImplserver-wide today, but the wire protocol already encodes them per-mount inMountResp.SupportedMsandMountResp.MaxMessageSize. That mismatch is what made multi-Serverlook like the cheap fix to me when I shaped this PR, since each backend brings its ownlisafs.Serverand sets those values independently, which is how our fork runs today.Looking at it again with your framing, there are costs I underweighted for sure. Each registered backend ends up as its own
lisafs.Serverlifecycle where the gofer command has to fan outWaitandDestroy, and each backend gets its ownconnWgand rootNodetree on top of the points you mentioned. As I am familiarizing myself more with the codebase, I am also realizing this design does not help augment-style backends (the future example posts and documentation on how to bring in a custom backend), because they would have to bring their own server and lose thecontrolFDLisainfrastructure they would otherwise inherit.Your point about allowing lisafs connections to define custom behavior, and about adding optional fields in
lisafs.Connection, reads to me as opening the door to making those values per-Connection, which would make the single-Servershape work for both augment-style and replace-style backends, IIUC?If that direction sounds right to you, I think the cleanest path is two PRs where this one gets repurposed for the second. The first would be a
pkg/lisafs/change that movesSupportedMessages,MaxMessageSize, andServerOptsfrom per-ServerImplto per-Connection, with 0 behavior change for fsgofer because it would set per-connection what it sets per-server today. I think that should be ok because the wire format already encodes these per-mount and the per-ServerImplmethods are the inconsistency (?)Once that lands, I would rebase this PR on top with the reworked extension mechanism, where there is one
lisafs.Serverand custom backends register aExtensionthat hijacksLisafsServer.Mount, returns aControlFDImplplus the per-connection values. Then any custom augment-style and replace-style extensions/abckends plug in the same way, and the relocate and rename from your other comment land here naturally.What do you think on the sequencing, and is there anything on your end I should be considering before I rework this? Especially the stuff around moving
SupportedMessagesand rest to per Connection?Thanks for helping brainstorm on this too!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, I totally agree with your plan of action. It makes sense. Yeah the
SupportedMessagesandMaxMessageSizeare already set up per-connection basis. Decoupling that from the server makes sense. Looking forward!