Repository navigation
Run migrations before deploying brainstore and service pods - #128
Kevin Green (KevinGreen) wants to merge 11 commits into
Conversation
Kevin Green (KevinGreen)
changed the title
Kevin Green (KevinGreen)
changed the title
…lm-migration-job # Conflicts: # braintrust/templates/ai-gateway-deployment.yaml
Kevin Green (KevinGreen)
changed the title
Kevin Green (KevinGreen)
changed the title
Codex Review Summary: ✅ CompletedThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d460dc908b
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "Codex (@codex) review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "Codex (@codex) address that feedback".
| else: | ||
| # Compatible Pods cannot compensate for an incompatible desired image. | ||
| image = container_image(template.get("spec", {}), container) | ||
| if not compatible(image, minimum): |
There was a problem hiding this comment.
Can we keep ordering working with SHA-pinned Brainstore images? A fully Ready/current rollout with brainstore.image.tag=deadbeef still fails here even with deploymentOrdering.minimumVersion=2.22.0; that override only handles opaque API tags. Turning deploymentOrdering.enabled off also removes migration ordering, so it isn't an equivalent fallback. We hit this with the original gate. Could we cover API and Brainstore SHA pins while keeping sequencing enabled?
| apiVersion: v1 | ||
| kind: ConfigMap | ||
| metadata: | ||
| name: {{ include "braintrust.brainstoreStartupGate.name" . }} |
There was a problem hiding this comment.
Can we keep this ConfigMap available through the upgrade? The old API pods are still serving traffic while the new ones wait for Brainstore. If an old pod needs replacing (for example, after a node is drained), its replacement can't start without this ConfigMap. That means we could lose API capacity before the new pods can take over. Could we keep it available?
| {{- $config := include "braintrust.deploymentOrdering.config" . | fromYaml -}} | ||
| {{- if $config.enabled -}} | ||
| {{- $name := include "braintrust.migrations.name" . -}} | ||
| {{- $api := include "braintrust.apiPoolConfig" (dict "root" . "overrides" (dict)) | fromYaml -}} |
There was a problem hiding this comment.
How should this handle Brainstore newer than a pinned API? API v2.21.0 + Brainstore v2.22.0 renders a v2.21 migration Job, and the rollout gate accepts that combination. But the v2.21 bundle doesn't contain migration 74, which the v2.22 readers needed in the OE incident. Could we select a migration bundle that covers the Brainstore release and add a mixed-version test? Otherwise the Job can succeed while the required schema is still missing.
Summary
Run a schema-migration Job per Helm revision. Brainstore starts after migrations succeed; API pools, AI Gateway, and Loop Runtime start after the full Brainstore rollout completes.
deploymentOrdering.enabled(defaulttrue) controls the entire sequence. When disabled, workloads start without chart-managed ordering.aiGateway.serviceAccount.name), independent of ordering.This orders container startup, not Helm’s updates to Kubernetes objects. See release-ordering configuration and lifecycle.
Validation
./test.sh: 581 Helm tests across 45 suites, 64 Python tests, AWS/Google/Azure/minimal rendering, and strict lint passed.Upgrade caveat
The old API pod emitted
FailedMountwarnings when the legacy gate ConfigMap was deleted. The new release is healthy; old-ReplicaSet replacement recovery was not verified.Resolves INF-869.