feat: init custom activation #18

Open
ecoppens wants to merge 25 commits from feature/custom-activation into main
Owner
No description provided.
flake.nix Outdated
@ -83,6 +83,7 @@
devShell = pkgs.mkShell {
RUST_SRC_PATH = "${pkgs.rustPlatform.rustLibSrc}";
NIX_PATH = "nixpkgs=${pkgs.path}";
PROTOC = "${pkgs.protobuf}/bin/protoc";
Owner

getExe' pkgs.protobuf "protoc"

`getExe' pkgs.protobuf "protoc"`
ecoppens marked this conversation as resolved
ecoppens force-pushed feature/custom-activation from d6bad0d85f to 04dfa25d0b 2025-06-09 13:57:28 +02:00 Compare
ecoppens force-pushed feature/custom-activation from 04dfa25d0b to c6720d7bc8 2025-08-04 22:23:37 +02:00 Compare
ecoppens force-pushed feature/custom-activation from fc7964eb0a to d9e41ab07d 2025-08-05 14:10:05 +02:00 Compare
ecoppens force-pushed feature/custom-activation from 21bf640c47 to 3ca9b95f44 2025-09-08 17:29:22 +02:00 Compare
ecoppens changed title from WIP: custom activation to feat: init custom activation 2025-09-09 10:42:37 +02:00
ecoppens force-pushed feature/custom-activation from 3ca9b95f44 to 747f87e4bd 2025-09-13 17:29:37 +02:00 Compare
@ -0,0 +72,4 @@
targets: Arc<Mutex<HashMap<Id, TH>>>,
}
impl<TH, AT, BT, CT> ActivationServiceImpl<TH, AT, BT, CT>
Owner

Tu veux pas juste prendre des Box au lieu de monomorphiser des async fn? Ça m'a l'air extrêmement fragile et compliqué

Tu veux pas juste prendre des Box<dyn T> au lieu de monomorphiser des async fn? Ça m'a l'air extrêmement fragile et compliqué
Author
Owner

j'avais fait pas mal de recherches et de tests qui concluaient que ça ne pouvait pas marcher
mais j'ai refait une recherche pour être sûr aujoud'hui et j'ai vu qu'en utilisant https://docs.rs/futures/latest/futures/future/type.BoxFuture.html ça pourrait peut-être fonctioner...

j'avais fait pas mal de recherches et de tests qui concluaient que ça ne pouvait pas marcher mais j'ai refait une recherche pour être sûr aujoud'hui et j'ai vu qu'en utilisant https://docs.rs/futures/latest/futures/future/type.BoxFuture.html ça pourrait peut-être fonctioner...
Author
Owner

Après des tests plus approfondis, je pense que ça ne fonctionnera pas
Si tu as une idée plus précise d'implémentation qui respectent toutes les conditions et qui soit pas plus compliqué que la solution actuelle, je veux bien l'implémenter

Par ailleurs je pense pas que la solution actuelle soit fragile (mais extrêmement compliqué, oui...)

Après des tests plus approfondis, je pense que ça ne fonctionnera pas Si tu as une idée plus précise d'implémentation qui respectent toutes les conditions et qui soit pas plus compliqué que la solution actuelle, je veux bien l'implémenter Par ailleurs je pense pas que la solution actuelle soit fragile (mais extrêmement compliqué, oui...)
Owner

Je disais "fragile" dans le sens où le jour où tu sors un objet chelou qui remplit pas une trait condition, tu te retrouves à debug l'enfer mais OK

Je disais "fragile" dans le sens où le jour où tu sors un objet chelou qui remplit pas une trait condition, tu te retrouves à debug l'enfer mais OK
Author
Owner

J'avais compris ce sens, mais si tu remplis pas une trait condition, même si tu boxes la chose, ça atterit sur le même résultat.
Normalement j'ai fait en sorte que les traits doivent couvrir le maximum de cas (sauf peut-être la condition 'static)

