rename max-concurrent-reconciles-vds to max-concurrent-reconciles #483
Conversation
…bal and apply to all controllers
main.go
Outdated
| @@ -83,8 +83,8 @@ func main() { | |||
| fmt.Sprintf( | |||
| "The type of client cache persistence model that should be employed."+ | |||
| "choices=%v", []string{persistenceModelDirectUnencrypted, persistenceModelDirectEncrypted, persistenceModelNone})) | |||
| flag.IntVar(&vdsOptions.MaxConcurrentReconciles, "max-concurrent-reconciles-vds", 100, | |||
| "Maximum number of concurrent reconciles for the VaultDynamicSecrets controller.") | |||
| flag.IntVar(&controllerOptions.MaxConcurrentReconciles, "max-concurrent-reconciles-global", 100, | |||
There was a problem hiding this comment.
I wonder if we could rename the option to max-concurrent-reconciles, since global seems to imply that we support per controller reconciles. Also, I wonder if we might be breaking anyone who has set max-concurrent-reconciles-vds, should we maybe leave that around as a deprecated option. We could assign its value to the option?
There was a problem hiding this comment.
Sounds good about renaming to max-concurrent-reconciles and applying to all controllers.
Keeping the old flag is maybe a bit hacky since we don't have a way of knowing if the flag was even passed due to the way the flags.Int() function takes a default value. I could set it to 0 by default and ignore it unless it's non-zero I suppose.
At any rate, this would only presumably be a problem for someone who's using Kustomize and had set the flag, and for those who are using the helm chart but pinning old versions of the operator binary. I think that second one is a bit non-standard and questionably supportable?
I guess I wonder if it's a trade-off of documenting it as a breaking change for Kustomize users, and/or making the code a little sloppy? Thoughts?
There was a problem hiding this comment.
Yeah, keeping it around is not the best, but I think it is possible to set it from controller.manager.extraArgs here:
vault-secrets-operator/chart/values.yaml
Line 330 in 5845d8a
There was a problem hiding this comment.
Okay - moved PKI, HCPVS, VDS, VSS to max-concurrent-reconciles, preserving max-concurrent-reconciles-vds which will override max-concurrent-reconciles if supplied.
| // It is mostly here to allow for backward compatibility from when we introduced the flag | ||
| // `--max-concurrent-reconciles`. | ||
| vdsOverrideOpts := controller.Options{} | ||
| if vdsOptions.MaxConcurrentReconciles != defaultVaultDynamicSecretsConcurrency { |
There was a problem hiding this comment.
Can we log a deprecation warning if vdsOptions.MaxConcurrentReconciles > 0?
| @@ -83,8 +91,10 @@ func main() { | |||
| fmt.Sprintf( | |||
| "The type of client cache persistence model that should be employed."+ | |||
| "choices=%v", []string{persistenceModelDirectUnencrypted, persistenceModelDirectEncrypted, persistenceModelNone})) | |||
| flag.IntVar(&vdsOptions.MaxConcurrentReconciles, "max-concurrent-reconciles-vds", 100, | |||
| flag.IntVar(&vdsOptions.MaxConcurrentReconciles, "max-concurrent-reconciles-vds", defaultVaultDynamicSecretsConcurrency, | |||
There was a problem hiding this comment.
Can we document the deprecation here, pointing the user at the option?
…shicorp#483) * deprecate max-concurrent-reconciles-vds manager flag and add max-concurrent-reconciles applying it to all controllers, replacing the existing behaviour with the new flag in the helm chart. * Update helm values and add an Info log to note flag deprecation
Renames the controller manager flag
--max-concurrent-reconciles-vdsto--max-concurrent-reconcilesand applies it to VPS, VSS, VHCPSA and VDS controllers.This has the effect of enabling the all Syncable Secret controllers to process in parallel which we've tested at scale successfully and was causing scaling issues in the current architecture.
The original flag
--max-concurrent-reconciles-vdsis preserved for backward compatibility and will override the new flag for the VDS controller only.The helm
values.yamlis updated to reflect the new behaviour ofcontroller.manager.maxConcurrentReconciles.