Add a generic abstraction for filesystem based stores by apostasie · Pull Request #3364 · containerd/nerdctl

apostasie · 2024-08-26T05:53:21Z

Although our codebase makes use of flock when we manipulate the filesystem (normally...) , we have had a relatively significant number of issues in the past with our various "file stores". They typically fall in the following categories:

unprotected concurrent access to the same resource
filesystem details leaking upward into higher level abstractions where they are manipulated unsafely
lock that does not really lock
lock that should lock over a larger section of code
sequential (individually locked) operations being done assuming that data cannot change in-between them
partial writes to the filesystem

Issues arising from these are generally heinous to debug, as they usually manifest in a flaky way (as they may only happen in high-concurrency situations / under pressure), or tend to bubble-up in different places making seemingly unrelated operations fail in cryptic ways.

Truth is, it is not 100% trivial to achieve a concurrency-safe and atomic filesystem storage system, even a very simple one like what we do - operating system crash or restarts, or generally interrupted writes, for example, are often overlooked as something that has to be thought about, to boot.

While we have fixed a number of these issues, there is clearly more lingering, and we are bound to produce even more in the future short of having a stronger basis.
I do not think it is fair to assume that everyone will be very mindful and intentional with their locking, or that the filesystem is a good enough, "safe" abstraction...

Furthermore, and even when done right, these things are inherently repetitive (mutex / filesystem lock / path sanitization / atomic write) and we now have enough of these "filestores" to justify having a shared base:

namestore
volumestore
lifecycledata
hostsstore

This PR first commit does introduce an interface and a reference (filesystem) implementation of a reusable Store component that every "store" can leverage and that provides:

concurrency safety (thread safe and safe across concurrent invocations of the binary)
atomic operations (write will never leave incomplete data in a file)
path components validation, per-platform
a small set of simple methods that should cover most use-cases
completely sealed underlying implementation that does not leak
proper, detailed errors
some level of locking enforcement (eg: will error if there is no lock)
tests

Structs that leverage it are thus freed from having to deal with mutex / filelocks, or underlying implementations (eg: filesystem). They can rely on a simple API that is responsible for just that, and can focus solely on the abstraction they have to provide for their specific data.

Hopefully, if things go our way, we should never see again a bug report about hosed json files / broken systems on restart, or flakyness involving stores...

The second commit is a mere cleanup of the (useless) mock for the volume store

The third commit is a refactor of our various stores to leverage that:

all four stores have been reviewed and their syntax harmonized
better erroring
code duplication removal
all explicit locking mechanisms have been removed (to simplify the user life) (with the exception of the VolumeStore) and all stores now expose methods tailored for their use cases that lock on their own
all use cases have been reviewed to make sure nothing is racy (exceptions with Composer, obviously)
ensuring locking is done at the namespace level, or lower

Incidentally, the following known issues are getting fixed or partially fixed:

Finally, I do appreciate this is a far reaching PR, as it is touching code in the critical path of basically everything.
To a large extent, though, this is mere refactoring and literally no novelty.

apostasie · 2024-08-27T04:10:23Z

 		if err != nil {
 			log.L.Warn(err)
 		} else {
-			volEnts, err := volStore.List(false)


List() is expensive.
Replaced with the much lighter Count.

apostasie · 2024-08-27T04:10:46Z

-		return false, volGetErr
-	}
+	// FIXME: this is racy. See note in up_volume.go
+	options.VolumeExists = volStore.Exists


There is now a Exists method, much cheaper than Get

apostasie · 2024-08-27T04:12:37Z

 	if err != nil {
 		return err
 	}
-	err = volStore.Lock()


Replacing all of that with a volStore.Prune(func) that does locking.

apostasie · 2024-08-27T04:15:56Z

