In order to reduce errors caused by improper ordering of container shutdown within MW-on-K8s pods (request in flight within MediaWiki reaching out to the mesh when the mesh container is already killed), we need to implement native sidecar containers
Description
Details
- Other Assignee
- jasmine_
| Status | Subtype | Assigned | Task | ||
|---|---|---|---|---|---|
| Open | Blake | T417800 Implement proper sidecar container support in mediawiki pods | |||
| Open | BUG REPORT | None | T364245 Recentchanges and cu_changes tables are occasionally missing revisions on multiple wikis |
Event Timeline
@Clement_Goubert @Scott_French checking in with you two, our bandwidth seems constrained again for next quarter. What's your assessment of the risk of keeping the temporary measures in place again next quarter?
We should also get a sense of how much effort we need to book for this project.
My impression is that the configuration we arrived at in T364245 is not more precarious than what we've had historically (manually configured preStop hook timers), while substantially addressing the underlying issues (by adding inbound listener drains and reordering app-container vs. envoy shutdown).
Thus, although it's desirable to prioritize this [0] in order to simplify shutdown sequencing and unify what we do between Jobs and traffic-serving workloads, my thinking is that it's not the end of the world if need to defer further given everything else happening.
That said, if this is particularly low-effort, then I wonder if that might tip the balance toward actually getting it done. @Clement_Goubert - I don't have a great mental model for that (e.g., how brittle the relevant mediawiki chart bits are), but I know you've thought about this a fair bit more than I have. Thoughts?
[0] Ideally together with the work in T427024 to clean up the way drains are configured.
The implementation work is fairly straightforward, basically anything that isn't the MediaWiki pod goes into the initContainers section with the relevant config. There's some changes to make to the container templates in the vendored modules to accept setting the restartPolicy, and maybe a little testing for ordering?
There's an incidental free win with implementing it, which is that image changes to sidecar containers do not trigger a full pod redeploy, just a container restart.
I don't think there's any particular risk in keeping our current config one more quarter, but I'd love not to punt this down the road for ages either. I estimate the effort at around 3 FTE days of actual work, including testing, without baking in possible unforeseen consequences delays or anything like canary soaking.
Needs attention to T427024: Move MediaWiki envoy drain configuration from mesh.extra_env to mesh.admin when implementing.
Linking T386694: Replace k8s-controller-sidecars with built in Sidecar containers on k8s 1.31 here since it's adjacent/similar work
Just adding a couple of additional notes for when we pick this up:
Adopting sidecar container support should allow us to drop the 8s-sleep preStop hooks on the sidecars that have it (i.e., other than envoy, which instead invokes the drain tool, and can arguably lose the DRAIN_WAIT_MINIMUM_S), since MediaWiki app container lifetime becomes authoritative for triggering their termination.
However, we may want to give some thought to what we do in the MediaWiki app container. IMO, while we could potentially do something more sophisticated with PHP-FPM shutdown, we don't want to remove the sleep preStop hook there initially - i.e., that's what's load-bearing in terms of allowing clients to observe the effect of the drain before PHP-FPM stops accepting new requests (IIUC, on SIGTERM) [0].
In any case, just wanted to mention this, as I'm not sure what folks have in mind, and cross-link T364245#11741538, which has some examples of how we can arrive at surprising sequencing given our combination of the termination grace period and PHP-FPM termination behavior even now.
[0] Example: if we didn't have a sleep there at all, and there were long-running requests that kept PHP-FPM workers - and thus the pod - alive, that would partially "undo" the effect of the graceful drain (i.e., new requests coming in would just immediately get 503'd by apache).
Change #1313111 had a related patch set uploaded (by Blake; author: Blake):
[operations/deployment-charts@master] mediawiki: Refactor lamp.deployment into component containers.
Change #1325883 had a related patch set uploaded (by Blake; author: Blake):
[operations/deployment-charts@master] mediawiki: optionally use initContainers for sidecars.
Change #1325884 had a related patch set uploaded (by Blake; author: Blake):
[operations/deployment-charts@master] mediawiki: stop using lamp.deployment in job.yaml.tpl.
Change #1313111 merged by jenkins-bot:
[operations/deployment-charts@master] mediawiki: Refactor lamp.deployment into component containers.
Mentioned in SAL (#wikimedia-operations) [2026-08-18T10:14:34Z] <blake@deploy1003> Started scap sync-world: no-build deployment for T417800
Mentioned in SAL (#wikimedia-operations) [2026-08-18T10:17:49Z] <blake@deploy1003> Finished scap sync-world: no-build deployment for T417800 (duration: 04m 40s)
Change #1325884 merged by jenkins-bot:
[operations/deployment-charts@master] mediawiki: stop using lamp.deployment in job.yaml.tpl.
Mentioned in SAL (#wikimedia-operations) [2026-08-19T12:35:07Z] <blake@deploy1003> Started scap sync-world: non-build deploy for T417800
Mentioned in SAL (#wikimedia-operations) [2026-08-19T12:38:22Z] <blake@deploy1003> Finished scap sync-world: non-build deploy for T417800 (duration: 03m 52s)
Change #1325883 merged by jenkins-bot:
[operations/deployment-charts@master] mediawiki: optionally use initContainers for sidecars.
Mentioned in SAL (#wikimedia-operations) [2026-09-02T10:03:38Z] <blake@deploy1003> Started scap sync-world: non-build deployment for T417800
Mentioned in SAL (#wikimedia-operations) [2026-09-02T10:08:08Z] <blake@deploy1003> Finished scap sync-world: non-build deployment for T417800 (duration: 05m 37s)
Change #1333837 had a related patch set uploaded (by Blake; author: Blake):
[operations/deployment-charts@master] sidecars: Specify restartPolicy: Always.
Taking another look at this after being significantly sidetracked by other work.
First of all, there was a piece I missed in the lamp.deployment refactor: restartPolicy needs to be set for these containers in order for them to be considered sidecars. That oversight should be fixable directly, but the k8s documentation got me thinking.
Particularly, this bit: Upon Pod termination, the kubelet postpones terminating sidecar containers until the main application container has fully stopped. The sidecar containers are then shut down in the opposite order of their appearance in the Pod specification.
We should, then, be able to use the order of the initContainers to set the order in which containers are taken down. That does mean, though, that we'll need to bring mesh.deployment.container into initContainers (because we want to ensure Envoy is running until the main application has stopped). If we put mesh.deployment.container at the top of the initContainers list, it will be the last container to shut down. I think that means that we'll want a version of mesh.deployment.container which has its restartPolicy set to always.
So, I think the place to start is by adding a way to set restartPolicy to Always in mesh.deployment.container. Then, we change the native_sidecar if block of charts/mediawiki/templates/deployment.yaml.tpl to include a sidecar'd version of the Envoy container. Then, we test, and consider removing the pre-stop sleeps for everything that isn't Envoy.
Change #1333855 had a related patch set uploaded (by Blake; author: Blake):
[operations/deployment-charts@master] mcrouter: update to 1.3.6 (copy)
Change #1333856 had a related patch set uploaded (by Blake; author: Blake):
[operations/deployment-charts@master] mcrouter: Upgrade to 1.3.6.
Change #1334012 had a related patch set uploaded (by Blake; author: Blake):
[operations/deployment-charts@master] httpd: Add version 1.0.3 (copy)
Change #1334013 had a related patch set uploaded (by Blake; author: Blake):
[operations/deployment-charts@master] httpd: Add switch to enable Kubernetes native sidecar behaviour.
Change #1334015 had a related patch set uploaded (by Blake; author: Blake):
[operations/deployment-charts@master] php-fpm: Add version 1.0.2 (copy)
Change #1334016 had a related patch set uploaded (by Blake; author: Blake):
[operations/deployment-charts@master] php-fpm: Make php-fpm-exporter a sidecar.
Change #1334735 had a related patch set uploaded (by Blake; author: Blake):
[operations/deployment-charts@master] localmemcached: Add restart policy if we should be a sidecar.
Change #1334738 had a related patch set uploaded (by Blake; author: Blake):
[operations/deployment-charts@master] statsd: Upgrade to 1.0.5 (copy)
Change #1334739 had a related patch set uploaded (by Blake; author: Blake):
[operations/deployment-charts@master] statsd: Enable a native k8s sidecar option.
Change #1334741 had a related patch set uploaded (by Blake; author: Blake):
[operations/deployment-charts@master] mesh: upgrade to 1.3.3 (copy)
Change #1334742 had a related patch set uploaded (by Blake; author: Blake):
[operations/deployment-charts@master] mesh: Add switch to enable k8s native sidecar behaviour.
When this proves functional, we should elevate this pattern into the standard mesh model and with that to all other charts. I think that would be a really nice reliability addition that might remove some seen or unseen issues with other services - especially during deployments.
Change #1334741 merged by jenkins-bot:
[operations/deployment-charts@master] mesh: upgrade to 1.3.3 (copy)
Change #1334742 merged by jenkins-bot:
[operations/deployment-charts@master] mesh: Add switch to enable k8s native sidecar behaviour.
Change #1334738 merged by jenkins-bot:
[operations/deployment-charts@master] statsd: Upgrade to 1.0.5 (copy)
Change #1334739 merged by jenkins-bot:
[operations/deployment-charts@master] statsd: Enable a native k8s sidecar option.
Change #1334012 merged by jenkins-bot:
[operations/deployment-charts@master] httpd: Add version 1.0.3 (copy)
Change #1334013 merged by jenkins-bot:
[operations/deployment-charts@master] httpd: Add switch to enable Kubernetes native sidecar behaviour.
Change #1334015 merged by jenkins-bot:
[operations/deployment-charts@master] php-fpm: Add version 1.0.2 (copy)
Change #1334016 merged by jenkins-bot:
[operations/deployment-charts@master] php-fpm: Make php-fpm-exporter a sidecar.
Change #1333855 merged by jenkins-bot:
[operations/deployment-charts@master] mcrouter: update to 1.3.6 (copy)
Change #1333856 merged by jenkins-bot:
[operations/deployment-charts@master] mcrouter: Upgrade to 1.3.6.
Change #1338254 had a related patch set uploaded (by Blake; author: Blake):
[operations/deployment-charts@master] mediawiki: stop using lamp.deployment in cronjob.
Change #1338227 had a related patch set uploaded (by Blake; author: Blake):
[operations/deployment-charts@master] mediawiki: stop using lamp.deployment in job.
Change #1338254 abandoned by Blake:
[operations/deployment-charts@master] mediawiki: stop using lamp.deployment in cronjob.
Reason:
subsumed by Icfd20a7298905315799ffa6219dcb5e16a6a6964
Giving this a test with a checkout of the patch:
⏳ Starting Version.php on Kubernetes as job mw-script.eqiad.mpmu1d84 ... 🚀 Job is running. For streaming logs, run: K8S_CLUSTER=eqiad KUBECONFIG=/etc/kubernetes/mw-script-eqiad.config kubectl logs -f job/mw-script.eqiad.mpmu1d84 mediawiki-mpmu1d84-app blake@deploy1003:~/deployment-charts$ K8S_CLUSTER=eqiad KUBECONFIG=/etc/kubernetes/mw-script-eqiad.config kubectl logs -f job/mw-script.eqiad.mpmu1d84 mediawiki-mpmu1d84-app MediaWiki version: 1.47.0-wmf.20 LTS (built: 07:07, 16 September 2026)
And, if I'm understanding this correctly, we do have a diff in mw-cron with the new chart, but the diff appears to be exclusively whitespace changes:
Comparing release=main, chart=/home/blake/deployment-charts/charts/mediawiki, namespace=mw-cron
mw-cron, abstractwiki-update-generated-articles, CronJob (batch) has changed:
...
kind: CronJob
metadata:
name: abstractwiki-update-generated-articles
labels:
app: mediawiki
- chart: mediawiki-0.13.3
+ chart: mediawiki-0.13.4
release: main
heritage: Helm
team: "abstract-wikipedia"
script: "UpdateAbstractWikiArticleStore.php-weekly-refresh"
...
spec:
terminationGracePeriodSeconds: 10
containers:
# When adding or removing containers, also update the pod.kubernetes.io/sidecars ann
otation
# above.
-
### The MediaWiki container
- name: mediawiki-main-app
image: docker-registry.discovery.wmnet/restricted/mediawiki-multiversion-cli:2026-
09-16-081606-publish-83
imagePullPolicy: IfNotPresent
# MediaWiki cronjobs may require a tty to run, as a first approximation make it al
ways true.
...
- name: mediawiki-main-php-envvars
mountPath: /etc/php/8.3/fpm/env
readOnly: true
- name: mediawiki-main-cron-captcha
mountPath: /etc/fancycaptcha/
- readOnly: true
- # Add the following exporters:
-
+ readOnly: true
- name: mediawiki-main-tls-proxy
image: docker-registry.discovery.wmnet/envoy:1.39.0-1
imagePullPolicy: IfNotPresent
env:
- name: SERVICE_NAME(and an additional copy of this same diff for each other cronjob)
Change #1338227 merged by jenkins-bot:
[operations/deployment-charts@master] mediawiki: stop using lamp.deployment.
Change #1342304 had a related patch set uploaded (by Blake; author: Blake):
[operations/deployment-charts@master] mediawiki: remove lamp.deployment.
Change #1342304 merged by jenkins-bot:
[operations/deployment-charts@master] mediawiki: remove lamp.deployment.
Mentioned in SAL (#wikimedia-operations) [2026-09-17T10:07:26Z] <blake@deploy1003> Started scap sync-world: cleanup for T417800
Mentioned in SAL (#wikimedia-operations) [2026-09-17T10:10:56Z] <blake@deploy1003> Finished scap sync-world: cleanup for T417800 (duration: 03m 57s)
Change #1333837 merged by jenkins-bot:
[operations/deployment-charts@master] sidecars: enable a restartPolicy: Always option.
Mentioned in SAL (#wikimedia-operations) [2026-09-29T10:33:33Z] <blake@deploy1003> Started scap sync-world: add sidecar flag for T417800
Mentioned in SAL (#wikimedia-operations) [2026-09-29T10:37:14Z] <blake@deploy1003> Finished scap sync-world: add sidecar flag for T417800 (duration: 04m 14s)
Change #1346489 had a related patch set uploaded (by Blake; author: Blake):
[operations/deployment-charts@master] mw-experimental: enable native k8s sidecars.
Change #1346502 had a related patch set uploaded (by Blake; author: Blake):
[operations/deployment-charts@master] sidecars: do not add a pre-stop sleep if we're a native sidecar
Change #1346489 abandoned by Blake:
[operations/deployment-charts@master] mw-experimental: enable native k8s sidecars.
Reason:
other work needed first
Change #1346536 had a related patch set uploaded (by Blake; author: Blake):
[operations/deployment-charts@master] mw deployment: move mesh sidecar container to the top
Change #1346536 abandoned by Blake:
[operations/deployment-charts@master] mw deployment: move mesh sidecar container to the top
Reason:
subsumed by I5510edd6540854d407b689a9fe4343cf6a6a6964
Change #1346502 merged by jenkins-bot:
[operations/deployment-charts@master] sidecars: remove pre-stop sleep, stop envoy last
Mentioned in SAL (#wikimedia-operations) [2026-10-05T10:05:51Z] <blake@deploy1003> Started scap sync-world: helm-only deploy for T417800
Mentioned in SAL (#wikimedia-operations) [2026-10-05T10:14:58Z] <blake@deploy1003> Finished scap sync-world: helm-only deploy for T417800 (duration: 09m 40s)
Change #1333740 had a related patch set uploaded (by Blake; author: Blake):
[operations/deployment-charts@master] mw-experimental: enable native_sidecars on mw-experimental
Change #1333740 merged by jenkins-bot:
[operations/deployment-charts@master] mw-experimental: enable native_sidecars on mw-experimental
Change #1352466 had a related patch set uploaded (by Blake; author: Blake):
[operations/deployment-charts@master] mw-experimental: declare mesh as a native k8s sidecar
Tried to deploy this to mw-experimental, and it looks like I've missed a step. I think, because deployment-charts/charts/mediawiki/templates/vendor/mesh still contains deployment_1.3.2.tpl, we haven't actually picked up the change that would allow us to enable native sidecars for Envoy. So, we'll need to do that, and then submit https://gerrit.wikimedia.org/r/c/operations/deployment-charts/+/1352466.
Change #1352468 had a related patch set uploaded (by Blake; author: Blake):
[operations/deployment-charts@master] mediawiki: update module versions to support native sidecars
Change #1352468 merged by jenkins-bot:
[operations/deployment-charts@master] mediawiki: update module versions to support native sidecars
Mentioned in SAL (#wikimedia-operations) [2026-10-07T10:33:24Z] <blake@deploy1003> Started scap sync-world: update vendored modules for T417800
Mentioned in SAL (#wikimedia-operations) [2026-10-07T10:39:55Z] <blake@deploy1003> Finished scap sync-world: update vendored modules for T417800 (duration: 07m 32s)
Change #1352466 merged by jenkins-bot:
[operations/deployment-charts@master] mw-experimental: declare mesh, mcrouter as native k8s sidecars
Change #1334735 merged by jenkins-bot:
[operations/deployment-charts@master] localmemcached,rsyslog: Add restart policy if we should be a sidecar.
Mentioned in SAL (#wikimedia-operations) [2026-10-07T16:17:19Z] <blake@deploy1003> Started scap sync-world: add restart policies for T417800
Mentioned in SAL (#wikimedia-operations) [2026-10-07T16:21:14Z] <blake@deploy1003> Finished scap sync-world: add restart policies for T417800 (duration: 05m 23s)
Change #1352571 had a related patch set uploaded (by Blake; author: Blake):
[operations/deployment-charts@master] mcrouter: update to 1.3.7 (copy)
Change #1352572 had a related patch set uploaded (by Blake; author: Blake):
[operations/deployment-charts@master] mcrouter: Add native sidecar capabilities to the mcrouter exporter