Add framework level constructs to track shard indexing pressure. #496

getsaurabh02 · 2021-04-05T14:52:36Z

Description

This PR is 1st of the 4 planned PRs for Shard Indexing Pressure ((#478). It provides the framework level constructs to track shard indexing pressure.

Shard Indexing Pressure introduces smart rejections of indexing requests when there are too many stuck/slow requests in the cluster, breaching key performance thresholds. This prevents the nodes in cluster to run into cascading effects of failures.

Co-authored-by: Dharmesh Singh sdharms@amazon.com

Issues Resolved

Addresses Item 1 of #478

Check List

[+ ] New functionality includes testing.
- [+ ] All tests pass
[+ ] New functionality has been documented.
- [+ ] New functionality has javadoc added
[+] Commits are signed per the DCO using --signoff

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.

odfe-release-bot · 2021-04-05T14:53:48Z

✅ Gradle Wrapper Validation success 225fb0fc3a4b4c2e22599811861434e5bb519429

odfe-release-bot · 2021-04-05T14:54:45Z

❌ DCO Check Failed 225fb0fc3a4b4c2e22599811861434e5bb519429
Run ./dev-tools/signoff-check.sh remotes/origin/main 225fb0fc3a4b4c2e22599811861434e5bb519429 to check locally
Use git commit with -s to add 'Signed-of-by: {EMAIL}' on impacted commits

odfe-release-bot · 2021-04-05T14:57:52Z

✅ Gradle Precommit success 225fb0fc3a4b4c2e22599811861434e5bb519429

Signed-off-by: Saurabh Singh <sisurab@amazon.com>

odfe-release-bot · 2021-04-05T14:59:49Z

✅ Gradle Wrapper Validation success 7a80862

odfe-release-bot · 2021-04-05T15:00:44Z

✅ DCO Check Passed 7a80862

odfe-release-bot · 2021-04-05T15:03:45Z

✅ Gradle Precommit success 7a80862

Bukhtawar

Thanks for the changes. Few comments some good to have.
It might be good to see some unit tests around ShardIndexingPressureMemoryManager while I see we have ITs around this

Bukhtawar · 2021-04-07T14:09:13Z

server/src/main/java/org/opensearch/cluster/service/ClusterService.java

@@ -190,6 +193,22 @@ public MasterService getMasterService() {
        return masterService;
    }

+    /**
+     * Getter and Setter for Indexing Pressure, This method is added specifically for getting IndexingPressure
+     * instance in ODFE PA plugin via ClusterService. Indexing Pressure instances can be accessible only via


Might be good to reword this as This method exposes IndexingPressure stats to other plugins

Bukhtawar · 2021-04-07T14:11:47Z

server/src/main/java/org/opensearch/index/ShardIndexingPressure.java

+        long shardCombinedBytes = tracker.currentCombinedCoordinatingAndPrimaryBytes.addAndGet(bytes);
+
+        boolean shardLevelLimitBreached = false;
+        if (!forceExecution) {


Following the convention lets use forceExecution == false and elsewhere

Sure, will update across.

Bukhtawar · 2021-04-07T14:14:24Z

server/src/main/java/org/opensearch/index/ShardIndexingPressure.java

+ * 4. Calling methods of {@link ShardIndexingPressureMemoryManager} to evaluate if a request can be process successfully
+ * and can increase the memory limits for a shard under certain scenarios
+ */
+public class ShardIndexingPressure {


Serves as a good candidate for extending IndexingPressure. Maybe good to extend

We intend to keep ShardIndexingPressure as a sub feature and is contained with IndexingPressure. Similarly other sub-features or related improvements can be added to IndexingPressure, while keeping IndexingPressure as a top level object for accessibility in plugins/actions. Hence will like to keep it same and can revisit this refactoring later if really needed.

Have introduced IndexingPressureService which acts as an orchestrator for method invocation of ShardIndexingPressure vs IndexingPressure (base), based on the dynamic setting value. Going forward IndexingPressureService can be the required construct for exposing lister/interfaces or building new constructs in parallel to current one for back-pressure.

Bukhtawar · 2021-04-07T14:24:24Z

server/src/main/java/org/opensearch/index/ShardIndexingPressureSettings.java

+    public static final Setting<Boolean> SHARD_INDEXING_PRESSURE_ENABLED =
+            Setting.boolSetting("aes.shard_indexing_pressure.enabled", false, Setting.Property.Dynamic, Setting.Property.NodeScope);
+
+    /**
+     * Feature level setting to operate in shadow-mode or in enforced-mode. If enforced field is set to true, shard level
+     * rejection will be performed, otherwise only rejection metrics will be populated.
+     */
+    public static final Setting<Boolean> SHARD_INDEXING_PRESSURE_ENFORCED =
+            Setting.boolSetting("aes.shard_indexing_pressure.enforced", false, Setting.Property.Dynamic, Setting.Property.NodeScope);
+
+    // This represents the last N request samples that will be considered for secondary parameter evaluation.
+    public static final Setting<Integer> REQUEST_SIZE_WINDOW =
+            Setting.intSetting("aes.shard_indexing_pressure.secondary_parameter.throughput.request_size_window", 2000,
+                Setting.Property.NodeScope, Setting.Property.Dynamic);
+
+    //Each shard will be initially given 1/1000th bytes of node limits.
+    public static final Setting<Double> SHARD_MIN_LIMIT =
+            Setting.doubleSetting("aes.shard_indexing_pressure.primary_parameter.shard.min_limit", 0.001d, 0.0d,
+                Setting.Property.NodeScope, Setting.Property.Dynamic);
+


Lets check if aes is still complaint to be used. Good to have enabled/enforced as discrete values supported against the same settings

Will confirm on the aes compliance. It provides control access from managed service perspective.
We have kept enabled & enforced as boolean settings for intuitiveness and simplicity. We also envision that in ideal state enforced as a setting can be independently deprecated faster, once we achieve a self sustainable model, hence easier cleanup/maintenance.
Let me know if you think otherwise.

Bukhtawar · 2021-04-07T15:31:21Z

server/src/main/java/org/opensearch/index/ShardIndexingPressureTracker.java

+ *
+ * For more details on 6,7,8,9,10,11 see {@link ShardIndexingPressureMemoryManager}
+ */
+public class ShardIndexingPressureTracker {


Might be good to break down the tracker

ShardIndexingPressureTracker is currently a simple object which acts as a container for about 12 fields related to shard level information. These are directly accessed/updated and breaking them further would add more indirections. Since the fields are not complex (only long values), we have clubbed them together with comments for added maintainability.
I believe we will learn from the first release, to add or remove more fields, and then based on the need we can break it down further.

The code blocks on related properties itself seem to suggest it can be broken down
In the spirit of encapsulation, lets also make fields private

Makes sense. Have introduced multiple inner classes by logically clubbing fields to add maintainability inside ShardIndexingPressureTracker.

Let's open a follow up on this. I propose something like

private RoleTracker primaryRoleTracker; private RoleTracker coordinatingRoleTracker; private RoleTracker replicaRoleTracker; private CommonTracker commonTracker; class RoleTracker { private final StatsTracker statsTracker = new StatsTracker(); private final PerformanceTracker performanceTracker = new PerformanceTracker(); } class PerformanceTracker { private final ConcurrentLinkedQueue<Double> throughputMovingQueue = new ConcurrentLinkedQueue(); private final AtomicLong throughputMovingAverage = new AtomicLong(); private final AtomicLong outstandingRequests = new AtomicLong(); private final AtomicLong lastSuccessfulRequestTimestamp = new AtomicLong(); private final AtomicLong lastSuccessfulRequestTimestampNew = new AtomicLong(); } class StatsTracker { private final AtomicLong timeInMillis = new AtomicLong(); private final AtomicLong count = new AtomicLong(); private final AtomicLong rejection = new AtomicLong(); } class CommonTracker { //common tracking properties go here }

++ Have added a list item in #478 to revisit the modelling.

Bukhtawar · 2021-04-07T15:34:05Z

server/src/main/java/org/opensearch/index/ShardIndexingPressureMemoryManager.java

+ * current shard utilization.
+ *
+ */
+public class ShardIndexingPressureMemoryManager {


Maybe changing limits on-demand when we are on the verge of soft limits breaching, would help simplify computations

I agree there is some scope of tuning and multiple ways with respect to how memory management can be done. Its hard to arrive at a perfect solution in the first go. I will like to keep this approach for the first release as planned, and then based on the learnings from the shadow/enforced mode metrics, we can improve the approaches around. We have performed performance and load testing around the changes using rally workload and have found no regressions.

…of comments. Signed-off-by: Saurabh Singh <sisurab@amazon.com>

odfe-release-bot · 2021-04-08T05:06:46Z

✅ DCO Check Passed afabf25

odfe-release-bot · 2021-04-08T05:07:52Z

✅ Gradle Wrapper Validation success afabf25

odfe-release-bot · 2021-04-08T05:12:02Z

✅ Gradle Precommit success afabf25

Signed-off-by: Dharmesh 💤 <sdharms@amazon.com>

odfe-release-bot · 2021-04-09T12:06:37Z

✅ Gradle Wrapper Validation success 71c4018

odfe-release-bot · 2021-04-09T12:06:37Z

✅ DCO Check Passed 71c4018

odfe-release-bot · 2021-04-09T12:15:39Z

✅ Gradle Precommit success 71c4018

Bukhtawar · 2021-04-10T06:49:53Z

server/src/main/java/org/opensearch/index/ShardIndexingPressure.java

+    private final Logger logger = LogManager.getLogger(getClass());
+
+    private final IndexingPressure indexingPressure;
+    private static ClusterService clusterService;


Any reason why this is static

This is used to access & verify the node level attribute for shard-indexing-pressure for ser/de from Node Stats.

Bukhtawar

I'm yet to review tests. I'll share more comments as I look through deeper

Bukhtawar · 2021-04-10T07:30:22Z

server/src/main/java/org/opensearch/index/ShardIndexingPressureTracker.java

+ *
+ * For more details on 6,7,8,9,10,11 see {@link ShardIndexingPressureMemoryManager}
+ */
+public class ShardIndexingPressureTracker {


The code blocks on related properties itself seem to suggest it can be broken down
In the spirit of encapsulation, lets also make fields private

Bukhtawar · 2021-04-10T10:27:01Z

server/src/main/java/org/opensearch/common/settings/ClusterSettings.java

+            ShardIndexingPressureSettings.SHARD_INDEXING_PRESSURE_ENABLED,
+            ShardIndexingPressureSettings.SHARD_INDEXING_PRESSURE_ENFORCED,
+            ShardIndexingPressureSettings.REQUEST_SIZE_WINDOW,
+            ShardIndexingPressureSettings.SHARD_MIN_LIMIT,
+            ShardIndexingPressureStore.MAX_CACHE_STORE_SIZE,
+            ShardIndexingPressureMemoryManager.LOWER_OPERATING_FACTOR,
+            ShardIndexingPressureMemoryManager.OPTIMAL_OPERATING_FACTOR,
+            ShardIndexingPressureMemoryManager.UPPER_OPERATING_FACTOR,
+            ShardIndexingPressureMemoryManager.NODE_SOFT_LIMIT,
+            ShardIndexingPressureMemoryManager.THROUGHPUT_DEGRADATION_LIMITS,
+            ShardIndexingPressureMemoryManager.SUCCESSFUL_REQUEST_ELAPSED_TIMEOUT,
+            ShardIndexingPressureMemoryManager.MAX_OUTSTANDING_REQUESTS,


These might need doc update whenever we have one

Bukhtawar · 2021-04-10T15:34:25Z

server/src/main/java/org/opensearch/node/Node.java

@@ -732,6 +737,14 @@ protected Node(final Environment initialEnvironment,
        }
    }

+    private static Settings addShardIndexingBackPressureAttributeSettings(Settings settings) {
+        // Shard Indexing BackPressure is enabled from AES-7.9 onwards.


Lets scan through all AES specific interactions

Thanks for pointing this. Have verified and removed

Bukhtawar · 2021-04-10T15:57:10Z

server/src/main/java/org/opensearch/node/Node.java

@@ -586,7 +590,8 @@ protected Node(final Environment initialEnvironment,
            final SearchTransportService searchTransportService =  new SearchTransportService(transportService,
                SearchExecutionStatsCollector.makeWrapper(responseCollectorService));
            final HttpServerTransport httpServerTransport = newHttpTransport(networkModule);
-            final IndexingPressure indexingLimits = new IndexingPressure(settings);
+            final IndexingPressure indexingLimits = new IndexingPressure(settings, clusterService);
+            clusterService.setIndexingPressure(indexingLimits);


Lets think for a more generic way to expose metrics preferably through a ShardIndexingPressureListener that exposes methods to be extended by the plugins.

Ideally we want plugins to be able to do an on-demand fetch and get the current view of the system. This allows a quick way for plugin for now. Event listener for shard tracker might get too overwhelmed by huge amount of concurrent updates during indexing workload. Will need to think through on how we can do this in a generic and optimised way. Added item to the the meta issue #478 list for a followup.

Bukhtawar · 2021-04-10T16:02:40Z

server/src/main/java/org/opensearch/node/Node.java

+        String ShardIndexingBackPressureEnabledValue = "true";
+        return Settings.builder().put(settings)
+                .put(NODE_ATTRIBUTES.getKey() + SHARD_INDEXING_PRESSURE_ENABLED_ATTRIBUTE_KEY, ShardIndexingBackPressureEnabledValue)
+                .build();
+    }


Wondering if this can go in the yaml instead of code here

Keeping this in code prevent unintentional updates or issues due to yml copy etc, as this can lead to severe serialisation issue otherwise.

Bukhtawar · 2021-04-10T16:15:58Z