Clear and migration to version 2 #1015

icristescu · 2020-06-23T15:12:30Z

It depends on #1008 and #939. (I hesitated to add it in one of the two PR already opened, but it really needs both for the tests).
It's only the latest commit that is new w.r.t the two other PRs.

Clear is used when opening a fresh instance. Because of this an unrelated test is failing (it did not properly closed the store before) and so I commented out. I'll make a separate PR to remove and explain it.

icristescu · 2020-07-27T15:56:31Z

This PR is now ready for reviews. It contains:

clear function from Clear stores #939; in irmin-pack the implementation of clear consists of increasing the generation; writing the generation to file; removing the file and opening a new one.
snapshot isolations tests for readonly (asked by irmin-pack: ensure snapshot-isolation of read-only instances #1040) in readonly_find_old and readonly_find_old_after_rewrite.
migration from V1 (without the generation in the header) to V2 (includes the generation): when opening the store, if the store is in V1 then migrate_to_current_version is automatically called.

One issue is clearing the lru for the RO instance: (see commit dd3bcf6)
After a clear, during a sync, the RO instance has to clear its lru as well, otherwise it will keep finding old values in the lru. To do this it check if the generation is updated in the pack IO.
Contents, commits and nodes stores each have their own lru but share the same instance of pack IO. Therefore the first store to sync, updates pack IO and has to clear the lrus for all the other stores. This is achieved with a clear_lrus callback.
Note that the stores cannot share the same lru, as they have different value types.

test/irmin-pack/migration.ml

src/irmin-http/irmin_http_server.ml

src/irmin-test/store.ml

src/irmin-pack/dict.ml

src/irmin-pack/pack.ml

src/irmin-pack/inode.ml

src/irmin-pack/irmin_pack.ml

src/irmin-pack/pack.ml

After a clear, during a sync, the RO instance has to clear its lru as well, otherwise it will keep finding old values in the lru. To do this it check if the generation is updated in the pack IO. Contents, commits and nodes stores each have their own lru but share the same instance of pack IO. Therefore the first store to sync, updates pack IO and has to clear the lrus for all the other stores. This is achieved with a [clear_lru] callback.

craigfe

LGTM. I have a few nits, mostly about out-of-date comments.

CHANGES.md

craigfe · 2020-07-29T13:34:45Z

src/irmin-pack/irmin_pack.mli

+
+  val migrate : Irmin.config -> unit Lwt.t
+  (** [migrate conf] migrates a store with configuration [conf] to the current
+      version. Raises [RO_Not_Allowed] if called by a readonly instance. *)


Suggested change

version. Raises [RO_Not_Allowed] if called by a readonly instance. *)

version. *)

it does raise this exception (https://github.com/mirage/irmin/pull/1015/files#diff-1beb7e2142825f197755316a39d00699R607). you think we shouldn't mention it?

craigfe · 2020-07-29T13:36:09Z

src/irmin-pack/pack.ml

-    if IO.version block <> current_version then
-      Fmt.failwith "invalid version: got %S, expecting %S" (IO.version block)
-        current_version;


This check seems like it might still be worthwhile.

I use v from irmin_pack and thus unsafe_v from pack in the migrate function, so this needs to accept both V1 and V2. there is a check that files are in either V1 or V2 in IO (https://github.com/mirage/irmin/pull/1015/files#diff-8a044b6b9c9b0a69a5f770d759c9d1b9R264)

test/irmin-pack/migration.ml

craigfe · 2020-07-29T13:39:46Z

test/irmin-pack/migration.ml

+  let cmd = Printf.sprintf "cp -rp %s %s" root_V1_source root_V1 in
+  Fmt.epr "exec: %s\n%!" cmd;


Need to quote such commands with Filename.quote_command for this to work with certain filepaths on Windows.

It seems that this is only included in ocaml 4.10

craigfe · 2020-07-29T13:41:15Z

test/irmin-pack/migration.ml

+    | Some commit -> check r commit "check old values b" [ "b" ] "y"
+  in
+  (* dune removes the write permission from root_V1, we can only open it in
+     readonly mode in the tests. We can migrate with a read-write pack, because


We can migrate with a read-write pack

Not sure what you mean here, since migrate doesn't take a pack. Is this out-of-date?

the comment is not clear here indeed, I removed it

craigfe · 2020-07-29T13:45:10Z

test/irmin-pack/migration.ml

+    | None -> Alcotest.failf "branch foo not found"
+    | Some commit -> check r commit "check old values b" [ "b" ] "y"
+  in
+  (* dune removes the write permission from root_V1, we can only open it in


dune removes the write permission from root_V1

We could use install rather than cp if this is a problem. I have no idea what the Windows portability is like for this though...

the issue is the copy from the deps source_tree stanza, which removes the write permissions for testV1.
it's ok to open the store in the readonly mode in this test. and it made me realise that I only need to open it in RO in the migrate function too, so it is a good constraint from dune.

Co-authored-by: Craig Ferguson <me@craigfe.io>

icristescu · 2020-07-29T19:21:03Z

I get the following exception when I try to migrate the archive store from comanche (The testing branch is here https://github.com/icristescu/tezos/tree/migrate).

+613050987us irmin.pack 784k contents / 6198k nodes / 17k commits
Thread 13 killed on uncaught exception Unix.Unix_error(24, "write", "")
Raised by primitive operation at file "src/unix/syscalls.ml" (inlined), line 11, characters 2-49
Raised by primitive operation at file "src/unix/raw.ml", line 33, characters 12-73
Called from file "src/unix/raw.ml" (inlined), line 60, characters 2-27
Called from file "src/unix/index_unix.ml", line 54, characters 6-47
Called from file "src/index.ml", line 565, characters 14-99
Called from file "src/index.ml", line 649, characters 18-71
Called from file "src/stats.ml", line 92, characters 15-19
Called from file "src/unix/index_unix.ml", line 290, characters 29-34
Re-raised at file "src/unix/index_unix.ml", line 293, characters 8-17
Called from file "thread.ml", line 39, characters 8-14

so we shouldn't merge this for now.

samoht · 2020-08-27T08:41:52Z

Closing as this has been merged part of #1071 and #1070

icristescu force-pushed the clear_ro branch from d966273 to c9b24ac Compare July 1, 2020 12:12

icristescu mentioned this pull request Jul 9, 2020

Clear stores #939

Closed

icristescu mentioned this pull request Jul 20, 2020

Add Irmin_pack.clear which ensures snapshot isolation of read-only instances #1047

Merged

icristescu force-pushed the clear_ro branch 3 times, most recently from 230c515 to 84617e0 Compare July 27, 2020 15:39

icristescu commented Jul 27, 2020

View reviewed changes

test/irmin-pack/migration.ml Show resolved Hide resolved

icristescu changed the title ~~Clear + ro~~ Clear and migration to version 2 Jul 27, 2020

icristescu force-pushed the clear_ro branch from 84617e0 to 718b089 Compare July 27, 2020 16:19

craigfe reviewed Jul 27, 2020

View reviewed changes

icristescu added 5 commits July 28, 2020 21:31

Clear stores API

54a3087

Clear forces a refill on AW and dict

1d0925a

Clear removes file

00b556a

Add tests

2a7fee8

icristescu force-pushed the clear_ro branch 2 times, most recently from abd10df to a6d1de4 Compare July 29, 2020 13:05

craigfe approved these changes Jul 29, 2020

View reviewed changes

icristescu and others added 8 commits July 29, 2020 19:58

Migrate to version 2

68a68ee

Add test dependency

1a53976

Add changes entry

3aa107b

irmin-pack: reshuffle data locations for the migration test

675a68c

Path for dune runtest

1256dd8

irmin-http: de-duplicate implementation of clear endpoint

f3aa12c

Suggested changes

7ab09fa

Co-authored-by: Craig Ferguson <me@craigfe.io>

Explicit migration

4b5c3c0

icristescu force-pushed the clear_ro branch from a6d1de4 to a691d34 Compare July 29, 2020 18:44

Open test dependencies in readonly mode

82c268a

icristescu force-pushed the clear_ro branch from a691d34 to 82c268a Compare July 29, 2020 19:15

samoht added a commit to icristescu/irmin that referenced this pull request Aug 11, 2020

Be closer to mirage#1015

f56370f

samoht added a commit to samoht/irmin that referenced this pull request Aug 11, 2020

Be closer to mirage#1015

8dc05d7

samoht added a commit to samoht/irmin that referenced this pull request Aug 11, 2020

Be closer to mirage#1015

a002c9e

samoht added a commit to icristescu/irmin that referenced this pull request Aug 11, 2020

Be closer to mirage#1015

8dde0da

samoht added a commit to icristescu/irmin that referenced this pull request Aug 13, 2020

Be closer to mirage#1015

28ed89e

samoht added a commit to samoht/irmin that referenced this pull request Aug 18, 2020

Be closer to mirage#1015

8fe1173

craigfe self-assigned this Aug 19, 2020

This was referenced Aug 21, 2020

irmin-pack: add a migration function to the V2 format #1070

Merged

Add a clear function to all backends #1071

Merged

samoht closed this Aug 27, 2020

icristescu deleted the clear_ro branch August 30, 2020 19:20

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Clear and migration to version 2 #1015

Clear and migration to version 2 #1015

icristescu commented Jun 23, 2020

icristescu commented Jul 27, 2020

craigfe left a comment

craigfe Jul 29, 2020

icristescu Jul 29, 2020

craigfe Jul 29, 2020

icristescu Jul 29, 2020

craigfe Jul 29, 2020

icristescu Jul 29, 2020

craigfe Jul 29, 2020

icristescu Jul 29, 2020

craigfe Jul 29, 2020

icristescu Jul 29, 2020

icristescu commented Jul 29, 2020

samoht commented Aug 27, 2020

	version. Raises [RO_Not_Allowed] if called by a readonly instance. *)
	version. *)

		let cmd = Printf.sprintf "cp -rp %s %s" root_V1_source root_V1 in
		Fmt.epr "exec: %s\n%!" cmd;

Clear and migration to version 2 #1015

Clear and migration to version 2 #1015

Conversation

icristescu commented Jun 23, 2020

icristescu commented Jul 27, 2020

craigfe left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

icristescu commented Jul 29, 2020

samoht commented Aug 27, 2020