Skip to content

Commit 0d665b0

Browse files
benjaminjbbenjaminjb
committed
Use SIGKILL to reload gunicorn logger
Do we really want to do this?
1 parent 3a69d47 commit 0d665b0

File tree

5 files changed

+92
-119
lines changed

5 files changed

+92
-119
lines changed

‎internal/controller/standalone_pgadmin/config.go‎

Lines changed: 3 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -7,10 +7,9 @@ package standalone_pgadmin
77
// Include configs here used by multiple files
88
const (
99
// ConfigMap keys used also in mounting volume to pod
10-
settingsConfigMapKey = "pgadmin-settings.json"
11-
settingsClusterMapKey = "pgadmin-shared-clusters.json"
12-
gunicornLoggingConfigKey = "gunicorn-logging-config.json"
13-
gunicornConfigKey = "gunicorn-config.json"
10+
settingsConfigMapKey = "pgadmin-settings.json"
11+
settingsClusterMapKey = "pgadmin-shared-clusters.json"
12+
gunicornConfigKey = "gunicorn-config.json"
1413

1514
// Port address used to define pod and service
1615
pgAdminPort = 5050

‎internal/controller/standalone_pgadmin/configmap.go‎

Lines changed: 9 additions & 24 deletions
Original file line numberDiff line numberDiff line change
@@ -105,11 +105,10 @@ func configmap(ctx context.Context, pgadmin *v1beta1.PGAdmin,
105105
configmap.Data[settingsClusterMapKey] = clusterSettings
106106
}
107107

108-
gunicornSettings, gunicornLoggingSettings, err := generateGunicornConfig(pgadmin,
108+
gunicornSettings, err := generateGunicornConfig(pgadmin,
109109
logRetention, maxBackupRetentionNumber, gunicornRetentionPeriod)
110110
if err == nil {
111111
configmap.Data[gunicornConfigKey] = gunicornSettings
112-
configmap.Data[gunicornLoggingConfigKey] = gunicornLoggingSettings
113112
}
114113

115114
return configmap, err
@@ -239,7 +238,7 @@ func generateClusterConfig(
239238
// - https://docs.gunicorn.org/en/latest/settings.html
240239
func generateGunicornConfig(pgadmin *v1beta1.PGAdmin,
241240
logRetention bool, maxBackupRetentionNumber int, gunicornRetentionPeriod string,
242-
) (string, string, error) {
241+
) (string, error) {
243242
settings := map[string]any{
244243
// Bind to all IPv4 addresses and set 25 threads by default.
245244
// - https://docs.gunicorn.org/en/latest/settings.html#bind
@@ -256,21 +255,6 @@ func generateGunicornConfig(pgadmin *v1beta1.PGAdmin,
256255
// Write mandatory settings over any specified ones.
257256
// - https://docs.gunicorn.org/en/latest/settings.html#workers
258257
settings["workers"] = 1
259-
260-
// To avoid spurious reconciles, the following value must not change when
261-
// the spec does not change. [json.Encoder] and [json.Marshal] do this by
262-
// emitting map keys in sorted order. Indent so the value is not rendered
263-
// as one long line by `kubectl`.
264-
buffer := new(bytes.Buffer)
265-
encoder := json.NewEncoder(buffer)
266-
encoder.SetEscapeHTML(false)
267-
encoder.SetIndent("", " ")
268-
err := encoder.Encode(settings)
269-
270-
if err != nil {
271-
return buffer.String(), "", err
272-
}
273-
274258
// Gunicorn logging dict settings
275259
logSettings := map[string]any{}
276260

@@ -333,16 +317,17 @@ func generateGunicornConfig(pgadmin *v1beta1.PGAdmin,
333317
},
334318
}
335319
}
320+
settings["logconfig_dict"] = logSettings
336321

337322
// To avoid spurious reconciles, the following value must not change when
338323
// the spec does not change. [json.Encoder] and [json.Marshal] do this by
339324
// emitting map keys in sorted order. Indent so the value is not rendered
340325
// as one long line by `kubectl`.
341-
logBuffer := new(bytes.Buffer)
342-
logEncoder := json.NewEncoder(logBuffer)
343-
logEncoder.SetEscapeHTML(false)
344-
logEncoder.SetIndent("", " ")
345-
err = logEncoder.Encode(logSettings)
326+
buffer := new(bytes.Buffer)
327+
encoder := json.NewEncoder(buffer)
328+
encoder.SetEscapeHTML(false)
329+
encoder.SetIndent("", " ")
330+
err := encoder.Encode(settings)
346331

347-
return buffer.String(), logBuffer.String(), err
332+
return buffer.String(), err
348333
}

‎internal/controller/standalone_pgadmin/configmap_test.go‎

Lines changed: 62 additions & 64 deletions
Original file line numberDiff line numberDiff line change
@@ -6,7 +6,6 @@ package standalone_pgadmin
66

77
import (
88
"context"
9-
"strings"
109
"testing"
1110

1211
"gotest.tools/v3/assert"
@@ -258,14 +257,14 @@ func TestGenerateGunicornConfig(t *testing.T) {
258257

259258
expectedString := `{
260259
"bind": "0.0.0.0:5050",
260+
"logconfig_dict": {},
261261
"threads": 25,
262262
"workers": 1
263263
}
264264
`
265-
actualString, logString, err := generateGunicornConfig(pgAdmin, false, 0, "H")
265+
actualString, err := generateGunicornConfig(pgAdmin, false, 0, "H")
266266
assert.NilError(t, err)
267267
assert.Equal(t, actualString, expectedString)
268-
assert.Assert(t, strings.Contains(logString, "{}"))
269268
})
270269

271270
t.Run("Add Settings", func(t *testing.T) {
@@ -281,14 +280,14 @@ func TestGenerateGunicornConfig(t *testing.T) {
281280
"bind": "0.0.0.0:5050",
282281
"certfile": "/path/to/certfile",
283282
"keyfile": "/path/to/keyfile",
283+
"logconfig_dict": {},
284284
"threads": 25,
285285
"workers": 1
286286
}
287287
`
288-
actualString, logString, err := generateGunicornConfig(pgAdmin, false, 0, "H")
288+
actualString, err := generateGunicornConfig(pgAdmin, false, 0, "H")
289289
assert.NilError(t, err)
290290
assert.Equal(t, actualString, expectedString)
291-
assert.Assert(t, strings.Contains(logString, "{}"))
292291
})
293292

294293
t.Run("Update Defaults", func(t *testing.T) {
@@ -302,14 +301,14 @@ func TestGenerateGunicornConfig(t *testing.T) {
302301

303302
expectedString := `{
304303
"bind": "127.0.0.1:5051",
304+
"logconfig_dict": {},
305305
"threads": 30,
306306
"workers": 1
307307
}
308308
`
309-
actualString, logString, err := generateGunicornConfig(pgAdmin, false, 0, "H")
309+
actualString, err := generateGunicornConfig(pgAdmin, false, 0, "H")
310310
assert.NilError(t, err)
311311
assert.Equal(t, actualString, expectedString)
312-
assert.Assert(t, strings.Contains(logString, "{}"))
313312
})
314313

315314
t.Run("Update Mandatory", func(t *testing.T) {
@@ -322,14 +321,14 @@ func TestGenerateGunicornConfig(t *testing.T) {
322321

323322
expectedString := `{
324323
"bind": "0.0.0.0:5050",
324+
"logconfig_dict": {},
325325
"threads": 25,
326326
"workers": 1
327327
}
328328
`
329-
actualString, logString, err := generateGunicornConfig(pgAdmin, false, 0, "H")
329+
actualString, err := generateGunicornConfig(pgAdmin, false, 0, "H")
330330
assert.NilError(t, err)
331331
assert.Equal(t, actualString, expectedString)
332-
assert.Assert(t, strings.Contains(logString, "{}"))
333332
})
334333

335334
t.Run("OTel enabled", func(t *testing.T) {
@@ -341,73 +340,72 @@ func TestGenerateGunicornConfig(t *testing.T) {
341340
logs: { retentionPeriod: 5h },
342341
},
343342
}`)
344-
actualString, logString, err := generateGunicornConfig(pgAdmin, true, 4, "H")
343+
actualString, err := generateGunicornConfig(pgAdmin, true, 4, "H")
345344

346345
expectedString := `{
347346
"bind": "0.0.0.0:5050",
348-
"threads": 25,
349-
"workers": 1
350-
}
351-
`
352-
expectedLogString := `{
353-
"formatters": {
354-
"generic": {
355-
"class": "logging.Formatter",
356-
"datefmt": "[%Y-%m-%d %H:%M:%S %z]",
357-
"format": "%(asctime)s [%(process)d] [%(levelname)s] %(message)s"
347+
"logconfig_dict": {
348+
"formatters": {
349+
"generic": {
350+
"class": "logging.Formatter",
351+
"datefmt": "[%Y-%m-%d %H:%M:%S %z]",
352+
"format": "%(asctime)s [%(process)d] [%(levelname)s] %(message)s"
353+
},
354+
"json": {
355+
"class": "jsonformatter.JsonFormatter",
356+
"format": {
357+
"level": "levelname",
358+
"message": "message",
359+
"name": "name",
360+
"time": "created"
361+
},
362+
"separators": [
363+
",",
364+
":"
365+
]
366+
}
358367
},
359-
"json": {
360-
"class": "jsonformatter.JsonFormatter",
361-
"format": {
362-
"level": "levelname",
363-
"message": "message",
364-
"name": "name",
365-
"time": "created"
368+
"handlers": {
369+
"console": {
370+
"class": "logging.StreamHandler",
371+
"formatter": "generic",
372+
"stream": "ext://sys.stdout"
366373
},
367-
"separators": [
368-
",",
369-
":"
370-
]
371-
}
372-
},
373-
"handlers": {
374-
"console": {
375-
"class": "logging.StreamHandler",
376-
"formatter": "generic",
377-
"stream": "ext://sys.stdout"
374+
"file": {
375+
"backupCount": 4,
376+
"class": "logging.handlers.TimedRotatingFileHandler",
377+
"filename": "/var/lib/pgadmin/logs/gunicorn.log",
378+
"formatter": "json",
379+
"interval": 1,
380+
"when": "H"
381+
}
378382
},
379-
"file": {
380-
"backupCount": 4,
381-
"class": "logging.handlers.TimedRotatingFileHandler",
382-
"filename": "/var/lib/pgadmin/logs/gunicorn.log",
383-
"formatter": "json",
384-
"interval": 1,
385-
"when": "H"
383+
"loggers": {
384+
"gunicorn.access": {
385+
"handlers": [
386+
"file"
387+
],
388+
"level": "INFO",
389+
"propagate": true,
390+
"qualname": "gunicorn.access"
391+
},
392+
"gunicorn.error": {
393+
"handlers": [
394+
"file"
395+
],
396+
"level": "INFO",
397+
"propagate": true,
398+
"qualname": "gunicorn.error"
399+
}
386400
}
387401
},
388-
"loggers": {
389-
"gunicorn.access": {
390-
"handlers": [
391-
"file"
392-
],
393-
"level": "INFO",
394-
"propagate": true,
395-
"qualname": "gunicorn.access"
396-
},
397-
"gunicorn.error": {
398-
"handlers": [
399-
"file"
400-
],
401-
"level": "INFO",
402-
"propagate": true,
403-
"qualname": "gunicorn.error"
404-
}
405-
}
402+
"threads": 25,
403+
"workers": 1
406404
}
407405
`
406+
408407
assert.NilError(t, err)
409408
assert.Equal(t, actualString, expectedString)
410-
assert.Equal(t, logString, expectedLogString)
411409
})
412410

413411
}

‎internal/controller/standalone_pgadmin/pod.go‎

Lines changed: 12 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -22,13 +22,12 @@ import (
2222
)
2323

2424
const (
25-
configMountPath = "/etc/pgadmin/conf.d"
26-
configFilePath = "~postgres-operator/" + settingsConfigMapKey
27-
clusterFilePath = "~postgres-operator/" + settingsClusterMapKey
28-
configDatabaseURIPath = "~postgres-operator/config-database-uri"
29-
ldapFilePath = "~postgres-operator/ldap-bind-password"
30-
gunicornConfigFilePath = "~postgres-operator/" + gunicornConfigKey
31-
gunicornLogConfFilePath = "~postgres-operator/" + gunicornLoggingConfigKey
25+
configMountPath = "/etc/pgadmin/conf.d"
26+
configFilePath = "~postgres-operator/" + settingsConfigMapKey
27+
clusterFilePath = "~postgres-operator/" + settingsClusterMapKey
28+
configDatabaseURIPath = "~postgres-operator/config-database-uri"
29+
ldapFilePath = "~postgres-operator/ldap-bind-password"
30+
gunicornConfigFilePath = "~postgres-operator/" + gunicornConfigKey
3231

3332
// scriptMountPath is where to mount a temporary directory that is only
3433
// writable during Pod initialization.
@@ -208,10 +207,6 @@ func podConfigFiles(configmap *corev1.ConfigMap, pgadmin v1beta1.PGAdmin) []core
208207
Key: gunicornConfigKey,
209208
Path: gunicornConfigFilePath,
210209
},
211-
{
212-
Key: gunicornLoggingConfigKey,
213-
Path: gunicornLogConfFilePath,
214-
},
215210
},
216211
},
217212
},
@@ -267,10 +262,7 @@ func startupScript(pgadmin *v1beta1.PGAdmin) []string {
267262

268263
// startCommands (v8 image includes Gunicorn)
269264
var startCommandV7 = "pgadmin4 &"
270-
// For Gunicorn, watch the logging config and reload if changes are detected.
271265
var startCommandV8 = "gunicorn -c /etc/pgadmin/gunicorn_config.py" +
272-
" --reload-extra-file " + configMountPath + `/` + gunicornLogConfFilePath +
273-
" --log-config-json " + configMountPath + `/` + gunicornLogConfFilePath +
274266
" --chdir $PGADMIN_DIR pgAdmin4:app &"
275267

276268
// This script sets up, starts pgadmin, and runs the appropriate `loadServerCommand` to register the discovered servers.
@@ -319,10 +311,15 @@ loadServerCommand
319311
// descriptor and uses the timeout of the builtin `read` to wait. That same
320312
// descriptor gets closed and reopened to use the builtin `[ -nt` to check mtimes.
321313
// - https://unix.stackexchange.com/a/407383
314+
// In order to get gunicorn to reload the logging config
315+
// we need to send a KILL rather than a HUP signal.
316+
// - https://github.com/benoitc/gunicorn/issues/3353
317+
// Right now the config file is on the same configMap as the cluster file
318+
// so if the mtime changes for any of those files, it will change for all.
322319
var reloadScript = `
323320
exec {fd}<> <(:||:)
324321
while read -r -t 5 -u "${fd}" ||:; do
325-
if [[ "${cluster_file}" -nt "/proc/self/fd/${fd}" ]] && loadServerCommand
322+
if [[ "${cluster_file}" -nt "/proc/self/fd/${fd}" ]] && loadServerCommand && kill -KILL $(head -1 ${PGADMIN4_PIDFILE?});
326323
then
327324
exec {fd}>&- && exec {fd}<> <(:||:)
328325
stat --format='Loaded shared servers dated %y' "${cluster_file}"

0 commit comments

Comments
 (0)