You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@apisix.apache.org by GitBox <gi...@apache.org> on 2020/09/15 09:57:48 UTC
[GitHub] [apisix] tokers opened a new pull request #2233: change: check etcd cluster version when init_etcd
tokers opened a new pull request #2233:
URL: https://github.com/apache/apisix/pull/2233
Signed-off-by: tokers <zc...@gmail.com>
### What this PR does / why we need it:
<!--- Why is this change required? What problem does it solve? -->
<!--- If it fixes an open issue, please link to the issue here. -->
### Pre-submission checklist:
* [ ] Did you explain what problem does this PR solve? Or what new features have been added?
* [ ] Have you added corresponding test cases?
* [ ] Have you modified the corresponding document?
* [ ] Is this PR backward compatible?
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [apisix] moonming commented on pull request #2233: change: check etcd cluster version when init_etcd
Posted by GitBox <gi...@apache.org>.
moonming commented on pull request #2233:
URL: https://github.com/apache/apisix/pull/2233#issuecomment-692677880
the etcd in CI is 3.2, so failed: https://github.com/apache/apisix/pull/2233/checks?check_run_id=1116987808#step:6:288
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [apisix] tokers commented on pull request #2233: change: check etcd cluster version when init_etcd
Posted by GitBox <gi...@apache.org>.
tokers commented on pull request #2233:
URL: https://github.com/apache/apisix/pull/2233#issuecomment-693204656
@membphis Fixed.
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [apisix] tokers commented on pull request #2233: change: check etcd cluster version when init_etcd
Posted by GitBox <gi...@apache.org>.
tokers commented on pull request #2233:
URL: https://github.com/apache/apisix/pull/2233#issuecomment-693127956
@moonming It seems the version selection for etcd is not specified in `./bin/apisix` or other stuffs. Is it under the Github Action's control?
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [apisix] tokers commented on pull request #2233: change: check etcd cluster version when init_etcd
Posted by GitBox <gi...@apache.org>.
tokers commented on pull request #2233:
URL: https://github.com/apache/apisix/pull/2233#issuecomment-693130100
@moonming OK, let me rebase my branch. By the way, i think it's better to check out the CHANGELOG of etcd, AKAIK there are some bugs in 3.x series releases, but i'm not sure the exact version range.
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [apisix] tokers commented on pull request #2233: change: check etcd cluster version when init_etcd
Posted by GitBox <gi...@apache.org>.
tokers commented on pull request #2233:
URL: https://github.com/apache/apisix/pull/2233#issuecomment-693134034
@moonming I found PR #2036 also checked the etcd version, but IMHO the way is wrong.
Firstly, when we ask the version message from etcd, it returns a JSON string like this:
```json
{"etcdserver":"3.5.0-pre","etcdcluster":"3.5.0"}
```
we should check the version of `etcdcluster`, not the endpoint's server version (`etcdserver`) that we requested, the cluster version is the minimal server version of all endpoints in the cluster. So that's the one we need to check.
Secondly, the semantic version check method doesn't consider the `pre release` situation, although it's rare in prod, i think's better to consider 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:
users@infra.apache.org
[GitHub] [apisix] Yiyiyimu commented on pull request #2233: change: check etcd cluster version when init_etcd
Posted by GitBox <gi...@apache.org>.
Yiyiyimu commented on pull request #2233:
URL: https://github.com/apache/apisix/pull/2233#issuecomment-693424234
@tokers Thank you for the fix!
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [apisix] moonming commented on pull request #2233: change: check etcd cluster version when init_etcd
Posted by GitBox <gi...@apache.org>.
moonming commented on pull request #2233:
URL: https://github.com/apache/apisix/pull/2233#issuecomment-693135758
nice! we can fix it in this PR :)
Thanks,
Ming Wen
Twitter: _WenMing
Alex Zhang <no...@github.com> 于2020年9月16日周三 上午10:36写道：
> @moonming <https://github.com/moonming> I found PR #2036
> <https://github.com/apache/apisix/pull/2036> also checked the etcd
> version, but IMHO the way is wrong.
>
> Firstly, when we ask the version message from etcd, it returns a JSON
> string like this:
>
> {"etcdserver":"3.5.0-pre","etcdcluster":"3.5.0"}
>
> we should check the version of etcdcluster, not the endpoint's server
> version (etcdserver) that we requested, the cluster version is the
> minimal server version of all endpoints in the cluster. So that's the one
> we need to check.
>
> Secondly, the semantic version check method doesn't consider the pre
> release situation, although it's rare in prod, i think's better to
> consider it.
>
> —
> You are receiving this because you were mentioned.
> Reply to this email directly, view it on GitHub
> <https://github.com/apache/apisix/pull/2233#issuecomment-693134034>, or
> unsubscribe
> <https://github.com/notifications/unsubscribe-auth/AGJZBK5HWE4ZRBDK3NWH743SGAQERANCNFSM4RM3Y5IA>
> .
>
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [apisix] tokers commented on a change in pull request #2233: change: check etcd cluster version when init_etcd
Posted by GitBox <gi...@apache.org>.
tokers commented on a change in pull request #2233:
URL: https://github.com/apache/apisix/pull/2233#discussion_r489186175
##########
File path: bin/apisix
##########
@@ -885,20 +939,44 @@ local function init_etcd(show_output)
yaml_conf.etcd.host = {yaml_conf.etcd.host}
end
+ local cluster_version
local host_count = #(yaml_conf.etcd.host)
-
- local etcd_ok = false
+ -- check the etcd cluster version
for index, host in ipairs(yaml_conf.etcd.host) do
- -- check if etcd version above 3.4
- cmd = "curl " .. host .. "/version 2>&1"
- local res = excute_cmd(cmd)
- local op_ver = str_split(res, "\"")[4]
- local need_ver = "3.4.0"
- if not check_version(op_ver, need_ver) then
- io.stderr:write("etcd version must >=", need_ver, " current ", op_ver, "\n")
+ uri = host .. "/version"
+ local cmd = string.format("curl -s -m %d %s | sed -e 's/[{}]/''/g'", timeout * 2, uri)
+ local res = execute_cmd(cmd)
+ local m = split(res, [[%s*,%s*]])
Review comment:
That'd be better.
##########
File path: bin/apisix
##########
@@ -885,20 +939,44 @@ local function init_etcd(show_output)
yaml_conf.etcd.host = {yaml_conf.etcd.host}
end
+ local cluster_version
local host_count = #(yaml_conf.etcd.host)
-
- local etcd_ok = false
+ -- check the etcd cluster version
for index, host in ipairs(yaml_conf.etcd.host) do
- -- check if etcd version above 3.4
- cmd = "curl " .. host .. "/version 2>&1"
- local res = excute_cmd(cmd)
- local op_ver = str_split(res, "\"")[4]
- local need_ver = "3.4.0"
- if not check_version(op_ver, need_ver) then
- io.stderr:write("etcd version must >=", need_ver, " current ", op_ver, "\n")
+ uri = host .. "/version"
+ local cmd = string.format("curl -s -m %d %s | sed -e 's/[{}]/''/g'", timeout * 2, uri)
Review comment:
OK, will use jkjson.
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [apisix] tokers commented on a change in pull request #2233: change: check etcd cluster version when init_etcd
Posted by GitBox <gi...@apache.org>.
tokers commented on a change in pull request #2233:
URL: https://github.com/apache/apisix/pull/2233#discussion_r489186316
##########
File path: bin/apisix
##########
@@ -885,20 +939,44 @@ local function init_etcd(show_output)
yaml_conf.etcd.host = {yaml_conf.etcd.host}
end
+ local cluster_version
local host_count = #(yaml_conf.etcd.host)
-
- local etcd_ok = false
+ -- check the etcd cluster version
for index, host in ipairs(yaml_conf.etcd.host) do
- -- check if etcd version above 3.4
- cmd = "curl " .. host .. "/version 2>&1"
- local res = excute_cmd(cmd)
- local op_ver = str_split(res, "\"")[4]
- local need_ver = "3.4.0"
- if not check_version(op_ver, need_ver) then
- io.stderr:write("etcd version must >=", need_ver, " current ", op_ver, "\n")
+ uri = host .. "/version"
+ local cmd = string.format("curl -s -m %d %s | sed -e 's/[{}]/''/g'", timeout * 2, uri)
+ local res = execute_cmd(cmd)
+ local m = split(res, [[%s*,%s*]])
+ local errmsg = string.format("got malformed version message: \"%s\" from etcd", res)
+
+ for i = 1, #m do
+ local mm = split(m[i], [[%s*:%s*]])
+ if #mm ~= 2 then
+ io.stderr:write(errmsg)
+ return
+ end
+
+ if mm[1] == "\"etcdcluster\"" then
+ cluster_version = string.sub(mm[2], 2, -2)
+ break
+ end
+ end
+
+ if not cluster_version then
+ io.stderr:write(errmsg)
return
end
+ if compare_semantic_version(cluster_version, min_etcd_version) then
+ io.stderr:write(string.format("etcd cluster version \"%s\" is less than the required version \"%s\", please upgrade your etcd cluster", cluster_version, min_etcd_version))
Review comment:
OK, will split this line.
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [apisix] membphis commented on a change in pull request #2233: change: check etcd cluster version when init_etcd
Posted by GitBox <gi...@apache.org>.
membphis commented on a change in pull request #2233:
URL: https://github.com/apache/apisix/pull/2233#discussion_r489165395
##########
File path: bin/apisix
##########
@@ -885,20 +939,44 @@ local function init_etcd(show_output)
yaml_conf.etcd.host = {yaml_conf.etcd.host}
end
+ local cluster_version
local host_count = #(yaml_conf.etcd.host)
-
- local etcd_ok = false
+ -- check the etcd cluster version
for index, host in ipairs(yaml_conf.etcd.host) do
- -- check if etcd version above 3.4
- cmd = "curl " .. host .. "/version 2>&1"
- local res = excute_cmd(cmd)
- local op_ver = str_split(res, "\"")[4]
- local need_ver = "3.4.0"
- if not check_version(op_ver, need_ver) then
- io.stderr:write("etcd version must >=", need_ver, " current ", op_ver, "\n")
+ uri = host .. "/version"
+ local cmd = string.format("curl -s -m %d %s | sed -e 's/[{}]/''/g'", timeout * 2, uri)
Review comment:
it depends on the tool `sed`. External tools, the less dependency the better.
##########
File path: bin/apisix
##########
@@ -885,20 +939,44 @@ local function init_etcd(show_output)
yaml_conf.etcd.host = {yaml_conf.etcd.host}
end
+ local cluster_version
local host_count = #(yaml_conf.etcd.host)
-
- local etcd_ok = false
+ -- check the etcd cluster version
for index, host in ipairs(yaml_conf.etcd.host) do
- -- check if etcd version above 3.4
- cmd = "curl " .. host .. "/version 2>&1"
- local res = excute_cmd(cmd)
- local op_ver = str_split(res, "\"")[4]
- local need_ver = "3.4.0"
- if not check_version(op_ver, need_ver) then
- io.stderr:write("etcd version must >=", need_ver, " current ", op_ver, "\n")
+ uri = host .. "/version"
+ local cmd = string.format("curl -s -m %d %s | sed -e 's/[{}]/''/g'", timeout * 2, uri)
+ local res = execute_cmd(cmd)
+ local m = split(res, [[%s*,%s*]])
+ local errmsg = string.format("got malformed version message: \"%s\" from etcd", res)
+
+ for i = 1, #m do
+ local mm = split(m[i], [[%s*:%s*]])
+ if #mm ~= 2 then
+ io.stderr:write(errmsg)
+ return
+ end
+
+ if mm[1] == "\"etcdcluster\"" then
+ cluster_version = string.sub(mm[2], 2, -2)
+ break
+ end
+ end
+
+ if not cluster_version then
+ io.stderr:write(errmsg)
return
end
+ if compare_semantic_version(cluster_version, min_etcd_version) then
+ io.stderr:write(string.format("etcd cluster version \"%s\" is less than the required version \"%s\", please upgrade your etcd cluster", cluster_version, min_etcd_version))
Review comment:
this line is too long. it is not easy for reading
##########
File path: bin/apisix
##########
@@ -885,20 +939,44 @@ local function init_etcd(show_output)
yaml_conf.etcd.host = {yaml_conf.etcd.host}
end
+ local cluster_version
local host_count = #(yaml_conf.etcd.host)
-
- local etcd_ok = false
+ -- check the etcd cluster version
for index, host in ipairs(yaml_conf.etcd.host) do
- -- check if etcd version above 3.4
- cmd = "curl " .. host .. "/version 2>&1"
- local res = excute_cmd(cmd)
- local op_ver = str_split(res, "\"")[4]
- local need_ver = "3.4.0"
- if not check_version(op_ver, need_ver) then
- io.stderr:write("etcd version must >=", need_ver, " current ", op_ver, "\n")
+ uri = host .. "/version"
+ local cmd = string.format("curl -s -m %d %s | sed -e 's/[{}]/''/g'", timeout * 2, uri)
+ local res = execute_cmd(cmd)
+ local m = split(res, [[%s*,%s*]])
Review comment:
I think we can use `dkjson` the parse the JSON response body, it was written by pure Lua code.
https://luarocks.org/modules/dhkolf/dkjson
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [apisix] moonming merged pull request #2233: change: check etcd cluster version when init_etcd
Posted by GitBox <gi...@apache.org>.
moonming merged pull request #2233:
URL: https://github.com/apache/apisix/pull/2233
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [apisix] moonming commented on pull request #2233: change: check etcd cluster version when init_etcd
Posted by GitBox <gi...@apache.org>.
moonming commented on pull request #2233:
URL: https://github.com/apache/apisix/pull/2233#issuecomment-693129140
we used to be the default version installed by github actions, now it is the etcd 3.4 version installed manually in https://github.com/apache/apisix/pull/2036
----------------------------------------------------------------
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:
users@infra.apache.org