Skip to content

Commit 2a33bf3

Browse files
authored
improve Patroni config sync (zalando#1635)
* improve Patroni config sync * collect new and updated slots to patch patroni * refactor httpGet in Patroni and extend unit tests * GetMemberData should call the patroni endpoint * add PATCH test
1 parent 6dc239a commit 2a33bf3

File tree

6 files changed

+224
-90
lines changed

6 files changed

+224
-90
lines changed

e2e/tests/test_e2e.py

Lines changed: 5 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -1099,19 +1099,19 @@ def test_patroni_config_update(self):
10991099

11001100
def compare_config():
11011101
effective_config = k8s.patroni_rest(masterPod.metadata.name, "config")
1102-
desired_patroni = pg_patch_config["spec"]["patroni"]
1102+
desired_config = pg_patch_config["spec"]["patroni"]
11031103
desired_parameters = pg_patch_config["spec"]["postgresql"]["parameters"]
11041104
effective_parameters = effective_config["postgresql"]["parameters"]
11051105
self.assertEqual(desired_parameters["max_connections"], effective_parameters["max_connections"],
11061106
"max_connections not updated")
11071107
self.assertTrue(effective_config["slots"] is not None, "physical replication slot not added")
1108-
self.assertEqual(desired_patroni["ttl"], effective_config["ttl"],
1108+
self.assertEqual(desired_config["ttl"], effective_config["ttl"],
11091109
"ttl not updated")
1110-
self.assertEqual(desired_patroni["loop_wait"], effective_config["loop_wait"],
1110+
self.assertEqual(desired_config["loop_wait"], effective_config["loop_wait"],
11111111
"loop_wait not updated")
1112-
self.assertEqual(desired_patroni["retry_timeout"], effective_config["retry_timeout"],
1112+
self.assertEqual(desired_config["retry_timeout"], effective_config["retry_timeout"],
11131113
"retry_timeout not updated")
1114-
self.assertEqual(desired_patroni["synchronous_mode"], effective_config["synchronous_mode"],
1114+
self.assertEqual(desired_config["synchronous_mode"], effective_config["synchronous_mode"],
11151115
"synchronous_mode not updated")
11161116
return True
11171117

pkg/cluster/k8sres.go

Lines changed: 8 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -30,13 +30,12 @@ import (
3030
)
3131

3232
const (
33-
pgBinariesLocationTemplate = "/usr/lib/postgresql/%v/bin"
34-
patroniPGBinariesParameterName = "bin_dir"
35-
patroniPGParametersParameterName = "parameters"
36-
patroniPGHBAConfParameterName = "pg_hba"
37-
localHost = "127.0.0.1/32"
38-
connectionPoolerContainer = "connection-pooler"
39-
pgPort = 5432
33+
pgBinariesLocationTemplate = "/usr/lib/postgresql/%v/bin"
34+
patroniPGBinariesParameterName = "bin_dir"
35+
patroniPGHBAConfParameterName = "pg_hba"
36+
localHost = "127.0.0.1/32"
37+
connectionPoolerContainer = "connection-pooler"
38+
pgPort = 5432
4039
)
4140

4241
type pgUser struct {
@@ -277,11 +276,11 @@ PatroniInitDBParams:
277276
local, bootstrap := getLocalAndBoostrapPostgreSQLParameters(pg.Parameters)
278277

279278
if len(local) > 0 {
280-
config.PgLocalConfiguration[patroniPGParametersParameterName] = local
279+
config.PgLocalConfiguration[constants.PatroniPGParametersParameterName] = local
281280
}
282281
if len(bootstrap) > 0 {
283282
config.Bootstrap.DCS.PGBootstrapConfiguration = make(map[string]interface{})
284-
config.Bootstrap.DCS.PGBootstrapConfiguration[patroniPGParametersParameterName] = bootstrap
283+
config.Bootstrap.DCS.PGBootstrapConfiguration[constants.PatroniPGParametersParameterName] = bootstrap
285284
}
286285
}
287286
// Patroni gives us a choice of writing pg_hba.conf to either the bootstrap section or to the local postgresql one.

pkg/cluster/sync.go

Lines changed: 35 additions & 25 deletions
Original file line numberDiff line numberDiff line change
@@ -395,18 +395,24 @@ func (c *Cluster) syncStatefulSet() error {
395395
// get Postgres config, compare with manifest and update via Patroni PATCH endpoint if it differs
396396
// Patroni's config endpoint is just a "proxy" to DCS. It is enough to patch it only once and it doesn't matter which pod is used.
397397
for i, pod := range pods {
398+
emptyPatroniConfig := acidv1.Patroni{}
398399
podName := util.NameFromMeta(pods[i].ObjectMeta)
399-
config, err := c.patroni.GetConfig(&pod)
400+
patroniConfig, pgParameters, err := c.patroni.GetConfig(&pod)
400401
if err != nil {
401402
c.logger.Warningf("could not get Postgres config from pod %s: %v", podName, err)
402403
continue
403404
}
404-
instanceRestartRequired, err = c.checkAndSetGlobalPostgreSQLConfiguration(&pod, config)
405-
if err != nil {
406-
c.logger.Warningf("could not set PostgreSQL configuration options for pod %s: %v", podName, err)
407-
continue
405+
406+
// empty config probably means cluster is not fully initialized yet, e.g. restoring from backup
407+
// do not attempt a restart
408+
if !reflect.DeepEqual(patroniConfig, emptyPatroniConfig) || len(pgParameters) > 0 {
409+
instanceRestartRequired, err = c.checkAndSetGlobalPostgreSQLConfiguration(&pod, patroniConfig, pgParameters)
410+
if err != nil {
411+
c.logger.Warningf("could not set PostgreSQL configuration options for pod %s: %v", podName, err)
412+
continue
413+
}
414+
break
408415
}
409-
break
410416
}
411417

412418
// if the config update requires a restart, call Patroni restart for replicas first, then master
@@ -493,16 +499,9 @@ func (c *Cluster) AnnotationsToPropagate(annotations map[string]string) map[stri
493499

494500
// checkAndSetGlobalPostgreSQLConfiguration checks whether cluster-wide API parameters
495501
// (like max_connections) have changed and if necessary sets it via the Patroni API
496-
func (c *Cluster) checkAndSetGlobalPostgreSQLConfiguration(pod *v1.Pod, patroniConfig map[string]interface{}) (bool, error) {
502+
func (c *Cluster) checkAndSetGlobalPostgreSQLConfiguration(pod *v1.Pod, patroniConfig acidv1.Patroni, effectivePgParameters map[string]string) (bool, error) {
497503
configToSet := make(map[string]interface{})
498504
parametersToSet := make(map[string]string)
499-
effectivePgParameters := make(map[string]interface{})
500-
501-
// read effective Patroni config if set
502-
if patroniConfig != nil {
503-
effectivePostgresql := patroniConfig["postgresql"].(map[string]interface{})
504-
effectivePgParameters = effectivePostgresql[patroniPGParametersParameterName].(map[string]interface{})
505-
}
506505

507506
// compare parameters under postgresql section with c.Spec.Postgresql.Parameters from manifest
508507
desiredPgParameters := c.Spec.Parameters
@@ -514,36 +513,47 @@ func (c *Cluster) checkAndSetGlobalPostgreSQLConfiguration(pod *v1.Pod, patroniC
514513
}
515514

516515
if len(parametersToSet) > 0 {
517-
configToSet["postgresql"] = map[string]interface{}{patroniPGParametersParameterName: parametersToSet}
516+
configToSet["postgresql"] = map[string]interface{}{constants.PatroniPGParametersParameterName: parametersToSet}
518517
}
519518

520519
// compare other options from config with c.Spec.Patroni from manifest
521520
desiredPatroniConfig := c.Spec.Patroni
522-
if desiredPatroniConfig.LoopWait > 0 && desiredPatroniConfig.LoopWait != uint32(patroniConfig["loop_wait"].(float64)) {
521+
if desiredPatroniConfig.LoopWait > 0 && desiredPatroniConfig.LoopWait != patroniConfig.LoopWait {
523522
configToSet["loop_wait"] = desiredPatroniConfig.LoopWait
524523
}
525-
if desiredPatroniConfig.MaximumLagOnFailover > 0 && desiredPatroniConfig.MaximumLagOnFailover != float32(patroniConfig["maximum_lag_on_failover"].(float64)) {
524+
if desiredPatroniConfig.MaximumLagOnFailover > 0 && desiredPatroniConfig.MaximumLagOnFailover != patroniConfig.MaximumLagOnFailover {
526525
configToSet["maximum_lag_on_failover"] = desiredPatroniConfig.MaximumLagOnFailover
527526
}
528-
if desiredPatroniConfig.PgHba != nil && !reflect.DeepEqual(desiredPatroniConfig.PgHba, (patroniConfig["pg_hba"])) {
527+
if desiredPatroniConfig.PgHba != nil && !reflect.DeepEqual(desiredPatroniConfig.PgHba, patroniConfig.PgHba) {
529528
configToSet["pg_hba"] = desiredPatroniConfig.PgHba
530529
}
531-
if desiredPatroniConfig.RetryTimeout > 0 && desiredPatroniConfig.RetryTimeout != uint32(patroniConfig["retry_timeout"].(float64)) {
530+
if desiredPatroniConfig.RetryTimeout > 0 && desiredPatroniConfig.RetryTimeout != patroniConfig.RetryTimeout {
532531
configToSet["retry_timeout"] = desiredPatroniConfig.RetryTimeout
533532
}
534-
if desiredPatroniConfig.Slots != nil && !reflect.DeepEqual(desiredPatroniConfig.Slots, patroniConfig["slots"]) {
535-
configToSet["slots"] = desiredPatroniConfig.Slots
536-
}
537-
if desiredPatroniConfig.SynchronousMode != patroniConfig["synchronous_mode"] {
533+
if desiredPatroniConfig.SynchronousMode != patroniConfig.SynchronousMode {
538534
configToSet["synchronous_mode"] = desiredPatroniConfig.SynchronousMode
539535
}
540-
if desiredPatroniConfig.SynchronousModeStrict != patroniConfig["synchronous_mode_strict"] {
536+
if desiredPatroniConfig.SynchronousModeStrict != patroniConfig.SynchronousModeStrict {
541537
configToSet["synchronous_mode_strict"] = desiredPatroniConfig.SynchronousModeStrict
542538
}
543-
if desiredPatroniConfig.TTL > 0 && desiredPatroniConfig.TTL != uint32(patroniConfig["ttl"].(float64)) {
539+
if desiredPatroniConfig.TTL > 0 && desiredPatroniConfig.TTL != patroniConfig.TTL {
544540
configToSet["ttl"] = desiredPatroniConfig.TTL
545541
}
546542

543+
// check if specified slots exist in config and if they differ
544+
slotsToSet := make(map[string]map[string]string)
545+
for slotName, desiredSlot := range desiredPatroniConfig.Slots {
546+
if effectiveSlot, exists := patroniConfig.Slots[slotName]; exists {
547+
if reflect.DeepEqual(desiredSlot, effectiveSlot) {
548+
continue
549+
}
550+
}
551+
slotsToSet[slotName] = desiredSlot
552+
}
553+
if len(slotsToSet) > 0 {
554+
configToSet["slots"] = slotsToSet
555+
}
556+
547557
if len(configToSet) == 0 {
548558
return false, nil
549559
}

pkg/util/constants/postgresql.go

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -8,6 +8,8 @@ const (
88
PostgresDataMount = "/home/postgres/pgdata"
99
PostgresDataPath = PostgresDataMount + "/pgroot"
1010

11+
PatroniPGParametersParameterName = "parameters"
12+
1113
PostgresConnectRetryTimeout = 2 * time.Minute
1214
PostgresConnectTimeout = 15 * time.Second
1315

pkg/util/patroni/patroni.go

Lines changed: 49 additions & 41 deletions
Original file line numberDiff line numberDiff line change
@@ -10,9 +10,11 @@ import (
1010
"strconv"
1111
"time"
1212

13+
"github.com/zalando/postgres-operator/pkg/util/constants"
1314
httpclient "github.com/zalando/postgres-operator/pkg/util/httpclient"
1415

1516
"github.com/sirupsen/logrus"
17+
acidv1 "github.com/zalando/postgres-operator/pkg/apis/acid.zalan.do/v1"
1618
v1 "k8s.io/api/core/v1"
1719
)
1820

@@ -31,7 +33,7 @@ type Interface interface {
3133
SetPostgresParameters(server *v1.Pod, options map[string]string) error
3234
GetMemberData(server *v1.Pod) (MemberData, error)
3335
Restart(server *v1.Pod) error
34-
GetConfig(server *v1.Pod) (map[string]interface{}, error)
36+
GetConfig(server *v1.Pod) (acidv1.Patroni, map[string]string, error)
3537
SetConfig(server *v1.Pod, config map[string]interface{}) error
3638
}
3739

@@ -109,28 +111,23 @@ func (p *Patroni) httpPostOrPatch(method string, url string, body *bytes.Buffer)
109111
}
110112

111113
func (p *Patroni) httpGet(url string) (string, error) {
112-
request, err := http.NewRequest("GET", url, nil)
113-
if err != nil {
114-
return "", fmt.Errorf("could not create request: %v", err)
115-
}
114+
p.logger.Debugf("making GET http request: %s", url)
116115

117-
p.logger.Debugf("making GET http request: %s", request.URL.String())
118-
119-
resp, err := p.httpClient.Do(request)
116+
response, err := p.httpClient.Get(url)
120117
if err != nil {
121118
return "", fmt.Errorf("could not make request: %v", err)
122119
}
123-
bodyBytes, err := ioutil.ReadAll(resp.Body)
120+
defer response.Body.Close()
121+
122+
bodyBytes, err := ioutil.ReadAll(response.Body)
124123
if err != nil {
125124
return "", fmt.Errorf("could not read response: %v", err)
126125
}
127-
if err := resp.Body.Close(); err != nil {
128-
return "", fmt.Errorf("could not close request: %v", err)
129-
}
130126

131-
if resp.StatusCode != http.StatusOK {
132-
return string(bodyBytes), fmt.Errorf("patroni returned '%d'", resp.StatusCode)
127+
if response.StatusCode != http.StatusOK {
128+
return string(bodyBytes), fmt.Errorf("patroni returned '%d'", response.StatusCode)
133129
}
130+
134131
return string(bodyBytes), nil
135132
}
136133

@@ -194,30 +191,43 @@ type MemberData struct {
194191
Patroni MemberDataPatroni `json:"patroni"`
195192
}
196193

197-
func (p *Patroni) GetConfigOrStatus(server *v1.Pod, path string) (map[string]interface{}, error) {
198-
result := make(map[string]interface{})
194+
func (p *Patroni) GetConfig(server *v1.Pod) (acidv1.Patroni, map[string]string, error) {
195+
var (
196+
patroniConfig acidv1.Patroni
197+
pgConfig map[string]interface{}
198+
)
199199
apiURLString, err := apiURL(server)
200200
if err != nil {
201-
return result, err
201+
return patroniConfig, nil, err
202202
}
203-
body, err := p.httpGet(apiURLString + path)
204-
err = json.Unmarshal([]byte(body), &result)
203+
body, err := p.httpGet(apiURLString + configPath)
205204
if err != nil {
206-
return result, err
205+
return patroniConfig, nil, err
206+
}
207+
err = json.Unmarshal([]byte(body), &patroniConfig)
208+
if err != nil {
209+
return patroniConfig, nil, err
207210
}
208211

209-
return result, err
210-
}
211-
212-
func (p *Patroni) GetStatus(server *v1.Pod) (map[string]interface{}, error) {
213-
return p.GetConfigOrStatus(server, statusPath)
214-
}
212+
// unmarshalling postgresql parameters needs a detour
213+
err = json.Unmarshal([]byte(body), &pgConfig)
214+
if err != nil {
215+
return patroniConfig, nil, err
216+
}
217+
pgParameters := make(map[string]string)
218+
if _, exists := pgConfig["postgresql"]; exists {
219+
effectivePostgresql := pgConfig["postgresql"].(map[string]interface{})
220+
effectivePgParameters := effectivePostgresql[constants.PatroniPGParametersParameterName].(map[string]interface{})
221+
for parameter, value := range effectivePgParameters {
222+
strValue := fmt.Sprintf("%v", value)
223+
pgParameters[parameter] = strValue
224+
}
225+
}
215226

216-
func (p *Patroni) GetConfig(server *v1.Pod) (map[string]interface{}, error) {
217-
return p.GetConfigOrStatus(server, configPath)
227+
return patroniConfig, pgParameters, err
218228
}
219229

220-
//Restart method restarts instance via Patroni POST API call.
230+
// Restart method restarts instance via Patroni POST API call.
221231
func (p *Patroni) Restart(server *v1.Pod) error {
222232
buf := &bytes.Buffer{}
223233
err := json.NewEncoder(buf).Encode(map[string]interface{}{"restart_pending": true})
@@ -228,9 +238,13 @@ func (p *Patroni) Restart(server *v1.Pod) error {
228238
if err != nil {
229239
return err
230240
}
231-
status, err := p.GetStatus(server)
232-
pending_restart, ok := status["pending_restart"]
233-
if !ok || !pending_restart.(bool) {
241+
memberData, err := p.GetMemberData(server)
242+
if err != nil {
243+
return err
244+
}
245+
246+
// do restart only when it is pending
247+
if !memberData.PendingRestart {
234248
return nil
235249
}
236250
return p.httpPostOrPatch(http.MethodPost, apiURLString+restartPath, buf)
@@ -243,19 +257,13 @@ func (p *Patroni) GetMemberData(server *v1.Pod) (MemberData, error) {
243257
if err != nil {
244258
return MemberData{}, err
245259
}
246-
response, err := p.httpClient.Get(apiURLString)
247-
if err != nil {
248-
return MemberData{}, fmt.Errorf("could not perform Get request: %v", err)
249-
}
250-
defer response.Body.Close()
251-
252-
body, err := ioutil.ReadAll(response.Body)
260+
body, err := p.httpGet(apiURLString + statusPath)
253261
if err != nil {
254-
return MemberData{}, fmt.Errorf("could not read response: %v", err)
262+
return MemberData{}, err
255263
}
256264

257265
data := MemberData{}
258-
err = json.Unmarshal(body, &data)
266+
err = json.Unmarshal([]byte(body), &data)
259267
if err != nil {
260268
return MemberData{}, err
261269
}

0 commit comments

Comments
 (0)