Skip to content
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

Factor our connection code for worker proto like serve proto #10782

Merged
merged 1 commit into from
Jun 3, 2024

Conversation

Ericson2314
Copy link
Member

@Ericson2314 Ericson2314 commented May 27, 2024

Motivation

This increases test coverage, and gets the worker protocol ready to be used by Hydra.

Why don't we just try to use the store interface in Hydra? Well, the problem is that the store interface works on connection pools, with each operation getting potentially a different connection, but the way temp roots work requires that we keep one logical "transaction" (temp root session) using the same connection.

The longer-term solution probably is making connections themselves implement the store interface, but that is something that builds on this, so I feel OK that this is not churn in the wrong direction.

Context

Fixes #9584

Progress on NixOS/hydra#688

Priorities and Process

Add 👍 to pull requests you find important.

The Nix maintainer team uses a GitHub project board to schedule and track reviews.

This increases test coverage, and gets the worker protocol ready to be
used by Hydra.

Why don't we just try to use the store interface in Hydra? Well, the
problem is that the store interface works on connection pools, with each
opreation getting potentially a different connection, but the way temp
roots work requires that we keep one logical "transaction" (temp root
session) using the same connection.

The longer-term solution probably is making connections themselves
implement the store interface, but that is something that builds on
this, so I feel OK that this is not churn in the wrong direction.

Fixes NixOS#9584
@github-actions github-actions bot added the store Issues and pull requests concerning the Nix store label May 27, 2024
@Ericson2314 Ericson2314 added the protocol Things involving the daemon protocol & compatibility issues label May 27, 2024
@fricklerhandwerk fricklerhandwerk added the with-tests Issues related to testing. PRs with tests have some priority label May 28, 2024
@edolstra
Copy link
Member

Why don't we just try to use the store interface in Hydra? Well, the problem is that the store interface works on connection pools, with each operation getting potentially a different connection, but the way temp roots work requires that we keep one logical "transaction" (temp root session) using the same connection.

Not sure I follow. IIRC NixOS/hydra#1200 performs the copying and building in one process (hydra-build-step) which should have only one connection to the remote store.

I guess there is an architectural issue with temp roots, remote stores and connection pooling, but that's not really Hydra-specific.

Regardless, 👍 on factoring out the protocol code.

@Ericson2314
Copy link
Member Author

@edolstra Yeah I mean short of your PR, with the current architecture. It is true with your PR this sort of stuff is not needed, but per

Regardless, 👍 on factoring out the protocol code.

I think it is a fine thing to do anyways, or at least neutral enough that we shouldn't worry about doing it and then it becoming unnecessary.

@edolstra edolstra merged commit d16fcae into NixOS:master Jun 3, 2024
12 checks passed
@Ericson2314 Ericson2314 deleted the both-connections branch June 3, 2024 13:10
@nixos-discourse
Copy link

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

https://discourse.nixos.org/t/2024-06-03-nix-team-meeting-minutes-149/46582/1

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
protocol Things involving the daemon protocol & compatibility issues store Issues and pull requests concerning the Nix store with-tests Issues related to testing. PRs with tests have some priority
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Protocol version negotiation currently doesn't result in agreement
4 participants