Disable JetStream on disk errors #6292

souravagrawal · 2024-12-22T07:44:14Z

Currently, there are scenarios where NATS JetStream may encounter permission errors when file system goes into read only mode, which can lead to an inconsistent state. In such cases, the system continues to allow publishing messages by resetting stream state, leading to a misaligned consumer stream sequence.

This PR introduces changes to gracefully handle these permission errors and prevent NATS from continuing in an inconsistent state when:

Flushing stream state to disk
Deleting expired messages on startup
Creating new block for messages
After this PR, If NATS is running in non-clustered mode, the user will be unable to issue write requests until the issue is resolved. In clustered mode, only the affected node will stop accepting requests, while the system will continue to function as long as the required quorum remains healthy.

PR potentially fixes : #6211 which leads to consumer sequence reaching higher than stream sequence.
Signed-off-by: Sourabh Agrawal [email protected]

neilalexander

We should probably have better detection of a wider array of filesystem errors like permissions etc, agreed, but I'm not entirely sure why this would be a configurable option and I don't particularly like it either.

The disk being pulled out from underneath JetStream is effectively a catastrophic operational issue and for JS to continue to try to operate in those circumstances feels problematic, i.e. with the potential for data loss that no one may even notice. Even more so in a clustered scenario where the metalayer and Raft logs are being written out to disk.

souravagrawal · 2024-12-24T14:28:57Z

We should probably have better detection of a wider array of filesystem errors like permissions etc, agreed, but I'm not entirely sure why this would be a configurable option and I don't particularly like it either.

The disk being pulled out from underneath JetStream is effectively a catastrophic operational issue and for JS to continue to try to operate in those circumstances feels problematic, i.e. with the potential for data loss that no one may even notice. Even more so in a clustered scenario where the metalayer and Raft logs are being written out to disk.

Thank you for taking the time to review this.
I will remove the flag. As this is my first patch in the server, I wanted to err on the side of caution.
Are there any other cases you think I should address as part of this?

souravagrawal · 2024-12-25T08:14:52Z

Reopening PR, as it was closed mistakenly

souravagrawal · 2024-12-25T08:27:08Z

We should probably have better detection of a wider array of filesystem errors like permissions etc, agreed, but I'm not entirely sure why this would be a configurable option and I don't particularly like it either.
The disk being pulled out from underneath JetStream is effectively a catastrophic operational issue and for JS to continue to try to operate in those circumstances feels problematic, i.e. with the potential for data loss that no one may even notice. Even more so in a clustered scenario where the metalayer and Raft logs are being written out to disk.

Thank you for taking the time to review this. I will remove the flag. As this is my first patch in the server, I wanted to err on the side of caution. Are there any other cases you think I should address as part of this?

Thank you for the suggestion @neilalexander. I have implemented the recommended change and removed the flag.

souravagrawal · 2025-01-02T04:35:10Z

Hello @neilalexander, Happy New Year.
Just following up to check if you’ve had a chance to review the changes.

neilalexander

I think having the filestore.go call DisableJetStream directly may not be the best approach here. In a number of places we already surface errors up to stream.go, i.e. for isOutOfSpaceErr.

It feels like the more natural thing to do here would be to extend that same pattern, such that the stream examines surfaced errors and reacts, similar to how we do with the disk out-of-space warnings. Otherwise we end up with different types of disk errors being handled differently and that makes the code difficult to trace.

neilalexander · 2025-01-16T13:06:15Z

server/filestore.go

-		fs.expireMsgsOnRecover()
+		err := fs.expireMsgsOnRecover()
+		if err != nil && err == errFileSystemPermissionDenied {
+			fs.srv.Warnf("file system permission denied while expiring msgs, disabling jetstream: %v", err)


Please make sure any log lines are capitalised/cased correctly.

neilalexander · 2025-01-16T13:08:05Z

server/filestore.go

-			os.Remove(mb.mfn)
+			err := os.Remove(mb.mfn)
+			if err != nil && os.IsPermission(err){
+				return errFileSystemPermissionDenied


Is there a reason to do this rather than just check os.IsPermission further up the callstack?

make sense, I will change it.

It may even make sense to define a new error-detect function for this case, similar to how we already have this in store.go for out-of-space:

func isOutOfSpaceErr(err error) bool { return err != nil && (strings.Contains(err.Error(), "no space left")) }

souravagrawal · 2025-01-16T15:52:45Z

I think having the filestore.go call DisableJetStream directly may not be the best approach here. In a number of places we already surface errors up to stream.go, i.e. for isOutOfSpaceErr.

It feels like the more natural thing to do here would be to extend that same pattern, such that the stream examines surfaced errors and reacts, similar to how we do with the disk out-of-space warnings. Otherwise we end up with different types of disk errors being handled differently and that makes the code difficult to trace.

I agree; it makes sense to handle this within the same pattern. I have implemented the change as you suggested.
However, there are a few other cases where stream-related operations, such as expireMsgs and writeFullState, are executed within a Go routine and need to be addressed in filestore.go, I believe.

neilalexander · 2025-01-20T13:52:32Z

server/filestore_test.go

@@ -8562,3 +8563,81 @@ func TestFileStoreDontSpamCompactWhenMostlyTombstones(t *testing.T) {
 	fmb.bytes /= 2
 	require_True(t, fmb.shouldCompactInline())
 }
+
+func TestStoreRawMessageThrowsPermissionErrorIfFSModeReadOnly(t *testing.T) {


All tests in filestore_test.go need to start TestFileStore, otherwise CI won't pick them up properly.

neilalexander · 2025-01-20T13:54:14Z

server/filestore.go

-			fs.writeFullState()
+			err := fs.writeFullState()
+			if isPermissionError(err) {
+				fs.warn("File system permission denied when flushing stream state, disabling jetstream %v", err)


Nit: Capitalisation JetStream, pop a : before the %v too.

neilalexander · 2025-01-20T13:55:25Z

server/filestore.go

+				fs.warn("File system permission denied when flushing stream state, disabling jetstream %v", err)
+				// messages in block cache could be lost in the worst case.
+				// In the clustered mode it is very highly unlikely as a result of replication.
+				fs.srv.DisableJetStream()


I don't think fs.srv can be nil here but let's add a fs.srv != nil guard just in case.

neilalexander · 2025-01-20T13:56:27Z

server/filestore.go

@@ -8743,6 +8772,10 @@ func (fs *fileStore) _writeFullState(force bool) error {
 	// Protect with dios.
 	<-dios
 	err := os.WriteFile(fn, buf, defaultFilePerms)
+	// if file system is not writable isPermissionError is set to true
+	if isPermissionError(err) {


This must be done after the dios <- struct{}{} line, otherwise the semaphore becomes unbalanced in a failure scenario.

neilalexander · 2025-01-20T13:57:28Z

server/stream.go

+			// messages in block cache could be lost in the worst case.
+			// In the clustered mode it is very highly unlikely as a result of replication.
+			mset.srv.DisableJetStream()
+			mset.srv.Warnf("File system permission denied while writing msg, disabling jetstream: %v", err)


Same nit here with capitalisation etc.

Filesystem permission denied while writing message, disabling JetStream: %v

Signed-off-by: Sourabh Agrawal <[email protected]>

souravagrawal · 2025-01-20T18:56:46Z

Thank you, @neilalexander. All the comments have been addressed.

souravagrawal force-pushed the main branch 3 times, most recently from 835c8bd to a309fcf Compare December 22, 2024 09:13

souravagrawal marked this pull request as ready for review December 22, 2024 12:48

souravagrawal requested a review from a team as a code owner December 22, 2024 12:48

souravagrawal mentioned this pull request Dec 24, 2024

Stop stream when stream state inconsistency is detected during flush #6237

Open

neilalexander reviewed Dec 24, 2024

View reviewed changes

souravagrawal closed this Dec 25, 2024

souravagrawal force-pushed the main branch from c162327 to 1a0d55f Compare December 25, 2024 06:09

souravagrawal reopened this Dec 25, 2024

souravagrawal force-pushed the main branch from d5d8b82 to a81fe5d Compare January 16, 2025 11:06

neilalexander self-requested a review January 16, 2025 12:59

neilalexander requested changes Jan 16, 2025

View reviewed changes

souravagrawal force-pushed the main branch from a81fe5d to 3899b1c Compare January 16, 2025 15:43

souravagrawal force-pushed the main branch 2 times, most recently from 9b4d96d to 0caaf62 Compare January 16, 2025 15:57

souravagrawal requested a review from neilalexander January 17, 2025 12:47

neilalexander requested changes Jan 20, 2025

View reviewed changes

souravagrawal added 2 commits January 20, 2025 23:57

Disable jetstream if filesystem permission denied detected during write

20b1f1d

Signed-off-by: Sourabh Agrawal <[email protected]>

Disable jetstream if filesystem permission denied detected during write

068bd41

Signed-off-by: Sourabh Agrawal <[email protected]>

souravagrawal force-pushed the main branch 2 times, most recently from 491222c to 0cde6ae Compare January 20, 2025 18:54

Disable jetstream if disk permission error while writing raft state

f34efd3

souravagrawal force-pushed the main branch from 0cde6ae to f34efd3 Compare January 20, 2025 18:58

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Disable JetStream on disk errors #6292

Disable JetStream on disk errors #6292

souravagrawal commented Dec 22, 2024 •

edited

Loading

neilalexander left a comment

souravagrawal commented Dec 24, 2024

souravagrawal commented Dec 25, 2024

souravagrawal commented Dec 25, 2024

souravagrawal commented Jan 2, 2025

neilalexander left a comment

neilalexander Jan 16, 2025

neilalexander Jan 16, 2025

souravagrawal Jan 16, 2025

neilalexander Jan 16, 2025

souravagrawal commented Jan 16, 2025

neilalexander Jan 20, 2025

neilalexander Jan 20, 2025

neilalexander Jan 20, 2025

neilalexander Jan 20, 2025

neilalexander Jan 20, 2025

souravagrawal commented Jan 20, 2025

Disable JetStream on disk errors #6292

Are you sure you want to change the base?

Disable JetStream on disk errors #6292

Conversation

souravagrawal commented Dec 22, 2024 • edited Loading

neilalexander left a comment

Choose a reason for hiding this comment

souravagrawal commented Dec 24, 2024

souravagrawal commented Dec 25, 2024

souravagrawal commented Dec 25, 2024

souravagrawal commented Jan 2, 2025

neilalexander left a 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

souravagrawal commented Jan 16, 2025

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

souravagrawal commented Jan 20, 2025

souravagrawal commented Dec 22, 2024 •

edited

Loading