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

Consumable system features #588

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

LisannaAtHome
Copy link

@LisannaAtHome LisannaAtHome commented Aug 13, 2018

Adds support for consumable system features.

Remote build machines can specify a supported or mandatory feature as
consumable with a "feature:n" notation, where n is the quantity of the
feature that the machine provides. "feature:0" and "feature" are both
interpreted as a non-consumable (infinite) feature.

Derivations can specify how much of a feature they consume with a
similar "feature:n" notation, where n is the quantity of the feature
that the derivation consumes. "feature" is interpreted as "feature:1"
(i.e., consumes one of the feature), and "feature:0" is interpreted as
still requiring the feature but not consuming any of it.

Closes #585

I tested this locally with a variety of different feature combinations and it behaves correctly.

Also, I am not a C++ programmer. A lot of this may very well be written in a non-optimal or non-idiomatic way. If there's a better way to write any of the code I've got here, please let me know.

Design notes are at NixOS/nix#2307, feedback / discussion on the design should happen there.

There's a corresponding implementation that should go into NixOS/nix, but that will probably have to be authored by someone who's more familiar with the codebase than I am.

Adds support for consumable system features.

Remote build machines can specify a supported or mandatory feature as
consumable with a "feature:n" notation, where n is the quantity of the
feature that the machine provides.  "feature:0" and "feature" are both
interpreted as a non-consumable (infinite) feature.

Derivations can specify how much of a feature they consume with a
similar "feature:n" notation, where n is the quantity of the feature
that the derivation consumes.  "feature" is interpreted as "feature:1"
(i.e., consumes one of the feature), and "feature:0" is interpreted as
still requiring the feature but not consuming any of it.

Closes NixOS#585

Signed-off-by: Lisanna Dettwyler <lisanna.dettwyler@gmail.com>
@LisannaAtHome
Copy link
Author

don't know how to add people for reviewing... so just tagging @edolstra @shlevy @rbvermaa

Intent is that this would also make its way into Nix eventually... either when I have time or probably @edolstra has time. We should make sure that the notation especially wouldn't break anything in Nix before the hydra part gets merged (assuming people actually think this is useful enough to merge :^))

Any objections to the hydra part getting merged before the corresponding Nix patch is ready? This shouldn't break anything in terms of backwards compatibility... Nix already doesn't allow ":" characters in system feature names, so AFAIK no one is using it already incidentally for some other meaning.

@Taneb
Copy link
Contributor

Taneb commented Feb 14, 2019

I'd be very happy if this could get reviewed, I've been looking for pretty much exactly like this change for work.

@nlewo
Copy link
Member

nlewo commented Apr 3, 2019

@ledettwy Thanks a lot for this!

I've rebased this PR onto master and also added more status reporting in this branch:
https://github.com/nlewo/hydra/tree/consumable-feature

Thanks to consumable features, my NixOS tests can specify how many CPUs they use. Hydra will schedule them on build slaves. Without this feature, these tests often "timeouts" because there is not enough CPUs to run them.

There is still an issue on the Nix side since the Nix configuration must contain all feature amounts:/ For example, in a NixOS test deviation, I have:

stdenv.mkDerivation {
  ...
  ...
   requiredSystemFeatures = [ "kvm" "nixos-test" "cpu:2" ];
}

The /etc/nix/machines file contains:

localhost x86_64-linux,builtin - 8 1 kvm,nixos-test,big-parallel,cpu:8

And the /etc/nix/config.nix file:

system-features = kvm nixos-test cpu:2

With this configuration, Hydra never schedule more than 4 NixOS tests in parallel, even if the max jobs number is 8.

nlewo added a commit to nlewo/nix that referenced this pull request Apr 4, 2019
Derivation can specify required features. Hydra supports consumable
features [1] by specifying an amount as suffix feature.
Basically, a derivation can specify a `requiredSystemFeature`
`feature:N` meaning this derivation requires `N` units of the feature
`feature`. The Hydra scheduler schedules jobs on Nix slaves by
respecting these constraints. The available amounts of features on
slaves are specified in the `machines` file.

Currently, before building a derivation, Nix checks that all required
features of this derivation are provided (they are specified via the
`system-features` option). Since the derivation needs `feature:N`,
`feature:N` must be specified in the `system-features` option
list. This is not convenient since the value of `N` can vary!

This patch proposes to ignore the suffix `:N` in the
`requiredSystemFeature` attribute of deviations. This allows Nix to
build derivations specifying consumable features by only specifying
the `feature` (and not `feature:N`) in the `system-features` list.

A warning is printed (`Chatty` mode) to inform the user consumable
feature amount is ignored.

    "warning: the amount of the consumable feature 'cpu:4' is ignored in derivation '/nix/store/ndf6b9r8wb6ljci3k1w3jddrsrscgm7d-vm-test-run-tcp-flows.drv'

Note this could break backward compatibility for users that are using
a `requiredFeatures` containing a `:`.

[1] NixOS/hydra#588
nlewo added a commit to cloudwatt/nixpkgs-tungsten that referenced this pull request Apr 5, 2019
The `kvm` feature keyword is repeated by the number of core of each
tests. Hydra then ensures it has enough cores at scheduling time.
See NixOS/hydra#588 for details.
nlewo added a commit to cloudwatt/nixpkgs-tungsten that referenced this pull request Apr 5, 2019
The `kvm` feature keyword is repeated by the number of core of each
tests. Hydra then ensures it has enough cores at scheduling time.
See NixOS/hydra#588 for details.
nlewo added a commit to cloudwatt/nixpkgs-tungsten that referenced this pull request Apr 5, 2019
The `kvm` feature keyword is repeated by the number of core of each
tests. Hydra then ensures it has enough cores at scheduling time.
See NixOS/hydra#588 for details.
@nlewo
Copy link
Member

nlewo commented Apr 5, 2019

FYI, I finally added the support to specify consumables feature by repeating the feature keyword in d6d31ac. For instance requireSystemFeatures = [ "kvm" "kvm" ] is equivalent to requireSystemFeatures = [ "kvm:2" ].
The former is ignored by Nix so you can still build such kind of derivations locally. To build derivation with kvm:2, we would need to add the system-feature kvm:2 to your local Nix configuration (since commit NixOS/nix@1e7b8de).

To have a correct support of consumable features in Hydra, I think we have to patch Nix to either support consumable features, or ignore them... because right now, it's a bit hacky!

eonpatapon pushed a commit to cloudwatt/nixpkgs-tungsten that referenced this pull request Apr 5, 2019
The `kvm` feature keyword is repeated by the number of core of each
tests. Hydra then ensures it has enough cores at scheduling time.
See NixOS/hydra#588 for details.
@PkmX
Copy link

PkmX commented Nov 25, 2019

+1 for this feature. We also need this for controlling scheduling tests onto physical hardware that only allow one job at a time.

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.

Consumable system features
5 participants