Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

change: check etcd cluster version when init_etcd #2233

Merged
merged 4 commits into from
Sep 16, 2020

Conversation

tokers
Copy link
Contributor

@tokers tokers commented Sep 15, 2020

Signed-off-by: tokers zchao1995@gmail.com

What this PR does / why we need it:

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?

@moonming
Copy link
Member

@tokers
Copy link
Contributor Author

tokers commented Sep 16, 2020

@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?

@moonming
Copy link
Member

we used to be the default version installed by github actions, now it is the etcd 3.4 version installed manually in #2036

@tokers
Copy link
Contributor Author

tokers commented Sep 16, 2020

@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.

@tokers
Copy link
Contributor Author

tokers commented Sep 16, 2020

@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:

{"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.

@moonming
Copy link
Member

moonming commented Sep 16, 2020 via email

@tokers tokers force-pushed the change/etcdcluster_version_check branch from 8f43ad7 to a4cf24a Compare September 16, 2020 02:49
bin/apisix Outdated Show resolved Hide resolved
bin/apisix Outdated Show resolved Hide resolved
bin/apisix Outdated Show resolved Hide resolved
Signed-off-by: tokers <zchao1995@gmail.com>
@tokers tokers force-pushed the change/etcdcluster_version_check branch from a4cf24a to da05a15 Compare September 16, 2020 06:34
@moonming moonming added this to the 2.0 milestone Sep 16, 2020
@tokers
Copy link
Contributor Author

tokers commented Sep 16, 2020

@membphis Fixed.

@moonming moonming merged commit e93cdbd into apache:master Sep 16, 2020
@Yiyiyimu
Copy link
Member

@tokers Thank you for the fix!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants