DGNum/colmena
12
0
Fork 2
Code Issues 6 Pull requests 3 Projects Releases Packages Wiki Activity

add node aliases, optimized node eval #14

Merged
rlahfa merged 2 commits from griffi-gh/colmena:alias into main 2025-02-26 02:46:08 +01:00
Conversation 5 Commits 2 Files changed 6 +289 -123
griffi-gh commented 2025-02-19 22:16:11 +01:00
Contributor
Copy link

changes eval strategy and adds aliases attr to node deploy conf

(should work but needs more testing)

based on #13

Old Flow:

Either:
    In case only the node names are needed to run the filter, evaluate just the list of node names.
    If tags are used in the query, evaluate ALL deployment attributes of ALL nodes and use that to run the filter.
ONLY if deployment attributes of ALL nodes haven't been evaluated in the previous step:
    Evaluate all attributes (full configuration) of nodes selected by the filter.
Use that information for deployment.

New Flow:

Get what needs to be evaluated to run the current filter  (nothing, names+aliases, tags, or both).
Evaluate just those parts (not the entire configuration) of ALL nodes and run the filter using them
Evaluate all attributes of nodes selected by the filter in the previous step (and use that information for deployment).

There's no separate flow for evaluating just the names anymore; it should be more flexible (controlled by the first step), and in theory, this approach is much simpler.
(It's probably broken though, needs testing :p)

changes eval strategy and adds aliases attr to node deploy conf (should work but needs more testing) based on https://git.dgnum.eu/DGNum/colmena/pulls/13 --- Old Flow: Either: In case only the node names are needed to run the filter, evaluate just the list of node names. If tags are used in the query, evaluate ALL deployment attributes of ALL nodes and use that to run the filter. ONLY if deployment attributes of ALL nodes haven't been evaluated in the previous step: Evaluate all attributes (full configuration) of nodes selected by the filter. Use that information for deployment. New Flow: Get what needs to be evaluated to run the current filter (nothing, names+aliases, tags, or both). Evaluate just those parts (not the entire configuration) of ALL nodes and run the filter using them Evaluate all attributes of nodes selected by the filter in the previous step (and use that information for deployment). There's no separate flow for evaluating just the names anymore; it should be more flexible (controlled by the first step), and in theory, this approach is much simpler. (It's probably broken though, needs testing :p)
griffi-gh changed title from WIP: add node aliases to add node aliases 2025-02-19 22:32:49 +01:00
requested review from rlahfa 2025-02-19 22:33:06 +01:00
griffi-gh force-pushed alias from ba6a9dc798 to 9276456b97 2025-02-19 22:38:26 +01:00 Compare
griffi-gh changed title from add node aliases to WIP: add node aliases 2025-02-19 22:40:07 +01:00
lbailly approved these changes 2025-02-19 23:31:36 +01:00
Dismissed
lbailly left a comment
Member
Copy link

It needs some cleanup, but looks good to me.

It needs some cleanup, but looks good to me.
src/nix/hive/mod.rs Outdated
@ -399,0 +400,4 @@
assert!(needs_eval.any(), "no need to evaluate");
let expr = format!(
"map (attrs: {{ inherit (attrs) {}{}; }}) hive.deploymentConfig",
lbailly commented 2025-02-19 23:17:34 +01:00
Member
Copy link

missing space between the 2 {} in inherit (attrs) {}{}; (when both are required).

missing space between the 2 `{}` in `inherit (attrs) {}{};` (when both are required).
griffi-gh marked this conversation as resolved
src/nix/node_filter.rs Outdated
@ -31,0 +81,4 @@
}
fn from_tags(tags: Vec<String>) -> Self {
Self {
aliases: Some(vec![]),
lbailly commented 2025-02-19 23:24:21 +01:00
Member
Copy link

Should be None as we only queried tags ?
(Same applies for tags in from_aliases bellow)
If it's only for test purposes, maybe these functions should be moved in the test part of the file.

Should be `None` as we only queried tags ? (Same applies for `tags` in `from_aliases` bellow) If it's only for test purposes, maybe these functions should be moved in the test part of the file.
griffi-gh commented 2025-02-20 00:20:09 +01:00
Author
Contributor
Copy link

yeah, those are only used for tests... moving them would make sense...

yeah, those are only used for tests... moving them would make sense...
griffi-gh marked this conversation as resolved
lbailly commented 2025-02-19 23:38:20 +01:00
Member
Copy link

It feels weird that we allow a node to have an alias being the name of another, like a and b, with b.aliases = [ "a" ], in which case colmena apply --on a applies both nodes. Idk if we want to introduce a warning in such situations.

It feels weird that we allow a node to have an alias being the name of another, like `a` and `b`, with `b.aliases = [ "a" ]`, in which case `colmena apply --on a` applies both nodes. Idk if we want to introduce a warning in such situations.
lbailly reviewed 2025-02-19 23:50:15 +01:00
src/nix/hive/mod.rs Outdated
@ -399,0 +397,4 @@
&self,
needs_eval: NeedsEval,
) -> ColmenaResult<HashMap<NodeName, PartialNodeConfig>> {
assert!(needs_eval.any(), "no need to evaluate");
lbailly commented 2025-02-19 23:50:15 +01:00
Member
Copy link

This assert fails in the case of empty filter (which should matches all nodes)

This assert fails in the case of empty filter (which should matches all nodes)
griffi-gh marked this conversation as resolved
griffi-gh commented 2025-02-20 01:13:24 +01:00
Author
Contributor
Copy link

@lbailly wrote in #14 (comment):

It feels weird that we allow a node to have an alias being the name of another, like a and b, with b.aliases = [ "a" ], in which case colmena apply --on a applies both nodes. Idk if we want to introduce a warning in such situations.

i believe that should be a hard error?

@lbailly wrote in https://git.dgnum.eu/DGNum/colmena/pulls/14#issuecomment-12302: > It feels weird that we allow a node to have an alias being the name of another, like `a` and `b`, with `b.aliases = [ "a" ]`, in which case `colmena apply --on a` applies both nodes. Idk if we want to introduce a warning in such situations. i believe that should be a hard error?
👍 1
griffi-gh changed title from WIP: add node aliases to add node aliases 2025-02-20 22:32:27 +01:00
griffi-gh changed title from add node aliases to add node aliases, optimized node eval 2025-02-20 22:44:21 +01:00
griffi-gh changed title from add node aliases, optimized node eval to WIP: add node aliases, optimized node eval 2025-02-20 22:47:00 +01:00
lbailly reviewed 2025-02-21 10:47:48 +01:00
src/nix/node_filter.rs Outdated
@ -313,1 +364,3 @@
None
/// In case of trivial filters which dont actually use any node info
/// Try to eval them immediately
pub fn try_eval_trivial(&self) -> Option<bool> {
lbailly commented 2025-02-21 10:47:48 +01:00
Member
Copy link

Currently, the only trivial filter is Empty as it's not possible to build trivial Union or Inter (trivial = size < 2), nor to have an Empty in a leaf of a more complex filter.

Currently, the only trivial filter is `Empty` as it's not possible to build trivial `Union` or `Inter` (trivial = size < 2), nor to have an `Empty` in a leaf of a more complex filter.
lbailly commented 2025-02-21 10:56:45 +01:00
Member
Copy link

(In syntactic analysis, !a&a is trivial, but require a semantic analysis, which I don't think we want)

(In syntactic analysis, `!a&a` is trivial, but require a semantic analysis, which I don't think we want)
griffi-gh commented 2025-02-21 20:41:23 +01:00
Author
Contributor
Copy link

Yea, I agree that it's kinda over-engineered...
(but it does handle the Empty case so i guess its fine?)

Should i change this?

Yea, I agree that it's kinda over-engineered... (but it does handle the Empty case so i guess its fine?) Should i change this?
lbailly commented 2025-02-21 21:39:05 +01:00
Member
Copy link

I don't think it's necessary to change this, this over-engineering won't weight much and will make future development cleaner (if the invariant was invalidated for example).

I don't think it's necessary to change this, this over-engineering won't weight much and will make future development cleaner (if the invariant was invalidated for example).
lbailly marked this conversation as resolved
lbailly requested changes 2025-02-21 21:55:09 +01:00
Dismissed
src/nix/node_filter.rs Outdated
@ -31,0 +33,4 @@
// pub enum NeedsEval {
// Tags,
// Aliases,
// }
lbailly commented 2025-02-21 21:53:42 +01:00
Member
Copy link

You can remove

You can remove
griffi-gh marked this conversation as resolved
src/nix/node_filter.rs Outdated
@ -318,3 +393,4 @@
}
}
// TODO fix tests
lbailly commented 2025-02-21 21:53:25 +01:00
Member
Copy link

they look good to me, no ?

they look good to me, no ?
griffi-gh commented 2025-02-21 22:59:30 +01:00
Author
Contributor
Copy link

yeah i forgot to remove it
i already fixed them in ba6a9dc798d77b3e32dfb1302543827cbaaf3a34

yeah i forgot to remove it i already fixed them in ba6a9dc798d77b3e32dfb1302543827cbaaf3a34
griffi-gh marked this conversation as resolved
lbailly commented 2025-02-21 21:59:45 +01:00
Member
Copy link

Forgot to write the final comment: it just need to make aliases optional, to avoid the error

error:
       … while evaluating attribute 'ap01'

       … while evaluating attribute 'aliases'
         at «string»:1:341:
            1| with builtins; let eval = import /tmp/colmena-assets-b9tLyQ/eval.nix; hive = eval { rawHive = import /home/catvayor/ghq/git.dgnum.eu/DGNum/infrastructure/hive.nix; colmenaOptions = import /tmp/colmena-assets-b9tLyQ/options.nix; colmenaModules = import /tmp/colmena-assets-b9tLyQ/modules.nix; }; in (mapAttrs (name: attrs: { inherit (attrs) aliases ; }) hive.deploymentConfig)
             |                                                                                                                                                                                                                                                                                                                                                     ^

       error: attribute 'aliases' missing
       at «string»:1:341:
            1| with builtins; let eval = import /tmp/colmena-assets-b9tLyQ/eval.nix; hive = eval { rawHive = import /home/catvayor/ghq/git.dgnum.eu/DGNum/infrastructure/hive.nix; colmenaOptions = import /tmp/colmena-assets-b9tLyQ/options.nix; colmenaModules = import /tmp/colmena-assets-b9tLyQ/modules.nix; }; in (mapAttrs (name: attrs: { inherit (attrs) aliases ; }) hive.deploymentConfig)
             | 
                                                                                                                   ^
[ERROR] -----
[ERROR] Operation failed with error: Child process exited with error code: 1
Hint: Backtrace available - Use `RUST_BACKTRACE=1` environment variable to display a backtrace

and there's two dead code warning

Forgot to write the final comment: it just need to make `aliases` optional, to avoid the error ``` error: … while evaluating attribute 'ap01' … while evaluating attribute 'aliases' at «string»:1:341: 1| with builtins; let eval = import /tmp/colmena-assets-b9tLyQ/eval.nix; hive = eval { rawHive = import /home/catvayor/ghq/git.dgnum.eu/DGNum/infrastructure/hive.nix; colmenaOptions = import /tmp/colmena-assets-b9tLyQ/options.nix; colmenaModules = import /tmp/colmena-assets-b9tLyQ/modules.nix; }; in (mapAttrs (name: attrs: { inherit (attrs) aliases ; }) hive.deploymentConfig) | ^ error: attribute 'aliases' missing at «string»:1:341: 1| with builtins; let eval = import /tmp/colmena-assets-b9tLyQ/eval.nix; hive = eval { rawHive = import /home/catvayor/ghq/git.dgnum.eu/DGNum/infrastructure/hive.nix; colmenaOptions = import /tmp/colmena-assets-b9tLyQ/options.nix; colmenaModules = import /tmp/colmena-assets-b9tLyQ/modules.nix; }; in (mapAttrs (name: attrs: { inherit (attrs) aliases ; }) hive.deploymentConfig) | ^ [ERROR] ----- [ERROR] Operation failed with error: Child process exited with error code: 1 Hint: Backtrace available - Use `RUST_BACKTRACE=1` environment variable to display a backtrace ``` and there's two dead code warning
lbailly reviewed 2025-02-21 22:03:28 +01:00
src/nix/hive/mod.rs Outdated
@ -399,0 +441,4 @@
let expr = format!(
"(mapAttrs (name: attrs: {{ inherit (attrs) {} {}; }}) hive.deploymentConfig)",
needs_eval.aliases.then_some("aliases").unwrap_or_default(),
needs_eval.tags.then_some("tags").unwrap_or_default(),
lbailly commented 2025-02-21 22:03:28 +01:00
Member
Copy link
        let expr = format!(
            "(mapAttrs (name: attrs: {{ {} {} }}) hive.deploymentConfig)",
            needs_eval.aliases.then_some("aliases = attrs.aliases or [ ];").unwrap_or_default(),
            needs_eval.tags.then_some("tags = attrs.tags or [ ];").unwrap_or_default(),
        );
```rust let expr = format!( "(mapAttrs (name: attrs: {{ {} {} }}) hive.deploymentConfig)", needs_eval.aliases.then_some("aliases = attrs.aliases or [ ];").unwrap_or_default(), needs_eval.tags.then_some("tags = attrs.tags or [ ];").unwrap_or_default(), ); ```
lbailly commented 2025-02-21 22:08:27 +01:00
Member
Copy link

no

no
lbailly marked this conversation as resolved
lbailly commented 2025-02-21 22:11:46 +01:00
Member
Copy link

in src/nix/hive/options.nix you should add

aliases = lib.mkOption {
  description = ''
    A list of aliases for the node.

    Can be used to select a node with another name.
  '';
  type = types.listOf types.str;
  default = [];
};
in `src/nix/hive/options.nix` you should add ```nix aliases = lib.mkOption { description = '' A list of aliases for the node. Can be used to select a node with another name. ''; type = types.listOf types.str; default = []; }; ```
👍 1
griffi-gh started working 2025-02-21 23:04:21 +01:00
griffi-gh canceled time tracking 2025-02-21 23:04:26 +01:00
griffi-gh changed title from WIP: add node aliases, optimized node eval to add node aliases, optimized node eval 2025-02-21 23:04:46 +01:00
lbailly approved these changes 2025-02-21 23:05:36 +01:00
lbailly left a comment
Member
Copy link

LGTM @rlahfa

LGTM @rlahfa
griffi-gh force-pushed alias from b4e269759d to 57dc8e8ccd 2025-02-21 23:59:50 +01:00 Compare
griffi-gh force-pushed alias from 57dc8e8ccd to 55b56196d5 2025-02-22 00:04:04 +01:00 Compare
rlahfa reviewed 2025-02-23 16:31:29 +01:00
src/nix/hive/options.nix
@ -93,6 +93,7 @@ with builtins; rec {
# Largely compatible with NixOps/Morph.
deploymentOptions = { name, lib, ... }: let
inherit (lib) types;
mdDoc = lib.mdDoc or lib.id;
rlahfa commented 2025-02-23 16:31:29 +01:00
Owner
Copy link

Why is mdDoc needed? That seems dead code to me.

Why is `mdDoc` needed? That seems dead code to me.
griffi-gh commented 2025-02-23 16:36:52 +01:00
Author
Contributor
Copy link

that part is from the #13
since i based the pr on it

(idk what the mdDoc is for, but the flake was failing to evaluate because of a missing variable
so i copied the same let binding that was used in the other 2 sections)

that part is from the https://git.dgnum.eu/DGNum/colmena/pulls/13 since i based the pr on it (idk what the mdDoc is for, but the flake was failing to evaluate because of a missing variable so i copied the same let binding that was used in the other 2 sections)
rlahfa reviewed 2025-02-23 16:31:58 +01:00
src/nix/node_filter.rs Outdated
@ -29,2 +30,4 @@
}
#[derive(Clone, Copy, Debug, Default)]
pub struct NeedsEval {
rlahfa commented 2025-02-23 16:31:58 +01:00
Owner
Copy link

What is NeedsEval, can we have a comment on what is this structure supposed to carry?

What is `NeedsEval`, can we have a comment on what is this structure supposed to carry?
griffi-gh commented 2025-02-23 16:35:09 +01:00
Author
Contributor
Copy link

which parts of the deployment metadata need to be evaluated to run the filter.
(Should probably add a doc comment)

which parts of the deployment metadata need to be evaluated to run the filter. (Should probably add a doc comment)
griffi-gh marked this conversation as resolved
rlahfa reviewed 2025-02-23 16:32:24 +01:00
src/nix/node_filter.rs Outdated
@ -264,0 +317,4 @@
|| config
.aliases
.as_ref()
.expect("aliases missing")
rlahfa commented 2025-02-23 16:32:24 +01:00
Owner
Copy link

expect means we are crashing here, can we have better error reporting?

`expect` means we are crashing here, can we have better error reporting?
rlahfa marked this conversation as resolved
rlahfa reviewed 2025-02-23 16:32:28 +01:00
src/nix/node_filter.rs Outdated
@ -266,1 +325,3 @@
.tags()
.tags
.as_ref()
.expect("tags missing")
rlahfa commented 2025-02-23 16:32:28 +01:00
Owner
Copy link

expect means we are crashing here, can we have better error reporting?

`expect` means we are crashing here, can we have better error reporting?
griffi-gh commented 2025-02-23 16:34:09 +01:00
Author
Contributor
Copy link

this error should never happen as tags are guaranteed to be gathered by the previous step (even the tags are empty, the field should never be None)

Panic if tags/deployment info is missing was also the original behavior before the rewrite...

same sith the aliases one

this error should never happen as tags are guaranteed to be gathered by the previous step (even the tags are empty, the field should never be None) Panic if tags/deployment info is missing was also the original behavior before the rewrite... same sith the aliases one
rlahfa commented 2025-02-26 02:45:43 +01:00
Owner
Copy link

makes sense

makes sense
rlahfa marked this conversation as resolved
rlahfa requested changes 2025-02-23 16:32:51 +01:00
rlahfa left a comment
Owner
Copy link

Few changes and we can merge.

Few changes and we can merge.
griffi-gh force-pushed alias from 55b56196d5 to 3ef5ba8658 2025-02-23 17:54:57 +01:00 Compare
griffi-gh commented 2025-02-24 18:51:25 +01:00
Author
Contributor
Copy link

I don't think those two parts need proper error handling
(the panic behaviour matches the original one and should never happen at runtime, unless there's another huge bug with eval)

I added the documentation to the NeedsEval struct as requested

I don't think those two parts need proper error handling (the panic behaviour matches the original one and should never happen at runtime, unless there's another huge bug with eval) I added the documentation to the `NeedsEval` struct as requested
👍 2
requested review from rlahfa 2025-02-24 18:56:16 +01:00
rlahfa merged commit 3ef5ba8658 into main 2025-02-26 02:46:08 +01:00
Sign in to join this conversation.
Reviewers
No reviewers
lbailly
rlahfa
Labels
Clear labels
  
bug

Something is not working

  
duplicate

This issue or pull request already exists

  
enhancement

New feature

  
help wanted

Need some help

  
invalid

Something is wrong

  
question

More information is needed

  
wontfix

This won't be fixed

No labels
bug
duplicate
enhancement
help wanted
invalid
question
wontfix
Milestone
Clear milestone
No items
No milestone
Projects
Clear projects
No items
No project
Assignees
Clear assignees
No assignees
3 participants
Notifications
Due date
The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set.

Reference: DGNum/colmena#14
No description provided.
Delete branch "griffi-gh/colmena:alias"

Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?