Reject out-of-range WLM node threshold updates at validation time

[dzane17]

Description

The four node-level WLM threshold settings — wlm.workload_group.node.{cpu,memory}_{rejection,cancellation}_threshold — had their range checks (max value, and the rejection <= cancellation ordering invariant) enforced only inside their settings-update consumers (the setters registered via addSettingsUpdateConsumer).

Cluster settings updates are validated by a dry run (ClusterSettings.validateUpdate / validate) that runs each setting's parser and its Setting.Validator, but not its update consumer. The consumer runs later, when the committed cluster state is applied. So an out-of-range value (e.g. cpu_cancellation_threshold = 0.98, above the 0.95 max) passed validation, was committed to cluster state, and only threw IllegalArgumentException at apply time. On the elected cluster-manager that aborts cluster-state application, causing it to step down and re-elect repeatedly; because the bad value is already persisted, a corrective update cannot be published either.

This change attaches a Setting.Validator to each of the four settings so the bounds and ordering invariant are enforced at validation time — the update is rejected up front with a 400 and never committed. Key points:

• Single-value bounds (non-negative, and max: 0.95 for cancellation, 0.90 for rejection) are enforced by the validator and by the setters, via a shared helper so the two paths cannot drift.
• The cross-setting rejection <= cancellation ordering invariant is enforced only in the validator (against the final, consistent settings) and at startup in the constructor — deliberately not in the setters. Consumers are applied one setting at a time, so a setter checking ordering against the sibling's not-yet-updated field would throw when both thresholds are lowered together (a consistent final state), reintroducing the same apply-time failure.
• Values already persisted from before this change are archived on gateway recovery rather than applied, so an affected cluster self-heals on restart.

Related Issues

N/A

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.

[github-actions]

PR Reviewer Guide 🔍

