Disable JetStream on disk errors

[souravagrawal]

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]

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]

Reopening PR, as it was closed mistakenly

[souravagrawal]

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]

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]

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

[neilalexander]

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

[souravagrawal]

make sense, I will change it.

[neilalexander]

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]

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.