From 8deb9495515b97898514e8ffb8f002c8afe3bfa7 Mon Sep 17 00:00:00 2001 From: Jo-Philipp Wich Date: Thu, 26 Apr 2018 08:52:55 +0200 Subject: [PATCH] treewide: rework uci apply workflow Switch to rpcd based uci apply/rollback workflow which helps to avoid soft- bricking devices by requiring an explicit confirmation call after config apply. When a user now clicks "Save & Apply", LuCI first issues a call to uci apply which commits and reloads configuration, then goes into a polling countdown mode where it repeatedly attempts to call uci confirm. If the committed configuration is sane, the confirm call will go through and cancel rpcd's pending rollback timer. If the configuration change leads to a loss of connectivity (e.g. due to bad firewall rules or similar), the rollback mechanism will kick in after the timeout and revert configuration files and pending changes to the pre-apply state. In order to cover such rare cases where a lost of connectivity is expected and desired, the user is offered an "unchecked" apply option after timing out, which allows committing and applying the changes anyway, without the extra safety checks. As a consequence of this change, the luci-reload mechanism is now completely unsused since rpcd uses ubus config reload signals to reload affected services, which means that only procd-enabled services will receive proper reload treatment with the new workflow. Signed-off-by: Jo-Philipp Wich --- .../luasrc/view/radicale/tabmap_nsections.htm | 17 +- modules/luci-base/luasrc/cbi.lua | 14 +- .../luasrc/controller/admin/servicectl.lua | 49 ----- modules/luci-base/luasrc/dispatcher.lua | 3 + modules/luci-base/luasrc/model/uci.lua | 139 +++++++------- modules/luci-base/luasrc/model/uci.luadoc | 57 +++++- .../luasrc/view/cbi/apply_widget.htm | 181 ++++++++++++++++++ .../luci-base/luasrc/view/cbi/apply_xhr.htm | 43 ----- modules/luci-base/luasrc/view/cbi/map.htm | 17 +- modules/luci-base/root/etc/config/luci | 6 + .../luasrc/controller/admin/uci.lua | 95 ++++++--- .../luasrc/view/admin_uci/apply.htm | 23 --- .../luasrc/view/admin_uci/changes.htm | 29 +-- .../luasrc/view/admin_uci/revert.htm | 25 ++- 14 files changed, 452 insertions(+), 246 deletions(-) delete mode 100644 modules/luci-base/luasrc/controller/admin/servicectl.lua create mode 100644 modules/luci-base/luasrc/view/cbi/apply_widget.htm delete mode 100644 modules/luci-base/luasrc/view/cbi/apply_xhr.htm delete mode 100644 modules/luci-mod-admin-full/luasrc/view/admin_uci/apply.htm diff --git a/applications/luci-app-radicale/luasrc/view/radicale/tabmap_nsections.htm b/applications/luci-app-radicale/luasrc/view/radicale/tabmap_nsections.htm index 45fe60cc8..2b526a31d 100644 --- a/applications/luci-app-radicale/luasrc/view/radicale/tabmap_nsections.htm +++ b/applications/luci-app-radicale/luasrc/view/radicale/tabmap_nsections.htm @@ -2,12 +2,23 @@
<%=pcdata(msg)%>
<%- end end -%> -<%-+cbi/apply_xhr-%> -
<% if self.title and #self.title > 0 then %>

<%=self.title%>

