Introduce higher and lower bounds for the number of instances #178
Conversation
Reduce the number of instances to the min_instances if it is lower and to the max_instances if it is higher. -1 for either of those means there is no lower or upper bound. In addition, terminate the operator when there is a nonsense in the configuration (i.e. max_instances < min_instances).
|
👍 |
pkg/util/config/config.go
Outdated
| err = fmt.Errorf("number of works should be higher than 0") | ||
| } | ||
| return | ||
| } |
There was a problem hiding this comment.
Nothing really wrong with this, but I'm curious: why check all errors but only panic on the last one? (not really important as there are just two, but could this grow in the future? it would probably be most helpful to log all of them and then panic)
There was a problem hiding this comment.
Yes, didn't care much about that for now (also, there are many other parameters that could be validated), just picked up a low-hanging fruit in the config to validate, since we had no validation before altogether.
|
👍 |
pkg/cluster/k8sres.go
Outdated
| @@ -464,14 +464,16 @@ func (c *Cluster) generateStatefulSet(spec *spec.PostgresSpec) (*v1beta1.Statefu | |||
| return nil, fmt.Errorf("could not generate volume claim template: %v", err) | |||
| } | |||
|
|
|||
| numberOfInstaces := c.getNumberOfInstances(spec) | |||
| @@ -33,6 +34,8 @@ type Resources struct { | |||
| PodEnvironmentConfigMap string `name:"pod_environment_configmap" default:""` | |||
| NodeEOLLabel map[string]string `name:"node_eol_label" default:"lifecycle-status:pending-decommission"` | |||
| NodeReadinessLabel map[string]string `name:"node_readiness_label" default:"lifecycle-status:ready"` | |||
| MaxInstances int32 `name:"max_instances" default:"-1"` | |||
There was a problem hiding this comment.
shall we comment here on the special meaning of -1 ?
There was a problem hiding this comment.
That went to the docs.
pkg/util/config/config.go
Outdated
| err = fmt.Errorf("minimum number of instances %d is set higher than the maximum number %d") | ||
| } | ||
| if cfg.Workers == 0 { | ||
| err = fmt.Errorf("number of works should be higher than 0") |
pkg/util/config/config.go
Outdated
| if cfg.MinInstances > 0 && cfg.MaxInstances > 0 && cfg.MinInstances > cfg.MaxInstances { | ||
| err = fmt.Errorf("minimum number of instances %d is set higher than the maximum number %d") | ||
| } | ||
| if cfg.Workers == 0 { |
There was a problem hiding this comment.
cfg.Workers > 0 ? It may be tempting to supply -1 meaning no limit :)
pkg/cluster/k8sres.go
Outdated
| newcur = min | ||
| } | ||
| if newcur != cur { | ||
| c.logger.Infof("adjusted number of instances to %d (min: %d, max: %d)", newcur, min, max) |
There was a problem hiding this comment.
should the log mention the old value as well ?
Fix some typos. Per code review by Jan Mußler and Sergey Dudoladov.
| ### Limiting the number of instances in clusters with `min_instances` and `max_instances` | ||
|
|
||
| As a preventive measure, one can restrict the minimum and the maximum number of instances permitted by each Postgres cluster managed by the operator. | ||
| If either `min_instances` or `max_instances` is set to a non-zero value, the operator may adjust the number of instances specified in the cluster manifest to match either the min or the max boundary. |
There was a problem hiding this comment.
and min_instances or max_instances goes to the operator configmap, right ?
There was a problem hiding this comment.
Yes, but there is no need to mention that, since that is part of the description of the operator parameters.
|
👍 |
1 similar comment
|
👍 |
Reduce the number of instances to the min_instances if it is lower and
to the max_instances if it is higher. -1 for either of those means there
is no lower or upper bound.
In addition, terminate the operator when there is a nonsense in the
configuration (i.e. max_instances < min_instances).