Repository navigation
Adopt the Kubernetes recommended labels: new installs use only them, older releases keep theirs - #203
Merged
Merged
Conversation
wu-sheng
force-pushed
the
modernize-labels
branch
from
October 13, 2026 05:26
7dba04f to
482fc08
Compare
wankai123
approved these changes
Oct 13, 2026
kezhenxu94
approved these changes
Oct 13, 2026
There was a problem hiding this comment.
🟡 Changes recommended
Selector auto-detection can still mutate modern selectors, and the upgrade test leaves a key immutable-Job path unverified.
3 open findings
What changed in this PR
Adds Kubernetes-recommended labels while preserving upgrade compatibility for immutable Deployment selectors.
Changes:
- Centralizes legacy and modern label helpers with automatic selector detection.
- Adds upgrade and fresh-install E2E coverage.
- Documents selector migration and render-only deployment requirements.
| File | Description |
|---|---|
.github/workflows/e2e.ci.yaml |
Adds the upgrade E2E cell. |
chart/skywalking/values.yaml |
Adds deploymentSelector. |
chart/skywalking/templates/_helpers.tpl |
Implements shared labels and selector detection. |
chart/skywalking/templates/oap-clusterrole.yaml |
Applies shared OAP labels. |
chart/skywalking/templates/oap-clusterrolebinding.yaml |
Applies shared OAP labels. |
chart/skywalking/templates/oap-cm-dynamic.yaml |
Applies shared OAP labels. |
chart/skywalking/templates/oap-cm-override.yaml |
Applies shared OAP labels. |
chart/skywalking/templates/oap-deployment.yaml |
Adds dual pod labels and adaptive selectors. |
chart/skywalking/templates/oap-init.job.yaml |
Labels the Job and excludes selector mode from its hash. |
chart/skywalking/templates/oap-role.yaml |
Applies shared OAP labels. |
chart/skywalking/templates/oap-rolebinding.yaml |
Applies shared OAP labels. |
chart/skywalking/templates/oap-serviceaccount.yaml |
Applies shared OAP labels. |
chart/skywalking/templates/oap-svc.yaml |
Applies shared labels while retaining legacy selection. |
chart/skywalking/templates/satellite-cm-override.yaml |
Applies shared Satellite labels. |
chart/skywalking/templates/satellite-deployment.yaml |
Adds dual pod labels and adaptive selectors. |
chart/skywalking/templates/satellite-role.yaml |
Corrects and modernizes component labels. |
chart/skywalking/templates/satellite-rolebinding.yaml |
Corrects and modernizes component labels. |
chart/skywalking/templates/satellite-serviceaccount.yaml |
Applies shared Satellite labels. |
chart/skywalking/templates/satellite-svc.yaml |
Retains legacy Service selection. |
chart/skywalking/templates/ui-configmap.yaml |
Applies shared UI labels. |
chart/skywalking/templates/ui-deployment.yaml |
Adds dual pod labels and adaptive selectors. |
chart/skywalking/templates/ui-ingress.yaml |
Applies shared UI labels. |
chart/skywalking/templates/ui-pvc.yaml |
Applies shared UI labels. |
chart/skywalking/templates/ui-svc.yaml |
Retains legacy Service selection. |
docs/changes/changes.md |
Records labels, selectors, and compatibility notes. |
docs/contributing/e2e-tests.md |
Documents the expanded E2E suite. |
docs/reference/skywalking-chart-values.md |
Documents the new value. |
docs/upgrade/upgrading.md |
Adds selector upgrade and migration guidance. |
test/e2e/e2e-config-override.yaml |
Verifies modern selectors on fresh installs. |
test/e2e/e2e-upgrade.yaml |
Exercises upgrades from chart 5.0.0. |
test/e2e/expected/fresh-selectors.yml |
Defines expected modern selectors. |
test/e2e/expected/upgrade-init-job.yml |
Defines init Job upgrade expectations. |
test/e2e/expected/upgrade-pods-relabelled.yml |
Defines expected pod components. |
test/e2e/expected/upgrade-selectors.yml |
Defines expected preserved legacy selectors. |
🧠 Review effort: Balanced
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
wu-sheng
force-pushed
the
modernize-labels
branch
2 times, most recently
from
October 13, 2026 08:00
94c7c29 to
e88c56c
Compare
wu-sheng
changed the title
wu-sheng
force-pushed
the
modernize-labels
branch
3 times, most recently
from
October 13, 2026 08:27
3e51a24 to
bd6798e
Compare
There was a problem hiding this comment.
Warning
Copilot couldn't run its full agentic review because it didn't start before the timeout. Make sure your repository has a runner available, or add a copilot-code-review.yml file specifying one with the runs-on attribute. See the docs for more details.
Copilot review overview
5 open findings
chart:label uses the raw${Chart.Name}-${Chart.Version}value without the… · NewlegacyComponentis constructed frominclude "skywalking.fullname" .without any length guard,… · New
🧠 Review effort: Lite
…older releases keep theirs Resolves the chart half of apache/skywalking#12296; the BanyanDB chart moved in 0.3. A release installed with 5.1.0 or later carries only the Kubernetes recommended labels: app.kubernetes.io/name (always the chart name, never nameOverride), app.kubernetes.io/instance, app.kubernetes.io/component, app.kubernetes.io/managed-by and helm.sh/chart. Every selector uses them: the Deployments, the Services, pod anti-affinity, OAP cluster discovery and dynamic configuration (SW_CLUSTER_K8S_LABEL), Satellite's OAP lookup, and the init Job's pod. A release installed before 5.1.0 keeps its app/release/component/chart/heritage labels and its selectors on them, because a Deployment's selector is immutable, and gains the recommended labels next to them. The style is release-wide: skywalking.labelStyle looks up the release's OAP, UI and Satellite Deployments, so `helm upgrade` needs no extra step, and a component enabled later follows the release. `lookup` sees nothing without cluster access, so the new `labelStyle` value (auto | legacy | modern) lets render-only setups keep an older release on the legacy style. Moving an older release to the new labels is optional: delete its three Deployments and upgrade. The init Job's name hash leaves `labelStyle` out and adds the style for a modern release: an older release hashes exactly what 5.0.0 hashed, so an upgrade with unchanged values relabels its Job in place, and a move to the new labels renames the Job instead of patching its immutable pod template. All label values are quoted and valid: the chart version is sanitized in both chart labels and component values are capped at 63 characters. The Satellite Role and RoleBinding no longer carry OAP's component label. Tests: e2e-upgrade.yaml installs the published 5.0.0 chart, upgrades it to the working tree with identical values and again setting only labelStyle, and checks the kept selectors, the relabelled pods, the init Job, and that a Job keeping its name keeps its pod template, rendered from 5.0.0 and the working tree for each backend. It waits with kubectl, since Helm 3's --wait never ends on kind with the BanyanDB subchart's LoadBalancer Service. e2e-config-override.yaml checks that a fresh install carries only the recommended labels and selects by them. Docs: the 5.1.0 changelog, an upgrade-guide section, the values reference, the init Job guide and the e2e guide. The kubectl commands across the operate, install and troubleshooting pages read the current init Job from the release's rendered manifest and reach the pods through their Deployment's own selector, so they work for any chart version, either label style, and releases sharing a namespace; `helm template | kubectl apply` users run the same awk over their own render.
wu-sheng
force-pushed
the
modernize-labels
branch
from
October 13, 2026 09:10
bd6798e to
2dcd358
Compare
Copilot stopped reviewing on behalf of
wu-sheng due to an error
October 13, 2026 09:51
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Resolves the chart half of apache/skywalking#12296. The BanyanDB chart moved to the recommended labels in its 0.3 release.
What changes
A release's labels depend on when it was first installed, because a Deployment's selector can never change after creation:
app,release,component,chart,heritage, plus the recommended labelsapp,release,componentSW_CLUSTER_K8S_LABEL), Satellite's OAP lookupapp,release,componentapp.kubernetes.io/name(always the chart name, notnameOverride, because it is part of an immutable selector),app.kubernetes.io/instance,app.kubernetes.io/component,app.kubernetes.io/managed-byandhelm.sh/chart. All label values are quoted and valid: the chart version is sanitized in both chart labels (+becomes_), and component values are capped at 63 characters.lookupon the release's OAP, UI and Satellite Deployments, sohelm upgradeneeds no extra step, and a component enabled later follows the release. The newlabelStylevalue (auto|legacy|modern) overrides it.labelStyleout and adds the style for a new-style release. An older release hashes exactly what 5.0.0 hashed, so an upgrade with unchanged values relabels its Job in place; a move to the new labels renames the Job instead of patching its immutable pod template.Docs: the 5.1.0 changelog, a new upgrade-guide section, the values reference, the init Job guide, the e2e guide, and the
kubectlcommands across the operate, install and troubleshooting pages. Those read the current init Job from the release's rendered manifest (helm get manifest;helm template | kubectl applyusers run the same awk over their own render) and reach the pods through their Deployment's own selector, so they work for any chart version, either label style, and releases sharing a namespace.Breaking changes
app,release,component,chartorheritagelabel. Selectors of your own (kubectl -l, NetworkPolicies, monitors) must useapp.kubernetes.io/*for it.lookupreturns nothing when the chart is rendered without cluster access (helm template | kubectl apply, Argo CD,--dry-run=client). Such a setup must setlabelStyle: legacyfor a release installed before 5.1.0, or the Services switch to labels the running pods lack and OAP and the UI become unreachable until it is set.helm upgradeneeds nothing.Tests
New e2e coverage:
e2e-upgrade.yamlinstalls the published 5.0.0 chart, upgrades it to the working tree with identical values and again setting onlylabelStyle, and checks the kept selectors, the relabelled pods, the init Job, and that a Job keeping its name keeps its pod template (rendered from 5.0.0 and the working tree for BanyanDB, Elasticsearch and PostgreSQL). It waits withkubectl, since Helm 3's--waitnever ends on kind with the BanyanDB subchart'sLoadBalancerService.e2e-config-override.yamlchecks that a fresh install carries only the recommended labels and selects by them.Run locally on kind with Helm 3.22.0 (CI's major version), OAP from GHCR master with 2 replicas, Horizon 1.0.0, dynamic config enabled:
Every scenario checks the Deployment and Service selectors, pod labels, Service endpoints, that nothing is labelled
release=on a new-style release, theSW_CLUSTER_K8S_LABELvalue and that it matches the dynamic-config ConfigMap, OAP cluster membership (both nodes), Horizon login and OAP reachability, the init Job, and Satellite's discovered OAP addresses.helm pullof 5.0.0.helm template | kubectl applyon an older release (withlabelStyle: legacy: no errors; without it: the documented Deployment rejections), Satellite enabled later on an older and on a new release, andhelm rollbackafter a move.kubectlcommands, extracted verbatim, against: this branch with a second release in the namespace, chart 5.0.0, 5.0.0 upgraded to this branch, values A → B → A through Helm and throughhelm template | kubectl apply(stale Jobs left behind), and a release whose OAP Deployment name exceeds 40 characters.ultraeffort, given these test results; its findings are addressed.Found while testing, not changed here
helm upgradeof an Elasticsearch-backed release fails even from 5.0.0 to 5.0.0: Helm's server-side apply conflicts with the ECK operator on the Elasticsearch resource's.spec.nodeSets.--server-side=falseworks.docker.io/bitnami/postgresql:15.1.0-debian-11-r0, is gone from Docker Hub'sbitnaminamespace and now lives underbitnamilegacy, sopostgresql.enabled=truecannot pull it.OAP_SELECTORdefault) and two OAP docs pages select this chart's pods by the old labels; they need the recommended labels once they move to a 5.1.0 chart.🤖 Generated with Claude Code