Skip to content

native memory based admission control - #21191

Merged
Bukhtawar merged 14 commits into
opensearch-project:mainfrom
pradeep-L:nativeMemoryController
May 17, 2026
Merged

native memory based admission control#21191
Bukhtawar merged 14 commits into
opensearch-project:mainfrom
pradeep-L:nativeMemoryController

Conversation

@pradeep-L

@pradeep-L pradeep-L commented Apr 9, 2026

Copy link
Copy Markdown
Contributor

Description

This change introduces a Native Memory Based Admission Controller that throttles incoming transport requests (search, indexing, cluster admin) based on actual physical memory utilization on the node. The controller reads native memory usage from OsProbe. which parses MemAvailable from /proc/meminfo on Linux systems. The NativeMemoryBasedAdmissionController evaluates this usage against configurable per-action-type thresholds and rejects the requests with HTTP 429 . This gives the operators a more accurate signal for memory pressure since it accounts for OS-level page cache and slab memory rather than relying solely on JVM heap usage.

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

github-actions Bot commented Apr 9, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

PR Reviewer Guide 🔍

(Review updated until commit 50e9d44)

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

Possible Issue

The production constructor passes resourceTrackerSettings to computeEffectiveNativeMemory, but that method reads volatile fields from the settings holder. If the holder's fields are updated by a cluster-settings consumer on another thread between the time getAsLong() is called and the time the tracker uses the returned value, the effective native memory can change mid-computation. This can cause the percentage to be computed against a stale or inconsistent denominator, leading to incorrect admission control decisions when the limit or buffer is updated dynamically.

 
public AverageNativeMemoryUsageTracker(
    ThreadPool threadPool,
    TimeValue pollingInterval,
    TimeValue windowDuration,
    ResourceTrackerSettings resourceTrackerSettings
) {
    super(threadPool, pollingInterval, windowDuration);
    this.rssAnonSupplier = () -> OsProbe.getInstance().getProcessRssAnon();
    this.heapCommittedSupplier = () -> ManagementFactory.getMemoryMXBean().getHeapMemoryUsage().getCommitted();
    this.effectiveNativeMemorySupplier = () -> computeEffectiveNativeMemory(resourceTrackerSettings);
}
Possible Issue

When effectiveNativeMemory is zero or negative, getUsage() returns 0L and logs a warning. However, the warning message hardcodes effectiveNativeMemory=0 in the log string, even though the actual value might be negative. This can mislead operators debugging why the tracker reports zero usage when the limit is misconfigured or the buffer fully consumes the limit.

 
if (effectiveNativeMemory <= 0L) {
    LOGGER.warn(
        "Native memory poll: rssAnon={} heapCommitted={} nativeUsed={} effectiveNativeMemory=0 -> 0%",
        rssAnon,
        heapCommitted,
        nativeUsed
    );
    return 0L;
}
Possible Issue

getProcessRssAnon() catches IOException and logs it at warn level, then returns -1L. However, readRssAnonFromProcSelfStatus() can also throw IOException when the file cannot be read. If readProcSelfStatus() throws IOException, the catch block in getProcessRssAnon() logs the exception, but the method comment states that failure paths are logged at debug level. This inconsistency can cause unexpected warn-level logs on every polling cycle if /proc/self/status is temporarily unavailable, flooding the logs.

 
public long getProcessRssAnon() {
    if (Constants.LINUX == false) {
        return -1L;
    }
    try {
        return readRssAnonFromProcSelfStatus();
    } catch (IOException e) {
        logger.warn("failed to read /proc/self/status", e);
        return -1L;
    }
}
Serialization Issue

The NodeResourceUsageStats constructor and serialization logic add nativeMemoryUtilizationPercent only when the stream version is on or after V_3_7_0. However, if a mixed-version cluster has nodes running versions before V_3_7_0, those nodes will not send or expect this field. When a newer node receives stats from an older node, nativeMemoryUtilizationPercent defaults to 0.0, which can cause the admission controller to incorrectly report zero native memory usage for older nodes, leading to incorrect admission control decisions in a mixed-version cluster.

 
    if (in.getVersion().onOrAfter(Version.V_3_7_0)) {
        this.nativeMemoryUtilizationPercent = in.readDouble();
    } else {
        this.nativeMemoryUtilizationPercent = 0.0;
    }
}
Possible Issue

isLimitsBreached() checks if clusterService.state() and clusterService.state().nodes() are non-null, but does not check if getLocalNodeId() returns null. If the local node ID is null (e.g., during cluster initialization or when the node is not yet part of the cluster), getNodeStatistics(null) will be called, which can return an empty Optional or throw an exception, causing the admission controller to silently fail to enforce limits during startup or cluster state transitions.

 
private boolean isLimitsBreached(String actionName, AdmissionControlActionType admissionControlActionType) {
    // check if cluster state is ready
    if (clusterService.state() != null && clusterService.state().nodes() != null) {
        long maxMemoryLimit = this.getMemoryRejectionThreshold(admissionControlActionType);
        Optional<NodeResourceUsageStats> nodePerformanceStatistics = this.resourceUsageCollectorService.getNodeStatistics(
            this.clusterService.state().nodes().getLocalNodeId()
        );
        if (nodePerformanceStatistics.isPresent()) {
            double memoryUsage = nodePerformanceStatistics.get().getNativeMemoryUtilizationPercent();
            if (memoryUsage >= maxMemoryLimit) {
                LOGGER.warn(
                    "NativeMemoryBasedAdmissionController limit reached as the current native memory "
                        + "usage [{}] exceeds the allowed limit [{}] for transport action [{}] in admissionControlMode [{}]",
                    memoryUsage,
                    maxMemoryLimit,
                    actionName,
                    this.settings.getTransportLayerAdmissionControllerMode()
                );
                return true;
            }
        }
    }
    return false;
}

@github-actions

github-actions Bot commented Apr 9, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

PR Code Suggestions ✨

Latest suggestions up to 50e9d44

Explore these optional code suggestions:

CategorySuggestion                                                                                                                                    Impact
General
Reduce log level for unconfigured limits

When effectiveNativeMemory is zero or negative, the method logs a warning and
returns 0L, which could mask configuration errors. Consider using LOGGER.debug
instead of LOGGER.warn to avoid excessive logging when the native memory limit is
intentionally unconfigured, or add a flag to distinguish between "unconfigured" and
"misconfigured" states.

server/src/main/java/org/opensearch/node/resource/tracker/AverageNativeMemoryUsageTracker.java [100-109]

 long effectiveNativeMemory = effectiveNativeMemorySupplier.getAsLong();
 if (effectiveNativeMemory <= 0L) {
-    LOGGER.warn(
+    LOGGER.debug(
         "Native memory poll: rssAnon={} heapCommitted={} nativeUsed={} effectiveNativeMemory=0 -> 0%",
         rssAnon,
         heapCommitted,
         nativeUsed
     );
     return 0L;
 }
Suggestion importance[1-10]: 6

__

Why: This suggestion has merit as logging at warn level for an intentionally unconfigured setting could create unnecessary noise. Using debug level when effectiveNativeMemory is zero would be more appropriate for this scenario, though the current behavior is not incorrect.

Low
Reduce log level for I/O failures

The method logs a warning when /proc/self/status cannot be read, but this may
produce excessive log noise during normal operation if the file is temporarily
unavailable. Consider using logger.debug instead of logger.warn for the IOException
case, or add rate-limiting to prevent log flooding.

server/src/main/java/org/opensearch/monitor/os/OsProbe.java [248-258]

 public long getProcessRssAnon() {
     if (Constants.LINUX == false) {
         return -1L;
     }
     try {
         return readRssAnonFromProcSelfStatus();
     } catch (IOException e) {
-        logger.warn("failed to read /proc/self/status", e);
+        logger.debug("failed to read /proc/self/status", e);
         return -1L;
     }
 }
Suggestion importance[1-10]: 5

__

Why: The suggestion to use logger.debug instead of logger.warn is reasonable to reduce log noise, but the current warn level is appropriate for production environments where file read failures should be visible. The impact is moderate as it's a logging preference rather than a functional issue.

Low
Prevent potential overflow in multiplication

The multiplication kb * 1024L can overflow if kb is very large (e.g., close to
Long.MAX_VALUE / 1024). Although unlikely in practice, verify that kb is within a
safe range before multiplying, or use Math.multiplyExact to detect overflow and
handle it gracefully.

server/src/main/java/org/opensearch/monitor/os/OsProbe.java [279-295]

-long readRssAnonFromProcSelfStatus() throws IOException {
-    for (final String line : readProcSelfStatus()) {
-        if (line.startsWith("RssAnon:")) {
-            final String[] parts = line.split("\\s+");
-            if (parts.length >= 2) {
-                try {
-                    final long kb = Long.parseLong(parts[1]);
-                    if (kb < 0L) {
-                        return -1L;
-                    }
-                    return kb * 1024L;
-                } catch (NumberFormatException nfe) {
-                    logger.warn("malformed RssAnon value in /proc/self/status", nfe);
-                    return -1L;
-                }
-            }
-            ...
-        }
+try {
+    final long kb = Long.parseLong(parts[1]);
+    if (kb < 0L) {
+        return -1L;
     }
-    ...
+    return Math.multiplyExact(kb, 1024L);
+} catch (NumberFormatException nfe) {
+    logger.warn("malformed RssAnon value in /proc/self/status", nfe);
+    return -1L;
+} catch (ArithmeticException ae) {
+    logger.warn("RssAnon value overflow in /proc/self/status", ae);
+    return -1L;
 }
Suggestion importance[1-10]: 4

__

Why: While overflow protection is generally good practice, the likelihood of /proc/self/status reporting a value large enough to cause overflow when multiplied by 1024 is extremely low in real-world scenarios. The suggestion adds defensive code but addresses a highly improbable edge case. The added complexity may not justify the minimal risk mitigation.

Low
Return sentinel value for unavailable data

Returning 0L when rssAnon is unavailable could be misleading, as it suggests zero
usage rather than an error state. Consider returning a sentinel value (e.g., -1L) or
throwing an exception to clearly indicate the measurement failed, allowing callers
to handle the error appropriately.

server/src/main/java/org/opensearch/node/resource/tracker/AverageNativeMemoryUsageTracker.java [91-95]

 long rssAnon = rssAnonSupplier.getAsLong();
 if (rssAnon < 0L) {
-    LOGGER.warn("Native memory poll skipped: RssAnon unavailable from /proc/self/status");
-    return 0L;
+    LOGGER.debug("Native memory poll skipped: RssAnon unavailable from /proc/self/status");
+    return -1L;
 }
Suggestion importance[1-10]: 3

__

Why: The suggestion to return -1L instead of 0L is not necessarily better. The method getUsage() is designed to return a percentage (0-100), and returning 0L when data is unavailable is a reasonable design choice that allows the system to continue operating. The existing code already logs a warning, making the state clear.

Low

Previous suggestions

Suggestions up to commit f740729
CategorySuggestion                                                                                                                                    Impact
General
Align log level with documentation

The method logs at warn level when /proc/self/status cannot be read, but the Javadoc
states "failure paths are logged at debug level." This inconsistency can cause
excessive logging noise. Change the log level to debug to match the documented
behavior and avoid polluting logs during normal operation when the file is
temporarily unavailable.

server/src/main/java/org/opensearch/monitor/os/OsProbe.java [248-258]

 public long getProcessRssAnon() {
     if (Constants.LINUX == false) {
         return -1L;
     }
     try {
         return readRssAnonFromProcSelfStatus();
     } catch (IOException e) {
-        logger.warn("failed to read /proc/self/status", e);
+        logger.debug("failed to read /proc/self/status", e);
         return -1L;
     }
 }
Suggestion importance[1-10]: 7

__

Why: The Javadoc at line 244 states "failure paths are logged at debug level" but the code at line 255 logs at warn level. This inconsistency should be corrected to match the documented behavior and avoid excessive logging noise.

Medium
Possible issue
Prevent potential arithmetic overflow

The buffer calculation limit * bufferPercent / 100L can overflow when limit is very
large (e.g., near Long.MAX_VALUE). This could produce incorrect effective memory
values. Use Math.multiplyExact or perform the division before multiplication to
prevent silent overflow and ensure correct budget computation.

server/src/main/java/org/opensearch/node/resource/tracker/AverageNativeMemoryUsageTracker.java [140-149]

 long computeEffectiveNativeMemory(ResourceTrackerSettings resourceTrackerSettings) {
     long limit = resourceTrackerSettings.getNativeMemoryLimitBytes();
     if (limit <= 0L) {
         return 0L;
     }
     int bufferPercent = resourceTrackerSettings.getNativeMemoryBufferPercent();
-    long buffer = limit * bufferPercent / 100L;
+    long buffer = (limit / 100L) * bufferPercent;
     long effective = Math.max(0L, limit - buffer);
     return effective;
 }
Suggestion importance[1-10]: 6

__

Why: The calculation limit * bufferPercent / 100L at line 146 could overflow for very large limit values near Long.MAX_VALUE. Reordering to (limit / 100L) * bufferPercent prevents this issue and ensures correct budget computation.

Low
Prevent overflow in byte conversion

The multiplication kb * 1024L can overflow when kb is extremely large (e.g., near
Long.MAX_VALUE / 1024). This could return an incorrect negative value or wrap
around. Use Math.multiplyExact(kb, 1024L) and catch ArithmeticException to detect
overflow and return -1L safely.

server/src/main/java/org/opensearch/monitor/os/OsProbe.java [279-290]

 long readRssAnonFromProcSelfStatus() throws IOException {
     for (final String line : readProcSelfStatus()) {
         if (line.startsWith("RssAnon:")) {
             // Format: "RssAnon:\t 12345 kB"
             final String[] parts = line.split("\\s+");
             if (parts.length >= 2) {
                 try {
                     final long kb = Long.parseLong(parts[1]);
                     if (kb < 0L) {
                         return -1L;
                     }
-                    return kb * 1024L;
-                } catch (NumberFormatException nfe) {
-                    logger.warn("malformed RssAnon value in /proc/self/status", nfe);
+                    return Math.multiplyExact(kb, 1024L);
+                } catch (NumberFormatException | ArithmeticException e) {
+                    logger.warn("malformed or overflow RssAnon value in /proc/self/status", e);
                     return -1L;
                 }
             }
             ...
         }
     }
     ...
 }
Suggestion importance[1-10]: 6

__

Why: The multiplication kb * 1024L at line 290 can overflow when kb is extremely large. Using Math.multiplyExact with proper exception handling would detect overflow and return -1L safely, preventing incorrect negative values.

Low
Suggestions up to commit 2d34120
CategorySuggestion                                                                                                                                    Impact
General
Prevent potential arithmetic overflow

The computation limit * bufferPercent / 100L can overflow when limit is very large
(e.g., near Long.MAX_VALUE). This could produce incorrect effective memory values.
Use Math.multiplyExact or perform the division before multiplication to prevent
silent overflow.

server/src/main/java/org/opensearch/node/resource/tracker/AverageNativeMemoryUsageTracker.java [140-149]

 long computeEffectiveNativeMemory(ResourceTrackerSettings resourceTrackerSettings) {
     long limit = resourceTrackerSettings.getNativeMemoryLimitBytes();
     if (limit <= 0L) {
         return 0L;
     }
     int bufferPercent = resourceTrackerSettings.getNativeMemoryBufferPercent();
-    long buffer = limit * bufferPercent / 100L;
+    long buffer = (limit / 100L) * bufferPercent;
     long effective = Math.max(0L, limit - buffer);
     return effective;
 }
Suggestion importance[1-10]: 6

__

Why: The computation limit * bufferPercent / 100L at line 146 could overflow for very large limit values near Long.MAX_VALUE. While unlikely in practice, reordering to (limit / 100L) * bufferPercent prevents this potential issue with minimal code change.

Low
Prevent arithmetic overflow in conversion

The multiplication kb * 1024L can overflow when kb is very large, producing
incorrect byte values. Validate that kb is within a safe range before multiplying,
or use Math.multiplyExact to detect overflow and handle it gracefully.

server/src/main/java/org/opensearch/monitor/os/OsProbe.java [279-302]

 long readRssAnonFromProcSelfStatus() throws IOException {
     for (final String line : readProcSelfStatus()) {
         if (line.startsWith("RssAnon:")) {
             final String[] parts = line.split("\\s+");
             if (parts.length >= 2) {
                 try {
                     final long kb = Long.parseLong(parts[1]);
                     if (kb < 0L) {
                         return -1L;
                     }
-                    return kb * 1024L;
+                    return Math.multiplyExact(kb, 1024L);
                 } catch (NumberFormatException nfe) {
                     logger.warn("malformed RssAnon value in /proc/self/status", nfe);
+                    return -1L;
+                } catch (ArithmeticException ae) {
+                    logger.warn("RssAnon value overflow in /proc/self/status", ae);
                     return -1L;
                 }
             }
             logger.warn("RssAnon line has unexpected shape: [{}]", line);
             return -1L;
         }
     }
     logger.warn("RssAnon line not found in /proc/self/status");
     return -1L;
 }
Suggestion importance[1-10]: 6

__

Why: The multiplication kb * 1024L at line 290 could overflow for extremely large kb values from /proc/self/status. Using Math.multiplyExact would detect overflow and allow graceful handling, though such large values are unlikely in practice.

Low
Fix log level inconsistency

The method logs at warn level when /proc/self/status cannot be read, but the Javadoc
states "failure paths are logged at debug level." This inconsistency may cause
excessive logging in production. Change the log level to debug to match the
documented behavior and avoid polluting logs with expected failures.

server/src/main/java/org/opensearch/monitor/os/OsProbe.java [248-258]

 public long getProcessRssAnon() {
     if (Constants.LINUX == false) {
         return -1L;
     }
     try {
         return readRssAnonFromProcSelfStatus();
     } catch (IOException e) {
-        logger.warn("failed to read /proc/self/status", e);
+        logger.debug("failed to read /proc/self/status", e);
         return -1L;
     }
 }
Suggestion importance[1-10]: 5

__

Why: The Javadoc at line 243 states "failure paths are logged at debug level" but the implementation at line 255 logs at warn level. This inconsistency should be fixed, though the impact is moderate since it only affects log verbosity.

Low
Reduce log noise for expected failures

Logging at warn level on every polling cycle when RssAnon is unavailable (e.g., on
non-Linux or when file read fails) can flood logs. Since this is an expected
condition, reduce the log level to debug to avoid excessive warnings in production.

server/src/main/java/org/opensearch/node/resource/tracker/AverageNativeMemoryUsageTracker.java [88-95]

 public long getUsage() {
     long rssAnon = rssAnonSupplier.getAsLong();
     if (rssAnon < 0L) {
-        LOGGER.warn("Native memory poll skipped: RssAnon unavailable from /proc/self/status");
+        LOGGER.debug("Native memory poll skipped: RssAnon unavailable from /proc/self/status");
         return 0L;
     }
     ...
 }
Suggestion importance[1-10]: 5

__

Why: Logging at warn level on every polling cycle when RssAnon is unavailable (line 93) can flood logs in non-Linux environments or when file reads fail. Reducing to debug level would be more appropriate for this expected condition.

Low
Suggestions up to commit b89cd50
CategorySuggestion                                                                                                                                    Impact
General
Prevent potential arithmetic overflow

The computation limit * bufferPercent / 100L can overflow when limit is large (e.g.,
near Long.MAX_VALUE). This could produce incorrect effective memory values or
negative results. Use Math.multiplyExact or check for overflow before multiplication
to prevent silent data corruption.

server/src/main/java/org/opensearch/node/resource/tracker/AverageNativeMemoryUsageTracker.java [140-149]

 long computeEffectiveNativeMemory(ResourceTrackerSettings resourceTrackerSettings) {
     long limit = resourceTrackerSettings.getNativeMemoryLimitBytes();
     if (limit <= 0L) {
         return 0L;
     }
     int bufferPercent = resourceTrackerSettings.getNativeMemoryBufferPercent();
-    long buffer = limit * bufferPercent / 100L;
+    long buffer = (long) ((double) limit * bufferPercent / 100.0);
     long effective = Math.max(0L, limit - buffer);
     return effective;
 }
Suggestion importance[1-10]: 7

__

Why: The computation limit * bufferPercent / 100L at line 146 can overflow when limit is near Long.MAX_VALUE, potentially producing incorrect results. Using floating-point arithmetic prevents this overflow issue.

Medium
Fix log level mismatch

The method logs at warn level when /proc/self/status cannot be read, but the Javadoc
states "failure paths are logged at debug level." This inconsistency could lead to
excessive log noise in production. Change the log level to debug to match the
documented behavior.

server/src/main/java/org/opensearch/monitor/os/OsProbe.java [248-258]

 public long getProcessRssAnon() {
     if (Constants.LINUX == false) {
         return -1L;
     }
     try {
         return readRssAnonFromProcSelfStatus();
     } catch (IOException e) {
-        logger.warn("failed to read /proc/self/status", e);
+        logger.debug("failed to read /proc/self/status", e);
         return -1L;
     }
 }
Suggestion importance[1-10]: 6

__

Why: The Javadoc at line 244 states "failure paths are logged at debug level" but the code at line 255 logs at warn level. This inconsistency should be corrected to match the documented behavior and avoid excessive log noise.

Low
Reduce log verbosity for expected failures

Logging at warn level on every polling cycle when RssAnon is unavailable (e.g., on
non-Linux or when file read fails) can flood logs. Since this is expected behavior
on non-Linux platforms and the tracker is already gated to Linux in
NodeResourceUsageTracker, downgrade to debug level to reduce noise.

server/src/main/java/org/opensearch/node/resource/tracker/AverageNativeMemoryUsageTracker.java [89-95]

 public long getUsage() {
     long rssAnon = rssAnonSupplier.getAsLong();
     if (rssAnon < 0L) {
-        LOGGER.warn("Native memory poll skipped: RssAnon unavailable from /proc/self/status");
+        LOGGER.debug("Native memory poll skipped: RssAnon unavailable from /proc/self/status");
         return 0L;
     }
     ...
 }
Suggestion importance[1-10]: 5

__

Why: Logging at warn level on every polling cycle when RssAnon is unavailable can flood logs. Since this is expected behavior on non-Linux platforms, downgrading to debug level would reduce log noise while still providing diagnostic information when needed.

Low
Suggestions up to commit 1b68af3
CategorySuggestion                                                                                                                                    Impact
Possible issue
Prevent overflow in memory calculation

The multiplication kb * 1024L can overflow when kb is very large (e.g., near
Long.MAX_VALUE / 1024). Validate kb is within a safe range before multiplying to
prevent silent overflow and incorrect memory calculations.

server/src/main/java/org/opensearch/monitor/os/OsProbe.java [270-296]

 long readRssAnonFromProcSelfStatus() throws IOException {
     try (BufferedReader reader = Files.newBufferedReader(PathUtils.get("/proc/self/status"))) {
         String line;
         while ((line = reader.readLine()) != null) {
             if (line.startsWith("RssAnon:")) {
                 // Format: "RssAnon:\t 12345 kB"
                 String[] parts = line.split("\\s+");
                 if (parts.length >= 2) {
                     try {
                         long kb = Long.parseLong(parts[1]);
-                        if (kb < 0L) {
+                        if (kb < 0L || kb > Long.MAX_VALUE / 1024L) {
                             return -1L;
                         }
                         return kb * 1024L;
                     } catch (NumberFormatException nfe) {
                         logger.warn("malformed RssAnon value in /proc/self/status", nfe);
                         return -1L;
                     }
                 }
                 logger.warn("RssAnon line has unexpected shape: [{}]", line);
                 return -1L;
             }
         }
         logger.warn("RssAnon line not found in /proc/self/status");
         return -1L;
     }
 }
Suggestion importance[1-10]: 7

__

Why: The suggestion identifies a valid potential overflow issue when multiplying kb * 1024L at line 283. While the overflow scenario is unlikely in practice (would require RssAnon > 8 petabytes), adding the overflow check kb > Long.MAX_VALUE / 1024L would prevent silent overflow and incorrect memory calculations, improving robustness.

Medium
General
Reduce log noise on repeated failures

The warning log in the catch block will fire on every poll when /proc/self/status is
unavailable, potentially flooding logs. Consider using logger.debug instead of
logger.warn to reduce noise, or add a flag to log the warning only once.

server/src/main/java/org/opensearch/monitor/os/OsProbe.java [249-259]

 public long getProcessRssAnon() {
     if (Constants.LINUX == false) {
         return -1L;
     }
     try {
         return readRssAnonFromProcSelfStatus();
     } catch (IOException e) {
-        logger.warn("failed to read /proc/self/status", e);
+        logger.debug("failed to read /proc/self/status", e);
         return -1L;
     }
 }
Suggestion importance[1-10]: 6

__

Why: The suggestion correctly identifies that logger.warn in the catch block will fire on every poll when /proc/self/status is unavailable, potentially flooding logs. However, the method already has a debug log at line 256 for the same scenario, and the warning is appropriate for an IOException which indicates a real problem. The suggestion to use logger.debug would reduce visibility of actual I/O errors.

Low
Reduce repeated warning log noise

The warning log fires on every poll when RssAnon is unavailable, which can flood
logs if the file is persistently inaccessible. Use logger.debug or implement a flag
to log the warning only once to avoid excessive log entries.

server/src/main/java/org/opensearch/node/resource/tracker/AverageNativeMemoryUsageTracker.java [89-95]

 @Override
 public long getUsage() {
 
     long rssAnon = rssAnonSupplier.getAsLong();
     if (rssAnon < 0L) {
-        LOGGER.warn("Native memory poll skipped: RssAnon unavailable from /proc/self/status");
+        LOGGER.debug("Native memory poll skipped: RssAnon unavailable from /proc/self/status");
         return 0L;
     }
     ...
Suggestion importance[1-10]: 6

__

Why: The suggestion correctly identifies that the warning log at line 93 fires on every poll when RssAnon is unavailable, which can flood logs. However, this warning is intentional to alert operators that native memory tracking is not working. The suggestion to use logger.debug would reduce visibility of this important operational issue.

Low
Handle buffer consuming entire limit explicitly

When bufferPercent is 100, the expression limit * bufferPercent / 100L equals limit,
causing effective to become zero. This edge case should be validated or documented
to prevent silent misconfiguration where the entire budget is consumed by the
buffer.

server/src/main/java/org/opensearch/node/resource/tracker/AverageNativeMemoryUsageTracker.java [140-149]

 long computeEffectiveNativeMemory(ResourceTrackerSettings resourceTrackerSettings) {
     long limit = resourceTrackerSettings.getNativeMemoryLimitBytes();
     if (limit <= 0L) {
         return 0L;
     }
     int bufferPercent = resourceTrackerSettings.getNativeMemoryBufferPercent();
+    if (bufferPercent >= 100) {
+        return 0L;
+    }
     long buffer = limit * bufferPercent / 100L;
     long effective = Math.max(0L, limit - buffer);
     return effective;
 }
Suggestion importance[1-10]: 5

__

Why: The suggestion identifies a valid edge case where bufferPercent equals 100, but the existing code already handles this correctly via Math.max(0L, limit - buffer) at line 147. The explicit check for bufferPercent >= 100 would make the intent clearer but doesn't fix a bug since the current implementation already returns 0L in this case.

Low
Suggestions up to commit a12c25b
CategorySuggestion                                                                                                                                    Impact
Possible issue
Fix array index parsing issue

The parsing logic assumes parts[1] is the numeric value, but the comment shows the
format includes a tab character before the value. The split("\s+") will create an
empty string at parts[0] if the line starts with whitespace. Verify the array index
is correct or adjust the split pattern to handle leading whitespace properly.

server/src/main/java/org/opensearch/monitor/os/OsProbe.java [270-295]

 long readRssAnonFromProcSelfStatus() throws IOException {
     try (BufferedReader reader = Files.newBufferedReader(PathUtils.get("/proc/self/status"))) {
         String line;
         while ((line = reader.readLine()) != null) {
             if (line.startsWith("RssAnon:")) {
                 // Format: "RssAnon:\t 12345 kB"
                 String[] parts = line.split("\\s+");
-                if (parts.length >= 2) {
-                    try {
-                        long kb = Long.parseLong(parts[1]);
-                        if (kb < 0L) {
+                // Skip empty parts and find the numeric value
+                for (int i = 1; i < parts.length; i++) {
+                    if (!parts[i].isEmpty()) {
+                        try {
+                            long kb = Long.parseLong(parts[i]);
+                            if (kb < 0L) {
+                                return -1L;
+                            }
+                            return kb * 1024L;
+                        } catch (NumberFormatException nfe) {
+                            logger.warn("malformed RssAnon value in /proc/self/status", nfe);
                             return -1L;
                         }
-                        return kb * 1024L;
-                    } catch (NumberFormatException nfe) {
-                        logger.warn("malformed RssAnon value in /proc/self/status", nfe);
-                        return -1L;
                     }
                 }
                 logger.warn("RssAnon line has unexpected shape: [{}]", line);
                 return -1L;
             }
         }
         logger.warn("RssAnon line not found in /proc/self/status");
         return -1L;
     }
 }
Suggestion importance[1-10]: 7

__

Why: The suggestion correctly identifies a potential parsing issue where split("\\s+") on a line starting with "RssAnon:\t" could produce an empty string at parts[0]. However, the existing code already checks parts.length >= 2 and accesses parts[1], which should work correctly if the split produces at least two elements. The improved code is more robust by iterating through parts to find the first non-empty numeric value, making it a worthwhile improvement for edge cases.

Medium
Prevent arithmetic overflow in calculation

The multiplication limit * bufferPercent can overflow when limit is very large
(e.g., near Long.MAX_VALUE). Use checked arithmetic or cast to BigInteger for the
intermediate calculation to prevent silent overflow that would produce incorrect
effective memory values.

server/src/main/java/org/opensearch/node/resource/tracker/AverageNativeMemoryUsageTracker.java [140-149]

 long computeEffectiveNativeMemory(ResourceTrackerSettings resourceTrackerSettings) {
     long limit = resourceTrackerSettings.getNativeMemoryLimitBytes();
     if (limit <= 0L) {
         return 0L;
     }
     int bufferPercent = resourceTrackerSettings.getNativeMemoryBufferPercent();
-    long buffer = limit * bufferPercent / 100L;
+    long buffer = Math.multiplyExact(limit, bufferPercent) / 100L;
     long effective = Math.max(0L, limit - buffer);
     return effective;
 }
Suggestion importance[1-10]: 6

__

Why: The suggestion identifies a valid concern about potential overflow when multiplying limit * bufferPercent. Using Math.multiplyExact() would throw an exception on overflow rather than silently producing incorrect results. However, in practice, bufferPercent is constrained to 0-100 (as seen in the setting definition), making overflow unlikely unless limit exceeds Long.MAX_VALUE / 100. Still, defensive programming with checked arithmetic is a reasonable improvement.

Low

@github-actions

github-actions Bot commented Apr 9, 2026

Copy link
Copy Markdown
Contributor

❌ Gradle check result for 17de515: 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?

himshikhagupta
himshikhagupta reviewed Apr 13, 2026
View reviewed changes
Comment thread server/src/main/java/org/opensearch/monitor/os/OsProbe.java Outdated
Comment thread server/src/main/java/org/opensearch/monitor/os/OsProbe.java
Comment thread server/src/main/java/org/opensearch/node/resource/tracker/NodeResourceUsageTracker.java Outdated
Comment thread server/src/main/java/org/opensearch/node/NodeResourceUsageStats.java Outdated
Comment thread ...nsearch/ratelimitting/admissioncontrol/controllers/NativeMemoryBasedAdmissionController.java
@pradeep-L
pradeep-L marked this pull request as ready for review April 16, 2026 05:17
@pradeep-L
pradeep-L requested a review from a team as a code owner April 16, 2026 05:17
@github-actions

github-actions Bot commented Apr 16, 2026

Copy link
Copy Markdown
Contributor

Persistent review updated to latest commit 1e8ec5a

@github-actions

github-actions Bot commented Apr 16, 2026

Copy link
Copy Markdown
Contributor

Persistent review updated to latest commit 99e846e

@github-actions

github-actions Bot commented Apr 16, 2026

Copy link
Copy Markdown
Contributor

❌ Gradle check result for 99e846e: 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?

himshikhagupta
himshikhagupta reviewed Apr 16, 2026
View reviewed changes
Comment thread server/src/main/java/org/opensearch/monitor/os/OsProbe.java Outdated
himshikhagupta
himshikhagupta reviewed Apr 16, 2026
View reviewed changes
Comment thread server/src/main/java/org/opensearch/monitor/os/OsProbe.java Outdated
himshikhagupta
himshikhagupta reviewed Apr 16, 2026
View reviewed changes
Comment thread server/src/main/java/org/opensearch/monitor/os/OsProbe.java
himshikhagupta
himshikhagupta reviewed Apr 16, 2026
View reviewed changes
Comment thread server/src/main/java/org/opensearch/node/NodeResourceUsageStats.java Outdated
himshikhagupta
himshikhagupta reviewed Apr 16, 2026
View reviewed changes
Comment thread server/src/main/java/org/opensearch/node/NodeResourceUsageStats.java Outdated
@github-actions

github-actions Bot commented Apr 20, 2026

Copy link
Copy Markdown
Contributor

Persistent review updated to latest commit cf12505

@github-actions

github-actions Bot commented Apr 20, 2026

Copy link
Copy Markdown
Contributor

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

@github-actions

github-actions Bot commented Apr 20, 2026

Copy link
Copy Markdown
Contributor

Persistent review updated to latest commit 76c4daf

@github-actions

github-actions Bot commented Apr 20, 2026

Copy link
Copy Markdown
Contributor

❌ Gradle check result for 76c4daf: 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?

himshikhagupta
himshikhagupta reviewed Apr 21, 2026
View reviewed changes
Comment thread server/src/test/java/org/opensearch/monitor/os/OsProbeTests.java Outdated
himshikhagupta
himshikhagupta reviewed Apr 21, 2026
View reviewed changes
Comment thread server/src/main/java/org/opensearch/node/NodeResourceUsageStats.java
@pradeep-L
pradeep-L force-pushed the nativeMemoryController branch from 76c4daf to 091d5dc Compare April 21, 2026 13:32
@pradeep-L
pradeep-L requested a review from jed326 as a code owner April 21, 2026 13:32
@github-actions

github-actions Bot commented May 16, 2026

Copy link
Copy Markdown
Contributor

Persistent review updated to latest commit 2d34120

@github-actions

github-actions Bot commented May 16, 2026

Copy link
Copy Markdown
Contributor

✅ Gradle check result for 2d34120: SUCCESS

Pradeep L added 13 commits May 17, 2026 09:47
Pradeep L @pradeep-L
native memory based admission control
d451514 
Signed-off-by: Pradeep L <spradeel@amazon.com>
Pradeep L @pradeep-L
added version check for native memory controller
1c3c377 
Signed-off-by: Pradeep L <spradeel@amazon.com>
Pradeep L @pradeep-L
Fix native memory admission controller for cross-platform compatibility
130da3a 
Signed-off-by: Pradeep L <spradeel@amazon.com>
Pradeep L @pradeep-L
adding read permission for /proc/meminfo
944ddf4 
Signed-off-by: Pradeep L <spradeel@amazon.com>
Pradeep L @pradeep-L
fix: review comment
6cd1d0c 
Signed-off-by: Pradeep L <spradeel@amazon.com>
Pradeep L @pradeep-L
fixed failing integ tests
c9ea9c2 
Signed-off-by: Pradeep L <spradeel@amazon.com>
Pradeep L @pradeep-L
changing source from meminfo to status
e5cfc3d 
Signed-off-by: Pradeep L <spradeel@amazon.com>
Pradeep L @pradeep-L
admission controll setting validation
0f10462 
Signed-off-by: Pradeep L <spradeel@amazon.com>
Pradeep L @pradeep-L
Add node-level native memory tracker budget
fd1664a 
Replace the datafusion/parquet-derived cap in
AverageNativeMemoryUsageTracker with two explicit node-scope dynamic
settings:

  node.native_memory.limit         (ByteSizeValue, default 0b)
  node.native_memory.buffer_percent (int 0-99, default 0)

The tracker now divides observed native-memory use
(max(0, RssAnon - HeapCommitted)) by
limit - (limit * bufferPercent / 100). When the limit is unset or the
buffer fully consumes it, getUsage() returns 0 without dividing.

Also: allow read access to /proc/self/status in the Java security
policy and systemd unit so OsProbe.getProcessRssAnon() works in
hardened installs.

Signed-off-by: Pradeep L <spradeel@amazon.com>
Pradeep L @pradeep-L
settings node.native_memory.limit node.native_memory.buffer_percent d…
2402207 
…ynamic update fix

Signed-off-by: Pradeep L <spradeel@amazon.com>
Pradeep L @pradeep-L
fixed failing tests
ffeb659 
Signed-off-by: Pradeep L <spradeel@amazon.com>
Pradeep L @pradeep-L
fix: spotlesscheck
fe9aa6c 
Signed-off-by: Pradeep L <spradeel@amazon.com>
Pradeep L @pradeep-L
increasing test coverage
f740729 
Signed-off-by: Pradeep L <spradeel@amazon.com>
@pradeep-L
pradeep-L force-pushed the nativeMemoryController branch from 2d34120 to f740729 Compare May 17, 2026 04:17
@github-actions

github-actions Bot commented May 17, 2026

Copy link
Copy Markdown
Contributor

Persistent review updated to latest commit f740729

@github-actions

github-actions Bot commented May 17, 2026

Copy link
Copy Markdown
Contributor

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

@github-actions

github-actions Bot commented May 17, 2026

Copy link
Copy Markdown
Contributor

Persistent review updated to latest commit 50e9d44

@github-actions

github-actions Bot commented May 17, 2026

Copy link
Copy Markdown
Contributor

✅ Gradle check result for 50e9d44: SUCCESS

@Bukhtawar
Bukhtawar merged commit 75b6e82 into opensearch-project:main May 17, 2026
23 of 24 checks passed
@GeekGlider GeekGlider mentioned this pull request May 18, 2026
Merged
3 tasks
@gaurav-amz gaurav-amz mentioned this pull request May 18, 2026
Closed
5 tasks
gaurav-amz added a commit to gaurav-amz/OpenSearch that referenced this pull request May 18, 2026
@gaurav-amz
Add query/datafusion pools, AC-aware default, breaker, R1, spill compat
71b9e9f 
Builds on opensearch-project#21703 with the framework-side changes that were not part of
its initial scope. Consumer wiring for arrow-flight-rpc and
analytics-engine is handled by opensearch-project#21465 (arrow-base, already merged) and
not duplicated here.

Pools added:
  POOL_QUERY       analytics-engine query execution
  POOL_DATAFUSION  DataFusion native MemoryPool mirror

Both registered with min/max settings dynamically updatable. The flight
and ingest pools registered by opensearch-project#21703 are unchanged.

Cross-cutting changes:

* NativeAllocatorListener SPI in arrow-spi for pool-resize callbacks
  (allocator-agnostic, no Arrow types in the signature). DataFusion
  uses this to mirror datafusion-pool resize to the Rust MemoryPool
  via df_set_memory_pool_limit (R1).

* Derive native.allocator.root.limit from node.native_memory.limit
  * 0.8 when unset, leaving 20% headroom for non-Arrow native usage
  that admission control still needs to throttle on (R5). Reconciles
  with opensearch-project#21191's admission control.

* Implement CircuitBreakerPlugin: register a native_arrow breaker
  with Durability.PERMANENT so root usage is reflected in
  _nodes/stats?breaker and the parent breaker rolls up off-heap
  pressure. Allocator periodically syncs root.getAllocatedMemory
  into the breaker counter via the rebalance hook.

* Probe df_set_spill_limit at NativeBridge static init. When the
  symbol is present, datafusion.spill_memory_limit_bytes becomes
  Dynamic and a listener calls the FFM symbol on resize. When
  absent, the setting stays NodeScope-only and OpenSearch rejects
  runtime PUTs cleanly. Lights up automatically once the upstream
  datafusion crate carrying df_set_spill_limit is picked up here.

* Delete dead DataFusionService.rootAllocator field and
  newChildAllocator() method (no production callers).

* Call rebalance() once after pool creation in createComponents so
  pools reach their max capacity even when the rebalancer is
  disabled by default. Without this, pools sit at min=0 and
  silently fail allocations.

* Add ensureForTesting helper for unit tests that bring up
  consumers without the plugin lifecycle.

Tests cover listener fan-out, multi-listener invocation, exception
isolation, breaker sync via rebalance, AC-derived default resolution.

Signed-off-by: Gaurav Singh <gauravsg@amazon.com>
@opensearch-ci-bot opensearch-ci-bot mentioned this pull request May 27, 2026
Merged
This was referenced Jun 17, 2026
Open
Open
Open
KhishorekumarBS pushed a commit to KhishorekumarBS/OpenSearch that referenced this pull request Jul 3, 2026
@pradeep-L bkhishor
Implement Native memory based admission control (opensearch-project#2…
eb81e88 
…1191)

Implement Native memory based admission control

Signed-off-by: Pradeep L <spradeel@amazon.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@bharath-techie bharath-techie bharath-techie left review comments
@Bukhtawar Bukhtawar Bukhtawar approved these changes
@jed326 jed326 Awaiting requested review from jed326
@peternied peternied Awaiting requested review from peternied
@andrross andrross Awaiting requested review from andrross
@ashking94 ashking94 Awaiting requested review from ashking94
@cwperks cwperks Awaiting requested review from cwperks
@dbwiddis dbwiddis Awaiting requested review from dbwiddis
@gbbafna gbbafna Awaiting requested review from gbbafna
@kotwanikunal kotwanikunal Awaiting requested review from kotwanikunal
@mch2 mch2 Awaiting requested review from mch2
@msfroh msfroh Awaiting requested review from msfroh
@owaiskazi19 owaiskazi19 Awaiting requested review from owaiskazi19
@reta reta Awaiting requested review from reta
@sachinpkale sachinpkale Awaiting requested review from sachinpkale
@saratvemulapalli saratvemulapalli Awaiting requested review from saratvemulapalli
@shwetathareja shwetathareja Awaiting requested review from shwetathareja
@sohami sohami Awaiting requested review from sohami
+1 more reviewer
@himshikhagupta himshikhagupta himshikhagupta left review comments
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@pradeep-L @Bukhtawar @bharath-techie @himshikhagupta