🚀 Advanced: [17.06] Graceful upgrade of containerd and runc state files upon live-restore by tiborvass · Pull Request #117 · docker-archive/docker-ce · GitHub

tiborvass · 2017-07-13T06:13:01Z

To address issue:

Containers can not start after docker upgrade from 17.03.0-ce to 17.06.0-ce docker/for-linux#49 Containers can not start after docker upgrade from 17.03.0-ce to 17.06.0-ce

mlaventure · 2017-07-13T17:23:38Z

 		return fmt.Errorf("container %s is already active", containerID)
 	}

+	if errUpgrade := v17_06_1.Upgrade(


by this time, it's too late if I'm not mistaken.

containerd on restart tries to load all of those files and would have failed to unmarshal them

thanks, this is not supposed to be here... only the other Upgrade() call. Will remove.

mlaventure · 2017-07-13T17:26:09Z

+		ids[i] = fi.Name()
+	}
+	f.Close()
+	for _, id := range ids {


I don't think we should upgrade all the entries, we should only upgrade the one which ids are found in /run/docker/libcontainerd/containerd

Nothing prevent the user to having different version of containerd/runc on the machine and we don't want to break the containers started with those.

Thanks for catching this. Will update.

andrewhsu · 2017-07-13T18:57:20Z

In latest build result:

00:27:05.307 FAIL: docker_cli_daemon_test.go:2813: DockerDaemonSuite.TestExecWithUserAfterLiveRestore
00:27:05.307 
00:27:05.307 [dd68c05e3b6f9] waiting for daemon to start
00:27:05.307 [dd68c05e3b6f9] daemon started
00:27:05.307 
00:27:05.307 [dd68c05e3b6f9] exiting daemon
00:27:05.307 [dd68c05e3b6f9] waiting for daemon to start
00:27:05.307 [dd68c05e3b6f9] daemon started
00:27:05.307 
00:27:05.307 docker_cli_daemon_test.go:2834:
00:27:05.307     c.Assert(err, check.IsNil, check.Commentf("Output: %s", out2))
00:27:05.307 ... value *exec.ExitError = &exec.ExitError{ProcessState:(*os.ProcessState)(0xc42054f1e0), Stderr:[]uint8(nil)} ("exit status 126")
00:27:05.307 ... Output: invalid character 's' after top-level value
00:27:05.307 
00:27:05.307 
00:27:05.307 [dd68c05e3b6f9] exiting daemon

tiborvass · 2017-07-13T19:24:45Z

Ping @mlaventure

tiborvass · 2017-07-13T19:34:58Z

+		MountLabel        string             `json:"mount_label"`
+		Hostname          string             `json:"hostname"`
+		Namespaces        configs.Namespaces `json:"namespaces"`
+		Capabilities      linuxCapabilities  `json:"capabilities"`


@mlaventure just realized that linuxCapabilities uses specs.LinuxCapabilities (https://github.com/docker/docker-ce/pull/117/files#diff-40363fc1cc8aab85c96e0f6d4e7d4315R97) while this is supposed to be configs.Capabilities, which thankfully happens to be the same.

Maybe it's fine for this version and we can think of something more accurate after?

tiborvass · 2017-07-13T19:35:39Z

+		if err != nil {
+			return err
+		}
+		// TODO: copy caps or not copy caps?


@mlaventure any thoughts on this?

wdym by copy?

@mlaventure I mean:

copy(boundingCaps, caps) copy(effectiveCaps, caps) ...

Otherwise, l.V.Bounding[i] == l.V.Effective[i], and if someone writes code to change an element of Bounding it will change all the other ones too, unwanted side-effect. Although it's only in very special cases, where the same underlying array is used.

It's the case currently in the daemon code too: https://github.com/moby/moby/blob/master/daemon/oci_linux.go#L258-L266

@mlaventure okay thanks, so let's keep it orthogonal.

andrewhsu · 2017-07-13T21:19:41Z

retriggered a new build https://jenkins.dockerproject.org/job/docker-ce-17.06-pr/188/console

tiborvass · 2017-07-18T20:45:41Z

@mlaventure I pushed a better version, that does all the decoding first, before overwriting the files. This is so that if one of the later files fails to be decoded, the previous ones keep their initial state and we don't end up in a mixed state.

tiborvass · 2017-07-18T22:58:07Z

And now it also encodes into a bytes.Buffer before writing to file, just in case the reencoding phase fails (not just the decoding).

I hope the extra buffering doesn't become an issue when there's a lot of containers.

tiborvass · 2017-07-21T00:03:45Z

Ping @mlaventure

stevvooe · 2017-07-21T00:58:35Z

LGTM after moving to atomic file write.

mlaventure · 2017-07-21T07:16:36Z

I agree with @stevvooe it's better to write to a tmp file, then rename it.

…e-restore Vendors new dependency github.com/crosbymichael/upgrade Signed-off-by: Tibor Vass <tibor@docker.com>

andrewhsu · 2017-07-22T06:08:55Z

LGTM

I've tested this PR by building a deb package and upgrading from a docker-ce 17.03 daemon that had a container started with live-restore. After the upgrade, container still seen as active from docker ps.

crosbymichael · 2017-07-24T17:53:16Z

LGTM

stevvooe · 2017-07-24T18:14:19Z

+		defer f.w.Close()
+	}
+	var errs []string
+	for _, f := range files {


Ok, so this sucks everything up into memory, creates an atomic file for each, then writes the buffer.

stevvooe · 2017-07-24T21:17:53Z

LGTM

tiborvass · 2017-07-24T21:20:44Z

@vieux Line for changelog: Fix issue upon upgrade, preventing docker from showing running containers when --live-restore is enabled

andrewhsu · 2017-07-24T21:51:44Z

@tiborvass @vieux I've also verified this PR works on upgrade from docker-ce 17.05.0.

andrewhsu · 2017-07-24T22:01:03Z

Note to selves: after conversation with @tiborvass may need to look into having all 3 json files updated atomically. As this PR stands, looks good to have containers still running with live restore.

[18.06] update package descriptions and info Upstream-commit: 2baa88e2bcf7c451ea8405fa54948c07f722cf9e Component: packaging

fixes #117 Print healthcheck information in pretty mode. Signed-off-by: Stephane Jeandeaux <stephane.jeandeaux@gmail.com> Upstream-commit: 05674a50961d99eaf5320f32beb91646fd1355b4 Component: cli

Signed-off-by: Stephane Jeandeaux <stephane.jeandeaux@gmail.com> Upstream-commit: d4ad7a94d217a8925923dd8e232fd949b3ec1bd4 Component: cli

[engine] Graceful upgrade of containerd and runc state files upon live-restore

andrewhsu added this to the 17.06.1 milestone Jul 13, 2017

mlaventure suggested changes Jul 13, 2017

View reviewed changes

tiborvass force-pushed the fix-live-restore branch from 9796c06 to 4f96326 Compare July 13, 2017 19:24

tiborvass force-pushed the fix-live-restore branch from 4f96326 to d6f885e Compare July 13, 2017 19:31

tiborvass commented Jul 13, 2017

View reviewed changes

tiborvass force-pushed the fix-live-restore branch from d6f885e to 7148d7b Compare July 18, 2017 20:43

tiborvass force-pushed the fix-live-restore branch from 7148d7b to ec6a007 Compare July 18, 2017 22:54

mlaventure approved these changes Jul 21, 2017

View reviewed changes

[engine] Graceful upgrade of containerd and runc state files upon liv…

358c36e

…e-restore Vendors new dependency github.com/crosbymichael/upgrade Signed-off-by: Tibor Vass <tibor@docker.com>

tiborvass force-pushed the fix-live-restore branch from ec6a007 to 358c36e Compare July 22, 2017 05:55

stevvooe reviewed Jul 24, 2017

View reviewed changes

vieux merged commit 8bdf679 into docker-archive:17.06 Jul 24, 2017

vieux changed the title ~~[engine] Graceful upgrade of containerd and runc state files upon live-restore~~ [17.06] Graceful upgrade of containerd and runc state files upon live-restore Jul 24, 2017

vieux mentioned this pull request Jul 24, 2017

[17.07] Graceful upgrade of containerd and runc state files upon live-restore #131

Merged

thaJeztah mentioned this pull request Sep 22, 2017

Containers can not start after docker upgrade from 17.03.0-ce to 17.06.0-ce docker/for-linux#49

Closed

docker-jenkins pushed a commit that referenced this pull request Jul 3, 2018

Merge pull request #117 from andrewhsu/p

83fa48a

[18.06] update package descriptions and info Upstream-commit: 2baa88e2bcf7c451ea8405fa54948c07f722cf9e Component: packaging

docker-jenkins pushed a commit that referenced this pull request Mar 19, 2019

[#117] remove blank line and fix order

992fde0

Signed-off-by: Stephane Jeandeaux <stephane.jeandeaux@gmail.com> Upstream-commit: d4ad7a94d217a8925923dd8e232fd949b3ec1bd4 Component: cli

silvin-lubecki pushed a commit to silvin-lubecki/docker-ce that referenced this pull request Feb 3, 2020

Merge pull request docker-archive#117 from tiborvass/fix-live-restore

4c41d3d

[engine] Graceful upgrade of containerd and runc state files upon live-restore

Conversation

tiborvass commented Jul 13, 2017 • edited by andrewhsu 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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

andrewhsu commented Jul 13, 2017

Uh oh!

tiborvass commented Jul 13, 2017

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!

tiborvass Jul 17, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

andrewhsu commented Jul 13, 2017

Uh oh!

tiborvass commented Jul 18, 2017

Uh oh!

tiborvass commented Jul 18, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

tiborvass commented Jul 21, 2017

Uh oh!

stevvooe commented Jul 21, 2017

Uh oh!

mlaventure commented Jul 21, 2017

Uh oh!

andrewhsu commented Jul 22, 2017

Uh oh!

crosbymichael commented Jul 24, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

stevvooe commented Jul 24, 2017

Uh oh!

tiborvass commented Jul 24, 2017

Uh oh!

andrewhsu commented Jul 24, 2017

Uh oh!

andrewhsu commented Jul 24, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

tiborvass commented Jul 13, 2017 •

edited by andrewhsu

Loading

tiborvass Jul 17, 2017 •

edited

Loading

tiborvass commented Jul 18, 2017 •

edited

Loading