MB-43559: Ensure eviction doesn't stop at high watermark if no replicas

In order to keep active data in memory preferentially,
PagingVisitor::visitBucket skipped evicting from active vbuckets if
both:

 1. active resident ratio < replica resident ratio
 2. current memory usage < high watermark

However, if there are _no_ replica vbuckets on the node

   store.getReplicaResidentRatio()

defaults to 100%.
As a result, once the active vbuckets became <100% resident, 1. is
always true. Given this, the PagingVisitor would evict until `mem_used
< mem_high_wat` (making 2. true), and would then skip any remaining
vbuckets.

This meant eviction would never reach the low watermark.

Fixed by removing the replica residency check from
PagingVisitor::visitBucket. This has negligible impact on eviction, as
active data is _already_ kept in memory preferentially after changes
for MB-40531.

Change-Id: I08b3b3de9a9a35673f2e9596652e370484ab6267
Reviewed-on: http://review.couchbase.org/c/kv_engine/+/143661
Tested-by: Build Bot <[email protected]>
Reviewed-by: Dave Rigby <[email protected]>

Author: jameseh96

engines/ep/src/paging_visitor.cc

@@ -228,11 +228,6 @@ void PagingVisitor::visitBucket(const VBucketPtr& vb) {
     // skip active vbuckets if active resident ratio is lower than replica
     auto current = static_cast<double>(stats.getEstimatedTotalMemoryUsed());
     auto lower = static_cast<double>(stats.mem_low_wat);
-    auto high = static_cast<double>(stats.mem_high_wat);
-    if (vb->getState() == vbucket_state_active && current < high &&
-        store.getActiveResidentRatio() < store.getReplicaResidentRatio()) {
-        return;
-    }
 
     if (current > lower) {
         if (vBucketFilter(vb->getId())) {

engines/ep/tests/module_tests/item_pager_test.cc

@@ -21,6 +21,7 @@
 
 #include "../mock/mock_global_task.h"
 #include "../mock/mock_paging_visitor.h"
+#include "../mock/mock_stat_collector.h"
 #include "bgfetcher.h"
 #include "checkpoint_manager.h"
 #include "checkpoint_utils.h"
@@ -36,6 +37,7 @@
 
 #include <folly/portability/GTest.h>
 #include <programs/engine_testapp/mock_server.h>
+#include <statistics/labelled_collector.h>
 #include <string_utilities.h>
 #include <xattr/blob.h>
 #include <xattr/utils.h>
@@ -198,7 +200,8 @@ class STBucketQuotaTest : public STParameterizedBucketTest {
      */
     size_t populateVbsUntil(const std::vector<Vbid>& vbids,
                             const std::function<bool()>& predicate,
-                            std::string_view keyPrefix = "key_") {
+                            std::string_view keyPrefix = "key_",
+                            size_t itemSize = 5000) {
         bool populate = true;
         int count = 0;
         while (populate) {
@@ -208,7 +211,8 @@ class STBucketQuotaTest : public STParameterizedBucketTest {
                 {
                     auto key = makeStoredDocKey(std::string(keyPrefix) +
                                                 std::to_string(count++));
-                    auto item = make_item(vbid, key, std::string(5000, 'x'));
+                    auto item =
+                            make_item(vbid, key, std::string(itemSize, 'x'));
                     // make the items have a range of MFU values
                     // without this, no matter what percent of items should be
                     // evicted, either all of them or none of them would be
@@ -1219,6 +1223,122 @@ TEST_P(STItemPagerTest, ItemPagerEvictionOrder) {
         ;
 }
 
+TEST_P(STItemPagerTest, MB43559_EvictionWithoutReplicasReachesLWM) {
+    // MB-43559: Test that eviction does not stop once memory usage drops
+    // below the high watermark if there are no replica vbuckets
+
+    if (!persistent()) {
+        // ephemeral buckets are never less than 100% resident and are not
+        // vulnerable to the bug (see MB)
+        return;
+    }
+
+    // Magma's memory usage makes calculations around recovering memory
+    // through eviction difficult. Increase the quota to allow eviction
+    // to actually reach the low watermark despite magma's memory usage
+    if (isMagma()) {
+        increaseQuota(2 * 1024 * 1024);
+    }
+
+    // issue involves eviction skipping vbuckets after mem_used < hwm,
+    // so multiple vbuckets are needed
+    std::vector<Vbid> vbids = {Vbid(0), Vbid(1), Vbid(2), Vbid(3)};
+
+    // set vbs active
+    for (const auto& vb : vbids) {
+        setVBucketStateAndRunPersistTask(vb, vbucket_state_active);
+    }
+
+    auto& stats = engine->getEpStats();
+
+    // check that baseline memory usage is definitely below the LWM
+    ASSERT_LT(stats.getPreciseTotalMemoryUsed(), stats.mem_low_wat.load());
+
+    auto aboveHWM = [&stats]() {
+        return stats.getPreciseTotalMemoryUsed() > stats.mem_high_wat.load();
+    };
+
+    auto itemCount = 0;
+    auto i = 0;
+    // items need to be persisted to be evicted, but flushing after each item
+    // would be slow. Load to the HWM, then flush, then repeat if we dropped
+    // below the HWM
+    while (!aboveHWM()) {
+        itemCount += populateVbsUntil(
+                vbids, aboveHWM, "keys_" + std::to_string(i++) + "_", 500);
+        for (const auto& vb : vbids) {
+            // flush and remove checkpoints as eviction will not touch
+            // dirty items
+            flushAndRemoveCheckpoints(vb);
+        }
+    }
+
+    EXPECT_GT(itemCount, 100);
+
+    // Note: prior to the fix for this MB, eviction relied upon cached
+    // residency ratios _which were only updated when gathering stats_.
+    // This is no longer the case post-fix, but to ensure the test _would_
+    // fail prior to the fix, this is done here.
+    auto refreshCachedResidentRatios = [&store=store, &engine=engine]() {
+        // call getAggregatedVBucketStats to updated cached resident ratios (as
+        // returned by `store->get*ResidentRatio()`)
+        testing::NiceMock<MockStatCollector> collector;
+        store->getAggregatedVBucketStats(collector.forBucket("foobar"));
+    };
+    refreshCachedResidentRatios();
+
+    // confirm we are above the high watermark, and can test the item pager
+    // behaviour
+    ASSERT_GT(stats.getEstimatedTotalMemoryUsed(), stats.mem_high_wat.load());
+
+    // there are no replica buckets
+    ASSERT_EQ(0, store->getNumOfVBucketsInState(vbucket_state_replica));
+    // the replica resident ratio defaults to 100 as there are no replica items
+    ASSERT_EQ(100, store->getReplicaResidentRatio());
+
+    // age is not relevant to this test, increase the mfu threshold under which
+    // age is ignored
+    auto& config = engine->getConfiguration();
+    config.setItemEvictionFreqCounterAgeThreshold(255);
+
+    // to make sure this isn't sensitive to small changes in memory usage,
+    // try to evict _everything_ (eviction ratio 1.0). If mem_used still hasn't
+    // gone below the lwm then something is definitely wrong.
+    auto available = std::make_shared<std::atomic<bool>>();
+
+    auto pv = std::make_unique<MockPagingVisitor>(
+            *store,
+            stats,
+            EvictionRatios{1.0 /* active&pending */, 1.0 /* replica */},
+            available,
+            ITEM_PAGER,
+            false,
+            VBucketFilter(vbids),
+            config.getItemEvictionAgePercentage(),
+            config.getItemEvictionFreqCounterAgeThreshold());
+
+    auto label = "Item pager";
+    auto taskid = TaskId::ItemPagerVisitor;
+    VBCBAdaptor task(store,
+                     taskid,
+                     std::move(pv),
+                     label,
+                     /*shutdown*/ false);
+
+    // call the run method repeatedly until it returns false, indicating the
+    // visitor is definitely done, rather than paused
+    while (task.run()) {
+        // need to refresh the resident ratio so it reflects active RR < 100
+        // after some items have been evicted. This wouldn't need to happen
+        // _during_ eviction in a full repro, just _once_ ever after active RR
+        // drops.
+        refreshCachedResidentRatios();
+    }
+
+    // confirm that memory usage is now below the low watermark
+    EXPECT_LT(stats.getPreciseTotalMemoryUsed(), stats.mem_low_wat.load());
+}
+
 /**
  * Test fixture for Ephemeral-only item pager tests.
  */

engines/ep/tests/module_tests/kv_bucket_test.h

@@ -95,6 +95,11 @@ class KVBucketTest : virtual public ::testing::Test {
      */
     void flush_vbucket_to_disk(Vbid vbid, size_t expected = 1);
 
+    /**
+     * Check if the current bucket is a persistent bucket.
+     */
+    bool persistent() const;
+
     /**
      * Flush the given vBucket to disk if the bucket is peristent, otherwise
      * do nothing.