Merge pull request #1004 from LibreQoE/tree_fix

Harden TreeGuard node and circuit identity handling

docs-es/v2.0/treeguard-es.md

@@ -86,6 +86,15 @@ Cuando está habilitado y no está en dry-run, TreeGuard puede persistir decisio
 
 TreeGuard está diseñado para no pelear con overrides del operador. Si existen overrides del operador para entidades enroladas, TreeGuard omite esas entidades y reporta advertencias.
 
+TreeGuard también se niega a gestionar nodos que ya estén marcados con `"virtual": true` en el `network.json` base. Si existen overrides viejos de TreeGuard para esos nodos, TreeGuard limpia su propia capa de overrides y vuelve a respetar la definición base de la topología.
+
+Para la gestión SQM por circuito, TreeGuard trata los valores duplicados de `device_id` como colisiones de identidad inseguras. Si el mismo `device_id` aparece en más de un circuito dentro de `ShapedDevices.csv`, TreeGuard omite esos circuitos afectados y limpia cualquier override SQM de TreeGuard asociado a esos `device_id` duplicados.
+
+La actividad reciente de TreeGuard está disponible en dos lugares:
+
+- Las vistas de estado/actividad de TreeGuard en la WebUI.
+- El journal de `lqosd`, donde TreeGuard ahora registra cada evento de actividad para que recargas, limpieza de overrides, cambios de SQM y fallos se puedan diagnosticar sin inspeccionar websockets.
+
 ## Páginas Relacionadas
 
 - [HTB + fq_codel + CAKE: Comportamiento Detallado de Colas](htb_fq_codel_cake-es.md)

docs/v2.0/treeguard.md

@@ -86,6 +86,15 @@ When enabled and not dry-run, TreeGuard may persist decisions to:
 
 TreeGuard is designed to avoid fighting operator-owned overrides. If operator overrides exist for enrolled entities, TreeGuard skips those entities and reports warnings.
 
+TreeGuard also refuses to manage nodes that are already marked `"virtual": true` in the base `network.json`. If stale TreeGuard-owned node-virtualization overrides exist for those nodes, TreeGuard clears its own override layer and falls back to the base topology definition.
+
+For circuit SQM management, TreeGuard treats duplicate `device_id` values as unsafe identity collisions. If the same `device_id` appears in more than one circuit in `ShapedDevices.csv`, TreeGuard skips those affected circuits and clears any TreeGuard-owned SQM overrides for those duplicate device IDs.
+
+Recent TreeGuard activity is available in two places:
+
+- The WebUI TreeGuard status/activity views.
+- The `lqosd` journal, where TreeGuard now logs each recorded activity event so reloads, override cleanup, SQM changes, and failures are diagnosable without websocket inspection.
+
 ## Related Pages
 
 - [HTB + fq_codel + CAKE: Detailed Queueing Behavior](htb_fq_codel_cake.md)

src/rust/lqos_config/src/cpu_topology.rs

