fix(k8s): make helm test pods schedule and probe correctly - #1272
fix(k8s): make helm test pods schedule and probe correctly#1272try-agaaain wants to merge 6 commits into
Conversation
Three classes of failures when running `helm test` on a control-plane tainted cluster with the builtin components: 1. Scheduling: test Pods for health/cubemastercli/mysql/redis/node-image had no control-plane placement, so they stuck Pending on tainted masters. Add cube.controlPlanePlacement to each. 2. CubeProxy probe: hitting "/" returns 400 (dataplane only serves sandbox traffic). Probe /admin/healthz with X-Cube-Admin-Token instead and assert HTTP 200. 3. DNS resolution: busybox nslookup times out on CoreDNS rewrite zones, and plain curl stalls on IPv6 (AAAA) probing. Use getent hosts with retries in the dns-test and wrap curl to force IPv4 with retries. Co-authored-by: Cursor <cursoragent@cursor.com> Signed-off-by: tsingyue <agaaain.try@gmail.com> Assisted-by: Cursor:deepseek-v4-flash
![cubesandboxbot[bot]](https://avatars.githubusercontent.com/in/3653584?s=60&v=4){kind=small}
| # DNS times out, so plain curl (IPv6 probe) fails to resolve even when | ||
| # A records exist. Force IPv4 for all lookups and retry the occasional | ||
| # transient lookup timeout in this test. | ||
| curl() { command curl -4 --retry 3 --retry-all-errors --retry-delay 1 "$@"; } |
There was a problem hiding this comment.
This wrapper shadows curl for the entire health-test script, not just DNS-sensitive lookups — the comment only mentions the IPv4/DNS case. Every subsequent call (CubeMaster/CubeOps/CubeAPI health, WebUI, and the kget kube-API calls) now inherits -4 --retry 3 --retry-all-errors --retry-delay 1:
- Fail-fast checks become retry-with-backoff: a genuinely unhealthy endpoint that returns 5xx (or times out at
--max-time) is retried 3×, so a single check can take up to ~48s before the pod fails. - The dataplane soft-fail check below hits a guaranteed
400on/, and with-f+--retry-all-errorsit now retries 3× on everyhelm testrun (~4s wasted each time).
If the retry is meant to absorb transient CoreDNS lookup timeouts only, consider applying -4 --retry ... to the specific lookups rather than wrapping curl globally — or confirm this broader retry behavior is intended.
| # The dataplane returns 400 for the bare "/" path (it only serves | ||
| # sandbox traffic), so probe the admin health endpoint instead. | ||
| # Retry to absorb transient CoreDNS lookup timeouts. | ||
| status="$(curl -4 --retry 3 --retry-all-errors --retry-delay 1 --connect-timeout 5 --max-time 15 -sS \ |
There was a problem hiding this comment.
This script runs under sh -ec (set -e). If curl exits non-zero — e.g. connection refused or timeout, rather than an HTTP status — the status="$(...)" command substitution aborts the script before the test "$status" = "200" line below, so the cube-proxy admin healthz status=... diagnostic is never printed and the failure is just curl's raw stderr. Consider capturing the exit code, e.g. status="$(curl ...)" || { echo "cube-proxy admin healthz unreachable"; exit 1; }, so connection failures produce a clear message too.
| # timeouts, so retry the lookup before treating it as a failure. | ||
| a_record() { | ||
| for _ in 1 2 3 4 5; do | ||
| if ip="$(getent hosts "$1" 2>/dev/null | awk '{print $1}' | tail -n 1)" && [ -n "$ip" ]; then |
There was a problem hiding this comment.
getent hosts can emit both A and AAAA lines in a dual-stack cluster, and tail -n 1 will take whichever family comes last — so the $domain_ip/$target_ip equality below is then compared on that family. Both names rewrite/resolve to the same ClusterIP set, so the assertion still holds, but this is a behavior change from the old IPv4-only nslookup+sed filter. Worth confirming the dual-stack case in the test plan. (Also confirming intent: this relies on getent being present in the curlimages/curl image — it is, via musl-utils in the default Alpine base.)
|
AI-generated review. Verified against the base-branch tree ( OverviewThis is a solid, well-scoped fix for
Two issues to address, both in Findings1.
|
- Scope the curl wrapper to IPv4 only; a global --retry made every fail-fast health check retry real 5xx endpoints for ~48s before failing, and the dataplane soft-fail probe retried a guaranteed 400. - Capture the admin healthz curl exit code explicitly so connection failures print a clear diagnostic under `set -e` instead of aborting silently. - Use getent ahostsv4 in dns-test so the equality assertions compare A records only (getent hosts can emit A and AAAA in a dual-stack cluster, and tail -n 1 picked whichever family came last). Signed-off-by: tsingyue <agaaain.try@gmail.com> Assisted-by: Cursor:deepseek-v4-flash
| imagePullSecrets: | ||
| {{- toYaml . | nindent 4 }} | ||
| {{- end }} | ||
| {{- include "cube.controlPlanePlacement" . | nindent 2 }} |
There was a problem hiding this comment.
This test validates cube-node (compute) assets, but the added cube.controlPlanePlacement requires a node labeled cube.tencent.com/cube-control: "true". On a compute-only deployment (externalControlPlane.enabled=true) — where the chart installs no control-plane workloads and no node carries that label — the pod becomes unschedulable (Pending), whereas before it scheduled on compute nodes and passed. The check here is API-based (lists cube-node pods via the ServiceAccount), so it does not need the control plane; cube.computePlacement (already used by node-runtime-test below) would fix the tainted-master scheduling for the single-node case while staying schedulable in compute-only clusters. The same compute-only unschedulability applies to the health-test placement addition (line 23), since that pod is created on every helm test run.
| - name: dns | ||
| image: {{ include "cube.image" .Values.helmTest.dnsImage | quote }} | ||
| imagePullPolicy: {{ .Values.helmTest.dnsImage.pullPolicy }} | ||
| image: {{ include "cube.image" .Values.helmTest.image | quote }} |
There was a problem hiding this comment.
The DNS test now uses helmTest.image instead of helmTest.dnsImage, so dnsImage is silently ignored here (it remains in use only by node-runtime-test below). Operators who override dnsImage for the DNS test lose that override without warning, and operators who override helmTest.image with a non-Alpine image will break the test, since it now depends on musl getent (specifically the ahostsv4 database). Consider a dedicated DNS-test image value (defaulting to the curl image) or documenting the getent dependency so the dnsImage override isn't silently dropped.
| # A records exist. Force IPv4 on every lookup; do NOT add a global | ||
| # --retry here because that would make the fail-fast health checks | ||
| # below retry real 5xx/timing-out endpoints for ~48s before failing. | ||
| curl() { command curl -4 "$@"; } |
There was a problem hiding this comment.
-4 is applied to every curl in this pod, including kget()'s calls to https://kubernetes.default.svc and the CubeMaster/CubeAPI/WebUI health checks. On a single-stack IPv6 cluster (or any Service whose ClusterIP is IPv6-only) these would fail to connect, even though the AAAA-probe stall this works around is specific to CoreDNS-forwarded names. Consider scoping -4 to just the DNS-sensitive lookups, or documenting the IPv4-only requirement in the values/README.
- health-test and node-image-test validate compute-plane assets (cube-node registration / DaemonSet pods), so controlPlanePlacement made them unschedulable on compute-only clusters (externalControlPlane.enabled) where no node carries the cube-control label. Switch both to cube.computePlacement, matching node-runtime-test. - Document that helmTest.dnsImage is now only used by node-runtime-test; the dns-test intentionally uses helmTest.image because busybox lacks a reliable getent for the CoreDNS rewrite zones, and warn against overriding helmTest.image with a non-Alpine image (getent dependency). - Document the IPv4-only requirement of the curl wrapper (CubeSandbox networking is IPv4; single-stack IPv6 is unsupported). Signed-off-by: tsingyue <agaaain.try@gmail.com> Assisted-by: Cursor:deepseek-v4-flash
| imagePullSecrets: | ||
| {{- toYaml . | nindent 4 }} | ||
| {{- end }} | ||
| {{- include "cube.computePlacement" . | nindent 2 }} |
There was a problem hiding this comment.
health-test is scheduled with computePlacement, which may still leave it Pending in the exact scenario this PR is fixing.
With the stock values.yaml, cube.computePlacement renders:
- nodeSelector
cube.tencent.com/cube-node: "true", and - a toleration only for the
cube.tencent.com/compute=true:NoScheduletaint.
It does not tolerate cube.tencent.com/control=true:NoSchedule — the taint that a "control-plane-tainted single-node cluster" applies to the master. So under default values, health-test (and node-image-test at ~line 380) remain Pending on a control-plane-tainted node unless the operator also layers values-single-node.yaml (which is the documented recipe that adds the control taint to compute tolerations).
Two consequences:
- On a control-plane-tainted single-node cluster using stock values, the scheduling fix is incomplete for these two pods — they need
controlPlanePlacement's control-taint toleration. health-testis rendered unconditionally (not gated oncubeNode.enabled, unlikenode-image-test). A control-plane-only cluster (cubeNode.enabled=false) that previously scheduled this unplaced pod anywhere would now leave it unschedulable, because no node matches thecube-nodenodeSelector.
Since health-test probes mostly control-plane services (CubeMaster / CubeOps / CubeAPI / WebUI / CubeProxy / DNS), cube.controlPlanePlacement — the helper this PR uses for the other in-cluster test pods — would resolve the Pending issue in both the single-node and control-only topologies. If compute placement is intentional, the dependency on the single-node profile should at least be documented (and health-test gated on cubeNode.enabled).
…raints The dns-test was switched to helmTest.image, silently dropping the dnsImage override operators could set for the DNS test. Restore dnsImage as the DNS-test image and default it to curlimages/curl: busybox lacked a reliable getent (ahostsv4) for the CoreDNS rewrite zones this chart configures, and the Alpine-based curl image ships it. node-runtime-test now uses the shared helmTest.image instead of dnsImage. Document in README that helm test pods are IPv4-only by design (CoreDNS forwards AAAA queries for the non-cluster.svc sandbox domain to upstream, where they can stall), and that dnsImage must ship musl getent. Signed-off-by: tsingyue <agaaain.try@gmail.com> Assisted-by: Cursor:deepseek-v4-flash
|
Addressed all review comments across 1. "curl() wrapper shadows every call — retry-with-backoff turns fail-fast checks into ~48s hangs" (health-test) 2. " 3. " 4. " 5. " 6. " 7. " Verified with |
| - name: node-runtime | ||
| image: {{ include "cube.image" .Values.helmTest.dnsImage | quote }} | ||
| imagePullPolicy: {{ .Values.helmTest.dnsImage.pullPolicy }} | ||
| image: {{ include "cube.image" .Values.helmTest.image | quote }} |
There was a problem hiding this comment.
This pod previously ran helmTest.dnsImage (default busybox, runs as root); it now runs helmTest.image (curlimages/curl, which runs as a non-root user curl_user/uid 1000 by default). node-runtime-test is the only test pod that mounts hostPaths and asserts on them (test -d <dataCubelet>, test -S <dataCubelet>/cubelet.sock, test -S <tmpCube>/network-agent-grpc.sock). stat() on those paths requires every parent directory to be traversable by uid 1000 — if cube-node-init/network-agent leave the host dirs root-only (0700/0750), these assertions will start failing where they succeeded under busybox. Since this image swap is incidental to the DNS-test fix, consider pinning securityContext.runAsUser: 0 to preserve the old root behavior, or explicitly verify the hostPath dirs are world-traversable.
| imagePullSecrets: | ||
| {{- toYaml . | nindent 4 }} | ||
| {{- end }} | ||
| {{- include "cube.computePlacement" . | nindent 2 }} |
There was a problem hiding this comment.
With the default values.yaml, cube.computePlacement tolerates only the cube.tencent.com/compute taint and requires the cube.tencent.com/cube-node label. On a single node carrying only the control-plane taint (cube.tencent.com/control — the "tainted masters" scenario in the PR description), this pod — and node-image-test below, also compute-placed — remains unschedulable unless that node also carries the compute label/taint or values-single-node.yaml (which adds both tolerations to both placements) is in use. Note the PR description says the stuck pods "had no control-plane placement", but these two are now given compute placement, not control-plane. Worth confirming the intended deployment profile for the single-node test plan.
node-runtime-test switched from the root-run busybox image to the non-root curlimages/curl image in the dnsImage rework, but it still stat()s hostPath sockets that cube-node creates root-owned, which uid 1000 cannot traverse. Restore a root context deliberately: allowPrivilegeEscalation: false, readOnlyRootFilesystem: true, drop all capabilities, and mount all three hostPaths read-only (the script only checks existence), so root can read the sockets without gaining write access to the host. dns-test now reports which name failed to resolve instead of aborting under `set -e` with no diagnostics, and prints both sides on an address-family mismatch. Also correct the IPv6 stall attribution in comments and README: AAAA probes of the probed names stall in practice; do not claim CoreDNS forwards them upstream (the chart's rewrite zone answers in-zone). Signed-off-by: tsingyue <agaaain.try@gmail.com> Assisted-by: Cursor:deepseek-v4-flash
New chart render guard asserting: health-test/node-image-test/node-runtime-test use computePlacement (schedulable in compute-only clusters), the control-plane checks (cubemastercli/mysql/redis/proxy-control/dns) use controlPlanePlacement, dns-test uses helmTest.dnsImage while node-runtime-test uses helmTest.image, proxy-control-test probes /admin/healthz with the admin token from the release Secret, and node-runtime-test mounts hostPaths read-only with a hardened securityContext. Picked up automatically by the kubernetes-chart-check test-*.sh loop. Signed-off-by: tsingyue <agaaain.try@gmail.com> Assisted-by: Cursor:deepseek-v4-flash
2dd0e79 to
f7fb35f
Compare
|
Follow-up commits Security — node-runtime-test (validated) Diagnostics — dns-test Retry semantics — proxy-control-test Documentation accuracy — IPv6 stall attribution Render guard test DCO |
| imagePullPolicy: {{ .Values.helmTest.dnsImage.pullPolicy }} | ||
| image: {{ include "cube.image" .Values.helmTest.image | quote }} | ||
| imagePullPolicy: {{ .Values.helmTest.image.pullPolicy }} | ||
| # Runs as root on purpose: it stat()s hostPath sockets that cube-node |
There was a problem hiding this comment.
This comment says the pod "runs as root on purpose", but nothing in this pod enforces root: the securityContext only sets allowPrivilegeEscalation, readOnlyRootFilesystem, and capability drops — there is no runAsUser. curlimages/curl's default image user is the non-root curl_user (uid 100), so the container will run as uid 100, not root. Previously this pod used the busybox image (root by default), so this is also a behavior change.
If the comment is accurate (cube-node really creates these sockets with root-only permissions that uid 100 "cannot traverse"), the test -d /data/cubelet / test -S /data/cubelet/cubelet.sock checks will fail on real hosts — the exact failure this PR is meant to fix. Either add runAsUser: 0 (+ runAsGroup: 0) so the manifest matches the comment, or verify uid 100 can traverse the hostPaths and correct the comment. Note the new guard script (test-helm-test-guards.sh) checks allowPrivilegeEscalation/drop/readOnly but not runAsUser, so CI would not catch a regression here.
| {{- toYaml . | nindent 4 }} | ||
| {{- end }} | ||
| {{- include "cube.computePlacement" . | nindent 2 }} | ||
| serviceAccountName: {{ include "cube.fullname" . }}-test |
There was a problem hiding this comment.
Tradeoff to be aware of: health-test is now scheduled with computePlacement (nodeSelector cube.tencent.com/cube-node: "true"), but it is rendered whenever helmTest.enabled is true — including with cubeNode.enabled=false. In a control-plane-only cluster (nodes labeled only cube.tencent.com/cube-control, no compute labels), this pod becomes unschedulable and helm test will hang on a Pending pod; before this change it had no placement and could run on untainted nodes. All three topologies listed in the PR description include compute nodes, so this is fine for those, but it's a regression for the control-plane-only case. Consider gating the compute placement on cubeNode.enabled (or documenting the constraint).
fslongjin
left a comment
fslongjin
left a comment
There was a problem hiding this comment.
Direction looks right — the problem is genuinely in helm test, not the business logic. Test pods that can't schedule and probing CubeProxy's / both misled operators before, worth fixing.
But I'm blocked on one point (see inline on node-runtime-test), and a couple others I'd like to align on before this merges.
Scope is overstated, please narrow it. The summary reads as if split-plane / tainted-master / compute-only are all fixed. What I actually see is more conservative: single-node mixed still needs values-single-node.yaml, and a control-plane-only cluster leaves health-test Pending (see inline). Please reword to "fixes helm test scheduling and probing, depends on X / Y profile" rather than "correct across all topologies".
One description nit: proxy-control-test / dns-test already had controlPlanePlacement on master. The placements newly added in this PR are health / cubemastercli / mysql / redis / node-image. Worth stating accurately so it doesn't look like all five are new.
These two I'm happy with, no change needed:
- Proxy →
/admin/healthz+ admin token: matches the real probe, clean fix. - DNS →
getent ahostsv4+ failure diagnostics: more reliable than busyboxnslookup; documenting IPv4-only in README is fine.
Test plan & guard: all three boxes are unchecked. If you have a cluster, please run at least single-node (with values-single-node) + compute-only; if not, mark the description "not yet run on a real cluster". The new guard is useful but only covers default values and doesn't assert runAsUser, so it won't catch the regression below — fine for now, just be aware.
Verdict: Request changes. The core fix is worth merging, especially Proxy healthz. Please align the "root" point (code or wording, pick one) and narrow the topology claims; I'll take another pass once those are cleared.
| imagePullPolicy: {{ .Values.helmTest.dnsImage.pullPolicy }} | ||
| image: {{ include "cube.image" .Values.helmTest.image | quote }} | ||
| imagePullPolicy: {{ .Values.helmTest.image.pullPolicy }} | ||
| # Runs as root on purpose: it stat()s hostPath sockets that cube-node |
There was a problem hiding this comment.
🚧 Blocker: comment says "runs as root on purpose", but the manifest doesn't.
The comment here and the PR description both say this pod runs as root deliberately so it can stat root-owned hostPath sockets, hardened by read-only mounts + dropped caps.
I rendered it: the pod has no runAsUser: 0. curlimages/curl's default user is uid 100, not root — the old default busybox was root. So the image swap silently changed the permission model but the manifest didn't follow.
On hosts where /data/cubelet or the socket is root-only, test -S /data/cubelet/cubelet.sock will fail — exactly the failure this PR is meant to fix.
Pick one, I lean toward 1:
- If root is actually needed → add
runAsUser: 0(andrunAsGroup: 0if needed) and have the guard assert it. - If uid 100 actually works → drop every "runs as root" claim so it doesn't mislead the next person.
Let's align on this first, then the rest.
| imagePullSecrets: | ||
| {{- toYaml . | nindent 4 }} | ||
| {{- end }} | ||
| {{- include "cube.computePlacement" . | nindent 2 }} |
There was a problem hiding this comment.
health-test renders whenever helmTest.enabled is true, but is now pinned to computePlacement. On a control-plane-only cluster (no cube-node-labeled nodes) this pod goes Pending — before this PR it had no placement and could schedule anywhere.
Either gate the placement on cubeNode.enabled (compute when cubeNode is on, control otherwise), or state explicitly in the PR description that control-only clusters aren't supported. Right now the summary reads like all topologies are covered.
| imagePullSecrets: | ||
| {{- toYaml . | nindent 4 }} | ||
| {{- end }} | ||
| {{- include "cube.controlPlanePlacement" . | nindent 2 }} |
There was a problem hiding this comment.
On a compute-only cluster that still ships the default cubemastercli, this test is pinned to controlPlanePlacement and will stay Pending with no cube-control node. The CLI test is a client — it doesn't strictly need to land on the same node as the control Deployment.
If compute-only is a supported target, either relax this placement or document that cubemastercli test must be disabled on compute-only clusters.
| if d.count("readOnly: true") < 3: | ||
| raise SystemExit("node-runtime-test hostPath mounts are not all read-only") | ||
| if "allowPrivilegeEscalation: false" not in d or "drop:" not in d: | ||
| raise SystemExit("node-runtime-test securityContext missing privilege hardening") |
There was a problem hiding this comment.
The guard here checks allowPrivilegeEscalation / drop / readOnly but not runAsUser, so it won't catch the missing runAsUser: 0 on node-runtime-test (see inline on node-health.yaml). If you go with option 1 there, please add an assertion here so it can't regress silently.
|
ping @try-agaaain |
|
Taking this over after the review went unanswered. #1272 cannot be rebased onto current Replacement: #1388 What changed vs this PR:
Please close this PR in favor of #1388 if that looks right. |
2 participants
Summary
Fixes
helm testso its pods schedule and probe correctly across clustertopologies (separated control/compute nodes, control-plane-tainted masters,
compute-only
externalControlPlanedeployments).cubemastercli-test,mysql-test,redis-test,proxy-control-test,dns-test) usecontrolPlanePlacement,consistent with the deployments they verify;
health-test,node-image-test,node-runtime-test)use
computePlacement, so they stay schedulable in compute-only clustersand land next to the
cube-nodeassets they inspect.400for the bare/path (itonly serves sandbox traffic).
proxy-control-testprobes/admin/healthzwith
X-Cube-Admin-Token, captures the curl exit code explicitly (soset -ecannot swallow the diagnostic), and asserts HTTP 200.nslookuptimes out on the CoreDNSrewritezones for
cubeProxy.domain, and plaincurlstalls on IPv6 (AAAA)probing.
dns-testresolves throughgetent ahostsv4(IPv4-only, nodual-stack ambiguity) with retries, reports which name failed, and the
DNS-test image is a dedicated
helmTest.dnsImage(defaults tocurlimages/curl, which ships muslgetent) so operators keep a distinctoverride for the DNS test.
so it runs as root deliberately, but with
allowPrivilegeEscalation: false,readOnlyRootFilesystem: true, dropped capabilities, and all hostPathsmounted read-only.
(
curl -4/getent ahostsv4) by design; this matches CubeSandbox'sIPv4-only sandbox networking and is documented in README.
Files
deploy/kubernetes/chart/templates/tests/node-health.yamldeploy/kubernetes/chart/values.yamldeploy/kubernetes/chart/README.mddeploy/kubernetes/chart/scripts/test-helm-test-guards.sh(render guard)Test plan
helm testpasses on a control-plane-tainted single-node cluster(with
values-single-node.yaml)helm testpasses on a multi-node cluster with compute nodeshelm teston a compute-only (externalControlPlane.enabled) clusterAssisted-by: Cursor:deepseek-v4-flash