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

Fixes #18981: Slow computation of dynamic groups on large system #3577

Conversation

ncharles
Copy link
Member

@ncharles ncharles commented Apr 4, 2021

@ncharles ncharles requested a review from fanf April 4, 2021 16:05
Copy link
Member

@fanf fanf left a comment

Choose a reason for hiding this comment

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

Appart for some little enhancement in comments, this seems to be a good PR with limited risk and clear gain. Nice !

}
updateManager ! GroupUpdateMessage.DynamicUpdateResult(processId, modId, startTime, DateTime.now, results.toMap)

Copy link
Member

Choose a reason for hiding this comment

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

There should be a debug-level log message here telling how much parallelism is used for groups without dependencies

@@ -441,6 +445,7 @@ class LDAPBasedConfigService(
rudder.generation.delay=0s
rudder.generation.trigger=all
node.accept.duplicated.hostname=false
rudder.compute.dyngroups.max.parallelism=1
Copy link
Member

Choose a reason for hiding this comment

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

I think that for consistancy sack, it should be like rudder.generation.max.parallelism, ie accepting either an integer like here, or xF, with F a float and x the character x, meaning F time the number of CPU available. The same parsing logic can be used (and can even be factored out in an utility object, which would also ensure to always return an integer at least equals to 1).
And with that, setting it to x0.5 (ie, number CPU / 2) seems to be a good default.

Copy link
Member Author

Choose a reason for hiding this comment

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

it's not really generation, as it is to compute dynamic groups (so also done outside of policy generation)

i'll change the xF

Copy link
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 sure i'd like to change the default to something larger than 1, as it depends on the size of the LDAP connection pool which is 2 by default

@ncharles
Copy link
Member Author

ncharles commented Apr 9, 2021

PR updated with a new commit

val error = (e.fullMsg + s"Error when updating dynamic group '${id.value}'")
logger.error(error)
// e.fullMsg.foreach(ex =>
// logger.error("Exception was:", ex)
Copy link
Member

Choose a reason for hiding this comment

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

this comment should be removed

@ncharles ncharles force-pushed the bug_18981/slow_computation_of_dynamic_groups_on_large_system branch from 4078275 to d65eda3 Compare May 6, 2021 06:42
@ncharles
Copy link
Member Author

ncharles commented May 6, 2021

PR updated with a new commit

@Normation-Quality-Assistant
Copy link
Contributor

This PR is not mergeable to upper versions.
Since it is "Ready for merge" you must merge it by yourself using the following command:
rudder-dev merge https://github.com/Normation/rudder/pull/3577
-- Your faithful QA
Kant merge: "It is beyond a doubt that all our knowledge begins with experience."
(https://ci.normation.com/jenkins/job/merge-accepted-pr/38551/console)

@fanf
Copy link
Member

fanf commented May 6, 2021

OK, squash merging this PR

@fanf fanf force-pushed the bug_18981/slow_computation_of_dynamic_groups_on_large_system branch from f016e6a to 074cb46 Compare May 6, 2021 09:36
@fanf fanf merged commit 074cb46 into Normation:branches/rudder/6.1 May 6, 2021
This pull request was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants