Retry conflict when updating RC in DC controller faster by mfojtik · Pull Request #10507 · openshift/origin

mfojtik · 2016-08-18T14:57:06Z

No description provided.

mfojtik · 2016-08-18T14:57:17Z

deads2k · 2016-08-18T14:59:37Z

pkg/deploy/controller/deploymentconfig/controller.go

+				var updatedDeployment *kapi.ReplicationController
+				if err := kclient.RetryOnConflict(wait.Backoff{Steps: 5}, func() error {
+					var err error
+					updatedDeployment, err = c.rn.ReplicationControllers(copied.Namespace).Update(copied)


huh? Doesn't this just ensure that you fail 5 times? If copied's resourceVersion doesn't change, you'll just conflict over and over.

Correct - @mfojtik you need to pull and apply the annotations on top

hups, correct

@deads2k will need Get() func in cache to re-get the rc

@deads2k will need Get() func in cache to re-get the rc

Listers already have a get function.

not RC listers...

On Thu, Aug 18, 2016 at 5:15 PM, David Eads notifications@github.com
wrote:

In pkg/deploy/controller/deploymentconfig/controller.go
#10507 (comment):

@@ -118,8 +119,13 @@ func (c *DeploymentConfigController) Handle(config *deployapi.DeploymentConfig)
copied.Annotations[deployapi.DeploymentCancelledAnnotation] = deployapi.DeploymentCancelledAnnotationValue
copied.Annotations[deployapi.DeploymentStatusReasonAnnotation] = deployapi.DeploymentCancelledNewerDeploymentExists

updatedDeployment, err := c.rn.ReplicationControllers(copied.Namespace).Update(copied)

if err != nil {

// Retry faster on conflicts

var updatedDeployment *kapi.ReplicationController

if err := kclient.RetryOnConflict(wait.Backoff{Steps: 5}, func() error {

var err error

updatedDeployment, err = c.rn.ReplicationControllers(copied.Namespace).Update(copied)

@deads2k https://github.com/deads2k will need Get() func in cache to
re-get the rc

Listers already have a get function.

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
https://github.com/openshift/origin/pull/10507/files/095b81adcf1aeb4f3f928f95f9e5fc1ebf91588f#r75327857,
or mute the thread
https://github.com/notifications/unsubscribe-auth/AACsaONbQSMzSUok8fR-5utg6oF7wixiks5qhHb-gaJpZM4JnlAW
.

Michal Fojtik <mi@mifo.sk mfojtik@mifo.sk>
Red Hat OpenShift, Engineering

upstream pull that adds it to all the upstream listers plz:)

On Thu, Aug 18, 2016 at 5:15 PM, Michal Fojtik notifications@github.com
wrote:

In pkg/deploy/controller/deploymentconfig/controller.go
#10507 (comment):

@@ -118,8 +119,13 @@ func (c *DeploymentConfigController) Handle(config *deployapi.DeploymentConfig)
copied.Annotations[deployapi.DeploymentCancelledAnnotation] = deployapi.DeploymentCancelledAnnotationValue
copied.Annotations[deployapi.DeploymentStatusReasonAnnotation] = deployapi.DeploymentCancelledNewerDeploymentExists

updatedDeployment, err := c.rn.ReplicationControllers(copied.Namespace).Update(copied)

if err != nil {

// Retry faster on conflicts

var updatedDeployment *kapi.ReplicationController

if err := kclient.RetryOnConflict(wait.Backoff{Steps: 5}, func() error {

var err error

updatedDeployment, err = c.rn.ReplicationControllers(copied.Namespace).Update(copied)

not RC listers...
… <#m_-8974691536388745868_>
On Thu, Aug 18, 2016 at 5:15 PM, David Eads _@_.***> wrote: In
pkg/deploy/controller/deploymentconfig/controller.go <#10507 (comment)
https://github.com/openshift/origin/pull/10507#discussion_r75327857>: >
@@ -118,8 +119,13 @@ func (c *DeploymentConfigController) Handle(config
*deployapi.DeploymentConfig) > copied.Annotations[deployapi.DeploymentCancelledAnnotation]
= deployapi.DeploymentCancelledAnnotationValue >
copied.Annotations[deployapi.DeploymentStatusReasonAnnotation] =
deployapi.DeploymentCancelledNewerDeploymentExists > > -
updatedDeployment, err := c.rn.ReplicationControllers(
copied.Namespace).Update(copied) > - if err != nil { > + // Retry faster
on conflicts > + var updatedDeployment *kapi.ReplicationController > + if
err := kclient.RetryOnConflict(wait.Backoff{Steps: 5}, func() error { > +
var err error > + updatedDeployment, err = c.rn.ReplicationControllers(
copied.Namespace).Update(copied) @deads2k https://github.com/deads2k <
https://github.com/deads2k> will need Get() func in cache to re-get the
rc Listers already have a get function. — You are receiving this because
you were mentioned. Reply to this email directly, view it on GitHub <
https://github.com/openshift/origin/pull/10507/files/
095b81a#r75327857>, or mute the thread <
https://github.com/notifications/unsubscribe-auth/AACsaONbQSMzSUok8fR-
5utg6oF7wixiks5qhHb-gaJpZM4JnlAW> .
-- ----------------------------------------------- Michal Fojtik <
mi@mifo.sk mfojtik@mifo.sk> Red Hat OpenShift, Engineering

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
https://github.com/openshift/origin/pull/10507/files/095b81adcf1aeb4f3f928f95f9e5fc1ebf91588f#r75328007,
or mute the thread
https://github.com/notifications/unsubscribe-auth/ADuFf2kSJbvmQmB94THvfmDwvUjb2l4eks5qhHcngaJpZM4JnlAW
.

not RC listers...

Figures. Let's do it with an upstream patch. We'll want it upstream too.

@Kargakis 🙉

mfojtik · 2016-08-18T15:23:51Z

@Kargakis re-check before I go add Get() upstream

0xmichalis · 2016-08-18T15:38:03Z

@Kargakis re-check before I go add Get() upstream

It's fine but while you are here, add it in all the listers.

mfojtik · 2016-08-18T16:22:15Z

[test]

@Kargakis upstream PR opened. I added Get() into all listers that have namespace which are only three atm.

0xmichalis · 2016-08-18T16:27:20Z

Fine with me for now

On Thu, Aug 18, 2016 at 6:22 PM, Michal Fojtik notifications@github.com
wrote:

[test]

@Kargakis https://github.com/kargakis upstream PR opened. I added Get()
into all listers that have namespace which are only three atm.

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

eparis · 2016-08-18T16:44:51Z

what is the upstream PR?

mfojtik · 2016-08-18T17:01:49Z

@eparis kubernetes/kubernetes#30896

mfojtik · 2016-08-18T17:55:19Z

@Kargakis that failure is interesting :-/

mfojtik · 2016-08-18T20:28:17Z

[test]

Michal Fojtik

On 18 August 2016 at 22:13:23, OpenShift Bot (notifications@github.com)
wrote:

Evaluated for origin test up to 7dceeed
7dceeed

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

0xmichalis · 2016-08-19T09:49:26Z

pkg/deploy/controller/deploymentconfig/controller.go

-					// replace the current deployment with the updated copy so that a future update has a chance at working
-					existingDeployments[i] = *updatedDeployment
-					c.recorder.Eventf(config, kapi.EventTypeNormal, "DeploymentCancelled", "Cancelled deployment %q superceded by version %d", deployment.Name, config.Status.LatestVersion)
+					if updatedDeployment != nil {


Is there any case this will be nil?

yes, RC was deleted between retry.

0xmichalis · 2016-08-19T09:49:56Z

@Kargakis that failure is interesting :-/

Welcome into the world of updating deployment controller tests

mfojtik · 2016-08-19T09:54:02Z

pkg/deploy/controller/deploymentconfig/controller.go

+					if err != nil {
+						return err
+					}
+					if kapierrors.IsNotFound(err) {


you missed this! :-) this should upper.

mfojtik · 2016-08-23T11:37:10Z

@Kargakis we are green!

I will squash this for merge if you're ready.

0xmichalis · 2016-08-23T11:45:50Z

pkg/deploy/controller/deploymentconfig/controller_test.go

 			delete(deployment.Annotations, deployapi.DesiredReplicasAnnotation)
 		}
 		deployment.Spec.Replicas = d.replicas
+		deployment.Namespace = "test"


No need for this since you have it in the util

mfojtik · 2016-08-23T13:30:13Z

@Kargakis sneaked one more commit here which should fix the ugly event in web console and in the describe output:

  1h        1h      1   {deploymentconfig-controller }          Warning     DeploymentScaleFailed   Failed to scale deployment "ruby-ex-4" from 0 to 0: Operation cannot be fulfilled on replicationcontrollers "ruby-ex-4": the object has been modified; please apply your changes to the latest version and try again

We fast-retry on conflict using the shared cache so we can also scale faster.

mfojtik · 2016-08-23T16:08:49Z

[test]

mfojtik · 2016-08-23T19:36:30Z

@Kargakis ptal, we are green

Michal Fojtik

On 23 August 2016 at 21:03:10, OpenShift Bot (notifications@github.com)
wrote:

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

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

0xmichalis · 2016-08-23T20:03:43Z

pkg/deploy/controller/deploymentconfig/controller.go

-				if err != nil {
+				// Retry faster on conflicts
+				var updatedDeployment *kapi.ReplicationController
+				if err := kclient.RetryOnConflict(wait.Backoff{Steps: 5}, func() error {


kclient.DefaultBackoff

0xmichalis · 2016-08-23T20:08:10Z

Two more comments, LGTM otherwise

mfojtik · 2016-08-24T10:25:05Z

@Kargakis fixed, thanks!

[merge]

openshift-bot · 2016-08-24T10:27:21Z

Evaluated for origin test up to f550e74

openshift-bot · 2016-08-24T10:27:28Z

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

openshift-bot · 2016-08-24T10:27:29Z

Evaluated for origin merge up to f550e74

openshift-bot · 2016-08-24T11:49:34Z

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

deads2k reviewed Aug 18, 2016
View reviewed changes

mfojtik force-pushed the retry-dc-cancel branch from 095b81a to 7694172 Compare August 18, 2016 15:23

mfojtik force-pushed the retry-dc-cancel branch from 7694172 to 2d36b20 Compare August 18, 2016 16:21

mfojtik force-pushed the retry-dc-cancel branch 2 times, most recently from d63d688 to 7dceeed Compare August 18, 2016 18:57

0xmichalis reviewed Aug 19, 2016
View reviewed changes

mfojtik reviewed Aug 19, 2016
View reviewed changes

mfojtik force-pushed the retry-dc-cancel branch 2 times, most recently from e2df1a2 to 8ad6eef Compare August 23, 2016 09:53

0xmichalis reviewed Aug 23, 2016
View reviewed changes

mfojtik force-pushed the retry-dc-cancel branch 2 times, most recently from 127f61a to 9f26be4 Compare August 23, 2016 11:50

0xmichalis reviewed Aug 23, 2016
View reviewed changes

mfojtik added 2 commits August 24, 2016 12:24

UPSTREAM: 30896: Add Get() to ReplicationController lister

bf8247e

deployments: make retries faster on update conflicts

f550e74

mfojtik force-pushed the retry-dc-cancel branch from caa5780 to f550e74 Compare August 24, 2016 10:24

openshift-bot merged commit 22bd662 into openshift:master Aug 24, 2016

0xmichalis mentioned this pull request Aug 26, 2016

deployment config controller doesn't sync, conformance flakes #10229

Closed

mfojtik deleted the retry-dc-cancel branch September 5, 2018 21:08

Conversation

mfojtik commented Aug 18, 2016

Uh oh!

mfojtik commented Aug 18, 2016

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 Aug 18, 2016

Uh oh!

0xmichalis commented Aug 18, 2016

Uh oh!

mfojtik commented Aug 18, 2016

Uh oh!

0xmichalis commented Aug 18, 2016

Uh oh!

eparis commented Aug 18, 2016

Uh oh!

mfojtik commented Aug 18, 2016

Uh oh!

mfojtik commented Aug 18, 2016

Uh oh!

mfojtik commented Aug 18, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

0xmichalis commented Aug 19, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mfojtik commented Aug 23, 2016 • 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!

mfojtik commented Aug 23, 2016

Uh oh!

mfojtik commented Aug 23, 2016

Uh oh!

mfojtik commented Aug 23, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

0xmichalis commented Aug 23, 2016

Uh oh!

mfojtik commented Aug 24, 2016

Uh oh!

openshift-bot commented Aug 24, 2016

Uh oh!

openshift-bot commented Aug 24, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

openshift-bot commented Aug 24, 2016

Uh oh!

openshift-bot commented Aug 24, 2016

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

mfojtik commented Aug 23, 2016 •

edited

Loading

openshift-bot commented Aug 24, 2016 •

edited

Loading