Fscache gc #262

kevinXYin · 2022-11-25T04:01:36Z

support fscahe gc

codecov-commenter · 2022-11-25T04:09:21Z

Codecov Report

Base: 31.57% // Head: 31.38% // Decreases project coverage by -0.19% ⚠️

Coverage data is based on head (a9145a2) compared to base (84c7439).
Patch coverage: 0.00% of modified lines in pull request are covered.

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #262      +/-   ##
==========================================
- Coverage   31.57%   31.38%   -0.20%     
==========================================
  Files          34       34              
  Lines        3506     3518      +12     
==========================================
- Hits         1107     1104       -3     
- Misses       2283     2298      +15     
  Partials      116      116

Impacted Files	Coverage Δ
pkg/daemon/client.go	`28.20% <0.00%> (-1.33%)`	⬇️
pkg/daemon/daemon.go	`0.00% <0.00%> (ø)`
pkg/daemon/rafs.go	`2.32% <ø> (ø)`
pkg/blob/blob_manager.go	`29.59% <0.00%> (-3.07%)`	⬇️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

pkg/daemon/daemon.go

changweige · 2022-11-28T02:11:07Z

pkg/daemon/client.go

+			query.Add("blob_id", blobID)
+		}
+	} else {
+		query.Add("blob_id", blobID)


Can we have a comment describing the blob id and domain id combination here or a UT test? Otherwise, it is hard to maintain

Yeah , will add comment after the API fixed

As the nydusd API was fixed and merged, shall we have a description comments now?

We have to consider the API compatibility here. Old nydusd won't extract "blob_id' from the request, right? So old nydusd still can work with this nydus-snapshotter?

Already add some comment for the API , and this new semantic is just used to delete fscache cache files, if the old nydusd did not support this then snapshotter will just ignore the error and print error info, which is as expectation. so I think we do not need compatible with an old nydusd.

pkg/filesystem/fs/fs.go

changweige · 2022-11-28T02:14:10Z

pkg/filesystem/fs/fs.go

+		}
+		// delete fscache blob cache file
+		if err := c.UnbindBlob("", blobID); err != nil {
+			log.L.Warnf("delete blob %s err %s", blobID, err)


Should return error?

yes , we better return error , but this will cause snapshotter can not forward compatible nydusd?

You are right, perhaps we should let snapshotter nydusd client is aware or nydusd version to workaround

pkg/filesystem/fs/fs.go

changweige · 2022-11-28T02:16:28Z

snapshot/snapshot.go

@@ -203,11 +203,17 @@ func NewSnapshotter(ctx context.Context, cfg *config.Config) (snapshots.Snapshot
 		return nil, err
 	}

+	syncRemove := cfg.SyncRemove
+	if cfg.FsDriver == config.FsDriverFscache {
+		log.L.Infof("for fscache mode enable syncRemove")


We can not use syncRemove flag to control whether to remove fscache files. Just directly remove it in a goroutine

AKAIK, the syncRemove is used for making removing snapshot in order , cause we need remove bootstrap snapshot first and then we can umount erofs. So should we force enable syncRemove for fscache ?

We can simply make fscache files deletion out of the syncRemove==true if arm

changweige · 2022-12-12T04:12:56Z

pkg/daemon/daemon.go

@@ -323,8 +324,9 @@ func (d *Daemon) sharedErofsUmount(rafs *Rafs) error {
 		return errors.Wrapf(err, "unbind blob %s", d.ID())
 	}
 	domainID := rafs.Annotations[AnnoFsCacheDomainID]
+	fscaheID := rafs.Annotations[AnnoFsCacheID]


It's better to handle the absence of fscacheid. Moreover, should the name be consistent? I saw it is named blobID in function UnbindBlob body

fscacheID will be set with domainID together, it can not be absent. fscacheID will used for generate fscache cookie key for bootstrap, and we need it when delete fscache cache file for bootstrap. Already add comment for this.

changweige · 2022-12-12T04:16:44Z

pkg/daemon/daemon.go

+	// delete fscache bootstrap cache file
+	if err := c.UnbindBlob("", fscaheID); err != nil {
+		log.L.Warnf("delete bootstrap %s err %s", fscaheID, err)
+	}


Why ignoring the unbind error here?

two reasons for this:

make compatible with old nydusd.

Is this need to return err to containerd when delete bootstrap cache failed? seems we do not do this for blob cache

Yes. It's a way to handle compatibility, however, it also hides the error if the nydusd supports this new API. Let's do it this way right now.

changweige · 2022-12-12T06:03:10Z

Can you please rebase this PR to latest main branch? The base code has changed. It will be easier to view those changes.

changweige · 2022-12-15T02:05:19Z

pkg/daemon/client.go

+ * 2. domainID + blobID, delete the blob entry, if the blob is bootstrap
+ *    also delete blob entries belong to it.
+ * 3. blobID, try to find and cull blob cache files by blobID in all domains.
+ */


Just a nit: // comment style might match the project comment style

changweige · 2022-12-15T02:35:17Z

pkg/daemon/daemon.go

+	// delete fscache bootstrap cache file
+	if err := c.UnbindBlob("", fscaheID); err != nil {
+		log.L.Warnf("delete bootstrap %s err %s", fscaheID, err)
+	}


Yes. It's a way to handle compatibility, however, it also hides the error if the nydusd supports this new API. Let's do it this way right now.

In fscache shared domain mode, before umount erofs we should call DELETE blob API with domainId + fscacheId. Signed-off-by Xin Yin <yinxin.x@bytedance.com>

delete fscache cache files for blobs and bootstrap when remove image. Signed-off-by:Xin Yin <yinxin.x@bytedance.com>

changweige

LGTM

kevinXYin mentioned this pull request Nov 25, 2022

nydusd: make DELETE /api/v2/blobs API support cull fscache blob dragonflyoss/nydus#894

Merged

changweige reviewed Nov 28, 2022

View reviewed changes

pkg/daemon/daemon.go Show resolved Hide resolved

changweige reviewed Nov 28, 2022

View reviewed changes

pkg/filesystem/fs/fs.go Outdated Show resolved Hide resolved

changweige reviewed Nov 28, 2022

View reviewed changes

pkg/filesystem/fs/fs.go Outdated Show resolved Hide resolved

changweige reviewed Nov 28, 2022

View reviewed changes

changweige reviewed Dec 12, 2022

View reviewed changes

kevinXYin force-pushed the fscache_gc branch 2 times, most recently from 71c0be3 to ba37249 Compare December 14, 2022 08:48

changweige reviewed Dec 15, 2022

View reviewed changes

kevinXYin force-pushed the fscache_gc branch 3 times, most recently from 6f7fa94 to 8654605 Compare December 16, 2022 03:09

Xin Yin added 2 commits December 16, 2022 11:58

fix paramters for DEL blob API in fscache shared domain mode

5647d3a

In fscache shared domain mode, before umount erofs we should call DELETE blob API with domainId + fscacheId. Signed-off-by Xin Yin <yinxin.x@bytedance.com>

call blob API to delete fscache blob files when remove image

a9145a2

delete fscache cache files for blobs and bootstrap when remove image. Signed-off-by:Xin Yin <yinxin.x@bytedance.com>

kevinXYin force-pushed the fscache_gc branch from 8654605 to a9145a2 Compare December 16, 2022 03:59

changweige approved these changes Dec 22, 2022

View reviewed changes

changweige merged commit 07d6352 into containerd:main Dec 22, 2022

adamqqqplay mentioned this pull request Mar 23, 2023

blob gc feature for fscache with inuse and cull dragonflyoss/nydus#454

Closed

yawqi mentioned this pull request Mar 23, 2023

WIP fscache: blob gc feature for fscache with inuse and cull dragonflyoss/nydus#518

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fscache gc #262

Fscache gc #262

kevinXYin commented Nov 25, 2022

codecov-commenter commented Nov 25, 2022 •

edited

Loading

changweige Nov 28, 2022

kevinXYin Nov 28, 2022

changweige Dec 12, 2022

changweige Dec 12, 2022

kevinXYin Dec 14, 2022

changweige Nov 28, 2022

kevinXYin Nov 28, 2022

changweige Nov 28, 2022

changweige Nov 28, 2022

kevinXYin Nov 28, 2022

changweige Nov 28, 2022

changweige Dec 12, 2022

kevinXYin Dec 14, 2022

changweige Dec 12, 2022

kevinXYin Dec 14, 2022

changweige Dec 15, 2022

changweige commented Dec 12, 2022 •

edited

Loading

changweige Dec 15, 2022

changweige Dec 15, 2022

changweige left a comment

Fscache gc #262

Fscache gc #262

Conversation

kevinXYin commented Nov 25, 2022

codecov-commenter commented Nov 25, 2022 • edited Loading

Codecov Report

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

changweige commented Dec 12, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

changweige left a comment

Choose a reason for hiding this comment

codecov-commenter commented Nov 25, 2022 •

edited

Loading

changweige commented Dec 12, 2022 •

edited

Loading