handle max storage when gc is disabled #5768

kjzz · 2018-11-12T12:44:21Z

feature #5140

License: MIT
Signed-off-by: Kejie Zhang 601172892@qq.com

schomatis · 2018-11-20T03:26:42Z

@kjzz Can I trouble you with a brief description of the PR please? Just a couple of sentences explaining what did you wanted to achieve and how, thanks!

kjzz · 2018-11-20T04:26:45Z

Hi @schomatis ， this pr is aimed to solve the problem that when a repo size limit is configured bug gc is disabled (i.e., no --enable-gc flag has been passed), I think we should have multiple levels of warnings:

1.warning when datastore usage is at 90%
2.non fatal error when datastore usage is at 95%
3.fatal error, preventing startup when datastore usage is at 99%

you can also see the discussion in issue #5140 .

schomatis · 2018-11-20T13:17:22Z

core/coreapi/unixfs.go

@@ -24,6 +25,10 @@ import (
 	mfs "gx/ipfs/QmcUXFi2Fp7oguoFT81f2poJpnb44dFkZanQhDBHMoYyG9/go-mfs"
 )

+var (
+	errFullStorage = errors.New("DataStorage is full,please enable gc and restart node")


nit: add a space after the "," (same for the log errors below)

schomatis · 2018-11-20T13:24:12Z

core/coreapi/unixfs.go

@@ -126,6 +131,23 @@ func (api *UnixfsAPI) Add(ctx context.Context, files files.File, opts ...options
 		fileAdder.SetMfsRoot(mr)
 	}

+	// Get storageMax when node is online.


Is Add the only entry point that will make the repo grow?

No. Unfortunately, this really needs to go much lower. We probably need some kind of datastore wrapper that rejects writes when the datastore is full (and calls a user-provided "free" function at N% of full).

schomatis · 2018-11-20T13:25:21Z

core/coreapi/unixfs.go

@@ -126,6 +131,23 @@ func (api *UnixfsAPI) Add(ctx context.Context, files files.File, opts ...options
 		fileAdder.SetMfsRoot(mr)
 	}

+	// Get storageMax when node is online.
+	if fileAdder.Pin && !n.EnableGC && n.OnlineMode() {


Do we need to check for Pin here? the file will be added anyway even if we don't pin it, right? (it can be removed later but it will make the repo grow)

schomatis · 2018-11-20T13:26:29Z

core/coreapi/unixfs.go

@@ -126,6 +131,23 @@ func (api *UnixfsAPI) Add(ctx context.Context, files files.File, opts ...options
 		fileAdder.SetMfsRoot(mr)
 	}

+	// Get storageMax when node is online.
+	if fileAdder.Pin && !n.EnableGC && n.OnlineMode() {
+		storageUsage, err := n.Repo.GetStorageUsage()


Is GetStorageUsage a faithful metric of repo size? Is it compatible with StorageMax? (this is a naive question but I'm not familiar with the repo API)

In my testing , maybe GetStorageUsage is a faithful metric of repo size.

schomatis · 2018-11-20T13:31:35Z

cmd/ipfs/daemon.go

+		return nil, err
+	}
+	if !gc.EnableGC {
+		log.Warningf("GC: %s", "Repo GC is disabled.")


Why do we need this warning? Is it part of the original intention of the PR?

Is it part of the original intention of the PR?

We should print some warning message when the gc is disable.
You can see the comment in the issue

schomatis · 2018-11-20T13:33:16Z

core/corerepo/gc.go

@@ -23,10 +23,12 @@ var ErrMaxStorageExceeded = errors.New("maximum storage limit exceeded. Try to u
 type GC struct {
 	Node       *core.IpfsNode
 	Repo       repo.Repo
+	EnableGC   bool


Why do we need this second EnableGC?

Because I need to get this param in func maybeRunGC(req *cmds.Request, node *core.IpfsNode) (<-chan error, error) .

schomatis · 2018-11-20T13:35:53Z

cmd/ipfs/daemon.go

+	if err != nil {
+		return nil, err
+	}
+	gc.EnableGC = enableGC


Let's keep enableGC set-value and load-value operations as close as possible, move the enableGC, _ := req.Options[enableGCKwd].(bool) line down here if it's not used anywhere else.

Because i should get this param in other function.

func maybeRunGC(req *cmds.Request, node *core.IpfsNode) (<-chan error, error) { gc, err := initGC(req, node) if err != nil { return nil, err } if !gc.EnableGC { log.Warningf("GC: %s", "Repo GC is disabled.") return nil, nil } .........

schomatis · 2018-11-20T13:37:06Z

cmd/ipfs/daemon.go

+	}
+
+	if cfg.Datastore.GCPeriod == "" {
+		cfg.Datastore.GCPeriod = "1h"


Unrelated to this PR (since this was already present in the code) but could we add a warning here to notify the user the GCPeriod wasn't set and we're using a default value of an hour?

schomatis · 2018-11-20T13:41:22Z

core/coreapi/unixfs.go

+		if err != nil {
+			return nil, err
+		}
+		storPercentage := float64(storageUsage) / float64(n.StorageMax)


From gc.go I'm seeing that StorageMax can be left unset by the user and we'll silently assume a 10GB limit which can make this test a little brittle, a user who just doesn't care about any of this (didn't set the GC, didn't set a max capacity for the repo) will hit this error (which will even prevent it from using IPFS) without understanding what's really going on (and how to fix it).

schomatis · 2018-11-20T13:44:34Z

Left several comments but the most important one to address first is #5768 (diff) (which may require a change in the approach of the PR's proposed solution).

schomatis · 2018-12-13T22:47:57Z

ping @kjzz

kjzz · 2018-12-14T02:06:55Z

Sorry @schomatis,recently I'm preparing the opening report of my graduation thesis.I will do for this on next week.Thx.

schomatis · 2018-12-14T02:31:12Z

Hey no rush, take your time, definitively concentrate on your thesis now :)

License: MIT Signed-off-by: Kejie Zhang <601172892@qq.com>

kjzz · 2019-01-04T12:56:31Z

@schomatis sorry for delay,i have update my pr.please help me review it again.Thx a lot!

Stebalien

Sorry for jumping in on this very late but, unfortunately, we need to handle this case at a much lower level. Ideally at the datastore level but we probably don't want to reject writes of non-blocks for any reason (will make the daemon unusable).

I believe the correct approach (for now) is to do this at the blockstore level. That is, we'll have to modify the blockstore to reject puts if RepoSize grows too much.

Stebalien · 2019-03-19T02:27:33Z

core/corerepo/gc.go

-	}
-
+// PeriodicGC create a goroutine to gc.
+func PeriodicGC(ctx context.Context, gc *GC) error {


At this point, this should just be a function on the GC instance.

Stebalien · 2019-03-19T02:29:02Z

core/coreapi/unixfs.go

@@ -126,6 +131,23 @@ func (api *UnixfsAPI) Add(ctx context.Context, files files.File, opts ...options
 		fileAdder.SetMfsRoot(mr)
 	}

+	// Get storageMax when node is online.


No. Unfortunately, this really needs to go much lower. We probably need some kind of datastore wrapper that rejects writes when the datastore is full (and calls a user-provided "free" function at N% of full).

kjzz requested a review from Kubuxu as a code owner November 12, 2018 12:44

parkan requested a review from Stebalien November 13, 2018 18:27

kjzz force-pushed the fix-gc branch 3 times, most recently from ab6a965 to a382d3a Compare November 14, 2018 12:04

Stebalien requested a review from schomatis November 20, 2018 02:58

schomatis reviewed Nov 20, 2018

View reviewed changes

schomatis added the need/author-input Needs input from the original author label Dec 13, 2018

handle max storage when gc is disabled

0d5d8e0

License: MIT Signed-off-by: Kejie Zhang <601172892@qq.com>

kjzz force-pushed the fix-gc branch from a382d3a to 0d5d8e0 Compare January 4, 2019 12:55

magik6k added need/review Needs a review and removed need/author-input Needs input from the original author labels Jan 7, 2019

Stebalien reviewed Mar 19, 2019

View reviewed changes

momack2 added this to Needs Review in ipfs/go-ipfs May 9, 2019

Stebalien closed this May 15, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

handle max storage when gc is disabled #5768

handle max storage when gc is disabled #5768

kjzz commented Nov 12, 2018

schomatis commented Nov 20, 2018

kjzz commented Nov 20, 2018 •

edited

schomatis Nov 20, 2018

schomatis Nov 20, 2018

Stebalien Mar 19, 2019

schomatis Nov 20, 2018

schomatis Nov 20, 2018

kjzz Jan 4, 2019

schomatis Nov 20, 2018

kjzz Jan 4, 2019 •

edited

schomatis Nov 20, 2018

kjzz Jan 4, 2019

schomatis Nov 20, 2018

kjzz Jan 4, 2019

schomatis Nov 20, 2018

kjzz Jan 4, 2019

schomatis Nov 20, 2018

schomatis commented Nov 20, 2018 •

edited

schomatis commented Dec 13, 2018

kjzz commented Dec 14, 2018

schomatis commented Dec 14, 2018

kjzz commented Jan 4, 2019

Stebalien left a comment

Stebalien Mar 19, 2019

Stebalien Mar 19, 2019

handle max storage when gc is disabled #5768

handle max storage when gc is disabled #5768

Conversation

kjzz commented Nov 12, 2018

schomatis commented Nov 20, 2018

kjzz commented Nov 20, 2018 • edited

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

kjzz Jan 4, 2019 • edited

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

schomatis commented Nov 20, 2018 • edited

schomatis commented Dec 13, 2018

kjzz commented Dec 14, 2018

schomatis commented Dec 14, 2018

kjzz commented Jan 4, 2019

Stebalien left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kjzz commented Nov 20, 2018 •

edited

kjzz Jan 4, 2019 •

edited

schomatis commented Nov 20, 2018 •

edited