Skip to content

refactor(network/shape): de-duplicate the traffic-shaper engine and fix interactive daemon block-node wiring - #949

Merged
brunodam merged 7 commits into
mainfrom
refactor-traffic-shaper-engine
Aug 2, 2026
Merged

refactor(network/shape): de-duplicate the traffic-shaper engine and fix interactive daemon block-node wiring#949
brunodam merged 7 commits into
mainfrom
refactor-traffic-shaper-engine

Conversation

@alex-au

@alex-au alex-au commented Jul 31, 2026

Copy link
Copy Markdown
Contributor

Description

Behavior-preserving cleanup of the traffic-shaper engine (internal/network/shape), which accumulated duplication and dead code as the feature landed across ~10 PRs, plus a fix for a real bug in the interactive daemon installer. Net −116 lines; no functional change to the shaping engine.

The daemon fix addresses a latent bug: the interactive daemon service install path built the block-node component without its kubeconfig or orbit namespace, guarded by a stale "traffic-shaper is stubbed" comment. The monitor is fully wired now, so that path produced an incomplete config versus block node install and daemon_offer.go, which both set these fields. The bn-orbit prompt and BNOrbit target already existed but were never hooked up.

Files changed

File Change
internal/network/shape/defaults.go (new) Data-driven egress/ingress default profiles + shared buildDefaultConfig
internal/network/shape/tc.go (new) Single build-tag-free TCRunner interface declaration
internal/network/shape/manager.go Share one provisionDefaults; drop twin default*Config + redundant exported RenderAndApplyEgress; use validateClassFields
internal/network/shape/validate.go Add validateClassFields + sumParseableRatesBps
internal/network/shape/override.go Use the shared validators; remove dead ClassDirection
internal/network/shape/registry.go Generic writeConfigJSON/readConfigJSON/removeConfigFile behind the device/class wrappers
internal/network/shape/tc_linux.go, tc_other.go Remove the now-shared TCRunner interface declaration
internal/ui/prompt/daemon.go Wire block-node kubeconfig + orbit prompt (bug fix)

Review guide

Checklist:

  • defaults.go:64 buildDefaultConfig — verify ceilFull classes emit the trunk rate verbatim (e.g. 1gbit, not 1000mbit) and percentage classes match the previous mbps*pct/100 maths (egress partner 40/70, public 30/70, reserve 30/trunk; ingress 80/10/10 all ceil=trunk).
  • manager.go provisionDefaults — egress passes an afterWrite that renders+applies the boot script; ingress passes nil (config only, no boot script). Both still run under withLock.
  • validate.go validateSumRates — the "device rate unparseable" and "new rate unparseable" early-return skips are preserved (guards the legacy SPEED path, intentionally untouched here).
  • registry.go — error-message wording changed (no test asserts it); behavior identical, mkdir target unchanged.
  • daemon.go — Step 4/5 mirror the consensus-node wiring; bn-orbit is prompted only when the block-node component is selected.

Tests:

go test ./internal/network/shape/... ./internal/blocknode/shaper/... ./internal/ui/prompt/...
task lint

All pass; golangci-lint reports 0 issues; go vet clean.

Not in scope

The legacy "SPEED" sysfs-fallback render path is deliberately left untouched — it is still reachable via network shape delete --device egress and legacy on-disk registries, and retiring it needs a DeleteDevice(egress) redesign first (tracked in #946). Two follow-up structural refactors are filed separately: #946 (collapse the render/apply pipeline + single-source tc encoding) and #947 (unify the firewall/traffic-shaping gated-feature wiring).

Related Issues

@alex-au
alex-au requested a review from a team as a code owner July 31, 2026 01:11
@alex-au
alex-au requested a review from tomzhenghedera July 31, 2026 01:11
@swirlds-automation

swirlds-automation commented Jul 31, 2026

Copy link
Copy Markdown

Snyk checks have passed. No issues have been found so far.

Status Scan Engine Critical High Medium Low Total (0)
Open Source Security 0 0 0 0 0 issues
Licenses 0 0 0 0 0 issues

💻 Catch issues earlier using the plugins for VS Code, JetBrains IDEs, Visual Studio, and Eclipse.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR cleans up internal/network/shape by consolidating duplicated logic (default profile construction, per-class validation, and registry JSON CRUD) while keeping behavior stable, and fixes the interactive daemon service install prompts to correctly populate block-node kubeconfig + orbit namespace needed by the traffic-shaper monitor.

Changes:

  • Deduplicates default egress/ingress profile generation via a shared, data-driven buildDefaultConfig.
  • Centralizes class-field validation (rate/ceil/prio) and rate-summing logic to ensure consistent enforcement across verbs.
  • Fixes interactive daemon installer wiring for the block-node component (sets kubeconfig and prompts for bn-orbit when selected).

Reviewed changes

Copilot reviewed 9 out of 9 changed files in this pull request and generated no comments.

Show a summary per file
File Description
internal/network/shape/defaults.go Introduces data-driven default profiles and a shared default-config builder.
internal/network/shape/manager.go Replaces duplicated default-provisioning paths with a shared provisionDefaults; removes redundant exported wrapper.
internal/network/shape/override.go Removes dead ClassDirection; reuses shared validators and shared rate-summing helper.
internal/network/shape/registry.go Consolidates device/class registry JSON read/write/remove into generic helpers.
internal/network/shape/tc.go Moves TCRunner interface into a build-tag-free file shared by linux/non-linux implementations.
internal/network/shape/tc_linux.go Removes duplicate TCRunner declaration (now shared from tc.go).
internal/network/shape/tc_other.go Removes duplicate TCRunner declaration (now shared from tc.go).
internal/network/shape/validate.go Adds validateClassFields and sumParseableRatesBps to deduplicate validation and sum checking.
internal/ui/prompt/daemon.go Fixes interactive daemon install prompts to wire BN kubeconfig and prompt for BN orbit when selected.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 9 out of 9 changed files in this pull request and generated no new comments.

alex-au added 2 commits August 1, 2026 11:46
Behavior-preserving cleanup of code that grew across the traffic-shaper PRs:

- Data-drive the egress/ingress default profiles (new defaults.go) and share
  one provisionDefaults method, replacing the twin default*Config builders and
  ProvisionDefault{Egress,Ingress} bodies.
- Extract validateClassFields + sumParseableRatesBps, removing the 3x per-class
  and 2x budget-check validation duplication.
- Generic registry CRUD (writeConfigJSON/readConfigJSON/removeConfigFile) behind
  the existing device/class wrappers.
- Move the TCRunner interface to a build-tag-free tc.go (was declared twice, in
  tc_linux.go and tc_other.go).
- Remove dead ClassDirection and the redundant exported RenderAndApplyEgress
  (no external callers).

No functional change; all internal/network/shape tests pass unchanged.

Signed-off-by: alex-au <alex.w.aus@gmail.com>
…aller

The interactive `daemon service install` path built the block-node component
without its kubeconfig or orbit namespace, guarded by a stale "traffic-shaper
is stubbed" comment. The monitor is fully wired now, so this produced an
incomplete config versus `block node install` and daemon_offer.go (which both
set these fields). Set the kubeconfig and prompt for the orbit via the existing
but previously unwired bn-orbit prompt and BNOrbit target.

Signed-off-by: alex-au <alex.w.aus@gmail.com>
@brunodam
brunodam force-pushed the refactor-traffic-shaper-engine branch from 0514a3b to 5fb6409 Compare August 1, 2026 01:46
alex-au added 5 commits August 1, 2026 11:51
Signed-off-by: alex-au <alex.w.aus@gmail.com>
…ify (#951)

Signed-off-by: alex-au <alex.w.aus@gmail.com>
…ngle-source tc encoding (#955)

Signed-off-by: alex-au <alex.w.aus@gmail.com>
…raffic-shaping (#952)

Signed-off-by: alex-au <alex.w.aus@gmail.com>
…work-feature wiring (#957)

Signed-off-by: alex-au <alex.w.aus@gmail.com>
@alex-au
alex-au force-pushed the refactor-traffic-shaper-engine branch from 5fb6409 to 01a1103 Compare August 1, 2026 01:51
@brunodam
brunodam merged commit 23e7a56 into main Aug 2, 2026
20 checks passed
@brunodam
brunodam deleted the refactor-traffic-shaper-engine branch August 2, 2026 00:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

refactor(network/shape): de-duplicate the traffic-shaper engine and fix interactive daemon block-node wiring

4 participants