osd: handle all the updated keys in OSD::maybe_override_options_for_qos

Up to now the first matching key from the changeset has been taken into
account only. Which is wrong as multiple relevant keys might be present
in it.
The primary issue with such a handling is a case when the first config
parameter update takes place. The received changset contains a bunch of
keys in this case and incorrect/incomplete processing might occur.
E.g. as a side effect this could fail TEST_recovery_limit_adjustment_mclock
from standalone/osd/mclock-config when mclock's OSD benchmark results
are out of expected range (e.g. when run on SSD drive) and hence there
 is no update for 'max_capacity_iops_config' config parameter. The
following update triggered from the test case isn't handled properly
then. Hence failing it.

This patch implements iteration over all the keys from the changeset and
does proper processing for all of them matching the patterns.

Fixes: https://tracker.ceph.com/issues/74678
Signed-off-by: Garry Drankovich <garry.drankovich@clyso.com>

diff --git a/src/osd/OSD.cc b/src/osd/OSD.cc
index 4de408d146f0438bdcd84b16354a341ed64533e6..f92985beec5043ccba99260e714e7e797a619ada 100644 (file)
--- a/src/osd/OSD.cc
+++ b/src/osd/OSD.cc
@@ -10360,49 +10360,39 @@ bool OSD::maybe_override_options_for_qos(const std::set<std::string> *changed)
         // Recovery options change was attempted without setting
         // the 'osd_mclock_override_recovery_settings' option.
         // Find the key to remove from the configuration db.
-        std::string key;
-        if (changed->count("osd_max_backfills")) {
-          key = "osd_max_backfills";
-        } else if (changed->count("osd_recovery_max_active")) {
-          key = "osd_recovery_max_active";
-        } else if (changed->count("osd_recovery_max_active_hdd")) {
-          key = "osd_recovery_max_active_hdd";
-        } else if (changed->count("osd_recovery_max_active_ssd")) {
-          key = "osd_recovery_max_active_ssd";
-        } else {
-          // No key that we are interested in. Return.
-          return true;
-        }
-
-        // Remove the current entry from the configuration if
-        // different from its default value.
-        auto val = recovery_qos_defaults.find(key);
-        if (val != recovery_qos_defaults.end() &&
+        static const std::vector<std::string> osds = {
+          "osd",
+          "osd." + std::to_string(whoami)
+        };
+        auto check_key = [&](const std::string& key) {
+          // Remove the current entry from the configuration if
+          // different from its default value.
+          auto val = recovery_qos_defaults.find(key);
+          if (val != recovery_qos_defaults.end() &&
             cct->_conf.get_val<uint64_t>(key) != val->second) {
-          static const std::vector<std::string> osds = {
-            "osd",
-            "osd." + std::to_string(whoami)
-          };
-
-          for (auto osd : osds) {
-            std::string cmd =
-              "{"
-                "\"prefix\": \"config rm\", "
-                "\"who\": \"" + osd + "\", "
-                "\"name\": \"" + key + "\""
-              "}";
-
-            dout(1) << __func__ << " Removing Key: " << key
-                    << " for " << osd << " from Mon db" << dendl;
-            monc->start_mon_command({std::move(cmd)}, {}, nullptr, nullptr, nullptr);
-          }
+            for (auto osd : osds) {
+              std::string cmd =
+                "{"
+                  "\"prefix\": \"config rm\", "
+                  "\"who\": \"" + osd + "\", "
+                  "\"name\": \"" + key + "\""
+                "}";
+
+              dout(1) << __func__ << " Removing Key: " << key
+                      << " for " << osd << " from Mon db" << dendl;
+              monc->start_mon_command({std::move(cmd)}, {}, nullptr, nullptr, nullptr);
+            }
 
-          // Raise a cluster warning indicating that the changes did not
-          // take effect and indicate the reason why.
-          clog->warn() << "Change to " << key << " on osd."
-                       << std::to_string(whoami) << " did not take effect."
-                       << " Enable osd_mclock_override_recovery_settings before"
-                       << " setting this option.";
+            // Raise a cluster warning indicating that the changes did not
+            // take effect and indicate the reason why.
+            clog->warn() << "Change to " << key << " on osd."
+                         << std::to_string(whoami) << " did not take effect."
+                         << " Enable osd_mclock_override_recovery_settings before"
+                         << " setting this option.";
+          }
+        };
+        for(auto& k : *changed) {
+          check_key(k);
         }
       }
     } else { // if (changed != nullptr) (osd boot-up)