Page MenuHomePhabricator

Implement proper sidecar container support in mediawiki pods
Open, HighPublic

Description

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

Details

Other Assignee
jasmine_
Related Changes in Gerrit:
SubjectAuthorRepoBranchLines +/-
Blakeoperations/deployment-chartsmaster+3 -0
Blakeoperations/deployment-chartsmaster+528 -0
Blakeoperations/deployment-chartsmaster+9 -1
Blakeoperations/deployment-chartsmaster+5 -0
Blakeoperations/deployment-chartsmaster+14 -5
Blakeoperations/deployment-chartsmaster+4 -0
Blakeoperations/deployment-chartsmaster+8 -8
Blakeoperations/deployment-chartsmaster+1 -1
Blakeoperations/deployment-chartsmaster+4 -0
Blakeoperations/deployment-chartsmaster+15 -1
Blakeoperations/deployment-chartsmaster+2 -401
Blakeoperations/deployment-chartsmaster+7 -4
Blakeoperations/deployment-chartsmaster+2 -2
Blakeoperations/deployment-chartsmaster+6 -0
Blakeoperations/deployment-chartsmaster+525 -0
Blakeoperations/deployment-chartsmaster+3 -0
Blakeoperations/deployment-chartsmaster+93 -0
Blakeoperations/deployment-chartsmaster+9 -0
Blakeoperations/deployment-chartsmaster+99 -0
Blakeoperations/deployment-chartsmaster+6 -0
Blakeoperations/deployment-chartsmaster+88 -0
Blakeoperations/deployment-chartsmaster+6 -0
Blakeoperations/deployment-chartsmaster+83 -0
Blakeoperations/deployment-chartsmaster+23 -1
Blakeoperations/deployment-chartsmaster+5 -2
Blakeoperations/deployment-chartsmaster+414 -2
Show related patches Customize query in gerrit

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes

We're at capacity for this quarter but we will consider this work for next quarter

@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.

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).

MLechvien-WMF moved this task from Inbox to Scheduled (this Q) on the ServiceOps board.
MLechvien-WMF updated Other Assignee, added: jasmine_.

Change #1313111 had a related patch set uploaded (by Blake; author: Blake):

[operations/deployment-charts@master] mediawiki: Refactor lamp.deployment into component containers.

https://gerrit.wikimedia.org/r/1313111

Change #1325883 had a related patch set uploaded (by Blake; author: Blake):

[operations/deployment-charts@master] mediawiki: optionally use initContainers for sidecars.

https://gerrit.wikimedia.org/r/1325883

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.

https://gerrit.wikimedia.org/r/1325884

Change #1313111 merged by jenkins-bot:

[operations/deployment-charts@master] mediawiki: Refactor lamp.deployment into component containers.

https://gerrit.wikimedia.org/r/1313111

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.

https://gerrit.wikimedia.org/r/1325884

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.

https://gerrit.wikimedia.org/r/1325883

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.

https://gerrit.wikimedia.org/r/1333837

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)

https://gerrit.wikimedia.org/r/1333855

Change #1333856 had a related patch set uploaded (by Blake; author: Blake):

[operations/deployment-charts@master] mcrouter: Upgrade to 1.3.6.

https://gerrit.wikimedia.org/r/1333856

Change #1334012 had a related patch set uploaded (by Blake; author: Blake):

[operations/deployment-charts@master] httpd: Add version 1.0.3 (copy)

https://gerrit.wikimedia.org/r/1334012

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.

https://gerrit.wikimedia.org/r/1334013

Change #1334015 had a related patch set uploaded (by Blake; author: Blake):

[operations/deployment-charts@master] php-fpm: Add version 1.0.2 (copy)

https://gerrit.wikimedia.org/r/1334015

Change #1334016 had a related patch set uploaded (by Blake; author: Blake):

[operations/deployment-charts@master] php-fpm: Make php-fpm-exporter a sidecar.

https://gerrit.wikimedia.org/r/1334016

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.

https://gerrit.wikimedia.org/r/1334735

Change #1334738 had a related patch set uploaded (by Blake; author: Blake):

[operations/deployment-charts@master] statsd: Upgrade to 1.0.5 (copy)

https://gerrit.wikimedia.org/r/1334738

Change #1334739 had a related patch set uploaded (by Blake; author: Blake):

[operations/deployment-charts@master] statsd: Enable a native k8s sidecar option.

https://gerrit.wikimedia.org/r/1334739

Change #1334741 had a related patch set uploaded (by Blake; author: Blake):

[operations/deployment-charts@master] mesh: upgrade to 1.3.3 (copy)

https://gerrit.wikimedia.org/r/1334741

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.

https://gerrit.wikimedia.org/r/1334742

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.

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)

https://gerrit.wikimedia.org/r/1334741

Change #1334742 merged by jenkins-bot:

[operations/deployment-charts@master] mesh: Add switch to enable k8s native sidecar behaviour.

https://gerrit.wikimedia.org/r/1334742

Change #1334738 merged by jenkins-bot:

[operations/deployment-charts@master] statsd: Upgrade to 1.0.5 (copy)

https://gerrit.wikimedia.org/r/1334738

Change #1334739 merged by jenkins-bot:

[operations/deployment-charts@master] statsd: Enable a native k8s sidecar option.

https://gerrit.wikimedia.org/r/1334739

Change #1334012 merged by jenkins-bot:

[operations/deployment-charts@master] httpd: Add version 1.0.3 (copy)

https://gerrit.wikimedia.org/r/1334012

Change #1334013 merged by jenkins-bot:

[operations/deployment-charts@master] httpd: Add switch to enable Kubernetes native sidecar behaviour.

https://gerrit.wikimedia.org/r/1334013

Change #1334015 merged by jenkins-bot:

[operations/deployment-charts@master] php-fpm: Add version 1.0.2 (copy)

https://gerrit.wikimedia.org/r/1334015

Change #1334016 merged by jenkins-bot:

[operations/deployment-charts@master] php-fpm: Make php-fpm-exporter a sidecar.

https://gerrit.wikimedia.org/r/1334016

Change #1333855 merged by jenkins-bot:

[operations/deployment-charts@master] mcrouter: update to 1.3.6 (copy)

https://gerrit.wikimedia.org/r/1333855

Change #1333856 merged by jenkins-bot:

[operations/deployment-charts@master] mcrouter: Upgrade to 1.3.6.

https://gerrit.wikimedia.org/r/1333856

Change #1338254 had a related patch set uploaded (by Blake; author: Blake):

[operations/deployment-charts@master] mediawiki: stop using lamp.deployment in cronjob.

https://gerrit.wikimedia.org/r/1338254

Change #1338227 had a related patch set uploaded (by Blake; author: Blake):

[operations/deployment-charts@master] mediawiki: stop using lamp.deployment in job.

https://gerrit.wikimedia.org/r/1338227

Change #1338254 abandoned by Blake:

[operations/deployment-charts@master] mediawiki: stop using lamp.deployment in cronjob.

Reason:

subsumed by Icfd20a7298905315799ffa6219dcb5e16a6a6964

https://gerrit.wikimedia.org/r/1338254

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.

https://gerrit.wikimedia.org/r/1338227

Change #1342304 had a related patch set uploaded (by Blake; author: Blake):

[operations/deployment-charts@master] mediawiki: remove lamp.deployment.

https://gerrit.wikimedia.org/r/1342304

Change #1342304 merged by jenkins-bot:

[operations/deployment-charts@master] mediawiki: remove lamp.deployment.

https://gerrit.wikimedia.org/r/1342304

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.

https://gerrit.wikimedia.org/r/1333837

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.

https://gerrit.wikimedia.org/r/1346489

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

https://gerrit.wikimedia.org/r/1346502

Change #1346489 abandoned by Blake:

[operations/deployment-charts@master] mw-experimental: enable native k8s sidecars.

Reason:

other work needed first

https://gerrit.wikimedia.org/r/1346489

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

https://gerrit.wikimedia.org/r/1346536

Change #1346536 abandoned by Blake:

[operations/deployment-charts@master] mw deployment: move mesh sidecar container to the top

Reason:

subsumed by I5510edd6540854d407b689a9fe4343cf6a6a6964

https://gerrit.wikimedia.org/r/1346536

Change #1346502 merged by jenkins-bot:

[operations/deployment-charts@master] sidecars: remove pre-stop sleep, stop envoy last

https://gerrit.wikimedia.org/r/1346502

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

https://gerrit.wikimedia.org/r/1333740

Change #1333740 merged by jenkins-bot:

[operations/deployment-charts@master] mw-experimental: enable native_sidecars on mw-experimental

https://gerrit.wikimedia.org/r/1333740

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

https://gerrit.wikimedia.org/r/1352466

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

https://gerrit.wikimedia.org/r/1352468

Change #1352468 merged by jenkins-bot:

[operations/deployment-charts@master] mediawiki: update module versions to support native sidecars

https://gerrit.wikimedia.org/r/1352468

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

https://gerrit.wikimedia.org/r/1352466

Change #1334735 merged by jenkins-bot:

[operations/deployment-charts@master] localmemcached,rsyslog: Add restart policy if we should be a sidecar.

https://gerrit.wikimedia.org/r/1334735

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)

https://gerrit.wikimedia.org/r/1352571

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

https://gerrit.wikimedia.org/r/1352572