Skip to content

Support use as a reverse proxy#57

Open
bacchanalia wants to merge 5 commits intomainfrom
zz/multi-store
Open

Support use as a reverse proxy#57
bacchanalia wants to merge 5 commits intomainfrom
zz/multi-store

Conversation

@bacchanalia
Copy link
Collaborator

nix-serve-ng` can connect to other substituters with the --store option.

URL is a standard nix store url see: nix help-stores for details.

--store=URL can be specified more than once. Stores will be queried order of the priority query parameter (lowest to highest). Sets of stores with the same priority will be queried concurrently.

--retry-timeout=SECONDS specifies how long to wait after a store operation fails before reenabling the store. The default is the value of the nix setting connect-timeout.

--cores=N specifies the maximum number of cores. The default value is the size of the largest group of stores with the same priority.

nix-serve-ng` can connect to other substituters with the --store option.

URL is a standard nix store url see: `nix help-stores` for details.

`--store=URL` can be specified more than once. Stores will be queried
order of the `priority` query parameter (lowest to highest). Sets of
stores with the same priority will be queried concurrently.

`--retry-timeout=SECONDS` specifies how long to wait after a store
operation fails before reenabling the store. The default is the value of
the nix setting `connect-timeout`.

`--cores=N` specifies the maximum number of cores. The default value is
the size of the largest group of stores with the same priority.
success <- act x
if success
then loop (n-1)
else retry x >> loop n

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If this fails, it will just recurse infinitely. There's no cap, but maybe that's okay for our purposes? But there's no logging here so none of retrying's behaviour is externally observable.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Remote stores may be down indefinitely, so we do want to look forever. We probably do still want to long on log levels other than verbose though. (The logging i taken care of by resetStore/disableStore).

-- TODO: This could be split into a path query / dump like we do for nars
-- so we can race the queries, but this isn't a hot path operation
-- so there is less of a benefit.
maybeBytes <- liftIO $ findJustM (flip Nix.dumpLog suffix) (Nix.storeList opts.stores)

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think there is a potential issue with the error handling of dumpLog.

withTimeout can return Nothing in 3 cases:

  • store is PreInit
  • store is Disabled
  • a CppException was thrown

dumpLog treats all Nothing from withTimeout as a hard error and throws CppException:

dumpLog :: Store -> ByteString -> IO (Maybe ByteString)
dumpLog store baseName = do
  result <- withTimeout store \url -> NixFFI.dumpLog url baseName
  maybe (Exception.throwIO CppException) pure result

so instead of returning Nothing, this breaks the findJustM here by throwing CppException for every single store inaccessibility state.

I think withTimeout may need to return more information so that dumpLog can determine whether or not to actually throw a CppException.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You're right it shouldn't be rethrowing here. I think we can always just soft fail though.

src/Nix.hs Outdated
dumpLog :: Store -> ByteString -> IO (Maybe ByteString)
dumpLog store baseName = do
result <- withTimeout store \url -> NixFFI.dumpLog url baseName
maybe (Exception.throwIO CppException) pure result

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

src/Nix.hs Outdated
dumpPath :: Store -> ByteString -> (Builder -> IO ()) -> IO ()
dumpPath store hashPath builderCallback = do
result <- withTimeout store \url -> NixFFI.dumpPath url hashPath builderCallback
maybe (Exception.throwIO CppException) pure result

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this error handling may cause problems in the future, see https://github.com/aristanetworks/nix-serve-ng/pull/57/changes#r2874318640

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The rethrow is correct for dumpPath, because it should be a hard failure to fail to dump a path from a store where it was already found.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah right, because dumpPath is only called after an Enabled store is found. CppException may be the wrong thing to convey that though.

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants