Fix 0 default value for repo snapshot speed (elastic#95854) (elastic#…

…95964)

When node bandwidth recovery settings are defined.

Fixes elastic#95561

docs/changelog/95854.yaml

@@ -0,0 +1,6 @@
+pr: 95854
+summary: Fix 0 default value for repo snapshot speed
+area: Snapshot/Restore
+type: bug
+issues:
+ - 95561

server/src/internalClusterTest/java/org/elasticsearch/repositories/blobstore/BlobStoreDynamicSettingsIT.java

@@ -7,6 +7,7 @@
  */
 package org.elasticsearch.repositories.blobstore;
 
+import org.apache.lucene.store.RateLimiter;
 import org.elasticsearch.ExceptionsHelper;
 import org.elasticsearch.action.ActionFuture;
 import org.elasticsearch.action.admin.cluster.snapshots.create.CreateSnapshotResponse;
@@ -18,6 +19,11 @@
 import org.elasticsearch.snapshots.mockstore.MockRepository;
 import org.elasticsearch.test.ESIntegTestCase;
 
+import static org.elasticsearch.indices.recovery.RecoverySettings.INDICES_RECOVERY_MAX_BYTES_PER_SEC_SETTING;
+import static org.elasticsearch.indices.recovery.RecoverySettings.NODE_BANDWIDTH_RECOVERY_DISK_READ_SETTING;
+import static org.elasticsearch.indices.recovery.RecoverySettings.NODE_BANDWIDTH_RECOVERY_DISK_WRITE_SETTING;
+import static org.elasticsearch.indices.recovery.RecoverySettings.NODE_BANDWIDTH_RECOVERY_NETWORK_SETTING;
+import static org.hamcrest.Matchers.equalTo;
 import static org.hamcrest.Matchers.instanceOf;
 
 @ESIntegTestCase.ClusterScope(scope = ESIntegTestCase.Scope.TEST, numDataNodes = 0)
@@ -121,4 +127,33 @@ largeSnapshotPool && randomBoolean()
         unblockNode(repoName, dataNode);
         assertSuccessful(snapshot1);
     }
+
+    public void testDefaultRateLimits() throws Exception {
+        boolean nodeBandwidthSettingsSet = randomBoolean();
+        Settings.Builder nodeSettings = Settings.builder();
+        if (randomBoolean()) {
+            nodeSettings = nodeSettings.put(INDICES_RECOVERY_MAX_BYTES_PER_SEC_SETTING.getKey(), "100m");
+        }
+        if (nodeBandwidthSettingsSet) {
+            nodeSettings = nodeSettings.put(NODE_BANDWIDTH_RECOVERY_NETWORK_SETTING.getKey(), "100m")
+                .put(NODE_BANDWIDTH_RECOVERY_DISK_READ_SETTING.getKey(), "100m")
+                .put(NODE_BANDWIDTH_RECOVERY_DISK_WRITE_SETTING.getKey(), "100m");
+        }
+        final String node = internalCluster().startMasterOnlyNode(nodeSettings.build());
+
+        String repoName = "test-repo";
+        createRepository(repoName, "mock", randomRepositorySettings());
+        final BlobStoreRepository repository = getRepositoryOnNode("test-repo", node);
+
+        RateLimiter snapshotRateLimiter = repository.getSnapshotRateLimiter();
+        if (nodeBandwidthSettingsSet) {
+            assertNull("default snapshot rate limiter should be null", snapshotRateLimiter);
+        } else {
+            assertThat("default snapshot speed should be 40mb/s", snapshotRateLimiter.getMBPerSec(), equalTo(40.0));
+        }
+        RateLimiter restoreRateLimiter = repository.getRestoreRateLimiter();
+        assertNull("default restore rate limiter should be null", restoreRateLimiter);
+
+        deleteRepository("test-repo");
+    }
 }

server/src/main/java/org/elasticsearch/indices/recovery/RecoverySettings.java

