Repository navigation
Run migrations before deploying brainstore and service pods - #128
Kevin Green (KevinGreen) wants to merge 14 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".
| 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.
| {{- $ordering := include "braintrust.deploymentOrdering.config" $root | fromYaml -}} | ||
| {{- if $ordering.enabled -}} | ||
| {{/* Release identity rejects a previous Ready generation before Helm updates it. */}} | ||
| {{- $_ := set $annotations "braintrust.dev/release-revision" (toString $root.Release.Revision) -}} |
There was a problem hiding this comment.
Question; if we put the release revision in every pod template, a helm upgrade would mean that it would restart all the deployments. Is that the intended/acceptable behavior? I wanted to ask because changing this doesn't really require a migration but would run a fresh migration anyway.
There was a problem hiding this comment.
Question; following up on the upgrade note at the end of the PR description, since this is mounted by the existing replica set of API pods and is required, and the new API pods are waiting on the brainstore rollout. In this window, any old API pods that get rescheduled won't be able to come up since the config map has been deleted already. Could we keep this around for another revision or is this not something that's a big enough problem?
There was a problem hiding this comment.
that's a good point, there's a chance it could be a problem so I added this back
| # Disable to deploy everything without chart-managed startup ordering. | ||
| # See README.md#release-ordering for configuration and operational limits. | ||
| deploymentOrdering: | ||
| enabled: true |
There was a problem hiding this comment.
Question; for customers who have set the old key to true, does this change silently make that key a no-op? More specifically, they didn't change their values.yaml but they get an error saying that the render failed? This is probably somewhat narrow since customers who have specifically opted out of the startup gate aren't likely very many but for those customers, the startup gate stops working and they get onboarded directly into the ordering. This is potentially just a behavior change worth calling out in the readme/release notes.
Summary
Order changed workloads as migrations → Brainstore → API pools, AI Gateway, and Loop Runtime.
deploymentOrdering.enabled(defaulttrue) controls the whole sequence; unchanged workloads keep running.migrationImageand stableminimumVersionassertion.This gates container startup, not Kubernetes object updates. The removed
api.brainstoreStartupGatesetting is ignored; existing opt-outs must setdeploymentOrdering.enabled=false. See README configuration and lifecycle.Input coverage
v, digest pinsValidation
Resolves INF-869.