First part of improved rolling update, allow dynamic next replication controller generation. #7268

brendandburns · 2015-04-23T23:28:20Z

@ghodss @jlowdermilk @bgrant0607

brendandburns · 2015-04-23T23:28:29Z

@deads2k @smarterclayton for good measure.

davidopp · 2015-04-24T00:04:53Z

Hey @brendandburns can you recommend a reviewer for me to assign this to?

j3ffml · 2015-04-24T00:34:58Z

pkg/kubectl/cmd/rollingupdate.go

Is this TODO still relevant?

Seems like the answer is "no"

j3ffml · 2015-04-24T00:50:21Z

Generally lgtm. Should run the e2e kubectl test to make sure existing behavior still works. Also need a test for the generation path; unit would be fine, though e2e is probably easiest (see test/e2e/kubectl.go).

brendandburns · 2015-04-24T04:17:28Z

Comments addressed (and PR expanded somewhat both to fix a bug, and add a --dry-run flag)

If this looks good enough to merge, I'll add some testing.

deads2k · 2015-04-24T11:46:43Z

pkg/kubectl/cmd/rollingupdate.go

Can you pull the static validation into a separate method?

brendandburns · 2015-04-24T18:26:37Z

Comments addressed, please re-check.

Thanks!
--brendan

deads2k · 2015-04-24T18:33:15Z

pkg/kubectl/cmd/rollingupdate.go

static validation.

deads2k · 2015-04-24T18:42:01Z

Just a couple nits. lgtm.

bgrant0607 · 2015-04-24T20:04:36Z

Sorry for the delay. Taking a look now.

bgrant0607 · 2015-04-24T20:14:04Z

cc @ironcladlou

brendandburns · 2015-04-24T20:41:14Z

Comments addressed, and initial unit test added. Please take another look.

Thanks!
--brendan

bgrant0607 · 2015-04-24T21:00:51Z

pkg/kubectl/cmd/rollingupdate.go

Replicas will just be set back to oldRc.Spec.Replicas, below, so this seems pointless.

bgrant0607 · 2015-04-24T21:44:18Z

pkg/kubectl/cmd/rollingupdate.go

Ideally, this would use the label update command, which we'd factor out into a library package, at least for the pod updates. We also need to update the selector on the RC, but there's not currently a generic way to do that (though I suspect we'll need one), so I'm more sympathetic to custom code for that.

brendandburns · 2015-04-24T21:44:42Z

Comment addressed. Added additional tests.

Please take another look.

--brendan

bgrant0607 · 2015-04-24T21:45:48Z

pkg/kubectl/cmd/rollingupdate.go

This is racy. Really, the RC needs to be updated first to add the label to its template, then the pods need to be updated, then the selector needs to be changed. Or, delete the RC before updating pods and recreate it after.

What you propose is actually worse, because the minute you update the RC, it orphans the old pods, which causes it to notice that all pods are missing, freak out and creates N pods, so you end up with N orphans, and N new pods. Even if you delete the orphans, it's going to cause a bunch of restart churn in your system.

Pro-actively labeling, updating the pods, and then deleting any orphans that might sneak in, is the best you can do, I think.

No, adding a new label key to the template doesn't orphan pods. That's why the selector update should be a separate step.

ah, I see your point wrt to splitting the updates. Will send a PR to do that.

--brendan

bgrant0607 · 2015-04-24T21:59:11Z

This is more or less ok. Just some implementation details to iron out.

… controller generation.

brendandburns · 2015-04-24T22:20:06Z

Comments addressed (or at least responded to) ptal.

Thanks
--brendan

bgrant0607 · 2015-04-24T23:11:03Z

LGTM

First part of improved rolling update, allow dynamic next replication controller generation.

antoineco · 2015-05-01T20:17:23Z

Wow these changes are very interesting, however things do not work as expected for me:

$ kubectl run-container redis --image=redis:2.8.8 --replicas=4
CONTROLLER   CONTAINER(S)   IMAGE(S)      SELECTOR              REPLICAS
redis        redis          redis:2.8.8   run-container=redis   4

$ kubectl get pods -o template --template="{{range.items}}{{.metadata.name}} {{.status.phase}} - {{end}}"
redis-5k4tk Running - redis-6nh1m Running - redis-j3pjd Running - redis-j7w8q Running

$ kubectl rolling-update redis redis289 --image=redis:2.8.9 --v=4
I0501 20:16:53.627137    1746 helpers.go:52] server response object: [{
  "metadata": {},
  "status": "Failure",
  "message": "replicationControllers \"redis\" cannot be updated: the resource was updated to 1925",
  "reason": "Conflict",
  "details": {
    "id": "redis",
    "kind": "replicationControllers"
  },
  "code": 409
}]
F0501 20:16:53.627725    1746 rollingupdate.go:73] Error from server: replicationControllers "redis" cannot be updated: the resource was updated to 1925

