From: Yuval Lifshitz <ylifshit@redhat.com>
Date: Thu, 12 May 2022 22:55:44 +0000 (+0300)
Subject: rgw: fix lua background crash on zone reload
X-Git-Tag: v18.0.0~742^2~1
X-Git-Url: http://git-server-git.apps.pok.os.sepia.ceph.com/?a=commitdiff_plain;h=221b499b654d62e4ee8a065987a72173d0a24bfc;p=ceph.git

rgw: fix lua background crash on zone reload

Signed-off-by: Yuval Lifshitz <ylifshit@redhat.com>
---

diff --git a/src/rgw/rgw_lua_background.cc b/src/rgw/rgw_lua_background.cc
index 33b0982b1d69..ab4f9f5fc4d6 100644
--- a/src/rgw/rgw_lua_background.cc
+++ b/src/rgw/rgw_lua_background.cc
@@ -5,16 +5,28 @@
 #include "include/ceph_assert.h"
 #include <lua.hpp>
 
+#define dout_subsys ceph_subsys_rgw
+
 namespace rgw::lua {
 
+Background::Background(rgw::sal::Store* store,
+    CephContext* cct,
+      const std::string& luarocks_path,
+      int execute_interval) :
+    execute_interval(execute_interval),
+    dp(cct, dout_subsys, "lua background: "),
+    store(store),
+    cct(cct),
+    luarocks_path(luarocks_path) {}
+
 void Background::shutdown(){
-  this->stop();
+  stopped = true;
+  cond.notify_all();
   if (runner.joinable()) {
     runner.join();
   }
-}
-void Background::stop(){
-  stopped = true;
+  started = false;
+  stopped = false;
 }
 
 void Background::start() {
@@ -29,12 +41,46 @@ void Background::start() {
   ceph_assert(rc == 0);
 }
 
+void Background::pause() {
+  {
+    std::unique_lock cond_lock(pause_mutex);
+    paused = true;
+  }
+  cond.notify_all();
+}
+
+void Background::resume(rgw::sal::Store* _store) {
+  store = _store;
+  paused = false;
+  cond.notify_all();
+}
+
 int Background::read_script() {
+  std::unique_lock cond_lock(pause_mutex);
+  if (paused) {
+    return -EAGAIN;
+  }
   std::string tenant;
-  return rgw::lua::read_script(dpp, store, tenant, null_yield, rgw::lua::context::background, rgw_script);
+  return rgw::lua::read_script(&dp, store, tenant, null_yield, rgw::lua::context::background, rgw_script);
+}
+
+const std::string Background::empty_table_value;
+
+const std::string& Background::get_table_value(const std::string& key) const {
+  std::unique_lock cond_lock(table_mutex);
+  const auto it = rgw_map.find(key);
+  if (it == rgw_map.end()) {
+    return empty_table_value;
+  }
+  return it->second;
 }
 
-//(1) Loads the script from the object
+void Background::put_table_value(const std::string& key, const std::string& value) {
+  std::unique_lock cond_lock(table_mutex);
+  rgw_map[key] = value;
+}
+
+//(1) Loads the script from the object if not paused
 //(2) Executes the script
 //(3) Sleep (configurable)
 void Background::run() {
@@ -44,11 +90,22 @@ void Background::run() {
   set_package_path(L, luarocks_path);
   create_debug_action(L, cct);
   create_background_metatable(L);
+  const DoutPrefixProvider* const dpp = &dp;
 
   while (!stopped) {
+    if (paused) {
+      ldpp_dout(dpp, 10) << "Lua background thread paused" << dendl;
+      std::unique_lock cond_lock(cond_mutex);
+      cond.wait(cond_lock, [this]{return !paused || stopped;}); 
+      if (stopped) {
+        ldpp_dout(dpp, 10) << "Lua background thread stopped" << dendl;
+        return;
+      }
+      ldpp_dout(dpp, 10) << "Lua background thread resumed" << dendl;
+    }
     const auto rc = read_script();
-    if (rc == -ENOENT) {
-      // no script, nothing to do
+    if (rc == -ENOENT || rc == -EAGAIN) {
+      // either no script or paused, nothing to do
     } else if (rc < 0) {
       ldpp_dout(dpp, 1) << "WARNING: failed to read background script. error " << rc << dendl;
     } else {
@@ -68,12 +125,14 @@ void Background::run() {
         perfcounter->inc((failed ? l_rgw_lua_script_fail : l_rgw_lua_script_ok), 1);
       }
     }
-    std::this_thread::sleep_for(std::chrono::seconds(execute_interval));
+    std::unique_lock cond_lock(cond_mutex);
+    cond.wait_for(cond_lock, std::chrono::seconds(execute_interval), [this]{return stopped;}); 
   }
+  ldpp_dout(dpp, 10) << "Lua background thread stopped" << dendl;
 }
 
 void Background::create_background_metatable(lua_State* L) {
-  create_metatable<rgw::lua::RGWTable>(L, true, &rgw_map, &m_mutex);
+  create_metatable<rgw::lua::RGWTable>(L, true, &rgw_map, &table_mutex);
 }
 
 } //namespace lua
diff --git a/src/rgw/rgw_lua_background.h b/src/rgw/rgw_lua_background.h
index c7aefc1f84ea..3baca6b321fc 100644
--- a/src/rgw/rgw_lua_background.h
+++ b/src/rgw/rgw_lua_background.h
@@ -3,6 +3,7 @@
 #include "rgw_common.h"
 #include <string>
 #include "rgw_lua_utils.h"
+#include "rgw_realm_reloader.h"
 
 namespace rgw::lua {
 
@@ -32,19 +33,24 @@ struct RGWTable : StringMapMetaTable<BackgroundMap,
     }
 };
 
-class Background {
+class Background : public RGWRealmReloader::Pauser {
 
 private:
   BackgroundMap rgw_map;
   bool stopped = false;
   bool started = false;
-  int execute_interval = INIT_EXECUTE_INTERVAL;
-  const DoutPrefixProvider* const dpp;
-  rgw::sal::Store* const store;
+  bool paused = false;
+  int execute_interval;
+  const DoutPrefix dp;
+  rgw::sal::Store* store;
   CephContext* const cct;
   const std::string luarocks_path;
   std::thread runner;
-  std::mutex m_mutex;
+  mutable std::mutex table_mutex;
+  std::mutex cond_mutex;
+  std::mutex pause_mutex;
+  std::condition_variable cond;
+  static const std::string empty_table_value;
 
   void run();
 
@@ -53,20 +59,20 @@ protected:
   virtual int read_script();
 
 public:
-  Background(const DoutPrefixProvider* dpp,
-      rgw::sal::Store* store,
+  Background(rgw::sal::Store* store,
       CephContext* cct,
-      const std::string& luarocks_path) :
-    dpp(dpp),
-    store(store),
-    cct(cct),
-    luarocks_path(luarocks_path) {}
+      const std::string& luarocks_path,
+      int execute_interval = INIT_EXECUTE_INTERVAL);
 
     virtual ~Background() = default;
     void start();
-    void stop();
     void shutdown();
     void create_background_metatable(lua_State* L);
+    const std::string& get_table_value(const std::string& key) const;
+    void put_table_value(const std::string& key, const std::string& value);
+    
+    void pause() override;
+    void resume(rgw::sal::Store* _store) override;
 };
 
 } //namepsace lua
diff --git a/src/rgw/rgw_main.cc b/src/rgw/rgw_main.cc
index 55edcce7a2f3..912957e8483a 100644
--- a/src/rgw/rgw_main.cc
+++ b/src/rgw/rgw_main.cc
@@ -188,6 +188,25 @@ static RGWRESTMgr *rest_filter(rgw::sal::Store* store, int dialect, RGWRESTMgr *
   }
 }
 
+class RGWPauser : public RGWRealmReloader::Pauser {
+  std::vector<Pauser*> pausers;
+
+public:
+  ~RGWPauser() override = default;
+  
+  void add_pauser(Pauser* pauser) {
+    pausers.push_back(pauser);
+  }
+
+  void pause() override {
+    std::for_each(pausers.begin(), pausers.end(), [](Pauser* p){p->pause();});
+  }
+  void resume(rgw::sal::Store* store) override {
+    std::for_each(pausers.begin(), pausers.end(), [store](Pauser* p){p->resume(store);});
+  }
+
+};
+
 /*
  * start up the RADOS connection and then handle HTTP messages as they come in
  */
@@ -606,8 +625,11 @@ int radosgw_Main(int argc, const char **argv)
 
   int fe_count = 0;
 
-  rgw::lua::Background lua_background(&dp, store, cct.get(), store->get_luarocks_path());
-  lua_background.start();
+  std::unique_ptr<rgw::lua::Background> lua_background;
+  if (rados) { /* Supported for only RadosStore */
+    lua_background = std::make_unique<rgw::lua::Background>(store, cct.get(), store->get_luarocks_path());
+    lua_background->start();
+  }
 
   for (multimap<string, RGWFrontendConfig *>::iterator fiter = fe_map.begin();
        fiter != fe_map.end(); ++fiter, ++fe_count) {
@@ -628,7 +650,7 @@ int radosgw_Main(int argc, const char **argv)
       config->get_val("prefix", "", &uri_prefix);
 
       RGWProcessEnv env = { store, &rest, olog, port, uri_prefix, 
-                            auth_registry, &ratelimiting, &lua_background};
+                            auth_registry, &ratelimiting, lua_background.get()};
 
       fe = new RGWLoadGenFrontend(env, config);
     }
