{"action":"created","issue":{"url":"https://api.github.com/repos/openshift/origin/issues/824","labels_url":"https://api.github.com/repos/openshift/origin/issues/824/labels{/name}","comments_url":"https://api.github.com/repos/openshift/origin/issues/824/comments","events_url":"https://api.github.com/repos/openshift/origin/issues/824/events","html_url":"https://github.com/openshift/origin/issues/824","id":56179442,"number":824,"title":"Controllers do not requeue events on transient errors","user":{"login":"smarterclayton","id":1163175,"avatar_url":"https://avatars.githubusercontent.com/u/1163175?v=3","gravatar_id":"","url":"https://api.github.com/users/smarterclayton","html_url":"https://github.com/smarterclayton","followers_url":"https://api.github.com/users/smarterclayton/followers","following_url":"https://api.github.com/users/smarterclayton/following{/other_user}","gists_url":"https://api.github.com/users/smarterclayton/gists{/gist_id}","starred_url":"https://api.github.com/users/smarterclayton/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/smarterclayton/subscriptions","organizations_url":"https://api.github.com/users/smarterclayton/orgs","repos_url":"https://api.github.com/users/smarterclayton/repos","events_url":"https://api.github.com/users/smarterclayton/events{/privacy}","received_events_url":"https://api.github.com/users/smarterclayton/received_events","type":"User","site_admin":false},"labels":[{"url":"https://api.github.com/repos/openshift/origin/labels/bug","name":"bug","color":"fc2929"}],"state":"open","locked":false,"assignee":null,"milestone":null,"comments":13,"created_at":"2015-02-01T21:39:08Z","updated_at":"2015-02-16T02:55:10Z","closed_at":null,"body":"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.\r\n\r\nStories 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.\r\n\r\nIt 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.\r\n\r\nGiven the nature of the current behavior, we may need to delay features from beta2 in favor of ensuing the controller loops are sound.  \r\n\r\n@bparees @ironcladlou"},"comment":{"url":"https://api.github.com/repos/openshift/origin/issues/comments/74453838","html_url":"https://github.com/openshift/origin/issues/824#issuecomment-74453838","issue_url":"https://api.github.com/repos/openshift/origin/issues/824","id":74453838,"user":{"login":"bparees","id":4974046,"avatar_url":"https://avatars.githubusercontent.com/u/4974046?v=3","gravatar_id":"","url":"https://api.github.com/users/bparees","html_url":"https://github.com/bparees","followers_url":"https://api.github.com/users/bparees/followers","following_url":"https://api.github.com/users/bparees/following{/other_user}","gists_url":"https://api.github.com/users/bparees/gists{/gist_id}","starred_url":"https://api.github.com/users/bparees/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/bparees/subscriptions","organizations_url":"https://api.github.com/users/bparees/orgs","repos_url":"https://api.github.com/users/bparees/repos","events_url":"https://api.github.com/users/bparees/events{/privacy}","received_events_url":"https://api.github.com/users/bparees/received_events","type":"User","site_admin":false},"created_at":"2015-02-16T02:55:10Z","updated_at":"2015-02-16T02:55:10Z","body":"@ironcladlou I was thinking of a similar path (creating an AddIfNotPresent method on FIFO) but the problem there is that if the newer item on the queue was consumed before the retry logic tried to push the old one onto the queue, you'd end up processing the old state after the new state.  The only way to avoid that (that i've thought of) is for there to be only a single reader on the queue, and for the thread doing the reading to also be the thread doing the retry push.  The problem with that is it means you can't really do a backoff before pushing the event (since you'd hang the controller loop), which means you end up potentially fast spinning in a fail/retry state.\r\n\r\n@smarterclayton i don't follow your last comment.  I think the issue @ironcladlou and I are discussing is the following scenario:\r\n\r\nevent A.1 is pushed onto the FIFO queue\r\ncontroller pops it, attempts to process it, fails\r\nevent A.2 is pushed onto the FIFO queue\r\ncontroller re-queues A.1 for retry\r\nFIFO queue now contains only A.1 (due to the nature of the FIFO queue queuing items by key and resolving collisions by replacing with the last push).\r\ncontroller loop pops A.1, attempts to process it again, will never process A.2 (barring a restart of the controller in which it refetches everything)\r\n\r\nOne way to address that is to refresh A prior to requeuing it, so instead of re-queuing A.1, the controller would end up \"re\"queuing A.2.  But runs into the issues we've discussed:\r\n1) race condition on the refresh/requeue\r\n2) a failure to refresh results in a failure to requeue for retry\r\n\r\n"}}