Too many events for can't create initial deployment

[smarterclayton]

We're creating a massive number of initial deployment events. Have we eliminated this being repeatedly sent?

https://app.getsentry.com/red-hat/openshift-3-stg/group/66857349/

*errors.errorString: couldn't create initial Deployment for DeploymentConfig barrett-project/cakephp-frontend:0: DeploymentConfig "cakephp-frontend" is invalid: triggers[0].imageChange.tag: invalid value 'latest': no image recorded for barrett-project/cakephp-example:latest
  File "/builddir/build/BUILD/openshift-git-21.5837ae7/_build/src/github.com/openshift/origin/pkg/cmd/util/serviceability/sentry.go", line 61, in CaptureError
  File "/builddir/build/BUILD/openshift-git-21.5837ae7/_build/src/github.com/openshift/origin/pkg/cmd/util/serviceability/panic.go", line 28, in CaptureError.fm
  File "/builddir/build/BUILD/openshift-git-21.5837ae7/_thirdpartyhacks/src/github.com/GoogleCloudPlatform/kubernetes/pkg/util/util.go", line 82, in HandleError
  File "/builddir/build/BUILD/openshift-git-21.5837ae7/_build/src/github.com/openshift/origin/pkg/deploy/controller/configchange/factory.go", line 73, in func.007
  File "/builddir/build/BUILD/openshift-git-21.5837ae7/_build/src/github.com/openshift/origin/pkg/controller/controller.go", line 141, in Retry
...
(6 additional frame(s) were not displayed)

[ironcladlou]

Can you expand a little on the scope/context of "massive"? Lots for a given deployment? Or within a namespace, or across namespaces? Like, it seems extremely common for deployment creation to beat image tag availability so far, which means at least a couple per initial deployment, but that doesn't seem like a lot within the context of a single deployment's life. But if we're seeing tons of them for a single deployment, I'd like to figure out how to curtail it.

[smarterclayton]

Seeing about 3k per hour per deployment that never gets this.

On Jun 29, 2015, at 3:10 PM, Dan Mace notifications@github.com wrote:

Can you expand a little on the scope/context of "massive"? Lots for a given
deployment? Or within a namespace, or across namespaces? Like, it seems
extremely common for deployment creation to beat image tag availability so
far, which means at least a couple per initial deployment, but that doesn't
seem like a lot within the context of a single deployment's life. But if
we're seeing tons of them for a single deployment, I'd like to figure out
how to curtail it.

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

[ironcladlou]

Okay, that definitely sucks. Let's prioritize the issue and figure out how to address it.

[smarterclayton]

@bparees can walk you through the process

On Jun 30, 2015, at 8:24 AM, Dan Mace notifications@github.com wrote:

Okay, that definitely sucks. Let's prioritize the issue and figure out how
to address it.

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

[ironcladlou]

Summarizing some IRC discussion here.

Conceptually what's happening is that the ConfigChange trigger is failing to fire because image stream tags defined on the ImageChange trigger aren't yet available. This isn't really an error that should be logged or reported via events; instead, this is a reason for a state that the trigger (or possibly the DC itself) is currently in. Some ways this can be represented:

1. The ConfigChange trigger has a status and a message indicating that it's not currently incrementing the new deployment version because image tags aren't yet available
2. The DeploymentConfig status has some message indicating that its ConfigChange trigger is currently ineffective due to the lack of image tag availability

In either case, the deployment can actually be started immediately with oc deploy ... --latest, which would result in a deployment using whatever images are defined on the containers in the DC's pod template. I feel it's important to make that distinction since ultimately the thing users are seeing is the absence of a triggered/automatic deployment- manual still works.

Do we agree that there needs to be additional API to store and present this information and eliminate the event reporting entirely?

[smarterclayton]

We should eliminate the error reporting now as a fix. We can have a
separate issue to present the information eventually.

On Wed, Jul 1, 2015 at 9:47 AM, Dan Mace notifications@github.com wrote:

Summarizing some IRC discussion here.

Conceptually what's happening is that the ConfigChange trigger is failing
to fire because image stream tags defined on the ImageChange trigger aren't
yet available. This isn't really an error that should be logged or reported
via events; instead, this is a reason for a state that the trigger (or
possibly the DC itself) is currently in. Some ways this can be represented:

1. The ConfigChange trigger has a status and a message indicating that
   it's not currently incrementing the new deployment version because image
   tags aren't yet available
