Add BWC tests to prevent throttling key removal

[mohit10011999]

Description

Add BWC tests for persisted throttling task keys

Add rolling-upgrade test for throttling settings

Related Issues

Resolves #[Issue number to be closed when this PR is merged]

Check List

• Functionality includes testing.
• API changes companion pull request created, if applicable.
• Public documentation issue/PR created, if applicable.

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.

[github-actions]

PR Reviewer Guide 🔍

(Review updated until commit f3fdd32)

Here are some key observations to aid the review process:

🧪 PR contains tests

🔒 No security concerns identified

✅ No TODO sections

🔀 No multiple PR themes

⚡ Recommended focus areas for review Overly Strict BWC Assertion testBWCAllCurrentEnumValuesAreCoveredByHistoricalSet fails whenever a new task key is added to the ClusterManagerTask enum, even though a brand-new key has not yet been persisted by any prior release and does not pose a BWC risk. This forces contributors to modify a test's "historically persisted" set for every new task, which conflates future keys with truly historical ones and can be confusing. Consider gating this check differently (e.g., a separate "known keys" list) or documenting the workflow more clearly. public void testBWCAllCurrentEnumValuesAreCoveredByHistoricalSet() { Set<String> currentEnumKeys = new HashSet<>(); for (ClusterManagerTask task : ClusterManagerTask.values()) { currentEnumKeys.add(task.getKey()); } Set<String> missingFromBWC = new HashSet<>(currentEnumKeys); missingFromBWC.removeAll(HISTORICALLY_PERSISTED_TASK_KEYS); assertTrue( "The following ClusterManagerTask keys exist in the enum but are NOT in " + "HISTORICALLY_PERSISTED_TASK_KEYS. Once a key is added to the enum and registered, " + "it may be persisted in cluster state and must be protected from removal. " + "Add these keys to HISTORICALLY_PERSISTED_TASK_KEYS: " + missingFromBWC, missingFromBWC.isEmpty() ); } Fragile Response Parsing verifyThrottlingSettingsInClusterState assumes the cluster settings response returns nested maps (cluster_manager -> throttling -> thresholds -> <key> -> value). The _cluster/settings API can return settings in flattened dotted form depending on flat_settings or defaults, in which case persistent.get("cluster_manager") will be null and the test will fail even when settings are correctly persisted. Consider using ?flat_settings=true and parsing dotted keys, or handling both formats. private void verifyThrottlingSettingsInClusterState(String taskKey, int expectedValue) throws IOException { Request request = new Request("GET", "_cluster/settings"); Response response = client().performRequest(request); assertEquals(200, response.getStatusLine().getStatusCode()); Map<String, Object> responseMap = entityAsMap(response); Map<String, Object> persistent = (Map<String, Object>) responseMap.get("persistent"); assertNotNull("Persistent settings should not be null", persistent); // Navigate the nested structure: cluster_manager.throttling.thresholds.<key>.value Map<String, Object> clusterManager = (Map<String, Object>) persistent.get("cluster_manager"); assertNotNull("cluster_manager settings should exist in persistent settings", clusterManager); Map<String, Object> throttling = (Map<String, Object>) clusterManager.get("throttling"); assertNotNull("cluster_manager.throttling settings should exist", throttling); Map<String, Object> thresholds = (Map<String, Object>) throttling.get("thresholds"); assertNotNull("cluster_manager.throttling.thresholds should exist", thresholds); Map<String, Object> taskSettings = (Map<String, Object>) thresholds.get(taskKey); assertNotNull( "Throttling settings for task key '" + taskKey + "' should still be persisted after upgrade. " + "If this is null, the task key may have been removed from the ClusterManagerTask enum " + "or its registration was removed, causing settings validation to fail during upgrade.", taskSettings ); String value = (String) taskSettings.get("value"); assertEquals( "Throttling threshold for '" + taskKey + "' should match the value set in old cluster", String.valueOf(expectedValue), value ); }

[github-actions]

PR Code Suggestions ✨

Latest suggestions up to f3fdd32
Explore these optional code suggestions:

Category	Suggestion	Impact
Possible issue	Avoid ClassCastException on settings value The _cluster/settings API may return numeric values as either String or Integer depending on the flat/nested response format and version. Casting directly to String can throw a ClassCastException on some versions. Convert the returned object using String.valueOf to be safe. qa/rolling-upgrade/src/test/java/org/opensearch/upgrades/ClusterManagerThrottlingSettingsIT.java [90-95] -String value = (String) taskSettings.get("value"); +Object value = taskSettings.get("value"); assertEquals( "Throttling threshold for '" + taskKey + "' should match the value set in old cluster", String.valueOf(expectedValue), - value + String.valueOf(value) ); Suggestion importance[1-10]: 6 __ Why: Reasonable defensive coding — the returned type from the settings API could vary, and using String.valueOf avoids a potential ClassCastException making the test more robust.	Low
General	Use safer threshold values within valid range The throttling threshold minimum is typically enforced (e.g., must be >= a minimum like 5 or 10). Using 100 for create-index is fine but 5000 for put-mapping may exceed maximum allowed values on some releases, and rejection on the OLD cluster would silently mask the BWC scenario. Choose modest values within known valid range (e.g., 50, 100) to ensure the PUT succeeds on all supported OLD versions. qa/rolling-upgrade/src/test/java/org/opensearch/upgrades/ClusterManagerThrottlingSettingsIT.java [35-36] public void testThrottlingSettingsSurviveRollingUpgrade() throws Exception { if (CLUSTER_TYPE == ClusterType.OLD) { // On the old cluster, persist throttling settings for several task types // These will be written into cluster state metadata - setThrottlingSetting("put-mapping", 5000); + setThrottlingSetting("put-mapping", 50); setThrottlingSetting("create-index", 100); Suggestion importance[1-10]: 4 __ Why: The suggestion is speculative — there's no clear evidence that 5000 exceeds the max threshold. However, using more conservative values does reduce risk of test failures across versions.	Low

---

Previous suggestions

Suggestions up to commit a1c4b3d

Category	Suggestion	Impact
General	Avoid unsafe cast on setting value Casting taskSettings.get("value") directly to String may throw ClassCastException if the JSON parser returns a numeric type, and it silently assumes string representation. Convert to string via String.valueOf to be robust against parser type differences. qa/rolling-upgrade/src/test/java/org/opensearch/upgrades/ClusterManagerThrottlingSettingsIT.java [90-95] -String value = (String) taskSettings.get("value"); +Object valueObj = taskSettings.get("value"); +assertNotNull("Throttling value for '" + taskKey + "' should not be null", valueObj); assertEquals( "Throttling threshold for '" + taskKey + "' should match the value set in old cluster", String.valueOf(expectedValue), - value + String.valueOf(valueObj) ); Suggestion importance[1-10]: 5 __ Why: A valid defensive improvement — cluster settings responses may return values as strings or other types, and using String.valueOf avoids a potential ClassCastException, making the test more robust.	Low
	Handle cluster type phases explicitly The AbstractRollingTestCase typically has three phases: OLD, MIXED, and UPGRADED. The current else branch will attempt to set-and-verify only during OLD, but since settings are only PUT in OLD, verification during MIXED and UPGRADED both re-check the same persisted values. However, if the test runs multiple times or state is reset between phases, the else fallback may not correctly distinguish these phases. Explicitly handle each ClusterType to make phase behavior clear and avoid unintended verification when CLUSTER_TYPE is a value not accounted for. qa/rolling-upgrade/src/test/java/org/opensearch/upgrades/ClusterManagerThrottlingSettingsIT.java [41-50] -} else { +} else if (CLUSTER_TYPE == ClusterType.MIXED || CLUSTER_TYPE == ClusterType.UPGRADED) { // After upgrade (mixed or fully upgraded), verify the persisted settings // are still valid and the cluster is healthy (no node startup failures) verifyThrottlingSettingsInClusterState("put-mapping", 5000); verifyThrottlingSettingsInClusterState("create-index", 100); // Also verify cluster health — if settings restoration failed, nodes would // have crashed and the cluster would not be green/yellow verifyClusterHealth(); } Suggestion importance[1-10]: 3 __ Why: Since ClusterType only has three values (OLD, MIXED, UPGRADED), the else branch is already correct and equivalent. The suggestion offers marginal readability improvement with no functional impact.	Low

[github-actions]

Persistent review updated to latest commit f3fdd32

[github-actions]

❌ Gradle check result for f3fdd32: FAILURE

Please examine the workflow log, locate, and copy-paste the failure(s) below, then iterate to green. Is the failure a flaky test unrelated to your change?