@@ -432,6 +432,10 @@ fn detect_from_cpuid(possible: &[u32]) -> Option<ResolvedHybridCpuTopology> {
     #[cfg(target_arch = "x86_64")]
     use std::arch::x86_64::__cpuid_count;
 
+    if __cpuid_count(0, 0).eax < 0x1a {
+        return None;
+    }
+
     let original_affinity = sched_getaffinity(Pid::from_raw(0)).ok()?;
     let restore_affinity = original_affinity;
     let mut performance = Vec::new();
@@ -448,7 +452,7 @@ fn detect_from_cpuid(possible: &[u32]) -> Option<ResolvedHybridCpuTopology> {
             return None;
         }
 
-        let leaf = unsafe { __cpuid_count(0x1a, 0) };
+        let leaf = __cpuid_count(0x1a, 0);
         let core_type = (leaf.eax >> 24) & 0xff;
         match core_type {
             CPUID_CORE_TYPE_CORE => performance.push(*cpu),

src/rust/lqosd/src/node_manager/local_api/scheduler.rs

@@ -1,6 +1,8 @@
 use serde::Serialize;
 
-use crate::tool_status::{is_scheduler_available, scheduler_error_message, scheduler_output_message};
+use crate::tool_status::{
+    is_scheduler_available, scheduler_error_message, scheduler_output_message,
+};
 
 // Remove ANSI escape sequences (basic CSI/OSC handling) for browser display
 fn strip_ansi(input: &str) -> String {

src/rust/lqosd/src/shaped_devices_tracker/mod.rs

@@ -277,7 +277,10 @@ pub fn get_funnel(circuit_id: &str) -> BusResponse {
             .rev()
             .skip(1)
         {
-            result.push((*idx, node_to_transport(&reader.get_nodes_when_ready()[*idx])));
+            result.push((
+                *idx,
+                node_to_transport(&reader.get_nodes_when_ready()[*idx]),
+            ));
         }
         return BusResponse::NetworkMap(result);
     }

src/rust/lqosd/src/treeguard/actor.rs

@@ -27,7 +27,7 @@ use parking_lot::RwLock;
 use std::collections::VecDeque;
 use std::sync::OnceLock;
 use std::time::{Duration, Instant};
-use tracing::{debug, warn};
+use tracing::{debug, info, warn};
 
 static TREEGUARD_SENDER: OnceLock<Sender<TreeguardCommand>> = OnceLock::new();
 static TREEGUARD_STATUS_CACHE: OnceLock<RwLock<TreeguardStatusData>> = OnceLock::new();
@@ -209,6 +209,7 @@ struct TreeguardRuntimeState {
     circuit_states: FxHashMap<String, CircuitState>,
     managed_nodes: FxHashSet<String>,
     managed_device_ids: FxHashSet<String>,
+    duplicate_device_conflict_circuits: FxHashSet<String>,
     last_dry_run: Option<bool>,
     reload_controller: ReloadController,
 }
@@ -228,6 +229,7 @@ fn run_tick(
     let circuit_states = &mut runtime_state.circuit_states;
     let managed_nodes = &mut runtime_state.managed_nodes;
     let managed_device_ids = &mut runtime_state.managed_device_ids;
+    let duplicate_device_conflict_circuits = &mut runtime_state.duplicate_device_conflict_circuits;
     let last_dry_run = &mut runtime_state.last_dry_run;
     let reload_controller = &mut runtime_state.reload_controller;
 
@@ -600,10 +602,60 @@ fn run_tick(
                     .as_ref()
                     .and_then(|overrides| overrides_node_virtual(overrides, node_name));
 
-                if node.virtual_node && treeguard_virtual_override.is_none() {
+                if node.virtual_node {
                     status.warnings.push(format!(
                         "TreeGuard links: node '{node_name}' is marked virtual in base network.json; TreeGuard will not manage it."
                     ));
+                    if treeguard_virtual_override.is_some() {
+                        let needs_reload = treeguard_virtual_override == Some(false);
+                        match overrides::clear_node_virtual(node_name) {
+                            Ok(changed) => {
+                                if changed {
+                                    if needs_reload {
+                                        reload_controller.request_reload(
+                                            ReloadPriority::Normal,
+                                            format!(
+                                                "Cleared stale TreeGuard override for base-virtual node '{node_name}'"
+                                            ),
+                                        );
+                                    }
+                                    push_activity(
+                                        activity,
+                                        TreeguardActivityEntry {
+                                            time: now_unix.to_string(),
+                                            entity_type: "node".to_string(),
+                                            entity_id: node_name.to_string(),
+                                            action: "clear_virtual_override_base_virtual"
+                                                .to_string(),
+                                            persisted: true,
+                                            reason: "Node is marked virtual in base network.json; TreeGuard refuses to manage base-virtual nodes.".to_string(),
+                                        },
+                                    );
+                                }
+                            }
+                            Err(e) => {
+                                status.warnings.push(format!(
+                                    "TreeGuard links: failed to clear stale TreeGuard override for base-virtual node '{node_name}': {e}"
+                                ));
+                                push_activity(
+                                    activity,
+                                    TreeguardActivityEntry {
+                                        time: now_unix.to_string(),
+                                        entity_type: "node".to_string(),
+                                        entity_id: node_name.to_string(),
+                                        action: "clear_virtual_override_base_virtual_failed"
+                                            .to_string(),
+                                        persisted: false,
+                                        reason: format!(
+                                            "Failed to clear stale TreeGuard override for base-virtual node: {e}"
+                                        ),
+                                    },
+                                );
+                            }
+                        }
+                    }
+                    managed_nodes.remove(node_name);
+                    link_states.remove(node_name);
                     continue;
                 }
 
@@ -935,10 +987,60 @@ fn run_tick(
                     .as_ref()
                     .and_then(|overrides| overrides_node_virtual(overrides, node_name));
 
-                if node.virtual_node && treeguard_virtual_override.is_none() {
+                if node.virtual_node {
                     status.warnings.push(format!(
-                    "TreeGuard links: node '{node_name}' is marked virtual in base network.json; TreeGuard will not manage it."
-                ));
+                        "TreeGuard links: node '{node_name}' is marked virtual in base network.json; TreeGuard will not manage it."
+                    ));
+                    if treeguard_virtual_override.is_some() {
+                        let needs_reload = treeguard_virtual_override == Some(false);
+                        match overrides::clear_node_virtual(node_name) {
+                            Ok(changed) => {
+                                if changed {
+                                    if needs_reload {
+                                        reload_controller.request_reload(
+                                            ReloadPriority::Normal,
+                                            format!(
+                                                "Cleared stale TreeGuard override for base-virtual node '{node_name}'"
+                                            ),
+                                        );
+                                    }
+                                    push_activity(
+                                        activity,
+                                        TreeguardActivityEntry {
+                                            time: now_unix.to_string(),
+                                            entity_type: "node".to_string(),
+                                            entity_id: node_name.clone(),
+                                            action: "clear_virtual_override_base_virtual"
+                                                .to_string(),
+                                            persisted: true,
+                                            reason: "Node is marked virtual in base network.json; TreeGuard refuses to manage base-virtual nodes.".to_string(),
+                                        },
+                                    );
+                                }
+                            }
+                            Err(e) => {
+                                status.warnings.push(format!(
+                                    "TreeGuard links: failed to clear stale TreeGuard override for base-virtual node '{node_name}': {e}"
+                                ));
+                                push_activity(
+                                    activity,
+                                    TreeguardActivityEntry {
+                                        time: now_unix.to_string(),
+                                        entity_type: "node".to_string(),
+                                        entity_id: node_name.clone(),
+                                        action: "clear_virtual_override_base_virtual_failed"
+                                            .to_string(),
+                                        persisted: false,
+                                        reason: format!(
+                                            "Failed to clear stale TreeGuard override for base-virtual node: {e}"
+                                        ),
+                                    },
+                                );
+                            }
+                        }
+                    }
+                    managed_nodes.remove(node_name);
+                    link_states.remove(node_name);
                     continue;
                 }
 
@@ -1365,8 +1467,36 @@ fn run_tick(
             }
         }
         managed_device_ids.clear();
+        duplicate_device_conflict_circuits.clear();
         circuit_states.clear();
     } else {
+        let mut circuits_by_device_id: FxHashMap<String, FxHashSet<String>> = FxHashMap::default();
+        circuits_by_device_id.reserve(shaped.devices.len());
+        for device in shaped.devices.iter() {
+            let device_id = device.device_id.trim();
+            let circuit_id = device.circuit_id.trim();
+            if device_id.is_empty() || circuit_id.is_empty() {
+                continue;
+            }
+            circuits_by_device_id
+                .entry(device_id.to_string())
+                .or_default()
+                .insert(circuit_id.to_string());
+        }
+        let duplicate_device_ids: FxHashMap<String, Vec<String>> = circuits_by_device_id
+            .into_iter()
+            .filter_map(|(device_id, circuits)| {
+                if circuits.len() <= 1 {
+                    return None;
+                }
+                let mut circuits: Vec<String> = circuits.into_iter().collect();
+                circuits.sort();
+                Some((device_id, circuits))
+            })
+            .collect();
+        let mut current_duplicate_device_conflict_circuits: FxHashSet<String> =
+            FxHashSet::default();
+
         // Reconcile device IDs removed from allowlisted circuits.
         let treeguard_device_ids_with_overrides: FxHashSet<String> = treeguard_overrides_snapshot
             .as_ref()
@@ -1460,6 +1590,102 @@ fn run_tick(
         }
 
         for circuit_id in enrolled_circuits.iter() {
+            let devices: Vec<lqos_config::ShapedDevice> = shaped
+                .devices
+                .iter()
+                .filter(|d| d.circuit_id == circuit_id.as_str())
+                .cloned()
+                .collect();
+
+            let circuit_name: Option<String> = devices.iter().find_map(|d| {
+                let name = d.circuit_name.trim();
+                if name.is_empty() {
+                    None
+                } else {
+                    Some(name.to_string())
+                }
+            });
+            let circuit_entity_id: String = match circuit_name.as_deref() {
+                Some(name) => format!("{name} ({circuit_id})"),
+                None => circuit_id.clone(),
+            };
+            let circuit_label: String = circuit_name.unwrap_or_else(|| circuit_id.clone());
+
+            let mut circuit_device_ids: Vec<String> =
+                devices.iter().map(|d| d.device_id.clone()).collect();
+            circuit_device_ids.sort();
+            circuit_device_ids.dedup();
+            let duplicate_details: Vec<(String, Vec<String>)> = circuit_device_ids
+                .iter()
+                .filter_map(|device_id| {
+                    duplicate_device_ids
+                        .get(device_id)
+                        .map(|circuits| (device_id.clone(), circuits.clone()))
+                })
+                .collect();
+            if !duplicate_details.is_empty() {
+                current_duplicate_device_conflict_circuits.insert(circuit_id.clone());
+                let duplicate_reason = duplicate_details
+                    .iter()
+                    .map(|(device_id, circuits)| {
+                        format!(
+                            "device_id '{}' is shared by circuits [{}]",
+                            device_id,
+                            circuits.join(", ")
+                        )
+                    })
+                    .collect::<Vec<String>>()
+                    .join("; ");
+                status.warnings.push(format!(
+                    "TreeGuard circuits: circuit '{circuit_id}' has duplicate device IDs; TreeGuard will not manage it. {duplicate_reason}"
+                ));
+                if !duplicate_device_conflict_circuits.contains(circuit_id) {
+                    push_activity(
+                        activity,
+                        TreeguardActivityEntry {
+                            time: now_unix.to_string(),
+                            entity_type: "circuit".to_string(),
+                            entity_id: circuit_entity_id.clone(),
+                            action: "skip_duplicate_device_id".to_string(),
+                            persisted: false,
+                            reason: format!(
+                                "TreeGuard refuses circuits with duplicate device IDs. {duplicate_reason}"
+                            ),
+                        },
+                    );
+                }
+                if !circuit_device_ids.is_empty() {
+                    match overrides::clear_device_overrides(&circuit_device_ids) {
+                        Ok(changed) => {
+                            if changed {
+                                push_activity(
+                                    activity,
+                                    TreeguardActivityEntry {
+                                        time: now_unix.to_string(),
+                                        entity_type: "circuit".to_string(),
+                                        entity_id: circuit_entity_id.clone(),
+                                        action: "clear_sqm_overrides_duplicate_device_id"
+                                            .to_string(),
+                                        persisted: true,
+                                        reason: "Duplicate device IDs detected; cleared TreeGuard SQM overlays and skipped management.".to_string(),
+                                    },
+                                );
+                            }
+                        }
+                        Err(e) => {
+                            status.warnings.push(format!(
+                                "TreeGuard circuits: failed to clear TreeGuard SQM overlays for duplicate device IDs on circuit '{circuit_id}': {e}"
+                            ));
+                        }
+                    }
+                    for did in circuit_device_ids.iter() {
+                        managed_device_ids.remove(did);
+                    }
+                }
+                circuit_states.remove(circuit_id);
+                continue;
+            }
+
             let state = circuit_states.entry(circuit_id.clone()).or_insert_with(|| {
                 let mut state = CircuitState::default();
                 if let Some(token) = infer_circuit_sqm_override_token(
@@ -1547,31 +1773,6 @@ fn run_tick(
                     up: None,
                 });
 
-            let devices: Vec<lqos_config::ShapedDevice> = shaped
-                .devices
-                .iter()
-                .filter(|d| d.circuit_id == circuit_id.as_str())
-                .cloned()
-                .collect();
-
-            let circuit_name: Option<String> = devices.iter().find_map(|d| {
-                let name = d.circuit_name.trim();
-                if name.is_empty() {
-                    None
-                } else {
-                    Some(name.to_string())
-                }
-            });
-            let circuit_entity_id: String = match circuit_name.as_deref() {
-                Some(name) => format!("{name} ({circuit_id})"),
-                None => circuit_id.clone(),
-            };
-            let circuit_label: String = circuit_name.unwrap_or_else(|| circuit_id.clone());
-
-            let mut circuit_device_ids: Vec<String> =
-                devices.iter().map(|d| d.device_id.clone()).collect();
-            circuit_device_ids.sort();
-            circuit_device_ids.dedup();
             let operator_conflict = circuit_device_ids
                 .iter()
                 .any(|did| operator_sqm_device_overrides.contains(did));
@@ -1680,8 +1881,10 @@ fn run_tick(
                     let mut persisted_ok = false;
                     let mut can_apply_live = true;
                     if tg.circuits.persist_sqm_overrides {
-                        let device_ids: Vec<String> =
-                            devices.iter().map(|device| device.device_id.clone()).collect();
+                        let device_ids: Vec<String> = devices
+                            .iter()
+                            .map(|device| device.device_id.clone())
+                            .collect();
                         match overrides::set_devices_sqm_override(&device_ids, &token) {
                             Ok(_) => {
                                 persisted_ok = true;
@@ -1788,6 +1991,7 @@ fn run_tick(
                 fq_codel_circuits += 1;
             }
         }
+        *duplicate_device_conflict_circuits = current_duplicate_device_conflict_circuits;
     }
 
     status.virtualized_nodes = virtualized_nodes;
@@ -1914,6 +2118,17 @@ fn infer_circuit_sqm_override_token(
 ///
 /// This function is not pure: it mutates `activity`.
 fn push_activity(activity: &mut VecDeque<TreeguardActivityEntry>, entry: TreeguardActivityEntry) {
+    if entry.action.contains("failed") {
+        warn!(
+            "TreeGuard activity: entity_type={} entity_id={} action={} persisted={} reason={}",
+            entry.entity_type, entry.entity_id, entry.action, entry.persisted, entry.reason
+        );
+    } else {
+        info!(
+            "TreeGuard activity: entity_type={} entity_id={} action={} persisted={} reason={}",
+            entry.entity_type, entry.entity_id, entry.action, entry.persisted, entry.reason
+        );
+    }
     if activity.len() >= ACTIVITY_RING_CAPACITY {
         activity.pop_front();
     }

src/rust/lqosd/src/treeguard/decisions.rs

@@ -436,8 +436,11 @@ mod tests {
     }
 
     #[test]
-    fn link_does_not_virtualize_when_cpu_low() {
-        let cpu = TreeguardCpuConfig::default();
+    fn link_does_not_virtualize_when_cpu_low_in_cpu_aware_mode() {
+        let cpu = TreeguardCpuConfig {
+            mode: TreeguardCpuMode::CpuAware,
+            ..TreeguardCpuConfig::default()
+        };
         let links = TreeguardLinksConfig::default();
         let qoo_cfg = TreeguardQooConfig::default();
         let state = LinkState::default();
@@ -460,6 +463,34 @@ mod tests {
         assert_eq!(decision, LinkVirtualDecision::NoChange);
     }
 
+    #[test]
+    fn link_virtualizes_when_cpu_low_in_traffic_rtt_only_mode() {
+        let cpu = TreeguardCpuConfig::default();
+        let links = TreeguardLinksConfig::default();
+        let qoo_cfg = TreeguardQooConfig::default();
+        let state = LinkState::default();
+        let decision = decide_link_virtualization(LinkVirtualizationInput {
+            now_unix: 1000,
+            allowlisted: true,
+            cpu_max_pct: Some(10),
+            cpu_cfg: &cpu,
+            links_cfg: &links,
+            qoo_cfg: &qoo_cfg,
+            rtt_missing: false,
+            qoo: DownUpOrder {
+                down: Some(100.0),
+                up: Some(100.0),
+            },
+            util_ewma_pct: DownUpOrder { down: 1.0, up: 1.0 },
+            sustained_idle: true,
+            state: &state,
+        });
+        assert_eq!(
+            decision,
+            LinkVirtualDecision::Set(LinkVirtualState::Virtual)
+        );
+    }
+
     #[test]
     fn link_unvirtualizes_on_util_spike() {
         let cpu = TreeguardCpuConfig::default();
@@ -694,8 +725,11 @@ mod tests {
     }
 
     #[test]
-    fn circuit_reverts_when_cpu_low() {
-        let cpu = TreeguardCpuConfig::default();
+    fn circuit_reverts_when_cpu_low_in_cpu_aware_mode() {
+        let cpu = TreeguardCpuConfig {
+            mode: TreeguardCpuMode::CpuAware,
+            ..TreeguardCpuConfig::default()
+        };
         let circuits = TreeguardCircuitsConfig::default();
         let qoo_cfg = TreeguardQooConfig::default();
         let state = CircuitState {
@@ -727,6 +761,40 @@ mod tests {
         assert_eq!(decision.up, Some(CircuitSqmState::Cake));
     }
 
+    #[test]
+    fn circuit_does_not_revert_when_cpu_low_in_traffic_rtt_only_mode() {
+        let cpu = TreeguardCpuConfig::default();
+        let circuits = TreeguardCircuitsConfig::default();
+        let qoo_cfg = TreeguardQooConfig::default();
+        let state = CircuitState {
+            down: CircuitDirectionState {
+                desired: CircuitSqmState::FqCodel,
+                ..Default::default()
+            },
+            up: CircuitDirectionState {
+                desired: CircuitSqmState::FqCodel,
+                ..Default::default()
+            },
+        };
+
+        let decision = decide_circuit_sqm(CircuitSqmInput {
+            now_unix: 1000,
+            allowlisted: true,
+            cpu_max_pct: Some(10),
+            cpu_cfg: &cpu,
+            circuits_cfg: &circuits,
+            qoo_cfg: &qoo_cfg,
+            rtt_missing: false,
+            qoo: DownUpOrder {
+                down: Some(90.0),
+                up: Some(90.0),
+            },
+            state: &state,
+        });
+        assert_eq!(decision.down, None);
+        assert_eq!(decision.up, None);
+    }
+
     #[test]
     fn circuit_missing_rtt_does_not_block_downgrade_when_idle() {
         let cpu = TreeguardCpuConfig::default();