-
-
Notifications
You must be signed in to change notification settings - Fork 60
feat(gocd): make ST deploys actually split rs/py #7606
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Conversation
| --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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
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)