Skip to content

change hook weight to -1 (job has to run after config maps placed)#776

Open
masbarnes wants to merge 2 commits intomainfrom
hook-weight-bug-fix
Open

change hook weight to -1 (job has to run after config maps placed)#776
masbarnes wants to merge 2 commits intomainfrom
hook-weight-bug-fix

Conversation

@masbarnes
Copy link
Collaborator

@masbarnes masbarnes commented Mar 10, 2026

Change hook weight for the job to -1 (the config maps with hook weight -2 need to be created in order for the job to succeed)

Greptile Summary

This PR fixes a Helm hook ordering bug by bumping database_init_job's helm.sh/hook-weight from -2 to -1, ensuring the job starts only after the config maps it depends on (all at weight -2) have been successfully created.

  • Fix is correct: Moving the init job to -1 guarantees it runs after all -2 config maps are in place, resolving the root cause described in the PR.
  • Ordering side-effect: The database_migration_job already sits at weight -1. With this change, both jobs share the same weight and Helm will run them concurrently. Previously the init job ran strictly before the migration job (init at -2, migration at -1). If both db.runDbInitScript and db.runDbMigrationScript can ever be true in the same release, the migration could race against an uninitialized database.
  • No other files changed: The fix is minimal and surgical.

Confidence Score: 4/5

  • Safe to merge assuming db.runDbInitScript and db.runDbMigrationScript are never both enabled simultaneously.
  • The change is minimal, intentional, and correctly addresses the stated bug. The only concern is a potential ordering issue between the init and migration jobs that now share the same hook weight, but this is mitigated if the two flags are mutually exclusive in practice.
  • charts/model-engine/templates/database_init_job.yaml — verify the concurrent execution of init and migration jobs at weight -1 is safe for all deployment scenarios.

Important Files Changed

Filename Overview
charts/model-engine/templates/database_init_job.yaml Hook weight changed from -2 to -1 so the job runs after config maps (weight -2) are created. This collapses the previously sequential init→migration ordering since database_migration_job is also at -1, but the two jobs are gated by separate flags so concurrent execution is likely not a real-world concern.

Flowchart

%%{init: {'theme': 'neutral'}}%%
flowchart TD
    subgraph weight_minus2 ["Hook Weight: -2 (runs first)"]
        CM1[service_config_map]
        CM2[service_template_config_map]
        CM3[inference_framework_config]
        CM4[aws_config_map]
    end

    subgraph weight_minus1 ["Hook Weight: -1 (after config maps)"]
        J1[database_init_job ✅ changed from -2]
        J2[database_migration_job]
    end

    subgraph weight_0 ["Hook Weight: 0"]
        J3[spellbook_init_job]
    end

    subgraph weight_1 ["Hook Weight: 1 (runs last)"]
        J4[restart_keda_operator]
        J5[populate_fine_tuning_repository_job]
    end

    weight_minus2 --> weight_minus1
    weight_minus1 --> weight_0
    weight_0 --> weight_1

    style J1 fill:#90EE90,stroke:#333
Loading
Prompt To Fix All With AI
This is a comment left during a code review.
Path: charts/model-engine/templates/database_init_job.yaml
Line: 10

Comment:
**Init and migration jobs now run concurrently**

This change correctly ensures the config maps (weight `-2`) are created before the `database_init_job` runs. However, before this PR the ordering was:

- `-2`: config maps **and** `database_init_job` (concurrent, which was itself the bug being fixed)
- `-1`: `database_migration_job`

After this PR, both `database_init_job` and `database_migration_job` share weight `-1`, meaning Helm will submit and run them **concurrently**. If `db.runDbInitScript` and `db.runDbMigrationScript` are ever both enabled in the same release (e.g., a first-install that also needs migrations), the migration job could start before the init job finishes — which could cause the migration to fail against an uninitialized database.

If these two flags are guaranteed to be mutually exclusive, this is fine. If there is any scenario where both could be `true` simultaneously, consider bumping `database_migration_job` to weight `0` to preserve a sequential init → migrate order.

How can I resolve this? If you propose a fix, please make it concise.

Last reviewed commit: 73cda88

Greptile also left 1 inline comment on this PR.

@masbarnes masbarnes requested a review from arniechops March 10, 2026 19:22
annotations:
"helm.sh/hook": pre-install,pre-upgrade
"helm.sh/hook-weight": "-2"
"helm.sh/hook-weight": "-1"
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Init and migration jobs now run concurrently

This change correctly ensures the config maps (weight -2) are created before the database_init_job runs. However, before this PR the ordering was:

  • -2: config maps and database_init_job (concurrent, which was itself the bug being fixed)
  • -1: database_migration_job

After this PR, both database_init_job and database_migration_job share weight -1, meaning Helm will submit and run them concurrently. If db.runDbInitScript and db.runDbMigrationScript are ever both enabled in the same release (e.g., a first-install that also needs migrations), the migration job could start before the init job finishes — which could cause the migration to fail against an uninitialized database.

If these two flags are guaranteed to be mutually exclusive, this is fine. If there is any scenario where both could be true simultaneously, consider bumping database_migration_job to weight 0 to preserve a sequential init → migrate order.

Prompt To Fix With AI
This is a comment left during a code review.
Path: charts/model-engine/templates/database_init_job.yaml
Line: 10

Comment:
**Init and migration jobs now run concurrently**

This change correctly ensures the config maps (weight `-2`) are created before the `database_init_job` runs. However, before this PR the ordering was:

- `-2`: config maps **and** `database_init_job` (concurrent, which was itself the bug being fixed)
- `-1`: `database_migration_job`

After this PR, both `database_init_job` and `database_migration_job` share weight `-1`, meaning Helm will submit and run them **concurrently**. If `db.runDbInitScript` and `db.runDbMigrationScript` are ever both enabled in the same release (e.g., a first-install that also needs migrations), the migration job could start before the init job finishes — which could cause the migration to fail against an uninitialized database.

If these two flags are guaranteed to be mutually exclusive, this is fine. If there is any scenario where both could be `true` simultaneously, consider bumping `database_migration_job` to weight `0` to preserve a sequential init → migrate order.

How can I resolve this? If you propose a fix, please make it concise.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants