Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

IDEA: restore kvs from persist-directory #777

Closed
wants to merge 7 commits into from

Conversation

grondo
Copy link
Contributor

@grondo grondo commented Aug 21, 2016

For testing purposes, it would be nice to be able to restore a previous kvs state to test against, instead of creating the kvs state on the fly. For example, there is a bug in flux wreck attach that seems to only be triggered from a lwj with >32K tasks. Rather than write a test that first runs 32K tasks (or even simulates a run of 32K tasks by writing to kvs), it would be nice to start with an existing persist-directory content store, and "recover" the kvs using the last know rootdir sha1 hash.

This PR is just a proof-of-concept of that idea (original credit going to @garlick for the idea).

Two new "plumbing" type commands are added flux kvs-getroot and flux kvs-setroot which are simple interfaces to kvs.getroot and kvs.setroot message/events.

If persist-directory is set, the output of kvs.getroot is saved to the persist-directory.

In order for the content store to "restart" from and existing sqlite db, two kind of hackish mods were made to the content-sqlite

  • Create the objects table with 'if not exists' so that an existing objects table is not an error
  • Do not error if mkdir returns EEXIST

Once a session is started using a previous persist-directory, the kvs can then be "restored" with a command like:

flux kvs-setroot $(flux getattr persist-directory)/kvsroot

I'm guessing this is a bit dangerous without some further sanity checks, so I'd only propose this for testing at this point. Here's some results

grondo@flux-core:~/workspace (kvs-restore) $ flux wreck ls | wc -l
102
grondo@flux-core:~/workspace (kvs-restore) $ exit
exit
grondo@flux-core:~/workspace (kvs-restore) $ src/cmd/flux start -o,-Spersist-directory=/tmp/tmp.zgK84E8ouy
grondo@flux-core:~/workspace (kvs-restore) $ flux wreck ls
/home/ubuntu/workspace/src/cmd/flux-wreck ls: Failed to get lwj info: No such file or directory
grondo@flux-core:~/workspace (kvs-restore) $ flux kvs-setroot $(flux getattr persist-directory)/kvsroot
Setting root to sha1-866b6bd85ca854e7335cfd7a4b73fcba1292476e
grondo@flux-core:~/workspace (kvs-restore) $ flux wreck ls | wc -l
102
grondo@flux-core:~/workspace (kvs-restore) $ exit
exit

grondo added 7 commits August 20, 2016 16:37
Allow a NULL JSON object to be sent in flux_rpc().
Add "if not exists" to CREATE TABLE command when opening content
store to allow the open of an existing content store sqlite
file.
Do not issue a fatal error on mkdir() if persist directory
already exists.

This allows opening an existing persist directory.
Add a simple script to dump `kvs.getroot` result to stdout.
If persist-directory is set save kvs root information
along with joblog.
Add command to set kvs root from a Lua table in a file.
Test automatic `flux kvs-getroot` with persist-directory, along with
kvs restore from a new session using `flux kvs-setroot`.
@grondo grondo added the review label Aug 21, 2016
@codecov-io
Copy link

codecov-io commented Aug 21, 2016

Current coverage is 74.76% (diff: 88.88%)

Merging #777 into master will decrease coverage by 0.01%

@@             master       #777   diff @@
==========================================
  Files           145        145          
  Lines         25022      25025     +3   
  Methods           0          0          
  Messages          0          0          
  Branches          0          0          
==========================================
- Hits          18712      18711     -1   
- Misses         6310       6314     +4   
  Partials          0          0          
Diff Coverage File Path
•••••••• 87% src/bindings/lua/flux-lua.c
•••••••••• 100% src/modules/content-sqlite/content-sqlite.c

Powered by Codecov. Last update 1df7925...724ec46

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.01%) to 75.087% when pulling 724ec46 on grondo:kvs-restore into 1df7925 on flux-framework:master.

@dongahn
Copy link
Member

dongahn commented Aug 21, 2016

Does this at all or How does this relate to the cvs snapshot idea captured on various issue tickets including #76?

@grondo
Copy link
Contributor Author

grondo commented Aug 21, 2016

I do remember having discussions about this and similar ideas for kvs snapshot/restore, but I searched for related issues and was unsuccessful at finding them.

This particular PR is more like a "stunt" to show feasibility, but I do have at least one idea for its use in testing, for example to reuse a kvs already populated with large number of jobs, large task count, or lots of IO etc -- for more efficient testing...

@dongahn
Copy link
Member

dongahn commented Aug 21, 2016

I am a big fan of use case-based development!

I'm just throwing other areas which can potentially benefit from similar enhancements. I don't remember exactly, but such a capability can help one implement various consistency models over how our cvs can be used in distributed computing : #77.

@grondo
Copy link
Contributor Author

grondo commented Aug 21, 2016

Hm, I didn't see how #77 is related, is that the correct issue?

I think in general the design of the kvs lends itself to easily supporting snapshots, rollback, historical "views" etc. This particular case is probably just a reduction to the simplest useful case.

I was pretty happy at how easy it was to get something working, so I though I'd post to get some discussion.

@dongahn
Copy link
Member

dongahn commented Aug 21, 2016

Oops. Fat finger -- doing this while prepping for church this morning. The issue I was trying to refer to was: #206

@grondo
Copy link
Contributor Author

grondo commented Aug 21, 2016

Thanks @dongahn,

What is proposed here is nothing as complex as some of things described in #206, except that something like flux kvs-getroot could be used to save off a kvs version. However, without a facility to create a kvsdir at a given root version, the kvs-setroot proposed here is global, so not all that useful outside of testing or one-time kvs restore.

@garlick
Copy link
Member

garlick commented Aug 21, 2016

This is great and without having looked in too much detail, sounds like what I had imagined for a sort of checkpoint/restart for a session.

Make sure not to violate the invariant that the "rootseq" is monotonically increasing, e.g. setroot should not allow that number to go backwards.

@grondo
Copy link
Contributor Author

grondo commented Aug 21, 2016

Great point!

I was thinking kvs-getroot might be installed if something like this PR were ever merged (or added to flux-kvs), but kvs-setroot might only be available for the testsuite.

It might be useful for now to store the kvs root info with persist-directory just because...

@garlick
Copy link
Member

garlick commented Aug 22, 2016

I wonder if maybe we need a new operation like

int kvs_link (flux_t h, const char *key, const char *blobref, bool directory);

that would allow any object to be grafted into the namespace by reference, with say a NULL key or a key of . indicating root?

The "rootseq" could be managed in the usual way by the rank 0 commit logic.

In addition to the above use case, this would allow hierarchies of KVS data to be built up using another mechanism (such as through reductions), then added with one commit.

@grondo
Copy link
Contributor Author

grondo commented Aug 22, 2016

I wonder if maybe we need a new operation like

This is a great idea.

The more I think about the approach in this current PR the more I realize, though simple, it probably isn't feasible even for the proposed testing use case. The tricky bits are:

  • If the kvs object representation changes, the tests will immediately break. Though we could write a converter, or update any content store file committed to the testing repository, this probably isn't work we want to take on this point.
  • Committing a prepared sqlite content store to git isn't going to be easily mangeable since it will be a possibly large, binary file. A better approach here might be to have a new content store type just for testing with a text backend?

I will probably close this pr unless there is something we want to capture into issues first (@garlick's kvs_link enhancement?)

@garlick
Copy link
Member

garlick commented Aug 22, 2016

I'm less worried about your first point as I hope KVS metadata changes will be infrequent and deliberate, and the next change will introduce metadata versioning as described in RFC 11

On test content-stores - maybe the right way to go about this is to provide tools so a content store could be created quickly offline with scripts and then provided to kvs_link() or equivalent; then just check in the scripts?

I'm OK if you want to close this. I can create a kvs_link() issue that references the discussion. I'll keep this issue in mind as I work #776 and maybe this can pop out fairly quickly after that.

@garlick garlick mentioned this pull request Aug 22, 2016
@grondo
Copy link
Contributor Author

grondo commented Aug 22, 2016

@garlick, yes your idea for test content stores is perfect for my use case. Thanks!

@garlick
Copy link
Member

garlick commented Oct 23, 2018

Since this PR a few built-in tools have been added which make this problem easier to solve. It's possible to save/restore a KVS root (although the root cannot overwrite the current root):

$ flux kvs getroot >/tmp/kvsroot
$ flux kvs put --treeobj snap=$(cat /tmp/kvsroot)

It seems like adding a getroot to the rc3, within the PERSISTDIR conditional, as was proposed in this PR is a good idea. Then with the proposed changes to content-sqlite to allow it to open an existing db, it seems like we have the basis for some nice functionality for using a flux instance to examine a KVS snapshot from a previous instance!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants