Repository navigation
update example to use role label instead of name - #101
Jeff McCollum (jeffmccollum) wants to merge 6 commits into
Conversation
Codex Review SummaryThis 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: 49807d61b1
ℹ️ 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".
|
|
||
| For Standard mode clusters, create node pools with local SSDs, then deploy: | ||
|
|
||
| Use the stable `braintrust/node-pool` role label in each node selector. Terraform node pool replacements can use generated GKE pool names. |
There was a problem hiding this comment.
For manually created GKE Standard pools or clusters produced by a Terraform module version predating the referenced module change, no node has the custom braintrust/node-pool label—GKE only supplies the former cloud.google.com/gke-nodepool label automatically. The new selectors therefore leave the API and every Brainstore pod Pending, while this section only tells users to create pools and configure selectors. Document the compatible Terraform module version and how to label manually created pools before directing users to these selectors.
Useful? React with 👍 / 👎.
|
Codex (@codex) review |
|
Codex Review: Didn't find any major issues. Keep them coming! Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
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". |
The Terraform module sets braintrust/node-pool to each pool's map key, so a Brainstore replacement pool keyed differently never matched the brainstore selector and Brainstore pods could not move to it. braintrust/workload is shared by every pool that serves a workload, which is what the Terraform module's README tells Helm to select on. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
`helm upgrade --reuse-values` from a chart without Brainstore budgets left `podDisruptionBudget` unset, and the template failed to render on every cloud. Treat a missing or null budget map as disabled, and default a missing or null `maxUnavailable` to 1 while keeping an explicit 0. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
| {{- toYaml . | nindent 4 }} | ||
| {{- end }} | ||
| spec: | ||
| {{- if hasKey $api.podDisruptionBudget "maxUnavailable" }} |
There was a problem hiding this comment.
if we have
podDisruptionBudget:
enabled: true
minAvailable: 2
maxUnavailable: nullthen we'll now render maxUnavailable: null with no minAvailable which is unexpected.
We could perhaps normalize it like in Brainstore first and then do something like
{{- $api := $pool.config -}}
-{{- if $api.podDisruptionBudget.enabled }}
+{{- $pdb := $api.podDisruptionBudget | default dict -}}
+{{- if $pdb.enabled }}
- {{- if hasKey $api.podDisruptionBudget "maxUnavailable" }}
- maxUnavailable: {{ $api.podDisruptionBudget.maxUnavailable | toYaml | trim }}
+ {{- if ne $pdb.maxUnavailable nil }}
+ maxUnavailable: {{ $pdb.maxUnavailable | toYaml | trim }}
{{- else }}
- minAvailable: {{ $api.podDisruptionBudget.minAvailable }}
+ minAvailable: {{ $pdb.minAvailable }}
{{- end }}
There was a problem hiding this comment.
Agreed—fixed in bd65df5 using the suggested normalization and nil check. A null maximum falls back to minAvailable, including when an isolated pool clears an inherited maximum; explicit zero still takes precedence. Missing PDB maps also render safely. Added regression coverage and updated the input matrix. The full chart test script passes.
terraform-google-braintrust-data-plane#27 labels Standard nodes with
braintrust/workload=servicesorbraintrust/workload=brainstore. GKE names those pools with a generated suffix, so the example selectorscloud.google.com/gke-nodepool: apiand: brainstorematch nothing and the pods stay Pending.This points the Standard example at
braintrust/workload. A replacement pool keeps that label when its Terraform key is different.#27 drains the old pool while respecting PDBs. The example sets
maxUnavailable: 1on the API and the Brainstore reader, fast reader, and writer. Chart defaults leave those budgets off.Merge this with or before #27. That module's Standard example links this values file on main.
Brainstore PDB inputs
Applies to each of
brainstore.reader,fastreader, andwriter, on AWS, Azure, and Google:podDisruptionBudgetenabled: false)null(--reuse-valuesfrom an older chart)enabled: true,maxUnavailablemissing ornullmaxUnavailable: 1enabled: true,maxUnavailable: 0maxUnavailable: 0enabled: true,maxUnavailable: "25%"maxUnavailable: "25%"Each row has a helm-unittest case in
brainstore-pdb_test.yaml.API PDB inputs
Applies to the default API pool and each enabled isolated pool on AWS, Azure, and Google:
podDisruptionBudgetenabled: false, including when a maximum is setnullmap in retained valuesenabled: true,maxUnavailablemissing ornullminAvailableenabled: true,maxUnavailable: 0maxUnavailable: 0; omitminAvailableenabled: true,maxUnavailable: 1maxUnavailable: 1; omitminAvailableenabled: true,maxUnavailable: "25%"minAvailableenabled: true,maxUnavailable: ""nullminAvailableCovered in
brainstore-pdb_test.yaml, including partial overrides and retained values.