Merge latest upstream sonic-gnmi master#219
Merged
Merged
Conversation
When FIPS mode is enabled (sonic_fips=1), the Go FIPS runtime calls EVP_default_properties_enable_fips(NULL, 1), which constrains all EVP_CIPHER_fetch calls to FIPS-approved ciphers only. The SymCrypt OpenSSL provider does not implement AES-128-CTR, so EVP_CIPHER_fetch returns NULL. The Go FIPS cipher cache stores this NULL and panics with 'unsupported cipher: AES-128' when TLS 1.3 tries to encrypt session tickets (which use AES-128-CTR). Disable session tickets in the gNMI TLS config to avoid the AES-128-CTR code path entirely. TLS 1.3 connections still work normally; only session resumption is affected, which is not needed for long-lived gRPC connections. Signed-off-by: Dawei Huang <daweihuang@microsoft.com>
* gnmi_server: do not fail server when one listener fails Previously, if either the TCP or UDS listener failed during initialization, NewServer() would tear down any already-created listener and return an error, preventing the server from starting entirely. Similarly, Serve() returned on the first listener error, effectively orphaning the remaining listener goroutine. Change NewServer() to log a warning and disable the failed listener instead of cascading the failure. The existing 'at least one listener' check ensures the server still fails when no listener is available. Change Serve() to use a WaitGroup so it blocks until all listener goroutines complete, rather than returning on the first error. Individual listener failures are logged but do not affect other listeners. Signed-off-by: Dawei Huang <daweihuang@microsoft.com> * gnmi_server: add tests for listener resilience Test that NewServer() continues when one listener fails: - TestTCPListenerFailsUDSContinues: TCP port in use, UDS still works - TestUDSListenerFailsTCPContinues: UDS path invalid, TCP still works - TestBothListenersFail: both fail, server returns error Test that Serve() blocks until all listeners stop: - TestServeBlocksWhenOneListenerCloses: closing one listener at runtime does not cause Serve() to return early Signed-off-by: Dawei Huang <daweihuang@microsoft.com> * gnmi_server: cover remaining listener resilience paths Add tests targeting the three uncovered code paths: - TestUDSBindFailsTCPContinues: socket path exceeds unix max length, directory creation succeeds but bind fails (lines 582-584) - TestUDSChmodFailsTCPContinues: uses gomonkey to simulate chmod failure after socket creation (lines 588-593) - TestServeUDSErrorDoesNotStopTCP: closes UDS listener at runtime to trigger UDS server error path in Serve() (lines 645-648) Signed-off-by: Dawei Huang <daweihuang@microsoft.com> * gnmi_server: drop unreliable gomonkey chmod test The TestUDSChmodFailsTCPContinues test used gomonkey.ApplyFunc to patch os.Chmod, which is unreliable under -race (used in CI). Remove it; the remaining tests cover 85% of the diff, above the 80% threshold. Signed-off-by: Dawei Huang <daweihuang@microsoft.com> --------- Signed-off-by: Dawei Huang <daweihuang@microsoft.com>
…C Rotate streams (#618) Signed-off-by: Pattela JAYARAGINI <pattelaj@google.com> Co-authored-by: Neha Das <nehadas@google.com>
…646) The fix in PR #618 (bumping parent context timeout to 10s) did not resolve the underlying race. The internal 5s context.WithTimeout still causes stream2.Recv() to return DeadlineExceeded before the server returns Aborted under CI load. Re-skip the test until the concurrency logic is fixed with explicit synchronization. Tracking: sonic-net/sonic-gnmi#616 Signed-off-by: Dawei Huang <daweihuang@microsoft.com>
…(#645)
* Upgrade go directive to 1.24.4
Bump go.mod from go 1.19 to go 1.24.4 to match the Go version
in the Trixie container (sonic-buildimage#26499).
Go 1.24 module graph pruning only resolves modules actually in the
import graph, so packages that don't transitively depend on
sonic-mgmt-common no longer fail when the replace target is absent.
This fixes silent build failures in 7 pure-CI packages.
Also fix non-constant format string in pkg/gnoi/system/system.go
that go vet now rejects under Go 1.24.
Signed-off-by: Dawei Huang <daweihuang@microsoft.com>
* Migrate Azure pipeline from Bookworm to Trixie
Update container images from sonic-slave-bookworm to sonic-slave-trixie.
Update artifact paths from bookworm to trixie for libyang, libnl,
sonic-yang-models, and sonic-swss-common packages.
Update .NET repository from debian/12 to debian/13.
Update PureCIJob Go version from 1.19.8 to 1.24.4.
Signed-off-by: Dawei Huang <daweihuang@microsoft.com>
* Add libpcre3 to trixie artifact download patterns
Trixie does not ship libpcre3, but libyang 1.0.73 depends on it.
The common_libs pipeline builds libpcre3 debs under target/debs/trixie/,
so download and install them alongside libyang.
Signed-off-by: Dawei Huang <daweihuang@microsoft.com>
* Add missing libpcre3 transitive dependencies for trixie
libpcre3-dev depends on libpcre16-3, libpcre32-3, and libpcrecpp0v5,
which are not available on Trixie. Download them from common_libs
alongside libpcre3 and libpcre3-dev.
Signed-off-by: Dawei Huang <daweihuang@microsoft.com>
* Fix .NET SDK install for Trixie
apt-key was removed in Debian Trixie. Use signed-by in the sources
list with the key stored in /etc/apt/keyrings/ instead.
Signed-off-by: Dawei Huang <daweihuang@microsoft.com>
* Dearmor Microsoft GPG key for Trixie sqv compatibility
Trixie uses sqv (Sequoia) for APT signature verification which
requires binary (dearmored) GPG keys. The previous commit stored the
key in ASCII-armored format (.asc) which sqv cannot parse, resulting
in 'Missing key EE4D7792F748182B' errors.
Signed-off-by: Dawei Huang <daweihuang@microsoft.com>
* Remove unused .NET SDK install from CI pipeline
The .NET SDK was never used by sonic-gnmi (pure Go project). It was
a leftover from a shared pipeline template. Removing it also avoids
the sqv GPG verification issue on Trixie.
Signed-off-by: Dawei Huang <daweihuang@microsoft.com>
* Fix non-constant format string in dbus_client for Go 1.24 vet
Go 1.24 vet rejects variable strings as format arguments. Change
fmt.Errorf(msg) to fmt.Errorf("%s", msg) for two call sites in
dbus_client.go. This also fixes sonic_data_client which imports
sonic_service_client transitively.
Signed-off-by: Dawei Huang <daweihuang@microsoft.com>
* Fix additional non-constant format strings for Go 1.24 vet
Go 1.24 vet rejects variable strings as format arguments in
printf-style functions. Fix log.V(1).Infof(msg) and
status.Errorf(code, errStr) patterns in sonic_data_client and
gnmi_server packages.
Signed-off-by: Dawei Huang <daweihuang@microsoft.com>
* Disable inlining in tests for gomonkey compatibility with Go 1.24
gomonkey patches functions by rewriting machine code at runtime,
which requires inlining to be disabled. Go 1.24 inlines more
aggressively than 1.19, causing gomonkey's ApplyMethod to fail
silently (the real method executes instead of the mock). Add
-gcflags=all=-l via TEST_FLAGS to all test invocations.
Signed-off-by: Dawei Huang <daweihuang@microsoft.com>
* Fix test timeouts and skip hanging CGO test for Trixie
- Increase inotify cert monitoring test timeouts from 10s to 30s to
account for slower execution with -gcflags=all=-l
- Skip TestNewEventClient: event_set_global_options blocks indefinitely
on Trixie, leaking a goroutine that causes the test binary to exceed
its timeout
- Add -timeout 20m to basic integration tests for safety margin
Signed-off-by: Dawei Huang <daweihuang@microsoft.com>
* Deinit event subscriber in TestDummyEventClient to prevent leaked C thread
C_init_subs creates a ZMQ subscriber background thread. Without
calling C_deinit_subs, this thread keeps the test binary alive until
the go test timeout kills it.
Signed-off-by: Dawei Huang <daweihuang@microsoft.com>
* Retrigger CI
Signed-off-by: Dawei Huang <daweihuang@microsoft.com>
* Remove C_init_subs from TestDummyEventClient
The events CGO subscriber creates a background C thread that blocks
indefinitely on Trixie, preventing the test binary from exiting.
Both C_init_subs and C_deinit_subs block on Trixie's swss-common.
Signed-off-by: Dawei Huang <daweihuang@microsoft.com>
* Fix non-constant format strings in gnmi_server for Go 1.24 vet
Go 1.24 vet rejects non-constant format strings in printf-style
functions. Fix all status.Errorf(code, err.Error()) and
status.Errorf(code, fmt.Sprintf(...)) calls to use explicit %s
format in gnmi_server package.
Signed-off-by: Dawei Huang <daweihuang@microsoft.com>
* Debug: use standard-verbose format for env integration tests
Temporarily use standard-verbose gotestsum format to see the actual
gnmi_server build error that testname format is hiding.
Signed-off-by: Dawei Huang <daweihuang@microsoft.com>
* Add pre-compile check to surface gnmi_server build errors
gotestsum --format testname swallows build/vet errors. Add an
explicit go test -run=^$ (no tests, just compile) before running
gotestsum so compiler errors appear in the log.
Signed-off-by: Dawei Huang <daweihuang@microsoft.com>
* Fix t.Logf vet errors in gnmi_server/server_test.go
Go 1.24 vet rejects non-constant format strings in printf-style
functions. Fix t.Logf(string(data)) to t.Logf("%s", string(data)).
Signed-off-by: Dawei Huang <daweihuang@microsoft.com>
* Increase timeout for GenerateCSR certz tests
GenerateCsrRSA, GenerateCsrECDSA, and GenerateCsrAttest all need
more than the default 1s timeout for key generation on Trixie. Set
timeout to 10s matching other CSR-related test cases.
The 1s default caused GenerateCsrRSA to timeout, which left the
Rotate RPC in progress, cascading to concurrent-RPC errors in
subsequent subtests.
Signed-off-by: Dawei Huang <daweihuang@microsoft.com>
* Disable both optimizations and inlining for gomonkey tests
Use -gcflags='all=-N -l' instead of just -gcflags=all=-l to also
disable compiler optimizations. Go 1.24 may optimize time.Sleep
in ways that prevent gomonkey from patching it at runtime.
Signed-off-by: Dawei Huang <daweihuang@microsoft.com>
* Replace time.Sleep mock with injectable variable for Go 1.24
Go 1.24 makes time.Sleep unpatchable by gomonkey even with
-gcflags=all=-l. Replace the direct time.Sleep call with an
exported SleepFunc variable that tests can reliably replace
via gomonkey.ApplyGlobalVar.
Revert -gcflags back to 'all=-l' (inlining only) since -N
causes tests to run too slowly and hit the 20m timeout.
Signed-off-by: Dawei Huang <daweihuang@microsoft.com>
* Increase test timeouts from 20m to 40m
With -gcflags=all=-l disabling inlining for gomonkey compatibility,
the gnmi_server test suite takes over 20 minutes. The top tests are
TestGnmiSubscribeMultiNs (188s), TestGnmiSubscribe (180s),
TestGNMINative (95s), TestSystem (73s), totaling well over 20m.
Signed-off-by: Dawei Huang <daweihuang@microsoft.com>
* Add unit tests for diff-coverage on vet-fix error paths
Add tests covering error return paths that were changed for Go 1.24
vet compliance (non-constant format strings in status.Errorf).
gnoi_reset_test.go:
- Marshal error (protojson.Marshal failure)
- Backend error (FactoryReset returns error)
- Unmarshal error (invalid JSON response)
gnoi_os_test.go:
- Enhanced fakeInstallServer with Recv() support
- RecvError on TransferRequest
- SendError on TransferReady response
- RecvError during TransferContent loop
- SendError on TransferEnd response
gnsi_authz_test.go:
- writeCredentialsMetadataToDB failure on version field
- writeCredentialsMetadataToDB failure on created_on field
- saveToAuthzFile failure
Signed-off-by: Dawei Huang <daweihuang@microsoft.com>
* Fix gofmt formatting in gnoi_os_test.go
Signed-off-by: Dawei Huang <daweihuang@microsoft.com>
* Trigger CI rebuild
Signed-off-by: Dawei Huang <daweihuang@microsoft.com>
* Add unit tests for remaining diff-coverage gaps
Cover additional error paths in:
- jwtAuth.go: invalid JWT token parsing error (line 80)
- gnoi_os.go: Send failure after processTransferContent (line 353)
- gnoi_system.go: GetRedisDBClient failures in Reboot, RebootStatus,
CancelReboot (lines 219, 251, 279)
Signed-off-by: Dawei Huang <daweihuang@microsoft.com>
* Fix TransferContent type in OS install test
Signed-off-by: Dawei Huang <daweihuang@microsoft.com>
* Fix OS install test: use temp dir instead of gomonkey for unexported methods
Signed-off-by: Dawei Huang <daweihuang@microsoft.com>
* Fix Send-error test coverage and add RebootStatus unmarshal test
- Set sendErr in TestInstall_SendErrorAfterTransferContent so the
fakeInstallServer.Send actually returns a non-nil error, covering
gnoi_os.go line 353 (previously the test hit the EOF path instead).
- Add assert.Contains to verify the error message, ensuring the test
exercises the intended code path.
- Add TestRebootStatus_UnmarshalError to cover gnoi_system.go line 263
(protojson.Unmarshal failure in RebootStatus). Patches
sendRebootReqOnNotifCh to return invalid JSON data.
Signed-off-by: Dawei Huang <daweihuang@microsoft.com>
* Generate diff-cover JSON for pipeline coverage decorator
The [Auto] Generate/Update build coverage properties tasks are
injected by a pipeline decorator after PublishCodeCoverageResults@2.
They expect .diff-coverage/diff-cover.json to exist in the Build
job workspace, but diff-cover only ran in the DiffCoverageCheck job.
Add a diff-cover --json-report step in the Build job before publishing
coverage so the decorator can read the JSON and report the
coverage.sonic-net.sonic-gnmi.build status check to GitHub.
Signed-off-by: Dawei Huang <daweihuang@microsoft.com>
* Use --break-system-packages for pip on Trixie
Trixie's system Python has Pygments installed via apt without a
RECORD file. pip refuses to upgrade it when installing diff-cover,
causing 'diff-cover: command not found' in the auto-injected
pipeline decorator step. This is why .diff-coverage/diff-cover.json
was never generated and the coverage status check got stuck.
Signed-off-by: Dawei Huang <daweihuang@microsoft.com>
* Remove redundant DiffCoverageCheck job and unused threshold variable
The diff-coverage check is handled by the org-level pipeline decorator
that auto-injects after PublishCodeCoverageResults@2. The separate
DiffCoverageCheck job is redundant.
Also remove the now-unused DIFF_COVER_THRESHOLD variable and add
--ignore-installed to pip install diff-cover to work around Trixie's
Pygments packaging (dist-info without RECORD file blocks uninstall).
Signed-off-by: Dawei Huang <daweihuang@microsoft.com>
* Fix coverage decorator by setting working directory and pre-installing diff-cover
The org-level pipeline decorator (code-diff-coverage) cd's to
DIFF_COVER_WORKING_DIRECTORY, defaulting to System.DefaultWorkingDirectory.
For sonic-gnmi, coverage.xml lives in the sonic-gnmi subdirectory, so
the decorator couldn't find it.
On Trixie, the decorator's pip install diff-cover also fails because
Debian ships Pygments via dist-info without a RECORD file, causing pip
to refuse the upgrade. Pre-installing with --ignore-installed before
the decorator runs makes the decorator's subsequent pip install a no-op.
Signed-off-by: Dawei Huang <daweihuang@microsoft.com>
* Use sudo for diff-cover pre-install to match decorator
Pipeline runs as azureuser_azpcontainer, not root. Without sudo, pip
installs to ~/.local/bin which is not on PATH. The decorator then runs
sudo pip3 install as root and still hits the Pygments RECORD issue.
Using sudo ensures diff-cover is installed system-wide where both the
decorator and subsequent steps can find it.
Signed-off-by: Dawei Huang <daweihuang@microsoft.com>
---------
Signed-off-by: Dawei Huang <daweihuang@microsoft.com>
Signed-off-by: Spandan Chowdhury <spandan@nexthop.ai>
Signed-off-by: Niranjani Vivek <niranjaniv@google.com>
* telemetry: fix auth bypass in noTLS code path
Two fixes to close unauthenticated gNMI access vulnerability:
1. Move cfg.UserAuth assignment above the TLS setup block so
application-layer authentication is enforced regardless of
whether TLS transport is active.
2. Fatal error instead of silently unsetting cert auth when
--client_auth cert is requested without --cacert. Prevents
server starting with no authentication configured.
Signed-off-by: xq9mend <xq9mend@users.noreply.github.com>
* telemetry: add --bind_address flag to restrict TCP listener
When --noTLS is active (no certs configured), the gRPC server runs
cleartext. Add --bind_address flag so operators can restrict the
listener to localhost only, preventing remote access to the cleartext
endpoint:
telemetry --noTLS --bind_address 127.0.0.1 --port 8080
When bind_address is empty (default), behavior is unchanged: binds all
interfaces. VRF-aware listener ignores bind_address (VRF binding is
handled by createVrfListener).
gnmi_server/server.go: add BindAddress to Config struct
telemetry/telemetry.go: add --bind_address flag and pass to Config
Signed-off-by: xq9mend <xq9mend@users.noreply.github.com>
* telemetry: reject --noTLS without --bind_address 127.0.0.1
--noTLS disables TLS, exposing cleartext gRPC. Require --bind_address 127.0.0.1
to ensure the cleartext endpoint is only accessible from localhost.
Also update existing test cases that use --noTLS to include --bind_address,
and add TestNoTLSRequiresBindAddress to verify the enforcement.
Signed-off-by: xq9mend <xq9mend@users.noreply.github.com>
* telemetry: relax --noTLS validation to require any loopback address
Use net.ParseIP + IsLoopback() instead of exact string match for 127.0.0.1.
This accepts the full 127.0.0.0/8 range and ::1 (IPv6 loopback) while
rejecting empty/non-loopback addresses.
Also update test cases to include --bind_address 127.0.0.1 where needed,
and add TestNoTLSRequiresLoopbackAddress covering valid and invalid cases.
Signed-off-by: xq9mend <xq9mend@users.noreply.github.com>
* telemetry: fix syntax error and gofmt issues in loopback validation
- Remove extra closing brace introduced during conflict resolution
- Fix 'net' import to correct alphabetical position
- Fix string continuation indentation in fmt.Errorf
Signed-off-by: xq9mend <xq9mend@users.noreply.github.com>
* gnmi_server,telemetry: fix extra closing brace from conflict resolution
Remove stray } that was left in both server.go and telemetry.go
during manual conflict resolution. Fixes compilation errors.
Signed-off-by: xq9mend <xq9mend@users.noreply.github.com>
* telemetry: add --bind_address 127.0.0.1 to noTLS auth tests
TestStartGNMIServerNoTLS and TestNoTLSAuthNotBypassed were missing
--bind_address which is now required when using --noTLS.
Signed-off-by: xq9mend <xq9mend@users.noreply.github.com>
---------
Signed-off-by: xq9mend <xq9mend@users.noreply.github.com>
Co-authored-by: xq9mend <xq9mend@users.noreply.github.com>
* Move diff-cover enforcement from build job to DiffCoverageCheck
Remove DIFF_COVER_* variables from the build job so it can be renamed
without breaking the wrapper's GitHub check name coupling. The
DiffCoverageCheck job now sets DIFF_COVER_ENABLE and related variables,
making it the sole hook point for the sonic-build-web coverage wrapper.
The native diff-cover --fail-under step is removed since the wrapper
handles threshold enforcement via GitHub checks.
Also remove the now-unused global DIFF_COVER_THRESHOLD variable.
Signed-off-by: Dawei Huang <daweihuang@microsoft.com>
* Rename build job to IntegrationTest
Now that diff-cover enforcement is on DiffCoverageCheck, the build job
name is no longer coupled to the wrapper's GitHub check name. Rename it
to IntegrationTest to better describe what it does.
Signed-off-by: Dawei Huang <daweihuang@microsoft.com>
* Use 'build' as job id for DiffCoverageCheck
Reuse the 'build' job id for the diff-coverage job so the wrapper's
GitHub check name remains coverage.{DefName}.build, matching existing
branch protection rules. No admin changes needed.
Signed-off-by: Dawei Huang <daweihuang@microsoft.com>
* Set displayName to 'build' for diff-cover job
Azure Pipelines uses displayName (not job id) for the status context
reported to GitHub. The wrapper also uses Agent.JobName which resolves
to displayName. Set it back to 'build' so both the Azure Pipelines
check (sonic-net.sonic-gnmi (Build build)) and the wrapper check
(coverage.sonic-net.sonic-gnmi.build) match existing branch protection
rules.
Signed-off-by: Dawei Huang <daweihuang@microsoft.com>
---------
Signed-off-by: Dawei Huang <daweihuang@microsoft.com>
Signed-off-by: Niranjani Vivek <niranjaniv@google.com>
Signed-off-by: Niranjani Vivek <niranjaniv@google.com>
* Support maximum gnmi message size of 125k Signed-off-by: Connor Roos <croos@nvidia.com> * Correctly set default to 4MB Signed-off-by: Connor Roos <croos@nvidia.com> * Run gofmt on telemetry.go Signed-off-by: Connor Roos <croos@nvidia.com> --------- Signed-off-by: Connor Roos <croos@nvidia.com>
Signed-off-by: Niranjani Vivek <niranjaniv@google.com>
The SmartSwitch upgrade workflow needs to verify the running OS version on individual DPUs after upgrade. OS.Verify and OS.Activate are already implemented in sonic-gnmi but not registered in the DPU proxy's forwardable methods list, so requests with DPU metadata headers are rejected. Add both RPCs as ForwardToDPU entries so external clients can call OS.Verify and OS.Activate targeting specific DPUs via the NPU proxy. Signed-off-by: rookie-who <rookie-who@users.noreply.github.com> Co-authored-by: rookie-who <rookie-who@users.noreply.github.com>
Register OS.Verify and OS.Activate as ForwardToDPU methods in the DPU proxy, and implement the unary forwarding handlers so requests are actually proxied to the target DPU. Previously, only System.Time had a forwarding handler. Other unary methods registered as ForwardToDPU hit the default switch case and returned Unimplemented. Signed-off-by: rookie-who <rookie-who@users.noreply.github.com> Co-authored-by: rookie-who <rookie-who@users.noreply.github.com>
Signed-off-by: Neha Das nehadas@google.com Signed-off-by: Neha Das nehadas@google.com
…(#673) * Revert "telemetry: add --bind_address flag to restrict TCP listener (#652)" This reverts commit 4cc592b. * telemetry: add test for cert auth fallback when ca_crt is empty Signed-off-by: Dawei Huang <daweihuang@microsoft.com> --------- Signed-off-by: Dawei Huang <daweihuang@microsoft.com>
…e.xml (#656)
* ci: surface gofmt failures instead of masking them as missing coverage.xml
A recent build produced a confusing "Path does not exist: coverage.xml"
failure whose real root cause was a gofmt violation. Three separate
issues combined to hide it:
1. In the integration and memory-leak jobs the bash step was written as:
pushd sonic-gnmi
make all && ... make check_*_junit
popd
bash's 'set -e' is explicitly disabled for commands that are not the
final one in an '&&' list, so a failing 'make all' did not abort the
step. The final command (popd) returned 0, so the task was reported
as succeeded even though nothing built. coverage.xml was never
generated, which in turn failed the downstream publish step with a
misleading error.
2. The 'Publish integration coverage artifact' step used
condition: always(), which guaranteed a secondary failure on top of
any real earlier failure and made the real error harder to find.
3. The .formatcheck Makefile target printed the entire list of
~170 unformatted files on a single line with no diff, which is hard
to read in the Azure DevOps log viewer.
Changes:
- Replace 'pushd; cmd1 && cmd2; popd' with plain 'cd; cmd1; cmd2' in
the integration and memory-leak bash steps so 'set -e' actually
aborts the step on first failure.
- Switch the integration coverage artifact publish step to
condition: succeeded() so it no longer emits a confusing secondary
error when tests have already failed.
- Add a 'Go format check (gofmt)' step to the fast PureCIJob (no SONiC
dependencies) so formatting issues fail the build in seconds instead
of being buried inside a long integration test step.
- Improve the .formatcheck target: print each offending file on its
own line, include the first 200 lines of 'gofmt -d' output, and
point users at a new top-level 'make fmt' target to auto-fix.
- Add convenience 'fmt-check' and 'fmt' phony targets.
Signed-off-by: Dawei Huang <daweihuang@microsoft.com>
* ci: add StaticChecks stage that gates Build, BuildAmd64, BuildArm64
Before this change, a formatting violation still burned a full set of
integration-test and deb-build runs on the self-hosted amd64/arm64
pools (~30-40 min each, running in parallel) before the pipeline went
red. That is wasted agent time on a constrained pool.
Introduce a dedicated StaticChecks stage running on a hosted
ubuntu-22.04 image with no SONiC dependencies. It runs gofmt via the
'make fmt-check' target in about a minute from a cold agent. The
three downstream stages (Build, BuildAmd64, BuildArm64) now declare
'dependsOn: [StaticChecks]' so they do not even start if static
checks fail.
Remove the duplicate gofmt step that was added to PureCIJob in the
previous commit; the StaticChecks stage supersedes it.
The StaticChecks stage is intentionally scoped to be the home for
future cheap, dependency-free Go checks (vet, go mod tidy drift,
golangci-lint) so they can be added as additional steps in the same
job without restructuring the pipeline again.
Signed-off-by: Dawei Huang <daweihuang@microsoft.com>
* make: share one fmt-check/fmt implementation between Makefile and pure.mk
Previously the pure.mk `fmt-check` target loop-called `gofmt -l` over
PACKAGES (narrow scope, per-package output, no diff) while the
Makefile's `.formatcheck` stamp-file target used a different shell
block with a temp log file. Keeping two implementations in sync by
hand is a bug waiting to happen.
Make pure.mk the single source of truth:
- pure.mk defines GO_FILES with the same find invocation as the
Makefile and runs gofmt -l on that full set, printing the per-file
list plus the first 200 lines of gofmt -d.
- pure.mk 'fmt' runs 'gofmt -w' on the same GO_FILES set.
- The Makefile's phony 'fmt-check'/'fmt' targets now delegate to
'make -f pure.mk fmt-check' / 'make -f pure.mk fmt'.
- The Makefile's incremental $(FORMAT_CHECK) stamp-file target (still
a dependency of the main 'sonic-gnmi' build target) uses the same
shell block as pure.mk, so running the full build or the standalone
target produces identical output.
- Drop the unused FORMAT_LOG variable.
Signed-off-by: Dawei Huang <daweihuang@microsoft.com>
* make: revert cross-makefile delegation for fmt-check
Previous commit made the Makefile's fmt-check delegate to pure.mk.
That is wrong: Makefile and pure.mk own different scopes (full repo
vs. pure packages) by design, and neither should depend on the other.
Revert to self-contained targets in each file:
- Makefile's fmt-check/fmt target scope is the full-repo GO_FILES list
via the existing $(FORMAT_CHECK) stamp-file target.
- pure.mk's fmt-check/fmt target scope is the PACKAGES list only.
Both targets use the same shell idiom and produce the same output
format (per-file list followed by the first 200 lines of 'gofmt -d'
plus a fix-it hint), but they are independent.
Signed-off-by: Dawei Huang <daweihuang@microsoft.com>
* ci: drop Makefile/pure.mk changes, inline gofmt in StaticChecks stage
The format check is a single 'gofmt -l' invocation; there is no need
for a new make target to wrap it. Revert Makefile and pure.mk to
their state on master and run gofmt inline from the StaticChecks
stage. This keeps build-system changes out of a CI-only fix.
The stage prints the list of offending files followed by the first
200 lines of 'gofmt -d' so a PR author can see exactly what needs
to change.
Signed-off-by: Dawei Huang <daweihuang@microsoft.com>
* ci: build sonic-mgmt-common in MemoryLeakJob before 'make all'
Build 1094963 failed in the Memory Leak Tests step with:
No such file or directory: '.../sonic-mgmt-common/build/yang/*.yang'
Root cause: MemoryLeakJob checks out sonic-mgmt-common but never
builds it. sonic-gnmi's 'make all' invokes a pyang proto-gen target
(Makefile:178) that globs yang files from
$(MGMT_COMMON_DIR)/build/yang — a directory that only exists after
dpkg-buildpackage has run in sonic-mgmt-common. IntegrationTest
already has an explicit 'Build sonic-mgmt-common' step; MemoryLeakJob
was missing it.
This bug was previously hidden by the same 'make all && ... popd'
pattern this PR fixed: 'make all' failed, && short-circuited, popd
returned 0, the task was reported succeeded, and check_memleak_junit
silently never ran. Memory leak tests have been a no-op on CI. With
'set -e' now honored correctly, the missing build step became
visible.
Add the 'Build sonic-mgmt-common' step to MemoryLeakJob, identical
to the one already present in IntegrationTest.
Signed-off-by: Dawei Huang <daweihuang@microsoft.com>
* ci: restructure azure-pipelines layout and factor shared templates
Rename and regroup stages/jobs so the layout matches what each piece
actually does, and extract the two biggest copy-paste blocks (Go install,
test-env preamble) into templates.
Stages:
Build -> Test (it runs tests, not a build)
BuildAmd64 -> Package (now a single stage with amd64 and arm64 jobs)
BuildArm64 -> Package
Jobs:
GoStaticChecks -> go_static_checks
PureCIJob -> pure_tests
MemoryLeakJob -> memleak_tests
IntegrationTest -> integration_tests
The 'build' job inside the Test stage is intentionally left with both
'job: build' and 'displayName: "build"'. The pipeline decorator that
publishes the diff-coverage check derives its GitHub name as
coverage.{BuildDefinitionName}.{Agent.JobName}, and Agent.JobName
resolves to the displayName. Renaming either would break the
coverage.sonic-net.sonic-gnmi.build check. Documented in the file
header.
Also:
* Drop the redundant 'Run Pure Package Tests (coverage only)' step
from the integration job; pure coverage is already produced by
pure_tests and consumed via the coverage-pure artifact.
* Hoist GO_VERSION to a pipeline-level variable.
* Factor .azure/templates/install-go.yml (was duplicated in two jobs).
* Factor .azure/templates/setup-test-env.yml (checkout x3 +
install-dependencies + build sonic-mgmt-common) used by both
memleak_tests and integration_tests.
Signed-off-by: Dawei Huang <daweihuang@microsoft.com>
---------
Signed-off-by: Dawei Huang <daweihuang@microsoft.com>
* Add OS.Verify and OS.Activate DPU proxy forwarding Register OS.Verify and OS.Activate as ForwardToDPU methods in the DPU proxy, and implement the unary forwarding handlers so requests are actually proxied to the target DPU. Previously, only System.Time had a forwarding handler. Other unary methods registered as ForwardToDPU hit the default switch case and returned Unimplemented. Signed-off-by: rookie-who <rookie-who@users.noreply.github.com> * Add completeness test for DPU proxy ForwardToDPU handlers Add TestAllForwardToDPUMethodsHaveHandlers which validates that every method registered as ForwardToDPU in defaultForwardableMethods has a corresponding forwarding handler. Signed-off-by: rookie-who <rookie-who@users.noreply.github.com> --------- Signed-off-by: rookie-who <rookie-who@users.noreply.github.com> Co-authored-by: rookie-who <rookie-who@users.noreply.github.com>
…_master # Conflicts: # .azure/templates/build-deb.yml # Makefile # azure-pipelines.yml # gnmi_server/cli_helpers_test.go # gnmi_server/interface_cli_test.go # gnmi_server/server.go # gnmi_server/virtual_db_test.go # show_client/interface_cli.go # sonic_data_client/client_test.go # sonic_data_client/mixed_db_client.go # sonic_data_client/virtual_db.go # telemetry/telemetry.go
Contributor
Author
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
The upstream-master and azure/kubesonic both added the same initCounters*Once / clearMappingsMu var block at different positions, and neither was inside a conflict marker, so the merge tool kept both. Drop the second copy. Signed-off-by: Zain Budhwani <zainbudhwani@microsoft.com>
Contributor
Author
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
sharmavijay-ms
approved these changes
May 18, 2026
sharmavijay-ms
left a comment
sharmavijay-ms
left a comment
There was a problem hiding this comment.
Big change. Reviewed at high level. LGTM.
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
9 participants
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.
Why I did it
How I did it
How to verify it
Which release branch to backport (provide reason below if selected)
Description for the changelog
Link to config_db schema for YANG module changes
A picture of a cute animal (not mandatory but encouraged)