-
Notifications
You must be signed in to change notification settings - Fork 8
feat: general service discovery for farm testnets #1335
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
Conversation
|
MSD Diff No diff |
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.
PR Overview
This PR introduces a general service discovery service for farm testnets that registers target machines and integrates with the vector stack for log scraping. Key changes include the implementation of a server module with endpoints for adding and retrieving targets, new storage implementations (in-memory and file), and updates to metrics, supervisor, and configuration builder modules to support the service discovery workflow.
Reviewed Changes
| File | Description |
|---|---|
| src/server/mod.rs | Implements the server with axum routes and a graceful shutdown. |
| src/storage/in_memory.rs and src/storage/file.rs | Introduces in-memory and file storage mechanisms for target persistence. |
| src/metrics.rs | Adds metrics instrumentation for monitoring target statuses. |
| src/supervisor.rs | Implements target supervision with target polling, error logging, and GC handling. |
| src/server/add_targets.rs & get_targets.rs | Define HTTP endpoints for target management. |
| Cargo.toml | Updates dependencies and workspace members. |
| multiservice-discovery-shared/builders/general_exec.rs | Adds a configuration builder for general service discovery. |
| multiservice-discovery-downloader | Updates configuration generation logic and target conversion functions. |
| multiservice-discovery-shared/builders/exec_log_config_structure.rs | Exports builder types for command execution configurations. |
Copilot reviewed 24 out of 24 changed files in this pull request and generated 1 comment.
Comments suppressed due to low confidence (2)
rs/ic-observability/multiservice-discovery-downloader/src/downloader_loop.rs:107
- Using unwrap on cli.output_dir.parent() may panic if the output directory has no parent; add a proper check or use a fallback value.
fs_err::create_dir_all(cli.output_dir.parent().unwrap()).unwrap();
rs/ic-observability/multiservice-discovery-downloader/src/downloader_loop.rs:190
- Deserialization with unwrap in convert_and_filter_general_targets can lead to a runtime panic if the input does not match the expected schema; consider error handling to gracefully skip or report faulty entries.
values.into_iter().map(|value| serde_json::from_value(value).unwrap())
rs/ic-observability/general-testnet-service-discovery/src/supervisor.rs
Outdated
Show resolved
Hide resolved
sasa-tomic
left a comment
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.
LGTM, thanks!
DFINITYManu
left a comment
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.
avoid system clock, use std time instant. These can be compared and have no risk of falling backwards.
This service will be used for all sorts of farm testnets which will monitor arbitrary services. The idea is to use this to register testnet machines and then use our vector stack to scrape logs