@@ -638,7 +660,7 @@ int radosgw_Main(int argc, const char **argv)
       std::string uri_prefix;
       config->get_val("prefix", "", &uri_prefix);
       RGWProcessEnv env{ store, &rest, olog, port, uri_prefix, 
-                         auth_registry, &ratelimiting, &lua_background};
+                         auth_registry, &ratelimiting, lua_background.get()};
       fe = new RGWAsioFrontend(env, config, sched_ctx);
     }
 
@@ -675,13 +697,19 @@ int radosgw_Main(int argc, const char **argv)
 
   std::unique_ptr<RGWRealmReloader> reloader;
   std::unique_ptr<RGWPeriodPusher> pusher;
-  std::unique_ptr<RGWFrontendPauser> pauser;
+  std::unique_ptr<RGWFrontendPauser> fe_pauser;
   std::unique_ptr<RGWRealmWatcher> realm_watcher;
+  std::unique_ptr<RGWPauser> rgw_pauser;
   if (store->get_name() == "rados") {
     // add a watcher to respond to realm configuration changes
     pusher = std::make_unique<RGWPeriodPusher>(&dp, store, null_yield);
-    pauser = std::make_unique<RGWFrontendPauser>(fes, implicit_tenant_context, pusher.get());
-    reloader = std::make_unique<RGWRealmReloader>(store, service_map_meta, pauser.get());
+    fe_pauser = std::make_unique<RGWFrontendPauser>(fes, implicit_tenant_context, pusher.get());
+    rgw_pauser = std::make_unique<RGWPauser>();
+    rgw_pauser->add_pauser(fe_pauser.get());
+    if (lua_background) {
+      rgw_pauser->add_pauser(lua_background.get());
+    }
+    reloader = std::make_unique<RGWRealmReloader>(store, service_map_meta, rgw_pauser.get());
 
     realm_watcher = std::make_unique<RGWRealmWatcher>(&dp, g_ceph_context,
 				  static_cast<rgw::sal::RadosStore*>(store)->svc()->zone->get_realm());
@@ -731,6 +759,10 @@ int radosgw_Main(int argc, const char **argv)
   rgw_log_usage_finalize();
   delete olog;
 
+  if (lua_background) {
+    lua_background->shutdown();
+  }
+
   StoreManager::close_storage(store);
   rgw::auth::s3::LDAPEngine::shutdown();
   rgw_tools_cleanup();
@@ -746,7 +778,6 @@ int radosgw_Main(int argc, const char **argv)
   rgw::kafka::shutdown();
 #endif
 
-  lua_background.shutdown();
 
   rgw_perf_stop(g_ceph_context);
 
diff --git a/src/test/rgw/test_rgw_lua.cc b/src/test/rgw/test_rgw_lua.cc
index 4e69510d0465..86bd79250a64 100644
--- a/src/test/rgw/test_rgw_lua.cc
+++ b/src/test/rgw/test_rgw_lua.cc
@@ -153,24 +153,22 @@ public:
   }
 };
 
