[GitHub] [apisix] spacewander commented on a change in pull request #3848: feat: add dump for consul_kv
spacewander commented on a change in pull request #3848:
URL: https://github.com/apache/apisix/pull/3848#discussion_r597598980
##
File path: apisix/discovery/consul_kv.lua
##
@@ -244,7 +244,7 @@ end
local function read_dump_srvs()
local data, err = util.read_file(dump_params.path)
if not data then
-log.warn("read dump file get error: ", err)
+log.notice("read dump file get error: ", err)
Review comment:
@yongboy
We just add the heavy library in the master branch:
https://github.com/apache/apisix/blob/1064e2d4b7edcc6f590ca69dc34a100ae6bbc12c/rockspec/apisix-master-0.rockspec#L68.
Would be fine if we can find another usage of this library.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
[GitHub] [apisix] spacewander commented on a change in pull request #3848: feat: add dump for consul_kv
spacewander commented on a change in pull request #3848:
URL: https://github.com/apache/apisix/pull/3848#discussion_r597481993
##
File path: apisix/discovery/consul_kv.lua
##
@@ -244,7 +244,7 @@ end
local function read_dump_srvs()
local data, err = util.read_file(dump_params.path)
if not data then
-log.warn("read dump file get error: ", err)
+log.notice("read dump file get error: ", err)
Review comment:
I think we can check if the file exists first via
https://refined-github-html-preview.kidonng.workers.dev/lunarmodules/Penlight/raw/master/docs/libraries/pl.path.html#exists
If not exists, log a notice message.
And then try to read it.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
[GitHub] [apisix] spacewander commented on a change in pull request #3848: feat: add dump for consul_kv
spacewander commented on a change in pull request #3848:
URL: https://github.com/apache/apisix/pull/3848#discussion_r597337548
##
File path: apisix/discovery/consul_kv.lua
##
@@ -230,6 +241,73 @@ local function update_application(server_name_prefix, data)
end
+local function read_dump_srvs()
+local data, err = util.read_file(dump_params.path)
+if not data then
+log.warn("read dump file get error: ", err)
Review comment:
So you suggest to prepare the dump file before staring APISIX to avoid
the unexpected event (file not found)?
##
File path: apisix/discovery/consul_kv.lua
##
@@ -230,6 +241,73 @@ local function update_application(server_name_prefix, data)
end
+local function read_dump_srvs()
+local data, err = util.read_file(dump_params.path)
+if not data then
+log.warn("read dump file get error: ", err)
Review comment:
@yongboy
So you suggest to prepare the dump file before staring APISIX to avoid the
unexpected event (file not found)?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
[GitHub] [apisix] spacewander commented on a change in pull request #3848: feat: add dump for consul_kv
spacewander commented on a change in pull request #3848:
URL: https://github.com/apache/apisix/pull/3848#discussion_r596718720
##
File path: apisix/discovery/consul_kv.lua
##
@@ -230,6 +241,73 @@ local function update_application(server_name_prefix, data)
end
+local function read_dump_srvs()
+local data, err = util.read_file(dump_params.path)
+if not data then
+log.warn("read dump file get error: ", err)
+return
+end
+
+core.log.info("read dump file: ", data)
+data = util.trim(data)
+if #data == 0 then
+core.log.error("dump file is empty")
+return
+end
+
+local entity, err = core.json.decode(data)
+if err then
+core.log.error("decoded dump data got error: ", err, ", file content:
", data)
+return
+end
+
+if not entity.services or not entity.last_update then
+core.log.error("decoded dump data miss fields, file content: ", data)
+return
+end
+
+local now_time = ngx.time()
+core.log.info("dump file last_update: ", entity.last_update, ",
dump_params.expire: ",
+dump_params.expire, ", now_time: ", now_time)
+if dump_params.expire ~= 0 and (entity.last_update + dump_params.expire)
< now_time then
+ core.log.warn("dump file: ", dump_params.path,
+ " had expired, ignored it")
+return
+end
+
+applications = entity.services
+core.log.info("load dump file into memory success")
+end
+
+
+local function write_dump_srvs()
+local entity = {
+services = applications,
+last_update = ngx.time(),
+expire = dump_params.expire, -- later need handle it
+}
+local data = core.json.encode(entity)
+local succ, err = util.write_file(dump_params.path, data)
+if not succ then
+core.log.error("write dump into file got error: ", err)
+end
+end
+
+
+local function show_dump_file()
+if not dump_params then
+return 503, "dump params is nil"
+end
+
+local data, err = util.read_file(dump_params.path)
+if not data then
+return 500, err
Review comment:
Should we use 503 for this too?
##
File path: apisix/discovery/consul_kv.lua
##
@@ -230,6 +241,73 @@ local function update_application(server_name_prefix, data)
end
+local function read_dump_srvs()
+local data, err = util.read_file(dump_params.path)
+if not data then
+log.warn("read dump file get error: ", err)
Review comment:
If we don't care about the existence of the dump file, maybe we can use
a lower log level when we don't find the file?
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
[GitHub] [apisix] spacewander commented on a change in pull request #3848: feat: add dump for consul_kv
spacewander commented on a change in pull request #3848: URL: https://github.com/apache/apisix/pull/3848#discussion_r596626770 ## File path: docs/en/latest/discovery/consul_kv.md ## @@ -73,6 +76,31 @@ The `keepalive` has two optional values: - `true`, default and recommend value, use the long pull way to query consul servers - `false`, not recommend, it would use the short pull way to query consul servers, then you can set the `fetch_interval` for fetch interval + Dump Data + +When we need reload `apisix` online, as the `consul_kv` module maybe loads data from CONSUL slower than load routes from ETCD, and would get the log at the moment before load successfully from consul: + +``` + http_access_phase(): failed to set upstream: no valid upstream node +``` + +So, we import the `dump` function for `consul_kv` module. When reload, would load the dump file before from consul; when the registered nodes in consul been updated, would dump the upstream nodes into file automatically. + +The `dump` has three optional values now: + +- `path`, the dump file save path +- support relative path, eg: `logs/consul_kv.dump` +- support absolute path, eg: `/tmp/consul_kv.bin` +- make sure the dump file's parent path exist +- make sure the `apisix` has the dump file's read-write access permission,eg: `chown www:root conf/upstream.d/` +- `load_on_init`, default value is `true` +- if `true`, will try load the data from dump file before load data from consul +- if `false`, ignore load data +- and anyhow, we don't need to prepare a dump file for apisix at anytime Review comment: If `load_on_init` is true, we need to prepare a dump file, otherwise a warning will be logged. This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: us...@infra.apache.org
[GitHub] [apisix] spacewander commented on a change in pull request #3848: feat: add dump for consul_kv
spacewander commented on a change in pull request #3848:
URL: https://github.com/apache/apisix/pull/3848#discussion_r596561428
##
File path: docs/en/latest/discovery/consul_kv.md
##
@@ -73,6 +76,31 @@ The `keepalive` has two optional values:
- `true`, default and recommend value, use the long pull way to query consul
servers
- `false`, not recommend, it would use the short pull way to query consul
servers, then you can set the `fetch_interval` for fetch interval
+ Dump Data
+
+When we need reload `apisix` online, as the `consul_kv` module maybe loads
data from CONSUL slower than load routes from ETCD, and would get the log at
the moment before load successfully from consul:
+
+```
+ http_access_phase(): failed to set upstream: no valid upstream node
+```
+
+So, we import the `dump` function for `consul_kv` module. When reload, would
load the dump file before from consul; when the registered nodes in consul been
updated, would dump the upstream nodes into file automatically.
+
+The `dump` has three optional values now:
+
+- `path`, the dump file save path
+- support relative path, eg: `logs/consul_kv.dump`
+- support absolute path, eg: `/tmp/consul_kv.bin`
+- make sure the dump file's parent path exist
+- make sure the `apisix` has the dump file's read-write access
permission,eg: `chown www:root conf/upstream.d/`
+- `load_on_init`, default value is `true`
+- if `true`, will try load the data from dump file before load data from
consul
+- if `false`, ignore load data
+- and anyhow, we don't need to prepare a dump file for apisix at anytime
Review comment:
This sentence confuses me. Does it look like we need to prepare a dump
file if load_on_init is true before starting APISIX?
```
local function read_dump_srvs()
local data, err = util.read_file(dump_params.path)
if not data then
log.warn("read dump file get error: ", err)
return
end
```
##
File path: apisix/discovery/consul_kv.lua
##
@@ -230,6 +241,73 @@ local function update_application(server_name_prefix, data)
end
+local function read_dump_srvs()
+local data, err = util.read_file(dump_params.path)
+if not data then
+log.warn("read dump file get error: ", err)
+return
+end
+
+core.log.info("read dump file: ", data)
+data = util.trim(data)
+if #data == 0 then
+core.log.error("dump file is empty")
+return
+end
+
+local entity, err = core.json.decode(data)
+if err then
+core.log.error("decoded dump data got error: ", err, ", file content:
", data)
+return
+end
+
+if not entity.services or not entity.last_update then
+core.log.error("decoded dump data miss fields, file content: ", data)
+return
+end
+
+local now_time = ngx.time()
+core.log.info("dump file last_update: ", entity.last_update, ",
dump_params.expire: ",
+dump_params.expire, ", now_time: ", now_time)
+if dump_params.expire ~= 0 and (entity.last_update + dump_params.expire)
< now_time then
+ core.log.warn("dump file: ", dump_params.path,
+ " had expired, ignored it")
+return
+end
+
+applications = entity.services
+core.log.info("load dump file into memory success")
+end
+
+
+local function write_dump_srvs()
+local entity = {
+services = applications,
+last_update = ngx.time(),
+expire = dump_params.expire, -- later need handle it
+}
+local data = core.json.encode(entity)
+local succ, err = util.write_file(dump_params.path, data)
+if not succ then
+core.log.error("write dump into file got error: ", err)
+end
+end
+
+
+local function show_dump_file()
+if not dump_params then
+return 500, "dump params is nil"
Review comment:
Personally speaking, I prefer to use 503 as 500 is for unexpected error.
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
[GitHub] [apisix] spacewander commented on a change in pull request #3848: feat: add dump for consul_kv
spacewander commented on a change in pull request #3848:
URL: https://github.com/apache/apisix/pull/3848#discussion_r596500069
##
File path: apisix/discovery/consul_kv.lua
##
@@ -230,6 +240,70 @@ local function update_application(server_name_prefix, data)
end
+local function read_dump_srvs()
+local data, err = util.read_file(dump_params.path)
+if not data then
+log.warn("read dump file get error: ", err)
+return
+end
+
+core.log.info("read dump file: ", data)
+data = util.trim(data)
+if #data == 0 then
+core.log.error("dump file is empty")
+return
+end
+
+local entity, err = core.json.decode(data)
+if err then
+core.log.error("decoded dump data got error: ", err, ", file content:
", data)
+return
+end
+
+if not entity.services or not entity.last_update then
+core.log.error("decoded dump data miss fields, file content: ", data)
+return
+end
+
+if dump_params.expire ~= 0 and (entity.last_update + dump_params.expire)
< ngx.time() then
+ core.log.warn("dump file: ", dump_params.path,
+ " had expired, ignored it")
+return
+end
+
+applications = entity.services
+core.log.info("load dump file into memory success")
+end
+
+
+local function write_dump_srvs()
+local entity = {
+services = applications,
+last_update = ngx.time(),
+expire = dump_params.expire, -- later need handle it
+}
+local data = core.json.encode(entity)
+local succ, err = util.write_file(dump_params.path, data)
+if not succ then
+core.log.error("write dump into file got error: ", err)
+end
+end
+
+
+local function show_dump_file()
+if not dump_params then
+return 200, "dump params is nil"
Review comment:
> Add Test for show_dump_file function when get an error ?
Yes
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
[GitHub] [apisix] spacewander commented on a change in pull request #3848: feat: add dump for consul_kv
spacewander commented on a change in pull request #3848:
URL: https://github.com/apache/apisix/pull/3848#discussion_r595868872
##
File path: apisix/discovery/consul_kv.lua
##
@@ -319,7 +397,7 @@ local function format_consul_params(consul_conf)
if scheme ~= "http" then
return nil, "only support consul http schema address, eg:
http://address:port;
elseif path ~= "/" or core.string.has_suffix(v, '/') then
-return nil, "invalid consul server address, the valid format:
http://address:port;
+return nil, "invald consul server address, the valid format:
http://address:port;
Review comment:
Should be `invalid`
##
File path: apisix/discovery/consul_kv.lua
##
@@ -356,6 +434,20 @@ function _M.init_worker()
return
end
+if consul_conf.dump then
+ local dump = consul_conf.dump
+ if not dump.path or #dump.path == 0 then
Review comment:
We can use `required` & `minLength` to validate the path in the schema.
##
File path: apisix/control/router.lua
##
@@ -31,6 +31,41 @@ local get_method = ngx.req.get_method
local _M = {}
+local function format_dismod_uri(mod_name, uri)
+if core.string.has_prefix(uri, "/v1/") then
+return uri
+end
+
+local tmp = {"/v1/discovery/", mod_name}
+if not core.string.has_prefix(uri, "/") then
+core.table.insert(tmp, "/")
+end
+core.table.insert(tmp, uri)
+
+return core.table.concat(tmp, "")
+end
+
+-- we do not hardcode the discovery module's control api uri
+local function format_dismod_controlapi_uris(mod_name, api_route)
Review comment:
Better to use `format_dismod_control_api_uris`
##
File path: t/discovery/consul_kv_dump.t
##
@@ -0,0 +1,346 @@
+#
+# Licensed to the Apache Software Foundation (ASF) under one or more
+# contributor license agreements. See the NOTICE file distributed with
+# this work for additional information regarding copyright ownership.
+# The ASF licenses this file to You under the Apache License, Version 2.0
+# (the "License"); you may not use this file except in compliance with
+# the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing, software
+# distributed under the License is distributed on an "AS IS" BASIS,
+# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+# See the License for the specific language governing permissions and
+# limitations under the License.
+#
+use t::APISIX 'no_plan';
+
+repeat_each(1);
+no_long_string();
+no_root_location();
+no_shuffle();
+log_level("info");
+
+
+add_block_preprocessor(sub {
+my ($block) = @_;
+
+my $http_config = $block->http_config // <<_EOC_;
+
+server {
+listen 30511;
+
+location /hello {
+content_by_lua_block {
+ngx.say("server 1")
+}
+}
+}
+_EOC_
+
+$block->set_value("http_config", $http_config);
+});
+
+our $yaml_config = <<_EOC_;
+apisix:
+ node_listen: 1984
+ config_center: yaml
+ enable_admin: false
+ enable_control: true
+
+discovery:
+ consul_kv:
+servers:
+ - "http://127.0.0.1:8500;
+dump:
+ path: "consul_kv.dump"
+ load_on_init: true
+_EOC_
+
+
+run_tests();
+
+__DATA__
+
+=== TEST 1: prepare nodes
+--- config
+location /v1/kv {
+proxy_pass http://127.0.0.1:8500;
+}
+--- request eval
+[
+"DELETE /v1/kv/upstreams/?recurse=true",
+"PUT /v1/kv/upstreams/webpages/127.0.0.1:30511\n" . "{\"weight\": 1,
\"max_fails\": 1, \"fail_timeout\": 1}",
+]
+--- response_body eval
+[
+'true',
+'true',
+'true',
+]
+
+
+
+=== TEST 2: show dump services
+--- yaml_config eval: $::yaml_config
+--- config
+location /t {
+content_by_lua_block {
+local json = require("toolkit.json")
+local t = require("lib.test_admin")
+ngx.sleep(2)
+
+local code, body, res =
t.test('/v1/discovery/consul_kv/show_dump_file',
+ngx.HTTP_GET)
+local entity = json.decode(res)
+ngx.say(json.encode(entity.services))
+}
+}
+--- timeout: 12
Review comment:
Do we need so long timeout?
##
File path: docs/en/latest/discovery/consul_kv.md
##
@@ -73,6 +76,30 @@ The `keepalive` has two optional values:
- `true`, default and recommend value, use the long pull way to query consul
servers
- `false`, not recommend, it would use the short pull way to query consul
servers, then you can set the `fetch_interval` for fetch interval
+ Dump Data
+
+When we need reload `apisix` online, as the `consul_kv` module maybe loads
data from CONSUL slower than load routes from ETCD, and would get the log at
the moment before load successfully from consul:
+
+```
+ http_access_phase(): failed to set upstream: no valid upstream node
+```
+
+So,
