From 98dee7420766e1d6a587d81dbee57e474f46c373 Mon Sep 17 00:00:00 2001 From: balaji Date: Wed, 19 Aug 2026 08:05:46 -0700 Subject: [PATCH] fix(container-cache): always publish NodePort and report inactive registry config Container Cache could install cleanly while function image pulls went straight to the upstream registry, with no error and no cache metrics. The DaemonSet writes the containerd mirror endpoint and the CRI-O drop-in as ${NODE_IP}:${port}, because both runtimes resolve it from the host network namespace where cluster service DNS and ClusterIP addresses are not dependable. The Service, though, only rendered a nodePort when service.type was NodePort, and the values table and user docs advertised ClusterIP and LoadBalancer as supported. Choosing either left the mirror pointing at a port nothing listens on, so every pull fell back to the upstream registry silently. Remove the knob rather than validate it. The service is now always NodePort, service.type is gone from values.yaml, and setting it to anything else fails the render with an explanation instead of installing a cache that pulls bypass. An explicit NodePort is still accepted so existing values files keep working. Separately, containerd reads registry.config_path only at daemon start and has no config reload, so correcting a node's config.toml does not take effect on its own. Hash config.toml around the configurator to detect that we changed it, and report the node instead of acting on it: a NOTICE in the container log and no readiness marker, so a DaemonSet ready count below its desired count is the signal that some nodes still pull straight from the upstream registry. Deliberately no containerd restart -- that is disruptive on nodes running function workloads, and node lifecycle is managed out of band. CRI-O has no equivalent gap, since its mirror lives directly in the drop-in rather than behind a startup-only path. It does need a nudge on first install, because auto_reload_registries only applies once CRI-O has read the crio.conf.d drop-in this DaemonSet writes. SIGHUP is a documented CRI-O reload rather than a kill, so signal it best-effort. Chart render tests cover the rejected override, the accepted NodePort, the pending-restart detection, the absence of any restart path, the readiness marker, and the CRI-O reload. Co-Authored-By: Balaji Ganesan --- deploy/helm/container-cache/README.md | 9 ++- .../deploy/templates/_helpers.tpl | 17 +++++ .../deploy/templates/daemonset.yaml | 68 +++++++++++++++++++ .../deploy/templates/service.yaml | 13 ++-- .../helm/container-cache/deploy/values.yaml | 7 +- .../tests/chart-render/verify-mirrors.sh | 42 ++++++++++++ .../cluster-management/container-cache.md | 15 ++-- 7 files changed, 160 insertions(+), 11 deletions(-) diff --git a/deploy/helm/container-cache/README.md b/deploy/helm/container-cache/README.md index c3ca5460e..e98d49024 100644 --- a/deploy/helm/container-cache/README.md +++ b/deploy/helm/container-cache/README.md @@ -295,9 +295,16 @@ Key behaviors: | Parameter | Default | Description | |-----------|---------|-------------| -| `service.type` | `NodePort` | `NodePort`, `ClusterIP`, or `LoadBalancer` | | `service.port` | `30345` | Primary service port | +The service is always `NodePort` and the type is not configurable. The host +container runtime reaches the cache at `${NODE_IP}:${port}` from the host +network namespace, where cluster service DNS and ClusterIP addresses are not +dependable, so the port has to be published on every node. Setting +`service.type` to anything other than `NodePort` fails the render: a ClusterIP +service publishes no node port, so the mirror endpoint would point at a closed +port and every pull would fall back to the upstream registry with no error. + ### CRI-O | Parameter | Default | Description | diff --git a/deploy/helm/container-cache/deploy/templates/_helpers.tpl b/deploy/helm/container-cache/deploy/templates/_helpers.tpl index 3f9db27a2..6301d0620 100644 --- a/deploy/helm/container-cache/deploy/templates/_helpers.tpl +++ b/deploy/helm/container-cache/deploy/templates/_helpers.tpl @@ -77,6 +77,23 @@ Create the name of the service account to use {{- end }} {{- end }} +{{/* +Reject a service.type override. + +The registry mirror endpoint written into hosts.toml and the CRI-O drop-in is +${NODE_IP}:${port}, which only resolves when the service publishes that port on +every node. Any other service type renders no nodePort, leaving the mirror +pointing at a closed port: pulls then fall back to the upstream registry with no +error, which is indistinguishable from a working cache until you look at cache +metrics. Fail the render instead of installing something that silently no-ops. +*/}} +{{- define "nvcf-container-cache.validateServiceType" -}} +{{- $type := (.Values.service | default dict).type -}} +{{- if and $type (ne $type "NodePort") -}} +{{- fail (printf "service.type=%s is not supported: container-cache must publish a NodePort so the host container runtime can reach it at ${NODE_IP}:${port}. Remove service.type from your values (the chart always renders NodePort) and set service.port instead." $type) -}} +{{- end -}} +{{- end }} + {{/* Compute CRI-O registry port mappings. If .Values.crio.registryPorts is set, use it. diff --git a/deploy/helm/container-cache/deploy/templates/daemonset.yaml b/deploy/helm/container-cache/deploy/templates/daemonset.yaml index e15a865b8..b8e722d07 100644 --- a/deploy/helm/container-cache/deploy/templates/daemonset.yaml +++ b/deploy/helm/container-cache/deploy/templates/daemonset.yaml @@ -59,6 +59,17 @@ spec: value: {{ $.Values.vault.certLocation | quote }} securityContext: privileged: true + # Not-ready means this node's registry config is written but not yet in + # effect (containerd needs a restart or a node cycle to pick up + # config_path). The pod keeps running either way -- this reports the + # node, it does not try to fix it. A DaemonSet whose ready count is + # below its desired count is the signal that some nodes are still + # pulling straight from the upstream registry. + readinessProbe: + exec: + command: ["/bin/bash", "-c", "test -f /tmp/nvcf-cc-ready"] + periodSeconds: 30 + failureThreshold: 1 {{- with $.Values.configure.resources }} resources: {{- toYaml . | nindent 10 }} @@ -86,6 +97,13 @@ spec: set -euo pipefail hosts=(${TARGET_HOST//,/ }) declare -A CRIO_PORTS + + # Cleared here and only written once the node's registry config is + # actually live; the readinessProbe reads it, so `kubectl get ds` + # counts exactly the nodes whose cache routing is in effect. + READY_MARKER=/tmp/nvcf-cc-ready + rm -f "${READY_MARKER}" + containerd_restart_pending=false {{- $crioPorts := include "nvcf-container-cache.crioRegistryPorts" . | fromYaml -}} {{- if $crioPorts }} {{- range $name, $val := $crioPorts }} @@ -128,7 +146,31 @@ spec: capabilities = ["pull", "resolve"] EOF done + + # containerd re-reads certs.d/*/hosts.toml on every pull, but + # registry.config_path lives in config.toml and is only read at + # startup -- containerd has no config reload, and SIGHUP kills the + # daemon rather than reloading it. So a config.toml edit is inert + # until containerd next starts. + # + # This DaemonSet deliberately does not restart containerd: that is + # disruptive on nodes running function workloads, and node lifecycle + # is managed out of band. Instead, hash around the updater to detect + # that we changed the active config, and report the node as pending + # so an operator can see which nodes still need a containerd restart + # or a node cycle. Until then this node keeps pulling from the + # upstream registry -- correctly, just without the cache. + before="$(sha256sum /host/etc/containerd/config.toml | cut -d' ' -f1)" python3 update_config.py /host/etc/containerd/config.toml + after="$(sha256sum /host/etc/containerd/config.toml | cut -d' ' -f1)" + + if [ "${before}" != "${after}" ]; then + containerd_restart_pending=true + echo "NOTICE: corrected containerd registry config on this node." >&2 + echo "NOTICE: containerd only reads registry.config_path at startup, so the" >&2 + echo "NOTICE: correction is inactive and image pulls still bypass the cache." >&2 + echo "NOTICE: Restart containerd or cycle this node to activate it." >&2 + fi fi # CRI-O: write a single drop-in. Do NOT touch /etc/containers/registries.conf; @@ -190,6 +232,32 @@ spec: mkdir -p "${user_drop_in_dir}" cp "${crio_conf}" "${user_drop_in_dir}/nvcf-container-cache.conf" fi + + # auto_reload_registries makes CRI-O watch registries.conf.d, but + # that setting only takes effect once CRI-O has read the drop-in we + # just wrote -- on a first install it has not. Unlike containerd, + # CRI-O reloads registry config on SIGHUP without dropping + # workloads, so nudge it rather than leaving the mirror inactive + # until something else restarts the daemon. Best-effort: a node + # where CRI-O is absent or unsignalable is not a failure. + crio_pid="$(pgrep -x crio | head -1 || true)" + if [ -n "${crio_pid}" ]; then + if kill -HUP "${crio_pid}" 2>/dev/null; then + echo "Reloaded CRI-O registry config (SIGHUP)." + else + echo "Could not signal CRI-O; the drop-in applies on its next reload." >&2 + fi + fi + fi + + # containerd is the only runtime that can be left with config on disk + # that is not yet in effect, because config_path is read at startup + # and it has no reload. CRI-O has no equivalent gap: the mirror lives + # directly in the drop-in, which it re-reads. + if [ "${containerd_restart_pending}" = "true" ]; then + echo "NOT READY: this node needs a containerd restart or a node cycle." >&2 + else + touch "${READY_MARKER}" fi # CRI-O picks up registries.conf.d changes via auto_reload_registries=true, diff --git a/deploy/helm/container-cache/deploy/templates/service.yaml b/deploy/helm/container-cache/deploy/templates/service.yaml index 605bea378..65adacef8 100644 --- a/deploy/helm/container-cache/deploy/templates/service.yaml +++ b/deploy/helm/container-cache/deploy/templates/service.yaml @@ -12,6 +12,7 @@ # WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. # See the License for the specific language governing permissions and # limitations under the License. +{{- include "nvcf-container-cache.validateServiceType" . -}} apiVersion: v1 kind: Service metadata: @@ -21,21 +22,23 @@ metadata: app: {{ $.Release.Name }} {{- include "nvcf-container-cache.labels" . | nindent 4 }} spec: - type: {{ .Values.service.type | default "ClusterIP" }} + # Always NodePort, and not configurable. The host container runtime + # (containerd/CRI-O) reaches this service as ${NODE_IP}:${port} from the host + # network namespace, where cluster service DNS and ClusterIP addresses are not + # dependable. A ClusterIP service renders no nodePort at all, so the mirror + # endpoint the DaemonSet writes points at a port nothing listens on and every + # pull silently falls back to the upstream registry. + type: NodePort ports: - port: {{ .Values.service.port | default 30345 }} - {{ if eq .Values.service.type "NodePort" }} nodePort: {{ .Values.service.port | default 30345 }} - {{ end }} targetPort: https name: https {{- $crioPorts := include "nvcf-container-cache.crioRegistryPorts" . | fromYaml -}} {{- if $crioPorts }} {{- range $name, $val := $crioPorts }} - port: {{ $val.port }} - {{ if eq $.Values.service.type "NodePort" }} nodePort: {{ $val.port }} - {{ end }} targetPort: crio-{{ $name }} name: crio-{{ $name }} {{- end }} diff --git a/deploy/helm/container-cache/deploy/values.yaml b/deploy/helm/container-cache/deploy/values.yaml index b89312cb1..ecec2d8fe 100644 --- a/deploy/helm/container-cache/deploy/values.yaml +++ b/deploy/helm/container-cache/deploy/values.yaml @@ -142,8 +142,13 @@ persistentVolumeClaim: freeProxyPct: 15 # Service configuration. +# +# The service type is not configurable: it is always NodePort. The host +# container runtime reaches the cache at ${NODE_IP}:${port} from the host +# network namespace, so the port has to be published on every node. Setting +# service.type to anything else fails the render rather than installing a cache +# that pulls silently bypass. service: - type: NodePort port: 30345 # Proxy service (nvcf-proxy-cache ClusterIP) configuration. diff --git a/deploy/helm/container-cache/tests/chart-render/verify-mirrors.sh b/deploy/helm/container-cache/tests/chart-render/verify-mirrors.sh index 2ac555dfc..2884b0d33 100644 --- a/deploy/helm/container-cache/tests/chart-render/verify-mirrors.sh +++ b/deploy/helm/container-cache/tests/chart-render/verify-mirrors.sh @@ -38,6 +38,48 @@ assert_has 'name: nvcf-container-cache' # nodes without a local cache pod (kube-proxy drops NodePort traffic). assert_not_has 'externalTrafficPolicy: Local' assert_not_has 'internalTrafficPolicy: Local' +# The registry mirror endpoint is ${NODE_IP}:${port}, so the port must be +# published on every node. A ClusterIP service renders no nodePort and the +# mirror then points at a closed port, which fails as a silent fallback to the +# upstream registry rather than a visible error. +assert_has 'type: NodePort' + +echo "Checking service.type cannot be overridden..." +# Capture rather than pipe: `set -o pipefail` would otherwise report helm's +# intentional non-zero exit as the pipeline's failure. +override_out="$(helm template container-cache ./deploy --set service.type=ClusterIP 2>&1 || true)" +if ! printf '%s' "${override_out}" | grep -F -q -- 'is not supported'; then + echo "FAILED: service.type=ClusterIP should fail the render with an explanation" >&2 + echo "${override_out}" >&2 + exit 1 +fi +# NodePort is still accepted, so existing values files keep working. +helm template container-cache ./deploy --set service.type=NodePort >/dev/null + +echo "Checking containerd pending-restart reporting..." +# containerd reads registry.config_path only at daemon start and has no config +# reload, so correcting config.toml does not take effect on its own. Detect that +# we changed it and report the node; never restart containerd, which would be +# disruptive on nodes running function workloads. +assert_has 'before="$(sha256sum /host/etc/containerd/config.toml | cut -d'"'"' '"'"' -f1)"' +assert_has 'if [ "${before}" != "${after}" ]; then' +assert_has 'containerd_restart_pending=true' +assert_not_has 'systemctl restart containerd' +assert_not_has 'nsenter' + +echo "Checking readiness reflects whether cache routing is live..." +# A DaemonSet ready count below its desired count is the signal that some nodes +# still pull straight from the upstream registry. +assert_has 'test -f /tmp/nvcf-cc-ready' +assert_has 'readinessProbe:' +assert_has 'touch "${READY_MARKER}"' + +echo "Checking CRI-O reload..." +# SIGHUP is a documented CRI-O reload, not a kill, so it is safe to signal. +# Needed because auto_reload_registries only applies once CRI-O has read the +# crio.conf.d drop-in this DaemonSet writes. +assert_has 'kill -HUP "${crio_pid}"' +assert_has 'pgrep -x crio' echo "Checking multi-domain NodePort listeners..." assert_has 'nodePort: 30346' diff --git a/docs/user/cluster-management/container-cache.md b/docs/user/cluster-management/container-cache.md index 99d548b80..35cb739ed 100644 --- a/docs/user/cluster-management/container-cache.md +++ b/docs/user/cluster-management/container-cache.md @@ -290,19 +290,26 @@ persistentVolumeClaim: ### Service Configuration -The service type and port can be configured based on your access requirements: +The service port is configurable. The service type is not: Container Cache is +always exposed as a `NodePort`. ```yaml # values.yaml service: - # Service type: ClusterIP, NodePort, or LoadBalancer - type: ClusterIP - # Port for the Container Cache service port: 30345 ``` +The container runtime on each node reaches the cache at `${NODE_IP}:${port}` +from the host network namespace, where cluster service DNS and ClusterIP +addresses are not dependable, so the port must be published on every node. + +Setting `service.type` fails the install with an explicit error. A ClusterIP +service publishes no node port, so the registry mirror written to each node +would point at a port nothing listens on, and image pulls would fall back to +the upstream registry with no error and no cache involvement. + ### Metrics Configuration Container Cache includes Prometheus metrics for monitoring cache performance: