From 545e683f8246e637913143a8a4bfb4a1c7e64089 Mon Sep 17 00:00:00 2001 From: Andy Goldstein Date: Thu, 28 May 2015 13:42:58 -0400 Subject: [PATCH 1/2] Fix build image change race The build image change controller was performing the following logic once it determined a build was needed: 1. Bump the BuildConfig so the ResourceVersion would be updated, in theory to prevent other image change controllers from being able to trigger a build for the same BuildConfig revision. 2. Make an API call to instantiate a build (BuildGenerator) 3. The BuildGenerator would try to modify the BuildConfig's LastVersion 4. The BuildGenerator would try to create a Build 5. The image change controller would try to update the BuildConfig to set the new LastTriggeredImageID value. If another instance of an image change controller bumped the BuildConfig's ResourceVersion before 3 above, you could get into an infinite race loop where 1 controller would keep bumping the ResourceVersion just before the BuildGenerator tried to update the config's LastVersion. This change makes it so the BuildGenerator is the only thing responsible for both creating a Build and updating the BuildConfig. The Build is created prior to updating the BuildConfig. If the Build creation fails, it won't try to update the BuildConfig. BuildRequest gains a new required field (Image) that the image change controller sets to tell the BuildGenerator what value to set for LastTriggeredImageID. --- pkg/build/api/types.go | 3 + pkg/build/api/v1/types.go | 3 + pkg/build/api/v1beta1/types.go | 3 + pkg/build/api/v1beta3/types.go | 3 + pkg/build/controller/factory/factory.go | 2 - .../controller/image_change_controller.go | 79 ++++++------------- .../image_change_controller_test.go | 55 ++++++------- pkg/build/generator/generator.go | 34 ++++++-- pkg/build/generator/generator_test.go | 10 ++- pkg/cmd/server/origin/master.go | 3 +- 10 files changed, 95 insertions(+), 100 deletions(-) diff --git a/pkg/build/api/types.go b/pkg/build/api/types.go index 3a9749dbbd7a..d3d7187e98b2 100644 --- a/pkg/build/api/types.go +++ b/pkg/build/api/types.go @@ -406,6 +406,9 @@ type BuildRequest struct { // Revision is the information from the source for a specific repo snapshot. Revision *SourceRevision `json:"revision,omitempty"` + + // Image is the image that triggered this build. + Image string } // BuildLogOptions is the REST options for a build log diff --git a/pkg/build/api/v1/types.go b/pkg/build/api/v1/types.go index ba78ede4958d..d379a2f1ac53 100644 --- a/pkg/build/api/v1/types.go +++ b/pkg/build/api/v1/types.go @@ -390,6 +390,9 @@ type BuildRequest struct { // Revision is the information from the source for a specific repo snapshot. Revision *SourceRevision `json:"revision,omitempty"` + + // Image is the image that triggered this build. + Image string `json:"image,omitempty"` } // BuildLogOptions is the REST options for a build log diff --git a/pkg/build/api/v1beta1/types.go b/pkg/build/api/v1beta1/types.go index b36e04cc4869..e41d5b2e7e2c 100644 --- a/pkg/build/api/v1beta1/types.go +++ b/pkg/build/api/v1beta1/types.go @@ -455,6 +455,9 @@ type BuildRequest struct { // Revision is the information from the source for a specific repo snapshot. Revision *SourceRevision `json:"revision,omitempty"` + + // Image is the image that triggered this build. + Image string `json:"image,omitempty"` } // BuildLogOptions is the REST options for a build log diff --git a/pkg/build/api/v1beta3/types.go b/pkg/build/api/v1beta3/types.go index 3ebae8e40e81..539b63a9d078 100644 --- a/pkg/build/api/v1beta3/types.go +++ b/pkg/build/api/v1beta3/types.go @@ -390,6 +390,9 @@ type BuildRequest struct { // Revision is the information from the source for a specific repo snapshot. Revision *SourceRevision `json:"revision,omitempty"` + + // Image is the image that triggered this build. + Image string `json:"image,omitempty"` } // BuildLogOptions is the REST options for a build log diff --git a/pkg/build/controller/factory/factory.go b/pkg/build/controller/factory/factory.go index e84d00075524..b3a005b448c7 100644 --- a/pkg/build/controller/factory/factory.go +++ b/pkg/build/controller/factory/factory.go @@ -205,7 +205,6 @@ func (factory *BuildPodControllerFactory) CreateDeleteController() controller.Ru type ImageChangeControllerFactory struct { Client osclient.Interface BuildConfigInstantiator buildclient.BuildConfigInstantiator - BuildConfigUpdater buildclient.BuildConfigUpdater // Stop may be set to allow controllers created by this factory to be terminated. Stop <-chan struct{} } @@ -221,7 +220,6 @@ func (factory *ImageChangeControllerFactory) Create() controller.RunnableControl imageChangeController := &buildcontroller.ImageChangeController{ BuildConfigStore: store, - BuildConfigUpdater: factory.BuildConfigUpdater, BuildConfigInstantiator: factory.BuildConfigInstantiator, Stop: factory.Stop, } diff --git a/pkg/build/controller/image_change_controller.go b/pkg/build/controller/image_change_controller.go index d74bbf01f8ca..6124c08b6c24 100644 --- a/pkg/build/controller/image_change_controller.go +++ b/pkg/build/controller/image_change_controller.go @@ -4,6 +4,7 @@ import ( "fmt" "strings" + kerrors "github.com/GoogleCloudPlatform/kubernetes/pkg/api/errors" "github.com/golang/glog" kapi "github.com/GoogleCloudPlatform/kubernetes/pkg/api" @@ -32,7 +33,6 @@ func (e ImageChangeControllerFatalError) Error() string { type ImageChangeController struct { BuildConfigStore cache.Store BuildConfigInstantiator buildclient.BuildConfigInstantiator - BuildConfigUpdater buildclient.BuildConfigUpdater // Stop is an optional channel that controls when the controller exits Stop <-chan struct{} } @@ -51,11 +51,6 @@ func (c *ImageChangeController) HandleImageRepo(repo *imageapi.ImageStream) erro // TODO: this is inefficient for _, bc := range c.BuildConfigStore.List() { config := bc.(*buildapi.BuildConfig) - obj, err := kapi.Scheme.Copy(config) - if err != nil { - continue - } - originalConfig := obj.(*buildapi.BuildConfig) from := buildutil.GetImageStreamForStrategy(config.Parameters.Strategy) if from == nil || from.Kind != "ImageStreamTag" { @@ -63,6 +58,7 @@ func (c *ImageChangeController) HandleImageRepo(repo *imageapi.ImageStream) erro } shouldBuild := false + triggeredImage := "" // For every ImageChange trigger find the latest tagged image from the image repository and replace that value // throughout the build strategies. A new build is triggered only if the latest tagged image id or pull spec // differs from the last triggered build recorded on the build config. @@ -99,68 +95,37 @@ func (c *ImageChangeController) HandleImageRepo(repo *imageapi.ImageStream) erro next := latest.DockerImageReference if len(last) == 0 || (len(next) > 0 && next != last) { - trigger.ImageChange.LastTriggeredImageID = next + triggeredImage = next shouldBuild = true + // it doesn't really make sense to have multiple image change triggers any more, + // so just exit the loop now + break } } if shouldBuild { - // The following update is meant to reduce the chance that the image change controller - // will kick off multiple builds on an image change in a HA setup, where multiple controllers - // of the same type may be looking at the same etcd data. - // If multiple controllers read the same build config (with same ResourceVersion) above and - // make a determination that a build needs to be kicked off, the update will only allow one of - // those controllers to continue to launch the build, while the rest will return an error and - // reset their queue. This won't eliminate the chance of multiple builds, since another controller - // can read the build after this update and launch its own build. - // TODO: Find a better mechanism to synchronize in a HA setup. - if err := c.BuildConfigUpdater.Update(originalConfig); err != nil { - // Cannot make an update to the original build config. Likely it has been changed by another process - glog.V(4).Infof("Cannot update BuildConfig %s/%s when preparing to update LastTriggeredImageID: %v", config.Namespace, config.Name, err) - return err - } - glog.V(4).Infof("Running build for BuildConfig %s/%s", config.Namespace, config.Name) // instantiate new build - request := &buildapi.BuildRequest{ObjectMeta: kapi.ObjectMeta{Name: config.Name}} - if _, err := c.BuildConfigInstantiator.Instantiate(config.Namespace, request); err != nil { - return fmt.Errorf("error instantiating Build from BuildConfig %s/%s: %v", config.Namespace, config.Name, err) + request := &buildapi.BuildRequest{ + ObjectMeta: kapi.ObjectMeta{ + Name: config.Name, + }, + Image: triggeredImage, } - // and update the config - if err := c.updateConfig(config); err != nil { - // This is not a retryable error. The worst case outcome of not updating the buildconfig - // is that we might rerun a build for the same "new" imageid change in the future, - // which is better than guaranteeing we run the build 2+ times by retrying it here. - return ImageChangeControllerFatalError{ - Reason: fmt.Sprintf("error updating BuildConfig %s/%s with new LastTriggeredImageID", config.Namespace, config.Name), - Err: err, + if _, err := c.BuildConfigInstantiator.Instantiate(config.Namespace, request); err != nil { + if kerrors.IsConflict(err) { + // This is not a retryable error. The worst case outcome of not updating the buildconfig + // is that we might rerun a build for the same "new" imageid change in the future, + // which is better than guaranteeing we run the build 2+ times by retrying it here. + return ImageChangeControllerFatalError{ + Reason: fmt.Sprintf("unable to instantiate Build for BuildConfig %s/%s due to a conflicting update: %v", config.Namespace, config.Name, err), + Err: err, + } } + + return fmt.Errorf("error instantiating Build from BuildConfig %s/%s: %v", config.Namespace, config.Name, err) } } } return nil } - -// updateConfig is responsible for updating current BuildConfig object which was changed -// during instantiate call, it basically copies LastTriggeredImageID to fresh copy -// of the BuildConfig object -func (c *ImageChangeController) updateConfig(config *buildapi.BuildConfig) error { - item, _, err := c.BuildConfigStore.Get(config) - if err != nil { - return err - } - if item == nil { - return fmt.Errorf("unable to retrieve BuildConfig %s/%s for updating", config.Namespace, config.Name) - } - newConfig := item.(*buildapi.BuildConfig) - for i, trigger := range newConfig.Triggers { - if trigger.Type != buildapi.ImageChangeBuildTriggerType { - continue - } - change := trigger.ImageChange - change.LastTriggeredImageID = config.Triggers[i].ImageChange.LastTriggeredImageID - } - glog.V(4).Infof("BuildConfig %s/%s is about to be updated", config.Namespace, config.Name) - - return c.BuildConfigUpdater.Update(newConfig) -} diff --git a/pkg/build/controller/image_change_controller_test.go b/pkg/build/controller/image_change_controller_test.go index 9f2f1b11339f..0b0a0898a98f 100644 --- a/pkg/build/controller/image_change_controller_test.go +++ b/pkg/build/controller/image_change_controller_test.go @@ -1,11 +1,13 @@ package controller import ( + "errors" "fmt" "strings" "testing" kapi "github.com/GoogleCloudPlatform/kubernetes/pkg/api" + kerrors "github.com/GoogleCloudPlatform/kubernetes/pkg/api/errors" "github.com/GoogleCloudPlatform/kubernetes/pkg/client/testclient" buildapi "github.com/openshift/origin/pkg/build/api" @@ -22,7 +24,7 @@ func TestNewImageID(t *testing.T) { image := mockImage("testImage@id", "registry.com/namespace/imagename:newImageID123") controller := mockImageChangeController(buildcfg, imageStream, image) bcInstantiator := controller.BuildConfigInstantiator.(*buildConfigInstantiator) - bcUpdater := controller.BuildConfigUpdater.(*mockBuildConfigUpdater) + bcUpdater := bcInstantiator.buildConfigUpdater err := controller.HandleImageRepo(imageStream) if err != nil { @@ -50,7 +52,7 @@ func TestNewImageIDDefaultTag(t *testing.T) { image := mockImage("testImage@id", "registry.com/namespace/imagename:newImageID123") controller := mockImageChangeController(buildcfg, imageStream, image) bcInstantiator := controller.BuildConfigInstantiator.(*buildConfigInstantiator) - bcUpdater := controller.BuildConfigUpdater.(*mockBuildConfigUpdater) + bcUpdater := bcInstantiator.buildConfigUpdater err := controller.HandleImageRepo(imageStream) if err != nil { @@ -78,7 +80,7 @@ func TestNonExistentImageStream(t *testing.T) { image := mockImage("testImage@id", "registry.com/namespace/imagename@id") controller := mockImageChangeController(buildcfg, imageStream, image) bcInstantiator := controller.BuildConfigInstantiator.(*buildConfigInstantiator) - bcUpdater := controller.BuildConfigUpdater.(*mockBuildConfigUpdater) + bcUpdater := bcInstantiator.buildConfigUpdater err := controller.HandleImageRepo(imageStream) if err != nil { @@ -99,7 +101,7 @@ func TestNewImageDifferentTagUpdate(t *testing.T) { image := mockImage("testImage@id", "registry.com/namespace/imagename@id") controller := mockImageChangeController(buildcfg, imageStream, image) bcInstantiator := controller.BuildConfigInstantiator.(*buildConfigInstantiator) - bcUpdater := controller.BuildConfigUpdater.(*mockBuildConfigUpdater) + bcUpdater := bcInstantiator.buildConfigUpdater err := controller.HandleImageRepo(imageStream) if err != nil { @@ -122,7 +124,7 @@ func TestNewImageDifferentTagUpdate2(t *testing.T) { image := mockImage("testImage@id", "registry.com/namespace/imagename@id") controller := mockImageChangeController(buildcfg, imageStream, image) bcInstantiator := controller.BuildConfigInstantiator.(*buildConfigInstantiator) - bcUpdater := controller.BuildConfigUpdater.(*mockBuildConfigUpdater) + bcUpdater := bcInstantiator.buildConfigUpdater err := controller.HandleImageRepo(imageStream) if err != nil { @@ -143,7 +145,7 @@ func TestNewDifferentImageUpdate(t *testing.T) { image := mockImage("testImage@id", "registry.com/namespace/imagename@id") controller := mockImageChangeController(buildcfg, imageStream, image) bcInstantiator := controller.BuildConfigInstantiator.(*buildConfigInstantiator) - bcUpdater := controller.BuildConfigUpdater.(*mockBuildConfigUpdater) + bcUpdater := bcInstantiator.buildConfigUpdater err := controller.HandleImageRepo(imageStream) if err != nil { @@ -166,7 +168,7 @@ func TestSameStreamNameDifferentNamespaces(t *testing.T) { image := mockImage("testImage@id", "registry.com/namespace/imagename@id") controller := mockImageChangeController(buildcfg, imageStream, image) bcInstantiator := controller.BuildConfigInstantiator.(*buildConfigInstantiator) - bcUpdater := controller.BuildConfigUpdater.(*mockBuildConfigUpdater) + bcUpdater := bcInstantiator.buildConfigUpdater err := controller.HandleImageRepo(imageStream) if err != nil { @@ -188,7 +190,7 @@ func TestBuildConfigWithDifferentTriggerType(t *testing.T) { image := mockImage("testImage@id", "registry.com/namespace/imagename@id") controller := mockImageChangeController(buildcfg, imageStream, image) bcInstantiator := controller.BuildConfigInstantiator.(*buildConfigInstantiator) - bcUpdater := controller.BuildConfigUpdater.(*mockBuildConfigUpdater) + bcUpdater := bcInstantiator.buildConfigUpdater err := controller.HandleImageRepo(imageStream) if err != nil { @@ -211,7 +213,7 @@ func TestNoImageIDChange(t *testing.T) { image := mockImage("testImage@id", "registry.com/namespace/imagename@id") controller := mockImageChangeController(buildcfg, imageStream, image) bcInstantiator := controller.BuildConfigInstantiator.(*buildConfigInstantiator) - bcUpdater := controller.BuildConfigUpdater.(*mockBuildConfigUpdater) + bcUpdater := bcInstantiator.buildConfigUpdater err := controller.HandleImageRepo(imageStream) if err != nil { @@ -233,7 +235,7 @@ func TestBuildConfigInstantiatorError(t *testing.T) { controller := mockImageChangeController(buildcfg, imageStream, image) bcInstantiator := controller.BuildConfigInstantiator.(*buildConfigInstantiator) bcInstantiator.err = fmt.Errorf("instantiating error") - bcUpdater := controller.BuildConfigUpdater.(*mockBuildConfigUpdater) + bcUpdater := bcInstantiator.buildConfigUpdater err := controller.HandleImageRepo(imageStream) if err == nil || !strings.Contains(err.Error(), "instantiating error") { @@ -254,9 +256,8 @@ func TestBuildConfigUpdateError(t *testing.T) { image := mockImage("testImage@id", "registry.com/namespace/imagename@id") controller := mockImageChangeController(buildcfg, imageStream, image) bcInstantiator := controller.BuildConfigInstantiator.(*buildConfigInstantiator) - bcUpdater := controller.BuildConfigUpdater.(*mockBuildConfigUpdater) - bcUpdater.err = fmt.Errorf("error") - bcUpdater.errUpdateCount = 2 + bcUpdater := bcInstantiator.buildConfigUpdater + bcUpdater.err = kerrors.NewConflict("BuildConfig", buildcfg.Name, errors.New("foo")) err := controller.HandleImageRepo(imageStream) if len(bcInstantiator.name) == 0 { @@ -274,7 +275,7 @@ func TestNewImageIDNoDockerRepo(t *testing.T) { image := mockImage("testImage@id", "registry.com/namespace/imagename@id") controller := mockImageChangeController(buildcfg, imageStream, image) bcInstantiator := controller.BuildConfigInstantiator.(*buildConfigInstantiator) - bcUpdater := controller.BuildConfigUpdater.(*mockBuildConfigUpdater) + bcUpdater := bcInstantiator.buildConfigUpdater err := controller.HandleImageRepo(imageStream) if err != nil { @@ -289,21 +290,14 @@ func TestNewImageIDNoDockerRepo(t *testing.T) { } type mockBuildConfigUpdater struct { - updateCount int - buildcfg *buildapi.BuildConfig - err error - errUpdateCount int + updateCount int + buildcfg *buildapi.BuildConfig + err error } func (m *mockBuildConfigUpdater) Update(buildcfg *buildapi.BuildConfig) error { m.buildcfg = buildcfg m.updateCount++ - if m.errUpdateCount > 0 { - if m.updateCount == m.errUpdateCount { - return m.err - } - return nil - } return m.err } @@ -366,10 +360,11 @@ func mockImage(name, dockerSpec string) *imageapi.Image { } type buildConfigInstantiator struct { - generator buildgenerator.BuildGenerator - name string - newBuild *buildapi.Build - err error + generator buildgenerator.BuildGenerator + buildConfigUpdater *mockBuildConfigUpdater + name string + newBuild *buildapi.Build + err error } func (i *buildConfigInstantiator) Instantiate(namespace string, request *buildapi.BuildRequest) (*buildapi.Build, error) { @@ -383,6 +378,7 @@ func mockBuildConfigInstantiator(buildcfg *buildapi.BuildConfig, imageStream *im Secrets: []kapi.ObjectReference{}, } instantiator := &buildConfigInstantiator{} + instantiator.buildConfigUpdater = &mockBuildConfigUpdater{} generator := buildgenerator.BuildGenerator{ Secrets: testclient.NewSimpleFake(), ServiceAccounts: testclient.NewSimpleFake(&builderAccount), @@ -391,7 +387,7 @@ func mockBuildConfigInstantiator(buildcfg *buildapi.BuildConfig, imageStream *im return buildcfg, nil }, UpdateBuildConfigFunc: func(ctx kapi.Context, buildConfig *buildapi.BuildConfig) error { - return nil + return instantiator.buildConfigUpdater.Update(buildConfig) }, CreateBuildFunc: func(ctx kapi.Context, build *buildapi.Build) error { instantiator.newBuild = build @@ -418,6 +414,5 @@ func mockImageChangeController(buildcfg *buildapi.BuildConfig, imageStream *imag return &ImageChangeController{ BuildConfigStore: buildtest.NewFakeBuildConfigStore(buildcfg), BuildConfigInstantiator: mockBuildConfigInstantiator(buildcfg, imageStream, image), - BuildConfigUpdater: &mockBuildConfigUpdater{}, } } diff --git a/pkg/build/generator/generator.go b/pkg/build/generator/generator.go index 322256f71297..5b49553b86c9 100644 --- a/pkg/build/generator/generator.go +++ b/pkg/build/generator/generator.go @@ -127,8 +127,29 @@ func (g *BuildGenerator) Instantiate(ctx kapi.Context, request *buildapi.BuildRe if err != nil { return nil, err } + glog.V(4).Infof("Build %s/%s has been generated from %s/%s BuildConfig", newBuild.Namespace, newBuild.ObjectMeta.Name, bc.Namespace, bc.ObjectMeta.Name) - return g.createBuild(ctx, newBuild) + updatedBuild, err := g.createBuild(ctx, newBuild) + if err != nil { + return nil, err + } + + if len(request.Image) > 0 { + for _, trigger := range bc.Triggers { + if trigger.Type != buildapi.ImageChangeBuildTriggerType { + continue + } + + trigger.ImageChange.LastTriggeredImageID = request.Image + } + } + + // need to update the BuildConfig because LastVersion and possibly LastTriggeredImageID changed + if err := g.Client.UpdateBuildConfig(ctx, bc); err != nil { + return nil, err + } + + return updatedBuild, nil } // Clone returns clone of a Build @@ -162,10 +183,6 @@ func (g *BuildGenerator) createBuild(ctx kapi.Context, build *buildapi.Build) (* // the Strategy, or uses the Image field of the Strategy. // Takes a BuildConfig to base the build on, and an optional SourceRevision to build. func (g *BuildGenerator) generateBuildFromConfig(ctx kapi.Context, bc *buildapi.BuildConfig, revision *buildapi.SourceRevision) (*buildapi.Build, error) { - builderSecrets, err := g.FetchServiceAccountSecrets(bc.Namespace) - if err != nil { - return nil, err - } // Need to copy the buildConfig here so that it doesn't share pointers with // the build object which could be (will be) modified later. obj, _ := kapi.Scheme.Copy(bc) @@ -187,14 +204,15 @@ func (g *BuildGenerator) generateBuildFromConfig(ctx kapi.Context, bc *buildapi. build.Config = &kapi.ObjectReference{Kind: "BuildConfig", Name: bc.Name, Namespace: bc.Namespace} build.Name = getNextBuildName(bc) - if err := g.Client.UpdateBuildConfig(ctx, bc); err != nil { - return nil, err - } if build.Labels == nil { build.Labels = make(map[string]string) } build.Labels[buildapi.BuildConfigLabel] = bcCopy.Name + builderSecrets, err := g.FetchServiceAccountSecrets(bc.Namespace) + if err != nil { + return nil, err + } if build.Parameters.Output.PushSecret == nil { build.Parameters.Output.PushSecret = g.resolveImageSecret(ctx, builderSecrets, build.Parameters.Output.To, bc.Namespace) } diff --git a/pkg/build/generator/generator_test.go b/pkg/build/generator/generator_test.go index cd867da95e4f..d2d759dae5e7 100644 --- a/pkg/build/generator/generator_test.go +++ b/pkg/build/generator/generator_test.go @@ -57,6 +57,14 @@ func TestInstantiate(t *testing.T) { } } +// agoldste: I'm not sure the intent of this test. Using the previous logic for +// the generator, which would try to update the build config before creating +// the build, I can see why the UpdateBuildConfigFunc is set up to return an +// error, but nothing is checking the value of instantiationCalls. We could +// update this test to fail sooner, when the build is created, but that's +// already handled by TestCreateBuildCreateError. We may just want to delete +// this test. +/* func TestInstantiateRetry(t *testing.T) { instantiationCalls := 0 fakeSecrets := []runtime.Object{} @@ -80,8 +88,8 @@ func TestInstantiateRetry(t *testing.T) { if err == nil || !strings.Contains(err.Error(), "update-error") { t.Errorf("Expected update-error, got different %v", err) } - } +*/ func TestInstantiateGetBuildConfigError(t *testing.T) { generator := BuildGenerator{Client: Client{ diff --git a/pkg/cmd/server/origin/master.go b/pkg/cmd/server/origin/master.go index 7b7ca799b859..49c08c42ba76 100644 --- a/pkg/cmd/server/origin/master.go +++ b/pkg/cmd/server/origin/master.go @@ -859,9 +859,8 @@ func (c *MasterConfig) RunBuildPodController() { // RunBuildImageChangeTriggerController starts the build image change trigger controller process. func (c *MasterConfig) RunBuildImageChangeTriggerController() { bcClient, _ := c.BuildControllerClients() - bcUpdater := buildclient.NewOSClientBuildConfigClient(bcClient) bcInstantiator := buildclient.NewOSClientBuildConfigInstantiatorClient(bcClient) - factory := buildcontrollerfactory.ImageChangeControllerFactory{Client: bcClient, BuildConfigInstantiator: bcInstantiator, BuildConfigUpdater: bcUpdater} + factory := buildcontrollerfactory.ImageChangeControllerFactory{Client: bcClient, BuildConfigInstantiator: bcInstantiator} factory.Create().Run() } From 95f33fe76013533fa65261a306c7b973079bcac2 Mon Sep 17 00:00:00 2001 From: Andy Goldstein Date: Thu, 28 May 2015 16:32:38 -0400 Subject: [PATCH 2/2] Fix code review comments --- pkg/build/generator/generator.go | 20 ++++++++++++-------- 1 file changed, 12 insertions(+), 8 deletions(-) diff --git a/pkg/build/generator/generator.go b/pkg/build/generator/generator.go index 5b49553b86c9..9805a5f26d5c 100644 --- a/pkg/build/generator/generator.go +++ b/pkg/build/generator/generator.go @@ -325,7 +325,16 @@ func (g *BuildGenerator) resolveImageSecret(ctx kapi.Context, secrets []kapi.Sec if len(secrets) == 0 || imageRef == nil { return nil } + emptyKeyring := credentialprovider.BasicDockerKeyring{} + + // Get the image pull spec from the image stream reference + imageSpec, err := g.resolveImageStreamReference(ctx, imageRef, buildNamespace) + if err != nil { + glog.V(2).Infof("Unable to resolve the image name for %s/%s: %v", buildNamespace, imageRef, err) + return nil + } + for _, secret := range secrets { keyring, err := credentialprovider.MakeDockerKeyring([]kapi.Secret{secret}, &emptyKeyring) if err != nil { @@ -333,18 +342,13 @@ func (g *BuildGenerator) resolveImageSecret(ctx kapi.Context, secrets []kapi.Sec continue } - // Get the image pull spec from the image stream reference - imageSpec, err := g.resolveImageStreamReference(ctx, imageRef, buildNamespace) - if err != nil { - glog.V(2).Infof("Unable to resolve the image name for %s/%s: %v", buildNamespace, imageRef, err) - continue - } - if _, found := keyring.Lookup(imageSpec); found { return &kapi.LocalObjectReference{Name: secret.Name} } } - glog.V(2).Infof("No secrets found for pushing or pulling the %s %s/%s", imageRef.Kind, buildNamespace, imageRef.Name) + + glog.V(5).Infof("No secrets found for pushing or pulling the %s %s/%s", imageRef.Kind, buildNamespace, imageRef.Name) + return nil }