postgres-operator is restarting pods while not necessary

[stang]

Currently, when adding or removing .spec.patroni.slots, it seem that the way postgres-operator is handling it is by updating the top-level StatefulSet with the corresponding SPILO_CONFIGURATION environment variable in its container's definition, and let spilo handle the management of the slots.

This is systematically restarting the postgres pod.

I believe this restart can be avoided if the postgres-operator is directly updating the configuration via the patroni API (assuming the API covers the same actions as we can manually do with patronictl) and reflect the changes in the DCS so they're persisted for later usage.

This would spare some precious downtime for users who are deploying clusters with one single instance.

---

UPDATE: actually, it seem that despite the pod restart, the configuration is not even applied (see issue #447).
Which makes it an even better reason to not reflect this change in the SPILO_CONFIGURATION?

Or should this be logged as an issue on the spilo project and the operator simply relies on it as some kind of black box? In which case, it keeps things simpler on this end, at the expense of optimal behavior for some edge cases.

[stang]

I've started to work on this one - I'll need some guidance tho.

I think the place to tweak the logic would be here: https://github.com/zalando/postgres-operator/blob/master/pkg/cluster/k8sres.go#L220-L222

Idea would be (pseudo code):

if clusterAlreadyBootstrapped then {
  config.Bootstrap.DCS.Slots = existingValuesForSlots
} else {
  config.Bootstrap.DCS.Slots = patroni.Slots
}

I'm not sure how I can get the existingValuesForSlots.

My thoughts:

• directly from the DCS (but it seem that this notion of DCS is managed in Spilo and we probably want to avoid too much inter-repo dependencies)
• from c.Statefulset.Spec.Template.Spec.Containers[0].env or something similar, but it would be a string to re-parse?̃

--
EDIT: My colleague just pointed out that this can be fetched from the patroni REST API.

[stang]

I've been working on this and figured out that my initial logic was wrong. Instead of the existingValuesForSlots, I'd need to get these values from the initial patroni config (the one used to bootstrap the cluster and that have been used to generate the SPILO_CONFIGURATION of the first StatefulSet).

If not: it would also restart the pods after the second time the slots are changed.

I didn't find a way to get the slotsFromInitialPatroniConfig with the information we already have, so I'm thinking of saving the initialPatroniConfig as an annotation of the StatefulSet. Any objection? I also though about saving it into the Endpoints object as Spilo seem to do it, but it seem that so far, postgres-operator is completely independent so it would be nice to keep it this way.

Also, after second thoughts: I suspect that all the settings that are falling under config.Bootstrap in this function should be handled the same way as for the Slots (cf: being set in the SPILO_CONFIGURATION only when the cluster is created, and then, being updated via the patroni REST API only).

[stang]

cc @Jan-M @CyberDem0n

[Jan-M]

this shows that there is some mix of responsibility and state between operator, spilo, patroni and statefulset that we do not have correctly covered.

We will have a discussion if we can make this right. A lot of the config present in the statefulset may be at the wrong place, as pointed out it does neither get applied on change nor is current.

[Jan-M]

Statefulset should be limited to "postgres version"?

Bootstrap options and intial Patroni config could be written straight to endpoint config object.

We hide/add a lot of defaults from Spilo though that need to be reflected/merged.

[kustodian]

Any progress on this issue?

[FxKu]

With #1103 merged, the will only be a rolling updated if there are changes in DCS struct under Patroni. #1535 enabled reflecting changes in Patroni struct such as slots in the Postgres config.