Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Use server GracefulStop instead of force stop #219

Merged
merged 9 commits into from
Jul 2, 2024
Merged
11 changes: 10 additions & 1 deletion gnmi_server/server.go
Original file line number Diff line number Diff line change
Expand Up @@ -188,13 +188,22 @@ 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 {
log.Errorf("Stop() failed: not initialized")
return
}
s.Stop()
s.GracefulStop()
zbud-msft marked this conversation as resolved.
Show resolved Hide resolved
}

// Address returns the port the Server is listening to.
Expand Down
17 changes: 11 additions & 6 deletions gnmi_server/server_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -3491,16 +3491,16 @@ func TestConnectionsKeepAlive(t *testing.T) {
defer s.Stop()

tests := []struct {
desc string
q client.Query
want []client.Notification
poll int
desc string
q client.Query
want []client.Notification
poll int
}{
{
desc: "Testing KeepAlive with goroutine count",
poll: 3,
q: client.Query{
Target: "COUNTERS_DB",
Target: "COUNTERS_DB",
Type: client.Poll,
Queries: []client.Path{{"COUNTERS", "Ethernet*"}},
TLS: &tls.Config{InsecureSkipVerify: true},
Expand All @@ -3511,12 +3511,14 @@ func TestConnectionsKeepAlive(t *testing.T) {
},
},
}
for _, tt := range tests {
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)

Expand All @@ -3538,6 +3540,9 @@ func TestConnectionsKeepAlive(t *testing.T) {
}
})
}
for _, cacheClient := range(clients) {
cacheClient.Close()
}
}
}

Expand Down
5 changes: 3 additions & 2 deletions telemetry/telemetry.go
Original file line number Diff line number Diff line change
Expand Up @@ -90,7 +90,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)

Expand Down Expand Up @@ -464,12 +464,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
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why not stop signalHandler and flush log?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

s.Stop() is a graceful stop which is only reserved for cert rotation. Since we are not trying to exit process, just stop the server, we will not need to return or flush logs.

// Both ServerStart and ServerRestart will loop and restart server
// We use different value to distinguish between write/create and remove/rename
}
Expand Down
4 changes: 3 additions & 1 deletion telemetry/telemetry_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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
})

Expand Down Expand Up @@ -822,6 +822,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
}

Expand Down
Loading