Biday prometheus

[Parthiba-Hazra]

[Title]

📚 Description of Changes

Provide an overview of your changes and why they’re needed. Link to any related issues (e.g., "Fixes #123"). If your PR fixes a bug, resolves a feature request, or updates documentation, please explain how.

• What Changed:
  (Describe the modifications, additions, or removals.)

• Why This Change:
  (Explain the problem this PR addresses or the improvement it provides.)

• Affected Components:
  (Which component does this change affect? - put x for all components)

• Compose

• K8s

• Other (please specify)

❓ Motivation and Context

Why is this change required? What problem does it solve?

• Context:
  (Provide background information or link to related discussions/issues.)

• Relevant Tasks/Issues:
  (e.g., Fixes: #GitHub Issue)

🔍 Types of Changes

Indicate which type of changes your code introduces (check all that apply):

• BUGFIX: Non-breaking fix for an issue.
• NEW FEATURE: Non-breaking addition of functionality.
• BREAKING CHANGE: Fix or feature that causes existing functionality to not work as expected.
• ENHANCEMENT: Improvement to existing functionality.
• CHORE: Changes that do not affect production (e.g., documentation, build tooling, CI).

🔬 QA / Verification Steps

Describe the steps a reviewer should take to verify your changes:

1. (Step one: e.g., "Run make test to verify all tests pass.")
2. (Step two: e.g., "Deploy to a Kind cluster with make create-kind && make deploy.")
3. (Additional steps as needed.)

✅ Global Checklist

Please check all boxes that apply:

• I have read and followed the CONTRIBUTING guidelines.
• My code follows the code style of this project.
• I have updated the documentation as needed.
• I have added tests that cover my changes.
• All new and existing tests have passed locally.
• I have run this code in a local environment to verify functionality.
• I have considered the security implications of this change.

---

Summary by Gitar

• Removed Prometheus dependency:
  • Fully decommissioned Prometheus server, node-exporter, and kube-state-metrics components.
  • Simplified collectors by removing Prometheus-reliant fallbacks and query logic.
• Unified metric collection:
  • Extended nodemon to aggregate node, container, and PVC metrics via stats/summary and /metrics/cadvisor endpoints.
  • Implemented a unified HTTP API for local metric consumption, replacing Prometheus as the primary data source.
• Historical data refactoring:
  • Replaced HistoricalMetricsCollector with HistoricalPercentileCache, fetching pre-computed workload percentiles directly from DAKR control plane.
• New internal services:
  • Introduced CAdvisorScraper for rate computation and PercentileFetcher for DAKR-backed historical metrics.
  • Added comprehensive integration testing for the new unified collector pipeline.

_{This will update automatically on new commits.}

[gitar-bot]

🚨 Bug: Container GPU metrics always empty — data never extracted from cache

After removing the Prometheus GPU query path, gpuMetrics is unconditionally set to an empty map (make(map[string]interface{})) at line 327. The comment claims "data flows through nodemonContainerMetricsCache" but there is no code that reads the GPU fields (GPUUtilization, GPUMemoryUsedMiB, etc.) from UnifiedContainerMetric and populates the gpuMetrics map before passing it to processContainerMetrics. This means container-level GPU metrics (utilization, memory, power, temperature) are silently lost for all GPU workloads — a regression from the previous Prometheus+nodemon path.

Suggested fix:

// GPU metrics from nodemon unified cache
if c.nodemonContainerMetricsCache != nil {
    podKey := pod.Namespace + "/" + pod.Name
    if cms, ok := c.nodemonContainerMetricsCache[podKey]; ok {
        for _, cm := range cms {
            if cm.Container == containerMetrics.Name && cm.GPUUtilization > 0 {
                gpuMetrics["gpu_utilization"] = cm.GPUUtilization
                gpuMetrics["gpu_memory_used_mib"] = cm.GPUMemoryUsedMiB
                gpuMetrics["gpu_memory_free_mib"] = cm.GPUMemoryFreeMiB
                gpuMetrics["gpu_power_watts"] = cm.GPUPowerWatts
                gpuMetrics["gpu_temperature"] = cm.GPUTemperature
                break
            }
        }
    }
}

_{Was this helpful? React with 👍 / 👎 | Reply gitar fix to apply this suggestion}

[gitar-bot]

⚠️ Bug: fmt.Sscanf with "%s/%s/%s" cannot parse slash-separated keys

In historical_percentile_cache.go:107, fmt.Sscanf(key, "%s/%s/%s", &ns, &name, &kind) is used to parse keys of the form namespace/name/kind. However, %s in Go's fmt.Sscanf consumes until whitespace, not until /. The first %s will consume the entire string (e.g., "default/my-deploy/Deployment") leaving nothing for the other two verbs. This means the fallback key parsing always fails silently, and those workloads won't be refreshed.

Suggested fix:

// Replace fmt.Sscanf with strings.Split
parts := strings.SplitN(key, "/", 3)
if len(parts) == 3 {
    workloads = append(workloads, HistoricalWorkloadQuery{
        Namespace:    parts[0],
        WorkloadName: parts[1],
        WorkloadKind: parts[2],
    })
}

_{Was this helpful? React with 👍 / 👎 | Reply gitar fix to apply this suggestion}

[gitar-bot]

⚠️ Bug: HistoricalPercentileCache goroutine started with uncancellable context

In setupMpaServer (line 1989), go cache.Start(context.Background()) launches a goroutine with a context that can never be cancelled. If the controller is stopped or the MPA server needs to be torn down, this goroutine (and its 15-minute ticker) will leak and continue making DAKR API calls indefinitely. The guard if r.MpaServer != nil { return nil } prevents duplicates only if setupMpaServer isn't called after a full teardown.

Suggested fix:

// Store a cancel func on the reconciler and use a derived context:
cacheCtx, cacheCancel := context.WithCancel(context.Background())
r.historicalCacheCancel = cacheCancel  // new field on reconciler
go cache.Start(cacheCtx)

// Then in teardown/Stop, call r.historicalCacheCancel()

_{Was this helpful? React with 👍 / 👎 | Reply gitar fix to apply this suggestion}

[gitar-bot]

Code Review 🚫 Blocked 0 resolved / 3 findings

Decommissions the Prometheus build dependency in favor of a unified nodemon-based collector, but misses container GPU metrics due to an unpopulated map. Fails on key parsing in the historical percentile cache and lacks proper cancellation for the background cache routine.

🚨 Bug: Container GPU metrics always empty — data never extracted from cache

📄 internal/collector/container_resource_collector.go:325-327

After removing the Prometheus GPU query path, gpuMetrics is unconditionally set to an empty map (make(map[string]interface{})) at line 327. The comment claims "data flows through nodemonContainerMetricsCache" but there is no code that reads the GPU fields (GPUUtilization, GPUMemoryUsedMiB, etc.) from UnifiedContainerMetric and populates the gpuMetrics map before passing it to processContainerMetrics. This means container-level GPU metrics (utilization, memory, power, temperature) are silently lost for all GPU workloads — a regression from the previous Prometheus+nodemon path.

Suggested fix

// GPU metrics from nodemon unified cache
if c.nodemonContainerMetricsCache != nil {
    podKey := pod.Namespace + "/" + pod.Name
    if cms, ok := c.nodemonContainerMetricsCache[podKey]; ok {
        for _, cm := range cms {
            if cm.Container == containerMetrics.Name && cm.GPUUtilization > 0 {
                gpuMetrics["gpu_utilization"] = cm.GPUUtilization
                gpuMetrics["gpu_memory_used_mib"] = cm.GPUMemoryUsedMiB
                gpuMetrics["gpu_memory_free_mib"] = cm.GPUMemoryFreeMiB
                gpuMetrics["gpu_power_watts"] = cm.GPUPowerWatts
                gpuMetrics["gpu_temperature"] = cm.GPUTemperature
                break
            }
        }
    }
}

⚠️ Bug: fmt.Sscanf with "%s/%s/%s" cannot parse slash-separated keys

📄 internal/collector/historical_percentile_cache.go:107

In historical_percentile_cache.go:107, fmt.Sscanf(key, "%s/%s/%s", &ns, &name, &kind) is used to parse keys of the form namespace/name/kind. However, %s in Go's fmt.Sscanf consumes until whitespace, not until /. The first %s will consume the entire string (e.g., "default/my-deploy/Deployment") leaving nothing for the other two verbs. This means the fallback key parsing always fails silently, and those workloads won't be refreshed.

Suggested fix

// Replace fmt.Sscanf with strings.Split
parts := strings.SplitN(key, "/", 3)
if len(parts) == 3 {
    workloads = append(workloads, HistoricalWorkloadQuery{
        Namespace:    parts[0],
        WorkloadName: parts[1],
        WorkloadKind: parts[2],
    })
}

⚠️ Bug: HistoricalPercentileCache goroutine started with uncancellable context

📄 internal/controller/collectionpolicy_controller.go:1989

In setupMpaServer (line 1989), go cache.Start(context.Background()) launches a goroutine with a context that can never be cancelled. If the controller is stopped or the MPA server needs to be torn down, this goroutine (and its 15-minute ticker) will leak and continue making DAKR API calls indefinitely. The guard if r.MpaServer != nil { return nil } prevents duplicates only if setupMpaServer isn't called after a full teardown.

Suggested fix

// Store a cancel func on the reconciler and use a derived context:
cacheCtx, cacheCancel := context.WithCancel(context.Background())
r.historicalCacheCancel = cacheCancel  // new field on reconciler
go cache.Start(cacheCtx)

// Then in teardown/Stop, call r.historicalCacheCancel()

🤖 Prompt for agents

Code Review: Decommissions the Prometheus build dependency in favor of a unified nodemon-based collector, but misses container GPU metrics due to an unpopulated map. Fails on key parsing in the historical percentile cache and lacks proper cancellation for the background cache routine.

1. 🚨 Bug: Container GPU metrics always empty — data never extracted from cache
   Files: internal/collector/container_resource_collector.go:325-327

   After removing the Prometheus GPU query path, `gpuMetrics` is unconditionally set to an empty map (`make(map[string]interface{})`) at line 327. The comment claims "data flows through nodemonContainerMetricsCache" but there is no code that reads the GPU fields (GPUUtilization, GPUMemoryUsedMiB, etc.) from `UnifiedContainerMetric` and populates the `gpuMetrics` map before passing it to `processContainerMetrics`. This means container-level GPU metrics (utilization, memory, power, temperature) are silently lost for all GPU workloads — a regression from the previous Prometheus+nodemon path.

   Suggested fix:
   // GPU metrics from nodemon unified cache
   if c.nodemonContainerMetricsCache != nil {
       podKey := pod.Namespace + "/" + pod.Name
       if cms, ok := c.nodemonContainerMetricsCache[podKey]; ok {
           for _, cm := range cms {
               if cm.Container == containerMetrics.Name && cm.GPUUtilization > 0 {
                   gpuMetrics["gpu_utilization"] = cm.GPUUtilization
                   gpuMetrics["gpu_memory_used_mib"] = cm.GPUMemoryUsedMiB
                   gpuMetrics["gpu_memory_free_mib"] = cm.GPUMemoryFreeMiB
                   gpuMetrics["gpu_power_watts"] = cm.GPUPowerWatts
                   gpuMetrics["gpu_temperature"] = cm.GPUTemperature
                   break
               }
           }
       }
   }

2. ⚠️ Bug: fmt.Sscanf with "%s/%s/%s" cannot parse slash-separated keys
   Files: internal/collector/historical_percentile_cache.go:107

   In `historical_percentile_cache.go:107`, `fmt.Sscanf(key, "%s/%s/%s", &ns, &name, &kind)` is used to parse keys of the form `namespace/name/kind`. However, `%s` in Go's `fmt.Sscanf` consumes until whitespace, not until `/`. The first `%s` will consume the entire string (e.g., "default/my-deploy/Deployment") leaving nothing for the other two verbs. This means the fallback key parsing always fails silently, and those workloads won't be refreshed.

   Suggested fix:
   // Replace fmt.Sscanf with strings.Split
   parts := strings.SplitN(key, "/", 3)
   if len(parts) == 3 {
       workloads = append(workloads, HistoricalWorkloadQuery{
           Namespace:    parts[0],
           WorkloadName: parts[1],
           WorkloadKind: parts[2],
       })
   }

3. ⚠️ Bug: HistoricalPercentileCache goroutine started with uncancellable context
   Files: internal/controller/collectionpolicy_controller.go:1989

   In `setupMpaServer` (line 1989), `go cache.Start(context.Background())` launches a goroutine with a context that can never be cancelled. If the controller is stopped or the MPA server needs to be torn down, this goroutine (and its 15-minute ticker) will leak and continue making DAKR API calls indefinitely. The guard `if r.MpaServer != nil { return nil }` prevents duplicates only if setupMpaServer isn't called after a full teardown.

   Suggested fix:
   // Store a cancel func on the reconciler and use a derived context:
   cacheCtx, cacheCancel := context.WithCancel(context.Background())
   r.historicalCacheCancel = cacheCancel  // new field on reconciler
   go cache.Start(cacheCtx)
   
   // Then in teardown/Stop, call r.historicalCacheCancel()

_{Was this helpful? React with 👍 / 👎 | Gitar}