-auto cct = new CephContext(CEPH_ENTITY_TYPE_CLIENT);
+auto g_cct = new CephContext(CEPH_ENTITY_TYPE_CLIENT);
 
-CctCleaner cleaner(cct);
+CctCleaner cleaner(g_cct);
+
+#define DEFINE_REQ_STATE RGWEnv e; req_state s(g_cct, &e, 0);
 
 TEST(TestRGWLua, EmptyScript)
 {
   const std::string script;
 
-  RGWEnv e;
-  uint64_t id = 0;
-  req_state s(cct, &e, id); 
+  DEFINE_REQ_STATE;
 
   const auto rc = lua::request::execute(nullptr, nullptr, nullptr, &s, "", script);
   ASSERT_EQ(rc, 0);
 }
 
-#define DEFINE_REQ_STATE RGWEnv e; req_state s(cct, &e, 0);
-
 TEST(TestRGWLua, SyntaxError)
 {
   const std::string script = R"(
@@ -268,7 +266,7 @@ TEST(TestRGWLua, SetResponse)
   ASSERT_EQ(rc, 0);
 }
 
-TEST(TestRGWLua, SetRGWId)
+TEST(TestRGWLua, RGWIdNotWriteable)
 {
   const std::string script = R"(
     assert(Request.RGWId == "foo")
@@ -480,7 +478,7 @@ TEST(TestRGWLua, Acl)
   ACLOwner owner;
   owner.set_id(rgw_user("jack", "black"));
   owner.set_name("jack black");
-  s.user_acl.reset(new RGWAccessControlPolicy(cct));
+  s.user_acl.reset(new RGWAccessControlPolicy(g_cct));
   s.user_acl->set_owner(owner);
   ACLGrant grant1, grant2, grant3, grant4, grant5;
   grant1.set_canon(rgw_user("jane", "doe"), "her grant", 1);
@@ -648,15 +646,18 @@ TEST(TestRGWLua, OpsLog)
 }
 
 class TestBackground : public rgw::lua::Background {
+  const unsigned read_time;
 protected:
   int read_script() override {
     // don't read the object from the store
+    std::this_thread::sleep_for(std::chrono::seconds(read_time));
     return 0;
   }
 
 public:
-  TestBackground(const std::string& script) : 
-    rgw::lua::Background(nullptr, nullptr, nullptr, "") {
+  TestBackground(const std::string& script, unsigned read_time = 0) : 
+    rgw::lua::Background(nullptr, g_cct, "", 1 /*run every second*/),
+    read_time(read_time) {
       // the script is passed in the constructor
       rgw_script = script;
     }
@@ -666,7 +667,7 @@ public:
   }
 };
 
-TEST(TestRGWLua, Background)
+TEST(TestRGWLuaBackground, Start)
 {
   {
     // ctr and dtor without running
@@ -676,56 +677,175 @@ TEST(TestRGWLua, Background)
     // ctr and dtor with running
     TestBackground lua_background("");
     lua_background.start();
-    // let the background context run for 5 seconds
-    std::this_thread::sleep_for(std::chrono::seconds(5));
   }
 }
 
-TEST(TestRGWLua, BackgroundScript)
+
+constexpr auto wait_time = std::chrono::seconds(2);
+
+TEST(TestRGWLuaBackground, Script)
 {
   const std::string script = R"(
     local key = "hello"
     local value = "world"
     RGW[key] = value
-    print(RGW[key] == value)
   )";
 
   TestBackground lua_background(script);
   lua_background.start();
-  // let the background context run for 5 seconds
-  std::this_thread::sleep_for(std::chrono::seconds(5));
+  std::this_thread::sleep_for(wait_time);
+  EXPECT_EQ(lua_background.get_table_value("hello"), "world");
 }
 
-TEST(TestRGWLua, BackgroundRequestScript)
+TEST(TestRGWLuaBackground, RequestScript)
 {
   const std::string background_script = R"(
     local key = "hello"
     local value = "from background"
-    -- set the value only if not set
-    if RGW[key] == nil then 
-      RGW[key] = value
-    end
-    print("from background:", RGW[key])
+    RGW[key] = value
   )";
 
   TestBackground lua_background(background_script);
   lua_background.start();
-  // let the background context run for 5 seconds
-  std::this_thread::sleep_for(std::chrono::seconds(5));
+  std::this_thread::sleep_for(wait_time);
 
   const std::string request_script = R"(
     local key = "hello"
+    assert(RGW[key] == "from background") 
     local value = "from request"
-    print("from request (before setting):", RGW[key])
     RGW[key] = value
-    print("from request (after setting):", RGW[key])
   )";
 
   DEFINE_REQ_STATE;
 
+  // to make sure test is consistent we have to puase the background
+  lua_background.pause();
   const auto rc = lua::request::execute(nullptr, nullptr, nullptr, &s, "", request_script, &lua_background);
   ASSERT_EQ(rc, 0);
