Controllers do not requeue events on transient errors

[smarterclayton]

All of the controllers currently have a serious flaw, in that none of them retry / requeue events when a failure occurs processing the event. This means that certain operations will not complete until the controller is restarted.

Stories need to be created for beta2 for deployments and builds individually that correctly handle errors (probably by simply requeueing the event that triggered the error). This should be done in a way that does not overly complicate the controller loops (moves the logic for retry up a level). See the scheduler for ideas about how these events can be requeued. It's important to note that some controller loops will depend on order, so requeuing may result in operations delivered out of order, which means controller loops need to check that their action is valid (ie, if they already processed a newer event). It's also important that the controllers can make progress in the face of transient errors.

It would be good for one person to look at the problem as a whole and help ensure the logic is right. More unit and integration tests will need to be added to the controller, and a wider range of failure conditions detected. Better logging behavior should also be applied, so that it's easy to diagnose failures.

Given the nature of the current behavior, we may need to delay features from beta2 in favor of ensuing the controller loops are sound.

@bparees @ironcladlou

[smarterclayton]

https://github.com/GoogleCloudPlatform/kubernetes/blob/master/plugin/pkg/scheduler/factory/factory.go#L225 Is an example from the scheduler, where pods can be unordered. Deployments need to ignore duplicates (but are set up to do so). Builds will need a bit more work in the case of image changes

[bparees]

I think both build+deployment scenarios where it's due to a new image will need some careful logic.

I assume that if we're currently retrying an operation that was triggered by newImage:v2 and we get a new event for newimage:v3, we should abandon the retry logic that's operating on newImage:v2. If we don't, we end up with a race condition where the final outcome may be based on either newImage:v2 or newImage:v3.

But i'm not sure it's always going to be that clear which is the "newer" one. Nor am I sure we can really eliminate that race condition fully (ie once a build for newImage:v2 is running, it's basically out of our hands unless we cancel it)

In sum, I think there's a pretty deep rathole here of "what should the behavior be" that we need to discuss.

[smarterclayton]

Deployments aren't a problem because there is a resource version attached to the IR - if you process it at time T0, fail, put it in the queue, and then process a different T1, you can use resource version to ignore the earlier T0. Builds and images should be possible as well - it's possible that writing the last image used to the trigger is wrong, and we should be writing the last IR version we see.

On Feb 1, 2015, at 5:11 PM, Ben Parees notifications@github.com wrote:

I think both build+deployment scenarios where it's due to a new image will need some careful logic.

I assume that if we're currently retrying an operation that was triggered by newImage:v2 and we get a new event for newimage:v3, we should abandon the retry logic that's operating on newImage:v2. If we don't, we end up with a race condition where the final outcome may be based on either newImage:v2 or newImage:v3.

But i'm not sure it's always going to be that clear which is the "newer" one. Nor am I sure we can really eliminate that race condition fully (ie once a build for newImage:v2 is running, it's basically out of our hands unless we cancel it)

In sum, I think there's a pretty deep rathole here of "what should the behavior be" that we need to discuss.

—
Reply to this email directly or view it on GitHub.

[smarterclayton]

Agreed, this needs careful thought and brainstorming. We have to make sure the system converges forward where it can, and with things like builds and webhooks we do a best effort.

On Feb 1, 2015, at 5:11 PM, Ben Parees notifications@github.com wrote:

I think both build+deployment scenarios where it's due to a new image will need some careful logic.

I assume that if we're currently retrying an operation that was triggered by newImage:v2 and we get a new event for newimage:v3, we should abandon the retry logic that's operating on newImage:v2. If we don't, we end up with a race condition where the final outcome may be based on either newImage:v2 or newImage:v3.

But i'm not sure it's always going to be that clear which is the "newer" one. Nor am I sure we can really eliminate that race condition fully (ie once a build for newImage:v2 is running, it's basically out of our hands unless we cancel it)

In sum, I think there's a pretty deep rathole here of "what should the behavior be" that we need to discuss.

—
Reply to this email directly or view it on GitHub.

[bparees]

trello card for build side of things: https://trello.com/c/8tI8Hzh2/491-handle-error-conditions-in-build-controllers

[ironcladlou]

Trello card for deployments: https://trello.com/c/qDeHffOz

[bparees]

So i've done a study of the various build flows and come to the following conclusions:

First of all, this is all about creating a build/build pod. it has nothing to do with the ultimate successful completion of that build. (in case that's not obvious to anyone reading this).

With that in mind:

1. for webhook triggered builds (which are not queue-polling based anyway) we should not change things. We currently return appropriate http status codes on the webhook call, and if we're going to move to a model where we retry failures, the status codes we return would be come useless (either we'd have to always return 200 and hope that eventually we could succeed, or we'd have to perform a synchronous call to ensure the build was started (or conclude we couldn't start it) before returning. I suggest instead we keep things as is and put the onus on the webhook client to retry if so desired. I'm not sure what github actually does on a failed invocation, i'm guessing it won't retry, but again i'm not sure we can consistently do something smarter, so i prefer to that we simply report our failure reliably.

2. For the image change triggers poll loop, we could fail in two places:
   a) could not create the build object definition due to issues looking up associated imagerepo data
   b) failure posting the build object to the rest endpoint

   • long term we probably want to break this down into retryable errors (5xx) vs permanent failures (4xx)
     For now, in both cases we should simply requeue and retry the image repo event in the same way the scheduler is doing it (this has the deficiency as noted by @ironcladlou that we have to re-pull the latest version of the object and if that fails we're stuck). The image repo event will always contain the most up to date tag data (since we re-pull it), so the worst case outcome here is that we got an image change repo event where the tag corresponded to v1, we failed, pulled the latest repo object, tag now points to v2, and we queued that. We'll ultimately run a build using image v2 and never have performed a build using image v1.

Since the "normal" flow would have resulted in 2 builds, one for v1 and then one for v2, we still end up in the same final state: the last build run is for v2 and any downstream deployments are done w/ that image.

risks:
a) race conditions on pulling the latest version of the object and enqueuing it. I don't think the scheduler addresses that today either.
b) permanent failures will retry forever

