From 198f1405c2bdfa6666ddca8c4ecf8b07ba6aa0b2 Mon Sep 17 00:00:00 2001 From: Robert Chacon Date: Sat, 4 Apr 2026 18:11:17 -0600 Subject: [PATCH] changes --- docs-es/v2.0/TechnicalDocs/integrations.md | 4 +- docs-es/v2.0/integrations-reference-es.md | 6 +- docs/v2.0/integrations-reference.md | 6 +- docs/v2.0/integrations-uisp.md | 6 +- .../src/strategies/full/routes_override.rs | 313 +++++++++++++++--- .../uisp_integration/src/strategies/full2.rs | 213 ++++++++++-- .../src/strategies/full2/net_json_parent.rs | 257 +++++++++++++- 7 files changed, 712 insertions(+), 93 deletions(-) diff --git a/docs-es/v2.0/TechnicalDocs/integrations.md b/docs-es/v2.0/TechnicalDocs/integrations.md index c1190990..9542837e 100644 --- a/docs-es/v2.0/TechnicalDocs/integrations.md +++ b/docs-es/v2.0/TechnicalDocs/integrations.md @@ -144,10 +144,12 @@ You have the option to run `uisp_integration` automatically on boot and every X Puede usar las siguientes entradas de override para reflejar su red con mayor precisión: - `Rate Override` en la página del árbol, guardado como overrides operatorios `AdjustSiteSpeed` en `lqos_overrides.json` - `Topology Override` en la página del árbol para nodos compatibles de UISP `full`, guardado en `lqos_overrides.json` -- integrationUISProutes.csv +- `uisp.route_overrides` en `lqos_overrides.json` +- integrationUISProutes.csv solo como entrada heredada de compatibilidad - integrationUISPbandwidths.csv solo como entrada heredada de compatibilidad Las compilaciones UISP actuales auto-migran un `integrationUISPbandwidths.csv` heredado hacia overrides operatorios `AdjustSiteSpeed` en la siguiente ejecución de la integración cuando todavía no existen overrides de tasa del operador. Si ya existen, el CSV se ignora. +Las compilaciones UISP actuales también auto-migran un `integrationUISProutes.csv` heredado hacia `uisp.route_overrides` en `lqos_overrides.json` en la siguiente ejecución de la integración cuando todavía no existen overrides de ruta en JSON. Si ya existen overrides de ruta en JSON, el CSV se ignora. Las plantillas de los archivos heredados siguen disponibles en `/opt/libreqos/src`. Si no las encuentra allí, puede obtenerlas [aquí](https://github.com/LibreQoE/LibreQoS/tree/develop/src). Para cambios nuevos de ancho de banda, prefiera los overrides operatorios en `lqos_overrides.json`. diff --git a/docs-es/v2.0/integrations-reference-es.md b/docs-es/v2.0/integrations-reference-es.md index 7fd3527b..e9e97e24 100644 --- a/docs-es/v2.0/integrations-reference-es.md +++ b/docs-es/v2.0/integrations-reference-es.md @@ -339,6 +339,8 @@ Si ya existe un archivo network.json, no será sobrescrito a menos que configure ShapedDevices.csv será sobrescrito cada vez que se ejecute la integración de UISP. +Si UISP expone un sitio y un AP con el mismo nombre visible dentro de la misma topología, las compilaciones actuales conservan el nombre del sitio en `network.json` y diferencian el nombre exportado del AP para que la rama del sitio no desaparezca del árbol. + Para asegurarse de que network.json siempre sea sobrescrito con la versión más reciente obtenida por la integración, edite el archivo `/etc/lqos.conf` con el comando `sudo nano /etc/lqos.conf` y configure el valor `always_overwrite_network_json` a `true`. Luego ejecute: `sudo systemctl restart lqosd`. @@ -349,12 +351,14 @@ Tiene la opción de ejecutar `uisp_integration` automáticamente al iniciar el e Puede usar las siguientes entradas de override para reflejar su red con mayor precisión: - `Rate Override` para cambios de ancho de banda a nivel de nodo guardados como entradas operatorias `AdjustSiteSpeed` en `lqos_overrides.json` - `Topology Override` para correcciones de selección de padre en UISP `full`, también guardadas en `lqos_overrides.json` -- integrationUISProutes.csv +- `uisp.route_overrides` en `lqos_overrides.json` +- integrationUISProutes.csv solo como entrada heredada de compatibilidad - integrationUISPbandwidths.csv solo como entrada heredada de compatibilidad Las compilaciones actuales aplican `Topology Override` antes de generar `network.json` y `ShapedDevices.csv`, y le dan precedencia sobre los overrides heredados de costos de ruta de UISP. El soporte actual en WebUI es solo `Pinned Parent`, forzando un único padre inmediato detectado. Las compilaciones UISP actuales también auto-migran un `integrationUISPbandwidths.csv` heredado hacia overrides operatorios `AdjustSiteSpeed` en la siguiente ejecución de la integración cuando todavía no existen overrides de tasa del operador. Si ya existen, el CSV se ignora. +Las compilaciones UISP actuales también auto-migran un `integrationUISProutes.csv` heredado hacia `uisp.route_overrides` en `lqos_overrides.json` en la siguiente ejecución de la integración cuando todavía no existen overrides de ruta en JSON. Si ya existen overrides de ruta en JSON, el CSV se ignora. Cada uno de los archivos mencionados arriba tienen plantillas, las cuales están disponibles en la carpeta `/opt/libreqos/src`. Si no los encuentra, puede obtenerlos [aquí](https://github.com/LibreQoE/LibreQoS/tree/develop/src). Para utilizarlos, copie el archivo (eliminando la parte `.template` del nombre) y edítelo con la información correspondiente. Para cambios nuevos de ancho de banda, prefiera los overrides operatorios en `lqos_overrides.json`. `integrationUISPbandwidths.csv` queda como una entrada heredada para migración única, no como el flujo preferido a largo plazo. diff --git a/docs/v2.0/integrations-reference.md b/docs/v2.0/integrations-reference.md index 8a71a050..2cef60c2 100644 --- a/docs/v2.0/integrations-reference.md +++ b/docs/v2.0/integrations-reference.md @@ -342,6 +342,8 @@ If a network.json file exists, it will not be overwritten, unless you set ```alw ShapedDevices.csv will be overwritten every time the UISP integration is run. +If UISP exposes a site and an AP with the same visible name in the same topology, current builds keep the site name stable in `network.json` and disambiguate the AP name during export so the site branch is not dropped from the tree. + To ensure the network.json is always overwritten with the newest version pulled in by the integration, please edit `/etc/lqos.conf` with the command `sudo nano /etc/lqos.conf`. Edit the file to set the value of `always_overwrite_network_json` to `true`. Then, run `sudo systemctl restart lqosd`. @@ -353,12 +355,14 @@ You have the option to run `uisp_integration` automatically on boot and every X You can also use the following override inputs to more accurately reflect your network: - `Rate Override` for node bandwidth changes stored as operator `AdjustSiteSpeed` entries in `lqos_overrides.json` - `Topology Override` for UISP `full` parent-selection corrections stored in `lqos_overrides.json` -- integrationUISProutes.csv +- `uisp.route_overrides` in `lqos_overrides.json` +- integrationUISProutes.csv as a legacy compatibility input only - integrationUISPbandwidths.csv as a legacy compatibility input only Current builds apply `Topology Override` before final `network.json` / `ShapedDevices.csv` emission and give it precedence over legacy UISP route-cost overrides. Current WebUI support is `Pinned Parent` only, forcing one detected immediate upstream parent. Current UISP builds also auto-migrate a legacy `integrationUISPbandwidths.csv` into operator `AdjustSiteSpeed` overrides on the next integration run when no operator rate overrides exist yet. If operator rate overrides already exist, the CSV is ignored. +Current UISP builds also auto-migrate a legacy `integrationUISProutes.csv` into `uisp.route_overrides` in `lqos_overrides.json` on the next integration run when no JSON route overrides exist yet. If JSON route overrides already exist, the CSV is ignored. Each of the files above have templates available in the `/opt/libreqos/src` folder. If you don't find them there, you can navigate [here](https://github.com/LibreQoE/LibreQoS/tree/develop/src). diff --git a/docs/v2.0/integrations-uisp.md b/docs/v2.0/integrations-uisp.md index adec16f9..9938fc2a 100644 --- a/docs/v2.0/integrations-uisp.md +++ b/docs/v2.0/integrations-uisp.md @@ -179,6 +179,8 @@ If a `network.json` file exists, it is only overwritten when `always_overwrite_n ShapedDevices.csv will be overwritten every time the UISP integration is run. +If UISP exposes a site and an AP with the same visible name in the same topology, current builds keep the site name stable in `network.json` and disambiguate the AP name during export so the site branch is not dropped from the tree. + When UISP client sites share the same name, LibreQoS now tries to disambiguate the generated circuit/site display names with a human-friendly suffix such as the first street-address segment, falling back to service name and then a short ID only when needed. Stable circuit identity still comes from the UISP site/service ID, not the display name. For integration-driven deployments, keep `always_overwrite_network_json = true` so topology stays aligned with UISP on each refresh cycle. @@ -190,10 +192,12 @@ You have the option to run `uisp_integration` automatically on boot and every X You can also use the following override inputs to more accurately reflect your network: - Tree-page `Rate Override` edits, stored as operator `AdjustSiteSpeed` entries in `lqos_overrides.json` - Tree-page `Topology Override` edits for supported UISP `full` nodes, stored in `lqos_overrides.json` -- integrationUISProutes.csv +- `uisp.route_overrides` in `lqos_overrides.json` +- integrationUISProutes.csv as a legacy compatibility input only - integrationUISPbandwidths.csv as a legacy compatibility input only Current UISP builds auto-migrate a legacy `integrationUISPbandwidths.csv` into operator `AdjustSiteSpeed` overrides on the next integration run when no operator rate overrides exist yet. If operator rate overrides already exist, the CSV is ignored and a warning is logged so there is only one active source of truth. +Current UISP builds also auto-migrate a legacy `integrationUISProutes.csv` into `uisp.route_overrides` in `lqos_overrides.json` on the next integration run when no JSON route overrides exist yet. If JSON route overrides already exist, the CSV is ignored and a warning is logged. UISP `full` strategy builds also expose tree-page `Topology Override` editing for supported nodes. These overrides are stored in `lqos_overrides.json`, take precedence over legacy UISP route-cost files, and resolve before final `network.json` / `ShapedDevices.csv` emission. Current WebUI support is `Pinned Parent` only. diff --git a/src/rust/uisp_integration/src/strategies/full/routes_override.rs b/src/rust/uisp_integration/src/strategies/full/routes_override.rs index 4b8e9af9..5b8f0a22 100644 --- a/src/rust/uisp_integration/src/strategies/full/routes_override.rs +++ b/src/rust/uisp_integration/src/strategies/full/routes_override.rs @@ -1,10 +1,15 @@ use crate::errors::UispIntegrationError; use csv::ReaderBuilder; use lqos_config::Config; +use lqos_overrides::OverrideFile; use serde::{Deserialize, Serialize}; -use std::path::Path; +use std::fs; +use std::path::{Path, PathBuf}; use std::sync::Arc; -use tracing::{error, info}; +use std::time::{SystemTime, UNIX_EPOCH}; +use tracing::{error, info, warn}; + +const LEGACY_UISP_ROUTES_FILE: &str = "integrationUISProutes.csv"; /// Represents a route override in the integrationUISProutes.csv file. #[derive(Serialize, Deserialize, Debug)] @@ -17,9 +22,11 @@ pub struct RouteOverride { pub cost: u32, } -/// Attempts to load integrationUISProutes.csv to use for -/// route overrides. Returns an empty set if not found. -/// Returns an error if the file is found but cannot be read. +/// Attempts to load UISP route overrides. +/// +/// Current precedence: +/// 1. `uisp.route_overrides` from `lqos_overrides.json` +/// 2. One-time migration from `integrationUISProutes.csv` into `lqos_overrides.json` /// /// The file should be a CSV with the following columns: /// @@ -40,66 +47,186 @@ pub struct RouteOverride { /// * An `Ok(Vec)` of `RouteOverride` objects /// * An `Err` if the file is found but cannot be read pub fn get_route_overrides(config: &Config) -> Result, UispIntegrationError> { - // Prefer overrides from lqos_overrides.json if present - if let Ok(of) = lqos_overrides::OverrideFile::load() - && let Some(uisp) = of.uisp() + let operator_path = OverrideFile::operator_path_for_config(config); + let mut operator_overrides = load_operator_override_file(&operator_path).map_err(|err| { + error!( + "Unable to load operator overrides from {}", + operator_path.display() + ); + error!("{err:?}"); + UispIntegrationError::CsvError + })?; + + if let Some(uisp) = operator_overrides.uisp() && !uisp.route_overrides.is_empty() { info!("Using UISP route overrides from lqos_overrides.json"); - let converted: Vec = uisp - .route_overrides - .iter() - .map(|r| RouteOverride { - from_site: r.from_site.clone(), - to_site: r.to_site.clone(), - cost: r.cost, - }) - .collect(); - return Ok(converted); + let legacy_path = legacy_routes_csv_path(config); + if legacy_path.exists() { + warn!( + "Legacy {} is present but UISP route overrides already exist in lqos_overrides.json; the CSV is ignored.", + LEGACY_UISP_ROUTES_FILE + ); + } + return Ok(materialize_uisp_route_overrides(&operator_overrides)); } - let file_path = Path::new(&config.lqos_directory).join("integrationUISProutes.csv"); - if file_path.exists() { - let reader = ReaderBuilder::new() - .has_headers(false) - .comment(Some(b'#')) - .trim(csv::Trim::All) - .from_path(file_path); - if reader.is_err() { - error!("Unable to read integrationUISProutes.csv"); - error!("{:?}", reader); + if let Some(migrated) = + try_migrate_legacy_csv_to_operator_overrides(config, &mut operator_overrides)? + { + let materialized = materialize_uisp_route_overrides(&operator_overrides); + if materialized.is_empty() { + return Ok(migrated); + } + return Ok(materialized); + } + + info!("No UISP route overrides loaded."); + Ok(Vec::new()) +} + +fn load_operator_override_file(operator_path: &Path) -> anyhow::Result { + if !operator_path.exists() { + return Ok(OverrideFile::default()); + } + OverrideFile::load_from_explicit_path(operator_path) +} + +fn legacy_routes_csv_path(config: &Config) -> PathBuf { + Path::new(&config.lqos_directory).join(LEGACY_UISP_ROUTES_FILE) +} + +fn materialize_uisp_route_overrides(overrides: &OverrideFile) -> Vec { + let Some(uisp) = overrides.uisp() else { + return Vec::new(); + }; + + uisp.route_overrides + .iter() + .map(|r| RouteOverride { + from_site: r.from_site.clone(), + to_site: r.to_site.clone(), + cost: r.cost, + }) + .collect() +} + +fn try_migrate_legacy_csv_to_operator_overrides( + config: &Config, + operator_overrides: &mut OverrideFile, +) -> Result>, UispIntegrationError> { + let file_path = legacy_routes_csv_path(config); + if !file_path.exists() { + return Ok(None); + } + + info!("Looking for {LEGACY_UISP_ROUTES_FILE}"); + let migrated = load_route_overrides_from_csv(&file_path)?; + if migrated.is_empty() { + warn!( + "Legacy {} exists but no usable rows were found; leaving the file untouched.", + LEGACY_UISP_ROUTES_FILE + ); + return Ok(Some(Vec::new())); + } + + for route in &migrated { + operator_overrides.add_uisp_route_override( + route.from_site.clone(), + route.to_site.clone(), + route.cost, + ); + } + + let operator_path = OverrideFile::operator_path_for_config(config); + if let Err(err) = operator_overrides.save_to_explicit_path(&operator_path) { + warn!( + "Unable to save migrated UISP route overrides to {}: {err:?}. Falling back to the legacy CSV for this run.", + operator_path.display() + ); + return Ok(Some(migrated)); + } + + if let Err(err) = rename_legacy_csv_to_backup(&file_path) { + warn!( + "Migrated legacy UISP route overrides into lqos_overrides.json, but could not rename {} to a backup: {err:?}", + file_path.display() + ); + } else { + info!( + "Migrated legacy {} into UISP route overrides in lqos_overrides.json", + LEGACY_UISP_ROUTES_FILE + ); + } + + Ok(Some(migrated)) +} + +fn load_route_overrides_from_csv( + file_path: &Path, +) -> Result, UispIntegrationError> { + let reader = ReaderBuilder::new() + .has_headers(false) + .comment(Some(b'#')) + .trim(csv::Trim::All) + .from_path(file_path); + let mut reader = match reader { + Ok(reader) => reader, + Err(err) => { + error!("Unable to read {}", file_path.display()); + error!("{err:?}"); return Err(UispIntegrationError::CsvError); } - let mut reader = reader.unwrap(); - let mut overrides = Vec::new(); + }; - for rec in reader.records() { - if let Ok(line) = rec { - if line.len() == 3 { - println!("{line:?}"); - // We got a line - if let Ok(cost) = &line[2].parse::() { - overrides.push(RouteOverride { - from_site: line[0].to_string(), - to_site: line[1].to_string(), - cost: *cost, - }); - } else { - error!("{line:?} is not a valid integer for cost"); - } - } - } else { - error!("Unable to read route overrides CSV"); - error!("{rec:?}"); + let mut overrides = Vec::new(); + for (line_num, rec) in reader.records().enumerate() { + if let Ok(line) = rec { + if line.len() != 3 { + error!( + "Wrong number of records in {} on line {}", + file_path.display(), + line_num + ); + continue; } - } - info!("Loaded {} route overrides", overrides.len()); - Ok(overrides) - } else { - info!("No integrationUISProutes.csv found - no route overrides loaded."); - Ok(Vec::new()) + if let Ok(cost) = line[2].parse::() { + overrides.push(RouteOverride { + from_site: line[0].to_string(), + to_site: line[1].to_string(), + cost, + }); + } else { + error!( + "{} is not a valid integer for cost on line {}", + &line[2], line_num + ); + } + } else { + error!( + "Unable to read route overrides CSV line from {}", + file_path.display() + ); + error!("{rec:?}"); + } } + + info!( + "Loaded {} legacy UISP route override row(s)", + overrides.len() + ); + Ok(overrides) +} + +fn rename_legacy_csv_to_backup(file_path: &Path) -> std::io::Result<()> { + let unix_secs = SystemTime::now() + .duration_since(UNIX_EPOCH) + .unwrap_or_default() + .as_secs(); + let backup_name = format!("{LEGACY_UISP_ROUTES_FILE}.migrated-{unix_secs}"); + let backup_path = file_path.with_file_name(backup_name); + fs::rename(file_path, backup_path) } #[allow(dead_code)] @@ -116,3 +243,81 @@ pub fn write_routing_overrides_template( writer.flush()?; Ok(()) } + +#[cfg(test)] +mod tests { + use super::*; + + fn unique_temp_dir(name: &str) -> PathBuf { + let unique = format!( + "libreqos-uisp-route-{}-{}-{}", + name, + std::process::id(), + SystemTime::now() + .duration_since(UNIX_EPOCH) + .unwrap_or_default() + .as_nanos() + ); + let path = std::env::temp_dir().join(unique); + fs::create_dir_all(&path).expect("temp dir should be created"); + path + } + + fn config_for_dir(dir: &Path) -> Config { + Config { + lqos_directory: dir.to_string_lossy().into_owned(), + ..Config::default() + } + } + + #[test] + fn operator_route_overrides_take_precedence() { + let dir = unique_temp_dir("operator-precedence"); + let config = config_for_dir(&dir); + let operator_path = OverrideFile::operator_path_for_config(&config); + + let mut overrides = OverrideFile::default(); + overrides.add_uisp_route_override("A".to_string(), "B".to_string(), 42); + overrides + .save_to_explicit_path(&operator_path) + .expect("operator overrides should save"); + fs::write(legacy_routes_csv_path(&config), "A,B,10\n").expect("legacy csv should write"); + + let loaded = get_route_overrides(&config).expect("overrides should load"); + assert_eq!(loaded.len(), 1); + assert_eq!(loaded[0].from_site, "A"); + assert_eq!(loaded[0].to_site, "B"); + assert_eq!(loaded[0].cost, 42); + assert!(legacy_routes_csv_path(&config).exists()); + } + + #[test] + fn legacy_route_csv_is_migrated_into_json_overrides() { + let dir = unique_temp_dir("csv-migrate"); + let config = config_for_dir(&dir); + fs::write(legacy_routes_csv_path(&config), "A,B,10\nB,C,25\n") + .expect("legacy csv should write"); + + let loaded = get_route_overrides(&config).expect("overrides should load"); + assert_eq!(loaded.len(), 2); + assert_eq!(loaded[0].from_site, "A"); + assert_eq!(loaded[0].to_site, "B"); + assert_eq!(loaded[0].cost, 10); + assert_eq!(loaded[1].from_site, "B"); + assert_eq!(loaded[1].to_site, "C"); + assert_eq!(loaded[1].cost, 25); + assert!(!legacy_routes_csv_path(&config).exists()); + + let operator_path = OverrideFile::operator_path_for_config(&config); + let saved = OverrideFile::load_from_explicit_path(&operator_path) + .expect("saved overrides should load"); + let materialized = materialize_uisp_route_overrides(&saved); + assert_eq!(materialized.len(), 2); + assert_eq!(materialized[0].from_site, "A"); + assert_eq!(materialized[0].to_site, "B"); + assert_eq!(materialized[0].cost, 10); + assert_eq!(materialized[1].from_site, "B"); + assert_eq!(materialized[1].to_site, "C"); + assert_eq!(materialized[1].cost, 25); + } +} diff --git a/src/rust/uisp_integration/src/strategies/full2.rs b/src/rust/uisp_integration/src/strategies/full2.rs index d113eb10..0a63d612 100644 --- a/src/rust/uisp_integration/src/strategies/full2.rs +++ b/src/rust/uisp_integration/src/strategies/full2.rs @@ -18,7 +18,7 @@ use crate::strategies::full2::directionality::{ use crate::strategies::full2::dot::save_dot_file; use crate::strategies::full2::graph_mapping::GraphMapping; use crate::strategies::full2::link_mapping::LinkMapping; -use crate::strategies::full2::net_json_parent::{NetJsonParent, walk_parents}; +use crate::strategies::full2::net_json_parent::{NetJsonParent, assign_export_names, walk_parents}; use crate::uisp_types::UispDevice; use lqos_config::{ CircuitEthernetMetadata, Config, EthernetPortLimitPolicy, RequestedCircuitRates, @@ -208,6 +208,17 @@ pub async fn build_full_network_v2( save_dot_file(&graph)?; let _ = blackboard_blob("uisp-graph", vec![graph.clone()]).await; + let export_names = assign_export_names( + graph.node_indices() + .map(|node| (graph[node].network_json_id(), &graph[node])), + ); + let root_node_id = graph[root_idx].network_json_id(); + let orphans_node_id = graph[orphans].network_json_id(); + let orphans_node_name = export_names + .get(&orphans_node_id) + .cloned() + .unwrap_or_else(|| graph[orphans].name()); + // Figure out the network.json layers let mut parents = HashMap::::new(); let mut topology_parent_candidates = Vec::::new(); @@ -219,6 +230,11 @@ pub async fn build_full_network_v2( GraphMapping::GeneratedSite { name } | GraphMapping::Site { name, .. } | GraphMapping::AccessPoint { name, .. } => { + let node_id = graph[node].network_json_id(); + let export_name = export_names + .get(&node_id) + .cloned() + .unwrap_or_else(|| name.to_owned()); let mut route_from_root_to_node = astar_route( &graph, root_idx, @@ -245,9 +261,13 @@ pub async fn build_full_network_v2( if route_from_root_to_node.is_empty() { //println!("No path detected from {:?} to {}", graph[node], root_site_name); parents.insert( - name.to_owned(), + node_id.clone(), NetJsonParent { - parent_name: "Orphans".to_string(), + node_id: node_id.clone(), + node_name: name.to_owned(), + export_name: export_name.clone(), + parent_id: Some(orphans_node_id.clone()), + parent_name: orphans_node_name.clone(), mapping: &graph[node], download: config.queues.generated_pn_download_mbps, upload: config.queues.generated_pn_upload_mbps, @@ -347,10 +367,15 @@ pub async fn build_full_network_v2( }; // We need the weight from node to parent_node in the graph edges if let Some(_edge) = graph.find_edge(parent_node, node) { + let parent_id = graph[parent_node].network_json_id(); let parent = graph[parent_node].name(); + let parent_export_name = export_names + .get(&parent_id) + .cloned() + .unwrap_or_else(|| parent.clone()); let current_parent = if is_real_topology_node(&graph, parent_node) { Some(TopologyParentCandidate { - node_id: graph[parent_node].network_json_id(), + node_id: parent_id.clone(), node_name: parent.clone(), }) } else { @@ -380,9 +405,13 @@ pub async fn build_full_network_v2( }); } parents.insert( - name.to_owned(), + node_id.clone(), NetJsonParent { - parent_name: parent, + node_id: node_id.clone(), + node_name: name.to_owned(), + export_name: export_name.clone(), + parent_id: Some(parent_id), + parent_name: parent_export_name, mapping: &graph[node], download: download_capacity, upload: upload_capacity, @@ -416,13 +445,33 @@ pub async fn build_full_network_v2( // Write the network.json file let mut network_json = serde_json::Map::new(); let mut visited = HashSet::new(); - for (name, node_info) in parents.iter().filter(|(_name, parent)| { - // Include if it's a direct child of root OR if it's in promote_to_root list - parent.parent_name == root_site_name || promote_to_root_set.contains(_name.as_str()) - }) { + let mut root_child_ids: Vec<&String> = parents + .iter() + .filter(|(_node_id, parent)| { + parent.parent_id.as_deref() == Some(root_node_id.as_str()) + || promote_to_root_set.contains(parent.node_name.as_str()) + }) + .map(|(node_id, _node_info)| node_id) + .collect(); + root_child_ids.sort_unstable_by(|left_id, right_id| { + let left = parents + .get(*left_id) + .expect("top-level node id should exist when sorting"); + let right = parents + .get(*right_id) + .expect("top-level node id should exist when sorting"); + left.export_name + .cmp(&right.export_name) + .then_with(|| left.node_id.cmp(&right.node_id)) + }); + for node_id in root_child_ids { + let node_info = parents + .get(node_id) + .expect("top-level node id should exist when building network.json"); + visited.insert(node_id.clone()); network_json.insert( - name.into(), - walk_parents(&parents, name, node_info, &mut visited).into(), + node_info.export_name.clone(), + walk_parents(&parents, node_id, &mut visited).into(), ); } let network_path = Path::new(&config.lqos_directory).join("network.json"); @@ -522,14 +571,17 @@ pub async fn build_full_network_v2( } let parent_node = { - if parents.contains_key(&ap_device.name) { - ap_device.name.clone() + let ap_node_id = format!("uisp:device:{}", ap_device.id); + if let Some(parent) = parents.get(&ap_node_id) { + parent.export_name.clone() } else { warn!( - "AP device '{}' not found in parents HashMap, assigning to Orphans", - ap_device.name + "AP device '{}' ({}) not found in parents HashMap, assigning to {}", + ap_device.name, + ap_device.id, + orphans_node_name ); - "Orphans".to_string() + orphans_node_name.clone() } }; @@ -926,7 +978,7 @@ fn topology_parent_candidates_for_node( .neighbors_directed(node, petgraph::Incoming) .filter(|candidate| is_real_topology_node(graph, *candidate)) .filter(|candidate| { - !astar_route(graph, root_idx, *candidate, devices, route_overrides).is_empty() + is_upstream_parent_candidate(graph, root_idx, node, *candidate, devices, route_overrides) }) .collect(); candidates.sort_unstable_by_key(|candidate| stable_node_key(graph, *candidate)); @@ -934,6 +986,31 @@ fn topology_parent_candidates_for_node( candidates } +fn is_upstream_parent_candidate( + graph: &GraphType, + root_idx: NodeIndex, + node: NodeIndex, + candidate: NodeIndex, + devices: &[UispDevice], + route_overrides: &[RouteOverride], +) -> bool { + if node == candidate { + return false; + } + + let path_to_candidate = astar_route(graph, root_idx, candidate, devices, route_overrides); + if path_to_candidate.is_empty() { + return false; + } + + // Topology parent overrides are meant to pin the immediate upstream hop. + // In the bidirectional UISP graph, direct children also appear as incoming + // neighbors, so exclude any candidate whose own path from root traverses the + // node being edited. Selecting those descendants can create a cycle-like + // reparenting result that causes the node to disappear from the exported tree. + !path_to_candidate.contains(&node) +} + fn immediate_parent_from_route(path: &[NodeIndex]) -> Option { if path.len() < 2 { None @@ -1143,8 +1220,8 @@ fn sorted_unique_neighbors(graph: &GraphType, node: NodeIndex) -> Vec #[cfg(test)] mod topology_override_tests { use super::{ - GraphType, TopologyParentOverrideSelection, resolve_parent_candidate, - topology_parent_candidates_for_node, + GraphType, TopologyParentOverrideSelection, is_upstream_parent_candidate, + resolve_parent_candidate, topology_parent_candidates_for_node, }; use crate::strategies::full2::graph_mapping::GraphMapping; use crate::strategies::full2::link_mapping::LinkMapping; @@ -1231,6 +1308,102 @@ mod topology_override_tests { let resolved = resolve_parent_candidate(&graph, t2, native_parent, &candidates, &overrides); assert_eq!(resolved, Some(t1)); } + + #[test] + fn child_access_points_are_not_offered_as_topology_parent_candidates() { + let mut graph = GraphType::new(); + let root = graph.add_node(GraphMapping::Root { + name: "Upstream".to_string(), + id: "root".to_string(), + latitude: None, + longitude: None, + }); + let target_site = graph.add_node(site("TrailerCity", "site-trailercity")); + let parent_site = graph.add_node(site("MRE", "site-mre")); + let child_ap = graph.add_node(GraphMapping::AccessPoint { + name: "JR_AP_TC_A".to_string(), + id: "device-jr-ap-tc-a".to_string(), + site_name: "TrailerCity".to_string(), + download_mbps: 1000, + upload_mbps: 1000, + }); + + for (from, to) in [ + (root, parent_site), + (parent_site, root), + (parent_site, target_site), + (target_site, parent_site), + (target_site, child_ap), + (child_ap, target_site), + ] { + graph.add_edge(from, to, LinkMapping::ethernet(1_000)); + } + + let candidates = topology_parent_candidates_for_node(&graph, root, target_site, &[], &[]); + assert_eq!(candidates, vec![parent_site]); + assert!(is_upstream_parent_candidate( + &graph, + root, + target_site, + parent_site, + &[], + &[], + )); + assert!(!is_upstream_parent_candidate( + &graph, + root, + target_site, + child_ap, + &[], + &[], + )); + } + + #[test] + fn legacy_bad_child_override_falls_back_to_native_parent() { + let mut graph = GraphType::new(); + let root = graph.add_node(GraphMapping::Root { + name: "Upstream".to_string(), + id: "root".to_string(), + latitude: None, + longitude: None, + }); + let parent_site = graph.add_node(site("MRE", "site-mre")); + let target_site = graph.add_node(site("TrailerCity", "site-trailercity")); + let child_ap = graph.add_node(GraphMapping::AccessPoint { + name: "JR_AP_TC_A".to_string(), + id: "device-jr-ap-tc-a".to_string(), + site_name: "TrailerCity".to_string(), + download_mbps: 1000, + upload_mbps: 1000, + }); + + for (from, to) in [ + (root, parent_site), + (parent_site, root), + (parent_site, target_site), + (target_site, parent_site), + (target_site, child_ap), + (child_ap, target_site), + ] { + graph.add_edge(from, to, LinkMapping::ethernet(1_000)); + } + + let candidates = topology_parent_candidates_for_node(&graph, root, target_site, &[], &[]); + let native_parent = Some(parent_site); + let mut overrides = HashMap::new(); + overrides.insert( + graph[target_site].network_json_id(), + TopologyParentOverrideSelection { + mode: TopologyParentOverrideMode::Pinned, + parent_node_ids: vec![graph[child_ap].network_json_id()], + }, + ); + + let resolved = + resolve_parent_candidate(&graph, target_site, native_parent, &candidates, &overrides); + assert_eq!(resolved, Some(parent_site)); + } } fn total_degree(graph: &GraphType, node: NodeIndex) -> usize { diff --git a/src/rust/uisp_integration/src/strategies/full2/net_json_parent.rs b/src/rust/uisp_integration/src/strategies/full2/net_json_parent.rs index 8951f9ef..5793f45f 100644 --- a/src/rust/uisp_integration/src/strategies/full2/net_json_parent.rs +++ b/src/rust/uisp_integration/src/strategies/full2/net_json_parent.rs @@ -3,21 +3,97 @@ use std::collections::{HashMap, HashSet}; #[derive(Debug)] pub struct NetJsonParent<'a> { + pub node_id: String, + pub node_name: String, + pub export_name: String, + pub parent_id: Option, pub parent_name: String, pub mapping: &'a GraphMapping, pub download: u64, pub upload: u64, } +fn export_name_candidates(base_name: &str, mapping: &GraphMapping, node_id: &str) -> Vec { + let kind_label = match mapping { + GraphMapping::AccessPoint { .. } => "AP", + GraphMapping::Root { .. } | GraphMapping::Site { .. } | GraphMapping::GeneratedSite { .. } => { + "Site" + } + }; + let short_id = node_id.rsplit(':').next().unwrap_or(node_id); + let short_id = &short_id[..short_id.len().min(8)]; + + match mapping { + GraphMapping::AccessPoint { .. } => vec![ + format!("{base_name} [AP]"), + format!("{base_name} [AP {short_id}]"), + ], + GraphMapping::Root { .. } | GraphMapping::Site { .. } | GraphMapping::GeneratedSite { .. } => { + vec![ + base_name.to_string(), + format!("{base_name} [Site]"), + format!("{base_name} [Site {short_id}]"), + format!("{base_name} [{kind_label} {short_id}]"), + ] + } + } +} + +pub fn assign_export_names<'a, I>(nodes: I) -> HashMap +where + I: IntoIterator, +{ + let mut entries: Vec<(String, &'a GraphMapping)> = nodes.into_iter().collect(); + let mut name_counts = HashMap::::new(); + for (_node_id, mapping) in &entries { + *name_counts.entry(mapping.name()).or_insert(0) += 1; + } + + entries.sort_unstable_by(|(left_id, left_mapping), (right_id, right_mapping)| { + left_mapping + .name() + .cmp(&right_mapping.name()) + .then_with(|| match (left_mapping, right_mapping) { + (GraphMapping::AccessPoint { .. }, GraphMapping::AccessPoint { .. }) => { + left_id.cmp(right_id) + } + (GraphMapping::AccessPoint { .. }, _) => std::cmp::Ordering::Greater, + (_, GraphMapping::AccessPoint { .. }) => std::cmp::Ordering::Less, + _ => left_id.cmp(right_id), + }) + }); + + let mut export_names = HashMap::with_capacity(entries.len()); + let mut used_names = HashSet::with_capacity(entries.len()); + + for (node_id, mapping) in entries { + let base_name = mapping.name(); + let export_name = if name_counts.get(&base_name).copied().unwrap_or(0) <= 1 { + base_name + } else { + export_name_candidates(&base_name, mapping, &node_id) + .into_iter() + .find(|candidate| !used_names.contains(candidate)) + .unwrap_or_else(|| format!("{base_name} [{}]", node_id)) + }; + used_names.insert(export_name.clone()); + export_names.insert(node_id, export_name); + } + + export_names +} + pub fn walk_parents( parents: &HashMap, - name: &String, - node_info: &NetJsonParent, + node_id: &str, visited: &mut HashSet, ) -> serde_json::Map { + let node_info = parents + .get(node_id) + .expect("walk_parents requires the current node to exist in the parents map"); let mut map = serde_json::Map::new(); // Entries are name, type, uisp_device or site, downloadBandwidthMbps, uploadBandwidthMbps, children - map.insert("name".into(), name.clone().into()); + map.insert("name".into(), node_info.export_name.clone().into()); map.insert("id".into(), node_info.mapping.network_json_id().into()); map.insert("downloadBandwidthMbps".into(), node_info.download.into()); map.insert("uploadBandwidthMbps".into(), node_info.upload.into()); @@ -64,16 +140,32 @@ pub fn walk_parents( } let mut children = serde_json::Map::new(); - for (name, node_info) in parents + let mut child_ids: Vec<&str> = parents .iter() - .filter(|(_node_name, node_info)| node_info.parent_name == *name) - { - if visited.contains(name) { + .filter(|(_child_id, node_info)| node_info.parent_id.as_deref() == Some(node_id)) + .map(|(child_id, _node_info)| child_id.as_str()) + .collect(); + child_ids.sort_unstable_by(|left_id, right_id| { + let left = parents + .get(*left_id) + .expect("child id should exist when sorting walk_parents output"); + let right = parents + .get(*right_id) + .expect("child id should exist when sorting walk_parents output"); + left.export_name + .cmp(&right.export_name) + .then_with(|| left.node_id.cmp(&right.node_id)) + }); + for child_id in child_ids { + let child_info = parents + .get(child_id) + .expect("child id should exist when building walk_parents output"); + if visited.contains(child_id) { continue; } - visited.insert(name.to_string()); - let child = walk_parents(parents, name, node_info, visited); - children.insert(name.into(), child.into()); + visited.insert(child_id.to_string()); + let child = walk_parents(parents, child_id, visited); + children.insert(child_info.export_name.clone(), child.into()); } map.insert("children".into(), children.into()); @@ -83,7 +175,7 @@ pub fn walk_parents( #[cfg(test)] mod tests { - use super::{NetJsonParent, walk_parents}; + use super::{NetJsonParent, assign_export_names, walk_parents}; use crate::strategies::full2::graph_mapping::GraphMapping; use std::collections::{HashMap, HashSet}; @@ -108,11 +200,23 @@ mod tests { download_mbps: 5000, upload_mbps: 5000, }; + let export_names = assign_export_names([ + (root_mapping.network_json_id(), &root_mapping), + (child_site_mapping.network_json_id(), &child_site_mapping), + (child_ap_mapping.network_json_id(), &child_ap_mapping), + ]); + let root_id = root_mapping.network_json_id(); + let site_id = child_site_mapping.network_json_id(); + let ap_id = child_ap_mapping.network_json_id(); let mut parents = HashMap::new(); parents.insert( - "MDC El Paso".to_string(), + site_id.clone(), NetJsonParent { + node_id: site_id.clone(), + node_name: "MDC El Paso".to_string(), + export_name: export_names.get(&site_id).unwrap().clone(), + parent_id: Some(root_id.clone()), parent_name: "WestRedd-SiteRouter".to_string(), mapping: &child_site_mapping, download: 5000, @@ -120,8 +224,12 @@ mod tests { }, ); parents.insert( - "Core-WestRedd".to_string(), + ap_id.clone(), NetJsonParent { + node_id: ap_id.clone(), + node_name: "Core-WestRedd".to_string(), + export_name: export_names.get(&ap_id).unwrap().clone(), + parent_id: Some(site_id.clone()), parent_name: "MDC El Paso".to_string(), mapping: &child_ap_mapping, download: 5000, @@ -129,15 +237,19 @@ mod tests { }, ); - let root_name = "WestRedd-SiteRouter".to_string(); let root_node = NetJsonParent { + node_id: root_id.clone(), + node_name: "WestRedd-SiteRouter".to_string(), + export_name: export_names.get(&root_id).unwrap().clone(), + parent_id: None, parent_name: "WestRedd".to_string(), mapping: &root_mapping, download: 5000, upload: 5000, }; + parents.insert(root_id.clone(), root_node); - let tree = walk_parents(&parents, &root_name, &root_node, &mut HashSet::new()); + let tree = walk_parents(&parents, &root_id, &mut HashSet::new()); assert_eq!( tree.get("parent_site").and_then(|value| value.as_str()), Some("WestRedd") @@ -165,4 +277,119 @@ mod tests { Some("MDC El Paso") ); } + + #[test] + fn disambiguates_duplicate_site_and_ap_names_without_losing_the_site_branch() { + let root_mapping = GraphMapping::Root { + name: "MRE".to_string(), + id: "site-mre".to_string(), + latitude: None, + longitude: None, + }; + let trailer_site_mapping = GraphMapping::Site { + name: "TrailerCity".to_string(), + id: "site-trailercity".to_string(), + latitude: None, + longitude: None, + }; + let trailer_ap_mapping = GraphMapping::AccessPoint { + name: "TrailerCity".to_string(), + id: "device-trailercity".to_string(), + site_name: "TrailerCity".to_string(), + download_mbps: 1200, + upload_mbps: 1200, + }; + let child_ap_mapping = GraphMapping::AccessPoint { + name: "JR_AP_TC_A".to_string(), + id: "device-jr-ap-tc-a".to_string(), + site_name: "TrailerCity".to_string(), + download_mbps: 5000, + upload_mbps: 5000, + }; + + let export_names = assign_export_names([ + (root_mapping.network_json_id(), &root_mapping), + (trailer_site_mapping.network_json_id(), &trailer_site_mapping), + (trailer_ap_mapping.network_json_id(), &trailer_ap_mapping), + (child_ap_mapping.network_json_id(), &child_ap_mapping), + ]); + let root_id = root_mapping.network_json_id(); + let site_id = trailer_site_mapping.network_json_id(); + let trailer_ap_id = trailer_ap_mapping.network_json_id(); + let child_ap_id = child_ap_mapping.network_json_id(); + + assert_eq!(export_names.get(&site_id).map(String::as_str), Some("TrailerCity")); + assert_eq!( + export_names.get(&trailer_ap_id).map(String::as_str), + Some("TrailerCity [AP]") + ); + + let mut parents = HashMap::new(); + parents.insert( + root_id.clone(), + NetJsonParent { + node_id: root_id.clone(), + node_name: "MRE".to_string(), + export_name: export_names.get(&root_id).unwrap().clone(), + parent_id: None, + parent_name: "Root".to_string(), + mapping: &root_mapping, + download: 5000, + upload: 5000, + }, + ); + parents.insert( + site_id.clone(), + NetJsonParent { + node_id: site_id.clone(), + node_name: "TrailerCity".to_string(), + export_name: export_names.get(&site_id).unwrap().clone(), + parent_id: Some(root_id.clone()), + parent_name: "MRE".to_string(), + mapping: &trailer_site_mapping, + download: 1200, + upload: 1200, + }, + ); + parents.insert( + trailer_ap_id.clone(), + NetJsonParent { + node_id: trailer_ap_id.clone(), + node_name: "TrailerCity".to_string(), + export_name: export_names.get(&trailer_ap_id).unwrap().clone(), + parent_id: Some(site_id.clone()), + parent_name: "TrailerCity".to_string(), + mapping: &trailer_ap_mapping, + download: 1200, + upload: 1200, + }, + ); + parents.insert( + child_ap_id.clone(), + NetJsonParent { + node_id: child_ap_id.clone(), + node_name: "JR_AP_TC_A".to_string(), + export_name: export_names.get(&child_ap_id).unwrap().clone(), + parent_id: Some(site_id.clone()), + parent_name: "TrailerCity".to_string(), + mapping: &child_ap_mapping, + download: 5000, + upload: 5000, + }, + ); + + let tree = walk_parents(&parents, &root_id, &mut HashSet::new()); + let site = tree + .get("children") + .and_then(|value| value.as_object()) + .and_then(|children| children.get("TrailerCity")) + .and_then(|value| value.as_object()) + .expect("site branch should survive duplicate-name export"); + let children = site + .get("children") + .and_then(|value| value.as_object()) + .expect("site children should exist"); + assert!(children.contains_key("TrailerCity [AP]")); + assert!(children.contains_key("JR_AP_TC_A")); + } }