lookup buildconfigs in shared indexed cache instead of listing #10923

bparees · 2016-09-15T07:59:31Z

fixes #10616

bparees · 2016-09-15T08:23:14Z

@Kargakis @smarterclayton ptal

bparees · 2016-09-15T08:23:19Z

[test]

bparees · 2016-09-15T08:23:34Z

[testextended][extended:core(builds)]

bparees · 2016-09-15T09:55:44Z

pkg/build/controller/factory/factory.go

 		},
 	}
 }

+func (factory *ImageChangeControllerFactory) waitForSyncedStores() {
+	for !factory.BuildConfigIndexSynced() {


@Kargakis this appears to never return true... am I missing some additional initialization step?

I would suggest rewritting the controller to use Informers instead of the RunnableController framework. Currently nothing initializes the cache so it never syncs. By using the shared informer framework, the master will do it for you.

yeah i'm not signing up for rewriting the controllers just yet, that's a much larger piece of work so for now i just want to fix the performance problem.

when i remove the sync check, it actually does work, so the cache is getting properly populated. I couldn't tell from your other PR what is supposed to trigger the cache to be initialized..I assumed since it was a shared informer, that should already be happening even if i'm not using it...

bparees · 2016-09-15T11:05:09Z

pkg/cmd/server/origin/run_components.go

@@ -291,7 +291,9 @@ func (c *MasterConfig) RunBuildPodController() {
 func (c *MasterConfig) RunBuildImageChangeTriggerController() {
 	bcClient, _ := c.BuildImageChangeTriggerControllerClients()
 	bcInstantiator := buildclient.NewOSClientBuildConfigInstantiatorClient(bcClient)
-	factory := buildcontrollerfactory.ImageChangeControllerFactory{Client: bcClient, BuildConfigInstantiator: bcInstantiator}
+	bcIndex := &oscache.StoreToBuildConfigListerImpl{c.Informers.BuildConfigs().Indexer()}
+	bcIndexSynced := c.Informers.BuildConfigs().Informer().HasSynced


@Kargakis the informer is being created/initialized here, no?

This method starts all shared informers and it's called in master here.

right, so why is my cache never synced? my informer should be one of the ones getting started, right?

smarterclayton · 2016-09-15T16:23:17Z

You have to call .Informer() prior to RunBuildPodController - otherwise
your informer doesn't get started.

On Thu, Sep 15, 2016 at 12:12 PM, OpenShift Bot [email protected]
wrote:

continuous-integration/openshift-jenkins/testextended FAILURE (
https://ci.openshift.redhat.com/jenkins/job/test_pr_origin_extended/485/)
(Extended Tests: core(builds))

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#10923 (comment),
or mute the thread
https://github.com/notifications/unsubscribe-auth/ABG_p8Qh-pQ6INdaYBddS9A3WO-mWh4Uks5qqW5_gaJpZM4J9l_s
.

bparees · 2016-09-15T16:44:17Z

You have to call .Informer() prior to RunBuildPodController - otherwise your informer doesn't get started.

@smarterclayton i am:
https://github.com/openshift/origin/pull/10923/files#diff-232120b6845c4d46e68c543a21dbf8c8R295

the issue appears to be that despite putting this in a go routine:
https://github.com/openshift/origin/pull/10923/files#diff-232120b6845c4d46e68c543a21dbf8c8R297

everything hangs inside the waitForSync called within the Create() function, and the sync never occurs unless the main thread is able to make progress. I've even added runtime.Gosched() calls inside my wait loop, but it seems to never give up control so it can return.

bparees · 2016-09-15T18:30:45Z

pkg/cmd/server/origin/run_components.go

+	factory := buildcontrollerfactory.ImageChangeControllerFactory{Client: bcClient, BuildConfigInstantiator: bcInstantiator, BuildConfigIndex: bcIndex, BuildConfigIndexSynced: bcIndexSynced}
+	go func() {
+		factory.Create().Run()
+	}()


@smarterclayton @Kargakis this was the issue. go factory.Create().Run() hangs everything because the Create() call (which is where i'm doing the wait for sync operation) is not run in a go routine. sigh.

You should not have to call Run(). master.Start should be calling Start for you after you exit. If that's not the case, debug until you find out why.

@smarterclayton
Every other controller that's being created is calling its own run method after creating the controller. Are you saying these are all wrong?

https://github.com/openshift/origin/blob/master/pkg/cmd/server/origin/run_components.go#L88-L89
https://github.com/openshift/origin/blob/master/pkg/cmd/server/origin/run_components.go#L108
https://github.com/openshift/origin/blob/master/pkg/cmd/server/origin/run_components.go#L158
https://github.com/openshift/origin/blob/master/pkg/cmd/server/origin/run_components.go#L176
https://github.com/openshift/origin/blob/master/pkg/cmd/server/origin/run_components.go#L337
https://github.com/openshift/origin/blob/master/pkg/cmd/server/origin/run_components.go#L358

Ok, I was looking at something else before. This is fine.

smarterclayton · 2016-09-15T21:13:07Z

Do not call factory.Create.Run() yourself. Load the informer in master
config.

On Thu, Sep 15, 2016 at 4:02 PM, OpenShift Bot [email protected]
wrote:

continuous-integration/openshift-jenkins/testextended SUCCESS (
https://ci.openshift.redhat.com/jenkins/job/test_pr_origin_extended/486/)
(Extended Tests: core(builds))

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#10923 (comment),
or mute the thread
https://github.com/notifications/unsubscribe-auth/ABG_p86FELUrBOTKBbtlyvxix5sImDYQks5qqaRrgaJpZM4J9l_s
.

bparees · 2016-09-15T21:57:35Z

Hm?

That factory is for the controller, not the informer, it's the same pattern
all the controllers follow (create controller, via factory or otherwise,
and call run)

And the informer is being created the same way the other controllers are
doing it, which is in masterconfig...

Ben Parees | OpenShift

On Sep 15, 2016 23:13, "Clayton Coleman" [email protected] wrote:

Do not call factory.Create.Run() yourself. Load the informer in master
config.

On Thu, Sep 15, 2016 at 4:02 PM, OpenShift Bot [email protected]
wrote:

continuous-integration/openshift-jenkins/testextended SUCCESS (
https://ci.openshift.redhat.com/jenkins/job/test_pr_origin_extended/486/
)
(Extended Tests: core(builds))

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#10923 (comment),
or mute the thread
<https://github.com/notifications/unsubscribe-auth/ABG_
p86FELUrBOTKBbtlyvxix5sImDYQks5qqaRrgaJpZM4J9l_s>
.

—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
#10923 (comment),
or mute the thread
https://github.com/notifications/unsubscribe-auth/AEvl3mwc2mLO3W9qmXgxt9fPDbPOOgWlks5qqbTtgaJpZM4J9l_s
.

0xmichalis · 2016-09-16T08:06:27Z

pkg/build/controller/factory/factory.go

+func (factory *ImageChangeControllerFactory) waitForSyncedStores() {
+	for !factory.BuildConfigIndexSynced() {
+		glog.V(4).Infof("Waiting for the bc caches to sync before starting the imagechange buildconfig controller worker")
+		<-time.After(StoreSyncedPollPeriod)


select between this and Stop

0xmichalis · 2016-09-16T08:07:58Z

pkg/client/cache/buildconfig.go

+	return configs, nil
+}
+
+func (s *StoreToBuildConfigListerImpl) buildconfigs(namespace string) storebuildconfigsNamespacer {


Should be exported.

0xmichalis · 2016-09-16T08:08:38Z

pkg/client/cache/index.go

@@ -15,6 +17,40 @@ const (
 func ImageStreamReferenceIndexFunc(obj interface{}) ([]string, error) {
 	switch t := obj.(type) {
 	// TODO: Add support for build configs


remove TODO

bparees · 2016-09-16T10:58:17Z

@Kargakis thanks, comments addressed.

bparees · 2016-09-16T12:33:13Z

flake #10951
[test]

openshift-bot · 2016-09-16T14:37:33Z

continuous-integration/openshift-jenkins/test Waiting: Determining build queue position

smarterclayton · 2016-09-16T20:52:18Z

pkg/client/cache/buildconfig.go

+		return nil, err
+	}
+	if !exists {
+		return nil, kapierrors.NewNotFound(buildapi.Resource("BuildConfig"), name)


should be "buildconfigs"

should this be deploymentconfigs as well?
https://github.com/openshift/origin/blob/master/pkg/client/cache/deploymentconfig.go#L89

Yes - anyplace you see "Resource" should be treated like the lower case name as seen by the client.

has been changed.

smarterclayton · 2016-09-16T20:52:37Z

pkg/client/cache/buildconfig.go

+	}
+
+	var configs []*buildapi.BuildConfig
+


no newline here

also a pattern previously established but sure...will delete.
https://github.com/openshift/origin/blob/master/pkg/client/cache/deploymentconfig.go#L63-L65

smarterclayton · 2016-09-16T20:52:58Z

pkg/client/cache/buildconfig.go

+}
+
+func (s *StoreToBuildConfigListerImpl) BuildConfigs(namespace string) storebuildconfigsNamespacer {
+	return storebuildconfigsNamespacer{s.Indexer, namespace}


storeBuildConfigsNamespacer (camelCase)

smarterclayton · 2016-09-16T20:55:00Z

pkg/client/cache/buildconfig.go

+// GetConfigsForImageStream returns all the build configs that are triggered by the provided image stream
+// by searching through using the ImageStreamReferenceIndex (build configs are indexed in the cache
+// by image stream references).
+func (s *StoreToBuildConfigListerImpl) GetConfigsForImageStreamTrigger(stream *imageapi.ImageStream) ([]*buildapi.BuildConfig, error) {


I don't love taking an image stream here - it would be better to take a stream namespace and name.

I followed the pattern that was already established:
https://github.com/openshift/origin/blob/master/pkg/client/cache/deploymentconfig.go#L57

I don't love taking an image stream here. I'd prefer these look like StoreToReplicationControllerLister. DeploymentConfig is old.

I don't see an equivalent example in StoreToReplicationControllerLister, GetPodControllers() for example takes a Pod object, not a namespace+podname. Which is done because it needs the pod labels. But there's no other similar "look up by this other index object" example.

I kind of prefer this existing signature because it also makes it clear the object you are to pass in must be an ImageStream, whereas just taking (string,string) does not make that as obvious. It also leaves us the freedom to use other parts of the imagestream for the lookup in the future w/o refactoring anything. What is the advantage/reason for preferring (namespace,name) as the signature?

We're trying to converge these accessors on the client interfaces, so taking namespace and name is closer to the actual goal. Also creating the object in some contexts is silly (create a whole deployment config to provide a name and namespace) and can actually hide the fact that only namespace and name should be used.

@deads2k - @smarterclayton says you get the final say on this. see discussion above.

@deads2k - @smarterclayton says you get the final say on this. see discussion above.

Namespace and name please. For the same reasons clayton listed.

will change.

smarterclayton · 2016-09-16T20:55:13Z

pkg/client/cache/index.go

-	// TODO: Add support for build configs
+	case *buildapi.BuildConfig:
+		var keys []string
+


i'll delete the one in the deploymentconfig case just below this as well.... though personally i like a newline separation between variable declarations and code logic.

smarterclayton · 2016-09-16T20:56:05Z

pkg/client/cache/index.go

+			// uses the input image of the buildconfig as the imagestream
+			// to trigger off, so we need to look that up.
+			if from == nil {
+				from = buildutil.GetInputReference(t.Spec.Strategy)


You need to index on source.

what source?

Build source

i'm still confused. Why do i need to index on build source? we're not doing any lookups on build source currently. (and by build source what build source do you mean? git source? image input source? secret-as-source?)

Don't you have to check .Spec.Source.Images in order to find the other triggers?

no. will add a more explanatory comment in the code.

bparees · 2016-09-16T22:40:31Z

@smarterclayton changes i understood have been made. still not clear what you're saying about invoking Run() on the controller after creation, or what you mean about indexing on source.

bparees · 2016-09-17T07:23:28Z

flake #10773
[test]

bparees · 2016-09-19T07:46:02Z

@smarterclayton pushed changes in a new commit. unresolved comments:

indexing on build source
whether indexer has a bug requiring empty keys to be returned in some cases
refactoring the buildconfig lookup by imagestream method signature to (string,string)

bparees · 2016-09-21T08:23:14Z

@smarterclayton bump on #10923 (comment)

bparees · 2016-09-26T20:11:43Z

@smarterclayton need to resolve #10923 (comment)

bparees · 2016-09-26T22:15:30Z

@smarterclayton ok the comment block in index.go has been updated to explain what's going on with the imagetrigger indexing.

other than the open question of what the right signature is for GetConfigsForImageStreamTrigger(stream *imageapi.ImageStream), I think all your comments have been addressed.

bparees · 2016-09-27T13:52:09Z

@smarterclayton this is ready for final review.

smarterclayton · 2016-09-27T14:54:03Z

pkg/client/cache/index.go

+			// instead it triggers on the image being used as the builder/base image
+			// as referenced in the build strategy, so if this is an ICT w/ no
+			// explicit image reference, use the image referenced by the strategy
+			// because this is the default ICT.


As a note for the next version of the API, we should default this onto the ICT so that it's explicit rather than being implicit. Implicit creates more complicated code and complicates clients.

i think we needed this because our first version of the ICT didn't even let you specify the image, so this default behavior was needed to maintain compatibility.

smarterclayton · 2016-09-27T14:54:21Z

Please squash

smarterclayton · 2016-09-27T16:31:41Z

I think you broke the shared build config test making it flake now

On Tue, Sep 27, 2016 at 12:22 PM, OpenShift Bot [email protected]
wrote:

continuous-integration/openshift-jenkins/test Running (
https://ci.openshift.redhat.com/jenkins/job/test_pr_origin/9352/)

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#10923 (comment),
or mute the thread
https://github.com/notifications/unsubscribe-auth/ABG_p42Z29CQm5J7_RM08Ybm4H05qGWDks5quULZgaJpZM4J9l_s
.

smarterclayton · 2016-09-27T16:31:58Z

Removing merge until we can be sure it isn't impacted.

bparees · 2016-09-27T16:36:05Z

I think you broke the shared build config test making it flake now

got a link to the failure?

bparees · 2016-09-27T18:18:44Z

@smarterclayton so the failure on TestConcurrentBuildImageChangeTriggerControllers is because the cache never syncs. I don't know why that isn't affecting other imagechangetrigger tests... is there something about multiple controllers running that is going to stop the cache from reaching a synced state?

I0927 11:18:44.082349    4355 factory.go:323] Waiting for the bc caches to sync before starting the imagechange buildconfig controller worker

bparees · 2016-09-27T18:34:43Z

@smarterclayton @deads2k i think the failure was caused because the namespace and name arguments were reversed when i switched the api from taking an imagestream. Which is another example of why i think that's a bad api and it's better to take the explicit object instead of 2 strings that can be reversed and cause subtle bugs.

bparees · 2016-09-27T18:37:02Z

running new integration tests on updated pr here:
https://ci.openshift.redhat.com/jenkins/job/test_pull_requests_origin_integration/6443/
https://ci.openshift.redhat.com/jenkins/job/test_pull_requests_origin_integration/6444/
https://ci.openshift.redhat.com/jenkins/job/test_pull_requests_origin_integration/6445/

bparees · 2016-09-27T21:02:05Z

[merge]

openshift-bot · 2016-09-28T14:57:41Z

Evaluated for origin testextended up to 373b27b

openshift-bot · 2016-09-28T14:57:50Z

Evaluated for origin test up to 373b27b

openshift-bot · 2016-09-28T16:32:37Z

continuous-integration/openshift-jenkins/testextended SUCCESS (https://ci.openshift.redhat.com/jenkins/job/test_pr_origin_extended/529/) (Extended Tests: core(builds))

openshift-bot · 2016-09-28T16:37:44Z

continuous-integration/openshift-jenkins/test SUCCESS (https://ci.openshift.redhat.com/jenkins/job/test_pr_origin/9406/)

openshift-bot · 2016-09-28T19:17:54Z

continuous-integration/openshift-jenkins/merge SUCCESS (https://ci.openshift.redhat.com/jenkins/job/test_pr_origin/9406/) (Image: devenv-rhel7_5093)

openshift-bot · 2016-09-28T19:17:54Z

Evaluated for origin merge up to 373b27b

bparees · 2016-09-28T19:38:10Z

[merge]

bparees force-pushed the indexed_buildconfig_cache branch 3 times, most recently from 6a58a10 to e5a4636 Compare September 15, 2016 08:20

bparees commented Sep 15, 2016

View reviewed changes

bparees force-pushed the indexed_buildconfig_cache branch 2 times, most recently from 94fe34b to e30d636 Compare September 15, 2016 10:35

bparees commented Sep 15, 2016

View reviewed changes

bparees force-pushed the indexed_buildconfig_cache branch from e30d636 to dfc6fdc Compare September 15, 2016 15:21

bparees force-pushed the indexed_buildconfig_cache branch from dfc6fdc to 584b411 Compare September 15, 2016 18:29

bparees commented Sep 15, 2016

View reviewed changes

0xmichalis reviewed Sep 16, 2016

View reviewed changes

bparees force-pushed the indexed_buildconfig_cache branch from 584b411 to 7e63abc Compare September 16, 2016 10:14

smarterclayton suggested changes Sep 16, 2016

View reviewed changes

bparees force-pushed the indexed_buildconfig_cache branch from 7e63abc to 177b021 Compare September 16, 2016 22:38

bparees force-pushed the indexed_buildconfig_cache branch 2 times, most recently from 1ed11db to aba0916 Compare September 16, 2016 22:43

bparees force-pushed the indexed_buildconfig_cache branch from 3ee36a2 to f5a2cd8 Compare September 26, 2016 22:14

bparees force-pushed the indexed_buildconfig_cache branch from f5a2cd8 to 85adf0f Compare September 27, 2016 13:51

smarterclayton reviewed Sep 27, 2016

View reviewed changes

smarterclayton approved these changes Sep 27, 2016

View reviewed changes

bparees force-pushed the indexed_buildconfig_cache branch from 85adf0f to 58ef19d Compare September 27, 2016 15:10

bparees force-pushed the indexed_buildconfig_cache branch from 58ef19d to afbbd76 Compare September 27, 2016 18:35

lookup buildconfigs in shared indexed cache instead of listing

373b27b

bparees force-pushed the indexed_buildconfig_cache branch from afbbd76 to 373b27b Compare September 28, 2016 14:48

openshift-bot merged commit 068f174 into openshift:master Sep 28, 2016

bparees deleted the indexed_buildconfig_cache branch September 28, 2016 21:04

lookup buildconfigs in shared indexed cache instead of listing #10923

lookup buildconfigs in shared indexed cache instead of listing #10923

Conversation

bparees commented Sep 15, 2016 • edited Loading

bparees commented Sep 15, 2016

bparees commented Sep 15, 2016

bparees commented Sep 15, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

smarterclayton commented Sep 15, 2016

bparees commented Sep 15, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

smarterclayton commented Sep 15, 2016

bparees commented Sep 15, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bparees commented Sep 16, 2016

bparees commented Sep 16, 2016

openshift-bot commented Sep 16, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bparees commented Sep 16, 2016

bparees commented Sep 17, 2016

bparees commented Sep 19, 2016 • edited Loading

bparees commented Sep 21, 2016

bparees commented Sep 26, 2016

bparees commented Sep 26, 2016

bparees commented Sep 27, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

smarterclayton commented Sep 27, 2016

smarterclayton commented Sep 27, 2016

smarterclayton commented Sep 27, 2016

bparees commented Sep 27, 2016

bparees commented Sep 27, 2016

bparees commented Sep 27, 2016

bparees commented Sep 27, 2016

bparees commented Sep 27, 2016

openshift-bot commented Sep 28, 2016

openshift-bot commented Sep 28, 2016

openshift-bot commented Sep 28, 2016

openshift-bot commented Sep 28, 2016

openshift-bot commented Sep 28, 2016 • edited Loading

openshift-bot commented Sep 28, 2016

bparees commented Sep 28, 2016

bparees commented Sep 15, 2016 •

edited

Loading

openshift-bot commented Sep 16, 2016 •

edited

Loading

bparees commented Sep 19, 2016 •

edited

Loading

openshift-bot commented Sep 28, 2016 •

edited

Loading