Produits Entr'ouvert » Hobo

Frédéric Péters a écrit :

Du coup, pourquoi ne pas simplement avoir la possibilité de mettre DISABLE_CRON_JOBS = True dans le settings.py ? (patch qui fait ça)

Yep. Mais peut-être uniquement en cas de --all-tenants ? (c'est "tenant_command --all-tenants" qui est utilisé par les crons)

• ajouter un petit help sur force_job
• en cas de verbose, un petit message si le DISABLE_CRON_JOBS est coupé
• pour mémoire et guise de simili-documentation, poser un DISABLE_CRON_JOBS = False dans debian/debian_config_common.py avec un mini commentaire qui explique que ça bloque les commandes lancées avec "tenant_command --all-tenants"

J'aime pas trop même si ça fait le job, tout ce qu'on lance avec tenant_command n'est pas un job cron, on va avoir de plus en plus de machines en TMA autre que la notre, ça fera un truc en plus à expliquer (faudra faire tenant_command --force-job shell pour avoir un shell sur ces machines par exemple, ou créer un superuser quand on en a pas, ou que sais je d'autre).

Ça ne vous dirait pas plus de simplement changer les debian/package.cron pour if ajouter des

test -f /etc/package/no-cron-job || la_command

Pas vu la remarque sur --all-tenants, effectivement ce serait plus acceptable dans ce cas.

Benjamin Dauvergne a écrit :

Ça ne vous dirait pas plus de simplement changer les debian/package.cron pour if ajouter des "test -f /etc/package/no-cron-job"

J'avoue que c'est compréhensible à un unixien de base (tel que moi). Je ne vois pas de désavantage. Fred, pourquoi on était parti sur un truc dans le code de l'appli ? On avait une raison ? (si oui je l'ai pas notée)

