[Timofei Larkin (lllamnyp)]

Adds metrics for the address ledger, raised in a review of cozystack/cozystack#3382: "Nothing reports Conflict, Lost or Released addresses. Turning metrics on, adding a ServiceMonitor and alerting on those phases would make the feature usable in production." Stacked on #11.

Changes

• Gauges. InventoryCollector, registered on the controller-runtime registry, counts from the informer cache at scrape time:

  • address_controller_ipaddresses{class, phase}. This includes the driver-owned Conflict and Lost phases.
  • address_controller_ipaddressclaims{namespace, class, phase}, where class is the resolved status.className.

  A failed list reports an invalid metric, so the scrape fails visibly rather than reading zero.

• Chart, metrics.enabled (default off):

  • binds metrics to :8443, served over HTTPS and authorized through the controller-runtime filter already wired in cmd/main.go;
  • adds the Service address-controller-metrics;
  • adds a ClusterRole and binding for TokenReview and SubjectAccessReview, which the filter needs;
  • adds an address-controller-metrics-reader ClusterRole (get on /metrics) to bind the scraper to.

  It overrides controller.metricsBindAddress.

• metrics.serviceMonitor.enabled: a ServiceMonitor scraping that Service with its bearer token. The certificate is self-signed, so TLS verification is skipped. metrics.serviceMonitor.labels adds labels for a Prometheus serviceMonitorSelector. The scraper's ServiceAccount still needs a ClusterRoleBinding to address-controller-metrics-reader, which the chart cannot create without knowing that ServiceAccount; the values file and README show it.

• metrics.prometheusRule.enabled: alerts on addresses in Conflict or Lost for 5m and on claims Pending for 15m.

• Both are rendered only where the monitoring.coreos.com API exists.

• README. A new Metrics section. It names claims as the billable unit, as PersistentVolumeClaims are for storage: address_controller_ipaddressclaims{phase="Bound"} counts what each namespace holds.

Not covered

Released addresses get no alert: under Retain they are expected, and #11's releaseDelay returns them to the pool. The gauge still counts them.

Testing

A unit test checks the collector's output against a fake cache. helm lint passes, and the templates render with and without the monitoring APIs. I have not run the scrape path against a live Prometheus.

[coderabbitai]

Important

• Trigger review

This repository does not receive automatic reviews because it has fewer than 10 stars.

Run configuration

• Source mode: Auto
• Repository: Unavailable
• Branch: Unavailable
• File: No configuration file from this repository applied
• Commit: Unavailable
• Layers: defaults
• Review profile: CHILL
• Plan: Advanced
• Run ID: 2a2c145c-6718-46fc-84ad-a33a7f232a75

• Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

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.

Share

• X
• Mastodon
• Reddit
• LinkedIn

[IvanHunters]

Review: report the ledger as metrics

The collector mechanics are sound: labels are bounded (class, the six-value phase enum, namespace by tenant count), so no cardinality blow-up; counts are recomputed from a cached list at scrape time so deleted objects drop out and nothing leaks; a failed list reports an invalid metric rather than a false zero; registration is single and race-free; the PromQL in the alerts is valid and the ServiceMonitor selectors line up. Good.

[MAJOR] The metrics cannot attribute held addresses to a tenant, which is the stated purpose

The design's User-facing-changes section makes the IPAddress ledger the unit of accounting: "Metering follows the claim ... the IPAddress ledger is the unit of accounting: claimRef says whose." The metrics do not deliver that:

• address_controller_ipaddresses is labelled {class, phase} only (internal/controller/metrics.go:33); it drops claimRef.Namespace, so the ledger metric cannot answer "how many addresses does tenant X hold."
• address_controller_ipaddressclaims carries namespace, but it counts claims, not addresses. A Dual claim holds a v4 and a v6 IPAddress, so a per-namespace claim count understates the addresses actually held and billable.

So neither metric answers the per-tenant accounting the design calls for. Adding namespace (from claimRef.Namespace) as a label on address_controller_ipaddresses fixes it, and the cardinality stays bounded by the tenant count.

[MINOR] ServiceMonitor and alerts are not turnkey

Scraping needs the Prometheus ServiceAccount bound to address-controller-metrics-reader, which the chart cannot create (it does not know the SA), so with serviceMonitor.enabled alone the scrape returns 403. The ServiceMonitor also carries no label for a specific Prometheus serviceMonitorSelector and no namespaceSelector. Worth documenting the required RoleBinding and label next to the values.

NOT LGTM on the MAJOR; the rest is clean.

[Timofei Larkin (lllamnyp)]

Thanks for the review. Per point:

[MAJOR] Per-tenant attribution. Not changed. The billable unit is the claim, as PersistentVolumeClaims are for storage, and address_controller_ipaddressclaims{namespace, class, phase} already counts claims per namespace.

The proposal's paragraph is titled "Metering follows the claim". As first written in community#35 (e04d0eb) it ended: "The IPAddress ledger is the unit of accounting — its claimRef says whose, its conditions say since when — and a platform that bills addresses bills claims." A later prose rewrite (82cd418) dropped that last clause. Without it, the ledger sentence reads as if IPAddress objects were the billable unit. Its point is that the ledger records whose address it is and since when. cozystack/community#126 restores the clause.

On Dual: a dual-stack claim holding two addresses is the claim's shape, as a PVC's size is the PVC's. If IPv4 and IPv6 should be priced apart, that is a family label on the claims gauge. Nobody has asked for that yet.

[MINOR] Not turnkey. Done in c22125b:

• metrics.serviceMonitor.labels adds labels to the ServiceMonitor, for a Prometheus serviceMonitorSelector.
• The values file and README spell out the ClusterRoleBinding from the scraper's ServiceAccount to address-controller-metrics-reader, with a kubectl example. The chart cannot create it without knowing that ServiceAccount.
• The README names claims as the billable unit.

[IvanHunters]

Lifting my request-changes; this is now an approve.

The MAJOR I raised was that the metrics could not attribute held addresses to a tenant while the design called the IPAddress ledger the unit of accounting. That is resolved: community#126 reframes the design to bill per claim, as storage bills PersistentVolumeClaims, and this PR documents the same, "Claims are the billable unit ... address_controller_ipaddressclaims{phase="Bound"} counts what each namespace holds." Under that model the per-namespace claim gauge is the billable unit (and a dual-stack claim is one unit, not an undercount), and the address ledger is the fleet-level record. The collector code is unchanged and its mechanics were already sound.

The MINOR is fixed too: the README now shows the required ClusterRoleBinding for the scraper, and metrics.serviceMonitor.labels lets the ServiceMonitor match a Prometheus serviceMonitorSelector.

One dependency to flag, not an objection: this approval rests on community#126, which reframes the accounting unit. Worth landing #126 together with or before this, so the design and the metrics agree.

LGTM.