Enhance daemon states management to improve usability and stability #152

changweige · 2022-08-30T07:53:58Z

Several community users and nydus contributors have complained about nydus-snapshotter's usability and stability.

nydusd is not ready so the container image can't be accessed
nydus image can't be accessed after rebooting the host

To solve the issues once and for all, this PR refactors nydusd daemons states management. It decouples in-memory states cache from DB persistence storage. Snapshotter never empties the DB when restarting but to rebuild the in-memory daemon states cache to get consistent with DB persistent storage. In addition, it will request API to restarted nydusd daemon to recover the API mounts.

Several E2E cases are accompanied.

Address: #142 #125

codecov-commenter · 2022-09-07T02:28:33Z

Codecov Report

Base: 30.00% // Head: 30.25% // Increases project coverage by +0.25% 🎉

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

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #152      +/-   ##
==========================================
+ Coverage   30.00%   30.25%   +0.25%     
==========================================
  Files          20       20              
  Lines        1650     1626      -24     
==========================================
- Hits          495      492       -3     
+ Misses       1090     1069      -21     
  Partials       65       65

Impacted Files	Coverage Δ
pkg/filesystem/fs/fs.go	`1.10% <0.00%> (-0.05%)`	⬇️
pkg/store/daemonstore.go	`0.00% <0.00%> (ø)`
pkg/filesystem/fs/blob_manager.go	`44.44% <0.00%> (-11.12%)`	⬇️

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.

imeoer · 2022-09-09T06:51:30Z

pkg/filesystem/fs/fs.go

