feat(gocd): make ST deploys actually split rs/py#7606
Conversation
![sentry[bot]](https://avatars.githubusercontent.com/in/12637?s=60&v=4){kind=small}
| --container-name="metrics-subscriptions-executor" \ | ||
| --container-name="search-issues-consumer" \ | ||
| --container-name="snuba-admin" \ | ||
| --container-name="transactions-subscriptions-consumer" \ | ||
| && k8s-deploy \ | ||
| --label-selector="${LABEL_SELECTOR}" \ | ||
| --image="us-docker.pkg.dev/sentryio/snuba-mr/image:${GO_REVISION_SNUBA_REPO}" \ | ||
| --container-name="snuba" \ | ||
| --container-name="snuba-admin" | ||
| --type="cronjob" \ | ||
| --container-name="optimize" \ | ||
| --container-name="cleanup" \ | ||
| --container-name="cardinality-report" |
There was a problem hiding this comment.
Bug: The deploy-st-py.sh script omits the snuba container, which was present in the original deploy-st.sh and is also in the non-ST deploy-py.sh script.
Severity: CRITICAL | Confidence: High
🔍 Detailed Analysis
The new deploy-st-py.sh script, created by splitting the original deploy-st.sh, fails to deploy the snuba container. The original script and the corresponding non-ST script (deploy-py.sh) both include snuba. This omission means the snuba service will no longer be deployed to single-tenant environments, which is a functional regression and inconsistent with the PR's stated goal of matching existing deployment patterns.
💡 Suggested Fix
Add the snuba container to the k8s-deploy call within deploy-st-py.sh, likely near the end of the first call to follow the pattern in deploy-py.sh.
🤖 Prompt for AI Agent
Review the code at the location below. A potential bug has been identified by an AI
agent.
Verify if this is a real issue. If it is, propose a fix; if not, explain why it's not
valid.
Location: gocd/templates/bash/deploy-st-py.sh#L2-L37
Potential issue: The new `deploy-st-py.sh` script, created by splitting the original
`deploy-st.sh`, fails to deploy the `snuba` container. The original script and the
corresponding non-ST script (`deploy-py.sh`) both include `snuba`. This omission means
the `snuba` service will no longer be deployed to single-tenant environments, which is a
functional regression and inconsistent with the PR's stated goal of matching existing
deployment patterns.
Did we get this right? 👍 / 👎 to inform future reviews.
Reference ID: 7724558
| --container-name="generic-metrics-sets-consumer" \ | ||
| --container-name="loadbalancer-outcomes-consumer" \ | ||
| --container-name="metrics-consumer" \ | ||
| --container-name="outcomes-billing-consumer" \ | ||
| --container-name="outcomes-consumer" \ | ||
| --container-name="profile-chunks-consumer" \ | ||
| --container-name="profiles-consumer" \ | ||
| --container-name="profiling-functions-consumer" \ | ||
| --container-name="querylog-consumer" \ | ||
| --container-name="replays-consumer" \ | ||
| --container-name="transactions-consumer" |
There was a problem hiding this comment.
Bug: The deploy-st-rs.sh script is missing the errors-replacer container, which is present in both the non-ST Rust deployment and the ST Python deployment.
Severity: CRITICAL | Confidence: High
🔍 Detailed Analysis
The deploy-st-rs.sh script omits the errors-replacer container. This service is included in both the non-ST Rust deployment (deploy-rs.sh) and the new single-tenant Python deployment (deploy-st-py.sh). The errors-replacer service handles critical event processing like group merges and deletions. Its absence in single-tenant Rust deployments is an unintentional omission that breaks consistency with other deployment configurations and will lead to a loss of functionality.
💡 Suggested Fix
Add --container-name="errors-replacer" to the k8s-deploy command in deploy-st-rs.sh to match the container list in deploy-rs.sh and deploy-st-py.sh.
🤖 Prompt for AI Agent
Review the code at the location below. A potential bug has been identified by an AI
agent.
Verify if this is a real issue. If it is, propose a fix; if not, explain why it's not
valid.
Location: gocd/templates/bash/deploy-st-rs.sh#L2-L23
Potential issue: The `deploy-st-rs.sh` script omits the `errors-replacer` container.
This service is included in both the non-ST Rust deployment (`deploy-rs.sh`) and the new
single-tenant Python deployment (`deploy-st-py.sh`). The `errors-replacer` service
handles critical event processing like group merges and deletions. Its absence in
single-tenant Rust deployments is an unintentional omission that breaks consistency with
other deployment configurations and will lead to a loss of functionality.
Did we get this right? 👍 / 👎 to inform future reviews.
Reference ID: 7724558
|
revert failed (conflict? already reverted?) -- check the logs |
This didn't work. In STs all containers are named snuba or snuba-admin. s4s2 probably changes this by unifying to the SaaS configuration Reverts #7606
3 participants
This was confusing me a bit when I was rolling out consumer metrics, I think it's useful to split these to get consistent behavior with de/us (even though 99% of the time
pyis going to roll out any changes first)