@@ -1,41 +0,0 @@
-/*


Moved inside the store.
These things are restrictions linked to the filesystem implementation, and not conceptually to restrictions we necessarily want to apply to identifiers.
Put otherwise: they are orthogonal concerns.

apostasie · 2024-08-27T04:16:29Z

@@ -0,0 +1,37 @@
+/*


This replace the whole uber go mock thing for mountutil.

apostasie · 2024-08-28T22:16:55Z

 	copyFileContent("/etc/resolv.conf", resolvConfPath)

-	etcHostsPath, err := hostsstore.AllocHostsFile(dataStore, m.globalOptions.Namespace, containerID)
+	hs, err := hostsstore.New(dataStore, m.globalOptions.Namespace)


Write only once, instead of allocating first, then writing.

apostasie · 2024-08-28T22:18:00Z

 		return fmt.Errorf("identifier %q must match pattern %q: %w", s, AllowedIdentfierChars, errdefs.ErrInvalidArgument)
 	}

-	if err := validatePlatformSpecific(s); err != nil {


Platform validation of identifiers is a filesystem concern.
These are moved into the store.Store, as they are orthogonal concerns.

apostasie · 2024-09-01T17:44:20Z

Rebase broke some tests.
Investigating.

apostasie · 2024-09-01T20:28:15Z

rebased
removed commented out extra stuff
changed volStore.Remove semantics as discussed above to allow for locking of a function provided as a generator

CI is green.

apostasie · 2024-09-02T17:23:28Z

Rebased. Pending green CI.

AkihiroSuda · 2024-09-03T01:41:23Z

GroupRename() seems added (and commented out) in the first commit and then removed in the third commit.
Probably this should be just removed from the first commit

apostasie · 2024-09-03T03:02:24Z

GroupRename() seems added (and commented out) in the first commit and then removed in the third commit. Probably this should be just removed from the first commit

My bad. Thanks for catching it.
Last push corrects it.

(CI failing on Hub being 500)

fahedouch · 2024-09-03T17:50:42Z

big UP for this PR 👍 , I have two points to raise here:

please avoid huge PR, it is really hard to review
we might need a document for this abstraction (e.g. big picture abstraction design)

apostasie · 2024-09-03T18:06:53Z

big UP for this PR 👍 , I have two points to raise here:

please avoid huge PR, it is really hard to review

we might need a document for this abstraction (e.g. big picture abstraction design)

Thanks a lot @fahedouch
Really appreciate the sharp eye review
On my phone now but will address your comments momentarily

ack on splitting in more commits
Will definitely do in upcoming work

Signed-off-by: apostasie <spam_blackhole@farcloser.world>

AkihiroSuda

Thanks

fahedouch

Thanks

apostasie marked this pull request as draft August 26, 2024 05:54

apostasie force-pushed the hardening-filestore branch 14 times, most recently from d500496 to e7bf627 Compare August 27, 2024 04:00

apostasie commented Aug 27, 2024

View reviewed changes

Comment thread pkg/cmd/container/remove.go

apostasie commented Aug 27, 2024

View reviewed changes

apostasie force-pushed the hardening-filestore branch 9 times, most recently from 03d3241 to 1a7f3d0 Compare August 28, 2024 00:27

apostasie commented Aug 28, 2024

View reviewed changes

AkihiroSuda reviewed Aug 29, 2024

View reviewed changes

Comment thread cmd/nerdctl/volume_remove_linux_test.go Outdated

apostasie commented Aug 29, 2024

View reviewed changes

Comment thread pkg/cmd/volume/rm.go

AkihiroSuda requested a review from fahedouch August 30, 2024 08:49

apostasie force-pushed the hardening-filestore branch from a83aafc to 0dce441 Compare September 1, 2024 17:11

apostasie force-pushed the hardening-filestore branch 2 times, most recently from 073c2cd to 0d4485f Compare September 1, 2024 19:17

apostasie force-pushed the hardening-filestore branch 2 times, most recently from 09a7aa9 to 60c68b9 Compare September 2, 2024 17:23

AkihiroSuda added this to the v2.0.0 milestone Sep 3, 2024

apostasie force-pushed the hardening-filestore branch from 60c68b9 to c340e25 Compare September 3, 2024 02:59

apostasie requested a review from AkihiroSuda September 3, 2024 03:02

fahedouch reviewed Sep 3, 2024

View reviewed changes

Comment thread pkg/cmd/container/remove.go

Comment thread pkg/cmd/volume/prune.go

Comment thread pkg/store/store.go Outdated

apostasie added 3 commits September 3, 2024 11:40

Add generic filestore

92188e1

Signed-off-by: apostasie <spam_blackhole@farcloser.world>

Remove mock for mountutil tests

c9756a9

Signed-off-by: apostasie <spam_blackhole@farcloser.world>

Refactor filestores using store.Store

bf89c08

Signed-off-by: apostasie <spam_blackhole@farcloser.world>

apostasie force-pushed the hardening-filestore branch from c340e25 to bf89c08 Compare September 3, 2024 18:41

apostasie mentioned this pull request Sep 3, 2024

Draft design document for store #3405

Merged

AkihiroSuda approved these changes Sep 4, 2024

View reviewed changes

AkihiroSuda requested a review from fahedouch September 4, 2024 15:17

fahedouch approved these changes Sep 5, 2024

View reviewed changes

fahedouch merged commit 678393e into containerd:main Sep 5, 2024

Conversation

apostasie commented Aug 26, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

apostasie Aug 27, 2024

Choose a reason for hiding this comment

Uh oh!

apostasie Aug 27, 2024

Choose a reason for hiding this comment

Uh oh!

Uh oh!

apostasie Aug 27, 2024

Choose a reason for hiding this comment

Uh oh!

apostasie Aug 27, 2024

Choose a reason for hiding this comment

Uh oh!

apostasie Aug 27, 2024

Choose a reason for hiding this comment

Uh oh!

apostasie Aug 28, 2024

Choose a reason for hiding this comment

Uh oh!

apostasie Aug 28, 2024

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

apostasie commented Sep 1, 2024

Uh oh!

apostasie commented Sep 1, 2024

Uh oh!

apostasie commented Sep 2, 2024

Uh oh!

AkihiroSuda commented Sep 3, 2024

Uh oh!

apostasie commented Sep 3, 2024

Uh oh!

Uh oh!

Uh oh!

Uh oh!

fahedouch commented Sep 3, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

apostasie commented Sep 3, 2024

Uh oh!

AkihiroSuda left a comment

Choose a reason for hiding this comment

Uh oh!

fahedouch left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

apostasie commented Aug 26, 2024 •

edited

Loading

fahedouch commented Sep 3, 2024 •

edited

Loading