(Review updated until commit 56e8d35)

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 Constructor invariant no longer enforced The PR description states the rejection <= cancellation ordering invariant is enforced "at startup in the constructor", but the setters no longer perform any validation and no explicit constructor check appears in the diff. If a node starts with initial settings where rejection > cancellation (e.g., from opensearch.yml or persisted settings that bypass dynamic validation), the invariant will not be caught at startup. Confirm whether ClusterSettings runs the Validator.validate(value, Map) cross-setting check during initial settings parsing; if not, an explicit startup check should be added. static final class NodeLevelThresholdValidator implements Setting.Validator<Double> { private final String settingName; private final double maxValue; private final Supplier<Setting<Double>> pairedSetting; private final boolean isCancellation; private NodeLevelThresholdValidator( String settingName, double maxValue, Supplier<Setting<Double>> pairedSetting, boolean isCancellation ) { this.settingName = settingName; this.maxValue = maxValue; this.pairedSetting = pairedSetting; this.isCancellation = isCancellation; } /** * Builds a validator for a cancellation threshold (the upper bound of the pair). * @param settingName this cancellation setting's key * @param maxValue the maximum allowed value for this cancellation threshold * @param rejectionSetting supplier of the paired rejection setting */ static NodeLevelThresholdValidator forCancellationThreshold( String settingName, double maxValue, Supplier<Setting<Double>> rejectionSetting ) { return new NodeLevelThresholdValidator(settingName, maxValue, rejectionSetting, true); } /** * Builds a validator for a rejection threshold (the lower bound of the pair). * @param settingName this rejection setting's key * @param maxValue the maximum allowed value for this rejection threshold * @param cancellationSetting supplier of the paired cancellation setting */ static NodeLevelThresholdValidator forRejectionThreshold( String settingName, double maxValue, Supplier<Setting<Double>> cancellationSetting ) { return new NodeLevelThresholdValidator(settingName, maxValue, cancellationSetting, false); } @Override public void validate(Double value) { ensureThresholdIsNotNegative(value, settingName); ensureThresholdIsNotGreaterThanMax(value, maxValue, settingName); } @Override public void validate(Double value, Map<Setting<?>, Object> settings) { final String pairedName = pairedSetting.get().getKey(); final Double pairedValue = (Double) settings.get(pairedSetting.get()); // Substitute this setting's incoming value for its own side of the rejection <= cancellation comparison. if (isCancellation) { ensureRejectionThresholdIsLessThanCancellation(pairedValue, value, pairedName, settingName); } else { ensureRejectionThresholdIsLessThanCancellation(value, pairedValue, settingName, pairedName); } } @Override public Iterator<Setting<?>> settings() { return List.<Setting<?>>of(pairedSetting.get()).iterator(); } } Negative check via Double.compare edge case ensureThresholdIsNotNegative uses Double.compare(value, 0.0) < 0, which treats -0.0 as less than 0.0 and would reject a -0.0 input as "negative". While unlikely in practice, a stricter and more intuitive check would be value < 0.0. Minor correctness/UX concern. private static void ensureThresholdIsNotNegative(Double thresholdValue, String thresholdSettingName) { if (Double.compare(thresholdValue, 0.0) < 0) { throw new IllegalArgumentException(thresholdSettingName + " value cannot be negative"); } }

[github-actions]

PR Code Suggestions ✨

Latest suggestions up to 56e8d35
Explore these optional code suggestions:

Category	Suggestion	Impact
Possible issue	Handle null paired value in cross-setting validation Guard against a null pairedValue from the settings map. If the paired setting is not present in the update batch, the map lookup can return null, causing a NullPointerException in Double.compare inside ensureRejectionThresholdIsLessThanCancellation. Skip the cross-setting check when the paired value is unavailable. server/src/main/java/org/opensearch/wlm/WorkloadManagementSettings.java [428-437] @Override public void validate(Double value, Map<Setting<?>, Object> settings) { final String pairedName = pairedSetting.get().getKey(); final Double pairedValue = (Double) settings.get(pairedSetting.get()); - // Substitute this setting's incoming value for its own side of the rejection <= cancellation comparison. + if (pairedValue == null) { + return; + } if (isCancellation) { ensureRejectionThresholdIsLessThanCancellation(pairedValue, value, pairedName, settingName); } else { ensureRejectionThresholdIsLessThanCancellation(value, pairedValue, settingName, pairedName); } } Suggestion importance[1-10]: 4 __ Why: The concern about a potential NPE is theoretically valid, but in practice OpenSearch's ClusterSettings.validate populates the settings map with all declared dependent settings (via settings()), so pairedValue should not be null. The suggestion is defensive but of limited practical impact.	Low

---

Previous suggestions

Suggestions up to commit 9a3ccc8

Category	Suggestion	Impact
General	Guard cross-setting validator against null sibling The two-arg validate is invoked even when the sibling's single-value validate has already failed (e.g. when the sibling is out-of-range or negative), which can lead to a confusing ordering-error message masking the real cause. Guard the ordering check by first re-validating this setting's own bounds, or at least ensure the sibling value is within valid range before comparing, so the primary bound violation surfaces first. This also protects against comparing against a null sibling if lookup ever returns null. server/src/main/java/org/opensearch/wlm/WorkloadManagementSettings.java [406-414] @Override public void validate(Double value, Map<Setting<?>, Object> settings) { final Double rejectionThreshold = (Double) settings.get(NODE_LEVEL_CPU_REJECTION_THRESHOLD); + if (rejectionThreshold == null) { + return; + } ensureRejectionThresholdIsLessThanCancellation( rejectionThreshold, value, NODE_CPU_REJECTION_THRESHOLD_SETTING_NAME, NODE_CPU_CANCELLATION_THRESHOLD_SETTING_NAME ); } Suggestion importance[1-10]: 3 __ Why: The sibling setting is a required cluster setting with a default value, so settings.get(...) should not return null in practice. The suggestion is defensive but low-impact, and adding an early return could mask genuine misconfigurations rather than improve robustness.	Low

[github-actions]

❌ Gradle check result for 9a3ccc8: 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?

[kkhatua]

@jainankitk @rajiv-kv @kaushalmahi12
Could you please review this?

[github-actions]

Persistent review updated to latest commit 56e8d35

[github-actions]

❌ Gradle check result for 56e8d35: 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?