Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Prevent reserved bytes underflow #2907

Merged
merged 7 commits into from
Mar 16, 2022
Merged

Prevent reserved bytes underflow #2907

merged 7 commits into from
Mar 16, 2022

Conversation

variadico
Copy link
Contributor

@variadico variadico commented Mar 7, 2022

Currently, there is a bug where js.storeReserved and js.memReserved can underflow, causing /varz metrics to be incorrect. Below is a history of the state changes.

  1. Create stream with no MaxBytes: js.storeReserved → 0
  2. Update stream with MaxBytes=1234: js.storeReserved → 0 (should have reserved 1234)
  3. Delete stream: js.storeReserved → -1234

Bug:js.storeReserved=-1234 is an int that later gets converted to a uint64, which causes an underflow.

This change releases the old MaxBytes and reserves the new MaxBytes during the stream update step. This way, when a stream is deleted, these values don't become negative.

Resolves #2882 (see for additional info)

server/stream.go Outdated Show resolved Hide resolved
Copy link
Contributor

@matthiashanel matthiashanel left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would you mind adding the following tests?

  1. Reserve 2/3rd of the total account limit, then update to one extra byte.
  2. Reserve 2/3rd of the total server limit, then update to one extra byte.

I believe we may have an accounting issue there as well.

These two scenarios apply to going down by a byte as well.

server/stream.go Outdated Show resolved Hide resolved
server/stream.go Outdated Show resolved Hide resolved
@variadico
Copy link
Contributor Author

The newer commits add the following new tests. They caught some bugs, so the original handling of MaxBytes was also updated.

--- PASS: TestStorageReservedBytes (0.06s)
    --- PASS: TestStorageReservedBytes/file_reserve_66%_of_system_limit (0.01s)
    --- PASS: TestStorageReservedBytes/memory_reserve_66%_of_system_limit (0.00s)

    --- PASS: TestStorageReservedBytes/file_update_past_system_limit (0.00s)
    --- PASS: TestStorageReservedBytes/memory_update_past_system_limit (0.00s)

    --- PASS: TestStorageReservedBytes/file_update_to_system_limit (0.00s)
    --- PASS: TestStorageReservedBytes/memory_update_to_system_limit (0.00s)

    --- PASS: TestStorageReservedBytes/file_reserve_66%_of_account_limit (0.00s)
    --- PASS: TestStorageReservedBytes/memory_reserve_66%_of_account_limit (0.00s)

    --- PASS: TestStorageReservedBytes/file_update_to_account_limit (0.00s)
    --- PASS: TestStorageReservedBytes/memory_update_to_account_limit (0.00s)

@variadico
Copy link
Contributor Author

Added more tests to verify that the leader's system limits don't override non-leader server's limits.

--- PASS: TestJetStreamSystemLimitsPlacement (2.11s)
    --- PASS: TestJetStreamSystemLimitsPlacement/file_create_large_stream_on_small_server (0.00s)
    --- PASS: TestJetStreamSystemLimitsPlacement/memory_create_large_stream_on_small_server (0.00s)

    --- PASS: TestJetStreamSystemLimitsPlacement/file_create_large_stream_on_medium_server (0.00s)
    --- PASS: TestJetStreamSystemLimitsPlacement/memory_create_large_stream_on_medium_server (0.00s)

    --- PASS: TestJetStreamSystemLimitsPlacement/file_create_large_stream_on_large_server (0.01s)
    --- PASS: TestJetStreamSystemLimitsPlacement/memory_create_large_stream_on_large_server (0.00s)

Copy link
Contributor

@matthiashanel matthiashanel left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@variadico
Copy link
Contributor Author

Added new tests where we create a supercluster and ensure we can't create a large MaxBytes stream on a smaller cluster.

--- PASS: TestJetStreamSuperClusterSystemLimitsPlacement (5.22s)
    --- PASS: TestJetStreamSuperClusterSystemLimitsPlacement/file_create_large_stream_on_small_cluster_b0 (0.01s)
    --- PASS: TestJetStreamSuperClusterSystemLimitsPlacement/memory_create_large_stream_on_small_cluster_b0 (0.00s)
    --- PASS: TestJetStreamSuperClusterSystemLimitsPlacement/file_create_large_stream_on_small_cluster_b1 (0.00s)
    --- PASS: TestJetStreamSuperClusterSystemLimitsPlacement/memory_create_large_stream_on_small_cluster_b1 (0.00s)
    --- PASS: TestJetStreamSuperClusterSystemLimitsPlacement/file_create_large_stream_on_small_cluster_b2 (0.00s)
    --- PASS: TestJetStreamSuperClusterSystemLimitsPlacement/memory_create_large_stream_on_small_cluster_b2 (0.00s)
    --- PASS: TestJetStreamSuperClusterSystemLimitsPlacement/file_create_large_stream_on_large_cluster_a0 (0.01s)
    --- PASS: TestJetStreamSuperClusterSystemLimitsPlacement/memory_create_large_stream_on_large_cluster_a0 (0.01s)
    --- PASS: TestJetStreamSuperClusterSystemLimitsPlacement/file_create_large_stream_on_large_cluster_a1 (0.01s)
    --- PASS: TestJetStreamSuperClusterSystemLimitsPlacement/memory_create_large_stream_on_large_cluster_a1 (0.00s)
    --- PASS: TestJetStreamSuperClusterSystemLimitsPlacement/file_create_large_stream_on_large_cluster_a2 (0.00s)
    --- PASS: TestJetStreamSuperClusterSystemLimitsPlacement/memory_create_large_stream_on_large_cluster_a2 (0.00s)

server/jetstream_test.go Outdated Show resolved Hide resolved
server/jetstream_test.go Outdated Show resolved Hide resolved
server/jetstream_test.go Outdated Show resolved Hide resolved
server/jetstream_test.go Outdated Show resolved Hide resolved
server/jetstream_test.go Show resolved Hide resolved
server/jetstream_test.go Outdated Show resolved Hide resolved
server/jetstream_test.go Outdated Show resolved Hide resolved
server/jetstream_test.go Outdated Show resolved Hide resolved
server/jetstream_test.go Outdated Show resolved Hide resolved
Copy link
Member

@kozlovic kozlovic left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@variadico variadico merged commit acfd456 into main Mar 16, 2022
@variadico variadico deleted the prevent-reserved-underflow branch March 16, 2022 22:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

ReservedStore and ReservedMemory report 18EB
4 participants