From 80cb3288ac2fd75b420ba77feb9ab7591405c800 Mon Sep 17 00:00:00 2001 From: mesutoezdil Date: Thu, 16 Jul 2026 23:51:51 +0200 Subject: [PATCH 1/3] fix(byoo-otel-collector): log otelcol-contrib exit error on shutdown runSecretsCheckLoop discarded the error from Process.Wait() after both a graceful shutdown and a secrets-triggered restart, so a non-zero or signal exit from otelcol-contrib was silently dropped. Log it instead. Verified with go build, go vet, go test ./..., and golangci-lint --enable-only errcheck, all clean. NO-REF Signed-off-by: mesutoezdil --- .../byoo-otel-collector/internal/cli/cli.go | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/src/compute-plane-services/byoo-otel-collector/internal/cli/cli.go b/src/compute-plane-services/byoo-otel-collector/internal/cli/cli.go index fe051196a..e13609974 100644 --- a/src/compute-plane-services/byoo-otel-collector/internal/cli/cli.go +++ b/src/compute-plane-services/byoo-otel-collector/internal/cli/cli.go @@ -72,7 +72,9 @@ func runSecretsCheckLoop(ctx context.Context, otelCollectorProc *os.Process, arg logger.Logger.Info("Received interrupt signal, terminating otelcol-contrib and exiting.") otelcollector.GracefulShutdown(otelCollectorProc) // Wait for the process to actually exit - otelCollectorProc.Wait() + if _, waitErr := otelCollectorProc.Wait(); waitErr != nil { + logger.Logger.Errorf("error waiting for otelcol-contrib to exit: %v", waitErr) + } return nil case <-restartCh: logger.Logger.Info("Regenerating the secret files and restarting otelcol-contrib due to the secret file changes.") @@ -88,7 +90,9 @@ func runSecretsCheckLoop(ctx context.Context, otelCollectorProc *os.Process, arg // shutdown the current otelcol-contrib process otelcollector.GracefulShutdown(otelCollectorProc) // Wait for it to exit - otelCollectorProc.Wait() + if _, waitErr := otelCollectorProc.Wait(); waitErr != nil { + logger.Logger.Errorf("error waiting for otelcol-contrib to exit: %v", waitErr) + } // run the otelcol-contrib process again if otelCollectorProc, err = otelcollector.RunOtelCollector(args); err != nil { From c6795b557501dbead343b6433f76fa8874922569 Mon Sep 17 00:00:00 2001 From: mesutoezdil Date: Fri, 17 Jul 2026 18:14:51 +0200 Subject: [PATCH 2/3] test(byoo-otel-collector): cover wait-error logging paths Extract the duplicated wait-and-log logic behind otelCollectorProc.Wait() into waitAndLogProcessExit, taking a small interface instead of the concrete *os.Process so both the interrupt and secret-triggered restart paths can be exercised with a fake in a unit test. JIRA: NO-REF NVBug: none Signed-off-by: mesutoezdil --- .../internal/cli/BUILD.bazel | 14 +++- .../byoo-otel-collector/internal/cli/cli.go | 22 ++++-- .../internal/cli/cli_test.go | 67 +++++++++++++++++++ 3 files changed, 96 insertions(+), 7 deletions(-) create mode 100644 src/compute-plane-services/byoo-otel-collector/internal/cli/cli_test.go diff --git a/src/compute-plane-services/byoo-otel-collector/internal/cli/BUILD.bazel b/src/compute-plane-services/byoo-otel-collector/internal/cli/BUILD.bazel index 466d402ce..9367bbad4 100644 --- a/src/compute-plane-services/byoo-otel-collector/internal/cli/BUILD.bazel +++ b/src/compute-plane-services/byoo-otel-collector/internal/cli/BUILD.bazel @@ -1,4 +1,4 @@ -load("@rules_go//go:def.bzl", "go_library") +load("@rules_go//go:def.bzl", "go_library", "go_test") go_library( name = "cli", @@ -21,3 +21,15 @@ alias( actual = ":cli", visibility = ["//:__subpackages__"], ) + +go_test( + name = "cli_test", + srcs = ["cli_test.go"], + embed = [":cli"], + deps = [ + "//internal/logger", + "@org_uber_go_zap//:zap", + "@org_uber_go_zap//zapcore", + "@org_uber_go_zap//zaptest/observer", + ], +) diff --git a/src/compute-plane-services/byoo-otel-collector/internal/cli/cli.go b/src/compute-plane-services/byoo-otel-collector/internal/cli/cli.go index e13609974..664f6bab7 100644 --- a/src/compute-plane-services/byoo-otel-collector/internal/cli/cli.go +++ b/src/compute-plane-services/byoo-otel-collector/internal/cli/cli.go @@ -37,6 +37,20 @@ import ( "github.com/NVIDIA/nvcf/src/compute-plane-services/byoo-otel-collector/internal/secrets" ) +// processWaiter is the subset of *os.Process used by runSecretsCheckLoop, extracted so tests +// can exercise the wait-error logging without spawning a real OS process. +type processWaiter interface { + Wait() (*os.ProcessState, error) +} + +// waitAndLogProcessExit waits for otelcol-contrib to exit and logs a non-nil error instead of +// discarding it. +func waitAndLogProcessExit(proc processWaiter) { + if _, waitErr := proc.Wait(); waitErr != nil { + logger.Logger.Errorf("error waiting for otelcol-contrib to exit: %v", waitErr) + } +} + func runSecretsCheckLoop(ctx context.Context, otelCollectorProc *os.Process, args []string, accountsSecrets, secretsFolder string, lastContent []byte, lastModTime time.Time) error { restartCh := make(chan struct{}, 1) @@ -72,9 +86,7 @@ func runSecretsCheckLoop(ctx context.Context, otelCollectorProc *os.Process, arg logger.Logger.Info("Received interrupt signal, terminating otelcol-contrib and exiting.") otelcollector.GracefulShutdown(otelCollectorProc) // Wait for the process to actually exit - if _, waitErr := otelCollectorProc.Wait(); waitErr != nil { - logger.Logger.Errorf("error waiting for otelcol-contrib to exit: %v", waitErr) - } + waitAndLogProcessExit(otelCollectorProc) return nil case <-restartCh: logger.Logger.Info("Regenerating the secret files and restarting otelcol-contrib due to the secret file changes.") @@ -90,9 +102,7 @@ func runSecretsCheckLoop(ctx context.Context, otelCollectorProc *os.Process, arg // shutdown the current otelcol-contrib process otelcollector.GracefulShutdown(otelCollectorProc) // Wait for it to exit - if _, waitErr := otelCollectorProc.Wait(); waitErr != nil { - logger.Logger.Errorf("error waiting for otelcol-contrib to exit: %v", waitErr) - } + waitAndLogProcessExit(otelCollectorProc) // run the otelcol-contrib process again if otelCollectorProc, err = otelcollector.RunOtelCollector(args); err != nil { diff --git a/src/compute-plane-services/byoo-otel-collector/internal/cli/cli_test.go b/src/compute-plane-services/byoo-otel-collector/internal/cli/cli_test.go new file mode 100644 index 000000000..eeda98809 --- /dev/null +++ b/src/compute-plane-services/byoo-otel-collector/internal/cli/cli_test.go @@ -0,0 +1,67 @@ +/* +SPDX-FileCopyrightText: Copyright (c) NVIDIA CORPORATION & AFFILIATES. All rights reserved. +SPDX-License-Identifier: Apache-2.0 + +Licensed under the Apache License, Version 2.0 (the "License"); +you may not use this file except in compliance with the License. +You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. +*/ + +package cli + +import ( + "errors" + "os" + "testing" + + "go.uber.org/zap" + "go.uber.org/zap/zapcore" + "go.uber.org/zap/zaptest/observer" + + "github.com/NVIDIA/nvcf/src/compute-plane-services/byoo-otel-collector/internal/logger" +) + +type fakeProcess struct { + waitErr error +} + +func (f *fakeProcess) Wait() (*os.ProcessState, error) { + return nil, f.waitErr +} + +func TestWaitAndLogProcessExit(t *testing.T) { + tests := []struct { + name string + waitErr error + wantLog bool + }{ + {name: "interrupt shutdown wait error is logged", waitErr: errors.New("wait: no child processes"), wantLog: true}, + {name: "secret-triggered restart wait error is logged", waitErr: errors.New("signal: killed"), wantLog: true}, + {name: "clean exit logs nothing", waitErr: nil, wantLog: false}, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + core, logs := observer.New(zapcore.DebugLevel) + logger.Logger = zap.New(core).Sugar() + + waitAndLogProcessExit(&fakeProcess{waitErr: tt.waitErr}) + + entries := logs.TakeAll() + if tt.wantLog && len(entries) != 1 { + t.Fatalf("expected 1 log entry, got %d", len(entries)) + } + if !tt.wantLog && len(entries) != 0 { + t.Fatalf("expected no log entries, got %d", len(entries)) + } + }) + } +} From 101666b47942adb4eedfd80d6cde4d995ff65203 Mon Sep 17 00:00:00 2001 From: mesutoezdil Date: Fri, 17 Jul 2026 18:22:51 +0200 Subject: [PATCH 3/3] test(byoo-otel-collector): address wait-error test review feedback Clarify the test names as helper-level coverage, restore the global logger after each case, and assert the logged message contains the underlying wait error text instead of only counting log entries. JIRA: NO-REF NVBug: none Signed-off-by: mesutoezdil --- .../internal/cli/cli_test.go | 23 +++++++++++++++---- 1 file changed, 18 insertions(+), 5 deletions(-) diff --git a/src/compute-plane-services/byoo-otel-collector/internal/cli/cli_test.go b/src/compute-plane-services/byoo-otel-collector/internal/cli/cli_test.go index eeda98809..687a8231c 100644 --- a/src/compute-plane-services/byoo-otel-collector/internal/cli/cli_test.go +++ b/src/compute-plane-services/byoo-otel-collector/internal/cli/cli_test.go @@ -20,6 +20,7 @@ package cli import ( "errors" "os" + "strings" "testing" "go.uber.org/zap" @@ -37,30 +38,42 @@ func (f *fakeProcess) Wait() (*os.ProcessState, error) { return nil, f.waitErr } +// TestWaitAndLogProcessExit covers waitAndLogProcessExit directly: both runSecretsCheckLoop +// call sites (interrupt shutdown and secret-triggered restart) delegate to this same helper, so +// exercising it once here covers both. func TestWaitAndLogProcessExit(t *testing.T) { tests := []struct { name string waitErr error wantLog bool }{ - {name: "interrupt shutdown wait error is logged", waitErr: errors.New("wait: no child processes"), wantLog: true}, - {name: "secret-triggered restart wait error is logged", waitErr: errors.New("signal: killed"), wantLog: true}, + {name: "wait error is logged with its message", waitErr: errors.New("wait: no child processes"), wantLog: true}, + {name: "a different wait error message is preserved verbatim", waitErr: errors.New("signal: killed"), wantLog: true}, {name: "clean exit logs nothing", waitErr: nil, wantLog: false}, } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { + orig := logger.Logger + t.Cleanup(func() { logger.Logger = orig }) + core, logs := observer.New(zapcore.DebugLevel) logger.Logger = zap.New(core).Sugar() waitAndLogProcessExit(&fakeProcess{waitErr: tt.waitErr}) entries := logs.TakeAll() - if tt.wantLog && len(entries) != 1 { + if !tt.wantLog { + if len(entries) != 0 { + t.Fatalf("expected no log entries, got %d", len(entries)) + } + return + } + if len(entries) != 1 { t.Fatalf("expected 1 log entry, got %d", len(entries)) } - if !tt.wantLog && len(entries) != 0 { - t.Fatalf("expected no log entries, got %d", len(entries)) + if !strings.Contains(entries[0].Message, tt.waitErr.Error()) { + t.Fatalf("expected log message to contain %q, got %q", tt.waitErr.Error(), entries[0].Message) } }) }