Implement "repo rm-root" command to unlink the files API root. #4446

kevina · 2017-12-01T19:21:24Z

Closes #3934.

Note I used the new API because the old API seams to initialize an IpfsNode even if cmds.Context.ConstructNode() is not called. (Setting doesNotUseRepo and/or doesNotUseConfigAsInput did not seam to prevent the node initialization.)

whyrusleeping · 2017-12-03T17:11:57Z

cc @keks on the comment about commands above

whyrusleeping · 2017-12-03T17:31:44Z

core/commands/repo.go

+'ipfs repo rm-root' will unlink the root used by the files API ('ipfs
+files' commands) without trying to read the root itself.  The root and
+its children will then be removed by the garbage collector unless
+pinned.  This command can only run when no ipfs daemons are running.


maybe when no ipfs daemon is running

I just copied and pasted the text from ipfs repo fsck. But will fix.

whyrusleeping · 2017-12-03T17:34:12Z

core/commands/repo.go

+			cmds.EmitOnce(res, &MessageOutput{"Files API root not found.\n"})
+		default:
+			c, err := cid.Cast(val.([]byte))
+			cidStr := ""


i would use var cidStr string and declare it before calling cid.Cast

whyrusleeping · 2017-12-03T17:34:55Z

core/commands/repo.go

+			c, err := cid.Cast(val.([]byte))
+			cidStr := ""
+			if err != nil {
+				cidStr = c.String()


if the err isnt empty, we probably shouldnt call .String() on the nil cid it returned

Oops. That was suppose to be if err == nil.

whyrusleeping · 2017-12-03T17:37:32Z

How does the daemon function when the files root is deleted? Does it just assume an empty root and start over?

Also, we should be very clear about why/when you would ever use this command in the helptext. Its basically only for extremely unfortunate circumstances.

kevina · 2017-12-03T17:41:25Z

How does the daemon function when the files root is deleted? Does it just assume an empty root and start over?

Yes. It just assumes an empty root and start over.

kevina · 2017-12-03T17:41:58Z

Also, we should be very clear about why/when you would ever use this command in the helptext. Its basically only for extremely unfortunate circumstances.

I am horrible at the wording for these things. Suggestions welcome.

kevina · 2017-12-04T04:56:45Z

Also, we should be very clear about why/when you would ever use this command in the helptext. Its basically only for extremely unfortunate circumstances.

I added some text to the help text. Let me know if its what you are looking for. Suggestions welcome.

Kubuxu · 2017-12-04T18:00:54Z

This is good command to have a manual confirmation (Y from stdin stream) with flag override but it might be hard to do with how currently the API works.

whyrusleeping · 2018-05-30T03:29:17Z

@Kubuxu @magik6k wanna review this one?

It would probably be good to have manual confirmation as suggested, but maybe for the user to run it with a '--confirm' flag? Don't want to have to figure out how to do an interactive prompt as part of the changeset.

magik6k

Something like --confirm would be nice. We could also check if the root node is present in the datastore and notify the user if it is (and possibly check if it is corrupted).

core/commands/repo.go

whyrusleeping · 2018-05-30T10:14:06Z

Yeah, I think some safety features might be good. Perhaps:

> ipfs repo rm-root
Error: this is a potentially dangerous operation please pass --confirm to proceed
> ipfs repo rm-root --confirm
Error: the root of your mfs directory exists locally. Are you sure you want to unlink this? pass --remote-existing-root to continue

kevina · 2018-05-30T20:17:08Z

@whyrusleeping I added the options as you suggested.

Stebalien · 2018-06-19T22:47:39Z

core/commands/repo.go

+	},
+}
+
+var repoRmRootCmd = &cmds.Command{


Doesn't this belong in the ipfs files command tree? Also, I'd just call it ipfs files chroot and allow the user to change the root arbitrarily.

However, I may just be missing some context here.

We could even make it: ipfs files chroot --save /oldroot /newroot to allow saving the old root inside the new root. Actually, we might just want to do that by default to make this command less safer.

Doesn't this belong in the ipfs files command tree?

@Stebalien this is meant to be a low level tool to deal with the case when the root node some how gets deleted and is not available anywhere. It is in repo because it a low level command. Like repo fsck or repo verify.

ipfs files chroot is an interesting idea, but more involved.

I see. So it's just a "last-ditch" fix my repo command. Got it.

We may want something like this for pin root too, as it can be useful in similar situations

Stebalien · 2018-06-19T22:51:29Z

core/commands/repo.go

+
+		// Can't use a full node as that interferes with the removal
+		// of the files root, so open the repo directly
+		repo, err := fsrepo.Open(configRoot)


That's... unfortunate. I run my daemon as a separate user. I'm fine with this as a temporary limitation, but we should fix it eventually.

Stebalien

Two nits

Stebalien · 2018-06-19T23:28:27Z

core/commands/repo.go

+			res.SetError(fmt.Errorf("root %s exists locally. Are you sure you want to unlink this? Pass --remove-local-root to continue", cidStr), cmdkit.ErrNormal)
+			return
+		} else if !have && removeLocalRoot {
+			res.SetError(fmt.Errorf("root does not %s exists locally. Please remove --remove-local-root to continue", cidStr), cmdkit.ErrNormal)


The documentation says "even if" not "only if" so I'd expect this case to work.

I can fix the documentation. The idea behind this is to prevent this flag from automatically being passed. It is intentionally annoying. if this will Create a problem I can remove this check.

Reviving...

IMO, having to pass --confirm is annoying enough. If a user wants to automate this, that's their problem (and their choice).

I've switched this to act as documented unless someone objects.

Stebalien · 2018-06-19T23:29:38Z

core/core.go

@@ -743,8 +743,11 @@ func (n *IpfsNode) loadBootstrapPeers() ([]pstore.PeerInfo, error) {
 	return toPeerInfos(parsed), nil
 }

+// FilesRootKey returns the datastore key for the files root
+func FilesRootKey() ds.Key { return ds.NewKey("/local/filesroot") }


I'd just make this a variable. I.e., var FilesRootKey ds.Key = ds.NewKey(...)

Stebalien · 2019-03-02T01:26:36Z

Rebased.

Closes #3934. License: MIT Signed-off-by: Kevin Atkinson <k@kevina.org>

License: MIT Signed-off-by: Steven Allen <steven@stebalien.com>

…cumentation This command is already annoying enough. If a user decides to automate this, that's their issue (and we should allow it). (also rename back to remove-existing-root from remove-local-root) License: MIT Signed-off-by: Steven Allen <steven@stebalien.com>

core/commands/repo.go

magik6k · 2019-03-02T02:36:56Z

core/commands/repo.go

+
+		err = repo.Datastore().Delete(core.FilesRootKey)
+		if err != nil {
+			return fmt.Errorf("unable to remove API root: %s.  Root hash was %s", err.Error(), cidStr)


API Sounds a bit confusing here IMO

magik6k · 2019-03-02T02:39:30Z

core/commands/repo.go

+	},
+}
+
+var repoRmRootCmd = &cmds.Command{


We may want something like this for pin root too, as it can be useful in similar situations

Stebalien · 2019-03-04T21:15:07Z

@magik6k I've renamed this to ipfs rm-files-root and improved the wording a bit.

License: MIT Signed-off-by: Steven Allen <steven@stebalien.com>

magik6k · 2019-03-05T11:58:48Z

Sharness seems to have failed on this: https://circleci.com/gh/ipfs/go-ipfs/14343?utm_campaign=vcs-integration-link&utm_medium=referral&utm_source=github-build-link

ghost assigned kevina Dec 1, 2017

ghost added the status/in-progress In progress label Dec 1, 2017

kevina mentioned this pull request Dec 1, 2017

IPFS Files root not avialable fix in 'ipfs repo fsck' #3934

Open

whyrusleeping reviewed Dec 3, 2017

View reviewed changes

kevina changed the title ~~Implement "repo rm-root" command to unlike the files API root.~~ Implement "repo rm-root" command to unlink the files API root. Dec 3, 2017

whyrusleeping requested review from Stebalien, Kubuxu and magik6k May 30, 2018 03:28

magik6k reviewed May 30, 2018

View reviewed changes

core/commands/repo.go Outdated Show resolved Hide resolved

kevina force-pushed the kevina/repo-recreate-root branch 2 times, most recently from 8a9a19b to a3c720e Compare May 30, 2018 19:35

kevina force-pushed the kevina/repo-recreate-root branch 2 times, most recently from ec87ec7 to 5ef87c2 Compare June 4, 2018 07:08

Stebalien reviewed Jun 19, 2018

View reviewed changes

Stebalien force-pushed the kevina/repo-recreate-root branch from 5ef87c2 to 089b8e8 Compare March 2, 2019 01:25

ghost assigned Stebalien Mar 2, 2019

Stebalien requested a review from magik6k March 2, 2019 01:28

Stebalien added the need/review Needs a review label Mar 2, 2019

kevina and others added 2 commits March 1, 2019 17:30

Implement "repo rm-root" command to unlike the files API root.

856b1b4

Closes #3934. License: MIT Signed-off-by: Kevin Atkinson <k@kevina.org>

cmds(repo rm-root): use a variable for FilesRootKey

03e79f4

License: MIT Signed-off-by: Steven Allen <steven@stebalien.com>

Stebalien force-pushed the kevina/repo-recreate-root branch from 089b8e8 to 4ce7273 Compare March 2, 2019 01:30

Stebalien force-pushed the kevina/repo-recreate-root branch from 4ce7273 to dc303d0 Compare March 2, 2019 01:31

magik6k reviewed Mar 2, 2019

View reviewed changes

Stebalien force-pushed the kevina/repo-recreate-root branch from 05aaf0c to 6c2b4c1 Compare March 4, 2019 21:14

Stebalien requested a review from magik6k March 4, 2019 21:15

cmds(ipfs rm-files-root): improve language around rm-root command

784e0d8

License: MIT Signed-off-by: Steven Allen <steven@stebalien.com>

Stebalien force-pushed the kevina/repo-recreate-root branch from 6c2b4c1 to 784e0d8 Compare March 5, 2019 00:03

momack2 added this to In Progress in ipfs/go-ipfs May 9, 2019

Stebalien unassigned Stebalien and kevina Apr 22, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement "repo rm-root" command to unlink the files API root. #4446

Implement "repo rm-root" command to unlink the files API root. #4446

Implement "repo rm-root" command to unlink the files API root. #4446

Are you sure you want to change the base?

Implement "repo rm-root" command to unlink the files API root. #4446

Conversation

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

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