From d9dcac54dd2ddb643c30e9e4a1e2ba83064c7212 Mon Sep 17 00:00:00 2001 From: Tomas Nozicka Date: Tue, 9 Jan 2018 08:31:54 +0100 Subject: [PATCH 1/3] Add test to verify we don't deploy unspecified images in DC --- test/extended/deployments/deployments.go | 93 ++++++++++++++++++- test/extended/testdata/bindata.go | 59 ++++++------ .../deployments/deployment-trigger.yaml | 59 ++++++------ 3 files changed, 146 insertions(+), 65 deletions(-) diff --git a/test/extended/deployments/deployments.go b/test/extended/deployments/deployments.go index fa47516d7108..d553c0a42c4b 100644 --- a/test/extended/deployments/deployments.go +++ b/test/extended/deployments/deployments.go @@ -237,12 +237,22 @@ var _ = g.Describe("[Feature:DeploymentConfig] deploymentconfigs", func() { } firstDeploymentName := appsutil.DeploymentNameForConfigVersion(name, 1) - firstDeployerRemoved := len(deploymentNamesToDeployers[firstDeploymentName]) == 0 + firstDeployerRemoved := true + for _, deployer := range deploymentNamesToDeployers[firstDeploymentName] { + if deployer.Status.Phase != kapiv1.PodFailed && deployer.Status.Phase != kapiv1.PodSucceeded { + firstDeployerRemoved = false + } + } secondDeploymentName := appsutil.DeploymentNameForConfigVersion(name, 2) - secondDeployerExists := len(deploymentNamesToDeployers[secondDeploymentName]) == 1 + secondDeployerRemoved := true + for _, deployer := range deploymentNamesToDeployers[secondDeploymentName] { + if deployer.Status.Phase != kapiv1.PodFailed && deployer.Status.Phase != kapiv1.PodSucceeded { + secondDeployerRemoved = false + } + } - return firstDeployerRemoved && secondDeployerExists, nil + return firstDeployerRemoved && !secondDeployerRemoved, nil }) o.Expect(err).NotTo(o.HaveOccurred()) }) @@ -1368,4 +1378,81 @@ var _ = g.Describe("[Feature:DeploymentConfig] deploymentconfigs", func() { o.Expect(err).NotTo(o.HaveOccurred()) }) }) + + g.Describe("won't deploy RC with unresolved images [Conformance]", func() { + dcName := "example" + rcName := func(i int) string { return fmt.Sprintf("%s-%d", dcName, i) } + g.AfterEach(func() { + failureTrap(oc, dcName, g.CurrentGinkgoTestDescription().Failed) + }) + + g.It("when patched with empty image", func() { + namespace := oc.Namespace() + + g.By("creating DC") + dc, err := readDCFixture(imageChangeTriggerFixture) + o.Expect(err).NotTo(o.HaveOccurred()) + o.Expect(dc.Name).To(o.Equal(dcName)) + + rcList, err := oc.KubeClient().CoreV1().ReplicationControllers(namespace).List(metav1.ListOptions{}) + o.Expect(err).NotTo(o.HaveOccurred()) + + dc.Spec.Replicas = 1 + dc, err = oc.AppsClient().Apps().DeploymentConfigs(namespace).Create(dc) + o.Expect(err).NotTo(o.HaveOccurred()) + + g.By("tagging the busybox:latest as test:v1 image to create ImageStream") + out, err := oc.Run("tag").Args("docker.io/busybox:latest", "test:v1").Output() + e2e.Logf("%s", out) + o.Expect(err).NotTo(o.HaveOccurred()) + + g.By("waiting for deployment #1 to complete") + _, err = waitForRCModification(oc, namespace, rcName(1), deploymentRunTimeout, + rcList.ResourceVersion, func(currentRC *kapiv1.ReplicationController) (bool, error) { + switch appsutil.DeploymentStatusFor(currentRC) { + case appsapi.DeploymentStatusComplete: + return true, nil + case appsapi.DeploymentStatusFailed: + return true, fmt.Errorf("deployment #1 failed") + default: + return false, nil + } + }) + o.Expect(err).NotTo(o.HaveOccurred()) + + g.By("setting DC image repeatedly to empty string to fight with image trigger") + for i := 0; i < 50; i++ { + dc, err = oc.AppsClient().Apps().DeploymentConfigs(namespace).Patch(dc.Name, types.StrategicMergePatchType, + []byte(`{"spec":{"template":{"spec":{"containers":[{"name":"test","image":""}]}}}}`)) + o.Expect(err).NotTo(o.HaveOccurred()) + } + + g.By("waiting to see if it won't deploy RC with invalid revision or the same one multiple times") + // Wait for image trigger to inject image + dc, err = waitForDCModification(oc, namespace, dc.Name, deploymentChangeTimeout, + dc.GetResourceVersion(), func(config *appsapi.DeploymentConfig) (bool, error) { + if config.Spec.Template.Spec.Containers[0].Image != "" { + return true, nil + } + return false, nil + }) + o.Expect(err).NotTo(o.HaveOccurred()) + + dcTmp, err := waitForDCModification(oc, namespace, dc.Name, deploymentChangeTimeout, + dc.GetResourceVersion(), func(config *appsapi.DeploymentConfig) (bool, error) { + if config.Status.ObservedGeneration >= dc.Generation { + return true, nil + } + return false, nil + }) + o.Expect(err).NotTo(o.HaveOccurred(), fmt.Sprintf("failed to wait on generation >= %d to be observed by DC %s/%s", dc.Generation, dc.Namespace, dc.Name)) + dc = dcTmp + + rcs, err := oc.KubeClient().CoreV1().ReplicationControllers(namespace).List(metav1.ListOptions{ + LabelSelector: appsutil.ConfigSelector(dc.Name).String(), + }) + o.Expect(rcs.Items).To(o.HaveLen(1)) + o.Expect(strings.TrimSpace(rcs.Items[0].Spec.Template.Spec.Containers[0].Image)).NotTo(o.BeEmpty()) + }) + }) }) diff --git a/test/extended/testdata/bindata.go b/test/extended/testdata/bindata.go index deedd91160a4..5af399658b38 100644 --- a/test/extended/testdata/bindata.go +++ b/test/extended/testdata/bindata.go @@ -4838,37 +4838,34 @@ func testExtendedTestdataDeploymentsDeploymentSimpleYaml() (*asset, error) { } var _testExtendedTestdataDeploymentsDeploymentTriggerYaml = []byte(`apiVersion: v1 -kind: List -items: -- apiVersion: v1 - kind: DeploymentConfig - metadata: - labels: - app: example - name: example - spec: - replicas: 1 - template: - metadata: - labels: - app: example - spec: - containers: - - imagePullPolicy: Always - name: test - command: - - /bin/sleep - - "100" - test: false - triggers: - - imageChangeParams: - automatic: true - containerNames: - - test - from: - kind: ImageStreamTag - name: test:v1 - type: ImageChange +kind: DeploymentConfig +metadata: + labels: + app: example + name: example +spec: + replicas: 1 + template: + metadata: + labels: + app: example + spec: + containers: + - imagePullPolicy: Always + name: test + command: + - /bin/sleep + - "100" + test: false + triggers: + - imageChangeParams: + automatic: true + containerNames: + - test + from: + kind: ImageStreamTag + name: test:v1 + type: ImageChange `) func testExtendedTestdataDeploymentsDeploymentTriggerYamlBytes() ([]byte, error) { diff --git a/test/extended/testdata/deployments/deployment-trigger.yaml b/test/extended/testdata/deployments/deployment-trigger.yaml index 5767bea02245..e59df35e92a9 100644 --- a/test/extended/testdata/deployments/deployment-trigger.yaml +++ b/test/extended/testdata/deployments/deployment-trigger.yaml @@ -1,32 +1,29 @@ apiVersion: v1 -kind: List -items: -- apiVersion: v1 - kind: DeploymentConfig - metadata: - labels: - app: example - name: example - spec: - replicas: 1 - template: - metadata: - labels: - app: example - spec: - containers: - - imagePullPolicy: Always - name: test - command: - - /bin/sleep - - "100" - test: false - triggers: - - imageChangeParams: - automatic: true - containerNames: - - test - from: - kind: ImageStreamTag - name: test:v1 - type: ImageChange +kind: DeploymentConfig +metadata: + labels: + app: example + name: example +spec: + replicas: 1 + template: + metadata: + labels: + app: example + spec: + containers: + - imagePullPolicy: Always + name: test + command: + - /bin/sleep + - "100" + test: false + triggers: + - imageChangeParams: + automatic: true + containerNames: + - test + from: + kind: ImageStreamTag + name: test:v1 + type: ImageChange From f0913b262a96968e2f71027f42eb812e3ceab221 Mon Sep 17 00:00:00 2001 From: Tomas Nozicka Date: Thu, 11 Jan 2018 14:49:43 +0100 Subject: [PATCH 2/3] Never deploy unspecified images in DC --- .../deploymentconfig_controller.go | 113 +++++++++++------- .../deploymentconfig_controller_test.go | 33 ++++- test/extended/deployments/deployments.go | 18 ++- 3 files changed, 118 insertions(+), 46 deletions(-) diff --git a/pkg/apps/controller/deploymentconfig/deploymentconfig_controller.go b/pkg/apps/controller/deploymentconfig/deploymentconfig_controller.go index f92a5a027d3e..2a5e580d63c2 100644 --- a/pkg/apps/controller/deploymentconfig/deploymentconfig_controller.go +++ b/pkg/apps/controller/deploymentconfig/deploymentconfig_controller.go @@ -87,7 +87,7 @@ func (c *DeploymentConfigController) Handle(config *appsapi.DeploymentConfig) er glog.V(5).Infof("Reconciling %s/%s", config.Namespace, config.Name) // There's nothing to reconcile until the version is nonzero. if appsutil.IsInitialDeployment(config) && !appsutil.HasTrigger(config) { - return c.updateStatus(config, []*v1.ReplicationController{}) + return c.updateStatus(config, []*v1.ReplicationController{}, true) } // List all ReplicationControllers to find also those we own but that no longer match our selector. @@ -118,7 +118,18 @@ func (c *DeploymentConfigController) Handle(config *appsapi.DeploymentConfig) er // the latest available information. Some deletions make take some time to complete so there // is value in doing this. if config.DeletionTimestamp != nil { - return c.updateStatus(config, existingDeployments) + return c.updateStatus(config, existingDeployments, true) + } + + // If the config is paused we shouldn't create new deployments for it. + if config.Spec.Paused { + // in order for revision history limit cleanup to work for paused + // deployments, we need to trigger it here + if err := c.cleanupOldDeployments(existingDeployments, config); err != nil { + c.recorder.Eventf(config, v1.EventTypeWarning, "DeploymentCleanupFailed", "Couldn't clean up deployments: %v", err) + } + + return c.updateStatus(config, existingDeployments, true) } latestExists, latestDeployment := appsutil.LatestDeploymentInfo(config, existingDeployments) @@ -128,38 +139,52 @@ func (c *DeploymentConfigController) Handle(config *appsapi.DeploymentConfig) er return err } } + + // Never deploy with invalid or unresolved images + for i, container := range config.Spec.Template.Spec.Containers { + if len(strings.TrimSpace(container.Image)) == 0 { + glog.V(4).Infof("Postponing rollout #%d for DeploymentConfig %s/%s because of invalid or unresolved image for container #%d (name=%s)", config.Status.LatestVersion, config.Namespace, config.Name, i, container.Name) + return c.updateStatus(config, existingDeployments, true) + } + } + configCopy := config.DeepCopy() // Process triggers and start an initial rollouts - shouldTrigger, shouldSkip := triggerActivated(configCopy, latestExists, latestDeployment, c.codec) - if !shouldSkip && shouldTrigger { - configCopy.Status.LatestVersion++ - return c.updateStatus(configCopy, existingDeployments) + shouldTrigger, shouldSkip, err := triggerActivated(configCopy, latestExists, latestDeployment, c.codec) + if err != nil { + return fmt.Errorf("triggerActivated failed: %v", err) } - // Have to wait for the image trigger to get the image before proceeding. - if shouldSkip && appsutil.IsInitialDeployment(config) { - return c.updateStatus(configCopy, existingDeployments) + + if shouldSkip { + return c.updateStatus(configCopy, existingDeployments, true) } + + if shouldTrigger { + configCopy.Status.LatestVersion++ + _, err := c.dn.DeploymentConfigs(configCopy.Namespace).UpdateStatus(configCopy) + return err + } + // If the latest deployment already exists, reconcile existing deployments // and return early. if latestExists { // If the latest deployment is still running, try again later. We don't // want to compete with the deployer. if !appsutil.IsTerminatedDeployment(latestDeployment) { - return c.updateStatus(config, existingDeployments) + return c.updateStatus(config, existingDeployments, false) } return c.reconcileDeployments(existingDeployments, config, cm) } - // If the config is paused we shouldn't create new deployments for it. - if config.Spec.Paused { - // in order for revision history limit cleanup to work for paused - // deployments, we need to trigger it here - if err := c.cleanupOldDeployments(existingDeployments, config); err != nil { - c.recorder.Eventf(config, v1.EventTypeWarning, "DeploymentCleanupFailed", "Couldn't clean up deployments: %v", err) - } - return c.updateStatus(config, existingDeployments) + // Never deploy with invalid or unresolved images + for i, container := range config.Spec.Template.Spec.Containers { + if len(strings.TrimSpace(container.Image)) == 0 { + glog.V(4).Infof("Postponing rollout #%d for DeploymentConfig %s/%s because of invalid or unresolved image for container #%d (name=%s)", config.Status.LatestVersion, config.Namespace, config.Name, i, container.Name) + return c.updateStatus(config, existingDeployments, true) + } } + // No deployments are running and the latest deployment doesn't exist, so // create the new deployment. deployment, err := appsutil.MakeDeploymentV1(config, c.codec) @@ -182,17 +207,17 @@ func (c *DeploymentConfigController) Handle(config *appsapi.DeploymentConfig) er if isOurs { // If the deployment was already created, just move on. The cache could be // stale, or another process could have already handled this update. - return c.updateStatus(config, existingDeployments) + return c.updateStatus(config, existingDeployments, true) } else { - err = fmt.Errorf("replication controller %s already exists and deployment config is not allowed to claim it.", deployment.Name) + err = fmt.Errorf("replication controller %s already exists and deployment config is not allowed to claim it", deployment.Name) c.recorder.Eventf(config, v1.EventTypeWarning, "DeploymentCreationFailed", "Couldn't deploy version %d: %v", config.Status.LatestVersion, err) - return c.updateStatus(config, existingDeployments) + return c.updateStatus(config, existingDeployments, true) } } c.recorder.Eventf(config, v1.EventTypeWarning, "DeploymentCreationFailed", "Couldn't deploy version %d: %s", config.Status.LatestVersion, err) // We don't care about this error since we need to report the create failure. cond := appsutil.NewDeploymentCondition(appsapi.DeploymentProgressing, kapi.ConditionFalse, appsapi.FailedRcCreateReason, err.Error()) - _ = c.updateStatus(config, existingDeployments, *cond) + _ = c.updateStatus(config, existingDeployments, true, *cond) return fmt.Errorf("couldn't create deployment for deployment config %s: %v", appsutil.LabelForDeploymentConfig(config), err) } msg := fmt.Sprintf("Created new replication controller %q for version %d", created.Name, config.Status.LatestVersion) @@ -206,7 +231,7 @@ func (c *DeploymentConfigController) Handle(config *appsapi.DeploymentConfig) er } cond := appsutil.NewDeploymentCondition(appsapi.DeploymentProgressing, kapi.ConditionTrue, appsapi.NewReplicationControllerReason, msg) - return c.updateStatus(config, existingDeployments, *cond) + return c.updateStatus(config, existingDeployments, true, *cond) } // reconcileDeployments reconciles existing deployment replica counts which @@ -285,13 +310,13 @@ func (c *DeploymentConfigController) reconcileDeployments(existingDeployments [] c.recorder.Eventf(config, v1.EventTypeWarning, "ReplicationControllerCleanupFailed", "Couldn't clean up replication controllers: %v", err) } - return c.updateStatus(config, updatedDeployments) + return c.updateStatus(config, updatedDeployments, true) } // Update the status of the provided deployment config. Additional conditions will override any other condition in the // deployment config status. -func (c *DeploymentConfigController) updateStatus(config *appsapi.DeploymentConfig, deployments []*v1.ReplicationController, additional ...appsapi.DeploymentCondition) error { - newStatus := calculateStatus(config, deployments, additional...) +func (c *DeploymentConfigController) updateStatus(config *appsapi.DeploymentConfig, deployments []*v1.ReplicationController, updateObservedGeneration bool, additional ...appsapi.DeploymentCondition) error { + newStatus := calculateStatus(config, deployments, updateObservedGeneration, additional...) // NOTE: We should update the status of the deployment config only if we need to, otherwise // we hotloop between updates. @@ -376,7 +401,7 @@ func (c *DeploymentConfigController) cancelRunningRollouts(config *appsapi.Deplo return nil } -func calculateStatus(config *appsapi.DeploymentConfig, rcs []*v1.ReplicationController, additional ...appsapi.DeploymentCondition) appsapi.DeploymentConfigStatus { +func calculateStatus(config *appsapi.DeploymentConfig, rcs []*v1.ReplicationController, updateObservedGeneration bool, additional ...appsapi.DeploymentCondition) appsapi.DeploymentConfigStatus { // UpdatedReplicas represents the replicas that use the current deployment config template which means // we should inform about the replicas of the latest deployment and not the active. latestReplicas := int32(0) @@ -394,10 +419,15 @@ func calculateStatus(config *appsapi.DeploymentConfig, rcs []*v1.ReplicationCont unavailableReplicas = 0 } + generation := config.Status.ObservedGeneration + if updateObservedGeneration { + generation = config.Generation + } + status := appsapi.DeploymentConfigStatus{ LatestVersion: config.Status.LatestVersion, Details: config.Status.Details, - ObservedGeneration: config.Generation, + ObservedGeneration: generation, Replicas: appsutil.GetStatusReplicaCountForDeployments(rcs), UpdatedReplicas: latestReplicas, AvailableReplicas: available, @@ -519,9 +549,9 @@ func (c *DeploymentConfigController) cleanupOldDeployments(existingDeployments [ // triggers were activated (config change or image change). The first bool indicates that // the triggers are active and second indicates if we should skip the rollout because we // are waiting for the trigger to complete update (waiting for image for example). -func triggerActivated(config *appsapi.DeploymentConfig, latestExists bool, latestDeployment *v1.ReplicationController, codec runtime.Codec) (bool, bool) { +func triggerActivated(config *appsapi.DeploymentConfig, latestExists bool, latestDeployment *v1.ReplicationController, codec runtime.Codec) (bool, bool, error) { if config.Spec.Paused { - return false, false + return false, false, nil } imageTrigger := appsutil.HasImageChangeTrigger(config) configTrigger := appsutil.HasChangeTrigger(config) @@ -529,7 +559,7 @@ func triggerActivated(config *appsapi.DeploymentConfig, latestExists bool, lates // no-op when no triggers are defined. if !hasTrigger { - return false, false + return false, false, nil } // Handle initial rollouts @@ -542,50 +572,49 @@ func triggerActivated(config *appsapi.DeploymentConfig, latestExists bool, lates // TODO: Technically this is not a config change cause, but we will have to report the image that caused the trigger. // In some cases it might be difficult because config can have multiple ICT. appsutil.RecordConfigChangeCause(config) - return true, false + return true, false, nil } glog.V(4).Infof("Rolling out initial deployment for %s/%s deferred until its images are ready", config.Namespace, config.Name) - return false, true + return false, true, nil } // Rollout if we only have config change trigger. if configTrigger { glog.V(4).Infof("Rolling out initial deployment for %s/%s", config.Namespace, config.Name) appsutil.RecordConfigChangeCause(config) - return true, false + return true, false, nil } // We are waiting for the initial RC to be created. - return false, false + return false, false, nil } // Wait for the RC to be created if !latestExists { - return false, true + return false, false, nil } // We need existing deployment at this point to compare its template with current config template. if latestDeployment == nil { - return false, false + return false, false, nil } if imageTrigger { if ok, imageNames := appsutil.HasUpdatedImages(config, latestDeployment); ok { glog.V(4).Infof("Rolling out #%d deployment for %s/%s caused by image changes (%s)", config.Status.LatestVersion+1, config.Namespace, config.Name, strings.Join(imageNames, ",")) appsutil.RecordImageChangeCauses(config, imageNames) - return true, false + return true, false, nil } } if configTrigger { isLatest, changes, err := appsutil.HasLatestPodTemplate(config, latestDeployment, codec) if err != nil { - glog.Errorf("Error while checking for latest pod template in replication controller: %v", err) - return false, true + return false, false, fmt.Errorf("error while checking for latest pod template in replication controller: %v", err) } if !isLatest { glog.V(4).Infof("Rolling out #%d deployment for %s/%s caused by config change, diff: %s", config.Status.LatestVersion+1, config.Namespace, config.Name, changes) appsutil.RecordConfigChangeCause(config) - return true, false + return true, false, nil } } - return false, false + return false, false, nil } diff --git a/pkg/apps/controller/deploymentconfig/deploymentconfig_controller_test.go b/pkg/apps/controller/deploymentconfig/deploymentconfig_controller_test.go index ea8ec73ebb02..0d0f11ed9f42 100644 --- a/pkg/apps/controller/deploymentconfig/deploymentconfig_controller_test.go +++ b/pkg/apps/controller/deploymentconfig/deploymentconfig_controller_test.go @@ -452,6 +452,9 @@ func newInt32(i int32) *int32 { func newDC(version, replicas, maxUnavailable int, cond appsapi.DeploymentCondition) *appsapi.DeploymentConfig { return &appsapi.DeploymentConfig{ + ObjectMeta: metav1.ObjectMeta{ + Generation: 1, + }, Spec: appsapi.DeploymentConfigSpec{ Replicas: int32(replicas), Strategy: appsapi.DeploymentStrategy{ @@ -502,8 +505,9 @@ func TestCalculateStatus(t *testing.T) { tests := []struct { name string - dc *appsapi.DeploymentConfig - rcs []*v1.ReplicationController + dc *appsapi.DeploymentConfig + rcs []*v1.ReplicationController + updateObservedGeneration bool expected appsapi.DeploymentConfigStatus }{ @@ -528,6 +532,29 @@ func TestCalculateStatus(t *testing.T) { }, }, }, + { + name: "available deployment with updating observedGeneration", + + dc: newDC(3, 3, 1, availableCond), + rcs: []*v1.ReplicationController{ + newRC(3, 2, 2, 1, 1), + newRC(2, 0, 0, 0, 0), + newRC(1, 0, 1, 1, 1), + }, + updateObservedGeneration: true, + + expected: appsapi.DeploymentConfigStatus{ + LatestVersion: int64(3), + ObservedGeneration: int64(1), + Replicas: int32(3), + ReadyReplicas: int32(2), + AvailableReplicas: int32(2), + UpdatedReplicas: int32(2), + Conditions: []appsapi.DeploymentCondition{ + availableCond, + }, + }, + }, { name: "unavailable deployment", @@ -552,7 +579,7 @@ func TestCalculateStatus(t *testing.T) { } for _, test := range tests { - status := calculateStatus(test.dc, test.rcs) + status := calculateStatus(test.dc, test.rcs, test.updateObservedGeneration) if !reflect.DeepEqual(status, test.expected) { t.Errorf("%s: expected status:\n%+v\ngot status:\n%+v", test.name, test.expected, status) } diff --git a/test/extended/deployments/deployments.go b/test/extended/deployments/deployments.go index d553c0a42c4b..593e4566584f 100644 --- a/test/extended/deployments/deployments.go +++ b/test/extended/deployments/deployments.go @@ -700,13 +700,15 @@ var _ = g.Describe("[Feature:DeploymentConfig] deploymentconfigs", func() { }) g.Describe("paused [Conformance]", func() { + dcName := "paused" g.AfterEach(func() { - failureTrap(oc, "paused", g.CurrentGinkgoTestDescription().Failed) + failureTrap(oc, dcName, g.CurrentGinkgoTestDescription().Failed) }) g.It("should disable actions on deployments", func() { resource, name, err := createFixture(oc, pausedDeploymentFixture) o.Expect(err).NotTo(o.HaveOccurred()) + o.Expect(name).To(o.Equal(dcName)) _, rcs, _, err := deploymentInfo(oc, name) o.Expect(err).NotTo(o.HaveOccurred()) @@ -756,6 +758,20 @@ var _ = g.Describe("[Feature:DeploymentConfig] deploymentconfigs", func() { }) o.Expect(err).NotTo(o.HaveOccurred()) o.Expect(waitForLatestCondition(oc, name, deploymentRunTimeout, deploymentReachedCompletion)).NotTo(o.HaveOccurred()) + + g.By("making sure it updates observedGeneration after being paused") + dc, err := oc.AppsClient().Apps().DeploymentConfigs(oc.Namespace()).Patch(dcName, + types.StrategicMergePatchType, []byte(`{"spec": {"paused": true}}`)) + o.Expect(err).NotTo(o.HaveOccurred()) + + _, err = waitForDCModification(oc, dc.Namespace, dc.Name, deploymentChangeTimeout, + dc.GetResourceVersion(), func(config *appsapi.DeploymentConfig) (bool, error) { + if config.Status.ObservedGeneration >= dc.Generation { + return true, nil + } + return false, nil + }) + o.Expect(err).NotTo(o.HaveOccurred(), fmt.Sprintf("failed to wait on generation >= %d to be observed by DC %s/%s", dc.Generation, dc.Namespace, dc.Name)) }) }) From cda584a51039693a239cab1f73711423fdb2f88b Mon Sep 17 00:00:00 2001 From: Tomas Nozicka Date: Tue, 5 Dec 2017 11:13:48 +0100 Subject: [PATCH 3/3] Fix DC image reactor to reconcile on DC dc.Spec.Template.Spec.Containers changes --- .../trigger/deploymentconfigs/deploymentconfigs.go | 11 +++++++---- 1 file changed, 7 insertions(+), 4 deletions(-) diff --git a/pkg/image/trigger/deploymentconfigs/deploymentconfigs.go b/pkg/image/trigger/deploymentconfigs/deploymentconfigs.go index b38b0991d3c4..98c2d1fb5b8a 100644 --- a/pkg/image/trigger/deploymentconfigs/deploymentconfigs.go +++ b/pkg/image/trigger/deploymentconfigs/deploymentconfigs.go @@ -128,13 +128,19 @@ func (i deploymentConfigTriggerIndexer) Index(obj, old interface{}) (string, *tr default: // updated dc = obj.(*appsapi.DeploymentConfig) + oldDC := old.(*appsapi.DeploymentConfig) triggers = calculateDeploymentConfigTriggers(dc) - oldTriggers := calculateDeploymentConfigTriggers(old.(*appsapi.DeploymentConfig)) + oldTriggers := calculateDeploymentConfigTriggers(oldDC) switch { case len(oldTriggers) == 0: change = cache.Added case !reflect.DeepEqual(oldTriggers, triggers): change = cache.Updated + // We need to react on image changes as well. Image names could change, + // images could be set to different value or resetted to "" e.g. by oc apply + // and we need to make sure those changes get reconciled by re-resolving images + case !reflect.DeepEqual(dc.Spec.Template.Spec.Containers, oldDC.Spec.Template.Spec.Containers): + change = cache.Updated } } @@ -187,9 +193,6 @@ func UpdateDeploymentConfigImages(dc *appsapi.DeploymentConfig, tagRetriever tri glog.V(4).Infof("trigger %#v in deployment %s is not resolveable", p, dc.Name) return nil, false, nil } - if ref == p.LastTriggeredImage { - continue - } if len(ref) == 0 { ref = p.LastTriggeredImage