Skip to content

feat: enable etcd health-check#4191

Merged
spacewander merged 27 commits into
apache:masterfrom
Yiyiyimu:fix/etcd-retry
Jun 30, 2021
Merged

feat: enable etcd health-check#4191
spacewander merged 27 commits into
apache:masterfrom
Yiyiyimu:fix/etcd-retry

Conversation

@Yiyiyimu

@Yiyiyimu Yiyiyimu commented May 6, 2021 •
edited by spacewander
Loading

Copy link
Copy Markdown
Member

What this PR does / why we need it:

fix #3673
fix #3937.

Import health check of lua-resty-etcd, so

  1. when one etcd node disconnected, apisix would try to connect another one and perform as normal, while health-check would continue print warning log about disconnected etcd node
  2. when all etcd nodes disconnected, apisix would try to re-connect with etcd, with a cyclic backoff interval for 2s, 2s, 4s, ..., 1024s, 2s, 4s ... so the error log would not been flushed when etcd are all down.

TODO:

  • for etcd node deployed in kubernetes, since nodes are behind the same domain, we could not know if one of them is taint, as talked in [discuss]: enable etcd health check #3692
  • add test, but since the current health check does not work for kubernetes, need to implement a new CI pipeline for multiple etcd nodes

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? If it is not backward compatible, please discuss on the mailing list first

@Yiyiyimu Yiyiyimu changed the title fix: enable etcd health-check, so one etcd node failure would not make apisix fail fix: enable etcd health-check May 10, 2021
@Yiyiyimu
Yiyiyimu marked this pull request as ready for review May 10, 2021 02:04
@juzhiyuan

juzhiyuan commented May 10, 2021

Copy link
Copy Markdown
Member

image

We need another fix hh

@Yiyiyimu

Yiyiyimu commented May 10, 2021 •
edited
Loading

Copy link
Copy Markdown
Member Author

Things get a bit weird right now 😕

Before I work on this fix, I test that if killing one node out of an etcd cluster, apisix would fail, as those related issues talk about. However, right now when I test this scenario again on the master branch, apisix would not be affected by the closed etcd node and runs normally. I checked the recent commits and it seems no changes have applied to this problem.

Could someone help me to reproduce the program again

Test running apisix and etcd in Kubernetes, delete one etcd node, and apisix runs normally with master branch. So don't know if anything went wrong yet.

@Yiyiyimu

Yiyiyimu commented May 10, 2021

Copy link
Copy Markdown
Member Author

Waiting for lua-resty-etcd new release: api7/lua-resty-etcd#129

@Yiyiyimu
Yiyiyimu requested a review from spacewander May 10, 2021 21:34
spacewander
spacewander reviewed May 11, 2021
View reviewed changes
Comment thread apisix/core/config_etcd.lua
Comment thread apisix/core/config_etcd.lua Outdated
Comment thread apisix/core/config_etcd.lua Outdated
Comment thread apisix/core/config_etcd.lua Outdated
@Yiyiyimu Yiyiyimu mentioned this pull request May 17, 2021
Closed
10 tasks
tokers
tokers reviewed Jun 4, 2021
View reviewed changes
Comment thread apisix/core/config_etcd.lua Outdated
spacewander
spacewander reviewed Jun 4, 2021
View reviewed changes
Comment thread apisix/core/config_etcd.lua Outdated
Comment thread apisix/core/config_etcd.lua
@spacewander spacewander mentioned this pull request Jun 11, 2021
Closed
@Yiyiyimu

Yiyiyimu commented Jun 21, 2021 •
edited
Loading

Copy link
Copy Markdown
Member Author

Waiting for api7/lua-resty-etcd#131

Done

spacewander
spacewander reviewed Jun 25, 2021
View reviewed changes
Comment thread apisix/core/config_etcd.lua Outdated
@Yiyiyimu
fix test
bb577f6 
Signed-off-by: yiyiyimu <[email protected]>
@spacewander spacewander changed the title fix: enable etcd health-check feat: enable etcd health-check Jun 29, 2021
tokers
tokers reviewed Jun 29, 2021
View reviewed changes
Comment thread apisix/core/config_etcd.lua Outdated
@Yiyiyimu
remove loop increment
043eafc 
Signed-off-by: yiyiyimu <[email protected]>
tokers
tokers reviewed Jun 29, 2021
View reviewed changes
Comment thread apisix/core/config_etcd.lua Outdated
if string.find(err, err_etcd_unhealthy_all) then
local reconnected = false
while err and not reconnected do
local backoff_duration, backoff_factor, backoff_step = 1, 2, 10

@tokers tokers Jun 29, 2021

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Will step 10 too large? As 1024 seconds is too long.

@Yiyiyimu Yiyiyimu Jun 29, 2021 •
edited
Loading

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not so familiar with the production environment. Do you have some recommendations, like 8, around 4 mins at most? @tokers

@tokers tokers Jun 30, 2021

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe 6 is enough, also, since we are in a timer, keep a Nginx timer living for a long while is not a good practice as it might cause the memory leaky.

@Yiyiyimu Yiyiyimu Jun 30, 2021

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for suggestion!
Changed to use outside counter till 32 to avoid keeping nginx timer too long. PTAL @tokers

@spacewander
spacewander merged commit 994f020 into apache:master Jun 30, 2021
@Yiyiyimu Yiyiyimu linked an issue Jun 30, 2021 that may be closed by this pull request
bug: etcd cluster setting,one node down,cannot connect to other healthy one #4414
Closed
@Yiyiyimu Yiyiyimu removed a link to an issue Jun 30, 2021
bug: etcd cluster setting,one node down,cannot connect to other healthy one #4414
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@spacewander spacewander spacewander approved these changes
@tokers tokers tokers approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

4 participants

@Yiyiyimu @juzhiyuan @spacewander @tokers