Repository navigation
Conversation
This was
linked to
issues
Oct 6, 2026
There was a problem hiding this comment.
Copilot review overview
🔵 Needs a closer look
The new PVID checks need human integration validation with live candidate data and CLI command sequencing.
Review effort: Balanced
Findings: 1
Open (1)
What changed in this PR
Updates Infix documentation, adds advisory CLI checks for bridge PVID configuration, and fixes hardware reporting and test reliability.
Changes:
- Warn about missing or mismatched PVIDs during CLI validation and commits.
- Restore VPD manufacturer reporting and isolate syslog test messages by run.
- Refresh networking, management, hardware, and image documentation.
| File | Description |
|---|---|
| test/case/syslog/property_filter/test.py | Isolates current-run messages and checks cleanup results. |
| src/statd/python/yanger/ietf_hardware.py | Corrects the manufacturer-key lookup. |
| src/klish-plugin-infix/xml/infix.xml | Hooks PVID checks into CLI commands. |
| src/klish-plugin-infix/src/infix.c | Implements advisory PVID checks. |
| doc/vpd.md | Documents discovery, attributes, and factory credentials. |
| doc/management.md | Updates Web services and RESTCONF guidance. |
| doc/ChangeLog.md | Records PVID warnings and manufacturer reporting fix. |
| doc/bridging.md | Explains VLAN ingress, egress, and PVID behavior. |
| doc/branding.md | Corrects a documentation script reference. |
| board/common/image/image-readme/README.md | Updates GNS3 appliance installation guidance. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
The VPD document covered the ONIE TLV encoding, but not the device tree bindings that make the system read an EEPROM, or which attributes of the VPD named "product" end up where: mainboard component, base MAC address, hostname, and factory password. Fixes #579 Signed-off-by: Joachim Wiberg <troglobit@gmail.com>
The section still described the Web server as an information page, gave the wrong port for the Web console, 7861 instead of 7681, and did not say that disabling RESTCONF only blocks remote access, since the WebUI uses it locally. Fixes #797 Signed-off-by: Joachim Wiberg <troglobit@gmail.com>
The manufacturer attribute was looked up with a misspelled key, "manufacter", so a VPD component in ietf-hardware never got its mfg-name set. Signed-off-by: Joachim Wiberg <troglobit@gmail.com>
The VLAN filtering bridge section did not say which frames a port accepts or drops, and claimed a default PVID of 1. Bridges are created without a default PVID, so a port without one drops untagged frames. Fixes #779 Signed-off-by: Joachim Wiberg <troglobit@gmail.com>
The appliance moved to the GNS3 Marketplace, but the image README still said a .gns3a file is included in the release tarball, and the branding guide named mkgns3a.sh, which is gone, as a consumer of os-release. Signed-off-by: Joachim Wiberg <troglobit@gmail.com>
Bridges are created without a default PVID, so a port that is an untagged member of a VLAN but has no PVID drops all untagged frames. A PVID for a VLAN the port is not a member of is ignored, with only a syslog message, and a PVID that differs from the port's untagged VLAN puts ingress and egress traffic in different VLANs. None of this is visible when configuring from the CLI. Check the candidate on check, commit, and leave, and print a warning per port. The configuration is still applied: admin@example:/config/> leave Warning: e1 is an untagged member of VLAN 10 on br0, but has no PVID. Untagged frames received on e1 are dropped. Warning: e2 has PVID 30, but is not a member of VLAN 30 on br0. The PVID is ignored. Warning: e3 is an untagged member of VLAN 10 on br0, but has PVID 20. admin@example:/> Fixes #354 Signed-off-by: Joachim Wiberg <troglobit@gmail.com>
The test sometimes failed on hardware: Expected 2 myapp messages in /var/log/myapp, got 4 /var/log persists between test runs on hardware. The cleanup step ignored the exit code of the remote rm, so when it failed, the log files from the previous run remained and every count doubled. Tag the messages with a unique token and count only lines carrying it. Also retry the cleanup on SSH transport errors and fail the test if the files cannot be removed, and wait for the last message rather than the first before checking the logs. Signed-off-by: Joachim Wiberg <troglobit@gmail.com>
jovatn
approved these changes
Oct 7, 2026
Contributor
Author
Just for completeness, we discussed this AFK already, the PVID behavior change was made years ago. It was just the docs that weren't updated. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
Minor fix and a few doc updates.
Checklist
Tick relevant boxes, this PR is-a or has-a: