add 'ipfs repo migrate' command by zaibon · Pull Request #7658 · ipfs/kubo

zaibon · 2020-09-07T18:59:29Z

this command allows to run the repo migration without starting the daemon.

resolves #7471

aschmahmann · 2020-09-08T18:09:45Z

Thanks @zaibon for starting this. Could you do two more things:

As specified in the issue you linked we need an --allow-downgrade flag. While this command has one it's not hooked up to anything.
There is nothing testing that this command works. You can modify https://github.com/ipfs/go-ipfs/blob/83c57eda1fe4c50660761b401b4e4f016e6ecfd0/test/sharness/t0066-migration.sh to also test that ipfs repo migrate and ipfs repo migrate --allow-downgrade work as expected

Thanks again and let me know if you need any guidance here.

zaibon · 2020-09-09T07:13:32Z

Thanks for the review @aschmahmann.

As specified in the issue you linked we need an --allow-downgrade flag. While this command has one it's not hooked up to anything.

Right, forgot that part. What should be the behavior when this flag is set ? At the moment I always call migrate.RunMigration(fsrepo.RepoVersion) should I read the desired version from somewhere else and then compare the current version with the desired version and only allow downgrade if the flag is set ?

There is nothing testing that this command works. You can modify https://github.com/ipfs/go-ipfs/blob/83c57eda1fe4c50660761b401b4e4f016e6ecfd0/test/sharness/t0066-migration.sh to also test that ipfs repo migrate and ipfs repo migrate --allow-downgrade work as expected

Yep, will do, thanks for the guidance here.

zaibon · 2020-09-09T11:00:24Z

  mkdir bin &&
  echo "#!/bin/bash" > bin/fs-repo-migrations &&
-  echo "echo 5" >> bin/fs-repo-migrations &&
+  echo "echo 10" >> bin/fs-repo-migrations &&


Had to change this here to make the detection of the fs-repo-migrations to work since it seems now the code also checks the version the migration tool is able to handle.

zaibon · 2020-09-14T13:21:37Z

@aschmahmann I hooked up the allow-downgrade flag. For this I had to add an extra argument to the RunMigration method. Not sure this is ok, I'm usually not a big fan of boolean argument. What's thought about it ?

I also added some test for the new command. I'm not sure how I can test the downgrade though, some input there would be welcome :-)

aschmahmann · 2020-09-18T17:26:33Z

@zaibon sorry it's taking a while to get to this. A little busy with things related to the v0.7.0 release, but will try and get to this next week.

zaibon · 2020-11-13T09:26:52Z

@aschmahmann ping ;-)

aschmahmann · 2020-11-13T15:13:47Z

Thanks @zaibon for the reminder and apologies it slipped off my radar. I'll try and take a look over the weekend.

I was actually thinking about this PR yesterday because some of the pain around migrations has made implementing something like ipfs/fs-repo-migrations#98 more important. This would likely rely on ipfs repo migrate instead of a separate fs-repo-migrations binary which would make this command even more useful (and make testing it simpler too).

zaibon · 2020-11-13T15:16:58Z

Nice, I'll have a look at ipfs/fs-repo-migrations#98.

Should I already try to rebase this PR since I guess it's a bit out of date with master or wait your review ?

aschmahmann · 2020-11-13T20:36:03Z

Should I already try to rebase this PR since I guess it's a bit out of date with master or wait your review

I suspect it's an easy rebase, but if it's a pain for some reason then I'd hold off until there's some progress on ipfs/fs-repo-migrations#98. If we do basically what's recommended in that issue then this command will replace the fs-repo-migrations binary for people doing manual migration manipulation.

gammazero · 2021-01-22T03:10:18Z

@zaibon Wanted to let you know that this PR has not been forgotten about. The work to overhaul the migrations and binary distribution is code-complete, and is waiting until there is time to review it all. Once #7857 is merged, then you can rebase this PR and make any necessary changes. Hopefully in the next couple of weeks.

zaibon · 2021-04-02T08:41:08Z

@gammazero seems ipfs/fs-repo-migrations#98 is now closed. Can I start rebasing and make the required change here or is there anything else in the pipeline ?

gammazero · 2021-04-02T09:01:47Z

@zaibon You can rebase now. You will need to make some changes to how you call RunMigration. See here:
https://github.com/ipfs/go-ipfs/blob/master/cmd/ipfs/daemon.go#L291-L293

aschmahmann · 2021-04-05T15:19:14Z

@zaibon not sure if this is ready for re-review yet, but when you're ready just request review from me or tag me

gammazero · 2021-04-06T02:32:15Z

@zaibon I took a look at the changes to the sharness tests, and realized that what was there (before your changes) needs to be fixed. go-ipfs no longer uses fs-repo-migrations to perform migrations, and the tests were not doing what they were supposed to be doing.

Attn: @aschmahmann
I have created a PR #8053 to fix this. It includes comments explaining what is going on as well. Please rebase this PR as soon as it can be merged. (sorry for another roadblock).

zaibon · 2021-04-16T08:20:33Z

@aschmahmann I just rebased on top of #8053, so this PR should be ready now.

gammazero · 2021-04-16T18:29:30Z

 test_expect_success "output looks good" '
  grep "Found outdated fs-repo, starting migration." migrate_out > /dev/null &&
-  grep "Success: fs-repo has been migrated to version 10" migrate_out > /dev/null
+  grep "Success: fs-repo has been migrated to version 11" migrate_out > /dev/null


This looks like a duplicate of the test at line 72-74.

You may want to take this test from the current master branch and reapply your changes.

This test is up to date with master branch, all the new code is only related this me PR.

This line looks similar to 71-74, cause it does test the same thing but it is the result of another command. In 71-74 we test the result of the ipfs daemon command, while here we test the result of the ipfs migrate command.

gammazero · 2021-04-16T18:56:56Z

@aschmahmann With the recent changes to allow migrations over IPFS, and to import downloaded migrations, we probably want this PR to at least be able to fetch migrations via IPFS. This command would also benefit from being able to import downloaded migrations as CAR files.

aschmahmann · 2021-04-16T19:18:31Z

we probably want this PR to at least be able to fetch migrations via IPFS.

👍

This command would also benefit from being able to import downloaded migrations as CAR files.

We get this basically for free if we can fetch the migrations using IPFS then if the user really wants to work with CAR files they can use ipfs dag import <migration car> before they run ipfs repo migrate and that'll do the job.

zaibon · 2021-05-24T08:00:23Z

@aschmahmann Do you still need some action from me on this ?

zaibon · 2021-08-13T15:31:12Z

@BigLep done.

Matchjuan859 · 2021-08-13T18:29:02Z

am not sure wwhATA U NED ME TO DUE

…

On Fri, Aug 13, 2021 at 11:31 AM Christophe de Carvalho < ***@***.***> wrote: @BigLep <https://github.com/BigLep> done. — You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub <#7658 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AOB2WPNRT4SA7T5CANRR4JTT4U3EXANCNFSM4Q6YG46Q> . Triage notifications on the go with GitHub Mobile for iOS <https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675> or Android <https://play.google.com/store/apps/details?id=com.github.android&utm_campaign=notification-email> .

gammazero · 2021-08-14T00:13:12Z

I took care of step 3 above to allow this command to use IPFS to download migrations.

BigLep · 2021-08-14T00:56:26Z

Arg, and not it looks like CI is not passing. I have limited internet bandwidth to track it down, but I believe I sawt his mentioned somewhere else. @aschumann is there another issue we can track against so can then merge this?

BigLep · 2021-08-20T15:25:29Z

@gammazero : can you please rebase on master and see if it's good to go?

BigLep · 2021-08-27T15:19:35Z

@gammazero : just checking in - are you able to get this across the line?

gammazero · 2021-09-07T19:06:38Z

@aschmahmann @BigLep As far as I can tell, this PR is done. The problem is that it does not have privileges to run the sharness tests using a 2xlarge instance as configured in the .circleci/config.yml here, and this is causing failure.

this command allows to run the repo migration without starting the daemon. resolves ipfs#7471

Additional change to remove leftover debug output from ipfsfetcher.go noticed during testing.

This PR replaces #7658 that was originally contributed by zaibons, in order to move code into a branch and avoid some CI problem. The command allows the user to run the repo migration without starting the daemon. resolves #7471

gammazero · 2021-09-10T18:57:38Z

Closing: This PR is replaced by #8428 to move it into a branch to avoid CI problems associated with the repo.

This PR replaces #7658 that was originally contributed by zaibons, in order to move code into a branch and avoid some CI problem. The command allows the user to run the repo migration without starting the daemon. resolves #7471

* Add 'ipfs repo migrate' command This PR replaces #7658 that was originally contributed by zaibons, in order to move code into a branch and avoid some CI problem. The command allows the user to run the repo migration without starting the daemon. resolves #7471 * return non-ErrNeedMigration errors from fsrepo.Open() Co-authored-by: Gus Eggert <gus@gus.dev>

This comment has been minimized.

Sign in to view

zaibon commented Sep 9, 2020

View reviewed changes

zaibon force-pushed the feat/cmds/repo-migrate branch from 54b4019 to f2e7133 Compare September 14, 2020 06:24

zaibon force-pushed the feat/cmds/repo-migrate branch from f2e7133 to 6499f36 Compare November 13, 2020 21:07

BigLep added the status/blocked Unable to be worked further until needs are met label Mar 29, 2021

zaibon force-pushed the feat/cmds/repo-migrate branch from 6499f36 to 2455742 Compare April 3, 2021 17:23

christophedcpm force-pushed the feat/cmds/repo-migrate branch from 2455742 to 82e1efb Compare April 6, 2021 07:42

zaibon force-pushed the feat/cmds/repo-migrate branch from 82e1efb to cc7e14f Compare April 16, 2021 08:12

gammazero requested changes Apr 16, 2021

View reviewed changes

gammazero mentioned this pull request Apr 16, 2021

Feat/migration ipfs download #8064

Merged

aschmahmann added need/author-input Needs input from the original author and removed status/blocked Unable to be worked further until needs are met labels Apr 19, 2021

aschmahmann self-assigned this May 24, 2021

BigLep added need/author-input Needs input from the original author and removed status/blocked Unable to be worked further until needs are met labels Aug 13, 2021

christophedcpm force-pushed the feat/cmds/repo-migrate branch from 67960ec to 3a94ced Compare August 13, 2021 15:16

BigLep removed the need/author-input Needs input from the original author label Aug 27, 2021

gammazero force-pushed the feat/cmds/repo-migrate branch from eca5f20 to 59a418b Compare September 3, 2021 22:33

zaibon and others added 8 commits September 10, 2021 05:30

add 'ipfs repo migrate' command

3af1e46

this command allows to run the repo migration without starting the daemon. resolves ipfs#7471

add tests for 'ipfs repo migrate' command

395ad25

add 'allow-downgrade' flag to ipfs repo migrate command

c480e68

use new code to start a repo migration

47ce267

fix shareness tests

73b349d

use $IPFS_REPO_VER variable in tests instead of hardcoded version

efd33fb

Allow repo migrate command to use IPFS for migration download

c05a2b4

Additional change to remove leftover debug output from ipfsfetcher.go noticed during testing.

Increase linter timeout

647cb41

gammazero force-pushed the feat/cmds/repo-migrate branch from f823d83 to 647cb41 Compare September 10, 2021 12:30

BigLep assigned gammazero Sep 10, 2021

gammazero mentioned this pull request Sep 10, 2021

Add 'ipfs repo migrate' command #8428

Merged

gammazero closed this Sep 10, 2021

Uh oh!

Conversation

zaibon commented Sep 7, 2020

Uh oh!

This comment has been minimized.

aschmahmann commented Sep 8, 2020

Uh oh!

zaibon commented Sep 9, 2020

Uh oh!

zaibon Sep 9, 2020

Choose a reason for hiding this comment

Uh oh!

zaibon commented Sep 14, 2020

Uh oh!

aschmahmann commented Sep 18, 2020

Uh oh!

zaibon commented Nov 13, 2020

Uh oh!

aschmahmann commented Nov 13, 2020

Uh oh!

zaibon commented Nov 13, 2020

Uh oh!

aschmahmann commented Nov 13, 2020

Uh oh!

gammazero commented Jan 22, 2021

Uh oh!

zaibon commented Apr 2, 2021

Uh oh!

gammazero commented Apr 2, 2021

Uh oh!

aschmahmann commented Apr 5, 2021

Uh oh!

gammazero commented Apr 6, 2021

Uh oh!

zaibon commented Apr 16, 2021

Uh oh!

gammazero Apr 16, 2021

Choose a reason for hiding this comment

Uh oh!

zaibon May 16, 2021

Choose a reason for hiding this comment

Uh oh!

Uh oh!

gammazero commented Apr 16, 2021

Uh oh!

aschmahmann commented Apr 16, 2021

Uh oh!

zaibon commented May 24, 2021

Uh oh!

zaibon commented Aug 13, 2021

Uh oh!

Matchjuan859 commented Aug 13, 2021 via email

Uh oh!

gammazero commented Aug 14, 2021

Uh oh!

BigLep commented Aug 14, 2021

Uh oh!

BigLep commented Aug 20, 2021

Uh oh!

BigLep commented Aug 27, 2021

Uh oh!

gammazero commented Sep 7, 2021

Uh oh!

gammazero commented Sep 10, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants