Adding deployment cancellation controller #2398
Conversation
abhgupta
commented
May 20, 2015
abhgupta
commented
- watches updates to deployments
- checks inflight (new/pending/running) deployments for the cancellation annotation
- if cancellation annotation is set, it sets the ActiveDeadlineSeconds on the deployer pod to 0
|
@derekwaynecarr PTAL |
| return nil | ||
| } | ||
|
|
||
| deployerPod, err := c.podClient.getPod(deployment.Namespace, deployutil.DeployerPodNameFor(deployment)) |
There was a problem hiding this comment.
The deployment is canceled, the pod is killed, when does the deployment go into a failed status? When the deployer pod shows a failed status, I assume the deployment will have its status updated?
There was a problem hiding this comment.
That is correct. The cancellation annotation is just to capture user intent. Based on that, we set the deployer pod ActiveDeadlineSeconds to 0. This results in the pod containers being killed and pod getting to a Failed state. This pod state/phase change triggers the state of the deployment being updated to Failed.
|
I am fine with the future issues to track unwinding the birth of all these client wrappers. [merge] |
|
continuous-integration/openshift-jenkins/merge SUCCESS (https://ci.openshift.redhat.com/jenkins/job/merge_pull_requests_origin/2063/) (Image: devenv-fedora_1601) |
|
[Test]ing while waiting on the merge queue |
|
continuous-integration/openshift-jenkins/test FAILURE (https://ci.openshift.redhat.com/jenkins/job/test_pull_requests_origin/2352/) |
|
Evaluated for origin up to 120c5b7 |
Merged by openshift-bot
|
Why is this controller separate from the deployment controller? I'd rather this check be called by the deployment controller (which has the exact same list of inputs). No real need for it to be separate. |
|
@smarterclayton I have no issues in merging the two - but this is exactly what I had mentioned on irc and you had indicated otherwise at that time. Refer the discussion around this line: https://botbot.me/freenode/openshift-dev/2015-05-21/?msg=39833705&page=8 Even if a combined controller is what we want, I would like to push it post-3.0. Let me know your thoughts and I will open an issue to track the refactor. |
|
Sorry, I was not promoting a separate controller as the primary discussion point, merely pointing out the store is our primary memory use concern. When operating on the same data (processing state of X to decide on behavior) generally they should be the same controller loop. I don't think deletion should be combined into here because it's a different access pattern. And it's likely I thought we were talking primarily about the deletion controller, not cancellation. Open a performance issue for post 3.0 please.
|
|
Agree that we probably don't want a separate controller. Let's make sure to consolidate ASAP. |