<% end %> <% if self.description and #self.description > 0 then %>
<%=self.description%>
<% end %> - <%- if firstmap and applymap then cbi_apply_xhr(self.config, parsechain, redirect) end -%> + <%- if firstmap and (applymap or confirmmap) then -%> + <%+cbi/apply_widget%> + <% cbi_apply_widget() %> + + + <%- end -%> <% if self.tabbed then %>
    diff --git a/modules/luci-base/luasrc/cbi.lua b/modules/luci-base/luasrc/cbi.lua index 218439503..472864211 100644 --- a/modules/luci-base/luasrc/cbi.lua +++ b/modules/luci-base/luasrc/cbi.lua @@ -388,21 +388,21 @@ function Map.parse(self, readinput, ...) if self.save then self:_run_hooks("on_save", "on_before_save") + local i, config for i, config in ipairs(self.parsechain) do self.uci:save(config) end self:_run_hooks("on_after_save") if (not self.proceed and self.flow.autoapply) or luci.http.formvalue("cbi.apply") then self:_run_hooks("on_before_commit") - for i, config in ipairs(self.parsechain) do - self.uci:commit(config) - - -- Refresh data because commit changes section names - self.uci:load(config) + if self.apply_on_parse == false then + for i, config in ipairs(self.parsechain) do + self.uci:commit(config) + end end self:_run_hooks("on_commit", "on_after_commit", "on_before_apply") - if self.apply_on_parse then - self.uci:apply(self.parsechain) + if self.apply_on_parse == true or self.apply_on_parse == false then + self.uci:apply(self.apply_on_parse) self:_run_hooks("on_apply", "on_after_apply") else -- This is evaluated by the dispatcher and delegated to the diff --git a/modules/luci-base/luasrc/controller/admin/servicectl.lua b/modules/luci-base/luasrc/controller/admin/servicectl.lua deleted file mode 100644 index 1d73eb4ec..000000000 --- a/modules/luci-base/luasrc/controller/admin/servicectl.lua +++ /dev/null @@ -1,49 +0,0 @@ --- Copyright 2010 Jo-Philipp Wich --- Licensed to the public under the Apache License 2.0. - -module("luci.controller.admin.servicectl", package.seeall) - -function index() - entry({"servicectl"}, alias("servicectl", "status")).sysauth = "root" - entry({"servicectl", "status"}, call("action_status")).leaf = true - entry({"servicectl", "restart"}, post("action_restart")).leaf = true -end - -function action_status() - local data = nixio.fs.readfile("/var/run/luci-reload-status") - if data then - luci.http.write("/etc/config/") - luci.http.write(data) - else - luci.http.write("finish") - end -end - -function action_restart(args) - local uci = require "luci.model.uci".cursor() - if args then - local service - local services = { } - - for service in args:gmatch("[%w_-]+") do - services[#services+1] = service - end - - local command = uci:apply(services, true) - if nixio.fork() == 0 then - local i = nixio.open("/dev/null", "r") - local o = nixio.open("/dev/null", "w") - - nixio.dup(i, nixio.stdin) - nixio.dup(o, nixio.stdout) - - i:close() - o:close() - - nixio.exec("/bin/sh", unpack(command)) - else - luci.http.write("OK") - os.exit(0) - end - end -end diff --git a/modules/luci-base/luasrc/dispatcher.lua b/modules/luci-base/luasrc/dispatcher.lua index 38d919481..baaa95ad7 100644 --- a/modules/luci-base/luasrc/dispatcher.lua +++ b/modules/luci-base/luasrc/dispatcher.lua @@ -883,6 +883,8 @@ local function _cbi(self, ...) local pageaction = true local parsechain = { } + local is_rollback, time_remaining = uci:rollback_pending() + for i, res in ipairs(maps) do if res.apply_needed and res.parsechain then local c @@ -910,6 +912,7 @@ local function _cbi(self, ...) res:render({ firstmap = (i == 1), applymap = applymap, + confirmmap = (is_rollback and time_remaining or nil), redirect = redirect, messages = messages, pageaction = pageaction, diff --git a/modules/luci-base/luasrc/model/uci.lua b/modules/luci-base/luasrc/model/uci.lua index fc2a605b3..34323f08b 100644 --- a/modules/luci-base/luasrc/model/uci.lua +++ b/modules/luci-base/luasrc/model/uci.lua @@ -3,6 +3,7 @@ local os = require "os" local util = require "luci.util" +local conf = require "luci.config" local table = require "table" @@ -143,22 +144,84 @@ function commit(self, config) return (err == nil), ERRSTR[err] end ---[[ -function apply(self, configs, command) - local _, config +function apply(self, rollback) + local _, err + + if rollback then + local timeout = tonumber(conf.apply and conf.apply.rollback or "") or 0 - assert(not command, "Apply command not supported anymore") + _, err = call("apply", { + timeout = (timeout > 30) and timeout or 30, + rollback = true + }) - if type(configs) == "table" then - for _, config in ipairs(configs) do - call("service", "event", { - type = "config.change", - data = { package = config } + if not err then + util.ubus("session", "set", { + ubus_rpc_session = session_id, + values = { rollback = os.time() + timeout } }) end + else + _, err = call("changes", {}) + + if not err then + if type(_) == "table" and type(_.changes) == "table" then + local k, v + for k, v in pairs(_.changes) do + _, err = call("commit", { config = k }) + if err then + break + end + end + end + end + + if not err then + _, err = call("apply", { rollback = false }) + end + end + + return (err == nil), ERRSTR[err] +end + +function confirm(self) + local _, err = call("confirm", {}) + if not err then + util.ubus("session", "set", { + ubus_rpc_session = session_id, + values = { rollback = 0 } + }) end + return (err == nil), ERRSTR[err] +end + +function rollback(self) + local _, err = call("rollback", {}) + if not err then + util.ubus("session", "set", { + ubus_rpc_session = session_id, + values = { rollback = 0 } + }) + end + return (err == nil), ERRSTR[err] +end + +function rollback_pending(self) + local deadline, err = util.ubus("session", "get", { + ubus_rpc_session = session_id, + keys = { "rollback" } + }) + + if type(deadline) == "table" and + type(deadline.values) == "table" and + type(deadline.values.rollback) == "number" and + deadline.values.rollback > os.time() + then + return true, deadline.values.rollback - os.time() + end + + return false, ERRSTR[err] end -]] function foreach(self, config, stype, callback) @@ -425,59 +488,3 @@ function delete_all(self, config, stype, comparator) return (err == nil), ERRSTR[err] end - - -function apply(self, configlist, command) - configlist = self:_affected(configlist) - if command then - return { "/sbin/luci-reload", unpack(configlist) } - else - return os.execute("/sbin/luci-reload %s >/dev/null 2>&1" - % util.shellquote(table.concat(configlist, " "))) - end -end - --- Return a list of initscripts affected by configuration changes. -function _affected(self, configlist) - configlist = type(configlist) == "table" and configlist or { configlist } - - -- Resolve dependencies - local reloadlist = { } - - local function _resolve_deps(name) - local reload = { name } - local deps = { } - - self:foreach("ucitrack", name, - function(section) - if section.affects then - for i, aff in ipairs(section.affects) do - deps[#deps+1] = aff - end - end - end) - - local i, dep - for i, dep in ipairs(deps) do - local j, add - for j, add in ipairs(_resolve_deps(dep)) do - reload[#reload+1] = add - end - end - - return reload - end - - -- Collect initscripts - local j, config - for j, config in ipairs(configlist) do - local i, e - for i, e in ipairs(_resolve_deps(config)) do - if not util.contains(reloadlist, e) then - reloadlist[#reloadlist+1] = e - end - end - end - - return reloadlist -end diff --git a/modules/luci-base/luasrc/model/uci.luadoc b/modules/luci-base/luasrc/model/uci.luadoc index ef89d09b9..d798b0033 100644 --- a/modules/luci-base/luasrc/model/uci.luadoc +++ b/modules/luci-base/luasrc/model/uci.luadoc @@ -28,12 +28,63 @@ Create a new Cursor initialized to the state directory. ]] ---[[ -Applies UCI configuration changes +Applies UCI configuration changes. + +If the rollback parameter is set to true, the apply function will invoke the +rollback mechanism which causes the configuration to be automatically reverted +if no confirm() call occurs within a certain timeout. + +The current default timeout is 30s and can be increased using the +"luci.apply.timeout" uci configuration key. @class function @name Cursor.apply -@param configlist List of UCI configurations -@param command Don't apply only return the command +@param rollback Enable rollback mechanism +@return Boolean whether operation succeeded +]] + +---[[ +Confirms UCI apply process. + +If a previous UCI apply with rollback has been invoked using apply(true), +this function confirms the process and cancels the pending rollback timer. + +If no apply with rollback session is active, the function has no effect and +returns with a "No data" error. + +@class function +@name Cursor.confirm +@return Boolean whether operation succeeded +]] + +---[[ +Cancels UCI apply process. + +If a previous UCI apply with rollback has been invoked using apply(true), +this function cancels the process and rolls back the configuration to the +pre-apply state. + +If no apply with rollback session is active, the function has no effect and +returns with a "No data" error. + +@class function +@name Cursor.rollback +@return Boolean whether operation succeeded +]] + +---[[ +Checks whether a pending rollback is scheduled. + +If a previous UCI apply with rollback has been invoked using apply(true), +and has not been confirmed or rolled back yet, this function returns true +and the remaining time until rollback in seconds. If no rollback is pending, +the function returns false. On error, the function returns false and an +additional string describing the error. + +@class function +@name Cursor.rollback_pending +@return Boolean whether rollback is pending +@return Remaining time in seconds ]] ---[[ diff --git a/modules/luci-base/luasrc/view/cbi/apply_widget.htm b/modules/luci-base/luasrc/view/cbi/apply_widget.htm new file mode 100644 index 000000000..543ef0b80 --- /dev/null +++ b/modules/luci-base/luasrc/view/cbi/apply_widget.htm @@ -0,0 +1,181 @@ +<% export("cbi_apply_widget", function(redirect_ok) -%> + + + + +<%- end) %> diff --git a/modules/luci-base/luasrc/view/cbi/apply_xhr.htm b/modules/luci-base/luasrc/view/cbi/apply_xhr.htm deleted file mode 100644 index daa57c1db..000000000 --- a/modules/luci-base/luasrc/view/cbi/apply_xhr.htm +++ /dev/null @@ -1,43 +0,0 @@ -<% export("cbi_apply_xhr", function(id, configs, redirect) -%> -
    - <%:Applying changes%> - - - <%:Loading%> - <%:Waiting for changes to be applied...%> -
    -<%- end) %> diff --git a/modules/luci-base/luasrc/view/cbi/map.htm b/modules/luci-base/luasrc/view/cbi/map.htm index e3210add6..69ef3615a 100644 --- a/modules/luci-base/luasrc/view/cbi/map.htm +++ b/modules/luci-base/luasrc/view/cbi/map.htm @@ -2,12 +2,23 @@
    <%=pcdata(msg)%>
    <%- end end -%> -<%-+cbi/apply_xhr-%> -
    <% if self.title and #self.title > 0 then %>

    <%=self.title%>

    <% end %> <% if self.description and #self.description > 0 then %>
    <%=self.description%>
    <% end %> - <%- if firstmap and applymap then cbi_apply_xhr(self.config, parsechain, redirect) end -%> + <%- if firstmap and (applymap or confirmmap) then -%> + <%+cbi/apply_widget%> + <% cbi_apply_widget(redirect) %> + + + <%- end -%> <% if self.tabbed then %>
      diff --git a/modules/luci-base/root/etc/config/luci b/modules/luci-base/root/etc/config/luci index baa3ac5d1..82c2230e5 100644 --- a/modules/luci-base/root/etc/config/luci +++ b/modules/luci-base/root/etc/config/luci @@ -22,3 +22,9 @@ config internal ccache option enable 1 config internal themes + +config internal apply + option rollback 30 + option holdoff 4 + option timeout 5 + option display 1.5 diff --git a/modules/luci-mod-admin-full/luasrc/controller/admin/uci.lua b/modules/luci-mod-admin-full/luasrc/controller/admin/uci.lua index ba317f9f4..9533ff5e6 100644 --- a/modules/luci-mod-admin-full/luasrc/controller/admin/uci.lua +++ b/modules/luci-mod-admin-full/luasrc/controller/admin/uci.lua @@ -11,54 +11,91 @@ function index() entry({"admin", "uci"}, nil, _("Configuration")) entry({"admin", "uci", "changes"}, call("action_changes"), _("Changes"), 40).query = {redir=redir} entry({"admin", "uci", "revert"}, post("action_revert"), _("Revert"), 30).query = {redir=redir} - entry({"admin", "uci", "apply"}, post("action_apply"), _("Apply"), 20).query = {redir=redir} - entry({"admin", "uci", "saveapply"}, post("action_apply"), _("Save & Apply"), 10).query = {redir=redir} + + local node + local authen = function(checkpass, allowed_users) + return "root", luci.http.formvalue("sid") + end + + node = entry({"admin", "uci", "apply_rollback"}, post("action_apply_rollback"), nil) + node.cors = true + node.sysauth_authenticator = authen + + node = entry({"admin", "uci", "apply_unchecked"}, post("action_apply_unchecked"), nil) + node.cors = true + node.sysauth_authenticator = authen + + node = entry({"admin", "uci", "confirm"}, post("action_confirm"), nil) + node.cors = true + node.sysauth_authenticator = authen end + function action_changes() - local uci = luci.model.uci.cursor() + local uci = require "luci.model.uci" local changes = uci:changes() luci.template.render("admin_uci/changes", { - changes = next(changes) and changes + changes = next(changes) and changes, + timeout = timeout }) end -function action_apply() - local path = luci.dispatcher.context.path - local uci = luci.model.uci.cursor() +function action_revert() + local uci = require "luci.model.uci" local changes = uci:changes() - local reload = {} - -- Collect files to be applied and commit changes + -- Collect files to be reverted + local r, tbl for r, tbl in pairs(changes) do - table.insert(reload, r) - if path[#path] ~= "apply" then - uci:load(r) - uci:commit(r) - uci:unload(r) - end + uci:revert(r) end - luci.template.render("admin_uci/apply", { - changes = next(changes) and changes, - configs = reload + luci.template.render("admin_uci/revert", { + changes = next(changes) and changes }) end -function action_revert() - local uci = luci.model.uci.cursor() - local changes = uci:changes() +local function ubus_state_to_http(errstr) + local map = { + ["Invalid command"] = 400, + ["Invalid argument"] = 400, + ["Method not found"] = 404, + ["Entry not found"] = 404, + ["No data"] = 204, + ["Permission denied"] = 403, + ["Timeout"] = 504, + ["Not supported"] = 500, + ["Unknown error"] = 500, + ["Connection failed"] = 503 + } - -- Collect files to be reverted - for r, tbl in pairs(changes) do - uci:load(r) - uci:revert(r) - uci:unload(r) + local code = map[errstr] or 200 + local msg = errstr or "OK" + + luci.http.status(code, msg) + + if code ~= 204 then + luci.http.prepare_content("text/plain") + luci.http.write(msg) end +end - luci.template.render("admin_uci/revert", { - changes = next(changes) and changes - }) +function action_apply_rollback() + local uci = require "luci.model.uci" + local _, errstr = uci:apply(true) + ubus_state_to_http(errstr) +end + +function action_apply_unchecked() + local uci = require "luci.model.uci" + local _, errstr = uci:apply(false) + ubus_state_to_http(errstr) +end + +function action_confirm() + local uci = require "luci.model.uci" + local _, errstr = uci:confirm() + ubus_state_to_http(errstr) end diff --git a/modules/luci-mod-admin-full/luasrc/view/admin_uci/apply.htm b/modules/luci-mod-admin-full/luasrc/view/admin_uci/apply.htm deleted file mode 100644 index 370027e51..000000000 --- a/modules/luci-mod-admin-full/luasrc/view/admin_uci/apply.htm +++ /dev/null @@ -1,23 +0,0 @@ -<%# - Copyright 2008 Steven Barth - Copyright 2008 Jo-Philipp Wich - Licensed to the public under the Apache License 2.0. --%> - -<%+header%> - -

      <%:Configuration%> / <%:Apply%>

      - -<% if changes then %> - <%+cbi/apply_xhr%> - <%+admin_uci/changelog%> - - <%- cbi_apply_xhr('uci-apply', configs) -%> - -

      <%:The following changes have been committed%>:

      - <%- uci_changelog(changes) -%> -<% else %> -

      <%:There are no pending changes to apply!%>

      -<% end %> - -<%+footer%> diff --git a/modules/luci-mod-admin-full/luasrc/view/admin_uci/changes.htm b/modules/luci-mod-admin-full/luasrc/view/admin_uci/changes.htm index 6e725c888..9e9ce2be2 100644 --- a/modules/luci-mod-admin-full/luasrc/view/admin_uci/changes.htm +++ b/modules/luci-mod-admin-full/luasrc/view/admin_uci/changes.htm @@ -1,40 +1,41 @@ <%# Copyright 2008 Steven Barth - Copyright 2008-2015 Jo-Philipp Wich + Copyright 2008-2018 Jo-Philipp Wich Licensed to the public under the Apache License 2.0. -%> <%+header%> +<%- + local node, redir_url = luci.dispatcher.lookup(luci.http.formvalue("redir")) + + include("cbi/apply_widget") + include("admin_uci/changelog") + + cbi_apply_widget(redir_url or url("admin/uci/changes")) +-%> +

      <%:Configuration%> / <%:Changes%>

      <% if changes then %> - <%+admin_uci/changelog%> <%- uci_changelog(changes) -%> <% else %>

      <%:There are no pending changes!%>

      <% end %> + +
      - <% local node, url = luci.dispatcher.lookup(luci.http.formvalue("redir")); if url then %> + <% if redir_url then %>
      -
      +
      <% end %>
      -
      - - " /> - -
      -
      - - " /> - -
      +
      " /> diff --git a/modules/luci-mod-admin-full/luasrc/view/admin_uci/revert.htm b/modules/luci-mod-admin-full/luasrc/view/admin_uci/revert.htm index 20327adff..dff53420a 100644 --- a/modules/luci-mod-admin-full/luasrc/view/admin_uci/revert.htm +++ b/modules/luci-mod-admin-full/luasrc/view/admin_uci/revert.htm @@ -1,26 +1,39 @@ <%# Copyright 2008 Steven Barth - Copyright 2008 Jo-Philipp Wich + Copyright 2008-2018 Jo-Philipp Wich Licensed to the public under the Apache License 2.0. -%> <%+header%> +<%- + local node, redir_url = luci.dispatcher.lookup(luci.http.formvalue("redir")) + + include("cbi/apply_widget") + include("admin_uci/changelog") + + cbi_apply_widget(redir_url or url("admin/uci/revert")) +-%> +

      <%:Configuration%> / <%:Revert%>

      <% if changes then %> - <%+cbi/apply_xhr%> - <%+admin_uci/changelog%> -

      <%:The following changes have been reverted%>:

      <%- uci_changelog(changes) -%> <% else %>

      <%:There are no pending changes to revert!%>

      <% end %> -<% local node, url = luci.dispatcher.lookup(luci.http.formvalue("redir")); if url then %> + + + +<% if redir_url then %>
      - +
      -- 2.25.1