You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@apisix.apache.org by to...@apache.org on 2021/05/10 09:03:23 UTC
[apisix-dashboard] branch master updated: fix: efficient error
handling in manager-api including graceful shutdown,
self contained methods. (#1814)
This is an automated email from the ASF dual-hosted git repository.
tokers pushed a commit to branch master
in repository https://gitbox.apache.org/repos/asf/apisix-dashboard.git
The following commit(s) were added to refs/heads/master by this push:
new 260b767 fix: efficient error handling in manager-api including graceful shutdown, self contained methods. (#1814)
260b767 is described below
commit 260b767476bf5301e6cc3a438f2483b02e96551d
Author: Bisakh Mondal <bi...@gmail.com>
AuthorDate: Mon May 10 14:33:14 2021 +0530
fix: efficient error handling in manager-api including graceful shutdown, self contained methods. (#1814)
---
api/cmd/managerapi.go | 66 ++++++++++++++++++++++-------------
api/internal/core/store/store.go | 6 ++--
api/internal/core/store/store_test.go | 3 +-
api/internal/utils/pid.go | 3 ++
api/test/shell/cli_test.sh | 53 ++++++++++++++++++++++++++++
5 files changed, 104 insertions(+), 27 deletions(-)
diff --git a/api/cmd/managerapi.go b/api/cmd/managerapi.go
index 4f56b5f..a3543b1 100644
--- a/api/cmd/managerapi.go
+++ b/api/cmd/managerapi.go
@@ -101,7 +101,7 @@ func manageAPI() error {
if err := utils.WritePID(conf.PIDPath); err != nil {
log.Errorf("failed to write pid: %s", err)
- panic(err)
+ return err
}
utils.AppendToClosers(func() error {
if err := os.Remove(conf.PIDPath); err != nil {
@@ -111,6 +111,15 @@ func manageAPI() error {
return nil
})
+ // Signal received to the process externally.
+ quit := make(chan os.Signal, 1)
+ signal.Notify(quit, syscall.SIGINT, syscall.SIGTERM)
+
+ defer func() {
+ utils.CloseAll()
+ signal.Stop(quit)
+ }()
+
droplet.Option.Orchestrator = func(mws []droplet.Middleware) []droplet.Middleware {
var newMws []droplet.Middleware
// default middleware order: resp_reshape, auto_input, traffic_log
@@ -122,17 +131,21 @@ func manageAPI() error {
if err := storage.InitETCDClient(conf.ETCDConfig); err != nil {
log.Errorf("init etcd client fail: %w", err)
- panic(err)
+ return err
}
if err := store.InitStores(); err != nil {
log.Errorf("init stores fail: %w", err)
- panic(err)
+ return err
}
+ var server, serverSSL *http.Server
+ // For internal error handling across multiple goroutines.
+ errsig := make(chan error, 1)
+
// routes
r := internal.SetUpRouter()
- addr := fmt.Sprintf("%s:%d", conf.ServerHost, conf.ServerPort)
- s := &http.Server{
+ addr := net.JoinHostPort(conf.ServerHost, strconv.Itoa(conf.ServerPort))
+ server = &http.Server{
Addr: addr,
Handler: r,
ReadTimeout: time.Duration(1000) * time.Millisecond,
@@ -141,21 +154,17 @@ func manageAPI() error {
log.Infof("The Manager API is listening on %s", addr)
- quit := make(chan os.Signal, 1)
- signal.Notify(quit, syscall.SIGINT, syscall.SIGTERM)
- defer signal.Stop(quit)
-
go func() {
- if err := s.ListenAndServe(); err != nil && err != http.ErrServerClosed {
- utils.CloseAll()
- log.Fatalf("listen and serv fail: %s", err)
+ if err := server.ListenAndServe(); err != nil && err != http.ErrServerClosed {
+ log.Errorf("listen and serv fail: %s", err)
+ errsig <- err
}
}()
// HTTPS
if conf.SSLCert != "" && conf.SSLKey != "" {
addrSSL := net.JoinHostPort(conf.ServerHost, strconv.Itoa(conf.SSLPort))
- serverSSL := &http.Server{
+ serverSSL = &http.Server{
Addr: addrSSL,
Handler: r,
ReadTimeout: time.Duration(1000) * time.Millisecond,
@@ -169,28 +178,37 @@ func manageAPI() error {
go func() {
err := serverSSL.ListenAndServeTLS(conf.SSLCert, conf.SSLKey)
if err != nil && err != http.ErrServerClosed {
- utils.CloseAll()
- log.Fatalf("listen and serve for HTTPS failed: %s", err)
+ log.Errorf("listen and serve for HTTPS failed: %s", err)
+ errsig <- err
}
}()
}
printInfo()
- sig := <-quit
- log.Infof("The Manager API server receive %s and start shutting down", sig.String())
+ select {
+ case err := <-errsig:
+ return err
- ctx, cancel := context.WithTimeout(context.TODO(), 5*time.Second)
- defer cancel()
+ case sig := <-quit:
+ log.Infof("The Manager API server receive %s and start shutting down", sig.String())
- if err := s.Shutdown(ctx); err != nil {
- log.Errorf("Shutting down server error: %s", err)
+ shutdownServer(server)
+ shutdownServer(serverSSL)
+ log.Infof("The Manager API server exited")
+ return nil
}
+}
- log.Infof("The Manager API server exited")
+func shutdownServer(server *http.Server) {
+ if server != nil {
+ ctx, cancel := context.WithTimeout(context.TODO(), 5*time.Second)
+ defer cancel()
- utils.CloseAll()
- return nil
+ if err := server.Shutdown(ctx); err != nil {
+ log.Errorf("Shutting down server error: %s", err)
+ }
+ }
}
func newStartCommand() *cobra.Command {
diff --git a/api/internal/core/store/store.go b/api/internal/core/store/store.go
index d529dd5..11f5082 100644
--- a/api/internal/core/store/store.go
+++ b/api/internal/core/store/store.go
@@ -21,6 +21,7 @@ import (
"context"
"encoding/json"
"fmt"
+ "os"
"reflect"
"sort"
"sync"
@@ -105,6 +106,7 @@ func (s *GenericStore) Init() error {
key := ret[i].Key[len(s.opt.BasePath)+1:]
objPtr, err := s.StringToObjPtr(ret[i].Value, key)
if err != nil {
+ fmt.Fprintln(os.Stderr, "Error occurred while initializing logical store: ", s.opt.BasePath)
return err
}
@@ -353,8 +355,8 @@ func (s *GenericStore) StringToObjPtr(str, key string) (interface{}, error) {
ret := objPtr.Interface()
err := json.Unmarshal([]byte(str), ret)
if err != nil {
- log.Errorf("json marshal failed: %s", err)
- return nil, fmt.Errorf("json unmarshal failed: %s", err)
+ log.Errorf("json unmarshal failed: %s", err)
+ return nil, fmt.Errorf("json unmarshal failed\n\tRelated Key:\t\t%s\n\tError Description:\t%s", key, err)
}
if setter, ok := ret.(entity.BaseInfoSetter); ok {
diff --git a/api/internal/core/store/store_test.go b/api/internal/core/store/store_test.go
index c557484..1cb8f8d 100644
--- a/api/internal/core/store/store_test.go
+++ b/api/internal/core/store/store_test.go
@@ -205,7 +205,8 @@ func TestGenericStore_Init(t *testing.T) {
Value: `{"Field1":"demo2-f1", "Field2":"demo2-f2"}`,
},
},
- wantErr: fmt.Errorf("json unmarshal failed: invalid character ',' after object key"),
+ wantErr: fmt.Errorf("json unmarshal failed\n\tRelated Key:\t\tdemo1-f1\n\tError Description:\t" +
+ "invalid character ',' after object key"),
wantListCalled: true,
},
}
diff --git a/api/internal/utils/pid.go b/api/internal/utils/pid.go
index 066a4c3..4080a53 100644
--- a/api/internal/utils/pid.go
+++ b/api/internal/utils/pid.go
@@ -26,6 +26,9 @@ import (
// WritePID write pid to the given file path.
func WritePID(filepath string) error {
+ if _, err := os.Stat(filepath); err == nil {
+ return fmt.Errorf("instance of Manager API already running: a pid file exists in %s", filepath)
+ }
pid := os.Getpid()
f, err := os.OpenFile(filepath, os.O_WRONLY|os.O_CREATE|os.O_TRUNC|os.O_CREATE, 0600)
if err != nil {
diff --git a/api/test/shell/cli_test.sh b/api/test/shell/cli_test.sh
index 876cd0e..0a1da75 100755
--- a/api/test/shell/cli_test.sh
+++ b/api/test/shell/cli_test.sh
@@ -520,6 +520,59 @@ if [[ `echo $(sudo ./manager-api remove) | grep -c "OK"` -ne "1" ]]; then
echo "error while removing the service"
exit 1
fi
+# test manager-api output for bad data on etcd
+# make a dummy entry
+./etcd-v3.4.14-linux-amd64/etcdctl put /apisix/routes/unique1 "{\"id\":}"
+sleep 2
+
+./manager-api 2>man-api.err &
+sleep 4
+
+if [[ `cat man-api.err | grep -c "Error occurred while initializing logical store: /apisix/routes"` -ne '1' ||
+`cat man-api.err | grep -c "Error: json unmarshal failed"` -ne '1' ]];then
+ echo "manager api failed to stream error on stderr for bad data"
+ exit 1
+fi
+# delete dummy entry
+./etcd-v3.4.14-linux-amd64/etcdctl del /apisix/routes/unique1
+# just to make sure
+./manager-api stop
+sleep 6
+clean_up
+
+# run manager api as os service
+# 2 times OK for installing and starting
+if [[ `echo $(sudo ./manager-api start) | grep -o "OK" | wc -l` -ne "2" ]]; then
+ echo "error while initializing the service"
+ exit 1
+fi
+# check running status
+if [[ `echo $(sudo ./manager-api status) | grep -c "running..."` -ne "1" ]]; then
+ echo "error while starting the service"
+ exit 1
+fi
+# stop the service
+sudo ./manager-api stop
+sleep 2
+# recheck running status
+if [[ `echo $(sudo ./manager-api status) | grep -c "Service is stopped"` -ne "1" ]]; then
+ echo "error while stopping the service"
+ exit 1
+fi
+# restart the service
+# 1 time OK for just for starting
+if [[ `echo $(sudo ./manager-api start) | grep -c "OK"` -ne "1" ]]; then
+ echo "error while restarting the service"
+ exit 1
+fi
+# stop the service
+sudo ./manager-api stop
+sleep 2
+# remove the service
+if [[ `echo $(sudo ./manager-api remove) | grep -c "OK"` -ne "1" ]]; then
+ echo "error while removing the service"
+ exit 1
+fi
pkill -f etcd