Create a generic trigger controller for image streams #13242

smarterclayton · 2017-03-06T04:39:47Z

Create the structure for a generic image change trigger controller. The controller is passed a set of informers, which also specify how the data is loaded off the object for triggers and how the updated image is processed. The controller indexes triggers to provide O(1) notifications, and in theory, based on a shared informer, should be as efficient as can be.

Demonstrates how an annotation based trigger could be loaded and how builds can be triggered.

TODOs:

tests to validate caching is correct
deployment config change trigger example
jsonpath reaction for annotations
handle expectations (triggered resource update)
do a memory and perf test
validate this can be converted easily to shared informers

@bparees @mfojtik

smarterclayton · 2017-03-06T04:39:59Z

[merge]

smarterclayton · 2017-03-06T04:40:50Z

pkg/image/controller/trigger/customtrigger.go

+	Instantiator BuildConfigInstantiator
+}
+
+func (r *BuildConfigReaction) ImageChanged(obj interface{}, is *imageapi.ImageStream, operations []TriggerOperation) error {


I think this is equivalent to the bc image_change_controller.

mfojtik · 2017-03-06T09:25:34Z

pkg/image/api/v1/trigger/trigger.go

+	// from is the object this should trigger from. The kind and name fields must be set.
+	From ObjectReference `json:"from"`
+	// fieldPath is a JSONPath string to the field to edit on the object. Required.
+	FieldPath string `json:"fieldPath"`


should we call this to ?

fieldPath is clearer

mfojtik · 2017-03-06T09:26:18Z

pkg/image/api/v1/trigger/trigger.go

+	// namespaced, or if left empty on a namespaced object, means the current namespace.
+	Namespace string `json:"namespace,omitempty"`
+	// apiVersion is the group and version the type exists in. Optional.
+	APIVersion string `json:"apiVersion,omitempty"`


do we need Group here as well?

APIVersion is group and version.

mfojtik · 2017-03-06T09:39:51Z

pkg/image/controller/trigger/cache.go

+		return nil, false, nil
+	}
+	r.Namespace = m.GetNamespace()
+	r.Key = r.Namespace + "/" + m.GetName()


i believe there is a helper for this somewhere in indexer.

It can return an error, which isn't the case here.

mfojtik · 2017-03-06T09:40:03Z

pkg/image/controller/trigger/cache.go

+						if len(namespace) == 0 {
+							namespace = entry.Namespace
+						}
+						keys = append(keys, namespace+"/"+name)


and the same helper should be used here.

mfojtik · 2017-03-06T09:40:46Z

pkg/image/controller/trigger/cache.go

