Tech: ajoute la possibilité de configurer un statement_timeout

[xavfernandez]

🤔 Pourquoi ?

Pour éviter que des locks en cascade ne bloquent toute la production.

🍰 Comment ?

Décrivez en quelques mots la solution retenue et mise en oeuvre, les difficultés ou problèmes rencontrés. Attirez l'attention sur les décisions d'architecture ou de conception importantes.

🚨 À vérifier

• Mettre à jour le CHANGELOG_breaking_changes.md ?
• Ajouter l'étiquette « Bug » ?

🏝️ Comment tester ?

Les instructions pour reproduire le problème, les profils de test, le parcours spécifique à utiliser, etc. Si vous disposez d'une recette jetable, mettre l'URL pour tester dans cette partie.

💻 Captures d'écran

[rsebille]

On voudrais pas plutôt utiliser lock_timeout ? Car là toute requête un peu longue va se faire tuer ce qui peux être un peu chiant au niveau des migrations ou des scripts, surtout avec le mode maintenance, et ça permettrais de pouvoir spécifier la valeur par défaut pour tout les environnements et avoir l'env var que pour éventuellement débrayer.

Je suppose que tu prévois qu'on utilise que WSGI_DB_STATEMENT_TIMEOUT pour justement ne pas avoir ce genre de problème, et même si c'est plus élégant je trouve ça un peu bizarre d'avoir 2 options mais une seul d'utilisée.

[francoisfreitag]

utilise[r] que WSGI_DB_STATEMENT_TIMEOUT

Je pense qu’on n’a pas trop le choix, puisque la configuration de la connection est dans les settings, qui sont communs à toutes nos instances de django.

[francoisfreitag]

Pour moi, statement timeout reste une bonne idée, surtout si on reste sur l’idée d’un statement timeout généreux à 15s. Un lock_timeout serait une sécurité supplémentaire qui me semble judicieuse, et on pourrait le descendre à 3-5s ?

[xavfernandez]

Nouvelle mouture pour détecter directement si on tourne dans uwsgi ou non.

Je pourrais donc éventuellement dropper DB_STATEMENT_TIMEOUT et n'avoir qu'un setting mais je le trouve pratique pour tester le comportement (vs lancer l'app avec uwsgi - UWSGI_DB_STATEMENT_TIMEOUT=1 uwsgi --http :9090 --wsgi-file config/wsgi.py --master --processes 4 --threads 2).
Et on pourrait aussi en fait avoir les deux en mettant un DB_STATEMENT_TIMEOUT à 60_000 pour nos cron/huey/migrations, sachant qu'on a rarement des requêtes qui durent aussi longtemps et que ça nous aurait (peut-être) évité le crash du dead lock de la migration des diagnostics (https://github.com/gip-inclusion/les-emplois/pull/6162/files).

Après statement_timeout vs lock_timeout: je suis plutôt pour éviter tout ce qui est trop long sur www.
Dans les faits ça sera des locks dans 80% des cas car on n'a (a priori 🤞 ) pas de grosses requêtes qui prennent plus de 7 secondes.

On pourrait effectivement combiner les 2 avec un statement_timeout à 10_000 et un lock_timeout à 5_000 (avec leurs déclinaisons UWSGI_DB_ / DB_).

Et effectivement, ça aurait du sens de mettre des valeurs par défaut (en plus, ça simplifie un peu le code).

[francoisfreitag]

Pour moi c’est bien, tu fais une mouture supplémentaire avec lock_timeout ?

[xavfernandez]

C'est fait.

[francoisfreitag]

On a déjà des statements plus long (voir les logs de la DB dans clever). J’ai facilement trouvé 85s.
Edit : Et même 123s en cherchant un peu plus.

[xavfernandez]

Effectivement: j'ai mis 0 comme valeur par défaut (équivalent à pas de timeout).

[francoisfreitag]

J’aurais bien mis un statement_timeout = 300_000 par défaut, histoire d’avoir une baseline. On pourra travailler à la réduire ensuite ?

[rsebille]

Je simplifierais la logique :

Suggested change

	except ImportError:
	uwsgi_db_statement_timeout = None
	uwsgi_db_lock_timeout = None
	else:
	uwsgi_db_statement_timeout = os.environ.get("UWSGI_DB_STATEMENT_TIMEOUT", 10_000)
	uwsgi_db_lock_timeout = os.environ.get("UWSGI_DB_LOCK_TIMEOUT", 5_000)
	db_statement_timeout = int(uwsgi_db_statement_timeout or os.getenv("DB_STATEMENT_TIMEOUT", 0))
	db_lock_timeout = int(uwsgi_db_lock_timeout or os.getenv("DB_LOCK_TIMEOUT", 0))
	except ImportError:
	db_statement_timeout = int(uwsgi_db_statement_timeout or os.getenv("DB_STATEMENT_TIMEOUT", 0))
	db_lock_timeout = int(uwsgi_db_lock_timeout or os.getenv("DB_LOCK_TIMEOUT", 0))
	else:
	db_statement_timeout = os.environ.get("UWSGI_DB_STATEMENT_TIMEOUT", 10_000)
	db_lock_timeout = os.environ.get("UWSGI_DB_LOCK_TIMEOUT", 5_000)

[xavfernandez]

Mais ta version empêche les confs DB_ de s'appliquer quand on tourne en uwsgi alors que je m'attendrais à avoir une conf générale DB_xxx éventuellement surchargée par UWSGI_DB_ si défini.

[rsebille]

Dans la version d'avant qu'il y a un défaut pour les USWGI_ on ne prend pas les DB_ dans tout les cas, à moins de mettre les USWGI_ à vide ce qui peux faire bizarre puisque 0 veux dire pas de timeout.

[xavfernandez]

Effectivement ma version actuelle n'est pas mieux 😅

[xavfernandez]

Bon je vois 2 options:

• rester sur l'équivalent du code actuel/la proposition de Romain avec:

try:
    import uwsgi
except ImportError:
    db_statement_timeout = os.environ.get("NOT_UWSGI_DB_STATEMENT_TIMEOUT", 300_000)
else:
    db_statement_timeout = os.environ.get("UWSGI_DB_STATEMENT_TIMEOUT", 10_000)

(le nom NOT_UWSGI_DB_STATEMENT_TIMEOUT n'est pas très joli mais a le mérite d'être très clair)

• aller vers ce que j'avais en tête initialement:

try:
    import uwsgi
except ImportError:
    uwsgi_db_statement_timeout = None
else:
    uwsgi_db_statement_timeout = os.environ.get("UWSGI_DB_STATEMENT_TIMEOUT")
db_statement_timeout = int(
    uwsgi_db_statement_timeout if uwsgi_db_statement_timeout is not None else os.getenv("DB_STATEMENT_TIMEOUT", 300_000)
)

(et donc ne pas avoir de valeur par défaut pour UWSGI_DB_STATEMENT_TIMEOUT )

(le code est simplifié mais il faut évidemment imaginer avoir le code équivalent pour le lock_timeout)

[francoisfreitag]

En voyant les deux, je préfère la solution 1., pour permettre de désactiver le statement timeout pour uWSGI.
Autrement, faudrait définir UWSGI_DB_STATEMENT_TIMEOUT=0 et DB_STATEMENT_TIMEOUT=0.
Avoir deux settings séparés rend la configuration plus claire.

[rsebille]

Dans la solution 2 tu n'as que UWSGI_DB_STATEMENT_TIMEOUT=0 à définir pour la désactivation vu que ça teste is not None et pas si c'est falsy.

J'ai proposé 1 car c'était la logique que j'avais comprise et donc je me suis dit qu'on voulais 2 manettes complètement distincte et pas une manette spécialisation de la précédente (la métaphore claqué au sol 😁), qui pour le coup est ce qui me semble plus naturelle et logique d'un point de vue configuration et administration.
Bon après je dit ça mais à la fin les deux seront configurés dans le même fichier car je doute qu'on veuille avoir la même valeur et donc ne garder que DB_STATEMENT_TIMEOUT, au final le fonctionnement sera celui de la solution 1...

Et je renommerais bien UWSGI_DB_STATEMENT_TIMEOUT en WWW_DB_STATEMENT_TIMEOUT, comme ça l'intuition est claire et agnostique ;).

[xavfernandez]

@rsebille @francoisfreitag Dernière ( 🤞 ) version ?