-		if _, e := fs.getSharedDaemon(); e != nil {
-			log.G(ctx).Warnf("initialize the shared nydus daemon")
+		if d := fs.getSharedDaemon(); d != nil && !d.Connected {
+			log.G(ctx).Infof("initialize the shared nydus daemon")
 			fs.initSharedDaemon(ctx)


The error needs to be handled.

imeoer · 2022-09-09T06:52:45Z

pkg/filesystem/fs/fs.go

+			if fs.mode == SharedInstance {
+				if d.ID != daemon.SharedNydusDaemonID && sharedDaemonConnected {
+					if err := d.SharedMount(); err != nil {
+						return nil, errors.Errorf("failed to mount rafs when recovering, %v", err)


return nil, errors.Wrapf(err, "failed to mount rafs when recovering") should be better.

imeoer · 2022-09-09T06:53:18Z

pkg/filesystem/fs/fs.go

+				}
+			} else {
+				if !d.Connected {
+					fs.manager.StartDaemon(d)


error needs to be handled.

imeoer · 2022-09-09T07:04:08Z

pkg/process/manager.go

+	s.idxBySnapshotID[daemon.SnapshotID] = daemon
+
+	if ok {
+		return old


Why do we need to return old? seems it doesn't to be used.

Yes. This is just a hint to callers. It is up to the caller if they need to check the daemon is previously inserted.
By returning old daemon pointer, caller has a chance to tell it is overwitten.

imeoer · 2022-09-09T07:13:56Z

pkg/process/manager.go

+			deleted = victim
+			continue
+		}
+		ds = append(ds, victim)


Why is the victim added to the survivor list? :)

The matching victim is already skipped. Please check it out on L83.

imeoer · 2022-09-09T07:14:35Z

pkg/process/manager.go

+	return old
+}
+
+func (s *DaemonStates) RemoveByDaemonID(id string) *daemon.Daemon {


Seem it not to be used.

Yes. It is the original semantic that daemon can be removed by its snapshot ID. So I transformed it a bit.

imeoer · 2022-09-09T07:17:47Z

pkg/process/manager.go

+	return daemon
+}
+
+func (s *DaemonStates) GetBySnapshotID(id string, op func(s *DaemonStates, d *daemon.Daemon)) *daemon.Daemon {


Is it necessary to put s as a parameter of the callback func?

Yes. For current usage, the callback is to delete an item from daemons states cache. So it has to access the s as daemon states manager

imeoer · 2022-09-09T07:21:22Z

pkg/process/manager.go

+	}
+
+	listed := make([]*daemon.Daemon, len(s.daemons))
+	copy(listed, s.daemons)


Is it necessary to copy one?

Yes. It is protected by the mutex lock. If not copying the slice, we can't say the returned slice is consistent.

Decouple daemon states cache from persistence storage in DB. So it is easier to do nydusd daemon recover. Signed-off-by: Changwei Ge <gechangwei@bytedance.com>

Signed-off-by: Changwei Ge <gechangwei@bytedance.com>

changweige changed the title ~~Recover after restart~~ wip: Recover after restart Aug 30, 2022

changweige force-pushed the recover-after-restart branch from 0a1baf6 to 474561b Compare August 31, 2022 03:01

changweige changed the title ~~wip: Recover after restart~~ Enhance daemon states management to improve usability and stability Aug 31, 2022

changweige force-pushed the recover-after-restart branch 5 times, most recently from 2c733e2 to 154e065 Compare September 7, 2022 02:22

imeoer reviewed Sep 9, 2022

View reviewed changes

changweige force-pushed the recover-after-restart branch from 50684ef to f203e5d Compare September 9, 2022 09:39

changweige added 13 commits September 13, 2022 15:22

add in-memory daemon states cache for daemons manager

0284cd5

Decouple daemon states cache from persistence storage in DB. So it is easier to do nydusd daemon recover. Signed-off-by: Changwei Ge <gechangwei@bytedance.com>

Store interface does not operate in-memory cache

6e71b1d

Signed-off-by: Changwei Ge <gechangwei@bytedance.com>

DB Store implementation does not operate states in-memory cache

224b816

Signed-off-by: Changwei Ge <gechangwei@bytedance.com>

daemons manager use new states cache

9b7920e

Signed-off-by: Changwei Ge <gechangwei@bytedance.com>

don't clean up DB when reconnecting all nydusd daemons

ca181d0

Signed-off-by: Changwei Ge <gechangwei@bytedance.com>

recover ever persisted daemon when restarting snapshotter

378f308

Signed-off-by: Changwei Ge <gechangwei@bytedance.com>

also recover each nydusd for mupltiple daemon mode

8e58c11

Signed-off-by: Changwei Ge <gechangwei@bytedance.com>

add integration test for recovering nydusds

57c0f77

Signed-off-by: Changwei Ge <gechangwei@bytedance.com>

no need to for new nydusd if they are already running

085bdc1

Signed-off-by: Changwei Ge <gechangwei@bytedance.com>

enable integration tests for only restarting snapshotter

5f1b9dd

Signed-off-by: Changwei Ge <gechangwei@bytedance.com>

add ErrNotFound effor defintion

b09247e

Signed-off-by: Changwei Ge <gechangwei@bytedance.com>

manager daemon state getter can't fail

f303066

Signed-off-by: Changwei Ge <gechangwei@bytedance.com>

e2e test re-run containiers after pruning all containers

f86c905

Signed-off-by: Changwei Ge <gechangwei@bytedance.com>

changweige force-pushed the recover-after-restart branch from f203e5d to f86c905 Compare September 13, 2022 07:22

imeoer approved these changes Sep 13, 2022

View reviewed changes

changweige merged commit f8cfe38 into containerd:main Sep 13, 2022

changweige deleted the recover-after-restart branch September 26, 2022 03:32

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Enhance daemon states management to improve usability and stability #152

Enhance daemon states management to improve usability and stability #152

changweige commented Aug 30, 2022 •

edited

Loading

codecov-commenter commented Sep 7, 2022 •

edited

Loading

imeoer Sep 9, 2022

changweige Sep 9, 2022

imeoer Sep 9, 2022

changweige Sep 9, 2022

imeoer Sep 9, 2022

changweige Sep 9, 2022

imeoer Sep 9, 2022

changweige Sep 9, 2022

imeoer Sep 9, 2022

changweige Sep 9, 2022

imeoer Sep 9, 2022

changweige Sep 9, 2022

imeoer Sep 9, 2022

changweige Sep 9, 2022

imeoer Sep 9, 2022

changweige Sep 9, 2022 •

edited

Loading

Enhance daemon states management to improve usability and stability #152

Enhance daemon states management to improve usability and stability #152

Conversation

changweige commented Aug 30, 2022 • edited Loading

codecov-commenter commented Sep 7, 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

changweige Sep 9, 2022 • edited Loading

Choose a reason for hiding this comment

changweige commented Aug 30, 2022 •

edited

Loading

codecov-commenter commented Sep 7, 2022 •

edited

Loading

changweige Sep 9, 2022 •

edited

Loading