In-process exclusion of readers and writers

[mtrmac]

EDIT: See the description in #1346 (comment) instead.

To see what fixing #1332 would be like. This is on top of, and includes, #1345.

The goal is to protect in-process readers against concurrent writes to memory by Load. Let’s discuss the theory of that, and the performance, in #1332 , this is an attempt to draft what that could look like.

(Most of this PR could be merged, and might be useful, without actually changing the locking scheme — but without changing the locking scheme I don’t think we have a reason to do and review all of those code changes).

Right now, locking is open-coded all over the >3000 lines of storage.store:

func (s *store) ListLayerBigData(id string) ([]string, error) {
        lstore, err := s.LayerStore()
        if err != nil {
                return nil, err
        }
        lstores, err := s.ROLayerStores()
        if err != nil {
                return nil, err
        }
        foundLayer := false
        for _, s := range append([]ROLayerStore{lstore}, lstores...) {
                store := s
                store.RLock()
                defer store.Unlock()
                if err := store.ReloadIfChanged(); err != nil {
                        return nil, err
                }
                data, err := store.BigDataNames(id)
                if err == nil {
                        return data, nil
                }
                if store.Exists(id) {
                        foundLayer = true
                }
        }
        if foundLayer {
                return nil, fmt.Errorf("locating big data for layer with ID %q: %w", id, os.ErrNotExist)
        }
        return nil, fmt.Errorf("locating layer with ID %q: %w", id, ErrLayerUnknown)
}

and adding extra code to each of these sites to deal with the in-process lock would be a tedious nightmare with too many opportunities to overlook something..

So, this PR proposes consolidating almost all of these repeated snippets into helpers that call closures: layerReadAccess and layerWriteAccess at the level of a layerStore, and even higher-level (especially for reads) store.writeToLayerStore and readAllLayerStores[T](*store).

The same function can now be:

func (s *store) ListLayerBigData(id string) ([]string, error) {
	foundLayer := false
	if data, err, done := readAllLayerStores(s, func(store roLayerStore, token layerReadToken) ([]string, error, bool) {
		data, err := store.bigDataNames(token, id)
		if err == nil {
			return data, nil, true
		}
		if store.exists(token, id) {
			foundLayer = true
		}
		return nil, nil, false
	}); done {
		return data, err
	}
	if foundLayer {
		return nil, fmt.Errorf("locating big data for layer with ID %q: %w", id, os.ErrNotExist)
	}
	return nil, fmt.Errorf("locating layer with ID %q: %w", id, ErrLayerUnknown)
}

which is < 2/3 of the original size.

At least for the read side, it seems to me that any variant without Go 1.18 generics would need 2 or 3 more tedious lines at every call site. That’s not a show-stopper, but let’s discuss whether we can do it this way. (A notable downside is that the Go 1.18 requirement might not allow us to backport the locking changes to any earlier version … but then with the size of the locking changes, more than almost all of store.go, we might not want to do that anyway?)

After having the closures, we can now quite cheaply add layerReadToken and layerWriteToken types that serve as markers that the layer store’s methods are called with the correct lock held. That’s not nearly as good as a safe language-level support (making the lock-protected fields outright unreachable without holding a lock), but it’s something.

---

This does not actually show handling of the harder cases, like CreateContainer, where we might need to lock several stores, and the like. I’m confident that can be done — we can always do locking manually instead of through the closures; but I didn’t want to spend time on this without having basic consensus on the approach at all.

Also, there seem to be quite a few opportunities for small-scale optimization, typically to check for ErrLayerUnknown from a call instead of calling Exists before/after the main query. After a few false starts, this PR tries very hard not to touch that logic, because that would ballon the scope of this work to a completely unmanageable and impossible to review size.

---

See the individual commit messages for details.

I’d very much appreciate

• opinions on whether this style is reasonable and acceptable overall
• any suggestions to make the code cleaner. (I never thought I’d say this but I found myself thinking that this would probably be significantly easier to do, and even easier to prove correct, in C++, due to tuples, variadic templates and const.)

[mtrmac]

⚠️ Do not merge as is: On top of the upgrade to Go 1.18, this outright disables all of golangci-lint, because it reports so many warnings, many of them at least a bit valid, and fixing them is not the point of this PR.

[mtrmac]

After having the closures, we can now quite cheaply add layerReadToken and layerWriteToken types that serve as markers that the layer store’s methods are called with the correct lock held.

External callers of layerStore could probably be protected easier by being provided a “open with lock” / “operations allowed when locked for reading” / ”operations allowed when locked for writing”, making it impossible to call the wrong methods without a cast.

But that would not protect internal calls within layerStore methods, because regardless of which interface they implement, they would be methods on the underlying object.

Also, the “Token” objects, unlike interface pointers, could in principle carry state to allow diagnosing their misuse (They don’t currently, they are zero-sized structs.). Interfaces pointers are ~stateless.

