cri: handle sandbox/container exit event in parallel#4682
cri: handle sandbox/container exit event in parallel#4682dmcgowan merged 1 commit intocontainerd:masterfrom
Conversation
|
Build succeeded.
|
fuweid
force-pushed
the
cri-handle-exit-event-separate
branch
from
eb13925 to
939b159
Compare
|
Build succeeded.
|
|
ping @AkihiroSuda and @mikebrow |
|
I don't think I understand this change.
|
|
The current design is that all the task exit events are put into one queue. The event consumer will consume the event one by one, serially and there is only one consumer. The consumer is required to handle one task in 10 seconds. If timeout, consumer will put it back into backoff queue and retry it later. If the node has to evict many pods for some reason, like disk pressure, the backlog of handling task exit event will increases. If each one exit event needs 10 seconds, the deletion of pods will take long time. For this case, I would like to propose to handle task exit event in parallel. When the container/sandbox monitors receive the task exit event, the monitor should handle it first. If it failed, it can put the event into backoff queue and event consumer can try it later. Sorry for unclear message. So, does it make sense to you? |
fuweid
changed the title
fuweid
changed the title
mikebrow
left a comment
mikebrow
left a comment
There was a problem hiding this comment.
I like this alot. Are there any paths left for handleEvent() that could still receive a TaskExit? If not we can probably remove that case..
There was a problem hiding this comment.
the error output here always bothered me :-) if err != store.ErrNotExist wrap the error with can't find? probably better to just say unexpected error retrieving sandbox for TaskExit event
fuweid
force-pushed
the
cri-handle-exit-event-separate
branch
from
939b159 to
9760f88
Compare
Hmm... |
|
The backoff is map and need mutex to lock it. Updated it later |
fuweid
force-pushed
the
cri-handle-exit-event-separate
branch
2 times, most recently
from
c6e6957 to
4de9c3d
Compare
hmm.... |
fuweid
force-pushed
the
cri-handle-exit-event-separate
branch
from
4de9c3d to
e024dde
Compare
|
ping @mikebrow the patch is ready to review. PTAL. Thanks! |
|
ping @AkihiroSuda and @cpuguy83 ~ |
The event monitor handles exit events one by one. If there is something wrong about deleting task, it will slow down the terminating Pods. In order to reduce the impact, the exit event watcher should handle exit event separately. If it failed, the watcher should put it into backoff queue and retry it. Signed-off-by: Wei Fu <[email protected]>
fuweid
force-pushed
the
cri-handle-exit-event-separate
branch
from
e024dde to
e56de63
Compare
|
updated the error message for go error message convention. |
6 participants
The current design is that all the task exit events are put into one queue. The event consumer will consume the event one by one, serially and there is only one consumer. The consumer is required to handle one task in 10 seconds. If timeout, consumer will put it back into backoff queue and retry it later.
If the node has to evict many pods for some reason, like disk pressure, the backlog of handling task exit event will increases. If each one exit event needs 10 seconds, the deletion of pods will take long time.
For this case, I would like to propose to handle task exit event in parallel. When the container/sandbox monitors receive the task exit event, the monitor should handle it first. If it failed, it can put the event into backoff queue and event consumer can try it later.
Signed-off-by: Wei Fu [email protected]