fix(plugin): make the hot reload of plugins transactional

apisix/admin/init.lua

@@ -294,22 +294,40 @@ local function unsupported_methods_reload_plugin()
 end
 
 
+-- defined after sync_local_conf_to_etcd
+local reload_plugins_and_sync
+
+
 local function post_reload_plugins()
     set_ctx_and_check_token()
 
+    -- reload on this worker first: if the new plugin set cannot be loaded,
+    -- report the error to the operator instead of an unconditional "done",
+    -- and neither bump the version nor broadcast, so that neither the other
+    -- workers nor the reconciliation timer are asked to apply a plugin set
+    -- which is already known not to load
+    local ok, err = reload_plugins_and_sync()
+    if not ok then
+        core.log.error("failed to hot reload plugins: ", err)
+        core.response.exit(500, {error_msg = "failed to reload plugins: " .. err})
+    end
+
     if plugins_conf_ver_dict then
-        -- bump the version before broadcasting, so that a process which never
-        -- receives the event (e.g. the privileged agent while it is
-        -- reconnecting to the events broker) still converges through the
-        -- periodic reconciliation below
-        local _, err = plugins_conf_ver_dict:incr(PLUGINS_CONF_VERSION_KEY, 1, 0)
-        if err then
+        -- bump the version, so that a process which never receives the event
+        -- (e.g. the privileged agent while it is reconnecting to the events
+        -- broker) still converges through the periodic reconciliation below
+        local ver, incr_err = plugins_conf_ver_dict:incr(PLUGINS_CONF_VERSION_KEY, 1, 0)
+        if incr_err then
             -- if the version cannot be bumped the reconciliation timer will
             -- never notice a change, so a worker that misses the broadcast
             -- would stay stale forever; fail loud instead of pretending success
-            core.log.error("failed to increase plugins conf version: ", err)
+            core.log.error("failed to increase plugins conf version: ", incr_err)
             core.response.exit(503, {error_msg = "failed to record plugins reload"})
         end
+
+        -- this worker already applied the new set above, don't let its own
+        -- reconciliation timer redo the work one second later
+        applied_plugins_conf_version = ver
     end
 
     local success, err = events:post(reload_event, get_method(), ngx_time())
@@ -397,7 +415,7 @@ local function sync_local_conf_to_etcd(reset)
 end
 
 
-local function reload_plugins(data, event, source, pid)
+function reload_plugins_and_sync()
     core.log.info("start to hot reload plugins")
 
     -- sample the version before loading: if another reload is accepted while
@@ -408,15 +426,39 @@ local function reload_plugins(data, event, source, pid)
         ver = plugins_conf_ver_dict:get(PLUGINS_CONF_VERSION_KEY)
     end
 
-    plugin.load()
+    local ok, err = plugin.load()
 
+    -- record the sampled version even when the load failed: the reconciliation
+    -- timer would otherwise retry the very same plugin set every second, and
+    -- every attempt tears the live set down and rebuilds it
     if ver then
         applied_plugins_conf_version = ver
     end
 
+    if not ok then
+        return nil, err
+    end
+
     if ngx_worker_id() == 0 then
         sync_local_conf_to_etcd()
     end
+
+    return true
+end
+
+
+local function reload_plugins(data, event, source, wid)
+    if wid == ngx_worker_id() then
+        -- this worker has already reloaded synchronously while serving the
+        -- Admin API request, see post_reload_plugins()
+        return
+    end
+
+    local ok, err = reload_plugins_and_sync()
+    if not ok then
+        core.log.error("failed to hot reload plugins: ", err,
+                       ", this worker keeps the old plugin set")
+    end
 end
 
 

apisix/control/router.lua

@@ -199,9 +199,18 @@ end
 
 end -- do
 
-local function reload_plugins()
+local function reload_plugins(data, event, source, wid)
+    if wid == ngx.worker.id() then
+        -- already reloaded synchronously in post_reload_plugins()
+        return
+    end
+
     core.log.info("start to hot reload plugins")
-    plugin_mod.load()
+    local ok, err = plugin_mod.load()
+    if not ok then
+        core.log.error("failed to hot reload plugins: ", err,
+                       ", this worker keeps the old plugin set")
+    end
 end
 
 

apisix/control/v1.lua

@@ -412,11 +412,21 @@ function _M.dump_plugin_metadata()
 end
 
 function _M.post_reload_plugins()
-    -- Bump the shared version before broadcasting so that a worker which misses the
-    -- event (the resty.events broker gives no delivery guarantee while a worker is
-    -- reconnecting) still converges through the admin reconciliation timer. This is
-    -- the same guard the admin reload path added in #13714; the control path was
-    -- left out. When the admin is disabled the timer is absent and this is a no-op.
+    -- reload on this worker first so that a plugin set which cannot be loaded
+    -- is reported to the caller instead of being broadcast
+    core.log.info("start to hot reload plugins")
+    local ok, err = plugin.load()
+    if not ok then
+        core.log.error("failed to hot reload plugins: ", err)
+        core.response.exit(500, {error_msg = "failed to reload plugins: " .. err})
+    end
+
+    -- Bump the shared version once the load succeeded so that a worker which
+    -- misses the event (the resty.events broker gives no delivery guarantee
+    -- while a worker is reconnecting) still converges through the admin
+    -- reconciliation timer. This is the same guard the admin reload path added
+    -- in #13714; the control path was left out. When the admin is disabled the
+    -- timer is absent and this is a no-op.
     if plugins_conf_ver_dict then
         local _, incr_err = plugins_conf_ver_dict:incr(PLUGINS_CONF_VERSION_KEY, 1, 0)
         if incr_err then