Deployment status (and build status, to a smaller degree) dominates in memory usage

[smarterclayton]

The deployment status message update is showing up in profiles of running systems with a significant amount of in memory use.

----------------------------------------------------------+-------------
                                             994MB   100% |   github.com/openshift/origin/pkg/deploy/controller/deployment.(*podClientImpl).createPod
         0     0% 81.49%   994.51MB 81.53%                | github.com/openshift/origin/pkg/deploy/controller/deploymentconfig.(*DeploymentConfigController).Handle
                                             710MB 71.43% |   strings.Join
                                             284MB 28.57% |   fmt.Sprintf
----------------------------------------------------------+-------------

The status message calculation is probably too aggressive - we look to be trying to update too much status, and not doing it efficiently (calculating a message and then throwing it away for a large percentage of the time).

We need to root cause how expensive this is.

[smarterclayton]

Showing up in the BU cluster, but could potentially be broader

[ironcladlou]

Ouch, yeah.

[ironcladlou]

It's also doing a lot of expensive lookups (like getting all buildconfigs across all namespaces for every image trigger in the config), and all this happens every time we handle a config.

[ironcladlou]

I wonder if this should be using cache.Store for the things it needs to analyze and deferring string building until it knows there's something to render.

[smarterclayton]

Also, if you update yourself, you're going to trigger an update... which
will result in yourself getting notified.

So first step - for both deployment (identified here) and build (@bparees)
let's do a quick triage to verify that we only update status when we have
to, not optimistically. If the controller decides not to take action, we
should record it. IF the controller hits an error, we should record it.
We shouldn't write status just because something changed.

On Thu, Oct 29, 2015 at 4:53 PM, Dan Mace notifications@github.com wrote:

I wonder if this should be using cache.Store for the things it needs to
analyze and deferring string building until it knows there's something to
render.

—
Reply to this email directly or view it on GitHub
#5504 (comment).

[ironcladlou]

Also, if you update yourself, you're going to trigger an update... which will result in yourself getting notified.

Double ouch

[smarterclayton]

Double ouch

On Thu, Oct 29, 2015 at 5:04 PM, Dan Mace notifications@github.com wrote:

Also, if you update yourself, you're going to trigger an update... which
will result in yourself getting notified.

Double ouch

—
Reply to this email directly or view it on GitHub
#5504 (comment).

[ironcladlou]

So first step - for both deployment (identified here) and build (@bparees) let's do a quick triage to verify that we only update status when we have to, not optimistically. If the controller decides not to take action, we should record it. IF the controller hits an error, we should record it. We shouldn't write status just because something changed.

Just riffing at the moment:

So, the status being reported here is based on externalities/associated resources. I think framing the update around the config updates might not be ideal. One option for a quick improvement in the short term would be to do the status updates in another controller that just periodically reconciles the config status with other resources.

[bparees]

I'm pretty sure (after surveying the code) builds only attempt to update the build.status.message if an error occurs during the handling process or we are explicitly changing the state (eg from running to complete)...

[smarterclayton]

Are we setting a Reason? Or just message?

On Oct 29, 2015, at 5:09 PM, Dan Mace notifications@github.com wrote:

So first step - for both deployment (identified here) and build (@bparees
https://github.com/bparees) let's do a quick triage to verify that we
only update status when we have to, not optimistically. If the controller
decides not to take action, we should record it. IF the controller hits an
error, we should record it. We shouldn't write status just because
something changed.

Just riffing at the moment:

So, the status being reported here is based on externalities/associated
resources. I think framing the update around the config updates might not
be ideal. One option for a quick improvement in the short term would be to
do the status updates in another controller that just periodically
reconciles the config status with other resources.

—
Reply to this email directly or view it on GitHub
#5504 (comment).

[ironcladlou]

Just a message.