I am extending this test to ensure that, depending on the available heap memory, we throttle the number of concurrent snapshot threads accordingly

This test is different to the previous. This is just a basic unit test to ensure that the function of heap memory -> snapshot threads is as expected, but doesn't test whether all threads are properly utilised

If there is an easier way to get the total heap memory then please let me know. I was following a similar approach to above with the MAX_HEAP_SIZE_FOR_SNAPSHOT_DELETION_SETTING setting.

My concern is that to modify HEAP_SIZE_SETTING inside the tests I had to use the Setting.Property.Dynamic property, but I don't want other users/code changing this percentage to something stupidly small and then the snapshotting takes too long and times out.

You can get the heap memory with Runtime.getRuntime().maxMemory() or JvmInfo.jvmInfo().getMem().getHeapMax(). Is that what you need?

I did consider this, but I thought it too static, and it also conflicted with the approach I took in #133558

My thought process is that by utilising the existing memorySizeSetting here to return 10% of the available heap memory for use when loading IndexMetadata objects, the actual % can be dynamically updated without requiring a code change which is nice, and the ByteSizeValue handles all the maths in the background. I still have a concern about this setting being abused by someone setting the value really low (say 1%) which will force us to use only a single snapshot thread, but this concern isn't specific to the MAX_HEAP_SIZE_FOR_INDEX_METADATA_SETTING setting, but a wider concern of any dynamically updatable setting.

My concern in:

If there is an easier way to get the total heap memory then please let me know

was because I was trying to get 100% of the heap via a setting which seemed long-winded, but after this comment by David it seems the best approach. Do you agree?

I think it's ok. I mean there's two made-up fiddle factors in play here, the "max 10% of heap" is a guess, as is the "max 50MiB of heap per IMD instance", but then the concurrency number is just one divided by the other. I'm not sure that's the most user-friendly interface really but my only other idea is to control the actual concurrency with a setting whose default is a function of JvmInfo.jvmInfo().getMem().getHeapMax(). One super-opaque setting or two slightly-less-opaque settings...

Prior to loading the IndexMetadata object from heap memory, can we check 1) how big the object we will be loading is, and 2) whether we have enough space to load it? Then, if we don't have space, we block until we do. It's not ideal, but better than OOMing?

Yet another possibility would be to introduce a variant of org.elasticsearch.cluster.metadata.IndexMetadata.Builder#fromXContent() which skips over any mappings. It's the mappings that take up most of the heap space in practice, but we simply don't need them here.

XContentPaser can be configured to filter out keys. For snapshots, we control the XContentParser creation in ChecksumBlobStoreFormat where we can configure it to skip the mappings key. I tested it briefly and it seems to be possible with the following change.

--- a/server/src/main/java/org/elasticsearch/repositories/blobstore/ChecksumBlobStoreFormat.java
+++ b/server/src/main/java/org/elasticsearch/repositories/blobstore/ChecksumBlobStoreFormat.java
 
@@ -150,7 +151,8 @@ public final class ChecksumBlobStoreFormat<T> {
                 try (
                     XContentParser parser = XContentHelper.createParserNotCompressed(
                         XContentParserConfiguration.EMPTY.withRegistry(namedXContentRegistry)
-                            .withDeprecationHandler(LoggingDeprecationHandler.INSTANCE),
+                            .withDeprecationHandler(LoggingDeprecationHandler.INSTANCE)
+                            .withFiltering(null, Set.of("*.mappings"), false),
                         bytesReference,
                         XContentType.SMILE
                     )
@@ -161,7 +163,8 @@ public final class ChecksumBlobStoreFormat<T> {
                     try (
                         XContentParser parser = XContentHelper.createParserNotCompressed(
                             XContentParserConfiguration.EMPTY.withRegistry(namedXContentRegistry)
-                                .withDeprecationHandler(LoggingDeprecationHandler.INSTANCE),
+                                .withDeprecationHandler(LoggingDeprecationHandler.INSTANCE)
+                                .withFiltering(null, Set.of("*.mappings"), false),
                             bytesReference,
                             XContentType.SMILE
                         )

If something like this is feasible, I'd think we don't need the memory limit which feels mostly a guess.

Interesting, TIL, I didn't know there was a filter-on-parse option.

Can we just use the includeStrings parameter? Really we only need settings.index.number_of_shards tho I could imagine we might have to pull a few more things in to satisfy various invariants on IndexMetadata.

On reflection, maybe we don't even need to load this as a full IndexMetadata. We could define another MetadataStateFormat<...> which defines a different fromXContent that only reads settings.index.number_of_shards and skips everything else.

I have pushed a WIP PR here if you guys wouldn't mind taking a look. If you agree with the approach, I would close this PR in favour of the above.

This was a heuristic since I didn't know what we wanted to throttle the snapshot threads to be.

For context to any reviewer, we currently have up to 10 snapshot threads on any size node, and in this instance a node with 1GiB heap OOMed because of this. I used a simple Math.pow(2, heapSizeInGb) heuristic, but this does mean that a node must have >4GB heap to use all 10 snapshot threads. I suspect this is far too conservative of an estimate so suggestions are appreciated. Happy to use a simple step function if people think that would suffice, ie if heap is less than 2GB, use 4 threads, else use 10

we currently have up to 10 snapshot threads on any size node

We do have fewer threads for smaller nodes, see here. The threshold is current 750MB. Are we considering this to be too low and want to raise it to something like 1GB or more? Or is this change specific about snapshot deletion? For the later, I'd probably just go with a simple heuristic like use half of the snapshot threads (minimal 1) if heap is smaller than 2GB?

This change is specific to index-metadata-loading during snapshot deletion, which is something we do today concurrently on all the available snapshot threads and which we've seen to need unusually large amounts of heap. Some IndexMetadata instances can be 50MiB or more when deserialized. Most of the work that the snapshot threads do has much lower heap footprint than this.

I don't think it makes sense to have the concurrency limit be an exponentially-increasing function of the heap size tho, since the resource usage is linear function of the concurrency limit. I'd suggest we take the observed 50MiB as a reasonable estimate of a big IndexMetadata blob and make sure we don't use more than, say, 10% of heap for this. So if the node has 1GiB of heap then allow 2 threads, 2GiB means 4 threads, etc.

Thanks both, based on the suggestion above I have extended BlobStoreRepository with a setting to read 10% of available heap memory, in the same way I did for MAX_HEAP_SIZE_FOR_SNAPSHOT_DELETION_SETTING

You can get the heap memory with Runtime.getRuntime().maxMemory() or JvmInfo.jvmInfo().getMem().getHeapMax(). Is that what you need?

we currently have up to 10 snapshot threads on any size node

We do have fewer threads for smaller nodes, see here. The threshold is current 750MB. Are we considering this to be too low and want to raise it to something like 1GB or more? Or is this change specific about snapshot deletion? For the later, I'd probably just go with a simple heuristic like use half of the snapshot threads (minimal 1) if heap is smaller than 2GB?