Refactor `Store` hierarchy with a new `IndirectRootStore` interface #8243

Ericson2314 · 2023-04-20T03:26:20Z

Motivation

This is preparatory work needed for the "mounted ssh store" feature implemented in Mounted SSH Store #7912. That feature is very useful for Nix with NFS, as in common in organization settings.
Even without the above feature, I think it bring greater clarity to the relationship between the existing stores, and between addIndirectRoot and addPermRoot.

As an example of the above:
- Without this, SSHStore had a remote-side addIndirectRoot implementation, but that makes no no sense. SSHStore does not expose the ambient file system, and so one should run the risk of accidentally making symlinks on the remote side that make no sense.
With this, SSHStore does not have an addIndirectRoot method at all.

See the extensive API docs for more details.

Context

#7912 will be rebased on top once this is merged. It will then be much smaller.

The commits here are separated for easier review.

#5729 is somewhat related, though in this case we are cleaning up the subclasses not Store itself.

(Note can review commit-by-commit.)

Checklist for maintainers

Maintainers: tick if completed or explain if not relevant

agreed on idea
agreed on implementation strategy
tests, as appropriate
- functional tests - tests/**.sh
- unit tests - src/*/tests
- integration tests - tests/nixos/*
documentation in the manual
documentation in the internal API docs
code and comments are self-explanatory
commit message explains why the change was made
new feature or incompatible change: updated release notes

Priorities

Add 👍 to pull requests you find important.

fricklerhandwerk · 2023-05-15T11:04:26Z

Discussed in Nix team meeting 2023-05-12:

@Ericson2314:
- Preperatory refactor for the "mounted SSH store" concept.
- Original PR, and this step, already discussed in prior meeting; agreed on this tentative implementation strategy (behind experimental feature)
- Have split this PR because it is ready to go (whereas the rest needs tests etc., since it is new feature)
- Want to keep things moving so outside contributor feels progress (this is a yak shave they are stuck waiting for)
assigned @thufschmitt for review

nixos-discourse · 2023-05-15T13:46:38Z

This pull request has been mentioned on NixOS Discourse. There might be relevant details there:

https://discourse.nixos.org/t/2023-05-12-nix-team-meeting-minutes-54/28197/1

thufschmitt

I'm a bit “meh” on that change. I see where it's coming from, but at the same time, the API and implementation of IndirectRootStore::addPermRoot pretty much force any instance of it to be baked by a locally mounted filesystem. This means that – unless I'm overlooking something – any instance of IndirectRootStore could be made an instance of LocalFSStore (and benefit from some performance benefits by bypassing the daemon where it can be).

(Btw, 781ead0 seems unrelated to that PR except that it is also part of #7912)

thufschmitt · 2023-06-02T08:36:24Z

src/libstore/gc-store.hh

+ *  - The base Store class has Store::addTempRoot() because for a store
+ *    that doesn't support garbage collection at all, a temporary GC root
+ *    is safely implementable as no-op.


This is debatable when the same store can be accessed in different ways where one can be gc-ed and the other can't (For instance I can't add a root over ssh, but that doesn't prevent someone from gc-ing that store locally)

Yeah that is a good point. I guess I will reward this comment as tech debt TODO instead :).

Added below.

thufschmitt · 2023-06-02T08:38:14Z

src/libstore/gc-store.hh

+ *  - The derived LocalFSStore class has LocalFSStore::addPermRoot,
+ *    which is not part of this class because it relies on the notion of
+ *    an ambient file system. There are stores (`ssh-ng://`, for one),
+ *    that *do* support garbage collection but *don't* expose any file
+ *    system, and LocalFSStore::addPermRoot thus does not make sense for
+ *    them.


That's not true any more since addPermRoot is part of IndirectRootStore, right?

No the abstract method is still in LocalFSStore. IndirectRootStore merely has an implementation. The next PR ads another implementation.

src/libstore/indirect-root-store.hh

thufschmitt · 2023-06-02T09:04:53Z

src/libstore/indirect-root-store.hh

+ * possible implementations many of which make no sense. Having this and
+ * that invariant enforced cuts down that space.
+ */
+struct IndirectRootStore : public virtual LocalFSStore


