rkt: handle multiple rendered images for the same imageID #1240

sgotti · 2015-08-04T12:09:10Z

NOTE:
This is going to change the on disk format for pods, see #1132. This means that actually, after applying it, rkt run-prepared, rkt gc and other commands working on pods will fail to find the stage1 path for exitstent pods and possibly other problems.

If an image has dependencies, and they have changed, for the same imageID we
can have multiple different rendered images (#1198).

Until now this wasn't handled and rkt and the treestore used the imageID to
reference the rendered image.

This patch, does multiple things:

the treestore now returns a treeStoreID on store.RenderTreeStore. The
treeStoreID is computed as an hash of the flattened dependency tree image keys.
In this way if some dependencies have changed a new ID will be generated and a
new image rendering will be done.
rkt has to use this treeStoreID instead of the imageID and save it under appsinfo/$appname/.
- for doing this, at prepare phase, the stage1 tree store ID and every app
  treeStoreID is saved in the pod.

Additionally in this patch the prepare phase will create a pod with all the
needed data for the run phase without the need to keep the needed images in the
store (excluding the need of the treestore rendered images for overlayfs)
(previously store.GetImageManifest was called in the run phase).

Now rkt image rm just removes the image from the blob store.
All the rendered images in the tree store aren't touched by rkt image rm.
They will need a future patch for cleaning the unused one (a store gc). This
will probably add a reference count mechanism to the treestore, since just
retrieving the treeStoreIDs from the pods is quite racy (due to pods getting
created between getting the lists and removing the tree store rendered image).

This means that now we can remove any image in the store at any time after the
prepare phase.

sgotti · 2015-08-28T10:29:11Z

Rebased. Do you have any thoughts on this? Thanks!

iaguis · 2015-08-28T11:00:36Z

common/common.go

+	return filepath.Join(AppsInfoPath(root), appName.String())
+}
+
+// AppInfoPath returns the path to the app's appsinfo directory inside a pod.


comment copypasta

iaguis · 2015-08-28T12:28:30Z

Some small comments but LGTM in general.

I'm not too worried about not being able to GC old containers or run old prepared ones (@jonboulle?).

After this we should definitely implement a store GC that can clean up the tree cache.

sgotti · 2015-08-28T17:05:51Z

@iaguis Thanks for your review. I fixed your notes and did some other little comment fixes/changes.

After this we should definitely implement a store GC that can clean up the tree cache.

Sure. I have to understand which is the better way to do this (locking or reference counting) to avoid possible races.

jonboulle · 2015-09-24T01:23:15Z

Sorry for the delay:

I'm not too worried about not being able to GC old containers or run old prepared ones (@jonboulle?).

I think this is fine. Let's make sure it's in a state we're happy with (and confident won't change again soon ;-) and land it in 0.9.0

@iaguis @alban could you please shepherd this through and get it merged when you're happy with it?

iaguis · 2015-09-25T13:16:45Z

Sure. I have to understand which is the better way to do this (locking or reference counting) to avoid possible races.

Any progress on this? I would like to merge this PR but I'm hesitant to do it without a store GC because it would mean that the CAS could grow indefinitely unless the user removes manually the images from the tree store.

sgotti · 2015-09-27T09:31:18Z

@iaguis Sorry for the delay. I opened #1487 as a follow up for initial image gc that implements the treestore gc. I tried to do this avoiding any reference counting and prevent races using a prepare lock.

iaguis · 2015-09-28T11:34:03Z

store/store_test.go

 	err = os.Chmod(filepath.Join(rootfs, "a"), 0600)
 	if err != nil {
 		t.Fatalf("unexpected error: %v", err)
 	}

 	// Verify image Hash. Should be different
