Deployment controller w/ caches #9327
Conversation
|
cc: @ironcladlou @mfojtik |
0xmichalis
changed the title
|
I still need to update tests but this is ready for review @deads2k @smarterclayton @mfojtik |
| } | ||
|
|
||
| func (c *DeploymentController) worker() { | ||
| for { |
There was a problem hiding this comment.
Why not following here the upstream pattern for worker?
There was a problem hiding this comment.
Because it is broken. workers never exit when the queue shuts down.
There was a problem hiding this comment.
@Kargakis I like that workers stay at work even the assembly line shut downs... reminds me us ;-)
There was a problem hiding this comment.
But it's not good both health- and mental-wise
:)
On Mon, Jun 27, 2016 at 2:09 PM, Michal Fojtik [email protected]
wrote:
In pkg/deploy/controller/deployment/factory.go
#9327 (comment):
- // Set default environment values
- for _, env := range factory.Environment {
if set.Has(env.Name) {continue- key := pod.Namespace + "/" + deployutil.DeploymentNameFor(pod)
- return c.getByKey(key)
+}
+func (c *DeploymentController) worker() {
- for {
@Kargakis https://github.com/kargakis I like that workers stay in work
even the assembly line shut downs... reminds me us ;-)—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
https://github.com/openshift/origin/pull/9327/files/2bc801bef0539b5191b4d473cdf2a34f2ce817cd#r68565227,
or mute the thread
https://github.com/notifications/unsubscribe/ADuFf01MFStzCBcVkYwqfzTWjfvO9dUnks5qP713gaJpZM4I1iD4
.
|
[test] |
|
Updated unit tests (1137a5d)! Not a nice experience. |
|
LGTM, but govet does not like you ;) |
| // Handle processes deployment and either creates a deployer pod or responds | ||
| // to a terminal deployment status. | ||
| func (c *DeploymentController) Handle(deployment *kapi.ReplicationController) error { | ||
| currentStatus := deployutil.DeploymentStatusFor(deployment) | ||
| nextStatus := currentStatus | ||
| deploymentScaled := false | ||
|
|
||
| deployerPodName := deployutil.DeployerPodNameForDeployment(deployment.Name) | ||
| deployer, deployerErr := c.pn.Pods(deployment.Namespace).Get(deployerPodName) |
There was a problem hiding this comment.
Once this is non-live, make sure to fend off mutations.
There was a problem hiding this comment.
This is the deployer pod and I am not sure I want to make it susceptible to a stale cache. Especially when we collapse the deployer pod controller on this controller, we will need its latest status in order to make calls for the status of the deployment. Isn't this a valid concern for having it live?
There was a problem hiding this comment.
This is the deployer pod and I am not sure I want to make it susceptible to a stale cache. Especially when we collapse the deployer pod controller on this controller, we will need its latest status in order to make calls for the status of the deployment. Isn't this a valid concern for having it live?
Live checks with multiple etcds are surprisingly unreliable. Didn't we get here from either a pod update (you really want to use a matching cache, otherwise you get an update, do a get, hit a stale etcd, and get a 404) or an RC (we'll get a pod update later)?
There was a problem hiding this comment.
Didn't we get here from either a pod update
Hm, I see. I think I agree. I will rework to use the cache
|
lgtm |
|
For this pull, I still need to fix unit tests:/ |
|
Tests updated finally, @deads2k do we need the queue wrapper for our ratelimiting queue? |
|
#9638, [test] |
|
Evaluated for origin test up to a62140f |
|
continuous-integration/openshift-jenkins/test SUCCESS (https://ci.openshift.redhat.com/jenkins/job/test_pr_origin/5581/) |
|
[merge] |
|
continuous-integration/openshift-jenkins/merge SUCCESS (https://ci.openshift.redhat.com/jenkins/job/test_pr_origin/5581/) (Image: devenv-rhel7_4495) |
|
Evaluated for origin merge up to a62140f |
@smarterclayton @deads2k this is the followup to #9291 that will ensure we will sync the deployment in UPDATE and DELETE events for deployer pods. It also paves the way for #9296.
Closes #9575
Closes #7190