[mtrmac]

For the record, the empty …Token structs are optimized out and cause no extra machine code to be generated, at least in toy examples.

[mtrmac]

Per the #1332 discussion, to see whether this locking has acceptable performance, essentially all of it has to be built first.

So, as much as I like the tokens and the extra bit of safety, that’s a bit too much churn.

So, I’ll be trying:

• To reduce boilerplate in store as much as possible, at least for the simple cases — to decrease the size of the code and the number of lines that need to be touched by any other changes.
• To replace {,R,Un}Lock on the C/I/L stores, + a manual ReloadIfChanged, with a higher-level {start,stop}{Reading,Writing}, explicitly so that callers don’t explicitly manage the inter-process locks, and instead express intent. That will also allow us to do cleanup in start* for Crash resiliency: deleting incomplete layers doesn’t reliably happen #1136 — even if the intended operation is a “read” the stores will have the freedom to lock for writing (or to loop obtaining and releasing a lock, if necessary). (The token approach is nice when it matches, but many of the important write code paths would require significant restructuring of the code or the locking scheme, and that’s an explicit non-goal of this effort.)
• Both of the above could be an improvement in every situation, at least decreasing code size. But we can then change what the locks do, just by modifying the very few start*/stop*/Load methods.

[mtrmac]

I think this is now broadly representative of where I want to get — but I realize the size is completely unmanageable.

Basically the only part worth looking at right now is 309fcfa (“PROTOTYPE: Turn loadMut into inProcessLock”) — that is a very rough sketch of the RFC part; the rest is just infrastructure.

[mtrmac]

Next steps are to merge #1345 , then I have a set of thematic branches lined up for PRs.

[vrothberg]

This is on my todo list but it's a massive PR that requires some uninterrupted time to review which is rare at the moment.

[mtrmac]

I’m afraid this PR grew as I was learning things… but basically all of it is trivial infrastructure to just centralize the locking decisions (and I’m filing discovered parts that aren’t infrastructure as separate PRs). I’m also filing that infrastructure as a set of smaller PRs.

The thing I’d like most feedback on is #1346 (comment) — both WRT correctness and performance. (And that’s also the thing that is least finished.)

[rhatdan]

@mtrmac needs a rebase.

[mtrmac]

Yes, I think this is ready for review (or possible CRI-O benchmarking).

[rhatdan]

@mrunalp @haircommander PTAL

[rhatdan]

LGTM
@alexlarsson @giuseppe @vrothberg @nalind @flouthoc @umohnani8 PTAL

[vrothberg]

Impressive work. LGTM

[vrothberg]

Absolutely non-blocking and just an idea. The Image struct in libimage has some fields that may be cached. I think you (@mtrmac) suggested to create an embedded struct to highlight the caching nature of these fields (see https://github.com/containers/common/blob/main/libimage/image.go#L38). Cached fields are always accessed via (*Image).cached.field which IMO makes the code much more expressive and comprehensible.

Would it help to add the fields that require locking in a similar embedded struct? (*containerStore).requiresLock.containers?

[mtrmac]

Ooh, that did not occur to me at all, and it’s definitely worth doing.

[mtrmac]

I have prototyped this, and it does seem valuable — but it’s a lot of churn.

E.g. on containers.go, the current PR is +80-14 non-whitespace non-comment changes (including some changes with comments), and moving the fields around would be about +51-45 . Also, in addition to C/I/L stores, the top-level store would benefit from the same updates, but store.go is mostly out of scope of this PR.

Overall, I’d prefer to do it in a separate PR a bit later: it would help maintenance but it should not have immediately user-visible impact, which makes it harder for me to prioritize right now — and the churn so far has already been disrupting Nalin’s work, so coordinating a bit better seems warranted.

I have added the suggestion to #1389; it might well be worth tracking as a higher-priority issue separate from the somewhat desperate #1389.

[vrothberg]

Yes, let's do that in a separate PR. Thank you!

[vrothberg]

s/only users/only use/

[mtrmac]

Fixed, thanks!

[giuseppe]

LGTM (after the comments already pointed out)

[haircommander]

I have tasked @wgahnagl with running some performance tests to see how this works. I can follow up here when we have results

[wgahnagl]

I have tasked @wgahnagl with running some performance tests to see how this works. I can follow up here when we have results

I've run the kubeBurner high-density stresstest against this PR, and it's maxing out at 500 pods, which is normal! 👍

[mtrmac]

@wgahnagl Thank you so much.

[rhatdan]

@mtrmac Could you fix the comments and then we can merge and release a new containers/storage.

[rhatdan]

@mtrmac Could you fix the comments and then we can merge and release a new containers/storage.

[vrothberg]

LGTM

[rhatdan]

LGTM