-
Notifications
You must be signed in to change notification settings - Fork 128
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
Mozh/t500 graceful restart #123
Conversation
…h/T500_graceful_restart
…h/T500_graceful_restart
cmd/acra_configui/acra_configui.go
Outdated
var outConfigParams configParamsYAML | ||
configParamsYAML, err := ioutil.ReadFile("acraserver_config_vars.yaml") | ||
check(err) | ||
tplPath := fmt.Sprintf("./cmd/acra_configui/%s", filepath.Join("static", "index.html")) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what about to add fetching path for static files from cli args or config? Acra may be used as binary and will run in different struct tree. And will be good to provide ability to override path to static files
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
cmd/acra_configui/acra_configui.go
Outdated
parsedTemplate, err = template.ParseFiles(tplPath) | ||
if err != nil { | ||
log.WithError(err).Errorf("err while parsing template - %v", tplPath) | ||
syscall.Exit(1) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Will be better if we will check template files on app start before running service. after app start check and go down if templates not exist. it will be better than go down at runtime
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
cmd/acra_configui/acra_configui.go
Outdated
@@ -175,9 +202,9 @@ func main() { | |||
|
|||
http.HandleFunc("/index.html", index) | |||
http.HandleFunc("/", index) | |||
http.Handle("/static/", http.StripPrefix("/static/", http.FileServer(http.Dir("./static")))) | |||
http.Handle("/static/", http.StripPrefix("/static/", http.FileServer(http.Dir("./cmd/acra_configui/static")))) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
let's use path to static from cli args/config?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Implemented cli-param
cmd/acraserver/acraserver.go
Outdated
) | ||
|
||
// DEFAULT_CONFIG_PATH relative path to config which will be parsed as default | ||
var DEFAULT_CONFIG_PATH = utils.GetConfigPathByName("acraserver") | ||
var SignalsChannel = make(chan os.Signal) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
https://github.com/cossacklabs/acra/blob/master/cmd/utils.go#L37
We has signal handler that used in acraproxy/acraserver (added before your PR:). Better to use one implementation instead many in different places. Better to extend existing in this case imho.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Refactored
cmd/acraserver/acraserver.go
Outdated
server, err := NewServer(config, keyStore) | ||
var server *SServer | ||
if os.Getenv("_GRACEFUL_RESTART") == "true" { | ||
server, err = NewFromFD(config, keyStore, 3, 4) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what means 3, 4
? Can we use here some constants?
plus why not extend NewServer and add new function?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Constants used
cmd/acraserver/acraserver.go
Outdated
err := server.WaitWithTimeout(10 * time.Second) | ||
if err == WaitTimeoutError { | ||
log.Debugf("Timeout when stopping server, %d active connections will be cut.\n", server.ConnectionsCounter()) | ||
os.Exit(-127) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why -127
? What about to add some error message to output? For the case when app will work without debug output
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Refactored
cmd/acraserver/listener.go
Outdated
socket.SetDeadline(time.Now()) | ||
} | ||
|
||
func (server *SServer) ListenerFD(socket *net.TCPListener) (uintptr, error) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What about to ListenerFileDescriptor
? uintptr
and FD
not so obvious imho
Plus this method doesn't use any fields from SServer
struct and better to implement it as function. Than we can use it in other places where will need to get descriptor from TCPListener
Plus we support not only TCPListener
, UnixListener
too (and connections). So what about to write something like that - https://play.golang.org/p/LuUqCB2YqyP ? Wrap it into functions GetDescriptorFromListener[Connection]
and use where we need
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Refactored
cmd/acraserver/listener.go
Outdated
} | ||
} | ||
|
||
func (server *SServer) Addr(socket *net.TCPListener) net.Addr { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the same, we need support unix too
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Refactored
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
do we use or need this function? where it used?
cmd/acraserver/listener.go
Outdated
var err error | ||
var connection net.Conn | ||
if graceful == true { | ||
listenerGraceful, err = network.ListenTCP(server.config.GetAcraAPIConnectionString()) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can use this function - https://github.com/cossacklabs/acra/blob/master/network/utils.go#L22 to support unix/tcp
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Instead of having if
checks everywhere, can we create two separate functions, one for unix sockets, another for TCP, and use if
once:
func (server *SServer) StartCommands(graceful bool) {
...
if graceful == true {
handleGracefulSocket()
} else {
handleSocket()
}
}
func handleGracefulSocket()
func handleSocket()
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Refactored
network/utils.go
Outdated
@@ -31,6 +34,24 @@ func Listen(connectionString string) (net.Listener, error) { | |||
} | |||
} | |||
|
|||
func ListenTCP(connectionString string) (*net.TCPListener, error) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
https://github.com/cossacklabs/acra/blob/master/network/utils.go#L22 can we use it or extend/replace something of these 2?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
cmd/acraserver/acraserver.go
Outdated
var WaitTimeoutError = errors.New("timeout") | ||
for sig := range SignalsChannel { | ||
if sig == syscall.SIGTERM { | ||
// Stop accepting new connections |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe that code would be much readable if we divide this laaarge if
handlers into separate functions.
like
func handleSigTerm(...)
func handleSigHup(...)
smaller, more readable
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Used our handler wrapper
cmd/acraserver/acraserver.go
Outdated
server.Stop(server.socketAPI) | ||
} | ||
// Wait a maximum of 10 seconds for existing connections to finish | ||
err := server.WaitWithTimeout(10 * time.Second) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
shall we define 10 as constant and move it to the top of file? for changing easily if needed?
like
var defaultConnectionTimeout int = 10
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Moved to constants
cmd/acraserver/listener.go
Outdated
continue | ||
} | ||
// unix socket and value == '@' | ||
if len(connection.RemoteAddr().String()) == 1 { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is it a good idea to check unix socket value like string length?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
for unix socket RemoteAddr always == "@", LocalAddr has path. For TCP socket and RemoteAddr and LocalAddr has some path like "127.0.0.1:2345" and "127.0.0.1:5431". How you suggest to checK?
cmd/acra_configui/acra_configui.go
Outdated
} | ||
|
||
func parseConfig() []byte { | ||
configPath, err_ := utils.AbsPath(fmt.Sprintf("./%s", CONFIG_PATH)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- you can read file with relative path without converting to absolute.
CONFIG_PATH
constains relative pathconfig/blabla
and it can be read without transformations - what if user want to use config with custom path like
/var/acra/some.conf
? or how we can change path if we will distribute our package with binary and place config in different place? for example binary will be placed to /usr/local/bin and configs to/etc/acra/*
or/usr/local/etc
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As discussed - will move configs/acraserver_config_vars.yaml to cmd/acra_configui/acraserver_config_vars.go as it's internal thing.
TODO: make run-time config
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
cmd/acra_configui/acra_configui.go
Outdated
@@ -105,12 +116,37 @@ func SubmitSettings(w http.ResponseWriter, r *http.Request) { | |||
w.Write(jsonToServer) | |||
} | |||
|
|||
func parseTemplate() { | |||
tplPath := fmt.Sprintf(filepath.Join(*staticPath, "index.html")) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what about to use function parameter staticPath
(func parseTemplate(staticPath string)
) instead global variable?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
cmd/acra_configui/acra_configui.go
Outdated
tplPath := fmt.Sprintf(filepath.Join(*staticPath, "index.html")) | ||
tplPath, err = utils.AbsPath(tplPath) | ||
if err != nil { | ||
log.WithError(err).Errorf("no config file[%v]", tplPath) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
return? syscall.exit()?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
cmd/acra_configui/acra_configui.go
Outdated
parsedTemplate, err = template.ParseFiles(tplPath) | ||
if err != nil { | ||
log.WithError(err).Errorf("err while parsing template - %v", tplPath) | ||
syscall.Exit(1) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what about to return error object and handle it on up level? then we can easily test it if need. otherwise it is non-testable function
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
cmd/acra_configui/acra_configui.go
Outdated
http.HandleFunc("/acraserver/submit_setting", SubmitSettings) | ||
log.Info(fmt.Sprintf("AcraConfigUI is listening @ :%d with PID %d", *port, os.Getpid())) | ||
err := http.ListenAndServe(fmt.Sprintf(":%d", *port), nil) | ||
log.Info(fmt.Sprintf("AcraConfigUI is listening @ %s:%d with PID %d", *host, *port, os.Getpid())) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
you can use just log.Infof
with placeholders without fmt.Sprintf
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
cmd/acraserver/acraserver.go
Outdated
|
||
// Wait a maximum of N seconds for existing connections to finish | ||
err = server.WaitWithTimeout(ACRASERVER_WAIT_TIMEOUT * time.Second) | ||
if err == errors.New("timeout") { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
better to compare with WaitTimeoutError
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
cmd/acraserver/listener.go
Outdated
} | ||
} | ||
|
||
var WaitTimeoutError = errors.New("timeout") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
go has convention to name all errors with prefix Err
at start (ErrWaitTimeout)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
cmd/acraserver/listener.go
Outdated
} | ||
} | ||
|
||
func (server *SServer) Addr(socket *net.TCPListener) net.Addr { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
do we use or need this function? where it used?
cmd/acraserver/listener.go
Outdated
if err != nil { | ||
log.WithError(err).Errorln("can't start listen command connections") | ||
log.WithError(err).Errorln("can't start listen command api connections") | ||
ErrorSignalChannel <- syscall.SIGTERM |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
same about global variable and channel
and to not repeat, will be good to check/apply fixes from Start
method in this method
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
cmd/constants.go
Outdated
@@ -9,4 +9,7 @@ const ( | |||
DEFAULT_ACRA_PORT = 9393 | |||
DEFAULT_ACRA_API_PORT = 9090 | |||
DEFAULT_ACRA_CONNECTION_PROTOCOL = "tcp" | |||
DEFAULT_ACRA_CONFIGUI_HOST = "127.0.0.1" | |||
DEFAULT_ACRA_CONFIGUI_PORT = 8000 | |||
DEFAULT_ACRA_CONFIGUI_STATIC = "./cmd/acra_configui/static" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
relative path without ./
correctly handled in golang functions that operate with files and paths
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
LGTM |
No description provided.