J'avais compris ce sens, mais si tu remplis pas une trait condition, même si tu boxes la chose, ça atterit sur le même résultat. Normalement j'ai fait en sorte que les traits doivent couvrir le maximum de cas (sauf peut-être la condition 'static)
ecoppens marked this conversation as resolved
@ -0,0 +8,4 @@
RequestHeader header = 1;
string toplevel = 2;
ApplyType apply_type = 3;
bool reboot = 4;
Owner

should be an enum so that reboot can take on multiple meanings: system reboot, kexec and leave space for more meanings later.

should be an enum so that reboot can take on multiple meanings: system reboot, kexec and leave space for more meanings later.
ecoppens marked this conversation as resolved
@ -0,0 +6,4 @@
message BuildRequest {
RequestHeader header = 1;
string derivation = 2;
bool remote = 3;
Owner

What does this mean?

What does this mean?
Author
Owner

remote building, i'll rename it remote_building

remote building, i'll rename it `remote_building`
ecoppens marked this conversation as resolved
@ -0,0 +36,4 @@
/// The gRPC connection to the activation program
grpc_client: ActivationClient<Channel>,
session_id: String,
Owner

A comment?

A comment?
@ -0,0 +70,4 @@
let mut grpc_client = None;
for _ in 0..5 {
Owner

This should use exponential backoff and the number of retries and spacing should be configurable I would say, but you can punt a TODO for it.

This should use exponential backoff and the number of retries and spacing should be configurable I would say, but you can punt a TODO for it.
Author
Owner

Yep, but this is here for now only to let activation_program the time to boot (which, in my tests, and the several programs I wrote, is always less than 5 seconds)

Yep, but this is here for now only to let `activation_program` the time to boot (which, in my tests, and the several programs I wrote, is always less than 5 seconds)
@ -0,0 +106,4 @@
header: Some(RequestHeader {
session_id: self.session_id.clone(),
}),
derivation: derivation.as_path().to_str().unwrap().to_string(),
Owner

Won't this lead to "x" instead of x in the string format? to_string() seems always the wrong method to call.

Won't this lead to `"x"` instead of `x` in the string format? `to_string()` seems always the wrong method to call.
Author
Owner

I don't understand your point.

I don't understand your point.
Author
Owner

If you mean that to_string() while add quote around the string, it is wrong.

str.to_string() == str.to_owned() (including performance)
I find to_string() clearer that we will actually convert it to a String

If you mean that `to_string()` while add quote around the string, it is wrong. `str.to_string()` == `str.to_owned()` (including performance) I find `to_string()` clearer that we will actually convert it to a `String`
ecoppens marked this conversation as resolved
@ -0,0 +195,4 @@
}),
store_path: closure
.as_path()
.as_os_str()
Owner

Big mehhhhh, use String::from_utf8_lossy and prepare for crashes when you encounter a non-UTF8 store path.

Big mehhhhh, use `String::from_utf8_lossy` and prepare for crashes when you encounter a non-UTF8 store path.
Author
Owner

Aren't store paths always UTF-8 in nix?

Aren't store paths always UTF-8 in nix?
Owner

my sweet summer child…

my sweet summer child…
@ -0,0 +1,109 @@
#![allow(clippy::pedantic)]
tonic::include_proto!("colmena.activation");
pub trait HeadedResponse {
Owner

Comment on what this is.

Comment on what this is.
@ -0,0 +63,4 @@
}
fn from_header(header: ResponseHeader) -> Self {
let new_store_path = if header.msg.starts_with("/nix/store") {
Owner

Why can we make this assumption?

Why can we make this assumption?
Author
Owner

Messages can be log of the building too, so not every message is a nix store path.

Messages can be log of the building too, so not every message is a nix store path.
ecoppens marked this conversation as resolved
@ -0,0 +250,4 @@
let session_id = Id(id);
let mut lock = self.targets.lock().await;
Owner

let mut targets = no?

`let mut targets =` no?
@ -0,0 +162,4 @@
}
};
let lock = self.targets.lock().await;
Owner

let targets = no?

`let targets =` no?
@ -0,0 +257,4 @@
session_id: session_id
.0
.iter()
.fold(String::new(), |s, i| format!("{s}{i:02x}")),
Owner

Can you explain the transformation you want to do here?

Can you explain the transformation you want to do here?
@ -0,0 +94,4 @@
.lines();
loop {
for stderr_line in stderr.by_ref() {
Owner

Mixing stderr and stdout in the same log seems a bit of a shame, no?

Mixing stderr and stdout in the same log seems a bit of a shame, no?
Author
Owner

I think we can because, the processing is a bit different:

  • if it comes from stderr, it is only logs, so we can send it raw to the client, so it can print it
  • if it comes from stdout, it is useful information, so we send it raw to the client as logging AND we process it as information and send it to the client as information
I think we can because, the processing is a bit different: - if it comes from stderr, it is only logs, so we can send it raw to the client, so it can print it - if it comes from stdout, it is useful information, so we send it raw to the client as logging AND we process it as information and send it to the client as information
Owner

OK, wfm

OK, wfm
rlahfa marked this conversation as resolved
@ -0,0 +20,4 @@
},
}
impl TryFrom<Url> for TargetHost {
Owner

Warning about URI encoding, space is %20%, your display formatter does not reintroduce this?

Warning about URI encoding, space is `%20%`, your display formatter does not reintroduce this?
Author
Owner

Display formatter has only one purpose for now: printing to stderr for debugging, so it is not important (and %20% will flood the stderr with useless and unreadable information)

Display formatter has only one purpose for now: printing to stderr for debugging, so it is not important (and `%20%` will flood the stderr with useless and unreadable information)
ecoppens marked this conversation as resolved
@ -0,0 +10,4 @@
import "copy_closure_request.proto";
import "copy_closure_response.proto";
service Activation {
Owner

What's the overall story for versioning this protocol?

What's the overall story for versioning this protocol?
rlahfa left a comment
Owner

More explanations are required at this point. This seems mostly good to me as a first step.

More explanations are required at this point. This seems mostly good to me as a first step.
This pull request can be merged automatically.
You are not authorized to merge this pull request.
View command line instructions

Checkout

From your project repository, check out a new branch and test the changes.
git fetch -u origin feature/custom-activation:feature/custom-activation
git switch feature/custom-activation

Merge

Merge the changes and update on Forgejo.

Warning: The "Autodetect manual merge" setting is not enabled for this repository, you will have to mark this pull request as manually merged afterwards.

git switch main
git merge --no-ff feature/custom-activation
git switch feature/custom-activation
git rebase main
git switch main
git merge --ff-only feature/custom-activation
git switch feature/custom-activation
git rebase main
git switch main
git merge --no-ff feature/custom-activation
git switch main
git merge --squash feature/custom-activation
git switch main
git merge --ff-only feature/custom-activation
git switch main
git merge feature/custom-activation
git push origin main
Sign in to join this conversation.
No reviewers
No milestone
No project
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#18
No description provided.