-
Notifications
You must be signed in to change notification settings - Fork 950
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
bugfix: event api connection may be recycled if keeps idle too long #2274
bugfix: event api connection may be recycled if keeps idle too long #2274
Conversation
Codecov Report
@@ Coverage Diff @@
## master #2274 +/- ##
==========================================
+ Coverage 66.7% 66.82% +0.12%
==========================================
Files 208 208
Lines 16934 16937 +3
==========================================
+ Hits 11295 11318 +23
+ Misses 4269 4262 -7
+ Partials 1370 1357 -13
|
apis/server/system_bridge.go
Outdated
// Notes(ziren): The pouchd http server set some http timeout parameters (see pouch/apis/server/server.go), | ||
// that may cause long idle connections been recycled. So, creating a tick here to keep the | ||
// event api alive. set the ticker to 8s because of the IdleTimeout of http server is 10s. | ||
tickChan := time.NewTicker(time.Second * 10).C |
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 think we should use time.NewTicker
and defer tick.Stop()
here, because we don't want to leave it active in the timer heap.
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 am afraid we may not need the defer
because i checked the example of NewTicker
when we should add defer tick.Stop()
when using NewTicker
, but not need the Stop
when using NewTicker().C
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.
OK, when i read the test cases in time
package, i realize i am wrong. we should call defer tick.Stop()
when creat a ticker.
apis/server/system_bridge.go
Outdated
@@ -11,6 +11,7 @@ import ( | |||
"github.com/alibaba/pouch/apis/types" | |||
"github.com/alibaba/pouch/pkg/httputils" | |||
"github.com/alibaba/pouch/pkg/utils" | |||
"github.com/sirupsen/logrus" |
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.
move it into next group
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.
good catch
b266303
to
9ebdce1
Compare
9ebdce1
to
f024952
Compare
@@ -384,15 +384,15 @@ func (c *Client) collectContainerdEvents() { | |||
} | |||
|
|||
switch e.Topic { | |||
case ContainersDeleteEventTopic: | |||
cDelEvent, ok := out.(*eventstypes.ContainerDelete) | |||
case TaskExitEventTopic: |
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.
Need know the exit code when a container exited, so we can get the task exit code from containerd task/exit
event
…ed if it keeps idle too long Signed-off-by: Michael Wan <zirenwan@gmail.com>
f024952
to
a41b6bd
Compare
events api connection broken because of the Refer: golang/go#24461 |
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.
LGTM
Signed-off-by: Michael Wan zirenwan@gmail.com
If pouchd start http server with parameters below, http server will recycle the long connection if it keeps idle too long.
So, i add a ticker in
event
api too keep the connection alive.Ⅰ. Describe what this PR did
Ⅱ. Does this pull request fix one issue?
none
Ⅲ. Why don't you add test cases (unit test/integration test)? (你真的觉得不需要加测试吗?)
none
Ⅳ. Describe how to verify it
none
Ⅴ. Special notes for reviews
none