-
Notifications
You must be signed in to change notification settings - Fork 69
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
Make concurrent reconciliation configurable #153
Conversation
Default for both, the ImageRepository and the ImagePolicy controllers is 4 workers. closes fluxcd#148 Signed-off-by: Max Jonas Werner <mail@makk.es>
Looking good, @makkes! One suggestion I have, for consistency's sake, I think it would make sense to define some types named |
Yeah, I saw these in the other controllers and was wondering why these were different. If you don't mind, I'll introduce these types in a subsequent PR to keep this focussed on the reconciliation configurability. What do you think? |
@makkes it's ok to introduce the |
Alright, I'll add a commit shortly. |
This is in alignment with other controllers such as the helm-controller. Signed-off-by: Max Jonas Werner <mail@makk.es>
cdac70a
to
03043e5
Compare
@relu @stefanprodan added option types for both controllers. |
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.
sure thing |
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.
Nice and neat, and consistent with e.g., kustomize controller, which is appreciated :-)
I've suggested an improvement, which you can take or leave ...
@@ -77,6 +78,7 @@ func main() { | |||
"Watch for custom resources in all namespaces, if set to false it will only watch the runtime namespace.") | |||
flag.StringVar(&storagePath, "storage-path", "/data", "Where to store the persistent database of image metadata") | |||
flag.Int64Var(&storageValueLogFileSize, "storage-value-log-file-size", 1<<28, "Set the database's memory mapped value log file size in bytes. Effective memory usage is about two times this size.") | |||
flag.IntVar(&concurrent, "concurrent", 4, "The number of concurrent resource reconciles.") |
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.
I think this flag definition comes from elsewhere; but either way, it deserves a better description, on two counts:
- it's the maximum number of workers
- "reconciles" doesn't tell the user what it affects.
Something like "The maximum number of scans that can run concurrently" would be an improvement, I reckon. WDYT?
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.
Oh it was merged already. OK never mind.
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.
@squaremo I agree. I'll come up with a follow-up PR.
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.
We could extract this into fluxcd/pkg/runtime
like we did with the log level and other common flags. The description used here is the same for all the others controllers, better to change it in one place.
Default for both, the ImageRepository and the ImagePolicy controllers
is 4 workers.
fix: #148
Signed-off-by: Max Jonas Werner mail@makk.es