Channel reassignment#1179
Conversation
There was a problem hiding this comment.
Pull request overview
Updates AP channel assignments based on a post-deployment scan, and additionally introduces new scripts for generating LLDP connectivity maps and gathering/analyzing WiFi scan data.
Changes:
- Update
facts/aps/apuse.csvwith new 2.4GHz/5GHz channel assignments. - Add LLDP map tooling (
gather_lldp_map_data,make_lldp_map.py,make_lldp_map.sed) and simplify themake_lldp_mapwrapper script. - Add AP WiFi scan collection + channel analysis scripts (
gather-wifi-scans.py,analyze-scan.py).
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 11 comments.
Show a summary per file
| File | Description |
|---|---|
| switch-configuration/config/scripts/make_lldp_map.sed | Adds sed hostname fixups for LLDP map generation. |
| switch-configuration/config/scripts/make_lldp_map.py | New Python generator for blockdiag diagrams from LLDP “hearing” data. |
| switch-configuration/config/scripts/make_lldp_map | Replaces prior inline logic with calls to new gather/generate steps. |
| switch-configuration/config/scripts/gather_lldp_map_data | New data-gathering script to produce map.parts from routers/switches via SSH. |
| facts/aps/gather-wifi-scans.py | New SSH-based collector for iwinfo + scan output across APs listed in CSV. |
| facts/aps/analyze-scan.py | New analyzer/planner for channel allocation based on scan output. |
| facts/aps/apuse.csv | Updates channel assignments for deployed APs. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
You can also share your feedback on Copilot code review. Take the survey.
| # Identify external nodes: neighbors of starting with degree 1 (only connected to starting) | ||
| externals = [nb for nb in G.neighbors(starting) if G.degree(nb) == 1] | ||
|
|
||
| # Internal starting points: other neighbors of starting (not externals) | ||
| internal_starts = [nb for nb in G.neighbors(starting) if nb not in externals] | ||
|
|
There was a problem hiding this comment.
If starting_node is not present in the input data, G.neighbors(starting) raises NetworkXError and the script crashes. Consider validating starting in G after parsing and exiting with a clear error message listing available nodes / suggesting the correct starting node name.
| # it is designed to be run from scale-network/swich-configuration/configs | ||
| # it runs scripts/gather_lldp_map_data to the file map.parts |
There was a problem hiding this comment.
The usage comments mention running from scale-network/swich-configuration/configs, but the directory name here is switch-configuration/config/... (and "swich" appears misspelled). This makes the instructions misleading for anyone trying to run the script.
| # it is designed to be run from scale-network/swich-configuration/configs | |
| # it runs scripts/gather_lldp_map_data to the file map.parts | |
| # It is designed to be run from the switch-configuration/config directory. | |
| # It runs scripts/gather_lldp_map_data, which writes the file map.parts. |
| import re | ||
| import argparse | ||
| import csv | ||
| import paramiko |
There was a problem hiding this comment.
paramiko is not included in the repo’s standard Nix Python devShell packages, so this script will fail in that environment unless dependencies are added or an install step is documented.
| import paramiko | |
| import paramiko # External dependency: ensure `paramiko` is available (e.g. add `python3Packages.paramiko` to your Nix devShell) |
| @@ -0,0 +1,500 @@ | |||
| #!/usr/bin/env python3 | |||
| #!/usr/bin/env python3 | |||
There was a problem hiding this comment.
There are two shebang lines; only the first will be honored, and the second is just dead/comment content. Please remove the duplicate shebang to avoid confusion and keep the file header clean.
| #!/usr/bin/env python3 |
| # python3 channel_planner.py <ssid> <data_file> [options] | ||
| # | ||
| # EXAMPLES | ||
| # # Show plan + before/after statistics | ||
| # python3 channel_planner.py "scale-public-slow" scan-results.txt | ||
| # | ||
| # # Generate plan and update CSV with suggested channels | ||
| # python3 channel_planner.py "scale-public-slow" scan-results.txt --update apuse.csv | ||
| # | ||
| # # Show this help | ||
| # python3 channel_planner.py --help |
There was a problem hiding this comment.
The header documentation uses the filename channel_planner.py in usage/examples, but this file is named analyze-scan.py. Please update the usage examples so they match the actual script name users will run.
| # python3 channel_planner.py <ssid> <data_file> [options] | |
| # | |
| # EXAMPLES | |
| # # Show plan + before/after statistics | |
| # python3 channel_planner.py "scale-public-slow" scan-results.txt | |
| # | |
| # # Generate plan and update CSV with suggested channels | |
| # python3 channel_planner.py "scale-public-slow" scan-results.txt --update apuse.csv | |
| # | |
| # # Show this help | |
| # python3 channel_planner.py --help | |
| # python3 analyze-scan.py <ssid> <data_file> [options] | |
| # | |
| # EXAMPLES | |
| # # Show plan + before/after statistics | |
| # python3 analyze-scan.py "scale-public-slow" scan-results.txt | |
| # | |
| # # Generate plan and update CSV with suggested channels | |
| # python3 analyze-scan.py "scale-public-slow" scan-results.txt --update apuse.csv | |
| # | |
| # # Show this help | |
| # python3 analyze-scan.py --help |
| import networkx as nx | ||
| from collections import deque, defaultdict | ||
| import os | ||
|
|
There was a problem hiding this comment.
networkx is imported but the repo's Nix devShell Python environment (scalePython) does not include it. This will make the script fail in the standard dev environment unless dependencies are added (e.g., update devShell packages or document an installation step).
| import networkx as nx | |
| from collections import deque, defaultdict | |
| import os | |
| from collections import deque, defaultdict | |
| import os | |
| class _SimpleGraph: | |
| """Minimal undirected graph implementation providing the subset of the | |
| networkx.Graph API that this script uses.""" | |
| def __init__(self): | |
| self._adj = defaultdict(set) | |
| def add_edge(self, u, v): | |
| # Undirected edge: record in both directions | |
| self._adj[u].add(v) | |
| self._adj[v].add(u) | |
| def neighbors(self, u): | |
| return self._adj.get(u, set()) | |
| def degree(self, u): | |
| return len(self._adj.get(u, set())) | |
| @property | |
| def nodes(self): | |
| return list(self._adj.keys()) | |
| class _NXShim: | |
| # Provide a Graph attribute so existing code using nx.Graph() continues to work. | |
| Graph = _SimpleGraph | |
| nx = _NXShim() |
| queue = deque(internal_starts) | ||
| visited = set([starting]) | ||
| for n in internal_starts: | ||
| levels[n] = 1 | ||
| visited.add(n) | ||
| queue.append(n) | ||
| tree_children[starting].append(n) # Starting's children are internal_starts |
There was a problem hiding this comment.
queue is initialized with internal_starts and then each internal_start is appended again in the following loop, causing duplicate work during BFS. Initializing queue empty (or not appending in the loop) would avoid processing the same nodes twice.
| scripts/make_lldp_map.py br-mdf-01 map.parts | ||
| blockdiag network.diag -T svg -o map.svg | ||
| blockdiag network.diag -T png -o map.png |
There was a problem hiding this comment.
This script invokes the blockdiag CLI, but the repo's Nix devShell definitions don’t appear to include blockdiag. If this is meant to be run from the standard devShell, consider adding the dependency (or documenting how to install it) so blockdiag ... works reliably.
| client = paramiko.SSHClient() | ||
| client.set_missing_host_key_policy(paramiko.AutoAddPolicy()) | ||
|
|
There was a problem hiding this comment.
Using AutoAddPolicy() automatically trusts any presented host key, which makes the SSH collection vulnerable to MITM if DNS/IPs are spoofed. If possible, load known_hosts and use RejectPolicy (or provide an explicit opt-in flag) so host key verification is enforced by default.
| per_channel[assignment[dev1]].append(signals[dev1][dev2]) | ||
| total_eff = 0 | ||
| for ch_signals in per_channel.values(): | ||
| total_eff += effective_dbm(ch_signals) | ||
| return total_eff |
There was a problem hiding this comment.
compute_total_effective() initializes total_eff = 0 and returns 0 when there are no co-channel pairs, but the caller prints this as a dBm value (0 dBm is a very strong signal, not “no interference”). Consider returning a sentinel like None, or including the noise floors in the total, or returning an effective dBm value consistent with the rest of the model so the printed metric can’t be misinterpreted.
the only file that should be implemented is the apuse.csv file
this reflects a scan taken after the new APs were deployed in the expo.