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 fe051196a..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,7 +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 - otelCollectorProc.Wait() + waitAndLogProcessExit(otelCollectorProc) return nil case <-restartCh: logger.Logger.Info("Regenerating the secret files and restarting otelcol-contrib due to the secret file changes.") @@ -88,7 +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 - otelCollectorProc.Wait() + 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..687a8231c --- /dev/null +++ b/src/compute-plane-services/byoo-otel-collector/internal/cli/cli_test.go @@ -0,0 +1,80 @@ +/* +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" + "strings" + "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 +} + +// 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: "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 { + 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 !strings.Contains(entries[0].Message, tt.waitErr.Error()) { + t.Fatalf("expected log message to contain %q, got %q", tt.waitErr.Error(), entries[0].Message) + } + }) + } +}