-  // let the background context run for 5 seconds
-  std::this_thread::sleep_for(std::chrono::seconds(5));
+  EXPECT_EQ(lua_background.get_table_value("hello"), "from request");
+  // now we resume and let the background set the value
+  lua_background.resume(nullptr);
+  std::this_thread::sleep_for(wait_time);
+  EXPECT_EQ(lua_background.get_table_value("hello"), "from background");
+}
+
+TEST(TestRGWLuaBackground, Pause)
+{
+  const std::string script = R"(
+    local key = "hello"
+    local value = "1"
+    if RGW[key] then
+      RGW[key] = value..RGW[key]
+    else
+      RGW[key] = value
+    end
+  )";
+
+  TestBackground lua_background(script);
+  lua_background.start();
+  std::this_thread::sleep_for(wait_time);
+  const auto value_len = lua_background.get_table_value("hello").size();
+  EXPECT_GT(value_len, 0);
+  lua_background.pause();
+  std::this_thread::sleep_for(wait_time);
+  // no change in len
+  EXPECT_EQ(value_len, lua_background.get_table_value("hello").size());
+}
+
+TEST(TestRGWLuaBackground, PauseWhileReading)
+{
+  const std::string script = R"(
+    local key = "hello"
+    local value = "world"
+    RGW[key] = value
+    if RGW[key] then
+      RGW[key] = value..RGW[key]
+    else
+      RGW[key] = value
+    end
+  )";
+
+  constexpr auto long_wait_time = std::chrono::seconds(6);
+  TestBackground lua_background(script, 2);
+  lua_background.start();
+  std::this_thread::sleep_for(long_wait_time);
+  const auto value_len = lua_background.get_table_value("hello").size();
+  EXPECT_GT(value_len, 0);
+  lua_background.pause();
+  std::this_thread::sleep_for(long_wait_time);
+  // one execution might occur after pause
+  EXPECT_TRUE(value_len + 1 >= lua_background.get_table_value("hello").size());
+}
+
+TEST(TestRGWLuaBackground, ReadWhilePaused)
+{
+  const std::string script = R"(
+    local key = "hello"
+    local value = "world"
+    RGW[key] = value
+  )";
+
+  TestBackground lua_background(script);
+  lua_background.pause();
+  lua_background.start();
+  std::this_thread::sleep_for(wait_time);
+  EXPECT_EQ(lua_background.get_table_value("hello"), "");
+  lua_background.resume(nullptr);
+  std::this_thread::sleep_for(wait_time);
+  EXPECT_EQ(lua_background.get_table_value("hello"), "world");
+}
+
+TEST(TestRGWLuaBackground, PauseResume)
+{
+  const std::string script = R"(
+    local key = "hello"
+    local value = "1"
+    if RGW[key] then
+      RGW[key] = value..RGW[key]
+    else
+      RGW[key] = value
+    end
+  )";
+
+  TestBackground lua_background(script);
+  lua_background.start();
+  std::this_thread::sleep_for(wait_time);
+  const auto value_len = lua_background.get_table_value("hello").size();
+  EXPECT_GT(value_len, 0);
+  lua_background.pause();
+  std::this_thread::sleep_for(wait_time);
+  // no change in len
+  EXPECT_EQ(value_len, lua_background.get_table_value("hello").size());
+  lua_background.resume(nullptr);
+  std::this_thread::sleep_for(wait_time);
+  // should be a change in len
+  EXPECT_GT(lua_background.get_table_value("hello").size(), value_len);
+}
+
+TEST(TestRGWLuaBackground, MultipleStarts)
+{
+  const std::string script = R"(
+    local key = "hello"
+    local value = "1"
+    if RGW[key] then
+      RGW[key] = value..RGW[key]
+    else
+      RGW[key] = value
+    end
+  )";
+
+  TestBackground lua_background(script);
+  lua_background.start();
+  std::this_thread::sleep_for(wait_time);
+  const auto value_len = lua_background.get_table_value("hello").size();
+  EXPECT_GT(value_len, 0);
+  lua_background.start();
+  lua_background.shutdown();
+  lua_background.shutdown();
+  std::this_thread::sleep_for(wait_time);
+  lua_background.start();
+  std::this_thread::sleep_for(wait_time);
+  // should be a change in len
+  EXPECT_GT(lua_background.get_table_value("hello").size(), value_len);
 }