From f03d470349731b5119d7b49e620d7a9bb2cbff38 Mon Sep 17 00:00:00 2001 From: Jhon Honce Date: Tue, 29 Sep 2020 11:32:47 -0700 Subject: Refactor IdleTracker to handle StateIdle transitions * Remove stutter naming for package and types * Stop treating StateIdle the same as StateClosed, rather transitions to StateIdle will keep API timeout window open * Remove redundate code Fixes #7826 Signed-off-by: Jhon Honce --- pkg/api/handlers/compat/containers_attach.go | 6 +- pkg/api/handlers/compat/exec.go | 6 +- pkg/api/server/idle/tracker.go | 96 ++++++++++++++++++++++++++++ pkg/api/server/idletracker/idletracker.go | 74 --------------------- pkg/api/server/server.go | 22 +++---- 5 files changed, 113 insertions(+), 91 deletions(-) create mode 100644 pkg/api/server/idle/tracker.go delete mode 100644 pkg/api/server/idletracker/idletracker.go (limited to 'pkg/api') diff --git a/pkg/api/handlers/compat/containers_attach.go b/pkg/api/handlers/compat/containers_attach.go index e20d48d86..4a1196c89 100644 --- a/pkg/api/handlers/compat/containers_attach.go +++ b/pkg/api/handlers/compat/containers_attach.go @@ -6,7 +6,7 @@ import ( "github.com/containers/podman/v2/libpod" "github.com/containers/podman/v2/libpod/define" "github.com/containers/podman/v2/pkg/api/handlers/utils" - "github.com/containers/podman/v2/pkg/api/server/idletracker" + "github.com/containers/podman/v2/pkg/api/server/idle" "github.com/gorilla/schema" "github.com/pkg/errors" "github.com/sirupsen/logrus" @@ -92,7 +92,7 @@ func AttachContainer(w http.ResponseWriter, r *http.Request) { return } - idleTracker := r.Context().Value("idletracker").(*idletracker.IdleTracker) + idleTracker := r.Context().Value("idletracker").(*idle.Tracker) hijackChan := make(chan bool, 1) // Perform HTTP attach. @@ -109,7 +109,7 @@ func AttachContainer(w http.ResponseWriter, r *http.Request) { // We do need to tell the idle tracker that the // connection has been closed, though. We can guarantee // that is true after HTTPAttach exits. - idleTracker.TrackHijackedClosed() + idleTracker.Close() } else { // A hijack was not successfully completed. We need to // report the error normally. diff --git a/pkg/api/handlers/compat/exec.go b/pkg/api/handlers/compat/exec.go index 1db950f85..df51293c2 100644 --- a/pkg/api/handlers/compat/exec.go +++ b/pkg/api/handlers/compat/exec.go @@ -10,7 +10,7 @@ import ( "github.com/containers/podman/v2/libpod/define" "github.com/containers/podman/v2/pkg/api/handlers" "github.com/containers/podman/v2/pkg/api/handlers/utils" - "github.com/containers/podman/v2/pkg/api/server/idletracker" + "github.com/containers/podman/v2/pkg/api/server/idle" "github.com/containers/podman/v2/pkg/specgen/generate" "github.com/gorilla/mux" "github.com/pkg/errors" @@ -174,7 +174,7 @@ func ExecStartHandler(w http.ResponseWriter, r *http.Request) { return } - idleTracker := r.Context().Value("idletracker").(*idletracker.IdleTracker) + idleTracker := r.Context().Value("idletracker").(*idle.Tracker) hijackChan := make(chan bool, 1) if err := sessionCtr.ExecHTTPStartAndAttach(sessionID, r, w, nil, nil, nil, hijackChan); err != nil { @@ -186,7 +186,7 @@ func ExecStartHandler(w http.ResponseWriter, r *http.Request) { // We do need to tell the idle tracker that the // connection has been closed, though. We can guarantee // that is true after HTTPAttach exits. - idleTracker.TrackHijackedClosed() + idleTracker.Close() } else { // A hijack was not successfully completed. We need to // report the error normally. diff --git a/pkg/api/server/idle/tracker.go b/pkg/api/server/idle/tracker.go new file mode 100644 index 000000000..1b378c492 --- /dev/null +++ b/pkg/api/server/idle/tracker.go @@ -0,0 +1,96 @@ +package idle + +import ( + "net" + "net/http" + "sync" + "time" + + "github.com/sirupsen/logrus" +) + +// Tracker holds the state for the server's idle tracking +type Tracker struct { + // Duration is the API idle window + Duration time.Duration + hijacked int // count of active connections managed by handlers + managed map[net.Conn]struct{} // set of active connections managed by http package + mux sync.Mutex // protect managed map + timer *time.Timer + total int // total number of connections made to this server instance +} + +// NewTracker creates and initializes a new Tracker object +// For best behavior, duration should be 2x http idle connection timeout +func NewTracker(idle time.Duration) *Tracker { + return &Tracker{ + managed: make(map[net.Conn]struct{}), + Duration: idle, + timer: time.NewTimer(idle), + } +} + +// ConnState is called on HTTP connection state changes. +// - Once StateHijacked, StateClose is _NOT_ called on that connection +// - There are two "idle" timeouts, the http idle connection (not to be confused with the TCP/IP idle socket timeout) +// and the API idle window. The caller should set the http idle timeout to 2x the time provided to NewTacker() which +// is the API idle window. +func (t *Tracker) ConnState(conn net.Conn, state http.ConnState) { + t.mux.Lock() + defer t.mux.Unlock() + + logrus.Debugf("IdleTracker %p:%v %dm+%dh/%dt connection(s)", conn, state, len(t.managed), t.hijacked, t.TotalConnections()) + switch state { + case http.StateNew, http.StateActive: + // stop the API timer when the server transitions any connection to an "active" state + t.managed[conn] = struct{}{} + t.timer.Stop() + t.total++ + case http.StateHijacked: + // hijacked connections should call Close() when finished. + // Note: If a handler hijack's a connection and then doesn't Close() it, + // the API timer will not fire and the server will _NOT_ timeout. + delete(t.managed, conn) + t.hijacked++ + case http.StateIdle: + // When any connection goes into the http idle state, we know: + // - we have an active connection + // - the API timer should not be counting down (See case StateNew/StateActive) + break + case http.StateClosed: + oldActive := t.ActiveConnections() + + // Either the server or a hijacking handler has closed the http connection to a client + if _, found := t.managed[conn]; found { + delete(t.managed, conn) + } else { + t.hijacked-- // guarded by t.mux above + } + + // Transitioned from any "active" connection to no connections + if oldActive > 0 && t.ActiveConnections() == 0 { + t.timer.Stop() // See library source for Reset() issues and why they are not fixed + t.timer.Reset(t.Duration) // Restart the API window timer + } + } +} + +// Close is used to update Tracker that a StateHijacked connection has been closed by handler (StateClosed) +func (t *Tracker) Close() { + t.ConnState(nil, http.StateClosed) +} + +// ActiveConnections returns the number of current managed or StateHijacked connections +func (t *Tracker) ActiveConnections() int { + return len(t.managed) + t.hijacked +} + +// TotalConnections returns total number of connections made to this instance of the service +func (t *Tracker) TotalConnections() int { + return t.total +} + +// Done is called when idle timer has expired +func (t *Tracker) Done() <-chan time.Time { + return t.timer.C +} diff --git a/pkg/api/server/idletracker/idletracker.go b/pkg/api/server/idletracker/idletracker.go deleted file mode 100644 index 1ee905a99..000000000 --- a/pkg/api/server/idletracker/idletracker.go +++ /dev/null @@ -1,74 +0,0 @@ -package idletracker - -import ( - "net" - "net/http" - "sync" - "time" - - "github.com/sirupsen/logrus" -) - -type IdleTracker struct { - http map[net.Conn]struct{} - hijacked int - total int - mux sync.Mutex - timer *time.Timer - Duration time.Duration -} - -func NewIdleTracker(idle time.Duration) *IdleTracker { - return &IdleTracker{ - http: make(map[net.Conn]struct{}), - Duration: idle, - timer: time.NewTimer(idle), - } -} - -func (t *IdleTracker) ConnState(conn net.Conn, state http.ConnState) { - t.mux.Lock() - defer t.mux.Unlock() - - oldActive := t.ActiveConnections() - logrus.Debugf("IdleTracker %p:%v %d/%d connection(s)", conn, state, oldActive, t.TotalConnections()) - switch state { - case http.StateNew, http.StateActive: - t.http[conn] = struct{}{} - // stop the timer if we transitioned from idle - if oldActive == 0 { - t.timer.Stop() - } - t.total++ - case http.StateHijacked: - // hijacked connections are handled elsewhere - delete(t.http, conn) - t.hijacked++ - case http.StateIdle, http.StateClosed: - delete(t.http, conn) - // Restart the timer if we've become idle - if oldActive > 0 && len(t.http) == 0 { - t.timer.Stop() - t.timer.Reset(t.Duration) - } - } -} - -func (t *IdleTracker) TrackHijackedClosed() { - t.mux.Lock() - defer t.mux.Unlock() - - t.hijacked-- -} - -func (t *IdleTracker) ActiveConnections() int { - return len(t.http) + t.hijacked -} - -func (t *IdleTracker) TotalConnections() int { - return t.total -} - -func (t *IdleTracker) Done() <-chan time.Time { - return t.timer.C -} diff --git a/pkg/api/server/server.go b/pkg/api/server/server.go index e7c031234..09a9f6370 100644 --- a/pkg/api/server/server.go +++ b/pkg/api/server/server.go @@ -16,7 +16,7 @@ import ( "github.com/containers/podman/v2/libpod" "github.com/containers/podman/v2/pkg/api/handlers" - "github.com/containers/podman/v2/pkg/api/server/idletracker" + "github.com/containers/podman/v2/pkg/api/server/idle" "github.com/coreos/go-systemd/v22/activation" "github.com/coreos/go-systemd/v22/daemon" "github.com/gorilla/mux" @@ -26,14 +26,14 @@ import ( ) type APIServer struct { - http.Server // The HTTP work happens here - *schema.Decoder // Decoder for Query parameters to structs - context.Context // Context to carry objects to handlers - *libpod.Runtime // Where the real work happens - net.Listener // mux for routing HTTP API calls to libpod routines - context.CancelFunc // Stop APIServer - idleTracker *idletracker.IdleTracker // Track connections to support idle shutdown - pprof *http.Server // Sidecar http server for providing performance data + http.Server // The HTTP work happens here + *schema.Decoder // Decoder for Query parameters to structs + context.Context // Context to carry objects to handlers + *libpod.Runtime // Where the real work happens + net.Listener // mux for routing HTTP API calls to libpod routines + context.CancelFunc // Stop APIServer + idleTracker *idle.Tracker // Track connections to support idle shutdown + pprof *http.Server // Sidecar http server for providing performance data } // Number of seconds to wait for next request, if exceeded shutdown server @@ -70,13 +70,13 @@ func newServer(runtime *libpod.Runtime, duration time.Duration, listener *net.Li } router := mux.NewRouter().UseEncodedPath() - idle := idletracker.NewIdleTracker(duration) + idle := idle.NewTracker(duration) server := APIServer{ Server: http.Server{ Handler: router, ReadHeaderTimeout: 20 * time.Second, - IdleTimeout: duration, + IdleTimeout: duration * 2, ConnState: idle.ConnState, ErrorLog: log.New(logrus.StandardLogger().Out, "", 0), }, -- cgit v1.2.3-54-g00ecf