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

feat(cmds): files: add new-root command to change the MFS root #8648

Draft
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

schomatis
Copy link
Contributor

@schomatis schomatis commented Jan 10, 2022

Some minor fixes pending but it should get a review at this point.

(Sharness test also pending.)

Connected to #7183.
Closes #8100.

@schomatis schomatis self-assigned this Jan 10, 2022
@nlko
Copy link

nlko commented Jan 10, 2022

Hello,

I don't have the necessary knowledge to analyse this PR but since I'm the author of #7183, my question is : will it be possible to use this command without the daemon running ?

In my case the root MFS corresponding file was missing in the blocks folder at startup and it prevented the daemon to start. Hence the need to have a replace-root working without daemon.

@bqv
Copy link

bqv commented Jan 10, 2022

will it be possible to use this command without the daemon running ?

As written now, yes

@schomatis
Copy link
Contributor Author

Correct, thanks.

Right now the idea is for this command to be used when the MFS filesystem is corrupted and the daemon fails to start (either hangs or refuses) and you need to run this standalone to fix it. If you run it alongside the daemon it will fail as it will try to get a lock on the repo which is already in use. Will add some more documentation around that.

"rm": filesRmCmd,
"flush": filesFlushCmd,
"chcid": filesChcidCmd,
"replace-root": filesReplaceRoot,
Copy link
Member

@lidel lidel Jan 11, 2022

Choose a reason for hiding this comment

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

💭 loose thoughts

  • since we already use unix terms like mkdir mv cp rm, perhaps this one should be named chroot ?
  • another idea: perhaps we could add a second (optional) argument to already existing chcid, which would allow in-place swapping of CIDs for any path (not just root)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

since we already use unix terms like mkdir mv cp rm, perhaps this one should be named chroot ?

The current name came from an informal Slack thread, but I'm fine with whatever is decided. My 2 cents on chroot is that it might imply some temporary change related to the running daemon/process when in fact we're doing a long-term modification of the single filesystem available (since we don't have any jail/sandbox abstractions).

another idea: perhaps we could add a second (optional) argument to already existing chcid, which would allow in-place swapping of CIDs for any path (not just root)

This sounds like it is beyond the scope of this issue which is fixing a corrupted filesystem preventing daemon startup (corruption in sub-paths wouldn't have the same effect). The idea is valid on itself though, maybe we should discuss it in another issue as a feature request.

@BigLep
Copy link
Contributor

BigLep commented Mar 3, 2022

@schomatis : what are the next steps here?

@BigLep BigLep removed the request for review from hsanjuan March 3, 2022 13:39
@BigLep BigLep added this to the Best Effort Track milestone Mar 3, 2022
@schomatis
Copy link
Contributor Author

@BigLep As noted in the OP: awaiting review.

@bqv
Copy link

bqv commented Mar 3, 2022

@BigLep As noted in the OP: awaiting review.

Perhaps @hsanjuan is busy, @schomatis perhaps you could review in lieu?

@schomatis
Copy link
Contributor Author

No, I can't review my own PR.

@bqv
Copy link

bqv commented Mar 3, 2022

No, I can't review my own PR.

Sorry I meant @BigLep!

@BigLep BigLep marked this pull request as ready for review March 4, 2022 01:25
@BigLep
Copy link
Contributor

BigLep commented Mar 4, 2022

@schomatis : ACK. I was confused because the PR was left as draft. I have marked it as ready for review.

@schomatis
Copy link
Contributor Author

@BigLep Ok, I see the confusion. The 'Ready for review' button is very misleading. It should actually be 'Ready for merge'. The draft state is a way to signal a WIP PR to make sure you won't accidentally merge it as is. If the IPFS team interprets drafts differently please let me know and I'll adjust my signalling.

@lidel lidel requested a review from a team as a code owner October 3, 2024 20:13
Copy link
Member

@lidel lidel left a comment

Choose a reason for hiding this comment

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

Updated to latest master and resolved conflicts.

Needs final polish:

  • agree on name (maybe set-root?)
  • remove / resolve TODOs inline
  • add new command to changelog
  • tests pass, CI green

It is an advanced command that you normally do *not* want to run except when
the filesystem has been corrupted and the daemon refuses to run.

FIXME: Add an example of the daemon not running once https://github.com/ipfs/go-ipfs/issues/7183
Copy link
Member

Choose a reason for hiding this comment

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

TODO

[nothing; empty dir]
$ ipfs files stat / --hash
QmUNLLsPACCz1vLxQVkXqqLX5R1X345qqfHbsf67hvA3Nn
# FIXME(BLOCKING): Need the following to somehow "start" the root dir, otherwise
Copy link
Member

Choose a reason for hiding this comment

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

TODO

Arguments: []cmds.Argument{
cmds.StringArg("new-root", true, false, "New root to use."),
},
// FIXME(BLOCKING): Can/should we do this with the repo running?
Copy link
Member

Choose a reason for hiding this comment

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

TODO

Comment on lines +1254 to +1255
// FIXME(BLOCKING): Check (a) that this CID exists *locally* and (b)
// that it's a dir.
Copy link
Member

Choose a reason for hiding this comment

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

TODO

Comment on lines +1289 to +1290
// FIXME(BLOCKING): Do we need this if we're closing the repo at the end
// of the command? Likely not.
Copy link
Member

Choose a reason for hiding this comment

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

TODO

@lidel lidel marked this pull request as draft October 3, 2024 20:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
No open projects
Status: 🔎 In Review
Development

Successfully merging this pull request may close these issues.

Add ability to update the local root MFS CID?
5 participants