From 110326b5a18f868819e63f3f5adc2b07c2e17c20 Mon Sep 17 00:00:00 2001 From: Zain Budhwani Date: Tue, 23 Apr 2024 00:00:15 +0000 Subject: [PATCH 1/6] Add graceful stop --- gnmi_server/server.go | 2 +- gnmi_server/server_test.go | 5 +++++ 2 files changed, 6 insertions(+), 1 deletion(-) diff --git a/gnmi_server/server.go b/gnmi_server/server.go index 4ac4f3a0..80787846 100644 --- a/gnmi_server/server.go +++ b/gnmi_server/server.go @@ -183,7 +183,7 @@ func (srv *Server) Stop() { log.Errorf("Stop() failed: not initialized") return } - s.Stop() + s.GracefulStop() } // Address returns the port the Server is listening to. diff --git a/gnmi_server/server_test.go b/gnmi_server/server_test.go index 8c660f24..6733c8b0 100644 --- a/gnmi_server/server_test.go +++ b/gnmi_server/server_test.go @@ -3503,11 +3503,13 @@ func TestConnectionsKeepAlive(t *testing.T) { }, } for _, tt := range(tests) { + var clients []*cacheclient.CacheClient for i := 0; i < 5; i++ { t.Run(tt.desc, func(t *testing.T) { q := tt.q q.Addrs = []string{"127.0.0.1:8081"} c := client.New() + clients = append(clients, c) wg := new(sync.WaitGroup) wg.Add(1) @@ -3529,6 +3531,9 @@ func TestConnectionsKeepAlive(t *testing.T) { } }) } + for _, cacheClient := range(clients) { + cacheClient.Close() + } } } From 91d04af177fbeb322feb4f47cf0578f87dfb00b5 Mon Sep 17 00:00:00 2001 From: Zain Budhwani Date: Mon, 1 Jul 2024 23:38:53 +0000 Subject: [PATCH 2/6] Use force stop for non cert rotation signals --- gnmi_server/server.go | 9 +++++++++ telemetry/telemetry.go | 5 +++-- telemetry/telemetry_test.go | 2 ++ 3 files changed, 14 insertions(+), 2 deletions(-) diff --git a/gnmi_server/server.go b/gnmi_server/server.go index 80787846..d02d8053 100644 --- a/gnmi_server/server.go +++ b/gnmi_server/server.go @@ -177,6 +177,15 @@ func (srv *Server) Serve() error { return srv.s.Serve(srv.lis) } +func (srv *Server) ForceStop() { + s := srv.s + if s == nil { + log.Errorf("ForceStop() failed: not initialized") + return + } + s.Stop() +} + func (srv *Server) Stop() { s := srv.s if s == nil { diff --git a/telemetry/telemetry.go b/telemetry/telemetry.go index 2cf0284f..efed369b 100644 --- a/telemetry/telemetry.go +++ b/telemetry/telemetry.go @@ -86,7 +86,7 @@ func runTelemetry(args []string) error { var serverControlSignal = make(chan ServerControlValue, 1) var stopSignalHandler = make(chan bool, 1) sigchannel := make(chan os.Signal, 1) - signal.Notify(sigchannel, syscall.SIGTERM, syscall.SIGQUIT) + signal.Notify(sigchannel, syscall.SIGTERM, syscall.SIGQUIT, syscall.SIGINT, syscall.SIGHUP) wg.Add(1) @@ -455,12 +455,13 @@ func startGNMIServer(telemetryCfg *TelemetryConfig, cfg *gnmi.Config, serverCont serverControlValue := <-serverControlSignal log.V(1).Infof("Received signal for gnmi server to close") - s.Stop() if serverControlValue == ServerStop { + s.ForceStop() // No graceful stop stopSignalHandler <- true log.Flush() return } + s.Stop() // Graceful stop // Both ServerStart and ServerRestart will loop and restart server // We use different value to distinguish between write/create and remove/rename } diff --git a/telemetry/telemetry_test.go b/telemetry/telemetry_test.go index 03bcd1fc..5f139b65 100644 --- a/telemetry/telemetry_test.go +++ b/telemetry/telemetry_test.go @@ -817,6 +817,8 @@ func TestINotifyCertMonitoringAddWatcherError(t *testing.T) { func TestSignalHandler(t *testing.T) { testHandlerSyscall(t, syscall.SIGTERM) testHandlerSyscall(t, syscall.SIGQUIT) + testHandlerSyscall(t, syscall.SIGINT) + testHandlerSyscall(t, syscall.SIGHUP) testHandlerSyscall(t, nil) // Test that ServerStop should make signalHandler exit } From 026a56e42bd39b2353f59888c860d2b3e66e0fb8 Mon Sep 17 00:00:00 2001 From: Zain Budhwani Date: Tue, 2 Jul 2024 01:00:41 +0000 Subject: [PATCH 3/6] Fix UT --- telemetry/telemetry_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/telemetry/telemetry_test.go b/telemetry/telemetry_test.go index 9e9bb4e1..10685a9b 100644 --- a/telemetry/telemetry_test.go +++ b/telemetry/telemetry_test.go @@ -186,7 +186,7 @@ func TestStartGNMIServer(t *testing.T) { wg := &sync.WaitGroup{} exitCalled := false - patches.ApplyMethod(reflect.TypeOf(&gnmi.Server{}), "Stop", func(_ *gnmi.Server) { + patches.ApplyMethod(reflect.TypeOf(&gnmi.Server{}), "ForceStop", func(_ *gnmi.Server) { exitCalled = true }) From 72d50a6182cb2bd52d03c4f01d1f2e8b471f5fe6 Mon Sep 17 00:00:00 2001 From: Zain Budhwani Date: Tue, 2 Jul 2024 18:58:53 +0000 Subject: [PATCH 4/6] Add UT for graceful stop --- telemetry/telemetry_test.go | 87 ++++++++++++++++++++++++++++++++++++- 1 file changed, 86 insertions(+), 1 deletion(-) diff --git a/telemetry/telemetry_test.go b/telemetry/telemetry_test.go index 9e9bb4e1..5a26dd1b 100644 --- a/telemetry/telemetry_test.go +++ b/telemetry/telemetry_test.go @@ -186,7 +186,7 @@ func TestStartGNMIServer(t *testing.T) { wg := &sync.WaitGroup{} exitCalled := false - patches.ApplyMethod(reflect.TypeOf(&gnmi.Server{}), "Stop", func(_ *gnmi.Server) { + patches.ApplyMethod(reflect.TypeOf(&gnmi.Server{}), "ForceStop", func(_ *gnmi.Server) { exitCalled = true }) @@ -206,6 +206,91 @@ func TestStartGNMIServer(t *testing.T) { wg.Wait() + if !exitCalled { + t.Errorf("s.ForceStop should be called if gnmi server is called to shutdown") + } +} + +func TestStartGNMIServerGracefulStop(t *testing.T) { + testServerCert := "../testdata/certs/testserver.cert" + testServerKey := "../testdata/certs/testserver.key" + timeoutInterval := 15 + tick := time.NewTicker(100 * time.Millisecond) + defer tick.Stop() + + ctx, cancel := context.WithTimeout(context.Background(), time.Duration(timeoutInterval) * time.Second) + defer cancel() + + originalArgs := os.Args + defer func() { + os.Args = originalArgs + }() + + fs := flag.NewFlagSet("testStartGNMIServer", flag.ContinueOnError) + os.Args = []string{"cmd", "-port", "8080", "-server_crt", testServerCert, "-server_key", testServerKey} + telemetryCfg, cfg, err := setupFlags(fs) + + if err != nil { + t.Errorf("Expected err to be nil, got err %v", err) + } + + patches := gomonkey.ApplyFunc(tls.LoadX509KeyPair, func(certFile, keyFile string) (tls.Certificate, error) { + return tls.Certificate{}, nil + }) + patches.ApplyFunc(gnmi.NewServer, func(cfg *gnmi.Config, opts []grpc.ServerOption) (*gnmi.Server, error) { + return &gnmi.Server{}, nil + }) + patches.ApplyFunc(grpc.Creds, func(credentials.TransportCredentials) grpc.ServerOption { + return grpc.EmptyServerOption{} + }) + patches.ApplyMethod(reflect.TypeOf(&gnmi.Server{}), "Serve", func(_ *gnmi.Server) error { + return nil + }) + patches.ApplyMethod(reflect.TypeOf(&gnmi.Server{}), "Address", func(_ *gnmi.Server) string { + return "" + }) + patches.ApplyFunc(iNotifyCertMonitoring, func(_ *fsnotify.Watcher, _ *TelemetryConfig, serverControlSignal chan<- ServerControlValue, testReadySignal chan<- int, certLoaded *int32) { + }) + + serverControlSignal := make(chan ServerControlValue, 1) + stopSignalHandler := make(chan bool, 1) + wg := &sync.WaitGroup{} + + counter := 0 + exitCalled := false + patches.ApplyMethod(reflect.TypeOf(&gnmi.Server{}), "Stop", func(_ *gnmi.Server) { + exitCalled = true + }) + patches.ApplyMethod(reflect.TypeOf(&gnmi.Server{}), "ForceStop", func(_ *gnmi.Server) { + }) + + + defer patches.Reset() + + wg.Add(1) + + go startGNMIServer(telemetryCfg, cfg, serverControlSignal, stopSignalHandler, wg) + + for { + select { + case <-tick.C: + if counter == 0 { // simulate cert rotation first + sendSignal(serverControlSignal, ServerRestart) + } else { // simulate sigterm second + sendSignal(serverControlSignal, ServerStop) + } + counter += 1 + case <-ctx.Done(): + t.Errorf("Failed to send shutdown signal") + return + } + if counter > 1 { // both signals have been sent + break + } + } + + wg.Wait() + if !exitCalled { t.Errorf("s.Stop should be called if gnmi server is called to shutdown") } From 191ce8e8f46a3e1d765035c109ed4472895c8366 Mon Sep 17 00:00:00 2001 From: Zain Budhwani Date: Tue, 2 Jul 2024 18:59:31 +0000 Subject: [PATCH 5/6] Add coverage for force stop --- gnmi_server/server_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/gnmi_server/server_test.go b/gnmi_server/server_test.go index 30f38253..ef6316b5 100644 --- a/gnmi_server/server_test.go +++ b/gnmi_server/server_test.go @@ -3419,7 +3419,7 @@ func TestClientConnections(t *testing.T) { func TestConnectionDataSet(t *testing.T) { s := createServer(t, 8081) go runServer(t, s) - defer s.Stop() + defer s.ForceStop() tests := []struct { desc string From 668b996ae33e14d9565646c12bb1595c75f89c83 Mon Sep 17 00:00:00 2001 From: Zain Budhwani Date: Tue, 2 Jul 2024 20:05:48 +0000 Subject: [PATCH 6/6] Add additional UT --- gnmi_server/server_test.go | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/gnmi_server/server_test.go b/gnmi_server/server_test.go index ef6316b5..4ec9e71f 100644 --- a/gnmi_server/server_test.go +++ b/gnmi_server/server_test.go @@ -3946,6 +3946,13 @@ func TestNilServerStop(t *testing.T) { s.Stop() } +func TestNilServerForceStop(t *testing.T) { + // Create a server with nil grpc server, such that s.ForceStop is called with nil value + t.Log("Expecting s.ForceStop to log error as server is nil") + s := &Server{} + s.ForceStop() +} + func TestInvalidServer(t *testing.T) { s := createInvalidServer(t, 9000) if s != nil {