I think that name is not great. The most important bit here isn't that the store can register indirect roots (any ssh-ng could in theory), but that it can register permanent roots (although addPermRoot is implemented and final here, it only makes sense for stores that have a local filesystem baking)

The important bit is in fact the indirect roots.

it only makes sense for stores that have a local filesystem baking

It is a subclass of LocalFSStore, so unless you meant something like "non ssh/NFS" by "local" that is in fact always the case.

LocalFSStore defines addPermRoot. So the point of this class isn't to define that interfaec, but to merely implement it. The point is the indirect root implementation.

Ericson2314 · 2023-06-06T21:47:29Z

@thufschmitt I think you got the inheritance backwards.

Any instance of IndirectRootStore could be made an instance of LocalFSStore (and benefit from some performance benefits by bypassing the daemon where it can be).

Any instance must be that, because IndirectRootStore is the addPermRoot implementation while LocalFSStore just defines the addPermRoot interface.

Ericson2314 · 2023-06-06T22:47:48Z

(Btw, 781ead0 seems unrelated to that PR except that it is also part of #7912)

It is needed so that UdsRemoteStore::addIndirectRoot can be implemented in a separate file.

thufschmitt · 2023-06-08T05:01:08Z

I think you got the inheritance backwards.

Indeed, I missed the “IndirectRootStore inherits from LocalFSSTore” bit. But that still doesn't seem right to me: It's not clear why we need an extra class for this (we could simply have this be part of the LocalFSStore interface). Is there any store that could have a reason to be made a LocalFSStore but not an IndirectRootStore?

Also,

SSHStore had a remote-side addIndirectRoot implementation, but that makes no no sense

Feels a bit weak as a justification to me. This feels less of an issue to me than having to leak the ConnectionHandles from the remote store implementation just for the sake of deferring the implementation of addIndirectRoot

Ericson2314 · 2023-06-08T14:50:10Z

Is there any store that could have a reason to be made a LocalFSStore but not an IndirectRootStore?

Yes, the MountedSSHStore in the folllow-up PR.

The client only does addPermRoot, and the server (or intermediary if another daemon) does the rest, in order to race conditions / the need for extra synchronization.

The table, which I save for that follow-up PR, is supposed to answer this question (given these leaf classes, we have these combinations to support with intermediate classes...).

This feels less of an issue to me than having to leak the ConnectionHandles from the remote store implementation just for the sake of deferring the implementation of addIndirectRoot

I made the type protected in the subclass so it doesn't actually leak. I would be happy to put it in a separate header too so things that don't care / can't use it don't even see it.

tomberek · 2023-07-24T12:33:52Z

Is re-use of https://github.com/NixOS/nix/pull/8243/files#diff-13403e7648a1890cf8bfb79fd605d19d11f6fd6809778cfdd70e9099746fda03R222

... looks like yes, the expected use would be via getConnection:

This is because `StoreConfig` also uses it.

tomberek

Comments help describe the structure and motivation. Implementation makes sense, builds, and allows for the planned future changes in #7912 (and any additional alternative store implementations, also can be useful for layered stores)

Will need to do subclass-specific implementations in the next commit. This isn't because there will be multiple variations of the daemon protocol (whew!) but because different clients pick and choose different parts to use.

See the API doc comments for details.

nixos-discourse · 2023-07-31T08:43:47Z

This pull request has been mentioned on NixOS Discourse. There might be relevant details there:

https://discourse.nixos.org/t/2023-07-24-nix-team-meeting-minutes-74/31116/1

Ericson2314 requested a review from thufschmitt as a code owner April 20, 2023 03:26

github-actions bot added the store Issues and pull requests concerning the Nix store label Apr 20, 2023

Ericson2314 mentioned this pull request Apr 20, 2023

Mounted SSH Store #7912

Merged

7 tasks

mupdt approved these changes Apr 20, 2023

View reviewed changes

thufschmitt self-assigned this May 12, 2023

thufschmitt reviewed Jun 2, 2023

View reviewed changes

Ericson2314 force-pushed the indirect-root-store branch 2 times, most recently from fa61b44 to 00edcac Compare June 6, 2023 22:45

thufschmitt assigned tomberek and unassigned thufschmitt Jul 24, 2023

Move Store::Params typedef to StoreConfig::Params

0a30b07

This is because `StoreConfig` also uses it.

tomberek approved these changes Jul 24, 2023

View reviewed changes

Ericson2314 added 2 commits July 24, 2023 09:17

Make RemoteStore::ConnectionHandle part of class and expose

13269ba

Will need to do subclass-specific implementations in the next commit. This isn't because there will be multiple variations of the daemon protocol (whew!) but because different clients pick and choose different parts to use.

Clean up store hierarchy with IndirectRootStore

60d8dd7

See the API doc comments for details.

Ericson2314 force-pushed the indirect-root-store branch from 00edcac to 60d8dd7 Compare July 24, 2023 13:20

Ericson2314 enabled auto-merge July 24, 2023 13:23

Ericson2314 merged commit 40c77f3 into NixOS:master Jul 24, 2023

Ericson2314 deleted the indirect-root-store branch July 24, 2023 14:08

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor `Store` hierarchy with a new `IndirectRootStore` interface #8243

Refactor `Store` hierarchy with a new `IndirectRootStore` interface #8243

Ericson2314 commented Apr 20, 2023 •

edited

Loading

fricklerhandwerk commented May 15, 2023 •

edited

Loading

nixos-discourse commented May 15, 2023

thufschmitt left a comment

thufschmitt Jun 2, 2023

Ericson2314 Jun 6, 2023

Ericson2314 Jun 6, 2023

thufschmitt Jun 2, 2023

Ericson2314 Jun 6, 2023

thufschmitt Jun 2, 2023

Ericson2314 Jun 6, 2023

Ericson2314 commented Jun 6, 2023

Ericson2314 commented Jun 6, 2023

thufschmitt commented Jun 8, 2023

Ericson2314 commented Jun 8, 2023 •

edited

Loading

tomberek commented Jul 24, 2023

tomberek left a comment

nixos-discourse commented Jul 31, 2023

Refactor Store hierarchy with a new IndirectRootStore interface #8243

Refactor Store hierarchy with a new IndirectRootStore interface #8243

Conversation

Ericson2314 commented Apr 20, 2023 • edited Loading

Motivation

Context

Checklist for maintainers

Priorities

fricklerhandwerk commented May 15, 2023 • edited Loading

nixos-discourse commented May 15, 2023

thufschmitt left a comment

Choose a reason for hiding this comment

thufschmitt Jun 2, 2023

Choose a reason for hiding this comment

Ericson2314 Jun 6, 2023

Choose a reason for hiding this comment

Ericson2314 Jun 6, 2023

Choose a reason for hiding this comment

thufschmitt Jun 2, 2023

Choose a reason for hiding this comment

Ericson2314 Jun 6, 2023

Choose a reason for hiding this comment

thufschmitt Jun 2, 2023

Choose a reason for hiding this comment

Ericson2314 Jun 6, 2023

Choose a reason for hiding this comment

Ericson2314 commented Jun 6, 2023

Ericson2314 commented Jun 6, 2023

thufschmitt commented Jun 8, 2023

Ericson2314 commented Jun 8, 2023 • edited Loading

tomberek commented Jul 24, 2023

tomberek left a comment

Choose a reason for hiding this comment

nixos-discourse commented Jul 31, 2023

Refactor `Store` hierarchy with a new `IndirectRootStore` interface #8243

Refactor `Store` hierarchy with a new `IndirectRootStore` interface #8243

Ericson2314 commented Apr 20, 2023 •

edited

Loading

fricklerhandwerk commented May 15, 2023 •

edited

Loading

Ericson2314 commented Jun 8, 2023 •

edited

Loading