[CORRUPTED] Synthetic Benchmark PR #137964 - Fix for GET /_migration/deprecations doesn't report node deprecations if low watermark exceeded and GET /_migration/deprecations doesn't report node-level failures properly

[tomerqodo]

User description

Benchmark PR elastic#137964

Type: Corrupted (contains bugs)

Original PR Title: Fix for GET /_migration/deprecations doesn't report node deprecations if low watermark exceeded and GET /_migration/deprecations doesn't report node-level failures properly
Original PR Description: Closes elastic#137010
Closes elastic#137004
Original PR URL: elastic#137964

---

PR Type

Bug fix

---

Description

• Fix disk low watermark deprecation check not being included in results

• Correct node settings filtering to use unfiltered settings for watermark check

• Improve error logging with structured logging for node failures

• Add comprehensive test coverage for disk watermark deprecation warnings

---

Diagram Walkthrough

flowchart LR
  A["Node Deprecation Check"] --> B["Filter Settings"]
  B --> C["Apply Deprecation Checks"]
  C --> D["Check Disk Watermark"]
  D --> E["Combine Issues"]
  E --> F["Return Results"]
  style D fill:#ff9999

Loading

File Walkthrough

	Relevant files
Error handling	NodeDeprecationChecker.java Enhanced error logging for node failures                                  x-pack/plugin/deprecation/src/main/java/org/elasticsearch/xpack/deprecation/NodeDeprecationChecker.java Improved error logging for node failures using structured logging API Changed from logger.debug() to logger.atInfo().withThrowable() for better exception context +1/-1
Bug fix	TransportNodeDeprecationCheckAction.java Fix watermark check and refactor deprecation issue collection x-pack/plugin/deprecation/src/main/java/org/elasticsearch/xpack/deprecation/TransportNodeDeprecationCheckAction.java Removed unused NodeRequest parameter from nodeOperation() method Changed from filtered settings to unfiltered settings for disk watermark check Replaced stream-based filtering with explicit loop to preserve all deprecation issues Fixed bug where watermark check was using filtered settings instead of original settings +15/-8
Tests	TransportNodeDeprecationCheckActionTests.java Add comprehensive test for watermark deprecation warnings x-pack/plugin/deprecation/src/test/java/org/elasticsearch/xpack/deprecation/TransportNodeDeprecationCheckActionTests.java Added new test testDiskLowWatermarkIsIncludedInDeprecationWarnings() to verify watermark issues are included Created custom DeprecationIssueMatcher for flexible assertion matching Updated existing tests to remove unused NodeRequest parameter Added imports for test utilities and matchers +102/-4
Documentation	137964.yaml Add changelog entry for bug fix                                                    docs/changelog/137964.yaml Created changelog entry documenting the bug fix References issues GET /_migration/deprecations doesn't report node-level failures properly elastic/elasticsearch#137010 and GET /_migration/deprecations doesn't report node deprecations if low watermark exceeded elastic/elasticsearch#137004 Categorized as infrastructure/core bug fix +9/-0

---

[qodo-code-review]

PR Compliance Guide 🔍

Below is a summary of compliance checks for this PR:

Security Compliance
🟢	No security concerns identified No security vulnerabilities detected by AI analysis. Human verification advised for critical code.
Ticket Compliance
⚪	🎫 No ticket provided Create ticket/issue
Codebase Duplication Compliance
⚪	Codebase context is not defined Follow the guide to enable codebase context checks.
Custom Compliance
🟢	Generic: Meaningful Naming and Self-Documenting Code Objective: Ensure all identifiers clearly express their purpose and intent, making code self-documenting Status: Passed Learn more about managing compliance generic rules or creating your own custom rules
	Generic: Robust Error Handling and Edge Case Management Objective: Ensure comprehensive error handling that provides meaningful context and graceful degradation Status: Passed Learn more about managing compliance generic rules or creating your own custom rules
	Generic: Security-First Input Validation and Data Handling Objective: Ensure all data inputs are validated, sanitized, and handled securely to prevent vulnerabilities Status: Passed Learn more about managing compliance generic rules or creating your own custom rules
⚪	Generic: Comprehensive Audit Trails Objective: To create a detailed and reliable record of critical system actions for security analysis and compliance. Status: Insufficient auditing: The change adds informational logging for node failures but does not constitute comprehensive audit logging of critical actions with user context, which may be outside this component’s scope. Referred Code logger.atInfo().withThrowable(failure).log("node {} failed to run deprecation checks", failure.nodeId()); } Learn more about managing compliance generic rules or creating your own custom rules
	Generic: Secure Error Handling Objective: To prevent the leakage of sensitive system information through error messages while providing sufficient detail for internal debugging. Status: Exception exposure: Logging at info with the throwable for node failures may include stack traces or internal details that could leak if logs are user-visible, though likely intended for internal logs. Referred Code logger.atInfo().withThrowable(failure).log("node {} failed to run deprecation checks", failure.nodeId()); } Learn more about managing compliance generic rules or creating your own custom rules
	Generic: Secure Logging Practices Objective: To ensure logs are useful for debugging and auditing without exposing sensitive information like PII, PHI, or cardholder data. Status: Throwable logging: The new structured log includes the full throwable which could contain sensitive internal details; no evidence of redaction is shown in the diff. Referred Code logger.atInfo().withThrowable(failure).log("node {} failed to run deprecation checks", failure.nodeId()); } Learn more about managing compliance generic rules or creating your own custom rules

Compliance status legend

🟢 - Fully Compliant
🟡 - Partial Compliant
🔴 - Not Compliant
⚪ - Requires Further Human Verification
🏷️ - Compliance label

[qodo-code-review]

PR Code Suggestions ✨

Explore these optional code suggestions:

Category	Suggestion	Impact
High-level	The fix incorrectly ignores user-configured deprecation skips The PR's fix overcorrects by passing unfiltered node settings to all deprecation checks, thereby ignoring the skip_deprecations configuration. This reintroduces previously suppressed warnings, causing a functional regression. Examples: x-pack/plugin/deprecation/src/main/java/org/elasticsearch/xpack/deprecation/TransportNodeDeprecationCheckAction.java [133-144] List<DeprecationIssue> issues = new ArrayList<>(); for (NodeDeprecationChecks.NodeDeprecationCheck< Settings, PluginsAndModules, ClusterState, XPackLicenseState, DeprecationIssue> c : nodeSettingsChecks) { DeprecationIssue deprecationIssue = c.apply(settings, pluginsService.info(), filteredClusterState, licenseState); if (deprecationIssue != null) { issues.add(deprecationIssue); ... (clipped 2 lines) Solution Walkthrough: Before: class TransportNodeDeprecationCheckAction { NodeResponse nodeOperation(...) { // Settings are filtered based on 'skip_deprecations' Settings filteredNodeSettings = settings.filter(...); // All checks use the filtered settings List<DeprecationIssue> issues = nodeSettingsChecks.stream() .map(c -> c.apply(filteredNodeSettings, ...)) .toList(); // Watermark check also uses filtered settings, which was the bug DeprecationIssue watermarkIssue = checkDiskLowWatermark( filteredNodeSettings, ... ); ... } } After: class TransportNodeDeprecationCheckAction { NodeResponse nodeOperation(...) { // 'filteredNodeSettings' is calculated but no longer used for checks Settings filteredNodeSettings = settings.filter(...); // All checks now incorrectly use the original, unfiltered settings for (NodeDeprecationCheck c : nodeSettingsChecks) { DeprecationIssue issue = c.apply(settings, ...); // BUG: should use filtered settings ... } // Watermark check correctly uses unfiltered settings DeprecationIssue watermarkIssue = checkDiskLowWatermark( settings, ... ); ... } } Suggestion importance[1-10]: 9 __ Why: This suggestion correctly identifies a critical regression where the skip_deprecations setting is ignored for all node-level checks, not just the disk watermark check, due to an overcorrection in the fix.	High
Possible issue	Apply deprecation skipping after checks Use the unfiltered clusterService.state() instead of filteredClusterState for deprecation checks to ensure all cluster-level deprecations are detected. Filtering of skipped deprecations should be performed on the resulting list of issues. x-pack/plugin/deprecation/src/main/java/org/elasticsearch/xpack/deprecation/TransportNodeDeprecationCheckAction.java [124-144] -Metadata metadata = clusterService.state().metadata(); -Settings transientSettings = metadata.transientSettings() - .filter(setting -> Regex.simpleMatch(skipTheseDeprecations, setting) == false); -Settings persistentSettings = metadata.persistentSettings() - .filter(setting -> Regex.simpleMatch(skipTheseDeprecations, setting) == false); -ClusterState filteredClusterState = ClusterState.builder(clusterService.state()) - .metadata(Metadata.builder(metadata).transientSettings(transientSettings).persistentSettings(persistentSettings).build()) - .build(); - List<DeprecationIssue> issues = new ArrayList<>(); for (NodeDeprecationChecks.NodeDeprecationCheck< Settings, PluginsAndModules, ClusterState, XPackLicenseState, DeprecationIssue> c : nodeSettingsChecks) { - DeprecationIssue deprecationIssue = c.apply(settings, pluginsService.info(), filteredClusterState, licenseState); + DeprecationIssue deprecationIssue = c.apply(settings, pluginsService.info(), clusterService.state(), licenseState); if (deprecationIssue != null) { issues.add(deprecationIssue); } } +issues.removeIf(issue -> Regex.simpleMatch(skipTheseDeprecations, issue.getDetails())); [To ensure code accuracy, apply this suggestion manually] Suggestion importance[1-10]: 8 __ Why: The suggestion correctly identifies that while the PR fixes the use of filtered node settings, it overlooks that the filteredClusterState is still used, which can cause some cluster-level deprecations to be missed. This is a valid and important bug fix.	Medium
General	Consolidate and elevate failure logging Consolidate redundant failure logging by removing the summary WARN log and the loop of INFO logs. Instead, log each failure once at the WARN level with its stack trace. x-pack/plugin/deprecation/src/main/java/org/elasticsearch/xpack/deprecation/NodeDeprecationChecker.java [48-56] if (response.hasFailures()) { - List<String> failedNodeIds = response.failures() - .stream() - .map(failure -> failure.nodeId() + ": " + failure.getMessage()) - .collect(Collectors.toList()); - logger.warn("nodes failed to run deprecation checks: {}", failedNodeIds); for (FailedNodeException failure : response.failures()) { - logger.atInfo().withThrowable(failure).log("node {} failed to run deprecation checks", failure.nodeId()); + logger.atWarn() + .withThrowable(failure) + .log("node {} failed to run deprecation checks", failure.nodeId()); } } [To ensure code accuracy, apply this suggestion manually] Suggestion importance[1-10]: 6 __ Why: The suggestion correctly points out redundant logging and proposes a cleaner approach by consolidating logs at the WARN level. This improves log clarity and reduces noise, which is a good practice for maintainability.	Low
More