[Bug] do_cleanup attend uniquement la complétion du job de delete et ignore l'échec (blocage 10 min + job échoué non purgé) #15

New Issue

xmortelette · 2026-06-12T09:29:36+02:00

xmortelette commented

2026-06-12 09:29:36 +02:00

Constat

Dans operator/src/instance_common.rs, do_cleanup() crée le job de suppression puis attend uniquement la condition is_job_completed :

job_api.create(&PostParams::default(), &serde_json::from_value(job_def)?).await?;

// Wait for the delete job to complete
let cond = await_condition(job_api.clone(), &job_name, conditions::is_job_completed());
tokio::time::timeout(std::time::Duration::from_secs(10 * 60), cond)
    .await
    .map_err(Error::Elapsed)?
    .map_err(Error::KubeWaitError)?;

// Delete the delete job
match job_api.delete(&job_name, &DeleteParams::foreground()).await { ... }

Problèmes

Aucune détection d'échec. is_job_completed() ne se déclenche que sur Complete. Si le job de delete part en Failed (ce qui arrive si la désinstallation rate), la condition n'est jamais vraie : on attend le timeout complet de 10 minutes avant de remonter une Elapsed, à chaque reconcile. La désinstallation d'une instance cassée devient très lente et bruyante.
Le job échoué n'est pas purgé sur le chemin d'erreur. Le job_api.delete(...) final n'est atteint qu'après une complétion réussie. En cas de timeout/erreur, le ? propage l'erreur et le job (et son pod en erreur) reste en place ; au reconcile suivant, create peut échouer (AlreadyExists) — il n'y a pas de upsert/delete-préalable comme pour le job d'install (upsert_job).

À comparer avec do_reconcile qui passe par upsert_job() (patch SSA + fallback delete/create) — le chemin cleanup n'a pas cette robustesse.

Pistes

Attendre une condition « terminal » (Complete ou Failed), comme is_job_terminal() déjà présent dans operator/src/jukebox.rs, puis brancher : succès ⇒ purge du job + await_change() ; échec ⇒ log + requeue temporisé (sans attendre 10 min).
Réutiliser upsert_job() / un delete-préalable pour le job de delete afin d'éviter les AlreadyExists au retry.
Purger le job dans tous les cas terminaux (y compris échec) avant de requeue.

Tests

Unitaire : extraire et tester un prédicat is_job_terminal/classification (Complete vs Failed) — réutiliser celui de jukebox.rs.
Intégration : forcer un job de delete en échec (script qui throw) → l'opérateur doit requeue rapidement avec un message clair, pas attendre 10 min, et ne pas accumuler des jobs AlreadyExists.

## Constat Dans [`operator/src/instance_common.rs`](operator/src/instance_common.rs), `do_cleanup()` crée le job de suppression puis attend uniquement la condition `is_job_completed` : ```rust job_api.create(&PostParams::default(), &serde_json::from_value(job_def)?).await?; // Wait for the delete job to complete let cond = await_condition(job_api.clone(), &job_name, conditions::is_job_completed()); tokio::time::timeout(std::time::Duration::from_secs(10 * 60), cond) .await .map_err(Error::Elapsed)? .map_err(Error::KubeWaitError)?; // Delete the delete job match job_api.delete(&job_name, &DeleteParams::foreground()).await { ... } ``` ## Problèmes 1. **Aucune détection d'échec.** `is_job_completed()` ne se déclenche que sur `Complete`. Si le job de delete part en `Failed` (ce qui arrive si la désinstallation rate), la condition n'est jamais vraie : on attend le timeout complet de **10 minutes** avant de remonter une `Elapsed`, à chaque reconcile. La désinstallation d'une instance cassée devient très lente et bruyante. 2. **Le job échoué n'est pas purgé sur le chemin d'erreur.** Le `job_api.delete(...)` final n'est atteint qu'après une complétion réussie. En cas de timeout/erreur, le `?` propage l'erreur et le job (et son pod en erreur) reste en place ; au reconcile suivant, `create` peut échouer (`AlreadyExists`) — il n'y a pas de `upsert`/delete-préalable comme pour le job d'install (`upsert_job`). À comparer avec `do_reconcile` qui passe par `upsert_job()` (patch SSA + fallback delete/create) — le chemin cleanup n'a pas cette robustesse. ## Pistes 1. Attendre une condition « terminal » (Complete **ou** Failed), comme `is_job_terminal()` déjà présent dans [`operator/src/jukebox.rs`](operator/src/jukebox.rs#L242), puis brancher : succès ⇒ purge du job + `await_change()` ; échec ⇒ log + requeue temporisé (sans attendre 10 min). 2. Réutiliser `upsert_job()` / un delete-préalable pour le job de delete afin d'éviter les `AlreadyExists` au retry. 3. Purger le job dans tous les cas terminaux (y compris échec) avant de requeue. ## Tests - Unitaire : extraire et tester un prédicat `is_job_terminal`/classification (Complete vs Failed) — réutiliser celui de `jukebox.rs`. - Intégration : forcer un job de delete en échec (script qui `throw`) → l'opérateur doit requeue rapidement avec un message clair, pas attendre 10 min, et ne pas accumuler des jobs `AlreadyExists`.

xmortelette added the Kind/Bug

labels 2026-06-12 09:29:36 +02:00

xmortelette referenced this issue

2026-06-12 09:29:51 +02:00

[Robustesse] unwrap()/panics sur les chemins de reconcile (namespace, désérialisation JSON) #16

shuss removed the

Reviewed

Confirmed

4

label 2026-06-12 10:27:50 +02:00

xmortelette commented

2026-06-12 11:24:33 +02:00

Note de liaison avec #12 : la proposition v2 de #12 prévoit, quand aucun paquet n'est trouvable au cleanup, une condition E_PACKAGE_GONE + requeue temporisé au lieu de l'erreur dure — ce chemin s'appuie sur la robustesse du job de delete décrite ici (condition terminale Complete ou Failed, purge du job dans tous les cas, upsert au retry). Cette issue est donc un prérequis naturel de #12-C ; à traiter en premier. La proposition reste valable telle quelle, rien à modifier.

Analyse et rédaction : Claude (assistant IA), publié via le compte de Xavier.

Note de liaison avec #12 : la proposition v2 de #12 prévoit, quand aucun paquet n'est trouvable au cleanup, une condition `E_PACKAGE_GONE` + requeue temporisé au lieu de l'erreur dure — ce chemin s'appuie sur la robustesse du job de delete décrite ici (condition terminale Complete **ou** Failed, purge du job dans tous les cas, upsert au retry). Cette issue est donc un prérequis naturel de #12-C ; à traiter en premier. La proposition reste valable telle quelle, rien à modifier. --- *Analyse et rédaction : Claude (assistant IA), publié via le compte de Xavier.*

shuss commented

2026-06-12 17:13:39 +02:00

Dans l'absolu la solution me parait saine et couvre en effet un angle mort actuel du code.

Je veux bien ce même axe d'analyse en complément de #17 sur les job des jukebox qui sont, au fond, encore plus souvent problématiques

Dans l'absolu la solution me parait saine et couvre en effet un angle mort actuel du code. Je veux bien ce même axe d'analyse en complément de #17 sur les job des jukebox qui sont, au fond, encore plus souvent problématiques

shuss added the

Reviewed

analysed

1

label 2026-06-12 17:35:20 +02:00

Sign in to join this conversation.

2 Participants

Notifications

Due Date

No due date set.

Dependencies

No dependencies set.

Reference: shuss/vynil#15