OCPBUGS-10431: move CRI-O metrics proxy from static pod to MCD daemonset - #6325
OCPBUGS-10431: move CRI-O metrics proxy from static pod to MCD daemonset#6325harche wants to merge 1 commit into
Conversation
|
Pipeline controller notification For optional jobs, comment This repository is configured in: LGTM mode |
|
Skipping CI for Draft Pull Request. |
|
@harche: This pull request references Jira Issue OCPBUGS-10431, which is invalid:
Comment The bug has been updated to refer to the pull request using the external bug tracker. DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughThe CRI-O metrics proxy moves into the ChangesCRI-O metrics proxy migration
Kubelet staticPodPath handling
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant MachineConfigDaemon
participant kube-rbac-proxy-crio
participant Kubelet
participant CRIOMetrics
MachineConfigDaemon->>kube-rbac-proxy-crio: Start proxy container
kube-rbac-proxy-crio->>kube-rbac-proxy-crio: Wait for legacy manifest removal and port 9637
kube-rbac-proxy-crio->>Kubelet: Read kubelet TLS material
kube-rbac-proxy-crio->>CRIOMetrics: Proxy secured metrics from 127.0.0.1:9537
Suggested reviewers: Important Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional. ❌ Failed checks (1 error)
✅ Passed checks (14 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: harche The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@manifests/machineconfigdaemon/daemonset.yaml`:
- Around line 88-93: Define both liveness and readiness probes for the
kube-rbac-proxy-crio container, using its crio-metrics port and the expected
health endpoint. Ensure readiness remains false while startup is blocked waiting
for the static pod manifest removal or port release, so upgrade failures are not
reported as healthy.
- Around line 126-129: Update the container resources block in the machineconfig
daemon workload to add both CPU and memory limits alongside the existing
requests. Use appropriate limit values consistent with the manifest’s resource
policy, while preserving the current request settings.
- Around line 124-125: Harden the machine-config-daemon metrics-proxy
container’s securityContext by removing privileged: true, enabling
readOnlyRootFilesystem and allowPrivilegeEscalation: false, and dropping all
Linux capabilities. Evaluate and enable runAsNonRoot: true if the proxy can
still read the kubelet certificates; otherwise retain only the restrictions that
preserve certificate access.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: c3b9af61-5566-4e70-81fc-8dd2392dbde1
📒 Files selected for processing (10)
install/0000_80_machine-config_00_service.yamlmanifests/machineconfigdaemon/daemonset.yamlmanifests/machineconfigdaemon/kube-rbac-proxy-crio-config.yamlpkg/operator/sync.gotemplates/arbiter/01-arbiter-kubelet/_base/files/criometricsproxy-config.yamltemplates/arbiter/01-arbiter-kubelet/_base/files/criometricsproxy.yamltemplates/master/01-master-kubelet/_base/files/criometricsproxy-config.yamltemplates/master/01-master-kubelet/_base/files/criometricsproxy.yamltemplates/worker/01-worker-kubelet/_base/files/criometricsproxy-config.yamltemplates/worker/01-worker-kubelet/_base/files/criometricsproxy.yaml
💤 Files with no reviewable changes (6)
- templates/arbiter/01-arbiter-kubelet/_base/files/criometricsproxy.yaml
- templates/arbiter/01-arbiter-kubelet/_base/files/criometricsproxy-config.yaml
- templates/master/01-master-kubelet/_base/files/criometricsproxy.yaml
- templates/master/01-master-kubelet/_base/files/criometricsproxy-config.yaml
- templates/worker/01-worker-kubelet/_base/files/criometricsproxy.yaml
- templates/worker/01-worker-kubelet/_base/files/criometricsproxy-config.yaml
5e89e4c to
b3a4b37
Compare
Live cluster test resultsValidated end-to-end on two separate cluster-bot 4.22.6 clusters (AWS, 3 masters + 3 workers). The second run tested this PR's exact head (all three commits combined). TL;DR: upgrade handoff from static pod to MCD sidecar completes with zero scrape gap ( Round 1 — upgrade path + hostPort admission bug foundDeployed as a dev image over a running 4.22.6 cluster (CVO scaled down, MCO deployment +
Round 2 — fresh cluster, this PR's exact headRepeated the full flow on a second cluster with this branch (daemonset move + hostPort fix + mergo empty-value fix folded in from #6319).
Runtime-level teardown audit (all 6 nodes)Verified via
Workers: 🤖 Generated with Claude Code |
16a6dc3 to
30f0bdb
Compare
|
monitoring CI jobs. /hold |
30f0bdb to
d4a8f0e
Compare
|
/retest-required |
|
/test e2e-gcp-op-part1 e2e-gcp-op-part2 e2e-gcp-op-ocl-part1 e2e-gcp-op-ocl-part2 e2e-gcp-op-single-node e2e-aws-ovn e2e-aws-ovn-upgrade e2e-hypershift |
|
/test e2e-hypershift |
|
/test e2e-aws-ovn-upgrade |
|
/test e2e-hypershift |
|
/test e2e-aws-ovn |
|
/test e2e-aws-ovn-upgrade |
|
@harche: The following test failed, say
Full PR test history. Your PR dashboard. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here. |
What this does
Moves the
kube-rbac-proxy-crioCRI-O metrics proxy off static pods and into the existingmachine-config-daemondaemonset as an additional container.The static pod (added in #4175 / OCPNODE-2098) is the only static pod running on every node role, which blocks disabling
staticPodPathon worker pools for STIG V-242397 (context in OCPBUGS-10431): removingstaticPodPathkills the proxy, port 9637 goes dead,up{job="crio"}drops to 0, and TargetDown fires.How
kube-rbac-proxy-criocontainer in the MCD daemonset. The MCD is alreadyhostNetwork: true, so it bindsnode:9637and forwards to CRI-O's loopback-only metrics endpoint at127.0.0.1:9537, serving the kubelet certificate (kubelet-server-current.pem) and validating clients against the kubelet CA — the cluster-monitoring-operator kubelet ServiceMonitor scrape chain (job="crio") is unchanged.criometricsproxy.yaml+criometricsproxy-config.yamlfor master/worker/arbiter). MCD removes the on-host files when the rendered config drops them.kube-rbac-proxy-crioConfigMap instead of a MachineConfig-written host file; the proxy uses in-cluster SA credentials (MCD SA already has tokenreview/SAR permissions) instead of the kubelet kubeconfig.kube-rbac-proxy-crioService now selects themachine-config-daemonpods.Upgrade ordering
syncMachineConfigDaemonwaits for the daemonset rollout before syncing the node configs that delete the static pod manifest. A naive bind on 9637 would crashloop against the still-running static pod and deadlock the upgrade. The new container instead waits for/etc/kubernetes/manifests/criometricsproxy.yamlto disappear and for the port to be released, then execs the proxy. Fresh installs pass both checks immediately. The container also declares noports:— with hostNetwork a declared containerPort acts as a hostPort, and the kubelet would reject the pod at admission while the legacy static pod still declares 9637.This PR also includes the #6319 change (mergo empty-value fix: clear the staticPodPath template default when the user sets it to empty), squashed into the single commit so the whole staticPodPath story is reviewable in one place; #6319 is closed in favor of this PR.
Still TODO (WIP)
up{job="crio"} == 1on all nodes throughout, no TargetDown. Also caught & fixed a hostPort admission conflict (now folded into the single squashed commit).staticPodPath: ""on the worker pool: KubeletConfig reports Success,staticPodPathabsent fromkubelet.confon all workers, port 9637 still served by the MCD sidecar, CRI-O metrics uninterrupted on all 6 nodes.test/extended-privgetAllKubeProxyPodlooks forkube-rbac-proxy-crio-*mirror pods; it tolerates finding none but the TLS-profile test loses that coverage until repointed at the MCD sidecarSummary by CodeRabbit
staticPodPathis explicitly set to"".staticPodPath: ""rendering behavior.