Is there anything I didn't grasp?

bgrant0607 · 2015-05-02T00:14:49Z

Fix was merged: #7540

googlebot added the cla: yes label Apr 23, 2015

brendandburns force-pushed the qps branch from 53aaa8e to 0d56ec5 Compare April 23, 2015 23:29

j3ffml reviewed Apr 24, 2015
View reviewed changes

davidopp assigned j3ffml Apr 24, 2015

brendandburns force-pushed the qps branch from 0d56ec5 to cf76024 Compare April 24, 2015 04:16

brendandburns mentioned this pull request Apr 24, 2015

Further implementation of rolling-update, add rename #7279

Merged

deads2k reviewed Apr 24, 2015
View reviewed changes

brendandburns force-pushed the qps branch from cf76024 to 3d1893e Compare April 24, 2015 18:26

deads2k reviewed Apr 24, 2015
View reviewed changes

brendandburns force-pushed the qps branch from 3d1893e to 9b42892 Compare April 24, 2015 20:41

bgrant0607 reviewed Apr 24, 2015
View reviewed changes

brendandburns force-pushed the qps branch from 9b42892 to c09ca2f Compare April 24, 2015 21:42

bgrant0607 reviewed Apr 24, 2015
View reviewed changes

brendandburns force-pushed the qps branch from c09ca2f to e7cdc21 Compare April 24, 2015 21:44

bgrant0607 reviewed Apr 24, 2015
View reviewed changes

First part of improved rolling update, allow dynamic next replication…

217e6ff

… controller generation.

brendandburns force-pushed the qps branch from e7cdc21 to 217e6ff Compare April 24, 2015 22:19

bgrant0607 added a commit that referenced this pull request Apr 24, 2015

Merge pull request #7268 from brendandburns/qps

7330ced

First part of improved rolling update, allow dynamic next replication controller generation.

bgrant0607 merged commit 7330ced into kubernetes:master Apr 24, 2015

j3ffml mentioned this pull request Apr 27, 2015

rolling-update doesn't actually accept a file from stdin #7286

Closed

brendandburns unassigned j3ffml Aug 12, 2015

First part of improved rolling update, allow dynamic next replication controller generation. #7268

First part of improved rolling update, allow dynamic next replication controller generation. #7268

Uh oh!

Conversation

brendandburns commented Apr 23, 2015

Uh oh!

brendandburns commented Apr 23, 2015

Uh oh!

davidopp commented Apr 24, 2015

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

j3ffml commented Apr 24, 2015

Uh oh!

brendandburns commented Apr 24, 2015

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

brendandburns commented Apr 24, 2015

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

deads2k commented Apr 24, 2015

Uh oh!

bgrant0607 commented Apr 24, 2015

Uh oh!

bgrant0607 commented Apr 24, 2015

Uh oh!

brendandburns commented Apr 24, 2015

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!

brendandburns commented Apr 24, 2015

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!

bgrant0607 commented Apr 24, 2015

Uh oh!

brendandburns commented Apr 24, 2015

Uh oh!

bgrant0607 commented Apr 24, 2015

Uh oh!

antoineco commented May 1, 2015

Uh oh!

bgrant0607 commented May 2, 2015

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants