MGM: FlatScheduler: fix contains implementation

We only checked the first element in the past leading to scheduler giving wrong results in large EC placements. Fix this, also add tests to catch this Fixes: EOS-6266 Signed-off-by: Abhishek Lekshmanan <[email protected]>
cern-eos · Nov 28, 2024 · 818e29c · 818e29c
1 parent 84152c3
commit 818e29c
Show file tree

Hide file tree

Showing 3 changed files with 69 additions and 7 deletions.
diff --git a/mgm/placement/PlacementStrategy.hh b/mgm/placement/PlacementStrategy.hh
@@ -73,12 +73,9 @@ struct PlacementResult {
   }
 
   bool contains(item_id_t item) const {
-    for (int i=0; i< n_replicas; ++i) {
-      if (ids[i] == item) {
-        return true;
-      }
-      return false;
-    }
+    return std::find(ids.cbegin(),
+                     ids.cbegin() + n_replicas,
+                     item) != ids.cbegin() + n_replicas;
   }
 };
 

diff --git a/unit_tests/CMakeLists.txt b/unit_tests/CMakeLists.txt
@@ -93,7 +93,8 @@ set(MGM_UT_SRCS
   mgm/placement/RRSeedTests.cc
   mgm/placement/SchedulerTests.cc
   mgm/placement/ThreadLocalRRSeedTests.cc
-  mgm/placement/FsSchedulerTests.cc)
+  mgm/placement/FsSchedulerTests.cc
+  mgm/placement/PlacementStrategyTests.cc)
 
 set(COMMON_UT_SRCS
   common/UtilsTests.cc

diff --git a/unit_tests/mgm/placement/PlacementStrategyTests.cc b/unit_tests/mgm/placement/PlacementStrategyTests.cc
@@ -0,0 +1,64 @@
+//------------------------------------------------------------------------------
+//! @file PlacementStrategy.hh
+//! @author Abhishek Lekshmanan <[email protected]>
+//-----------------------------------------------------------------------------
+
+/************************************************************************
+  * EOS - the CERN Disk Storage System                                   *
+  * Copyright (C) 2024 CERN/Switzerland                           *
+  *                                                                      *
+  * This program is free software: you can redistribute it and/or modify *
+  * it under the terms of the GNU General Public License as published by *
+  * the Free Software Foundation, either version 3 of the License, or    *
+  * (at your option) any later version.                                  *
+  *                                                                      *
+  * This program is distributed in the hope that it will be useful,      *
+  * but WITHOUT ANY WARRANTY; without even the implied warranty of       *
+  * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the        *
+  * GNU General Public License for more details.                         *
+  *                                                                      *
+  * You should have received a copy of the GNU General Public License    *
+  * along with this program.  If not, see <http://www.gnu.org/licenses/>.*
+  ************************************************************************/
+
+
+#include "mgm/placement/PlacementStrategy.hh"
+#include "gtest/gtest.h"
+
+TEST(PlacementResult, default)
+{
+  eos::mgm::placement::PlacementResult result;
+  EXPECT_EQ(result.ret_code, -1);
+  EXPECT_EQ(result.error_string(), "");
+  EXPECT_FALSE(result.is_valid_placement(2));
+}
+
+TEST(PlacementResult, is_valid_placement)
+{
+  eos::mgm::placement::PlacementResult result(2);
+  result.ids =  {1,2};
+  EXPECT_TRUE(result.is_valid_placement(2));
+
+  eos::mgm::placement::PlacementResult result2(2);
+  result2.ids = {1,-1};
+  EXPECT_FALSE(result2.is_valid_placement(2));
+}
+
+TEST(PlacementResult, contains)
+{
+  eos::mgm::placement::PlacementResult result(2);
+  result.ids = {1,2};
+  EXPECT_TRUE(result.contains(1));
+  EXPECT_TRUE(result.contains(2));
+  EXPECT_FALSE(result.contains(3));
+}
+
+TEST(PlacementResult, contains_invalid)
+{
+  eos::mgm::placement::PlacementResult result(2);
+  result.ids = {4,3,2,1}; // anything beyond 2nd slot is irrelevant
+  EXPECT_FALSE(result.contains(2));
+  EXPECT_FALSE(result.contains(1));
+  EXPECT_TRUE(result.contains(4));
+  EXPECT_TRUE(result.contains(3));
+}