@@ -701,7 +701,7 @@ private static void validateNodeBandwidthRecoverySettings(Settings settings) {
     /**
      * Whether the node bandwidth recovery settings are set.
      */
-    public static boolean hasNodeBandwidthRecoverySettings(Settings settings) {
+    private static boolean hasNodeBandwidthRecoverySettings(Settings settings) {
         return NODE_BANDWIDTH_RECOVERY_SETTINGS.stream()
             .filter(setting -> setting.get(settings) != ByteSizeValue.MINUS_ONE)
             .count() == NODE_BANDWIDTH_RECOVERY_SETTINGS.size();

server/src/main/java/org/elasticsearch/repositories/blobstore/BlobStoreRepository.java

@@ -313,13 +313,7 @@ public abstract class BlobStoreRepository extends AbstractLifecycleComponent imp
 
     public static final Setting<ByteSizeValue> MAX_SNAPSHOT_BYTES_PER_SEC = Setting.byteSizeSetting(
         "max_snapshot_bytes_per_sec",
-        (settings) -> {
-            if (RecoverySettings.hasNodeBandwidthRecoverySettings(settings)) {
-                return "0";
-            } else {
-                return "40mb";
-            }
-        },
+        ByteSizeValue.ofMb(40), // default is overridden to 0 (unlimited) if node bandwidth recovery settings are set
         Setting.Property.Dynamic,
         Setting.Property.NodeScope
     );
@@ -1620,19 +1614,18 @@ public BlobContainer shardContainer(IndexId indexId, int shardId) {
     /**
      * Configures RateLimiter based on repository and global settings
      *
-     * @param rateLimiter        the existing rate limiter to configure (or null if no throttling was previously needed)
-     * @param repositorySettings repository settings
-     * @param setting            setting to use to configure rate limiter
-     * @param warnIfOverRecovery log a warning if rate limit setting is over the effective recovery rate limit
+     * @param rateLimiter              the existing rate limiter to configure (or null if no throttling was previously needed)
+     * @param maxConfiguredBytesPerSec the configured max bytes per sec from the settings
+     * @param settingKey               setting used to configure the rate limiter
+     * @param warnIfOverRecovery       log a warning if rate limit setting is over the effective recovery rate limit
      * @return the newly configured rate limiter or null if no throttling is needed
      */
     private RateLimiter getRateLimiter(
         RateLimiter rateLimiter,
-        Settings repositorySettings,
-        Setting<ByteSizeValue> setting,
+        ByteSizeValue maxConfiguredBytesPerSec,
+        String settingKey,
         boolean warnIfOverRecovery
     ) {
-        ByteSizeValue maxConfiguredBytesPerSec = setting.get(repositorySettings);
         if (maxConfiguredBytesPerSec.getBytes() <= 0) {
             return null;
         } else {
@@ -1643,7 +1636,7 @@ private RateLimiter getRateLimiter(
                         "repository [{}] has a rate limit [{}={}] per second which is above the effective recovery rate limit "
                             + "[{}={}] per second, thus the repository rate limit will be superseded by the recovery rate limit",
                         metadata.name(),
-                        setting.getKey(),
+                        settingKey,
                         maxConfiguredBytesPerSec,
                         INDICES_RECOVERY_MAX_BYTES_PER_SEC_SETTING.getKey(),
                         effectiveRecoverySpeed
@@ -1660,17 +1653,30 @@ private RateLimiter getRateLimiter(
         }
     }
 
-    private RateLimiter getSnapshotRateLimiter() {
+    // package private for testing
+    RateLimiter getSnapshotRateLimiter() {
+        Settings repositorySettings = metadata.settings();
+        ByteSizeValue maxConfiguredBytesPerSec = MAX_SNAPSHOT_BYTES_PER_SEC.get(repositorySettings);
+        if (MAX_SNAPSHOT_BYTES_PER_SEC.exists(repositorySettings) == false && recoverySettings.nodeBandwidthSettingsExist()) {
+            assert maxConfiguredBytesPerSec.getMb() == 40;
+            maxConfiguredBytesPerSec = ByteSizeValue.ZERO;
+        }
         return getRateLimiter(
             snapshotRateLimiter,
-            metadata.settings(),
-            MAX_SNAPSHOT_BYTES_PER_SEC,
+            maxConfiguredBytesPerSec,
+            MAX_SNAPSHOT_BYTES_PER_SEC.getKey(),
             recoverySettings.nodeBandwidthSettingsExist()
         );
     }
 
-    private RateLimiter getRestoreRateLimiter() {
-        return getRateLimiter(restoreRateLimiter, metadata.settings(), MAX_RESTORE_BYTES_PER_SEC, true);
+    // package private for testing
+    RateLimiter getRestoreRateLimiter() {
+        return getRateLimiter(
+            restoreRateLimiter,
+            MAX_RESTORE_BYTES_PER_SEC.get(metadata.settings()),
+            MAX_RESTORE_BYTES_PER_SEC.getKey(),
+            true
+        );
     }
 
     @Override