PO to GMP Migration Tool: Podmonitor Limits, ScrapeClass, Metadata. FilterRunning Migration - #1989
Conversation
There was a problem hiding this comment.
Code Review
This pull request adds support for converting limits, node metadata attachment, and filter-running configurations from Prometheus PodMonitor resources to GMP PodMonitoring and ClusterPodMonitoring resources, alongside adding relevant unit tests and warnings for unsupported fields. The review feedback points out a concurrency/mutation risk where the metadata slice is modified in-place via pointer dereference. To prevent unexpected side effects on shared or cached configurations, it is recommended to copy the slice before appending new elements.
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request implements conversion logic for limits, node metadata attachment, and filter-running configurations from Prometheus PodMonitors to GMP PodMonitoring and ClusterPodMonitoring resources, along with corresponding test coverage and warnings for unsupported fields. The review feedback suggests improving the warning messages for endpoint-level filterRunning settings to clarify that the tool automatically applies the configuration globally and to ensure the correct resource type is referenced in the ClusterPodMonitoring converter.
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request adds support for mapping PodMonitor limit settings to GMP ScrapeLimits and handles several unsupported fields (such as followRedirects, enableHttp2, scrapeClassName, and scrapeProtocols) by logging warnings. It also adds conversion logic for AttachMetadata.Node and resource-level FilterRunning settings. The review feedback suggests avoiding taking the address of block-local variables when updating metadata, and refining the FilterRunning warning logic to only trigger when there is an actual conflict between endpoints.
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request adds support for converting PodMonitor limit settings, node metadata attachment, and filter-running configurations to GMP ScrapeLimits and PodMonitoring/ClusterPodMonitoring specs, along with warnings for unsupported fields. The reviewer recommends extracting the duplicated spec-level field processing logic from both convertToPodMonitoring and convertToClusterPodMonitoring into a single shared helper function in helpers.go to reduce code duplication and improve maintainability.
43c6120 to
cbeae05
Compare
cbeae05 to
99b9190
Compare
feba832 to
034c91a
Compare
ee7f76d to
3273c74
Compare
4aad090 to
32b06e2
Compare
334ea78 to
9b1b69f
Compare
9d02a0f to
78968b5
Compare
There was a problem hiding this comment.
🏗️ Architectural Proposal: The "action-required Filter" Pattern for Partial Migration
As we expand the PO-to-GMP migration tool to cover complex real-world Prometheus Operator (PO) manifests, we frequently encounter settings that are either structurally unsupported by GKE Managed Prometheus (GMP) CRDs, security-blocked by validation webhooks, or unresolvable in an offline CLI tool.
Currently, our conversion logic handles these friction points in one of two ways:
- Hard Failures (
return nil, error): Aborting conversion entirely when encountering schema incompatibilities (e.g., missing ports or basic-auth inproxyUrl). - Lossy Drops with Warning Logs: Dropping rules and letting the resource deploy in an active state (e.g., dropping annotation target filters or unresolved
ScrapeClasses).
Both approaches create friction: hard failures break batch automated migration pipelines, while active lossy drops can cause continuous scrape failure logs, paging alerts, or massive telemetry billing spikes.
To solve this across the broader migration architecture, we should adopt the "Inert Draft / No-Op Placeholder Filter" pattern as a core structural design principle.
⚙️ How the Mechanism Works (3-Step Pipeline)
When the migration tool encounters an unresolvable required setting, an unsupported authentication mechanism, or a dropped filtering rule that poses high operational risk, instead of throwing a fatal error or exporting a broken/unbounded active scraper, it performs three steps:
- Sanitize the Incompatible Field: Replace illegal or unresolvable values with valid schema defaults so that
kubectl applysucceeds without Kubernetes API webhook rejection. - Inject an No-Op action-required Pod Selector: Override or append to
selector.matchLabelsan intentionally impossible placeholder label requirement:selector: matchLabels: gmp.migration.todo/action-required: "configure-proxy-authentication"
- Attach an Actionable Remediation Annotation: Document the exact modification and manual operator follow-up steps directly in
metadata.annotations:metadata: annotations: gmp.migration.google.com/action-required: | Endpoint [0] proxyUrl contained plaintext credentials which were stripped for GMP API compatibility. Configure egress network/proxy authentication, then delete the 'gmp.migration.todo' label from matchLabels to activate live scraping.
🌟 Key Benefits of the Pattern
- Zero Operational Noise: Because no pods in the cluster will ever match the placeholder label
gmp.migration.todo/action-required: ..., Prometheus discovers 0 targets and performs 0 scrapes. - Prevents Scrape Scope & Billing Explosions: Eliminates the risk of dropped pre-scrape filtering rules accidentally causing an unbounded wildcard scrape across an entire namespace or cluster, safeguarding teams against unexpected billing surges.
- Eliminates Hard Migration Roadblocks: Converts static validation errors (which normally halt automated batch migration scripts) into cleanly exported, 99%-complete draft manifests sitting safely in git repositories ready for operator inspection.
📍List of Application Areas Across the Migration Tool
We should institutionalize this pattern across the following seven scenarios:
-
Credentials Embedded in
proxyUrl(Basic-Auth in URLs)- Current Behavior: Fatal error; completely halts resource conversion because GMP API validation rejects plaintext URL credentials.
- Placeholder Application: Strip out
user:pass@, export the sanitized URLhttp://proxy-server:8080, and apply the no-op selector directing operators to configure egress proxy auth at the network or service mesh layer.
-
Dropped Annotation & Node-Label Target Filters on Empty Selectors
- Current Behavior: Drops rules referencing pod annotations (
__meta_kubernetes_pod_annotation_*) or node labels (__meta_kubernetes_node_label_*) with a warning log, causing an empty selector to passively scrape every pod in the namespace or cluster. - Placeholder Application: When dropping a target filter leaves an unconstrained or empty selector, inject the no-op placeholder selector to prevent an accidental wildcard scrape and force operator verification.
- Current Behavior: Drops rules referencing pod annotations (
-
Dropped
KeepEqual,DropEqual,LabelKeep, andLabelDropActions- Current Behavior: Dropped with a log warning; removes ingestion deduplication and high-availability active/standby filters.
- Placeholder Application: When deduplication rules cannot be promoted to post-scrape
metricRelabelings, render the monitor inert with a placeholder selector to prevent double-ingesting high-volume metrics.
-
Missing or Omitted
portandtargetPortin Endpoints- Current Behavior: Fatal error; conversion aborts because GMP schemas strictly require a port definition.
- Placeholder Application: Emit a placeholder
port: "GMP_MIGRATION_TODO_PORT", apply the no-op selector, and add a remediation annotation prompting the operator to supply the target container port name.
-
Conflicting Pre-Scrape
keepRules on the Same Pod Label- Current Behavior: Fatal error; selector translation fails when multiple regex keep rules target the same Kubernetes pod label.
- Placeholder Application: Avoid aborting migration; generate an inert draft with instructions to manually resolve or promote complex boolean discovery logic into post-scrape rules.
-
Unsupported Protobuf Scrape Protocols (
PrometheusProto) & Disabled HTTP/2- Current Behavior: Logs a warning that scrapes may fail if the target lacks OpenMetrics/text fallbacks or HTTP/2 negotiation.
- Placeholder Application: Inject the no-op selector and diagnostic annotation to prevent immediate scrape negotiation failures in production until text fallbacks are confirmed on target workloads.
(My own writing)
This isn't in scope for this PR (would be a follow-up), and requires discussion with @bernot-dev and @bwplotka. I think we are going to encounter a lot of cases where we can translate most of a PodMonitor/ServiceMonitor, but get stuck on a few fields we can't handle. If there is some mechanism (i'm sure there alternatives to the above) to give the user what we are able to figure out, and clearly signal what is left to be migrated, I think that would help make this less brittle.
| @@ -353,12 +405,55 @@ func (c *PodMonitorConverter) convertToClusterPodMonitoring(pm *pomonitoringv1.P | |||
| logger.Warn("Resulting ClusterPodMonitoring selector is empty. It will select and scrape all pods across all namespaces. Verify if this is intended.") | |||
There was a problem hiding this comment.
I don't think namespace selector supports matchExpressions:
https://prometheus-operator.dev/docs/api-reference/api/#monitoring.coreos.com/v1.NamespaceSelector
| @@ -353,12 +405,55 @@ func (c *PodMonitorConverter) convertToClusterPodMonitoring(pm *pomonitoringv1.P | |||
| logger.Warn("Resulting ClusterPodMonitoring selector is empty. It will select and scrape all pods across all namespaces. Verify if this is intended.") | |||
There was a problem hiding this comment.
I don't think namespace selector supports matchExpressions:
https://prometheus-operator.dev/docs/api-reference/api/#monitoring.coreos.com/v1.NamespaceSelector
This change completes the missing configuration fields in the PodMonitor migration path to GKE Managed Prometheus (PodMonitoring and ClusterPodMonitoring).
Key changes:
attachMetadata.nodetotargetLabels.metadata: ["node"].sampleLimit,labelLimit,labelNameLengthLimit, andlabelValueLengthLimitto GMP'sScrapeLimits.filterRunningconfigurations to the resource level.scrapeClassNameis configured, noting that inherited settings will be lost. Added a TODO code comments to resolve and mergeScrapeClasssettings from the Prometheus CR once that migration pipeline is implemented.