Update RestartPolicy of container by WeiZhang555 · Pull Request #19116 · moby/moby

WeiZhang555 · 2016-01-06T04:13:41Z

Fixes #19025

Add --restart flag for update command, so we can change restart
policy for a running/stopped container.

Signed-off-by: Zhang Wei zhangwei555@huawei.com

thaJeztah · 2016-01-06T10:04:51Z

Thanks @WeiZhang555! I guess we should look at this, together with @vdemeester's proposal for #19104 ?

WeiZhang555 · 2016-01-06T10:44:44Z

@thaJeztah Yes, I also noticed #19104, it hides hostConfig from user and exposes a new API structure, that's good.
I think this PR can refactor based on @vdemeester 's very easily. 😄

thaJeztah · 2016-01-06T10:48:38Z

I think this PR can refactor based on @vdemeester 's very easily. 😄

@WeiZhang555 ah, great!

tianon · 2016-01-06T15:59:27Z

😍 This is the thing I want to change about existing containers most often! (Usually because I was a dummy and forgot to specify one on an important container...)

WeiZhang555 · 2016-01-13T15:16:35Z

Just made some updates to adapt newly introduced UpdateConfig structure.

@calavera Could you help vendor the engine-api? Because it's necessary to add a RestartPolicy into UpdateConfig. See docker-archive-public/docker.engine-api#41

I know I shouldn't add codes to vendor/ package directly, just copy the changes of engine-api here for better review, after vendoring I'll remove that part, also it seems impossible to pass Janky test without vendoring.

Thanks!

WeiZhang555 · 2016-01-16T07:39:38Z

Updated, wait for vendoring of engine-api

icecrime · 2016-01-26T05:57:55Z

I agree it's super useful, I'm just slightly confused:

root@c6f01a629a55:/# docker update --help

Usage:  docker update [OPTIONS] CONTAINER [CONTAINER...]

Update resources of one or more containers

So update is broader then resources after all? (cc @vdemeester @calavera @tiborvass)

tiborvass · 2016-01-26T06:49:56Z

@icecrime the idea was to have update be only about ressources for 1.10, while continuing the discussion about updating restart policy and labels. We should just be unusually conservative with update and make sure we try to think as much as possible about the downsides. If we decide that it's very useful, and has barely any downsides, we should accept it.

vdemeester · 2016-01-26T08:35:12Z

@icecrime @tiborvass said it all 👍

icecrime · 2016-01-26T16:19:38Z

OK, moving to code review I guess 😉

cpuguy83 · 2016-01-30T02:46:41Z

integration-cli/docker_cli_update_unix_test.go

lower case maximumRetryCount, err := please

cpuguy83 · 2016-01-30T02:58:14Z

Do you have a PR into engine-api already?

WeiZhang555 · 2016-01-30T03:04:38Z

@cpuguy83 Yes, I just checked and found that it's already merged and vendored, I'll do a rebase to make use of it.

WeiZhang555 · 2016-01-30T04:05:04Z

Updated.

cpuguy83 · 2016-01-30T14:32:22Z

integration-cli/docker_cli_update_unix_test.go

This one is flakey because the container is not going to be running for a very long time so it may be difficult to catch (And is probably why it's currently failed on gccgo).

Maybe checking just the RestartCount hits 5, like below, and then verify that it is in a stopped state.

Hmm, checking RestartCount hits 5 may bring new problem, if docker update fails to update the RestartPolicy of a running container, it will never hits 5...Besides that, we still need to waitInspect the RestartCount in timeout.

I'll try to give it a larger timeout, maybe 60 seconds? If the container can't stop in 60 seconds, I think some real problem happens.

Yep that's great.

WeiZhang555 · 2016-02-17T11:08:24Z

Ping @thaJeztah @moxiegirl for docs review.

calavera · 2016-02-17T23:40:26Z

The docs should explain what policyCOLONnumber means, or point to a place where we already explain it.

WeiZhang555 · 2016-02-19T02:10:27Z

@calavera OK, I'll change the docs together with @thaJeztah / @moxiegirl 's review result.

thaJeztah · 2016-02-19T13:53:42Z

docs/reference/api/docker_remote_api_v1.23.md

I think "update configuration of one or more containers" ("configuration" instead of "configs")

thaJeztah · 2016-02-19T15:15:22Z

Some nits, but overall looking good.

Can you add a mention to the API changes; https://github.com/docker/docker/blob/master/docs/reference/api/docker_remote_api.md#v123-api-changes ?

Add `--restart` flag for `update` command, so we can change restart policy for a container no matter it's running or stopped. Signed-off-by: Zhang Wei <zhangwei555@huawei.com>

GordonTheTurtle · 2016-02-20T09:09:07Z

Job: Docker-PRs-WoW-TP4 FAILED:

---
0T09:09:03Z" level=error msg="hcsshim::DestroyLayer - Win32 API call returned error r1=0x80070020 err=The process cannot access the file because it is being used by another process.id=CI flavour=0" 
ERROR:  hcsshim::DestroyLayer - Win32 API call returned error r1=0x80070020 err=The process cannot access the file because it is being used by another process.id=CI flavour=0
INFO: End of cleanup
INFO: Ended at Sat Feb 20 09:09:03 CUT 2016 (1m 23s)
Build step 'Execute shell' marked build as failure
[PostBuildScript] - Execution post build scripts.
[docker] $ sh -xe C:\Users\jenkins\hudson8046814883113902440.sh
+ set +e
+ set +x
INFO: Nuking /d/CI
time="2016-02-20T09:09:07Z" level=error msg="hcsshim::DestroyLayer - Win32 API call returned error r1=0x80070020 err=The process cannot access the file because it is being used by another process.id=CI flavour=0" 
ERROR:  hcsshim::DestroyLayer - Win32 API call returned error r1=0x80070020 err=The process cannot access the file because it is being used by another process.id=CI flavour=0
INFO: End
---

WeiZhang555 · 2016-02-20T09:10:24Z

@thaJeztah update, PTAL.

thaJeztah · 2016-02-20T10:33:16Z

Thanks @WeiZhang555, LGTM

ping @vdemeester ptal

vdemeester · 2016-02-20T10:38:57Z

LGTM 🐼

Update RestartPolicy of container

thaJeztah · 2016-02-20T10:40:41Z

Woohoo! 🎉 love this feature 😄

WeiZhang555 · 2016-02-20T11:08:21Z

Finally! 🎉
Also thank everybody for help reviewing! 😆

GordonTheTurtle added the status/0-triage label Jan 6, 2016

WeiZhang555 force-pushed the 19025-update-restart branch 3 times, most recently from 971d05c to f18c8e9 Compare January 6, 2016 07:30

vdemeester added status/1-design-review and removed status/0-triage labels Jan 6, 2016

calavera mentioned this pull request Jan 6, 2016

Add a specific config struct for the update command (#18957) #19104

Merged

2 tasks

WeiZhang555 force-pushed the 19025-update-restart branch from f18c8e9 to 0522fb9 Compare January 13, 2016 14:57

WeiZhang555 mentioned this pull request Jan 13, 2016

Add struct element for update restart-policy docker-archive-public/docker.engine-api#41

Merged

WeiZhang555 force-pushed the 19025-update-restart branch from 0522fb9 to 90544e4 Compare January 16, 2016 07:35

WeiZhang555 force-pushed the 19025-update-restart branch from 90544e4 to 4b0b07f Compare January 25, 2016 14:36

icecrime added status/2-code-review and removed status/1-design-review labels Jan 26, 2016

cpuguy83 reviewed Jan 30, 2016
View reviewed changes

WeiZhang555 force-pushed the 19025-update-restart branch from 4b0b07f to fe9f81b Compare January 30, 2016 03:59

cpuguy83 reviewed Jan 30, 2016
View reviewed changes

vdemeester removed the status/2-code-review label Feb 15, 2016

vdemeester mentioned this pull request Feb 15, 2016

Keep track of --rm daemon side (container update) ? #20334

Closed

thaJeztah mentioned this pull request Feb 16, 2016

Combining unless-stopped and on-failure restart policies #17272

Open

thaJeztah reviewed Feb 19, 2016
View reviewed changes

thaJeztah added impact/api impact/changelog impact/cli labels Feb 19, 2016

thaJeztah added this to the 1.11.0 milestone Feb 19, 2016

Update RestartPolicy of container

ff3ea4c

Add `--restart` flag for `update` command, so we can change restart policy for a container no matter it's running or stopped. Signed-off-by: Zhang Wei <zhangwei555@huawei.com>

WeiZhang555 force-pushed the 19025-update-restart branch from d5a4bf9 to ff3ea4c Compare February 20, 2016 09:06

vdemeester added a commit that referenced this pull request Feb 20, 2016

Merge pull request #19116 from WeiZhang555/19025-update-restart

fe27541

Update RestartPolicy of container

vdemeester merged commit fe27541 into moby:master Feb 20, 2016

WeiZhang555 deleted the 19025-update-restart branch February 20, 2016 11:09

albers mentioned this pull request Feb 21, 2016

Add bash completion for docker update --restart #20562

Merged

This was referenced Mar 3, 2016

Flaky test: TestUpdateRestartPolicy #20922

Closed

Detect container crashes regardless of restart policy #21057

Closed

sdurrheimer mentioned this pull request Mar 26, 2016

Add zsh completion for 'docker update --restart' #21543

Merged

Conversation

WeiZhang555 commented Jan 6, 2016

Uh oh!

thaJeztah commented Jan 6, 2016

Uh oh!

WeiZhang555 commented Jan 6, 2016

Uh oh!

thaJeztah commented Jan 6, 2016

Uh oh!

tianon commented Jan 6, 2016

Uh oh!

WeiZhang555 commented Jan 13, 2016

Uh oh!

WeiZhang555 commented Jan 16, 2016

Uh oh!

icecrime commented Jan 26, 2016

Uh oh!

tiborvass commented Jan 26, 2016

Uh oh!

vdemeester commented Jan 26, 2016

Uh oh!

icecrime commented Jan 26, 2016

Uh oh!

cpuguy83 Jan 30, 2016

Choose a reason for hiding this comment

Uh oh!

WeiZhang555 Jan 30, 2016

Choose a reason for hiding this comment

Uh oh!

cpuguy83 commented Jan 30, 2016

Uh oh!

WeiZhang555 commented Jan 30, 2016

Uh oh!

WeiZhang555 commented Jan 30, 2016

Uh oh!

cpuguy83 Jan 30, 2016

Choose a reason for hiding this comment

Uh oh!

WeiZhang555 Jan 31, 2016

Choose a reason for hiding this comment

Uh oh!

cpuguy83 Jan 31, 2016

Choose a reason for hiding this comment

Uh oh!

WeiZhang555 commented Feb 17, 2016

Uh oh!

calavera commented Feb 17, 2016

Uh oh!

WeiZhang555 commented Feb 19, 2016

Uh oh!

thaJeztah Feb 19, 2016

Choose a reason for hiding this comment

Uh oh!

thaJeztah commented Feb 19, 2016

Uh oh!

GordonTheTurtle commented Feb 20, 2016

Uh oh!

WeiZhang555 commented Feb 20, 2016

Uh oh!

thaJeztah commented Feb 20, 2016

Uh oh!

vdemeester commented Feb 20, 2016

Uh oh!

thaJeztah commented Feb 20, 2016

Uh oh!

WeiZhang555 commented Feb 20, 2016

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

11 participants