-	err = s.CheckTreeStore(key)
+	err = s.CheckTreeStore(id)
 	if err == nil {
 		t.Fatalf("unexpected error: %v", err)


This should be "expected error but got no error" or something like that

iaguis · 2015-09-28T11:39:42Z

A couple of unrelated nits I didn't notice before but LGTM.

Please fix those and let's merge this PR and make sure we get #1487 too before the release :)

If an image has dependencies, and they have changed, for the same imageID we can have multiple different rendered images. Until now this wasn't handled and rkt and the treestore used the imageID to reference the rendered image. This patch, does multiple things: * the treestore now returns a treeStoreID on store.RenderTreeStore. The treeStoreID is computed as an hash of the flattened dependency tree image keys. In this way if some dependencies have changed a new ID will be generated and a new image rendering will be done. * rkt has to use this treeStoreID instead of the imageID. * for doing this, at prepare phase, the stage1 tree store ID and every app treeStoreID is saved in the pod. Additionally in this patch the prepare phase will create a pod with all the needed data for the run phase without the need to keep the needed images in the store (excluding the need of the treestore rendered images for overlayfs) (previously store.GetImageManifest was called in the run phase). Now `rkt image rm` just removes the image from the blob store. All the rendered images in the tree store aren't touched by `rkt image rm`. They will need a future patch for cleaning the unused one (a store gc). This will probably add a reference count mechanism to the treestore, since just retrieving the treeStoreIDs from the pods is quite racy (due to pods getting created between getting the lists and removing the tree store rendered image). This means that now we can remove any image in the store at any time after the prepare phase.

sgotti · 2015-09-28T12:32:08Z

@iaguis Thanks! Updated.

Looks like semaphore failed only in ./tests/run-build.sh src v222 on TestAceValidator (but poststop OK is available in the output)

iaguis · 2015-09-28T12:51:21Z

Looks like semaphore failed only in ./tests/run-build.sh src v222 on TestAceValidator (but poststop OK is available in the output)

Weird, let's rebuild.

iaguis · 2015-09-28T13:17:25Z

I passed now ^^

…ncies rkt: handle multiple rendered images for the same imageID

sgotti · 2015-09-28T13:33:36Z

@iaguis Thanks! Since gc and run-prepared of previously created pods will fail now, is a release note needed (I also expect some issues being opened due to this)?

iaguis · 2015-09-28T13:39:06Z

is a release note needed (I also expect some issues being opened due to this)?

A mention in the release notes of 0.9.0 should be enough. We did something similar in 0.6.1

sgotti force-pushed the rkt_handle_changed_image_dependencies branch 3 times, most recently from 67c5de0 to f715129 Compare August 10, 2015 11:43

jonboulle added the kind/bug label Aug 19, 2015

jonboulle added this to the v0.9.0 milestone Aug 19, 2015

sgotti force-pushed the rkt_handle_changed_image_dependencies branch 2 times, most recently from 094e73d to faca338 Compare August 28, 2015 10:28

iaguis reviewed Aug 28, 2015
View reviewed changes

sgotti force-pushed the rkt_handle_changed_image_dependencies branch from faca338 to b043c09 Compare August 28, 2015 15:33

jonboulle added the component/store label Sep 10, 2015

sgotti force-pushed the rkt_handle_changed_image_dependencies branch 2 times, most recently from c5a89f1 to 1b71902 Compare September 16, 2015 15:47

jonboulle added needs/review priority/P1 labels Sep 24, 2015

sgotti force-pushed the rkt_handle_changed_image_dependencies branch from 1b71902 to 2032e4d Compare September 27, 2015 08:26

sgotti mentioned this pull request Sep 27, 2015

rkt: implement initial image gc for the tree store. #1487

Merged

sgotti mentioned this pull request Sep 27, 2015

rkt: Add --force flag to image rm subcommand #1489

Closed

blixtra mentioned this pull request Sep 28, 2015

rkt: exit with status 1 on image rm failure #1486

Merged

iaguis reviewed Sep 28, 2015
View reviewed changes

sgotti force-pushed the rkt_handle_changed_image_dependencies branch from 2032e4d to 906105a Compare September 28, 2015 11:59

iaguis added a commit that referenced this pull request Sep 28, 2015

Merge pull request #1240 from sgotti/rkt_handle_changed_image_depende…

aad9237

…ncies rkt: handle multiple rendered images for the same imageID

iaguis merged commit aad9237 into rkt:master Sep 28, 2015

iaguis mentioned this pull request Sep 28, 2015

treestore: detect changed image dependencies. #1198

Closed

blixtra referenced this pull request in yifan-gu/rkt Sep 28, 2015

api: add v1 API protobuf definition and generated codes.

28968f5

jonboulle added reviewed/lgtm and removed needs/review labels Sep 28, 2015

yifan-gu mentioned this pull request Sep 28, 2015

install/stage0: change file mode bits to several fils/dirs. #1452

Merged

sgotti mentioned this pull request Oct 6, 2015

stage 0: gc can fail to delete pods sometimes #1540

Closed

iaguis mentioned this pull request Oct 19, 2015

rkt: rkt list fails to list pods whose image is removed #1628

Closed

iaguis mentioned this pull request Jan 18, 2016

rkt image gc errors after upgrade to 928.0.0 #1990

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

rkt: handle multiple rendered images for the same imageID #1240

rkt: handle multiple rendered images for the same imageID #1240

sgotti commented Aug 4, 2015

sgotti commented Aug 28, 2015

iaguis Aug 28, 2015

iaguis commented Aug 28, 2015

sgotti commented Aug 28, 2015

jonboulle commented Sep 24, 2015

iaguis commented Sep 25, 2015

sgotti commented Sep 27, 2015

iaguis Sep 28, 2015

iaguis commented Sep 28, 2015

sgotti commented Sep 28, 2015

iaguis commented Sep 28, 2015

iaguis commented Sep 28, 2015

sgotti commented Sep 28, 2015

iaguis commented Sep 28, 2015

rkt: handle multiple rendered images for the same imageID #1240

rkt: handle multiple rendered images for the same imageID #1240

Conversation

sgotti commented Aug 4, 2015

sgotti commented Aug 28, 2015

iaguis Aug 28, 2015

Choose a reason for hiding this comment

iaguis commented Aug 28, 2015

sgotti commented Aug 28, 2015

jonboulle commented Sep 24, 2015

iaguis commented Sep 25, 2015

sgotti commented Sep 27, 2015

iaguis Sep 28, 2015

Choose a reason for hiding this comment

iaguis commented Sep 28, 2015

sgotti commented Sep 28, 2015

iaguis commented Sep 28, 2015

iaguis commented Sep 28, 2015

sgotti commented Sep 28, 2015

iaguis commented Sep 28, 2015