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

fsrepo migrations #950

Merged
merged 15 commits into from
Apr 20, 2015
Merged

fsrepo migrations #950

merged 15 commits into from
Apr 20, 2015

Conversation

jbenet
Copy link
Member

@jbenet jbenet commented Mar 20, 2015

This PR adds migrations for fsrepo.

The 1-to-2 isn't done yet and will be the meat of this PR. Need

@jbenet
Copy link
Member Author

jbenet commented Mar 20, 2015

@whyrusleeping not sure how you want to store things in keyspec, but is the layout in https://github.com/ipfs/specs/tree/master/repo/fs-repo#keys work for you? note the .{pub, pri, sym} endings.

@whyrusleeping
Copy link
Member

@jbenet thats a little non-standard (i was going for ssh-compliant naming and formats) but I can work with it.

@jbenet
Copy link
Member Author

jbenet commented Apr 1, 2015

see discussion in #975 (comment)

@jbenet jbenet added codereview and removed backlog labels Apr 1, 2015
@whyrusleeping
Copy link
Member

@jbenet why do you have a binary commited?

@jbenet
Copy link
Member Author

jbenet commented Apr 1, 2015

@whyrusleeping because i fail. thanks

  • remove 0-to-1 binary

@whyrusleeping
Copy link
Member

lets make sure it gets GCed out! 2MB makes a difference to new clones

@whyrusleeping
Copy link
Member

this looks good to me, i like the approach.

@whyrusleeping
Copy link
Member

@jbenet should the daemon check the repo version on startup, and run the appropriate migration tools? (or, prompt the user to run them)

@jbenet
Copy link
Member Author

jbenet commented Apr 7, 2015

@whyrusleeping for now prompt the user to run them. we can do things automatically later, but if something goes wrong for now, would be ideal to have the user on top of it

@whyrusleeping
Copy link
Member

im not convinced we should have a tool for each migration, i think a single tool would be the better approach

@jbenet
Copy link
Member Author

jbenet commented Apr 8, 2015

Need to make sure we can rest assured these will work years from now without our maintenance. The repo's code will progress, often change fundamentally. Forcing us to maintain things backwards compatible with all repo versions -- or even one tool -- is signing us up for pain in the future, testing with old things, etc. Instead, if we freeze its code now, with its own vendoring, we can count on these working with the current repo versions down the road.

I'm envisioning a pretty simple setup where:

fs-repo-migrations/
├── 0-to-1/
│   ├── all_the_code_needed_for_0-to-1/
│   └── main.go
├── 1-to-2/
│   ├── all_the_code_needed_for_1-to-2/
│   └── main.go
└── 2-to-3/
    ├── all_the_code_needed_for_2-to-3/
    └── main.go

so we can go into any of these, compile and know it will work.

depending on how we do things, we may not need to use godeps, we could just vendor directly (cp -r) what we need into those directories.

vers := string(ver)[:1]

