Configurable maxConcurrentReconciles and pollInterval with sensible defaults #140
Conversation
…aults Signed-off-by: Hasan Turken <turkenh@gmail.com>
| @@ -37,13 +37,14 @@ func main() { | |||
| var ( | |||
| app = kingpin.New(filepath.Base(os.Args[0]), "AWS support for Crossplane.").DefaultEnvars() | |||
| debug = app.Flag("debug", "Run with debug logging.").Short('d').Bool() | |||
| syncPeriod = app.Flag("sync", "Controller manager sync period such as 300ms, 1.5h, or 2h45m").Short('s').Default("1h").Duration() | |||
| syncInterval = app.Flag("sync", "Sync interval controls how often all resources will be double checked for drift.").Short('s').Default("1h").Duration() | |||
| pollInterval = app.Flag("poll", "Poll interval controls how often an individual resource should be checked for drift.").Default("10m").Duration() | |||
There was a problem hiding this comment.
Why are we increasing the poll interval from 1m to 10m?
There was a problem hiding this comment.
This is the proposed value after my experiments in crossplane/upjet#116 (comment) with the motivation below:
In upjet based controllers a typical reconcile loop tooks longer than a native provider due to underlying implementation. If we go with a polling interval like 1m, we immediately cause the work queue to fill up especially when there are multiple instances of the same kind. I believe it is more reasonable to default to a higher value if a typical reconcile loop takes is in the order of ~10s (i.e. refresh/plan).
| PollInterval: 1 * time.Minute, | ||
| MaxConcurrentReconciles: 1, | ||
| PollInterval: *pollInterval, | ||
| MaxConcurrentReconciles: *maxReconcileRate, |
There was a problem hiding this comment.
Should we have separate parameters for the max reconcile rate (which is a global rate shared between all reconcilers) and the max concurrent reconciles (which is a count of workers consuming the workqueue of a controller)?
There was a problem hiding this comment.
Thanks for the pointer @turkenh. I agree, we had better follow the convention suggested there.
Signed-off-by: Hasan Turken <turkenh@gmail.com>
Description of your changes
This PR makes maxConcurrentReconciles and pollInterval configurable with the proposed defaults experimented here.
It follows the same approach we used in the community providers to use input arguments as internal configurations.
Fixes crossplane/upjet#116
I have:
make reviewable testto ensure this PR is ready for review.How has this code been tested
See crossplane/upjet#116 (comment)