-
-
Notifications
You must be signed in to change notification settings - Fork 364
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
eval-machine-info.nix: ported to modules #1508
base: master
Are you sure you want to change the base?
Conversation
nix/eval-machine-info.nix
Outdated
stage1Eval = lib.evalModules { | ||
specialArgs = args; | ||
modules = [ | ||
(lib.mkRemovedOptionModule [ "require" ] "Use the imports option instead.") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@roberth you mentioned that the module system supports requires
,how?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
require
has the same meaning as imports
: https://github.com/NixOS/nixpkgs/blob/d81f048ef4ce360ba2d11d3d7c94398be134b380/lib/modules.nix#L368
nix/eval-machine-info.nix
Outdated
nixpkgs1 = <nixpkgs>; # this will be replaced on install by nixops' nixpkgs input | ||
lib = import "${nixpkgs1}/lib"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should use this exclusively for getting the nixpkgs
option value, and then use its lib for the rest of the evaluation.
nixpkgs1 = <nixpkgs>; # this will be replaced on install by nixops' nixpkgs input | |
lib = import "${nixpkgs1}/lib"; | |
libBootNixpkgs = <nixpkgs>; # this will be replaced on install by nixops' nixpkgs input | |
libBootLib = import "${nixpkgs1}/lib"; |
nix/eval-machine-info.nix
Outdated
}; | ||
defaults = mkOption { | ||
type = with types; | ||
# TODO: use types.raw once this PR is merged: https://github.com/NixOS/nixpkgs/pull/132448 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we'll want a type that doesn't prohibit merging, but returns a list or a mkMerge
instead.
This option is actually somewhat redundant, as you can type-merge modules into all nodes by redeclaring options.nodes
, but defaults
is expected and convenient.
(edited to say type-merge)
nix/eval-machine-info.nix
Outdated
in | ||
rec { | ||
|
||
network = (evalBoot.extendModules { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This extendModules
comes from libBoot
, so this should use lib.evalModules
from scratch instead.
(Sorry if I'm repetitive; made some commit comments by accident earlier)
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
Ok I've tried and failed to build a network, it goes in infinite recursion while evaluating |
It still goes in infinite recursion, I need to know what nixops expects exactly from this file and what can be safely removed. |
Maybe the |
I don't expect this to be documented anywhere. |
I don't need to be fully documented,only to tell me what the nixops cli and plugins take from the |
Tried with |
Ok it was an error on my part, I forgot |
Ok now the error I get is
Which I don't know where even to begin |
Just add a It's some silly ux :/ |
Ok latest error is definetly on the python side
|
This is a known issue. #1490 At this time, |
|
Where it would ask for a pkgs param? |
The error seems to originate from the "physical" expression that's generated by nixops. You can see that expression with |
|
#1490 strikes again☹ |
Ok why is |
I've made some fixes so that a nixops_aws deployment with resources now works. |
This seems a bit too radical to me. Besides the consistency and the risk, I feel like So I'm a bit hesitant, but I can be convinced, maybe? |
On 20/03/23 08:01, Robert Hensing ***@***.***> wrote:
`nodes` used to be consistent with
the NixOS tests
`nodes` is the only options nixops and the nixos test have in commons, I do not even belive that the nixos tests are implemented with the modules framework so it would only cause further confusions if we were to make the 2 more similar.
and I believe the distinction between nodes and machine
resources may have been greater in the past, and completely merging the
two may be a very grave mistake.
On the contrary i joined the 2 for a reason of Single Source of Truth: the users are asked to set the nodes options yet the nixops script goes to look into a completely different option. It's not hard to imagine a bug where the 2 begin to diverge.
Worse such bug would be hard to even detect,let alone debug, as the user begin to look in the complete wrong position and the nix language support is extremely poor.
There's no easy way out of such a decision.
`mkRenamedModule` and `mkRemovedModule` covers a multitude of sins
Besides the consistency and the risk, I feel like `nodes` is actually an
ingredient of a good solution, which is the idea that `resources` are
somewhat of a low level interface to the cloud, whereas almost all the
other options can be more high level.
I would go the opposite direction, with `machines` being merely one kind among many of cloud resources, and even being able to configure a network that has no machines in it.
It could even intrude into disnix's role and allow to configure,for example, a network of Java servlets on something like a Servlet as a Service or something.
NixOS itself is actually one of
these more high level options. For instance, it can select between
multiple backends.
When we support a generic resource backend like terraform, cloud support
would be largely be implemented in the module system,
Even now the we can access `resources.machines.*.system.build.toplevel` from inside the net module. It is still possible to implement the cloud supposrt from the module itself.
and these NixOS
configurations won't be represented by a single resource that was
specifically written to be this all-encompassing NixOS + VM + SSH +
whatnot python object.
So we can still do all kind of object shuffling with the right `networkExpr` and they can still have all kind of backend-dependent magic that can be referred from the nodes themselves.
Hell we could even steal from `deploy.rs` and have `resource.upgradeScripts` all from the module system.
So I'm a bit hesitant, but I can be convinced, maybe?
Remember that I have little experience with colud systems, let alone nixops specifically, so if any of the above sound wrong don't hesitate to call me out on it ☺
|
27ee1fc
to
4ba945b
Compare
…resources.machines
I'll share a response with my thoughts so far. Haven't been able to look into it more deeply yet.
They are.
If we have option definitions that assign the right thing, I don't see how divergence could reasonably occur, except perhaps through I would prefer to keep the delta small until we can say something definitive about this What worries me is a problem that we have in NixOps evaluation, where it could take an arbitrary number of Anyway, I'll do a bit more research and some testing before I actually make a decision. |
Doesn't the module system take care of that? Just evaluate once and all the stuff needed is in Or maybe there is some black magic in the python code that I am missing.
So what are
Would this allow nixops to interface for eg with Kubernetes in a remote future? Making nixops a mere terraform front-end seem like a kind of waste even as nice as both nixops and terraform are. |
Fixes #1486
Done:
requires
an error.nodes
option.network.nodesExtraArgs
To do:
resources
optionsnetwork
optionsassertions
andwarnings