+	return triggerCache{
+		ThreadSafeStore: cache.NewThreadSafeStore(
+			cache.Indexers{
+				"images": func(obj interface{}) ([]string, error) {


@Kargakis you were advocating last time I was making my private indexers that they should all live in shared package :-)

Last time you didn't have a generic controller below you :)

mfojtik · 2017-03-06T09:41:49Z

pkg/image/controller/trigger/cache.go

+					entry := obj.(*TriggerCacheEntry)
+					var keys []string
+					for _, t := range entry.Triggers {
+						if t.From.Kind != "ImageStreamTag" || len(t.From.APIVersion) != 0 || t.Paused {


@smarterclayton this will work just for images or do we plan to make it generic enough to work with other resource types, like secrets or config maps?

A suggest has already been made for a PodPreset trigger. I think we can refactor this at that time - trigger.ObjectFieldTrigger is fairly generic, but not totally generic. Maybe pkg/trigger and then some helper code for this and others to be reused.

mfojtik · 2017-03-06T09:42:54Z

@smarterclayton this looks really good, I will have closer look later today :)

mfojtik · 2017-03-06T09:43:12Z

@soltysh FYI since you were interested in this area

0xmichalis · 2017-03-06T15:30:38Z

As a follow-up to this, we should probably collapse DC config change triggers into the DC controller.

smarterclayton · 2017-03-21T06:19:51Z

Ok, this should be close to a working prototype:

Set of incoming shared informers for the types to react to (deployments, builds, etc) plus one for image streams
Image stream informer drives a work queue sharded by image stream, that processes each image stream for all objects that trigger off of it (any image that registers a trigger that watches the stream by name) into a TriggerOperation, which is then put onto the operation queue, keyed by resource (i.e. deploymentconfigs/NAMESPACE/NAME).
Each of the other informers has a TriggerIndexer that takes the items coming off each informer and calculates:
whether triggers are registered or whether they have changed (to update the index of streams -> triggered objects)
the latest state of the triggers as represented by []TriggerOperation
Each triggered informer keeps the cache up to date (fast lookup of image stream to triggered object) and also queues trigger operations onto the operation queue
The operation queue merges TriggerOperations based on RV - a newer read from the cache overrides the older item - to form the set of changes that need to be dispatched
The resourceWorker in the controller grabs objects off the operation queue (sharded by object key) and tries to apply the reaction, such as launching a build.

Because image stream triggers are level driven, we can keep merging the queued operations forever.

If a triggered object and image stream are created at the same time, there is a window where:

triggered object beats the image stream and reads nothing from the cache
image stream update makes it to the worker, but the triggered object hasn't registered its triggers yet
triggered object would not get fired

However, we can queue incomplete operations for the triggered object (tag didn't exist, go ahead and queue the operation), and as part of the resourceWorker do a lazy lookup on those tags. If the tags don't exist, requeue for later (backoff). That should ensure that we don't miss an update cycle. That is not yet implemented here.

smarterclayton · 2017-03-22T03:08:57Z

Fixed the race condition - I check the for image updates on a triggered object after the item is added to the index - that means that either the image is in the cache already (processed by imageStreamWorker) or guaranteed to get the item from the index in the imageStreamWorker if it hasn't completed.

0xmichalis · 2017-03-22T13:01:50Z

pkg/image/api/v1/trigger/trigger.go

+}
+
+// ObjectReference identifies an object by its name and kind.
+type ObjectReference struct {


Why not use the existing ObjectReference type?

So that it's not dependent on kapi. Also doesn't need the same fields. Still effectively versioned, but not tied to Kube.

0xmichalis · 2017-03-22T13:07:47Z

pkg/image/controller/trigger/buildconfigs.go

+	"github.com/openshift/origin/pkg/image/api/v1/trigger"
+)
+
+func calculateBuildConfigTriggers(bc *buildapi.BuildConfig, retrieveOperations bool) ([]trigger.ObjectFieldTrigger, []TriggerOperation) {


0xmichalis · 2017-03-22T13:36:12Z

pkg/image/controller/trigger/trigger.go

+}
+
+// ImageStreamNamespaceLister helps list and get ImageStreams.
+type ImageStreamNamespaceLister interface {


The listers and informers in this file need to move to a different place.

This is waiting for Andy's rebase anyway, so generated informers makes this irrelevant.

0xmichalis · 2017-03-22T16:26:24Z

Try to keep the deployment example in a separate commit to make clearer what's needed for adding support for a resource.

bparees · 2017-03-23T12:51:43Z

pkg/image/controller/trigger/buildconfigs.go

+	Instantiate(namespace string, request *buildapi.BuildRequest) (*buildapi.Build, error)
+}
+
+type BuildConfigReaction struct {


0xmichalis · 2017-03-28T13:44:17Z

pkg/image/api/v1/trigger/trigger.go

@@ -0,0 +1,24 @@
+package trigger


Should move out of the image api

These triggers are image specific. The struct is under the API because they are serialized into annotations for the generic path.

openshift-bot · 2017-05-15T15:54:24Z

Evaluated for origin merge up to fb5994d

smarterclayton · 2017-05-15T21:51:13Z

[test]

…

On Mon, May 15, 2017 at 3:29 PM, OpenShift Bot ***@***.***> wrote: continuous-integration/openshift-jenkins/testextended FAILURE ( https://ci.openshift.redhat.com/jenkins/job/test_pull_ request_origin_extended/396/) (Base Commit: a740a1a <a740a1a>) (Extended Tests: core(build)) — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#13242 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ABG_p4UrqIbHxJ6qsFHFQmAza9KDSNBjks5r6KewgaJpZM4MTt7h> .

smarterclayton · 2017-05-16T19:22:04Z

[test]

smarterclayton · 2017-05-16T21:48:26Z

[test]

…

On Tue, May 16, 2017 at 5:42 PM, OpenShift Bot ***@***.***> wrote: continuous-integration/openshift-jenkins/test SUCCESS ( https://ci.openshift.redhat.com/jenkins/job/test_pull_request_origin/1504/) (Base Commit: 5de6326 <5de6326> ) — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#13242 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ABG_pwF2fv8NUVSmqN48qkpuPO5xkMn5ks5r6hgsgaJpZM4MTt7h> .

0xmichalis · 2017-05-17T07:32:24Z

#14156 [test]

Build a cache from changes to a set of watched objects that allow us to efficiently detect which objects can be *triggered* by image changes. Add a controller that watches image streams and mutates those objects whenever the tag reference in the object no longer points to the same docker image reference. Fit the existing deployment config and build config triggers into this framework, and add support for a new annotation based trigger. The annotation trigger will use an array of references serialized under key `image.alpha.openshift.io/triggers` that points to an image stream tag and identifies a field path to mutate. Rather than implementing a generic field path accessor, we define fast paths for known object types. Future work should be able to leverage the unstructured type to add triggers for third party resources or other unknown objects. Note that triggers do not grant access to pull images.

Only on a limited set of types for now. Future work should detect which types can have triggers.

We were not identifying container names correctly.

Remove the old build image trigger controller

openshift-bot · 2017-05-18T05:05:38Z

Evaluated for origin testextended up to 0aec48e

openshift-bot · 2017-05-18T05:05:50Z

Evaluated for origin test up to 0aec48e

openshift-bot · 2017-05-18T06:47:34Z

continuous-integration/openshift-jenkins/test SUCCESS (https://ci.openshift.redhat.com/jenkins/job/test_pull_request_origin/1545/) (Base Commit: b370e3a)

openshift-bot · 2017-05-18T07:37:39Z

continuous-integration/openshift-jenkins/testextended FAILURE (https://ci.openshift.redhat.com/jenkins/job/test_pull_request_origin_extended/421/) (Base Commit: b370e3a) (Extended Tests: core(build))

smarterclayton · 2017-05-18T16:32:29Z

In order to get more soak on this I'm going to direct merge to bypass the queue.

smarterclayton · 2017-05-18T16:32:52Z

We need the soak time in master more than the other things in the queue do.

gabemontero · 2017-05-19T18:54:31Z

(8) Rewrite build imagechange controller to use shared informers [techdebt]

gabemontero · 2017-05-19T18:54:34Z

(8) Rewrite build imagechange controller to use shared informers [techdebt]

smarterclayton commented Mar 6, 2017

View reviewed changes

smarterclayton mentioned this pull request Mar 6, 2017

Be able to use ImageStream with k8s objects #13042

Closed

mfojtik reviewed Mar 6, 2017

View reviewed changes

smarterclayton force-pushed the generic_trigger branch from 0d0ef62 to e066b6a Compare March 21, 2017 06:07

smarterclayton force-pushed the generic_trigger branch from e066b6a to 9daaa24 Compare March 22, 2017 03:07

smarterclayton force-pushed the generic_trigger branch 2 times, most recently from c6dd9b0 to 263d9da Compare March 22, 2017 04:23

0xmichalis reviewed Mar 22, 2017

View reviewed changes

smarterclayton force-pushed the generic_trigger branch from 263d9da to 064c600 Compare March 23, 2017 04:57

0xmichalis mentioned this pull request Mar 23, 2017

Use shared informer in BuildPodController and BuildPodDeleteController #13510

Merged

bparees reviewed Mar 23, 2017

View reviewed changes

smarterclayton force-pushed the generic_trigger branch 3 times, most recently from 07cab0b to fc12799 Compare March 25, 2017 04:27

0xmichalis self-assigned this Mar 28, 2017

0xmichalis reviewed Mar 28, 2017

View reviewed changes

smarterclayton force-pushed the generic_trigger branch from fb5994d to d17781c Compare May 17, 2017 02:26

openshift-bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label May 17, 2017

smarterclayton force-pushed the generic_trigger branch from d17781c to ede55af Compare May 17, 2017 20:43

openshift-bot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label May 17, 2017

smarterclayton added 10 commits May 18, 2017 00:55

makefile: Generate OpenAPI in the right spot

9fad0bd

Remove debugging logs from scheduler component, not needed anymore

a04dbe2

api: Allow instantiate to exclude some triggers

67f681b

Allow deployment instantiate to skip images

835fe7b

DeploymentTrigger test does not reset timer

dc59a7e

Support annotation triggers in oc set trigger

3df9423

Only on a limited set of types for now. Future work should detect which types can have triggers.

Init containers should be targets for triggers

45b9087

We were not identifying container names correctly.

Enable the image trigger controller with policy

9e46c40

Remove the old build image trigger controller

Debugging help for annotation triggers

0aec48e

smarterclayton force-pushed the generic_trigger branch from ede55af to 0aec48e Compare May 18, 2017 04:55

smarterclayton merged commit 6590461 into openshift:master May 18, 2017

gabemontero mentioned this pull request May 19, 2017

remove obsolete build ict ctrl; transfer legacy error unit tests to new ict ctrl #14226

Merged

Create a generic trigger controller for image streams #13242

Create a generic trigger controller for image streams #13242

Uh oh!

Conversation

smarterclayton commented Mar 6, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

smarterclayton commented Mar 6, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mfojtik commented Mar 6, 2017

Uh oh!

mfojtik commented Mar 6, 2017

Uh oh!

0xmichalis commented Mar 6, 2017

Uh oh!

smarterclayton commented Mar 21, 2017

Uh oh!

smarterclayton commented Mar 22, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

0xmichalis commented Mar 22, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

openshift-bot commented May 15, 2017

Uh oh!

smarterclayton commented May 15, 2017 via email

Uh oh!

smarterclayton commented May 16, 2017

Uh oh!

smarterclayton commented May 16, 2017 via email

Uh oh!

0xmichalis commented May 17, 2017

Uh oh!

openshift-bot commented May 18, 2017

Uh oh!

openshift-bot commented May 18, 2017

smarterclayton commented Mar 6, 2017 •

edited

Loading

smarterclayton commented Mar 6, 2017 •

edited

Loading

smarterclayton commented Mar 22, 2017 •

edited

Loading