-
Notifications
You must be signed in to change notification settings - Fork 535
Integrate Load Balancer Health Probes into monitoring #2298
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
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
c7dfd8b to
86ff58b
Compare
Contributor
Author
|
Instead of spending time on #2658, I wanted to move this. |
e34a2cf to
b3f47ad
Compare
b3f47ad to
cbfe758
Compare
cbfe758 to
9609d7d
Compare
9609d7d to
b71b516
Compare
byucesoy
approved these changes
Jan 29, 2025
Member
byucesoy
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good IMO apart from few minor comments.
583873e to
6f55d98
Compare
We are moving the health checks into load_balancers_vms. The main reason these are not handled as part of VM monitoring is because the VM might be up and running but the load balancer makes a decision based on the application state. It is safer to keep this separate. This code will also interract with load balancer and keeping a code path as part of a VM that is not part of a load balancer doesn't seem clever. We are making a couple of logic changes here. 1. The state of the load balancer node is only updated when the monitor is sure their state is changed. This way, we don't have to look into counters at the time of update. 2. IPv4 and IPv6 availability is handled properly. Before, these were kind of mixed up and causing some logical errors. Now, they are distinctly checked and properly declared. This commit itself doesn't simply stop the health probe strands. We will need to handle their migration manually by destroying the strands.
With this commit, we are removing the prog and completely switching to monitor for health probes.
We are removing the logic that checks the counter in inverence endpoint availability check. The main reason is, now, if the state of the load_balancers_vms is switched to "down" we are already sure that the counter hit to the threshold.
6f55d98 to
92bf7df
Compare
bsatzger
reviewed
Jan 29, 2025
bsatzger
reviewed
Jan 29, 2025
The primary key is changed to id, state_counter is not needed and id cannot be null anymore.
92bf7df to
db74193
Compare
bsatzger
approved these changes
Jan 29, 2025
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Add id column to loadbalancers_vms
Prepare load_balancers_vms models for health probes
We are moving the health checks into load_balancers_vms. The main reason
these are not handled as part of VM monitoring is because the VM might
be up and running but the load balancer makes a decision based on the
application state. It is safer to keep this separate. This code will
also interract with load balancer and keeping a code path as part of a
VM that is not part of a load balancer doesn't seem clever. We are
making a couple of logic changes here.
is sure their state is changed. This way, we don't have to look into
counters at the time of update.
kind of mixed up and causing some logical errors. Now, they are
distinctly checked and properly declared.
This commit itself doesn't simply stop the health probe strands. We will
need to handle their migration manually by destroying the strands.
Switch using monitor and remove health probes prog
With this commit, we are removing the prog and completely switching to
monitor for health probes.
Inference endpoint doesn't use the counter anymore
We are removing the logic that checks the counter in inverence endpoint
availability check. The main reason is, now, if the state of the
load_balancers_vms is switched to "down" we are already sure that the
counter hit to the threshold.
Remove state_counter and update keys in load_balancers_vms
The primary key is changed to id, state_counter is not needed and id
cannot be null anymore.