3. BuilderController polling loop. This is the bit responsible for seeing new Build objects and creating BuildPods. It also monitors BuildPod changes and updates Build status accordingly.
   Failures in this flow are basically all due to api faliures (could not update status, could not create pod, etc). We should simply requeue the event (with a backoff and update) and try again, similar to scheduler behavior.

Longer term i'd like to address the indefinite failures with two additional strategies:

1. dividing api failures into retryable and non-retryable failures (don't requeue non-retryable failures)
2. compose a "retry count" field onto objects in the FIFO queues so the controllers can recognize when they've retried a particular event N times and stop retrying it.

thoughts?

[smarterclayton]

----- Original Message -----

So i've done a study of the various build flows and come to the following
conclusions:

First of all, this is all about creating a build/build pod. it has nothing
to do with the ultimate successful completion of that build. (in case
that's not obvious to anyone reading this).

With that in mind:

1. for webhook triggered builds (which are not queue-polling based anyway) we
   should not change things. We currently return appropriate http status codes
   on the webhook call, and if we're going to move to a model where we retry
   failures, the status codes we return would be come useless (either we'd have
   to always return 200 and hope that eventually we could succeed, or we'd have
   to perform a synchronous call to ensure the build was started (or conclude
   we couldn't start it) before returning. I suggest instead we keep things as
   is and put the onus on the webhook client to retry if so desired. I'm not
   sure what github actually does on a failed invocation, i'm guessing it won't
   retry, but again i'm not sure we can consistently do something smarter, so i
   prefer to that we simply report our failure reliably.

Agree - if you don't see 200 from your web hook, you retry, or you're not guaranteed to get a build.

2. For the image change triggers poll loop, we could fail in two places:
   a) could not create the build object definition due to issues looking up
   associated imagerepo data
   b) failure posting the build object to the rest endpoint
   • long term we probably want to break this down into retryable errors
     (5xx) vs permanent failures (4xx)
     For now, in both cases we should simply requeue and retry the image repo
     event in the same way the scheduler is doing it (this has the deficiency as
     noted by @ironcladlou that we have to re-pull the latest version of the
     object and if that fails we're stuck).

A future bit of work could be "identify items that are always failing and take punitive action", which is sending an event plus maybe blacklisting that item until the next resync.

The image repo event will always
contain the most up to date tag data (since we re-pull it), so the worst
case outcome here is that we got an image change repo event where the tag
corresponded to v1, we failed, pulled the latest repo object, tag now points
to v2, and we queued that. We'll ultimately run a build using image v2 and
never have performed a build using image v1.

Since the "normal" flow would have resulted in 2 builds, one for v1 and then
one for v2, we still end up in the same final state: the last build run is
for v2 and any downstream deployments are done w/ that image.

risks:
a) race conditions on pulling the latest version of the object and
enqueuing it. I don't think the scheduler addresses that today either.
b) permanent failures will retry forever

3. BuilderController polling loop. This is the bit responsible for seeing
   new Build objects and creating BuildPods. It also monitors BuildPod changes
   and updates Build status accordingly.
   Failures in this flow are basically all due to api faliures (could not
   update status, could not create pod, etc). We should simply requeue the
   event (with a backoff and update) and try again, similar to scheduler
   behavior.

Longer term i'd like to address the indefinite failures with two additional
strategies:

1. dividing api failures into retryable and non-retryable failures (don't
   requeue non-retryable failures)
2. compose a "retry count" field onto objects in the FIFO queues so the
   controllers can recognize when they've retried a particular event N times
   and stop retrying it.

thoughts?

Like. I think of the controller loop communicating a certain error type back to the thing calling it (the helper code that does retries and queues and loops) like ErrPermFail. The helper code would be parameterizable on how to handle perm fail (probably just send an event).

[ironcladlou]

risks: a) race conditions on pulling the latest version of the object and enqueuing it. I don't think the scheduler addresses that today either.

This is the issue I described in 4330. @smarterclayton, this is the main flaw I was trying to convey with using FIFO for retries. You can replace newer state with stale data in the queue. In the issue, I suggested one possible fix.

1. dividing api failures into retryable and non-retryable failures (don't
   requeue non-retryable failures)

Typed errors to drive pluggable retry mechanics makes sense to me.

[smarterclayton]

On Feb 13, 2015, at 5:59 PM, Dan Mace notifications@github.com wrote:

risks: a) race conditions on pulling the latest version of the object and enqueuing it. I don't think the scheduler addresses that today either.

This is the issue I described in 4330. @smarterclayton, this is the main flaw I was trying to convey with using FIFO for retries. You can replace newer state with stale data in the queue. In the issue, I suggested one possible fix.

Yes - you at minimum need to ensure you don't deliver new and then old, which is the implicit guarantee of fifo. Solvable as you noted by removing retry items from the retry queue when a newer item is about to be sent to nextstep()

1. dividing api failures into retryable and non-retryable failures (don't
   requeue non-retryable failures)

Typed errors to drive pluggable retry mechanics makes sense to me.

—
Reply to this email directly or view it on GitHub.

[ironcladlou]

Yes - you at minimum need to ensure you don't deliver new and then old, which is the implicit guarantee of fifo. Solvable as you noted by removing retry items from the retry queue when a newer item is about to be sent to nextstep()

I hadn't even thought of maintaining a retry queue... What I had suggested as a possibility elsewhere was to support versioning inside FIFO so that FIFO would no-op attempts to push new items which are deemed stale according to a pluggable versioning function. Then you can just re-fetch and push into the queue on retry, and if you raced (and lost) with the reflector adding some updated state to the queue while you were fetching, your old state would just silently get dropped internally.

[smarterclayton]

On Feb 13, 2015, at 8:38 PM, Dan Mace notifications@github.com wrote:

Yes - you at minimum need to ensure you don't deliver new and then old, which is the implicit guarantee of fifo. Solvable as you noted by removing retry items from the retry queue when a newer item is about to be sent to nextstep()

I hadn't even thought of maintaining a retry queue... What I had suggested as a possibility elsewhere was to support versioning inside FIFO so that FIFO would no-op attempts to push new items which are deemed stale according to a pluggable versioning function (for instance). Then you can just re-fetch and push into the queue on retry, and if you raced (and lost) with the reflector adding some updated state to the queue while you were fetching, your old state would just silently get dropped internally.

You are getting events from the source of truth, you don't need to refetch anything. If you don't get an event, it didn't change.
—
Reply to this email directly or view it on GitHub.