add KVS checkpoints #2783

garlick · 2020-02-28T19:19:43Z

Thought I'd park this although it needs further work.

This adds a table to the content-sqlite database that can store arbitrary key-value pairs, then modifies the KVS to read its root blobref from this table (if available) during startup, and write its final root blobref during shutdown. In the future, the table could accommodate multiple keys for live namespaces if we want to go that way (but right now we don't need to support that).

If an instance has the content.backing-path attribute set to the path to a sqlite file when the content-sqlite module is loaded, then it is used (existing content preserved). If it is set but the file does not exist, it is created empty, and retained on exit.

The net result is the systemd unit file could set this attribute to say /var/lib/flux/content.sqlite and then the KVS is preserved across service restarts.

I got rid of the persist-filesystem and persist-directory attributes. Hopefully they will not be missed.

Couple loose ends

If the hash method changes (from sha1 to sha256 say) this is not handled
Need to check but I think sha256 hash digests are being truncated to sha1 size right now in the backing store, which would be a big gaping security hole for multi-user
Maybe it would be nice to have control over "restart" behavior independent of the path setting
Ultimately this should be handled via config not attributes, but this was easier to get launched
More testing required

garlick · 2020-03-03T00:00:31Z

Rebased on current master and forced a push.

grondo · 2020-03-03T00:34:20Z

Without actually looking at the changes 😛, does this make it impossible for multiple instances to share the same content backing store? Not sure if that was even a use case, but it felt like before this was actually a possibility, but with non-unique metadata, no longer so.

(Sorry if a dumb question)

chu11 · 2020-03-03T20:35:39Z

src/broker/broker.c

@@ -149,7 +149,6 @@ static void runlevel_cb (runlevel_t *r, int level, int rc, double elapsed,
 static void runlevel_io_cb (runlevel_t *r, const char *name,
                            const char *msg, void *arg);

-static int create_persistdir (attr_t *attrs, uint32_t rank);


minor nit - add "persist-filesystem" and "persist-directory" into commit message, as its something people may search on (maybe applies to a few other commit messages)

I realized I spread the removal of this over several commits. I'll try to consolidate and include a good, searchable commit message.

chu11 · 2020-03-03T20:40:09Z

src/modules/content-sqlite/content-sqlite.c

-        if (!(dir = flux_attr_get (h, "broker.rundir"))) {
-            flux_log_error (h, "broker.rundir");
+    /* If 'content.backing-path' attribute is already set, then:
+     * - use value is the sqlite backing file


"value is" -> "value of"?

chu11 · 2020-03-03T20:45:10Z

src/modules/content-sqlite/content-sqlite.c

@@ -446,14 +443,6 @@ static int content_sqlite_opendb (struct content_sqlite *ctx)
        log_sqlite_error (ctx, "preparing store stmt");
        goto error;
    }
-    if (sqlite3_prepare_v2 (ctx->db,


it looks like dump_stmt just wasn't used anymore? Perhaps should be different commit than this one?

Yes, I removed the code that used it in #2786 but forgot to remove the statement. I'll break it out to a separate commit.

chu11 · 2020-03-03T21:56:34Z

src/cmd/builtin/content.c

+      NULL,
+    },
+    { "metaput",
+      "KEY VAL",


for consistency to flux kvs should input be "key=val" instead of "key val"?

I ended up dropping this from flux-content(1) and moving to a test program. I retained the existing usage but I assume it's no big deal in a test program.

chu11 · 2020-03-03T22:03:59Z

src/modules/content-sqlite/content-sqlite.c

@@ -36,12 +36,23 @@ const char *sql_load = "SELECT object,size FROM objects"
 const char *sql_store = "INSERT INTO objects (hash,size,object) "
                        "  values (?1, ?2, ?3)";

+const char *sql_create_table_meta = "CREATE TABLE if not exists meta("


commit message, "intented" -> "intended"?

garlick · 2020-03-04T04:10:48Z

Thanks @chu11 for the review comments, really helpful! I'll fix those tomorrow.

does this make it impossible for multiple instances to share the same content backing store? Not sure if that was even a use case, but it felt like before this was actually a possibility, but with non-unique metadata, no longer so.

Uhh yeah. True. I don't think it's a near term use case, but we do have a nice clean design that permits it, and this messes it up. Let me ponder whether there's a way we can do this that's looks less like a malignant growth on a perfectly good abstraction.

garlick · 2020-03-04T17:07:39Z

Just forced a push that refactors/rewords some of the commits and hopefully addresses all @chu11's review comments.

I went ahead and implemented a separate service called kvs-checkpoint for the checkpoint metadata. It is still implemented in the content-sqlite module at the moment, to share the same database, but I've tried to indicate that it's not part of the content store per se, and that this was done for expediency. Also, I removed the metaget/metaput subcommands from flux-content(1) and instead just implemented those in a test program since really this wasn't intended for general purpose use. Hopefully that at least makes its mixing with the content-sqlite service less of a design violation.

garlick · 2020-03-05T16:23:22Z

This is probably ready for a review. I might be able to increase the diff coverage in a couple of cases, but a lot of the uncovered code is in hard to reach error paths.

chu11

overall LGTM, only noticed this one thing

chu11 · 2020-03-05T23:36:03Z

src/modules/content-sqlite/content-sqlite.c

@@ -376,27 +375,31 @@ static void content_sqlite_closedb (struct content_sqlite *ctx)
            if (sqlite3_finalize (ctx->store_stmt) != SQLITE_OK)
                log_sqlite_error (ctx, "sqlite_finalize store_stmt");
        }
-        if (ctx->load_stmt) {


seems like a rebase error, where ctx->load_stmt got moved (in your "content-sqlite: handle content.backing-path attr " commit)

Nice catch! Fixing...

Problem: The 'sql_dump' prepared statement is unused, left over from code removed in flux-framework#2786. Remove unused prepared statement.

Drop code that validates a 'persist-directory' attribute, if set, or that sets 'persist-directory' to a unique directory in 'persist-filesystem', if set. Drop code that sets the broker 'log-filename' attribute to a location in 'persist-directory', if the former is unset and the latter is set. Drop code from rc3 that attempts to write out a KVS snapshot if 'persist-directory', if set. Drop relevant sharness tests from t0001-basic.t. Drop all tests from t2010-kvs-snapshot-restore.t for now. Drop these attributes from flux-broker-attributes(7) man page.

Drop the code that manipulates the location and persistence of the sqlite db file based on the 'persist-directory' attribute, and replace it with the following: If 'content.backing-path' attribute is set on entry, use its value as the sqlite db file. sqlite creates this file if it doesn't exist. On exit, it is preserved. If 'content.backing-path' is not set on entry, use '${rundir}/content.sqlite' as the sqlite db file. sqlite creates this file if it doesn't exist(*). On exit it is removed. Set the 'content.backing-path' attribte so the location of this file. Add a 'flux content flush' command to rc3 just before content-sqlite unload, to ensure that any dirty cache entries are flushed to the backing store before content-sqlite is unloaded. _______ (*) This file is not expected to exist initially given that rundir is a temporary directory, but it _could_ exit if the content-sqlite module is reloaded. Reusing its content is appropriate in that case.

Add a new sqlite table to store KVS checkpoint data, and offer a new service, 'kvs-checkpoint', with get/put methods. The table has the following properties: - keys and values are strings of any length - keys are unique - empty string key is not allowed - put to existing key replaces value Note: piggybacking this service/table onto content-sqlite is a temporary short cut to get something working quickly. It should probably be split out to its own module eventually, as implementing it here will make it cumbersome to replace the content backing module.

On rank 0 startup, try to get a 'kvs-primary' key from the kvs-checkpoint service. If the service is available and the key is found, use it as the blobref for the initial KVS root. If unavailable or not found, start with an empty directory as before. On rank 0 unload, try to put 'kvs-primary' to the kvs-checkpoint service. If the service is unavailable, don't treat this as an error, since the instance may be purposefully running without one.

Add a test program for exercising kvs-snapshot.get and kvs-snapshot.put service.

Describe the new content.backing-path attribute.

chu11 · 2020-03-07T00:22:32Z

Good to go now? I'll let ya set merge-when-passing if ya think so.

garlick · 2020-03-07T01:36:10Z

Thanks!

Restarted a builder that failed like this:

There was an error in the .travis.yml file from which we could not recover.
Unfortunately, we do not know much about this error.
Please review https://docs.travis-ci.com, or contact us at [email protected] with the error ID: cee1e4cd24984025927555fb39ff1d89

codecov-io · 2020-03-07T01:48:54Z

Codecov Report

Merging #2783 into master will decrease coverage by 0.05%.
The diff coverage is 63.55%.

@@            Coverage Diff             @@
##           master    #2783      +/-   ##
==========================================
- Coverage   81.03%   80.98%   -0.06%     
==========================================
  Files         250      250              
  Lines       39424    39497      +73     
==========================================
+ Hits        31949    31988      +39     
- Misses       7475     7509      +34

Impacted Files	Coverage Δ
src/broker/broker.c	`73.28% <ø> (-0.03%)`	⬇️
src/broker/log.c	`71.17% <ø> (+0.49%)`	⬆️
src/modules/content-sqlite/content-sqlite.c	`55.66% <59.3%> (+0.66%)`	⬆️
src/modules/kvs/kvs.c	`66.81% <75%> (+0.44%)`	⬆️
src/common/libpmi/simple_client.c	`86.33% <0%> (-3.28%)`	⬇️
src/common/libflux/handle.c	`83.7% <0%> (-2.01%)`	⬇️
src/common/libpmi/pmi.c	`91.5% <0%> (-1.89%)`	⬇️
src/common/libsubprocess/local.c	`80.06% <0%> (-0.35%)`	⬇️
src/cmd/flux-kvs.c	`81.67% <0%> (-0.23%)`	⬇️
... and 5 more

This was referenced Feb 28, 2020

libutil/cleanup: improve out of memory handling, conform to RFC 7 #2785

Merged

content-sqlite, content-cache: cleanup and refactoring #2786

Merged

garlick force-pushed the checkpoint_restart branch from 290a17e to 40409f1 Compare March 2, 2020 23:59

garlick mentioned this pull request Mar 4, 2020

broker: clean shutdown on SIGTERM, add broker.rc2_none attribute #2794

Merged

chu11 reviewed Mar 4, 2020

View reviewed changes

garlick force-pushed the checkpoint_restart branch from 40409f1 to fd987cd Compare March 4, 2020 17:02

garlick changed the title ~~WIP: add KVS checkpoints~~ add KVS checkpoints Mar 4, 2020

chu11 reviewed Mar 5, 2020

View reviewed changes

garlick added 8 commits March 5, 2020 16:18

content-sqlite: drop unused dump prepared stmt

1ef979b

Problem: The 'sql_dump' prepared statement is unused, left over from code removed in flux-framework#2786. Remove unused prepared statement.

testsuite: add kvs/checkpoint test program

d28bde7

Add a test program for exercising kvs-snapshot.get and kvs-snapshot.put service.

testsuite: cover KVS checkpoint/restart

d95eb5e

doc/flux-broker-attributes(7): add backing-path

558d46f

Describe the new content.backing-path attribute.

garlick force-pushed the checkpoint_restart branch from fd987cd to 558d46f Compare March 6, 2020 00:18

gonsie mentioned this pull request Mar 6, 2020

RST conversion #2806

Closed

chu11 approved these changes Mar 7, 2020

View reviewed changes

garlick added the merge-when-passing label Mar 7, 2020

mergify bot merged commit 02ef011 into flux-framework:master Mar 7, 2020

garlick deleted the checkpoint_restart branch March 7, 2020 02:09

This was referenced Mar 16, 2020

instance restartability #2650

Closed

config: need persistent storage option(s) #2641

Closed

garlick mentioned this pull request Apr 23, 2020

add content-sqlite option to set database location #1038

Closed

garlick mentioned this pull request Mar 31, 2021

kvs: log date of restored checkpoint #3580

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

add KVS checkpoints #2783

add KVS checkpoints #2783

garlick commented Feb 28, 2020

garlick commented Mar 3, 2020

grondo commented Mar 3, 2020

chu11 Mar 3, 2020

garlick Mar 4, 2020

chu11 Mar 3, 2020

chu11 Mar 3, 2020

garlick Mar 4, 2020

chu11 Mar 3, 2020

garlick Mar 4, 2020

chu11 Mar 3, 2020

garlick commented Mar 4, 2020

garlick commented Mar 4, 2020

garlick commented Mar 5, 2020

chu11 left a comment

chu11 Mar 5, 2020

garlick Mar 6, 2020

chu11 commented Mar 7, 2020

garlick commented Mar 7, 2020

codecov-io commented Mar 7, 2020

add KVS checkpoints #2783

add KVS checkpoints #2783

Conversation

garlick commented Feb 28, 2020

garlick commented Mar 3, 2020

grondo commented Mar 3, 2020

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

garlick commented Mar 4, 2020

garlick commented Mar 4, 2020

garlick commented Mar 5, 2020

chu11 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chu11 commented Mar 7, 2020

garlick commented Mar 7, 2020

codecov-io commented Mar 7, 2020

Codecov Report