Handle containerd event reliably #628

yanxuean · 2018-02-28T13:45:02Z

fix #434
Signed-off-by: yanxuean yan.xuean@zte.com.cn

yanxuean · 2018-02-28T13:45:33Z

/hold

yanxuean · 2018-02-28T14:40:29Z

/cc @Random-Liu PTAL
I will add test case later.

k8s-ci-robot · 2018-02-28T14:40:30Z

@yanxuean: GitHub didn't allow me to request PR reviews from the following users: PTAL.

Note that only containerd members and repo collaborators can review this PR, and authors cannot review their own PRs.

In response to this:

/cc @Random-Liu PTAL
I will add test case later.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

Random-Liu · 2018-03-05T22:06:43Z

Ref #642.

Random-Liu · 2018-03-05T18:51:39Z

pkg/server/events.go

@@ -43,6 +45,14 @@ type eventMonitor struct {
 	errCh          <-chan error
 	ctx            context.Context
 	cancel         context.CancelFunc
+	backOffQueue   backOffQueue


Can we put all backoff related thing into one struct? It will also be easier to unit test.

Random-Liu · 2018-03-06T01:25:44Z

pkg/server/events.go

+					logrus.WithError(err).Errorf("Failed to convert event envelope %+v", e)
+					break
+				}
+				if cID, backOffIng := em.isBackOffIng(any); backOffIng {


s/isBackOffIng/isInBackoff

Random-Liu · 2018-03-06T01:38:23Z

pkg/server/events.go

 	if e.Pid != sb.Status.Get().Pid {
 		// Non-init process died, ignore the event.
-		return
+		return nil
 	}
 	// No stream attached to sandbox container.
 	task, err := sb.Container.Task(context.Background(), nil)
 	if err != nil {
 		if !errdefs.IsNotFound(err) {
 			logrus.WithError(err).Errorf("failed to load task for sandbox %q", e.ContainerID)


log in handleEvent instead, since you've returned the error anyway.

return fmt.Errorf("failed to load task: %v", err)

And same for the following errors and also handleContainerExit.

Random-Liu · 2018-03-06T01:53:25Z

pkg/server/events.go

+	em.backOffQueue[key] = queue
+}
+
+func (em *eventMonitor) newBackOffTimer(key string) *time.Timer {


There is no backoff? The interval should increase if it keeps failing, right?

Random-Liu · 2018-03-06T01:58:20Z

pkg/server/events.go

+					em.enBackOff(cID, any)
+					break
+				}
+				if cID, err := em.handleEvent(any); err != nil && cID != "" {


Why err != nil && cID != ""?

Is it possible that err != nil but cID == ""? Is that an error?

Actually, you've got the container id here, why still let handleEvent return one?

To prevent accidents:)
will do

Random-Liu · 2018-03-06T02:01:20Z

pkg/server/events.go

 			case err := <-em.errCh:
 				logrus.WithError(err).Error("Failed to handle event stream")
 				close(closeCh)
 				return
+			case cID := <-em.backOffExpire:
+				for {
+					any := em.deBackOff(cID)


When will any == nil? Is that an error?

No, It is not error. It is in "for" loop. When we handle over the whole queue, it will be nil.

Random-Liu · 2018-03-06T02:06:12Z

pkg/server/events.go

+					if any == nil {
+						break
+					}
+					if _, err := em.handleEvent(any); err != nil && cID != "" {


cID != ""? You don't even set it based on the return value.

Random-Liu · 2018-03-06T02:11:06Z

pkg/server/events.go

+// enBackOff start to backOff again and put event to the begin of queue
+func (em *eventMonitor) reBackOff(key string, evt interface{}) {
+	newEvents := []interface{}{evt}
+	queue := em.backOffQueue[key]


Just requeue, and always stop timer as long as it is not nil.

No need to check length.

If then, We will stop timer twice when there is only one event.

Random-Liu · 2018-03-06T02:22:18Z

@yanxuean I feel like the per container timer logic is unnecessarily complex. And we haven't done real back yet. (See #628 (comment))

I feel like it is very easy to forget starting a timer, or leave an unnecessary timer over.

How about we just have a ticker which ticks every 1 second or so? And every time it ticks, we check whether there is any events which have exceed backoff time. And deal with them if there are any. This seems more reliable to me.

To optimize, we can only start the ticker when there is one event enqueued.
@yanxuean WDYT?

yanxuean · 2018-03-06T03:24:26Z

Will refactor it.

yanxuean · 2018-03-10T06:44:24Z

@Random-Liu PTAL, Tks

Random-Liu · 2018-03-14T07:52:59Z

pkg/server/events.go

@@ -74,16 +95,40 @@ func (em *eventMonitor) start() (<-chan struct{}, error) {
 		return nil, errors.New("event channel is nil")
 	}
 	closeCh := make(chan struct{})
+	em.backOff.ticker = time.NewTicker(1 * time.Second)


nit: Add a start function in backoff?

Random-Liu · 2018-03-14T07:55:28Z

pkg/server/events.go

@@ -54,6 +72,9 @@ func newEventMonitor(c *containerstore.Store, s *sandboxstore.Store) *eventMonit
 		sandboxStore:   s,
 		ctx:            ctx,
 		cancel:         cancel,
+		backOff: backOff{


nit: Add a new function?

Random-Liu · 2018-03-14T07:55:58Z

pkg/server/events.go

+					logrus.WithError(err).Errorf("Failed to convert event envelope %+v", e)
+					break
+				}
+				cID, backOffIng := em.backOff.isInBackOff(any)


s/backOffIng/inBackoff

Random-Liu · 2018-03-14T07:56:54Z

pkg/server/events.go

+					break
+				}
+				if err := em.handleEvent(any); err != nil {
+					em.backOff.enBackOff(cID, any)


log the error

Random-Liu · 2018-03-14T07:58:03Z

pkg/server/events.go

 			case err := <-em.errCh:
 				logrus.WithError(err).Error("Failed to handle event stream")
 				close(closeCh)
 				return
+			case <-em.backOff.ticker.C:


nit: Add a simple function to return the channel?

Random-Liu · 2018-03-14T08:02:19Z

pkg/server/events.go

-		logrus.WithError(err).Errorf("Failed to convert event envelope %+v", evt)
-		return
-	}
+func (em *eventMonitor) handleEvent(any interface{}) error {


Let's return error with description from this function, and we only need to log the error in start.

In this way, we don't need so many logs in this function.

Random-Liu · 2018-03-14T08:21:45Z

pkg/server/events.go

+}
+
+// enBackOff start to backOff and put event to the tail of queue
+func (b *backOff) enBackOff(key string, evt interface{}) {


queue, ok := b.queues[key] if ok { queue.events = append(queue.events, evt) return } b.queues[key] = backOffQueue{ events: []interface{}{evt}, duration: initialBackoffTime, start: time.Now(), } // Or add a function `newBackoffQueue`

Random-Liu · 2018-03-14T08:25:27Z

pkg/server/events.go

+}
+
+// enBackOff start to backOff again and put [nth:] events to the queue
+func (b *backOff) reBackOff(key string, queue backOffQueue, n int) {


func (b *backOff) reBackOff(key string, events []interface{}, oldDuration time.Duration) { duration := 2 * oldDuration if duration > maxBackoffTime { duration = maxBackoffTime } b.queues[key] = backOffQueue{ events: events, duration: duration, start: time.Now(), } }

Random-Liu · 2018-03-14T08:27:02Z

pkg/server/events.go

+	return !now.Before(t.expireTime)
+}
+
+func (t backOffTime) newTime() backOffTime {


I think this function is unnecessary complex... Simple and straightforward code is preferred. See comments above.

Random-Liu · 2018-03-14T08:27:36Z

pkg/server/events.go

+	var containers []string
+	now := time.Now()
+	for c, v := range b.queues {
+		if v.time.isExpire(now) {


if time.Since(v.start) > v.duration // or add a `isExpired` function for backOffQueue

Random-Liu · 2018-03-14T08:30:51Z

pkg/server/events_test.go

+		},
+	}
+
+	t.Logf("Should can backOff a event")


s/can/be able to

And also for following ones.

Random-Liu · 2018-03-14T08:31:52Z

pkg/server/events_test.go

+	}
+
+	t.Logf("Should can backOff a event")
+	actual := newEventMonitor(nil, nil).backOff


? I prefer using the new function suggested in https://github.com/containerd/cri/pull/628/files#r174375426.

Random-Liu · 2018-03-14T08:32:40Z

pkg/server/events_test.go

+	}
+	assert.Equal(t, isQueueListEqual(t, actual.queues, expectedQueues, 1*time.Second), true)
+
+	t.Logf("Should can check if the container is on backOff state")


Random-Liu · 2018-03-14T08:35:02Z

pkg/server/events_test.go

@@ -0,0 +1,150 @@
+/*


Use a fake clock for the unit test.
https://github.com/containerd/cri/tree/master/vendor/k8s.io/apimachinery/pkg/util/clock

Random-Liu · 2018-03-14T08:38:18Z

pkg/server/events_test.go

@@ -0,0 +1,150 @@
+/*
+Copyright 2018 The Kubernetes Authors.


containerd Authors

Random-Liu · 2018-03-14T08:39:24Z

pkg/server/events_test.go

+	assert.Equal(t, isQueueListEqual(t, actual.queues, expectedQueues, 1*time.Second), true)
+
+	t.Logf("Should can check if the container is on backOff state")
+	for k, queue := range inputQueues {


Should check an arbitrary container id is not in backoff.

Random-Liu · 2018-03-14T08:39:54Z

pkg/server/events_test.go

+		}
+	}
+
+	t.Logf("Should can get all keys who are expired for backOff")


s/who/which

Random-Liu · 2018-03-14T08:42:01Z

pkg/server/events_test.go

+	actKeyList := actual.getExpiredContainers()
+	assert.Equal(t, len(expKeyList), len(actKeyList))
+	for _, expKey := range expKeyList {
+		found := false


assert.Contains

Random-Liu · 2018-03-14T08:43:06Z

pkg/server/events_test.go

+		assert.Equal(t, isQueueEqual(t, actQueue, expectedQueues[k], 1*time.Second), true)
+	}
+
+	t.Logf("Should not get out the event again after having gut out the backOff event")


Random-Liu · 2018-03-14T08:44:42Z

pkg/server/events_test.go

+		expQueue := backOffQueue{
+			events: queue.events[failEventIndex:],
+		}
+		assert.Equal(t, isQueueEqual(t, actQueue, expQueue, 2*time.Second), true)


Use constant for code and test.

Signed-off-by: yanxuean <yan.xuean@zte.com.cn>

fix containerd#434 Signed-off-by: yanxuean <yan.xuean@zte.com.cn>

Random-Liu

LGTM with nits. I can take care of the nits in another PR.

Random-Liu · 2018-03-15T18:10:25Z

pkg/server/events.go

@@ -55,6 +82,8 @@ func newEventMonitor(c *containerstore.Store, s *sandboxstore.Store) *eventMonit
 		sandboxStore:   s,
 		ctx:            ctx,
 		cancel:         cancel,
+		backOff: newBackOff(backOffInitDuration, backOffMaxDuration,


I think we can just use the const instead of parameterize it.

Random-Liu · 2018-03-15T18:13:10Z

pkg/server/events.go

+					logrus.WithError(err).Errorf("Failed to convert event %+v", e)
+					break
+				}
+				if em.backOff.isInBackOff(cID) {


Add an info log here.

Random-Liu · 2018-03-15T18:29:30Z

pkg/server/events_test.go

+	t.Logf("Should be able to check that a container isn't in backOff state")
+	notExistKey := "containerNotExist"
+	assert.Equal(t, actual.isInBackOff(notExistKey), false)
+


Should add a test case that getExpiredContainers should be empty when not expired.

Random-Liu · 2018-03-15T18:30:20Z

pkg/server/events_test.go

+	for _, k := range actKeyList {
+		actKeyMap[k] = struct{}{}
+	}
+	assert.Equal(t, actKeyMap, expKeyMap)


We can compare length, and use assert.Contains.

Random-Liu · 2018-03-15T18:35:38Z

@yanxuean Thanks a lot! Good job! :D

k8s-ci-robot added the size/L label Feb 28, 2018

yanxuean force-pushed the nits branch 2 times, most recently from c7c71db to 76cbdcb Compare February 28, 2018 14:25

yanxuean changed the title ~~Handle containerd event reliably~~ [WIP] Handle containerd event reliably Feb 28, 2018

k8s-ci-robot requested a review from Random-Liu February 28, 2018 14:40

yanxuean force-pushed the nits branch from 76cbdcb to eb36440 Compare February 28, 2018 14:41

Random-Liu added this to the v1.0.0-rc.0 milestone Feb 28, 2018

Random-Liu self-assigned this Mar 1, 2018

Random-Liu mentioned this pull request Mar 5, 2018

Fix a potential panic. #643

Merged

Random-Liu reviewed Mar 6, 2018

View reviewed changes

yanxuean force-pushed the nits branch 2 times, most recently from cb75ccc to 0d93897 Compare March 10, 2018 05:29

yanxuean changed the title ~~[WIP] Handle containerd event reliably~~ Handle containerd event reliably Mar 10, 2018

yanxuean force-pushed the nits branch 3 times, most recently from 624dad3 to 5e2e5cb Compare March 10, 2018 06:43

Random-Liu reviewed Mar 14, 2018

View reviewed changes

yanxuean force-pushed the nits branch from 5e2e5cb to 395137f Compare March 15, 2018 07:50

some comments

7583bce

Signed-off-by: yanxuean <yan.xuean@zte.com.cn>

yanxuean force-pushed the nits branch from 395137f to 5741b4d Compare March 15, 2018 08:09

Random-Liu mentioned this pull request Mar 15, 2018

CRI-Containerd Missing Pieces #62

Closed

42 tasks

yanxuean force-pushed the nits branch from 5741b4d to d6c2f2e Compare March 15, 2018 08:43

yanxuean force-pushed the nits branch from d6c2f2e to e21ef39 Compare March 15, 2018 09:04

Handle containerd event reliably

c751847

fix containerd#434 Signed-off-by: yanxuean <yan.xuean@zte.com.cn>

yanxuean force-pushed the nits branch from e21ef39 to c751847 Compare March 15, 2018 09:14

Random-Liu reviewed Mar 15, 2018

View reviewed changes

Random-Liu merged commit eff311d into containerd:master Mar 15, 2018

Random-Liu mentioned this pull request Mar 15, 2018

Cleanup event backoff. #669

Merged

yanxuean deleted the nits branch March 23, 2018 02:16

Random-Liu mentioned this pull request Mar 26, 2018

Update cri to v1.0.0-rc.0 containerd/containerd#2233

Merged

Handle containerd event reliably #628

Handle containerd event reliably #628

Conversation

yanxuean commented Feb 28, 2018

yanxuean commented Feb 28, 2018

yanxuean commented Feb 28, 2018

k8s-ci-robot commented Feb 28, 2018

Random-Liu commented Mar 5, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Random-Liu commented Mar 6, 2018 • edited

yanxuean commented Mar 6, 2018

yanxuean commented Mar 10, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Random-Liu Mar 14, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Random-Liu left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Random-Liu commented Mar 15, 2018

Random-Liu commented Mar 5, 2018 •

edited

Random-Liu commented Mar 6, 2018 •

edited

Random-Liu Mar 14, 2018 •

edited