On avait une raison ? (si oui je l'ai pas notée)

Non. Moi ça me va aussi de ne rien faire, juste dans les notes d'installation redondante dire qu'il faut désactiver tels jobs.

En fait, à y reflechir, l'idée du "DISABLE_CRON_JOBS = True dans le settings.py" elle est pas super parce que ça rend un peu difficile une éventuelle bascule d'une machine à l'autre.

Avoir des "/etc/<package>/cron-disabled" c'est quand même pratique (bascule avec rm/touch et hop).

ack.

un-ack ; cf mes indications sur #1, le patch tel quel est trop brutal.

Christophe Siraut a écrit :

J'aimerais pouvoir poser cette option uniquement dans le settings du serveur hobo.

Le serveur hobo est installé sur une machine différente, donc c'est pas possible. Ou bien tu parlais d'autre chose ?

Vu ensemble déjà, ça me va bien ; pousse ça dès que tu as testé une exécution d'un truc en local.

C'est pas ok :

args_namespace, args = args_parser.parse_known_args(argv)

va supprimer le verbosity de args, qui en sera pas envoyé à la commande lancée sur les tenants cibles.

Il faudrait lire le verbosity avec un parser à part, qui ne touche pas à args, genre :

    verbosity_parser = argparse.ArgumentParser() 
    verbosity_parser.add_argument('-v', '--verbosity', action='store', dest='verbosity', default=1, type=int)
    args_verbosity, _ = verbosity_parser.parse_known_args(args)
    # et on a bien un args_verbosity.verbosity (à 1 par défaut)

Ack

Une proposition basée sur un lock global dans postgres, ça bloque aussi le fait
de faire tourner les crons durant les migrations, et inversement.

Bonne idée, je n'ai pas testé. Je vois que tu hardcode un temps d'attente de 1 heure pour les migrations, cela me semble beaucoup, et vu que les migrations sont interactives j'ajouterais un message à l'utilisateur pendant l'attente. Par rapport au lock global sur les cron, pour les installations de Publik sur un postgresql centralisé, nous ne ferons pas l’économie d'organiser les tâches cron des différentes briques (au rique que ce soit toujours la même qui prenne le verrou)

Qu'est-ce qui exigerait d'avoir les CRONs lancés toujours sur le même serveur ? Le fait que les machines ne soient pas toutes à jour en même temps ?

Mon avis sur les CRONs c'est qu'ils devraient être écrit pour pouvoir s'exécuter en parallèle autant que possible, avec des select_for_update() et à partir de Django 1.11 des select_for_update(skip_locked=True), mais qu'en attendant on peut utiliser PG comme gestionnaire de lock distribué.

(Reporté commentaire et patch que j'avais malencontreusement posé sur le #23471).

Dans le code actuel de PGLock(); est-ce qu'une tentative d'exécution de cron est empêchée par le verrou de global de migration?

je ne comprends pas encore pourquoi on doit dupliquer try_lock et take_lock, je réessaie demain.

Christophe Siraut a écrit :

Dans le code actuel de PGLock(); est-ce qu'une tentative d'exécution de cron est empêchée par le verrou de global de migration?

Oui toujours, j'ai pensé que c'était plus logique de faire comme cela, par contre je ne renvoie pas de code retour particulier, je ne sais pas si CommandError le permet, ce serait peut-être utile pour ne pas émettre de mail d'erreur inutile, mais bon c'est un détail.

Christophe Siraut a écrit :

Dans le code actuel de PGLock(); est-ce qu'une tentative d'exécution de cron est empêchée par le verrou de global de migration?

je ne comprends pas encore pourquoi on doit dupliquer try_lock et take_lock, je réessaie demain.

C'est juste que try_lock() renvoie une réponse tout de suite alors que take_lock prend un lock au minimum d'1s, j'aurai pu jouer la dessus et produire moins de code, je voulais juste jouer avec l'API de postgresql, aucun souci pour simplifier.

Dans quel cas d'utilisation de tenant_command.py voudrait-on spécifier manuellement un lock_id? Au lieu d'ajouter l'option '--lock' pourrait-on utiliser le nom de l'application (authentic/combo/etc) comme identifiant de verrou?

je ne comprends pas encore pourquoi on doit dupliquer try_lock et take_lock, je réessaie demain.

C'est juste que try_lock() renvoie une réponse tout de suite alors que take_lock prend un lock au minimum d'1s, j'aurai pu jouer la dessus et produire moins de code, je voulais juste jouer avec l'API de postgresql, aucun souci pour simplifier.

oui ça me plairait bien de simplifier, (ou de rajouter du texte sur ce à quoi correspond take_lock et try_lock); par ailleurs le block suivant ne me semble pas logique (soit on attend, soit on attend pas):

+            if self.wait == 0:
                 […]
+            else:
+                if self.wait:

Christophe Siraut a écrit :

Dans quel cas d'utilisation de tenant_command.py voudrait-on spécifier manuellement un lock_id? Au lieu d'ajouter l'option '--lock' pourrait-on utiliser le nom de l'application (authentic/combo/etc) comme identifiant de verrou?

De façon évidente non, postgresql ne support que des entiers comme identifiant de lock (32 ou 64 bits, je crois). Maintenant on pourrait générer un entier à partir d'une chaîne via une fonction de hash (genre (str.hash() % 0xFFFFF) << 12, on se laisse 12bits bas de marge).

De base ça n'a pas vraiment d'importance on peut sans trop de soucis utiliser le lock_id "1" pour tout le monde au départ, et faire de --lock un simple booléen et plus un paramètre entier.

je ne comprends pas encore pourquoi on doit dupliquer try_lock et take_lock, je réessaie demain.

C'est juste que try_lock() renvoie une réponse tout de suite alors que take_lock prend un lock au minimum d'1s, j'aurai pu jouer la dessus et produire moins de code, je voulais juste jouer avec l'API de postgresql, aucun souci pour simplifier.

oui ça me plairait bien de simplifier, (ou de rajouter du texte sur ce à quoi correspond take_lock et try_lock); par ailleurs le block suivant ne me semble pas logique (soit on attend, soit on attend pas):

[...]

Ok, je vais voir pour faire ça et supprimer vraisemblablement lock_id.