[Raft] Add Raft persistence by MariemBaccari · Pull Request #1470 · interuss/dss

MariemBaccari · 2026-05-20T07:25:30Z

This PR is part of the chain #1470 -> #1473 -> #1474 to address the issue #1463.
It adds WAL and snapshot persistency and management for the raftstore. Contrary to the diagrams from the issue #1463, we embed *Raft.MemoryStorage in the storage instead of setting it as a field in Consensus to avoid scattering the storage management.

the-glu

LGTM

barroco · 2026-06-01T19:52:00Z

+		return stacktrace.Propagate(err, "failed to save snapshot to wal")
+	}
+
+	return s.wal.ReleaseLockTo(snapshot.Metadata.Index)


Please add a comment on where the lock was acquired

+1
And does the lock need to be released when an error is returned?

These are os level flock locks on WAL segment files that are acquired internally in the WAL implementation. ReleaseLockTo is used to release the locks for the entries that are already covered by the snapshot and, thus, the files are not needed anymore.
We shouldn't release the locks when an error is returned since the files are still needed if we fail to save the snapshot.
I realized, while looking into etcd's source code, that this call is actually useless in the current state of our implementation. Etcd uses this as a way to mark files that can be cleaned up (separately). I will keep it for now and add a comment to the function and the github issue to implement the cleanup later.

Please also include this rationale in the comment in code :)

barroco · 2026-06-01T19:52:44Z

Thank you @MariemBaccari, please find some suggestions inline.

mickmis · 2026-06-02T08:27:13Z

 func init() {
 	flag.Uint64Var(&connectParameters.ID, "raft_node_id", 0, "raft node ID for this instance (must be non-zero and unique within the cluster)")
 	flag.StringVar(&connectParameters.Peers, "raft_peers", "", `comma-separated "nodeID=peerURL" pairs for all cluster members, including the current node, e.g. "1=http://node1:9021,2=http://node2:9021,3=http://node3:9021"`)
+	flag.StringVar(&connectParameters.DataDir, "raft_data_directory", defaultDataDir, "directory for raft data (snapshot and WAL storage)")


Whenever we write stuff to the local filesystem that becomes a something the operations need to deal with. So somehow, somewhere, we need documentation about how to handle the data in that directory. Is it temp data that can be trashed? If trashed how long does it take to reconstruct it? If not temp, does it need to be backed up? What happens if it is lost? How large can it become? Can it be hot swapped? Is there any impact from the underlying storage (e.g. if it is too slow?) etc.

e.g. I suspect snapshotCatchUpEntriesN has an impact on the size of file on filesytem: have this configurable and document it

I clarified the raft_datadir flag description, do you think that's sufficient for the moment ? I also added a cleanup task in #1463. I think the implementation of that will come with more documentation regarding the growth of the data directory.

I clarified the raft_datadir flag description, do you think that's sufficient for the moment ? I also added a cleanup task in #1463. I think the implementation of that will come with more documentation regarding the growth of the data directory.

This should be part of the user documentation - so not in the code necessarily (although code comment is useful too). I'd say as long as it is tracked that it is to be included in the documentation we are OK. I can't find on #1463 though?

I had added the "Old files cleanup" task to "future changes". I just specified the user documentation part now.

mickmis

Not sure if already suggested by @barroco, but using https://pkg.go.dev/go.etcd.io/raft/v3@v3.6.0/rafttest for testing would be quite important (not necessarily in this PR, but important to do in general)

mickmis · 2026-06-03T11:38:00Z

+			return nil, false, stacktrace.Propagate(err, "failed to create directory for wal storage at: %s", walPath)
+		}
+
+		w, err := wal.Create(logger, walPath, nil)


w is redefined here. If that is intended, why declaring var w above?

Thanks for catching this, I did not intend to redefine it.

mickmis · 2026-06-03T11:40:34Z

+		return stacktrace.Propagate(err, "failed to save snapshot to wal")
+	}
+
+	return s.wal.ReleaseLockTo(snapshot.Metadata.Index)


Please also include this rationale in the comment in code :)

mickmis · 2026-06-03T11:45:03Z

+}
+
+func loadSnapshot(logger *zap.Logger, walPath string, snapshotter *snap.Snapshotter) (*raftpb.Snapshot, error) {
+	if !wal.Exist(walPath) {


This check is done again right after the only call to loadSnapshot: looks like the control flow could be clarified a bit. Is the intent to load a snapshot only if the wal does not exist? If so, what about removing this here and calling loadSnapshot in an else of if !ok?

The snapshot is loaded only if the wal does already exist but yes it's better to move the load in an else, the Exist call is redundant. Will fix this.

mickmis · 2026-06-03T11:48:29Z

+}
+
+// getSnapshot calls all registered snapshot providers and combines their data into a single snapshot.
+func (s *storage) getSnapshot() ([]byte, error) {


Looks like this fits json.Marshaler interface: https://pkg.go.dev/encoding/json#Marshaler

That's true but I think we should keep this method signature as it clearly indicates the specific purpose of getting a snapshot of the storage. The underlying implementation can always change and just happens to be a json marshalling for the moment.

mickmis · 2026-06-03T11:51:13Z

+		return stacktrace.Propagate(err, "failed to save WAL entries")
+	}
+
+	if !raft.IsEmptySnap(snapshot) {


Control flow: is it actually necessary to check for this condition twice?

Since we cannot change the ordering of saving the snapshot -> saving the entries -> applying, I think we have to check for this condition twice.

mickmis · 2026-06-03T12:14:41Z

+// newStorage initializes the storage by loading the latest snapshot and wal entries from the disk
+// and applies them to the Raft memory storage.
+// It returns the initialized storage, a boolean indicating whether the storage was pre-existent or an error.
+func newStorage(ctx context.Context, logger *zap.Logger, dataDir string, nodeID uint64, snapshotCatchUpEntries uint64) (*storage, bool, error) {


And an additional clarification: is this storage OK in a cluster environment where there are multiple DSS pods? Is this the intent of the nodeID or not at all?

Yes, that is the intent of nodeID :) I can run multiple nodes locally safely.

MariemBaccari mentioned this pull request May 20, 2026

[raft] Raftstore implementation #1463

Open

the-glu reviewed May 20, 2026

View reviewed changes

Comment thread pkg/raftstore/consensus/storage.go Outdated

MariemBaccari force-pushed the add_raft_persistence branch 3 times, most recently from 166d5d0 to 5759374 Compare May 20, 2026 13:32

This was referenced May 20, 2026

[Raft] Add consensus logic #1472

Closed

[Raft] Initialization and configuration of consensus #1473

Merged

[Raft] Consensus entries publishing and proposal #1474

Merged

the-glu approved these changes May 27, 2026

View reviewed changes

barroco requested changes Jun 1, 2026

View reviewed changes

mickmis reviewed Jun 2, 2026

View reviewed changes

Comment thread pkg/raftstore/consensus/consensus.go Outdated

mickmis reviewed Jun 2, 2026

View reviewed changes

Comment thread pkg/raftstore/consensus/storage.go Outdated

MariemBaccari force-pushed the add_raft_persistence branch 2 times, most recently from 62208b4 to 610ad5f Compare June 2, 2026 14:21

MariemBaccari requested review from barroco and mickmis June 2, 2026 14:36

mickmis reviewed Jun 3, 2026

View reviewed changes

mickmis approved these changes Jun 3, 2026

View reviewed changes

mickmis reviewed Jun 3, 2026

View reviewed changes

MariemBaccari force-pushed the add_raft_persistence branch from 610ad5f to 78db271 Compare June 8, 2026 06:52

implement storage

090d9a6

MariemBaccari force-pushed the add_raft_persistence branch from 78db271 to 090d9a6 Compare June 8, 2026 07:18

barroco approved these changes Jun 8, 2026

View reviewed changes

barroco merged commit 67e337c into interuss:master Jun 8, 2026
12 checks passed

barroco deleted the add_raft_persistence branch June 8, 2026 07:52

Uh oh!

Conversation

MariemBaccari commented May 20, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

the-glu left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

barroco commented Jun 1, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

mickmis Jun 2, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

mickmis left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

MariemBaccari Jun 8, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

MariemBaccari commented May 20, 2026 •

edited

Loading

barroco commented Jun 1, 2026 •

edited

Loading

mickmis Jun 2, 2026 •

edited

Loading

MariemBaccari Jun 8, 2026 •

edited

Loading