feat/discover_inventory #127

Merged
letian merged 8 commits from feat/discover_inventory into refact/harmony_types 2025-08-31 22:45:09 +00:00
Owner

Fully automated inventory gathering now works!!

Been waiting a long time for this feature

Boot up harmony_inventory_agent with cargo run -p harmony_inventory_agent
Launch the DiscoverInventoryAgentScore , currently available this way :

RUST_LOG=info cargo run -p example-cli -- -f Discover -y

And you will have automatically all hosts saved to the database. Run cargo sqlx setup if you have not done it yet.

## Fully automated inventory gathering now works!! Been waiting a long time for this feature Boot up harmony_inventory_agent with `cargo run -p harmony_inventory_agent` Launch the DiscoverInventoryAgentScore , currently available this way : `RUST_LOG=info cargo run -p example-cli -- -f Discover -y` And you will have automatically all hosts saved to the database. Run `cargo sqlx setup` if you have not done it yet.
johnride added 4 commits 2025-08-31 05:07:23 +00:00
Boot up harmony_inventory_agent with `cargo run -p harmony_inventory_agent`
Launch the DiscoverInventoryAgentScore , currently available this way :

`RUST_LOG=info cargo run -p example-cli -- -f Discover -y`

And you will have automatically all hosts saved to the database. Run `cargo sqlx setup` if you have not done it yet.
chore: Update harmony_inventory_agent binary
Some checks failed
Run Check Script / check (pull_request) Failing after 1m14s
affcc657c1
johnride added 1 commit 2025-08-31 05:27:28 +00:00
fix tests
All checks were successful
Run Check Script / check (pull_request) Successful in 1m15s
a27c7c2310
letian reviewed 2025-08-31 15:53:32 +00:00
@ -35,3 +34,4 @@
}
}
pub fn summary(&self) -> String {
Owner

A garder un oeil si on aurait besoin de presenter ce summary de manieres differentes (e.g. fichier json). A ce moment-la, il pourrait etre interessant d'y aller avec le pattern de "presenter":

fn summary(&self, presenter: PhysicalHostPresenter) {
  presenter.present_model(self.labels, self.category);
  presenter.present_cpus(self.cpus);
  presenter.present_memory(self.memory_modules);
  presenter.present_storage(self.storage);
  // ...
}
A garder un oeil si on aurait besoin de presenter ce summary de manieres differentes (e.g. fichier json). A ce moment-la, il pourrait etre interessant d'y aller avec le pattern de "presenter": ```rs fn summary(&self, presenter: PhysicalHostPresenter) { presenter.present_model(self.labels, self.category); presenter.present_cpus(self.cpus); presenter.present_memory(self.memory_modules); presenter.present_storage(self.storage); // ... } ```
letian reviewed 2025-08-31 16:17:30 +00:00
@ -2,0 +11,4 @@
impl InventoryRepositoryFactory {
pub async fn build() -> Result<Box<dyn InventoryRepository>, RepoError> {
Ok(Box::new(
SqliteInventoryRepository::new(&(*DATABASE_URL)).await?,
Owner

Just sharing a thought even if it's not the most representative situation here (considering we're in the infra). Though it still indirectly highlights the underlying issue.

I understand the simplicity of having a config crate and using it when needed, but it actually makes things harder to reuse/test in isolation. All of this because in the end we depend on an env var that forces anyone using the harmony crate to use that as config mechanism instead of other options.

It's fine to use this config crate in the CLI (or TUI or any frontend), but for the actual harmony lib it would be a better practice to simply pass the params in the functions.

Now here, we're kinda "forced" to do this because the InventoryRepository is created in the middle of the execution of an Interpret. It would have been better to properly inject it, but that would mean its Score won't be stateless anymore (as it would be our only injection mechanism as of now).

I'm not saying we should fix this now because this is a bigger architectural issue, but we should keep an eye on it and tackle this sooner than later.

Extra note: despite the appearances using a static factory doesn't really save us from the OCP violation that we would introduce if we were directly instantiating the concrete class. Yes we can switch to another instance without modifying existing code, but it will force everyone using it to switch as well. Which might not be expected and will make it harder in the end to fix in a bigger codebase (it leaves scars 😢). Again, it's ok here, but we should be very careful about using this "pattern" (it's actually an anti-pattern).

Just sharing a thought even if it's not the most representative situation here (considering we're in the infra). Though it still indirectly highlights the underlying issue. I understand the simplicity of having a `config` crate and using it when needed, but it actually makes things harder to reuse/test in isolation. All of this because in the end we depend on an env var that forces anyone using the `harmony` crate to use that as config mechanism instead of other options. It's fine to use this `config` crate in the `CLI` (or `TUI` or any frontend), but for the actual `harmony` lib it would be a better practice to simply pass the params in the functions. Now here, we're kinda "forced" to do this because the `InventoryRepository` is created in the middle of the execution of an `Interpret`. It would have been better to properly inject it, but that would mean its `Score` won't be stateless anymore (as it would be our only injection mechanism as of now). I'm not saying we should fix this now because this is a bigger architectural issue, but we should keep an eye on it and tackle this sooner than later. _Extra note: despite the appearances using a static factory doesn't really save us from the OCP violation that we would introduce if we were directly instantiating the concrete class. Yes we can switch to another instance without modifying existing code, but it will force everyone using it to switch as well. Which might not be expected and will make it harder in the end to fix in a bigger codebase (it leaves scars 😢). Again, it's ok here, but we should be very careful about using this "pattern" (it's actually an anti-pattern)._
Owner

I'll create an issue about this dependency injection problem we have in our Interprets (and at some extent the Scores statelessness).

I'll create an issue about this dependency injection problem we have in our `Interprets` (and at some extent the `Scores` statelessness).
Author
Owner

Yeah the factory doesn't bring much value here, no time to do better but at least it allows a minimum of decoupling if we want to switch repository type for the entire application instance, or provide more methods that do take parameters and can apply additional logic when building the repo.

Yeah the factory doesn't bring much value here, no time to do better but at least it allows a minimum of decoupling if we want to switch repository type for the entire application instance, or provide more methods that do take parameters and can apply additional logic when building the repo.
letian reviewed 2025-08-31 16:22:41 +00:00
@ -16,3 +16,3 @@
impl SqliteInventoryRepository {
pub async fn new(database_url: &str) -> Result<Self, RepoError> {
let pool = SqlitePool::connect(database_url)
let _pool = SqlitePool::connect(database_url)
Owner

pool is used so we shouldn't need the _

`pool` is used so we shouldn't need the `_`
letian marked this conversation as resolved
letian reviewed 2025-08-31 16:29:58 +00:00
@ -53,0 +78,4 @@
.unwrap();
trace!("Found host information {host:?}");
// TODO its useless to have two distinct host types but requires a bit much
Owner

It seems useless but I'm not sure they should actually be merged: to me it feels like two different concepts with a different lifecycle. It is highlighted by the fact that the harmony_inventory_agent is a standalone service that could be consumed by other programs and that this inventory module here is just one of them. The inventory has its own representation of a PhysicalHost that might become different than the one from the harmony_inventory_agent. At least it's my hypothesis for now.

It seems useless but I'm not sure they should actually be merged: to me it feels like two different concepts with a different lifecycle. It is highlighted by the fact that the `harmony_inventory_agent` is a standalone service that could be consumed by other programs and that this `inventory` module here is just one of them. The `inventory` has its own representation of a `PhysicalHost` that might become different than the one from the `harmony_inventory_agent`. At least it's my hypothesis for now.
letian added 1 commit 2025-08-31 16:35:26 +00:00
fix var name and format warning
All checks were successful
Run Check Script / check (pull_request) Successful in 1m17s
8bcbe7a226
letian reviewed 2025-08-31 20:56:30 +00:00
@ -53,0 +71,4 @@
info!("Getting host inventory on service at {address} port {port}");
tokio::task::spawn(async move {
info!("Getting inventory for host {address} {port}");
Owner

This log and the one above are very similar

This log and the one above are very similar
letian marked this conversation as resolved
letian added 1 commit 2025-08-31 20:59:08 +00:00
remove extra info log
All checks were successful
Run Check Script / check (pull_request) Successful in 1m16s
57c098a6d8
letian reviewed 2025-08-31 22:02:55 +00:00
@ -58,1 +124,4 @@
status: InterpretStatus::SUCCESS,
message: "Discovery process completed successfully".to_string(),
})
}
Owner

There's an edge case here that might break this score:

discover_agents is awaited, but the spawned task inside the callback are not awaited at all. So in some edge cases, discover_agents might finish executing before the task itself.

And because we don't await for the task inside the callback, error handling is a bit awkward. Currently it says "all good" even though there could be an error:

[INFO ] Getting inventory for host 192.168.5.232 at port 8080

thread 'tokio-runtime-worker' panicked at /home/ian/Projects/nation-tech/harmony/harmony/src/modules/inventory/mod.rs:105:34:
called `Result::unwrap()` on an `Err` value: "Could not build repository : Could not connect to the database: error returned from database: (code: 14) unable to open database file"
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace
[INFO ] ✅ Discovery process completed successfully
There's an edge case here that might break this score: `discover_agents` is awaited, but the _spawned_ task inside the callback are not awaited at all. So in some edge cases, `discover_agents` might finish executing before the task itself. And because we don't _await_ for the task inside the callback, error handling is a bit awkward. Currently it says "all good" even though there could be an error: ``` [INFO ] Getting inventory for host 192.168.5.232 at port 8080 thread 'tokio-runtime-worker' panicked at /home/ian/Projects/nation-tech/harmony/harmony/src/modules/inventory/mod.rs:105:34: called `Result::unwrap()` on an `Err` value: "Could not build repository : Could not connect to the database: error returned from database: (code: 14) unable to open database file" note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace [INFO ] ✅ Discovery process completed successfully ```
letian added 1 commit 2025-08-31 22:07:49 +00:00
adjust logs/errors
All checks were successful
Run Check Script / check (pull_request) Successful in 1m14s
c99e9018af
letian merged commit 701d8cfab9 into refact/harmony_types 2025-08-31 22:45:09 +00:00
letian deleted branch feat/discover_inventory 2025-08-31 22:45:09 +00:00
Sign in to join this conversation.
No reviewers
No Milestone
No project
No Assignees
2 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: NationTech/harmony#127
No description provided.