feat: implement core metrics provider for workspace resource usage#1241
feat: implement core metrics provider for workspace resource usage#1241richabanker wants to merge 1 commit into
Conversation
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
|
I have kept this first PR intentionally scoped to just the data layer for the resource metrics API. Currently it adds no HTTP paths, handlers, Repositories wiring and RBAC changes (which I plan to split in smaller follow-ups). Please let me know if granularity works or if I should split this PR further into smaller chunks? Happy to go whichever way makes review easiest. |
christian-heusel
left a comment
There was a problem hiding this comment.
Hey @richabanker, thanks a lot for driving this and starting out with an implementation of your proposal! 🥳
I have left a few thoughts below!
/ok-to-test
| if err := metricsv1beta1.AddToScheme(scheme); err != nil { | ||
| return nil, fmt.Errorf("failed to add metrics types to scheme: %w", err) | ||
| } |
There was a problem hiding this comment.
I think you also need to do this for the tests:
[FAILED] in [BeforeSuite] - /home/chris/Documents/shared_projects/kion/kubeflow-notebooks/workspaces/backend/api/suite_test.go:132 @ 07/11/26 02:20:34.521
<< Timeline
[FAILED] Unexpected error:
<*fmt.wrapError | 0x17a49b88b620>:
unable to create manager: no kind is registered for the type v1beta1.PodMetrics in scheme "pkg/runtime/scheme.go:100"
{
msg: "unable to create manager: no kind is registered for the type v1beta1.PodMetrics in scheme \"pkg/runtime/scheme.go:100\"",
err: <*runtime.notRegisteredErr | 0x17a49b3c26c0>{
schemeName: "pkg/runtime/scheme.go:100",
gvk: {Group: "", Version: "", Kind: ""},
target: nil,
t: <*reflect.rtype | 0x2065a40>{
t: {Size_: 0x140, PtrBytes: 0x130, Hash: 614274612, TFlag: 7, Align_: 8, FieldAlign_: 8, Kind_: 25, Equal: nil, GCData: 85, Str: 253661, PtrToThis: 5271776},
},
},
}
occurred
In [BeforeSuite] at: /home/chris/Documents/shared_projects/kion/kubeflow-notebooks/workspaces/backend/api/suite_test.go:132 @ 07/11/26 02:20:34.521
------------------------------
🤖-generated reason
api/suite_test.gobuilds its manager's scheme manually (scheme.Scheme+kubefloworgv1beta1.AddToScheme), not viahelper.BuildScheme().helper.NewManager()was changed in this PR to unconditionally add&metricsv1beta1.PodMetrics{}toCache.DisableFor, which requires the passed-in scheme to know that GVK.
There was a problem hiding this comment.
yep, I was not registering metricsv1beta1 to the scheme used by the test client. Added it now.
| func podWithContainer(name string, containers ...corev1.Container) corev1.Pod { | ||
| return corev1.Pod{ | ||
| ObjectMeta: metav1.ObjectMeta{Name: name}, | ||
| Spec: corev1.PodSpec{ | ||
| Containers: containers, | ||
| }, | ||
| } | ||
| } |
There was a problem hiding this comment.
Should be fine to just //nolint here given that its in a test file:
$ make -C workspaces/backend lint
make: Entering directory '/home/chris/Documents/shared_projects/kion/kubeflow-notebooks/workspaces/backend'
/home/chris/Documents/shared_projects/kion/kubeflow-notebooks/workspaces/backend/bin/golangci-lint run
internal/models/metrics/funcs_test.go:168:23: `podWithContainer` - `name` always receives `"pod-1"` (unparam)
func podWithContainer(name string, containers ...corev1.Container) corev1.Pod {
^
make: *** [Makefile:77: lint] Error 1
make: Leaving directory '/home/chris/Documents/shared_projects/kion/kubeflow-notebooks/workspaces/backend'
There was a problem hiding this comment.
Actually changed a test to pass a value other than "pod-1" for the name param. That should make it go away.
| type WorkspaceResourceUsage struct { | ||
| // Available is true when live usage metrics were successfully retrieved. | ||
| Available bool `json:"available"` | ||
|
|
||
| // Source identifies which provider produced the data (e.g. "metrics-server"). | ||
| Source string `json:"source,omitempty"` | ||
|
|
||
| // Reason is a machine-readable code present only when Available is false. | ||
| Reason string `json:"reason,omitempty"` | ||
|
|
||
| // Message is a human-readable explanation present only when Available is false. | ||
| Message string `json:"message,omitempty"` | ||
|
|
||
| // Pods holds the per-pod usage. | ||
| Pods []PodResourceUsage `json:"pods"` | ||
| } |
There was a problem hiding this comment.
I'm not 100% sure with this, however I think we usually have API design that revolves around error states instead of success states, so the way that this types combines both paths (i.e. via Available & Reason) feels odd to me. However the last call regarding design decisions is of course with Andy and Mathew so would be good if they'd weigh in on this at some point 🤗
See for example this snippet:
notebooks/workspaces/backend/internal/models/common/assets/types.go
Lines 17 to 33 in e51895d
If we apply this pattern here, we would get something like this:
type WorkspaceResourceUsage struct {
// Source identifies which provider produced the data (e.g. "metrics-server").
// Empty when Error is set.
Source string `json:"source,omitempty"`
// Error is set when live usage metrics could not be retrieved.
Error *ResourceUsageError `json:"error,omitempty"`
// Pods holds the per-pod usage. Empty when Error is set.
Pods []PodResourceUsage `json:"pods"`
}
type ResourceUsageError struct {
Code ResourceUsageErrorCode `json:"code"`
Message string `json:"message,omitempty"`
}
type ResourceUsageErrorCode string
const (
ResourceUsageErrorMetricsAPIUnavailable ResourceUsageErrorCode = "METRICS_API_UNAVAILABLE"
ResourceUsageErrorWorkspaceNotRunning ResourceUsageErrorCode = "WORKSPACE_NOT_RUNNING"
)Also the ImageRefErrorCode values are in 'SCREAMING_SNAKE' casing ("CONFIGMAP_MISSING"), while this PR's constants are 'PascalCase' ("MetricsAPIUnavailable").
There was a problem hiding this comment.
ah good point. I like the proposed schema, can pivot to this to keep things consistent once @andyatmiami or @thesuperzapper confirm. Thanks for this suggestion!
5709569 to
8adbd44
Compare
Signed-off-by: Richa Banker <richabanker@google.com>
8adbd44 to
41996c1
Compare
This PR introduces the core models and provider logic for surfacing live, point-in-time pod resource utilization (CPU and Memory) for workspaces. This implements the foundation for the Resource Utilization Metrics proposal.
Changes:
metricspackage containing domain models (WorkspaceResourceUsage) and schema mappings for the API responsemetricsServerProviderto readPodMetricsfrom themetrics.k8s.ioaggregated API using the controller-runtime clienthelper.NewManager()to disable caching forPodMetrics, as the aggregated API does not support WATCHesIssue: #1222
Design Doc