operator: create PodMonitor resources to automate Prometheus monitoring of CSI metrics - #593
abhinav-phi wants to merge 2 commits into
Conversation
8e6c5d4 to
5fca691
Compare
|
liveness sidecar is deprecated and it will be removed |
|
Agreed, Madhu. This should not depend on the deprecated liveness sidecar, so here is a narrower proposal. Instead of scraping the To be clear about what this collects: these are CSI operation metrics from the controller sidecars, not a replacement for the old One correction to my earlier description: ceph-csi PR #6287 (PodMonitor support in the Helm charts) is still open, so I should not have described it as established upstream support. Would this controller-only scope address the architectural concern, or would you prefer a different upstream metrics path? This proposal was prepared with AI assistance. |
…metrics Each Ceph-CSI pod exposes its CSI metrics through the liveness-prometheus sidecar on a configurable metrics port, but so far the operator did not provide a way for the Prometheus Operator to discover and scrape those endpoints. Administrators had to hand-craft and maintain PodMonitor (or ServiceMonitor) resources for every deployed driver. This adds an optional, operator-managed PodMonitor resource for the controller plugin and node plugin pods of each driver: - Driver CRD: new spec.podMonitor field (enabled, labels, annotations, interval), also settable as a default for all drivers through spec.driverSpecDefaults.podMonitor of the OperatorConfig CR. The feature is disabled by default and requires spec.liveness.metricsPort to be configured, as the liveness sidecar serves the metrics endpoint. - The liveness-prometheus containers of both workloads now declare their metrics port under the fixed name 'metrics', so the Prometheus Operator can discover it via the PodMonitor endpoint. - The operator creates/updates the PodMonitor (named '<driver>-podmonitor', owned by and garbage collected with the Driver) selecting the driver's ctrlplugin and nodeplugin pods, scraping the 'metrics' port at /metrics. Disabling the feature (or removing spec.liveness) deletes the PodMonitor again. Clusters without the monitoring.coreos.com/v1 API are tolerated: reconciliation skips PodMonitor cleanup instead of failing, and the manager does not watch PodMonitor objects to avoid a hard dependency on the CRD. - RBAC for podmonitors.monitoring.coreos.com was added to the manager role, and the prometheus-operator monitoring API types are vendored. Pods running with hostNetwork (nodeplugins by default) bind the metrics port on the node itself; drivers sharing a node must use distinct spec.liveness.metricsPort values to avoid port conflicts, which is now documented in docs/design/podmonitor.md. The Helm charts expose the new settings (drivers and operatorConfig.driverSpecDefaults), and all generated manifests (CRDs, RBAC, all-in-one and multifile installers, chart CRD templates) were regenerated. Signed-off-by: abhinav-phi <alpha9coder@gmail.com> Assisted-by: ZCode AI assistant (review and revision planning)
5fca691 to
8bf1a2c
Compare
It would be fine to add this yamls to a separate directory and document this feature and how it can be used rather than adding this to the API, we already have helm support to create any arbitrary yamls as required that can be used for this one. |
|
Confirming: rewrite to separate-dir example yamls + usage docs, dropping API/RBAC/CRD surface, using Helm arbitrary-yaml - starting rewrite, will push. |
|
@Madhu-1 rewritten per your direction, please re-review. New HEAD: b9815e6 (was 8bf1a2c). Dropped: Driver PodMonitor API (api/v1, deepcopy), RBAC (monitoring.coreos.com podmonitors), CRD bases and generated manifests (all-in-one, multifile, charts), controller reconcile and tests, csi utils helpers, go.mod/go.sum dep and all vendor/prometheus-operator, drivers-chart docs table entries. Added:
Verified: no PodMonitorSpec or prometheus-operator refs remain in api/internal/config/vendor/cmd, go build ./internal/controller/... passes, git diff --check clean. make generate/manifests skipped (heavy); CI approval still needed. |
…e guide Per review direction, drop the Driver PodMonitor API, RBAC, CRD, vendor and controller surface and document Prometheus monitoring instead. Add deploy/examples/podmonitor RBD and CephFS controller PodMonitors scraping upstream CSI sidecar metrics (no liveness sidecar dependency, distinct ports with hostNetwork conflict note). Rewrite docs/design/podmonitor.md as usage-only guide with kubectl and Helm extraDeploy examples. Assisted-by: ZCode AI Signed-off-by: abhinav-phi <alpha9coder@gmail.com>
b9815e6 to
da9268e
Compare
|
Fixed DCO: added missing Signed-off-by to the rewrite commit (da9268e) - DCO now passes. This is the docs/examples-only head you asked for (no operator API/RBAC/CRD, no liveness dependency, controller sidecars only, hostNetwork note). Please re-review when you get a chance. Thanks! |
Madhu-1
left a comment
Madhu-1
left a comment
There was a problem hiding this comment.
there a no need to have design document and multiple example for the different drivers, as its going to remain same for all drivers, just add one example and add a document to create multiple resources as required.
add a pre-req to the document, you can follow similar to https://github.com/ceph/ceph-csi-operator/blob/main/docs/features/rbd-snapshot-metadata.md
This branch has not been deployed
2 participants
Summary
This PR addresses #578 by adding operator-managed Prometheus
PodMonitorresources for CSI workloads. The current implementation discovers the metrics endpoint of the Ceph-CSIliveness-prometheussidecar. Madhu pointed out that this sidecar is deprecated and is expected to be removed, so it should not be the foundation of new monitoring support.Status: awaiting agreement on a controller-only alternative. The alternative described below is a proposal, not the implementation currently on this branch. The recent commit amendment adds an AI assistance trailer without changing the source tree.
What the current branch implements
The branch adds an optional
spec.podMonitorconfiguration with enablement, labels, annotations, and scrape interval settings. These settings can also be supplied throughOperatorConfigdriver defaults. When enabled with liveness configured, reconciliation creates or updates a Driver-owned PodMonitor selecting that driver's controller and node plugin pods and scraping their namedmetricsport at/metrics.The current implementation declares that port on the liveness sidecars and removes the PodMonitor when monitoring is disabled or liveness configuration is removed. It includes the monitoring API dependency, RBAC, generated CRDs and installation manifests, Helm configuration, documentation, and reconciliation tests. The external PodMonitor CRD is included for envtest rather than installation into user clusters.
Monitoring is opt-in. The implementation is intended to tolerate an absent PodMonitor CRD when monitoring is disabled; enabling the feature requires the Prometheus Operator CRD. Its dependence on
spec.livenessis the architectural issue that remains unresolved.Proposed replacement for the liveness dependency
The discussion reply proposes starting with operation metrics from the controller's upstream Kubernetes CSI sidecars: external-provisioner, external-attacher, external-resizer, and csi-snapshotter when present. These support
--http-endpoint, with/metricsas the default metrics path. The operator already permits per-container arguments throughspec.controllerPlugin.containerExtraArgs, but arguments alone are not sufficient to complete monitoring integration.The proposed revision would provide distinct named metrics ports for the enabled sidecars, matching NetworkPolicy ingress, and a PodMonitor that is independent of liveness configuration. Monitoring would remain opt-in, and ports would be configurable to account for shared pod networking and controller deployments using host networking. The precise configuration interface and supported image versions would need to be checked as part of implementation.
These endpoints expose CSI operation metrics; they are not an equivalent replacement for the old
csi_livenessgauge. Node plugin monitoring would be excluded from this revision because the node driver registrar's health endpoint is not an equivalent operation-metrics source. The operator's own existing metrics monitoring is separate from this proposal.Corrections to the original description
ceph-csi PR #6287 is still open, so it should be treated as related discussion rather than established upstream Helm support. Also, host networking does not inherently rule out a ServiceMonitor; the choice here is direct pod discovery, not a claim that Service-based discovery is impossible.
Validation and review request
The original submission reported passing lint, chart, and reconciliation checks, with an envtest teardown failure on Windows. Those results have not been independently rerun during this follow-up and do not validate the proposed controller-only design. No tests were rerun for the commit-message-only amendment. DCO passed, but that alone does not establish that the complete CI suite has passed.
The sign-off is preserved, and the added
Assisted-by: ZCode AI assistant (review and revision planning)trailer records the verified assistance scope. Would this controller-only direction address the architectural concern, or is a different upstream metrics path preferred?Related issue
Related to #578. The current liveness-based implementation remains under review and has not been replaced by the proposed design.