Skip to content

Replace Engine with Worker - #34

Open
MarceloRGonc wants to merge 9 commits into
mainfrom
mg/OPS-4456
Open

Replace Engine with Worker#34
MarceloRGonc wants to merge 9 commits into
mainfrom
mg/OPS-4456

Conversation

@MarceloRGonc

Copy link
Copy Markdown
Contributor

Part of OPS-4456

Copilot AI review requested due to automatic review settings June 5, 2026 16:46
@linear

linear Bot commented Jun 5, 2026

Copy link
Copy Markdown

OPS-4456

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR updates the Helm chart to replace the “engine” component with a “worker” component, including renaming values keys and updating templates to deploy and wire the worker service.

Changes:

  • Rename values/config from engine to worker (including production/CI overlays and schema).
  • Update Kubernetes manifests (Deployment/Service/HPA/PDB/ServiceMonitor/NetworkPolicy/ServiceAccount) to target the worker component and port 3000.
  • Replace OPS_ENGINE_URL with OPS_WORKER_URL and add OPS_CONTAINER_TYPE env var(s).

Reviewed changes

Copilot reviewed 15 out of 15 changed files in this pull request and generated no comments.

Show a summary per file
File Description
chart/values.yaml Renames engine config/env to worker and updates ingress comment reference.
chart/values.schema.json Renames schema section from engine to worker and updates description text in that section.
chart/values.production.yaml Updates production overlay key from engine to worker.
chart/values.ci.yaml Updates CI overlay key from engine to worker.
chart/templates/servicemonitor.yaml Renames the engine ServiceMonitor to worker and points to worker metrics path.
chart/templates/serviceaccount-worker.yaml Switches ServiceAccount creation/labels from engine to worker.
chart/templates/service-worker.yaml Renames service to worker and changes service port/targetPort to 3000.
chart/templates/secret-env.yaml Updates secret auto-generation env sources to include .Values.worker.env instead of engine.
chart/templates/pdb-worker.yaml Switches PDB values/labels from engine to worker.
chart/templates/networkpolicy.yaml Updates network policies to reference worker component and port 3000.
chart/templates/hpa-worker.yaml Switches HPA target/values from engine to worker.
chart/templates/external-secret.yaml Updates external secret env aggregation from engine to worker.
chart/templates/deployment-worker.yaml Renames deployment/image/env wiring from engine to worker; updates container port to 3000 and adds OPS_CONTAINER_TYPE=WORKER.
chart/templates/deployment-app.yaml Adds OPS_CONTAINER_TYPE=APP to the app deployment.
chart/templates/_helpers.tpl Replaces openops.engineServiceUrl with openops.workerServiceUrl and updates port to 3000.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

MarceloRGonc and others added 2 commits June 29, 2026 09:06
Every PDB in the chart set minAvailable, while analytics and tables default
to a single replica. minAvailable: 1 against one replica evaluates to
disruptionsAllowed: 0, so those pods cannot be evicted and every voluntary
disruption fails: cluster upgrades, node image upgrades and autoscaler
scale-down all stall while it is set.

This is not theoretical. It left an Azure AKS estate on three-month-old node
images across four clusters, with the pools reporting Failed and nothing
alerting on it, because the drain could never complete.

maxUnavailable: 1 is equivalent at two replicas and drainable at one, so all
five components now default to it rather than only fixing the two that
deadlock today. Overriding replicas down to 1 is a supported thing to do and
should not reintroduce the deadlock.

minAvailable still works when maxUnavailable is unset, so existing overrides
are unaffected. A PDB may not set both; maxUnavailable takes precedence.

One behaviour change to note: at three or more replicas maxUnavailable: 1
permits one pod down at a time where minAvailable: 1 permitted all but one.
That is safer but makes drains slower.

tables uses ReadWriteOnce storage and cannot be scaled past one replica, so
it still incurs brief downtime while its node drains. This makes the drain
possible, not seamless.

Verified by rendering the chart: all five PDBs emit maxUnavailable: 1 with
default values, and an override of maxUnavailable: null with minAvailable: 2
still emits minAvailable: 2. helm lint and both CI template steps pass. Note
that CI never exercises this path, since values.ci.yaml disables PDBs.

Part of OPS-4725
@MarceloRGonc
MarceloRGonc changed the base branch from main to mg/pdb-maxunavailable August 13, 2026 11:29
The template guarded on truthiness, and Go templates treat 0 as false, so
maxUnavailable: 0 fell through to rendering minAvailable: 1 — silently
producing a PDB that differs from the values that asked for it. 0 is a valid
PodDisruptionBudget value.

kindIs "invalid" tests for nil instead, which keeps all four cases correct:
an explicit 0 renders as 0, an explicit null falls back to minAvailable, an
absent key falls back, and a set value renders. hasKey would not work here,
since it is true for maxUnavailable: null and would render an empty field.

Also correct two documentation errors. The README claimed a PDB "may not set
both" fields and then that maxUnavailable "wins if you set both", conflating
the rendered resource with the values schema. AGENTS.md described PDBs as
covering "all stateless components" while tables, which has one, is stateful.

Part of OPS-4725
Base automatically changed from mg/pdb-maxunavailable to main August 13, 2026 11:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants