-
Notifications
You must be signed in to change notification settings - Fork 112
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
Allow to configure MaxConcurrentReconciles for our controllers #623
Allow to configure MaxConcurrentReconciles for our controllers #623
Conversation
Hm, the sanity check failed in files I did not change. Correcting this in a separate commit. |
060e617
to
cce44a2
Compare
Hi Sascha, Please fix the code conflict. |
cce44a2
to
fddc792
Compare
/approve @zhangtbj feel free to put a slash lgtm in this PR if you happy with @SaschaSchwarze0 's responses to your comments and we can merge this one fwiw LGTM |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: gabemontero The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
pkg/config/config.go
Outdated
Controllers: Controllers{ | ||
Build: ControllerOptions{ | ||
MaxConcurrentReconciles: 1, | ||
}, | ||
BuildRun: ControllerOptions{ | ||
MaxConcurrentReconciles: 1, | ||
}, | ||
BuildStrategy: ControllerOptions{ | ||
MaxConcurrentReconciles: 1, | ||
}, | ||
ClusterBuildStrategy: ControllerOptions{ | ||
MaxConcurrentReconciles: 1, | ||
}, | ||
}, |
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.
A quick question, is that possible to skip this default setting here? And let framework to set default values for us, like before.
We manually set default to 1 for all, but I am not sure if the framework change the default value in future, then they will be mismatch.
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.
Done.
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.
It looks better to have a check by using zero
.
Thanks Sascha!
fddc792
to
9389ea5
Compare
/lgtm |
Changes
With the Git validation being added to the build reconciliation, we are observing increases in the time it takes to reconcile a build. This time is critical at the startup of the controller when all builds are reconciled. New builds that are getting created will then not be reconciled within a reasonable time. Example:
With 1000 builds in the system that do not use Git validation, we see the controller reconciling all of them in a little more than a minute. With Git validation enabled, it takes more than six minutes.
In this PR, I am exposing the MaxConcurrentReconciles controller setting as environment variable for our four controllers. The default is still 1.
Submitter Checklist
Release Notes