if vers != RepoVersion {
return nil, fmt.Errorf("Repo has incorrect version: '%s'\nProgram version is: '%s'\nPlease run the appropriate migration tool before continuing",
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

var MsgUpgradeRepo = `Repo has incorrect version: %s
Program version is: %s
Please run the repo migration tool before continuing:

<cmd for them to run>

`

I'll think about what a good way to do <cmd for them to run> is. the problem is they:

  • may not have it installed (installed ipfs as a binary).
  • may not have go installed (may not be able to compile).
  • do not have ipfs installed (cannot ipfs cat <hash>/<arch>/1-to-2 >1-to-2)

May be able to do something like:

wget http://gateway.ipfs.io/ipfs/<hash>/<arch>/1-to-2
chmod +x 1-to-2
./1-to-2

And ship a little shell script that does it as:

ipfs-repo-migrate 1-to-2

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

go 1.5 is coming at such a good moment...

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we could at least just have a page somewhere discussing migrations, maybe a github repo, and link them to it in the error message

@jbenet
Copy link
Member Author

jbenet commented Apr 9, 2015

@whyrusleeping LGTM!! I'll write the script to download the migrations for us.

// _before_ acquiring the daemon lock so the user gets an appropriate error
// message.
// NB: It's safe to read the config without the daemon lock, but not safe
// to write.
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@whyrusleeping will this still yield the right error?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the errors returned may now differ, but they actually mean something now. the other error was completely worthless

@jbenet
Copy link
Member Author

jbenet commented Apr 10, 2015

@whyrusleeping the new changes may alter the semantics of ipfs startup. are the right errors still gotten?

I'll start full tests manually.

@jbenet
Copy link
Member Author

jbenet commented Apr 10, 2015

Every commit passed for me. And i think for @whyrusleeping as well.

@jbenet
Copy link
Member Author

jbenet commented Apr 10, 2015

i tried to avoid using go-ipfs code when i could (minus a few repo imports) to make this easier.

+1. We should probably vendor those just in case :| -- we dont have to use godeps. we can just copy-paste. (:open_mouth:!)

@whyrusleeping
Copy link
Member

im fine with copy pasting, especially since we are 'vendoring' with the explicit goal of never changing that code

@jbenet
Copy link
Member Author

jbenet commented Apr 12, 2015

@whyrusleeping what's missing here? just the guide to upgrade?

@whyrusleeping
Copy link
Member

Just the guide.

@jbenet
Copy link
Member Author

jbenet commented Apr 18, 2015

Migrations CR

  • error needs to be friendlier

now:

Error: ipfs repo found in old '.go-ipfs' location, please run migration tool

should be something like:

Error: ipfs repo found in old '.go-ipfs' location, please run migration tool:

    wget http://gateway.ipfs.io/ipfs/<hash>/<go-platform>/ipfs-fsrepo-migrations
    chmod +x ipfs-fsrepo-migrations
    ./ipfs-fsrepo-migrations

If you encounter problems, please see https://github.com/ipfs/fs-repo-migrations/blob/master/run.md

This should work on every machine with as few steps as possible. We could detect if Go is installed, and suggest using go get, but this should only be if they do have it installed... we should not have any go get crap if the machine doesn't have go. We could also use a shell script and pipe straight into shell, but they dont have hashpipe installed and i dont want to do that if they don't.

It's really funny, if ipfs worked during this process, or could install things from it, this would be so easy. lol 🐔 🥚

Hmm. maybe it can be ok to say:

Error: ipfs repo found in old '.go-ipfs' location, please run migration tool.
Please see https://github.com/ipfs/fs-repo-migrations/blob/master/run.md

if we want to make it easy for ourselves this round.


@jbenet
Copy link
Member Author

jbenet commented Apr 19, 2015

On the message, maybe we can say:

Error: ipfs repo found in old '.go-ipfs' location, please run migration tool.
Please see https://github.com/ipfs/fs-repo-migrations/blob/master/run.md
Sorry for the inconvenience. In the future, these will run automatically.

@jbenet
Copy link
Member Author

jbenet commented Apr 20, 2015

  • mv daemon.lock -> repo.lock

jbenet and others added 15 commits April 20, 2015 02:20
This allows replacing the datastore without needing to write Close
through to every wrapped datastore.
WARNING: No migration performed! That needs to come in a separate
commit, perhaps amended into this one.

Migration must move keyspace "/b" from leveldb to the flatfs subdir,
while removing the "b" prefix (keys should start with just "/").
This changes .go-ipfs to .ipfs everywhere.
And by the way this defines a DefaultPathName const
for this name.

License: MIT
Signed-off-by: Christian Couder <chriscool@tuxfamily.org>
Improved the repo migration errors to provide instructions
to the user.
The "daemon.lock" was really a repo.lock, as the cli also took it
and the purpose was any process mutex. This is part of the 1-to-2
migration, and has already been handled in
https://github.com/ipfs/fs-repo-migrations/tree/master/ipfs-1-to-2
@jbenet
Copy link
Member Author

jbenet commented Apr 20, 2015

rebased on new master

jbenet added a commit that referenced this pull request Apr 20, 2015
@jbenet jbenet merged commit 7e887b9 into master Apr 20, 2015
@jbenet jbenet removed the codereview label Apr 20, 2015
@jbenet jbenet deleted the migrations branch April 20, 2015 10:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants