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

fix(balancer) ensure existing healthchecker is stopped when processing target event #7408

Merged
merged 1 commit into from
Jul 8, 2021

Conversation

onematchfox
Copy link
Contributor

@onematchfox onematchfox commented May 31, 2021

Summary

Ensures that existing healthchecks are stopped when processing CRUD events on targets.

Full changelog

  • Ensure stop_healthchecker is called for existing balancer prior to create_balancer. Follows pattern from do_upstream_event

Notes:

  • Haven't added tests for this. Figured I would try copy the existing test from do_upstream_event but it doesn't look like this is tested at present? (Was looking at https://github.com/Kong/kong/blob/2.4.1/spec/01-unit/09-balancer_spec.lua so perhaps Iäm in the wrong place?)
  • Opened PR against master. But it seems like the branching strategy in this repo has changed somewhat since I last submitted a PR. Feel free to let me know if you want this rebased to a (release?) branch.

Issues resolved

Fix #7406
Fix #7407

@onematchfox onematchfox changed the title fix(balancer) ensure healthchecker is stopped on target event fix(balancer) ensure existing healthchecker is stopped when processing target event May 31, 2021
@locao
Copy link
Contributor

locao commented Jun 28, 2021

Hi @onematchfox, thanks for your submission! This change is correct but needs a sister PR (Kong/lua-resty-healthcheck#74) to fix the issue. We'll merge this after releasing a new version of lua-resty-healthcheck and bumping it in Kong.

@onematchfox
Copy link
Contributor Author

Hi @onematchfox, thanks for your submission! This change is correct but needs a sister PR (Kong/lua-resty-healthcheck#74) to fix the issue. We'll merge this after releasing a new version of lua-resty-healthcheck and bumping it in Kong.

👍

Was aware of the additional change that was required and had already opened a PR for this (Kong/lua-resty-healthcheck#71). I hadn't removed worker_events.register_weak(self.ev_callback, self.EVENT_SOURCE) not sure I follow why you'd want to remove the registration of the callback as well but respect your expertise on the matter 😄

@locao
Copy link
Contributor

locao commented Jun 28, 2021

Sorry, I missed that PR, thanks for pointing that out!

About the worker_events.register_weak(self.ev_callback, self.EVENT_SOURCE), I removed it because it was being called twice, by new() and by start() functions.

@locao locao added the pr/ready (but hold merge) No more concerns, but do not merge yet (probably a conflict of interest with another PR or release) label Jun 29, 2021
@locao locao changed the base branch from master to release/2.5.x July 8, 2021 14:23
@locao locao removed the pr/ready (but hold merge) No more concerns, but do not merge yet (probably a conflict of interest with another PR or release) label Jul 8, 2021
@locao locao merged commit e229509 into Kong:release/2.5.x Jul 8, 2021
@onematchfox onematchfox deleted the fix/balancer branch July 8, 2021 21:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants