Repository navigation
Conversation
…entric schema. Created new test file for new-schema. Updated README.md to document the changes to usage. Closes CCI-MOC/MOC-issues#417
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
Critical segment-alignment and duplicate-billing issues must be resolved before approval.
Get a fresh assessment by requesting another Copilot review.
Review effort: Balanced
Findings: 4
Open (5)
What changed in this PR
Adds pod-centric metrics generation and ingestion alongside the legacy schema.
Changes:
- Generates and uploads pod-centric metrics.
- Extends merge support for both schemas.
- Adds tests and documentation.
| File | Review |
|---|---|
README.md |
Documents supported schemas. |
openshift_metrics/tests/test_pod_centric_schema.py |
Tests pod-centric processing and export. |
openshift_metrics/tests/test_merge.py |
Tests schema loading and merging. |
openshift_metrics/openshift_prometheus_metrics.py |
Critical: independently condensed resources produce incorrect billing segments (1 vote). Moderate: GPU mapping is unavailable to the collector (3 votes). Critical: uploading both schemas causes duplicate or corrupted report input (2 votes). |
openshift_metrics/metrics_processor.py |
Critical: loading independently bounded resource segments loses values and durations (3 votes). |
openshift_metrics/merge.py |
Critical: separate legacy and namespace files for the same period are both billed (1 vote). |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| if ( | ||
| "namespaces" in metrics_from_file | ||
| and "cpu_metrics" not in metrics_from_file | ||
| ): | ||
| new_namespaces.append(metrics_from_file["namespaces"]) |
| start = seg["start"] | ||
| entry = self.merged_data[ns][pod]["metrics"].setdefault(start, {}) | ||
| entry.update({k: v for k, v in seg.items() if k != "start"}) |
| cpu_segments = MetricsProcessor.condense_metric_series( | ||
| metrics_dict["cpu_metrics"], interval_seconds, "cpu_request" | ||
| ) | ||
| mem_segments = MetricsProcessor.condense_metric_series( | ||
| metrics_dict["memory_metrics"], interval_seconds, "memory_request" |
| if args.upload_to_s3: | ||
| utils.upload_to_s3(output_file, S3_METRICS_BUCKET, s3_location) | ||
| utils.upload_to_s3(namespaces_file, S3_METRICS_BUCKET, s3_namespaces_location) |
| gpu_segments = MetricsProcessor.condense_metric_series( | ||
| metrics_dict.get("gpu_metrics") or [], interval_seconds, "gpu_request" | ||
| ) | ||
| gpu_mapping = MetricsProcessor._load_gpu_mapping("gpu_node_map.json") |
naved001
left a comment
naved001
left a comment
There was a problem hiding this comment.
Sorry it took me a while to get to it. Here's some high level feedback that might make things more straight-forward.
-
I think the idea is to introduce the new schema, new way to merge it and produce report from it without deleting the old mechanism entirely. Not that the old mechanism should be compatible with the new metrics schema. Once we have established the new metrics producer, ingester etc then we can remove the old portions of the code. @knikolla correct me if I got wrong, but I think that was your intention.
-
I noticed the segments had individual sections for the resources and had some repetitions. This could be because you were trying to make it work with the old way we were ingesting the metrics - so if we let go of that it might make things more straightforward.
"segments": [
{
"start": 1790467200,
"duration": 86400,
"cpu_request": "0.015",
"node": "wrk-11",
"resource": "cpu"
},
{
"start": 1790467200,
"duration": 86400,
"memory_request": "335544320",
"node": "wrk-11",
"resource": "memory"
}This repeats the start timestamp, duration, and node. It should just be something like:
"segments": [
{
"start": 1790467200,
"duration": 86400,
"cpu_request": "0.015",
"memory_request": "335544320",
"node": "wrk-11",
}I'd imagine by "segment" here it refers to a time segment.
In case of a scenario where a resource request (cpu, memory, whatever) is missing, set it to 0. In case there's not an exact overlap, then we should break it down into multiple segments (I think copliot suggested this somwhere).
Added support for new pod-centric schema alongside the old resource centric schema
Created new test file for new-schema.
Updated README.md to document the changes to usage.
Closes CCI-MOC/MOC-issues#417