2. The DeploymentConfig status has some message indicating that its
   ConfigChange trigger is currently ineffective due to the lack of image tag
   availability

In either case, the deployment can actually be started immediately with oc
deploy ... --latest, which would result in a deployment using whatever
images are defined on the containers in the DC's pod template. I feel it's
important to make that distinction since ultimately the thing users are
seeing is the absence of a triggered/automatic deployment- manual still
works.

Do we agree that there needs to be additional API to store and present
this information and eliminate the event reporting entirely?

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

Clayton Coleman | Lead Engineer, OpenShift

[bparees]

The ConfigChange trigger isn't failing to fire, right? it's firing but failing to create a deployment because it can't resolve the IST.

It does seem like there is a needed status of "Deployment pending, waiting for IST availability" in these scenarios.

In either case, the deployment can actually be started immediately with oc deploy ... --latest, which would result in a deployment using whatever images are defined on the containers in the DC's pod template.

i don't quite follow this. you mean if the container points to a specific docker image and not an IST? I know the way i've used DCs before, the "image" I specify in the pod template is totally bogus, because I know it's going to get filled in from the ImageChangeTrigger. See for example:
https://github.com/openshift/origin/blob/master/examples/sample-app/application-template-stibuild.json#L226

Since "origin-ruby-sample" isn't a valid pull spec, that can never successfully deploy unless the ImageChangeTrigger resolves (and near as I can tell, you guys resolve the ImageChangeTrigger reference even if the deployment is triggered by a ConfigChange)

I would argue that even a manual deploy should be resolving the image based on the ICT definition, otherwise a manual deployment is likely to produce a fairly unexpected result. That said, we did that in the build logic for a while and ultimately decided it was a mess :)

[ironcladlou]

@bparees

The ConfigChange trigger isn't failing to fire, right? it's firing but failing to create a deployment because it can't resolve the IST.

It might be a distinction without a difference, I guess. The trigger is technically firing, but it's deciding not to do anything due to a probably transient error state. It's firing and not doing something the user expects for a reason it could be reporting somehow.

In either case, the deployment can actually be started immediately with oc deploy ... --latest, which would result in a deployment using whatever images are defined on the containers in the DC's pod template.

i don't quite follow this. you mean if the container points to a specific docker image and not an IST? I know the way i've used DCs before, the "image" I specify in the pod template is totally bogus, because I know it's going to get filled in from the ImageChangeTrigger. See for example:

It means that whatever pullspecs are on the container specs in the deployment config would get used. They might not be valid, as you point out. So maybe the fact that you can deploy manually doesn't really mean much.

I would argue that even a manual deploy should be resolving the image based on the ICT definition, otherwise a manual deployment is likely to produce a fairly unexpected result. That said, we did that in the build logic for a while and ultimately decided it was a mess :)

This goes back to the discussion in #2819 where we consider providing a means for the user to decide whether to use what's on the template if the tag replacements fail. It does seem hairy.

[ironcladlou]

We should eliminate the error reporting now as a fix. We can have a separate issue to present the information eventually.

Should we keep a V(4) error in the logs? Doesn't seem like we should totally ignore the error.

[smarterclayton]

If you are treating it as fatal (don't retry) then you can leave it at V(4).

On Wed, Jul 1, 2015 at 10:32 AM, Dan Mace notifications@github.com wrote:

We should eliminate the error reporting now as a fix. We can have a
separate issue to present the information eventually.

Should we keep a V(4) error in the logs? Doesn't seem like we should
totally ignore the error.

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

Clayton Coleman | Lead Engineer, OpenShift

[ironcladlou]

It can't be fatal, so I guess we'll just eliminate logging entirely for now and I'll spawn an issue.

[bparees]

it's going to "retry" every 2 minutes due to the nature of the sync loop.

On Wed, Jul 1, 2015 at 10:53 AM, Clayton Coleman notifications@github.com
wrote:

If you are treating it as fatal (don't retry) then you can leave it at
V(4).

On Wed, Jul 1, 2015 at 10:32 AM, Dan Mace notifications@github.com
wrote:

We should eliminate the error reporting now as a fix. We can have a
separate issue to present the information eventually.

Should we keep a V(4) error in the logs? Doesn't seem like we should
totally ignore the error.

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

Clayton Coleman | Lead Engineer, OpenShift

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

Ben Parees | OpenShift

[ironcladlou]

Created #3526 to track the API design.