fix(topology): read the spanning-tree root instead of guessing it (v0.23.35-beta)
Closes #131. Reported as the wrong device being marked root: an RB4011 at priority 4096 says "root bridge: yes", two APs at 8192 say they are not and name the RB4011's MAC, and the map crowned an AP anyway — consistently, always the same one. The detection had never worked at all. It looked for a device with no port whose STP role was 'root', but RouterOS reports the role as 'root-port'. Nothing ever matched, so the set of devices-with-a-root-port was permanently empty and the lookup returned the first device in the list. It was not choosing badly; it was not choosing. The same mismatch broke link state. Blocking was derived from a role of 'alternate' or 'backup' against values that arrive as 'alternate-port' and 'backup-port', so blocked links were drawn as forwarding — which is the "traffic connection path" half of the report, since a blocked link shown as live yields a path that does not exist. Roles are now normalised on write so both ends of the system share one vocabulary, and the root is no longer inferred. RouterOS answers it directly: /interface/bridge/monitor reports whether a bridge is the root and names the root it sees, which is exactly what the reporter's own screenshots show the device knowing. That is collected into device_bridges and resolved in utils/stpRoot.ts with 13 tests. Two properties of real networks shaped the resolution, both observed on the development fleet rather than reasoned about: * **A root belongs to a spanning tree, not to a fleet.** Two of four devices here legitimately report root at once, being separate bridge domains, so roots are resolved per connected component. * **The root may not be ours.** One switch points at priority 8192 on a MAC belonging to no device in the inventory. Naming a local device in that case is precisely the original bug, so the crown is withheld and the foreign bridge id reported instead. Verified against that fleet: the managed root resolves to the device that claims it, a self-rooted bridge keeps its own crown, and the switch with an external root now reports "not ours" where the old logic confidently crowned it — the reported symptom, reproduced and fixed. Also adds collectStp to the full sync. It previously ran only on the five-minute slow poll, so pressing Sync after rewiring did not refresh the map. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
This commit is contained in:
2GT-Rich
co-authored by
Claude Opus 5
Claude Opus 5
parent
273a0da737
commit
e47819e232
@@ -1,6 +1,6 @@
|
||||
{
|
||||
"name": "mikrotik-manager-frontend",
|
||||
"version": "0.23.34-beta",
|
||||
"version": "0.23.35-beta",
|
||||
"private": true,
|
||||
"type": "module",
|
||||
"scripts": {
|
||||
|
||||
@@ -27,6 +27,7 @@ import type { TopologyDevice, TopologyLink, ExternalTopologyNode } from '../type
|
||||
import clsx from 'clsx';
|
||||
import { useThemeStore } from '../store/themeStore';
|
||||
import { layoutTree } from '../utils/topologyLayout';
|
||||
import { resolveStpRoot, type BridgeInfo, type RootVerdict } from '../utils/stpRoot';
|
||||
|
||||
// ─── Helpers ─────────────────────────────────────────────────────────────────
|
||||
|
||||
@@ -209,6 +210,7 @@ function buildGraph(
|
||||
devices: TopologyDevice[],
|
||||
externalNodes: ExternalTopologyNode[],
|
||||
links: TopologyLink[],
|
||||
bridges: BridgeInfo[],
|
||||
segConns: { src: string; dst: string; port: string }[],
|
||||
manualLinkIds: { id: number; from_device_id: number; to_device_id: number }[],
|
||||
connectMode: boolean,
|
||||
@@ -259,14 +261,26 @@ function buildGraph(
|
||||
}
|
||||
|
||||
// ── STP root detection ───────────────────────────────────────────────────────
|
||||
const hasStp = links.some((l) => l.stp_role);
|
||||
const haveRootPort = new Set(links.filter((l) => l.stp_role === 'root').map((l) => String(l.from_device_id)));
|
||||
const stpRootId = hasStp ? devices.map((d) => String(d.id)).find((id) => !haveRootPort.has(id)) ?? '' : '';
|
||||
//
|
||||
// Resolved from what each bridge reports, not inferred from port roles. The
|
||||
// old inference compared against a role of 'root' while RouterOS reports
|
||||
// 'root-port', so it never matched anything and the first device in the list
|
||||
// was crowned every time (#131).
|
||||
const hasStp = bridges.length > 0;
|
||||
const rootVerdicts = new Map<string, RootVerdict>();
|
||||
const stpRootIds = new Set<string>();
|
||||
|
||||
function pickRoot(componentIds: string[]): string {
|
||||
if (hasStp) {
|
||||
const r = componentIds.find((id) => deviceIds.has(id) && !haveRootPort.has(id));
|
||||
if (r) return r;
|
||||
const managed = componentIds.filter((id) => deviceIds.has(id));
|
||||
if (hasStp && managed.length > 0) {
|
||||
const verdict = resolveStpRoot(managed, bridges);
|
||||
for (const id of componentIds) rootVerdicts.set(id, verdict);
|
||||
if (verdict.deviceId && managed.includes(verdict.deviceId)) {
|
||||
stpRootIds.add(verdict.deviceId);
|
||||
return verdict.deviceId;
|
||||
}
|
||||
// An external or unknown root leaves the drawing to connectivity, but the
|
||||
// crown is withheld rather than handed to an arbitrary device.
|
||||
}
|
||||
const inComp = new Set(componentIds);
|
||||
let best = componentIds[0], bestScore = -1;
|
||||
@@ -374,7 +388,7 @@ function buildGraph(
|
||||
id: String(d.id),
|
||||
type: 'deviceNode',
|
||||
position: positions.get(String(d.id)) ?? { x: 0, y: 0 },
|
||||
data: { ...d, isRootBridge: hasStp && String(d.id) === stpRootId, connectMode, orphan: orphanIds.has(String(d.id)) } as unknown as Record<string, unknown>,
|
||||
data: { ...d, isRootBridge: stpRootIds.has(String(d.id)), connectMode, orphan: orphanIds.has(String(d.id)) } as unknown as Record<string, unknown>,
|
||||
})),
|
||||
...allExtNodes.map((e) => ({
|
||||
id: e.id,
|
||||
@@ -517,6 +531,7 @@ export default function TopologyPage() {
|
||||
(data.devices as TopologyDevice[]) || [],
|
||||
(data.externalNodes as ExternalTopologyNode[]) || [],
|
||||
(data.links as TopologyLink[]) || [],
|
||||
(data.bridges as BridgeInfo[]) || [],
|
||||
(data.segConns as { src: string; dst: string; port: string }[]) || [],
|
||||
(data.manualLinkIds as { id: number; from_device_id: number; to_device_id: number }[]) || [],
|
||||
connectMode,
|
||||
|
||||
@@ -1014,6 +1014,8 @@ export const topologyApi = {
|
||||
devices: Device[];
|
||||
links: import('../types').TopologyLink[];
|
||||
externalNodes: import('../types').ExternalTopologyNode[];
|
||||
/** What each bridge reports about the spanning tree (#131). */
|
||||
bridges: import('../utils/stpRoot').BridgeInfo[];
|
||||
segConns: { src: string; dst: string; port: string }[];
|
||||
manualLinkIds: { id: number; from_device_id: number; to_device_id: number }[];
|
||||
}>('/topology'),
|
||||
|
||||
@@ -0,0 +1,131 @@
|
||||
import { describe, it, expect } from 'vitest';
|
||||
import { resolveStpRoot, bridgePriority, type BridgeInfo } from './stpRoot';
|
||||
|
||||
const RB4011 = '0x1000.2C:C8:1B:05:2B:B1'; // priority 4096
|
||||
const AP_A = '0x2000.AA:AA:AA:AA:AA:AA'; // priority 8192
|
||||
const AP_B = '0x2000.BB:BB:BB:BB:BB:BB';
|
||||
const FOREIGN = '0x2000.70:A7:41:EF:8F:FB'; // real root, not in the fleet
|
||||
|
||||
const b = (
|
||||
device_id: number, bridge_id: string, root_bridge: boolean, root_bridge_id: string | null,
|
||||
): BridgeInfo => ({
|
||||
device_id, bridge_name: 'bridge', bridge_id, root_bridge, root_bridge_id,
|
||||
root_port: null, root_path_cost: null, protocol_mode: 'rstp',
|
||||
});
|
||||
|
||||
describe('resolveStpRoot — the reported case', () => {
|
||||
// RB4011 at priority 4096 is root; two APs at 8192 both point at it. The old
|
||||
// logic crowned an AP because it matched port role 'root' instead of
|
||||
// 'root-port', so nothing matched and the first device in the list won.
|
||||
const fleet = [
|
||||
b(1, RB4011, true, RB4011),
|
||||
b(2, AP_A, false, RB4011),
|
||||
b(3, AP_B, false, RB4011),
|
||||
];
|
||||
|
||||
it('names the device that says it is root', () => {
|
||||
const v = resolveStpRoot(['1', '2', '3'], fleet);
|
||||
expect(v.deviceId).toBe('1');
|
||||
expect(v.confidence).toBe('reported');
|
||||
});
|
||||
|
||||
it('does not depend on the order devices arrive in', () => {
|
||||
// The precise failure being fixed: the answer used to be whichever sorted
|
||||
// first, so it must now be stable under any ordering.
|
||||
for (const order of [['1','2','3'], ['2','3','1'], ['3','2','1'], ['2','1','3']]) {
|
||||
expect(resolveStpRoot(order, fleet).deviceId).toBe('1');
|
||||
}
|
||||
});
|
||||
|
||||
it('is unmoved by shuffling the bridge rows themselves', () => {
|
||||
const shuffled = [fleet[2], fleet[0], fleet[1]];
|
||||
expect(resolveStpRoot(['1', '2', '3'], shuffled).deviceId).toBe('1');
|
||||
});
|
||||
});
|
||||
|
||||
describe('resolveStpRoot — a root we do not manage', () => {
|
||||
// Observed on a real switch: it points at priority 8192 on a MAC belonging to
|
||||
// no device in the inventory.
|
||||
const fleet = [
|
||||
b(8, AP_A, false, FOREIGN),
|
||||
b(9, AP_B, false, FOREIGN),
|
||||
];
|
||||
|
||||
it('declines to nominate one of ours', () => {
|
||||
const v = resolveStpRoot(['8', '9'], fleet);
|
||||
expect(v.deviceId).toBeNull();
|
||||
expect(v.confidence).toBe('external');
|
||||
});
|
||||
|
||||
it('reports the bridge id so the operator can identify it', () => {
|
||||
expect(resolveStpRoot(['8', '9'], fleet).rootBridgeId).toBe(FOREIGN);
|
||||
});
|
||||
});
|
||||
|
||||
describe('resolveStpRoot — separate spanning trees', () => {
|
||||
it('gives each component its own root', () => {
|
||||
// Two of four devices legitimately report root at once on a mixed fleet;
|
||||
// "the root" is per tree, not per fleet.
|
||||
const fleet = [
|
||||
b(1, RB4011, true, RB4011),
|
||||
b(2, AP_A, false, RB4011),
|
||||
b(7, AP_B, true, AP_B),
|
||||
];
|
||||
expect(resolveStpRoot(['1', '2'], fleet).deviceId).toBe('1');
|
||||
expect(resolveStpRoot(['7'], fleet).deviceId).toBe('7');
|
||||
});
|
||||
|
||||
it('prefers the claimant others actually point at', () => {
|
||||
const fleet = [
|
||||
b(1, RB4011, true, RB4011),
|
||||
b(2, AP_A, true, AP_A),
|
||||
b(3, AP_B, false, RB4011),
|
||||
];
|
||||
expect(resolveStpRoot(['1', '2', '3'], fleet).deviceId).toBe('1');
|
||||
});
|
||||
});
|
||||
|
||||
describe('resolveStpRoot — degraded input', () => {
|
||||
it('matches on bridge id when nobody claims the crown', () => {
|
||||
const fleet = [
|
||||
b(1, RB4011, false, RB4011),
|
||||
b(2, AP_A, false, RB4011),
|
||||
];
|
||||
const v = resolveStpRoot(['1', '2'], fleet);
|
||||
expect(v.deviceId).toBe('1');
|
||||
expect(v.confidence).toBe('matched');
|
||||
});
|
||||
|
||||
it('compares bridge ids without caring about MAC case', () => {
|
||||
const fleet = [
|
||||
b(1, RB4011.toUpperCase(), false, RB4011.toLowerCase()),
|
||||
b(2, AP_A, false, RB4011.toLowerCase()),
|
||||
];
|
||||
expect(resolveStpRoot(['1', '2'], fleet).deviceId).toBe('1');
|
||||
});
|
||||
|
||||
it('admits it does not know when there is no STP data', () => {
|
||||
// Better than guessing: the caller falls back to a connectivity heuristic
|
||||
// and can say so, rather than presenting a guess as fact.
|
||||
expect(resolveStpRoot(['1', '2'], [])).toEqual({
|
||||
deviceId: null, confidence: 'unknown', rootBridgeId: null,
|
||||
});
|
||||
});
|
||||
|
||||
it('ignores bridges belonging to devices outside the component', () => {
|
||||
const fleet = [b(99, RB4011, true, RB4011)];
|
||||
expect(resolveStpRoot(['1', '2'], fleet).confidence).toBe('unknown');
|
||||
});
|
||||
});
|
||||
|
||||
describe('bridgePriority', () => {
|
||||
it('reads the priority out of a bridge id', () => {
|
||||
expect(bridgePriority(RB4011)).toBe(4096);
|
||||
expect(bridgePriority(AP_A)).toBe(8192);
|
||||
expect(bridgePriority('0x8000.F4:1E:57:51:74:1E')).toBe(32768);
|
||||
});
|
||||
it('copes with nothing usable', () => {
|
||||
expect(bridgePriority(null)).toBeNull();
|
||||
expect(bridgePriority('garbage')).toBeNull();
|
||||
});
|
||||
});
|
||||
@@ -0,0 +1,112 @@
|
||||
/**
|
||||
* Which device is the spanning-tree root, per connected component.
|
||||
*
|
||||
* This was previously inferred: find a device with no port whose role is "root".
|
||||
* The inference never fired, because RouterOS reports the role as "root-port"
|
||||
* and the comparison was against "root" — so no device ever matched and the
|
||||
* topology crowned whichever device happened to sort first, consistently and
|
||||
* confidently (#131).
|
||||
*
|
||||
* The device already knows the answer. `/interface/bridge/monitor` reports
|
||||
* whether it is the root and names the root it can see, so the job here is
|
||||
* reading that rather than deducing it.
|
||||
*
|
||||
* Two properties of real networks shape the result:
|
||||
*
|
||||
* **There is a root per spanning tree, not per fleet.** Separate bridge domains
|
||||
* each have their own, and on a small mixed fleet several devices legitimately
|
||||
* report `root-bridge: true` at once. Roots are therefore resolved within a
|
||||
* connected component.
|
||||
*
|
||||
* **The root may not be ours.** A managed switch can point at a bridge id
|
||||
* belonging to equipment nobody has added. Naming a local device in that case
|
||||
* would be worse than admitting we cannot see it.
|
||||
*/
|
||||
|
||||
export interface BridgeInfo {
|
||||
device_id: number;
|
||||
bridge_name: string;
|
||||
bridge_id: string | null;
|
||||
root_bridge: boolean | null;
|
||||
root_bridge_id: string | null;
|
||||
root_port: string | null;
|
||||
root_path_cost: number | null;
|
||||
protocol_mode: string | null;
|
||||
}
|
||||
|
||||
export type RootConfidence =
|
||||
/** A device in this component reports itself as root. */
|
||||
| 'reported'
|
||||
/** No local device claims it, but one owns the bridge id others point at. */
|
||||
| 'matched'
|
||||
/** Devices agree on a root we do not manage. */
|
||||
| 'external'
|
||||
/** Nothing usable; the caller should fall back to a connectivity heuristic. */
|
||||
| 'unknown';
|
||||
|
||||
export interface RootVerdict {
|
||||
/** Device id as a string, matching topology node ids. Null when not ours. */
|
||||
deviceId: string | null;
|
||||
confidence: RootConfidence;
|
||||
/** The bridge id of the actual root, when known — shown when it is not ours. */
|
||||
rootBridgeId: string | null;
|
||||
}
|
||||
|
||||
/** Bridge ids are compared case-insensitively; RouterOS is inconsistent about MAC case. */
|
||||
const norm = (id: string | null | undefined): string => (id || '').trim().toLowerCase();
|
||||
|
||||
/**
|
||||
* Resolve the root for one component.
|
||||
*
|
||||
* `componentDeviceIds` are the managed devices in this component, as strings.
|
||||
*/
|
||||
export function resolveStpRoot(
|
||||
componentDeviceIds: string[],
|
||||
bridges: BridgeInfo[],
|
||||
): RootVerdict {
|
||||
const inComponent = new Set(componentDeviceIds);
|
||||
const local = bridges.filter((b) => inComponent.has(String(b.device_id)));
|
||||
if (local.length === 0) return { deviceId: null, confidence: 'unknown', rootBridgeId: null };
|
||||
|
||||
// A device that says it is the root, is the root.
|
||||
const claims = local.filter((b) => b.root_bridge === true);
|
||||
if (claims.length === 1) {
|
||||
return { deviceId: String(claims[0].device_id), confidence: 'reported', rootBridgeId: claims[0].bridge_id };
|
||||
}
|
||||
|
||||
// Several claims inside one component means several bridge domains share it —
|
||||
// a device bridging two segments, for instance. Prefer the one others actually
|
||||
// point at; that is the tree the component is really hanging from.
|
||||
const pointedAt = new Set(local.map((b) => norm(b.root_bridge_id)).filter(Boolean));
|
||||
if (claims.length > 1) {
|
||||
const agreed = claims.find((b) => pointedAt.has(norm(b.bridge_id)));
|
||||
const chosen = agreed ?? claims[0];
|
||||
return { deviceId: String(chosen.device_id), confidence: agreed ? 'reported' : 'matched', rootBridgeId: chosen.bridge_id };
|
||||
}
|
||||
|
||||
// Nobody claims it. Does anyone own the bridge id the others are pointing at?
|
||||
for (const target of pointedAt) {
|
||||
const owner = local.find((b) => norm(b.bridge_id) === target);
|
||||
if (owner) {
|
||||
return { deviceId: String(owner.device_id), confidence: 'matched', rootBridgeId: owner.bridge_id };
|
||||
}
|
||||
}
|
||||
|
||||
// They agree on something we do not manage. Say so rather than nominating one
|
||||
// of ours, which is exactly the mistake this replaces.
|
||||
const external = [...pointedAt][0];
|
||||
if (external) {
|
||||
const withId = local.find((b) => norm(b.root_bridge_id) === external);
|
||||
return { deviceId: null, confidence: 'external', rootBridgeId: withId?.root_bridge_id ?? null };
|
||||
}
|
||||
|
||||
return { deviceId: null, confidence: 'unknown', rootBridgeId: null };
|
||||
}
|
||||
|
||||
/** Priority out of a bridge id like "0x8000.F4:1E:57:51:74:1E", for display. */
|
||||
export function bridgePriority(bridgeId: string | null): number | null {
|
||||
const m = (bridgeId || '').match(/^0x([0-9a-f]+)\./i);
|
||||
if (!m) return null;
|
||||
const n = parseInt(m[1], 16);
|
||||
return Number.isFinite(n) ? n : null;
|
||||
}
|
||||
@@ -1 +1 @@
|
||||
export const APP_VERSION = 'v0.23.34 Beta';
|
||||
export const APP_VERSION = 'v0.23.35 Beta';
|
||||
|
||||
Reference in New Issue
Block a user