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

Make concurrent reconciliation configurable #153

Merged
merged 2 commits into from
Jun 24, 2021

Conversation

makkes
Copy link
Member

@makkes makkes commented Jun 23, 2021

Default for both, the ImageRepository and the ImagePolicy controllers
is 4 workers.

fix: #148

Signed-off-by: Max Jonas Werner mail@makk.es

Default for both, the ImageRepository and the ImagePolicy controllers
is 4 workers.

closes fluxcd#148

Signed-off-by: Max Jonas Werner <mail@makk.es>
@relu relu self-requested a review June 23, 2021 15:20
@relu
Copy link
Member

relu commented Jun 23, 2021

Looking good, @makkes!

One suggestion I have, for consistency's sake, I think it would make sense to define some types named ImagePolicyReconcilerOptions and respectively ImageRepositoryReconcilerOptions to hold the reconciliation specific options separately, similar to how it's done in other controllers.
Also see an example of the SetupWithManager implementation.

@makkes
Copy link
Member Author

makkes commented Jun 23, 2021

Looking good, @makkes!

One suggestion I have, for consistency's sake, I think it would make sense to define some types named ImagePolicyReconcilerOptions and respectively ImageRepositoryReconcilerOptions to hold the reconciliation specific options separately

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?

@stefanprodan
Copy link
Member

@makkes it's ok to introduce the Options types in this PR.

@makkes
Copy link
Member Author

makkes commented Jun 24, 2021

@makkes it's ok to introduce the Options types in this PR.

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>
@makkes
Copy link
Member Author

makkes commented Jun 24, 2021

@relu @stefanprodan added option types for both controllers.

Copy link
Member

@stefanprodan stefanprodan left a comment

Choose a reason for hiding this comment

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

LGTM

Thanks @makkes 🥇

PS. Can you please do the same in image-automation-controller?

@stefanprodan stefanprodan merged commit d41b3f1 into fluxcd:main Jun 24, 2021
@makkes makkes deleted the concurrent-reconciles branch June 24, 2021 14:37
@makkes
Copy link
Member Author

makkes commented Jun 24, 2021

PS. Can you please do the same in image-automation-controller?

sure thing

Copy link
Member

@squaremo squaremo left a 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.")
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 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?

Copy link
Member

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.

Copy link
Member Author

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.

Copy link
Member

@stefanprodan stefanprodan Jun 24, 2021

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.

@makkes
Copy link
Member Author

makkes commented Jun 24, 2021

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Repo timeout locks full reconciliation cycle
4 participants