Skip to content

Implement process manager for C in Go #7984

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.

Sign up for GitHub

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

Jump to bottom
Closed
wants to merge 7 commits into from
Closed

Implement process manager for C in Go #7984

wants to merge 7 commits into from

Conversation

Al2Klimov
Copy link
Member

@Al2Klimov Al2Klimov commented Apr 20, 2020 •
edited
Loading

fixes #7600
fixes #8196
closes #7744

@Al2Klimov Al2Klimov added this to the 2.13.0 milestone Apr 20, 2020
@Al2Klimov Al2Klimov self-assigned this Apr 20, 2020
@Al2Klimov
Copy link
Member Author

@N-o-X I like this one more than #7744 as (1) Redis isn't required and (2) it boosts > 2x. And you?

@Al2Klimov Al2Klimov assigned N-o-X and unassigned Al2Klimov Apr 20, 2020
@Al2Klimov Al2Klimov added the needs feedback We'll only proceed once we hear from you again label Apr 20, 2020
@Al2Klimov Al2Klimov force-pushed the bugfix/go-process-manager-7600 branch from 2eee05c to 6600ca8 Compare April 20, 2020 10:27
@Al2Klimov
Copy link
Member Author

bump

@N-o-X
Copy link
Contributor

N-o-X commented Sep 15, 2020 •
edited
Loading

Same here. Less dependencies is better in my opinion. Have you tested this PRs performance yet?

@Al2Klimov Al2Klimov removed the needs feedback We'll only proceed once we hear from you again label Sep 15, 2020
@Al2Klimov
Copy link
Member Author

After merge

@Al2Klimov Al2Klimov mentioned this pull request Sep 16, 2020
Closed
@Al2Klimov Al2Klimov removed their assignment Sep 16, 2020
@Al2Klimov
Copy link
Member Author

Have you tested this PRs performance yet?

Yes: #8196 (comment)

@Al2Klimov
Copy link
Member Author

@lippserd @N-o-X Shall we actually put this into v2.13.0?

@Al2Klimov Al2Klimov added the needs feedback We'll only proceed once we hear from you again label Sep 16, 2020
@N-o-X
Copy link
Contributor

N-o-X commented Sep 16, 2020 •
edited
Loading

The load seems to be a quiet a bit higher with this PR. I think we need a lot of testing and code reviewing before we can decide on this.

Also, even if we decide to put this into 2.13.0, we still need a fix that helps with check latency in 2.12.x.

@Al2Klimov
Copy link
Member Author

The load seems to be a quiet a bit higher with this PR.

Well, 2x checks/time need 2x load.

@lippserd
Copy link
Member

I really like this approach. Could we have some numbers for comparison of Icinga 2 w/ and w/o the Go process manager, i.e. load, latency, checks per second with 1m check interval and 1k hosts, 10k hosts, 100k hosts, 1000k hosts.

@lippserd lippserd assigned Al2Klimov and unassigned lippserd Sep 16, 2020
@N-o-X
Copy link
Contributor

N-o-X commented Sep 16, 2020

Could we have some numbers for comparison of Icinga 2 w/ and w/o the Go process manager, i.e. load, latency, checks per second with 1m check interval and 1k hosts, 10k hosts, 100k hosts, 1000k hosts.

Yes please!

@Al2Klimov
Copy link
Member Author

I'm sorry, but 1m checkables are too much. With our cloud's biggest boxes I could make 250k checkables with the true command. This should be enough as only the fork/exec/waitpid performance differs, but not the one of the check plugins themselves. Take a look at these few hours:

Bildschirmfoto 2020-09-17 um 10 52 52

This PR. Icinga fires almost the requested 250k/m w/ 4ms latency using about 1.0 load per CPU.

Bildschirmfoto 2020-09-17 um 10 52 59

v2.11.5. Doesn't even fire 100k/m and the checks jam blocks the checks for the dashboard, e.g. latency.

Bildschirmfoto 2020-09-17 um 10 53 08

v2.12.0. Directed by Robert B. Weide.

@Al2Klimov Al2Klimov removed their assignment Sep 17, 2020
@Al2Klimov
Copy link
Member Author

@lippserd @N-o-X Shall we maintain/package this Go binary separately?

@Al2Klimov
Copy link
Member Author

Same setup as above with 25k checkables – #7984, v2.11.5, v2.12.0

Bildschirmfoto 2020-09-17 um 14 53 56
Bildschirmfoto 2020-09-17 um 14 54 02
Bildschirmfoto 2020-09-17 um 14 54 09

@Al2Klimov
Copy link
Member Author

#7984

Bildschirmfoto 2020-09-22 um 16 15 05

#8269

Bildschirmfoto 2020-09-22 um 16 15 10

v2.11.5

Bildschirmfoto 2020-09-22 um 16 15 00

v2.12.0

Bildschirmfoto 2020-09-22 um 16 14 55

@N-o-X N-o-X mentioned this pull request Sep 23, 2020
Open
@Al2Klimov Al2Klimov force-pushed the bugfix/go-process-manager-7600 branch from 6600ca8 to 95f5428 Compare September 24, 2020 13:08
@Al2Klimov Al2Klimov force-pushed the bugfix/go-process-manager-7600 branch from 95f5428 to 803107f Compare September 24, 2020 13:20
@julianbrost julianbrost removed this from the 2.13.0 milestone May 31, 2021
@Al2Klimov
Copy link
Member Author

@cla-bot check

@cla-bot cla-bot bot added the cla/signed label Aug 4, 2021
@N-o-X N-o-X removed their assignment Dec 6, 2021
@Al2Klimov Al2Klimov closed this Jun 12, 2023
@icinga-probot icinga-probot bot deleted the bugfix/go-process-manager-7600 branch June 12, 2023 08:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
cla/signed needs feedback We'll only proceed once we hear from you again
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Make the process manager great again High check latency after 2.12 upgrade / memory usage
4 participants
@Al2Klimov @N-o-X @lippserd @julianbrost