Add 'Paused' to ImageChangeTrigger for BuildConfig

[wozniakjan]

When user adds ICT to BC, the image_trigger_controller.go starts a build and this may not be desired for certain scenarios where specifically only change in ImageStreamTag should trigger the build. Setting Paused: true tells the controller to not trigger the build.

https://trello.com/c/iT4bw5jg/1541-3-ability-to-set-imagechange-triggers-in-buildconfig-without-triggering-a-build-automatically

cc: @openshift/sig-developer-experience, @openshift/sig-master

[wozniakjan]

(3) Ability to set ImageChange triggers in BuildConfig without triggering a build automatically

[bparees]

@smarterclayton this is probably the place to have the discussion on what we want this API to be. Personally I like this approach because it mirrors the DC behavior which evolved after a number of painful experiences.

The challenges I see with the "paused" annotation is it's not clear what the behavior is for manually triggered builds and webhook triggered builds. It also doesn't allow for fine grained enable/disablement of ICTs which I admit isn't likely to be a 90% use case, but it's basically free w/ this implementation.

If we can define the expected behavior for a global "pause" on the resource, i'd be more amenable to it, but it's a deeper change since we'd have to add logical to our api to reject the build request (assuming we do indeed want to block all builds for the buildconfig when it is paused) and client tools to report the error to the user. It also becomes a very big hammer.

Alternatively, if the paused annotation still allowed webhook+manual triggered builds, I think users might find that confusing/surprising.

[bparees]

@smarterclayton an orthogonal question: When the build is unpaused (either individual ICT or the whole build), and if the image changed while it was paused, should be a build be run to "catch it up" or should image changes that occurred while the build was paused get ignored? (which would basically mean that while the build is paused we'd keep updating the ICT metadata as if the image had triggered a build, but not actually run the build).

My inclination would be that when it becomes unpaused, and if the image has changed, we would run a build using the latest image. (Which i think would happen automatically w/ the current implementation since the image change controller would see the ICT changed and reprocess it and see that it was "behind" the latest image)

[smarterclayton]

On May 4, 2018, at 7:32 AM, Ben Parees <notifications@github.com> wrote: @smarterclayton <https://github.com/smarterclayton> this is probably the place to have the discussion on what we want this API to be. Personally I like this approach because it mirrors the DC behavior which evolved after a number of painful experiences. The challenges I see with the "paused" annotation is it's not clear what the behavior is for manually triggered builds and webhook triggered builds We should use consistent naming rather than invent new inconsistent naming (generally). Automatic on DC was confusing. Some ways of saying it below: A paused webhook doesn’t fire (pretty clear). A paused image trigger doesn’t create new builds / deployments / statefulsets . It also doesn't allow for fine grained enable/disablement of ICTs which I admit isn't likely to be a 90% use case, but it's basically free w/ this implementation. There are two paused - one on image trigger annotations (already granular), and one on the object. Both are useful and we have examples of today. If we can define the expected behavior for a global "pause" on the resource, i'd be more amenable to it, but it's a deeper change since we'd have to add logical to our api to reject the build request (assuming we do indeed want to block all builds for the buildconfig when it is paused) What does Jenkins do? Paused on deployment means “don’t make any changes or reconcile me, I’m doing something”. That’s more “don’t reconcile” with your core controller. Of course, since build configs don’t launch a build when you change them, maybe paused is really about disabling the config change trigger. In which case since we don’t consistently implement that we might not want to add paused until we do. and client tools to report the error to the user. It also becomes a very big hammer. But individual ict flags are fiddly. Alternatively, if the paused annotation still allowed webhook+manual triggered builds, I think users might find that confusing/surprising. Same comment as above - I wasn’t suggesting an annotation. We already have a fine grained pause attribute on image change triggers as annotation. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#44 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ABG_pyESWcufU8RwZSzeX01hocX1XN80ks5tvDxEgaJpZM4TydF1> .

[bparees]

We already have a fine grained pause attribute on image change triggers as annotation.

ok. So it sounds like we need to:

1. add a paused attribute to the buildconfig ICT struct (basically what @wozniakjan did except rename it)
2. when paused is set back to false, a build will immediately run if the image changed while it was paused (this will happen just based on how the image_change_controller works today, i believe)
3. possibly someday in the future, add a paused attribute to our webhooks
4. possibly someday in the future add a paused attribute to the buildconfig itself

accurate?

[wozniakjan]

2. when paused is set back to false, a build will immediately run if the image changed while it was paused (this will happen just based on how the image_change_controller works today, i believe)

@bparees I think we may still need the change in buildconfigs.go handler used in image_change_controller.go as done here https://github.com/openshift/origin/pull/19624/files. Tested with a build from latest master and it happily triggered a build even with ICT paused=true field and I wasn't able to find any logic in the controller code that would lead me to think otherwise

[bparees]

@bparees I think we may still need the change in buildconfigs.go handler used in image_change_controller.go as done here

yeah, sorry I didn't mean to imply no changes were needed, we definitely need to look at whether the ICT is paused or not. What I was trying to say is that when you update the ICT to set paused=false, the image_change_controller will see the ICT update and reprocess the ICT and, if it's not paused and the image has changed, trigger a build. That logic is all in place already (watching for updates to buildconfigs/ICTs and running builds if the image is newer than what the ICT last saw).

[bparees]

s/controlls/controls/

[bparees]

@openshift/api-review i think this is ready to be re-reviewed.

Same comment as above - I wasn’t suggesting an annotation. We already have a fine grained pause attribute on image change triggers as annotation.

@smarterclayton i'm not familiar w/ this but we certainly don't have it for the imagechangetrigger object associated w/ builds (that object doesn't even have annotations)

[bparees]

@openshift/api-review bump

[wozniakjan]

@openshift/api-review any requests/comments?

[bparees]

s/object/image/

[liggitt]

the doc for objectfieldtrigger.paused is clearer:

// paused is true if this trigger is temporarily disabled. Optional.

would something like that be more accurate here? the "as well" in the current doc makes me wonder what paused: true doesn't pause

[bparees]

yeah i think that doc would be better.

[bparees]

@openshift/api-review approval please?

[bparees]

@openshift/api-review WIP tag removed.

[liggitt]

the doc improvement was all I noticed.

I had a hard time following the exchange between clayton and ben at #44 (comment). lgtm assuming that was resolved.

[bparees]

after this merges it will need to be cherry-picked into temporary-pre-3.11 and then origin can be bumped.