feat(ws): add ws counts to backend wsk model #368

roee1313 · 2025-05-21T11:53:55Z

This PR solves issue #352

google-oss-prow · 2025-05-21T11:54:00Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
Once this PR has been reviewed and has the lgtm label, please assign kimwnasptd for approval. For more information see the Kubernetes Code Review Process.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

OWNERS

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

jiridanek · 2025-05-21T15:16:51Z

/ok-to-test

andyatmiami

I managed to (briefly) talk with Mathew during the WG call today (Thursday, May 29th) and he stated he was comfortable embedding this "counts" information more naturally inline with the response schema (and not worrying about adhering to a 1:1 structural mapping w.r.t how the information is presented from k8s)

The below image represents my proposal on how this "embedding" could work. A couple notes:

The same schema should be applied to the "list" as well as the "get-by-id" endpoints
- For sake of brevity - I am only showing the "get-by-id" payload below
I opted to "co-locate" the counts information closest to relevant data within the response. This is certainly most clean for the values arrays - but then leads to a root workspacesCount attribute at the root of the WorkspaceKind element as there isn't a more suitable place for it.
For sake of consistency/readability - I chose to re-use the same attribute name throughout (workspacesCount) - with the "pattern" being <child resource name (plural)>Count... in the event this paradigm crops up elsewhere in the API - we could reuse this design philosophy
Certainly open to other's opinions on if this is the most appropriate design. I had considered a counts: {....} object as well... but chose not to advocate for that option as its too redundant/nested (having to repeat context like imageConfig as well as the IDs in order for a client to then have to perform the mapping to relevant object to display inline as we do in our table view today)

FYI: @thesuperzapper @ederign @paulovmr

google-oss-prow · 2025-05-29T19:26:57Z

@andyatmiami: changing LGTM is restricted to collaborators

In response to this:

I managed to (briefly) talk with Mathew during the WG call today (Thursday, May 29th) and he stated he was comfortable embedding this "counts" information more naturally inline with the response schema (and not worrying about adhering to a 1:1 structural mapping w.r.t how the information is presented from k8s)

The below image represents my proposal on how this "embedding" could work. A couple notes:

The same schema should be applied to the "list" as well as the "get-by-id" endpoints

For sake of brevity - I am only showing the "get-by-id" payload below

I opted to "co-locate" the counts information closest to relevant data within the response. This is certainly most clean for the values arrays - but then leads to a root workspaces_count attribute at the root of the WorkspaceKind element as there isn't a more suitable place for it.

For sake of consistency/readability - I chose to re-use the same attribute name throughout (workspaces_count) - with the "pattern" being <child resource name>_counts... in the event this paradigm crops up elsewhere in the API - we could reuse this design philosophy

Certainly open to other's opinions on if this is the most appropriate design. I had considered a counts: {....} object as well... but chose not to advocate for that option as its too redundant/nested (having to repeat context like imageConfig as well as the IDs in order for a client to then have to perform the mapping to relevant object to display inline as we do in our table view today)

FYI: @thesuperzapper @ederign @paulovmr

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

roee1313 · 2025-06-07T12:22:29Z

Hey @andyatmiami
I added the required fixes, and they are ready for to be checked,
Thanks!

andyatmiami · 2025-06-09T17:42:20Z

workspaces/backend/internal/models/workspacekinds/funcs.go

@@ -153,3 +156,12 @@ func buildOptionRedirect(redirect *kubefloworgv1beta1.OptionRedirect) *OptionRed
 		Message: message,
 	}
 }
+
+func FindConfigWorkspacesById(configs []kubefloworgv1beta1.OptionMetric, targetId string) int32 {


For optimal efficiency - I worry about this pattern of iterating through an array while we are already iterating over an array that gives us O(n^2)

We can improve this by iterating over the respective []kubefloworgv1beta1.OptionMetric once each (for imageConfig and podConfig) and building up a map object that we can then pass into the build...ConfigValues

This ensures while we are iterating over the PodConfig/ImageConfig .Values - we can get O(1) lookups for the respective counts.

Although being done in a slightly different context - you can see examples of "map initialization" in theNewWorkspaceKindModelFromWorkspaceKind function:

https://github.com/kubeflow/notebooks/pull/368/files#diff-5a4650065c6e40d76e86fc1df9189efbbdc0d93250c74928b71601e13eac2890R30

So, feels like this new use case also "fits"...

Hey @andyatmiami
That is a good point actually :) thanks for that!
Just fixed that please review when available
Thanks!

andyatmiami · 2025-06-10T17:06:46Z

/lgtm

Re-testing code after latest changes:

verified GET api/v1/workspacekinds includes workspacesCount in response for all elements
verified GET api/v1/workspacekinds/{name} includes workspacesCount in response

also confirmed Swagger UI properly includes these new attributes in schema:

andyatmiami · 2025-06-10T17:08:08Z

@thesuperzapper - PR ready / looking for your review/approval now - thanks!

andyatmiami

@roee1313

I had a chance to speak with @thesuperzapper about this PR - and here is the (hopefully!) last batch of feedback we have to give in order to get this PR merged 🤞

In the last round of edits - we made changes to expose a workspacesCount attribute "inline" / co-located amongst the relevant JSON object.

We want to tweak this structure slightly.. instead of workspacesCount: <int> - we want the following structure:

clusterMetrics: {
    workspacesCount: <int>
}

This change will make more sense as I open up a couple other related issues based on recent discussions with Mathew - but the high level motivations are as such:

clusterMetrics becomes a "bucket" to hold other future metrics in a well-understood structure
We will eventually add the concept of namespaceMetrics as well - to better delineate metrics that span the cluster (relevant to Joel) and metrics specific to a given namespace (relevant to Bella)
- One of the driving forces behind this change is the "dual nature" of use cases the GET /workspacekinds API find itself in:
  - For Bella, when listing WorkspaceKinds as part of the Create Workspace flow, she may not have permissions/ability to view ALL workspaces... so if we only had a SINGLE count exposed, this could be confusing as she may not see the count aligned with the number of workspaces accessible to her
  - For Joel, when listing WorkspaceKinds as part his workflow in the Workspace Kinds screen, he should have permissions/ability to view ALL workspaces... so presenting this clusterMetrics count is more natural / aligned with what he would find if clicking around in the UI

A few other implementation notes:

while not possible to observe today, we would want omitempty defined on the struct for clusterMetrics (however, as part of this PR, workspacesCount will always be defined
Please add a //TODO somewhere relevant in the code to highlight the need to extend your solution to apply to namespaceMetrics (in the future, after I write that issue up and get it prioritized).. the need to calculate namespaceMetrics would be triggered via a request to GET /workspacekinds having a ?namespaceFilter query parameter defined on it that would in some way be passed down into NewWorkspaceKindModelFromWorkspaceKind.. you don't need to implement these changes - just sharing for transparency/understanding)

andyatmiami · 2025-07-01T17:47:41Z

workspaces/backend/internal/models/workspacekinds/types.go

+	Labels         []OptionLabel   `json:"labels"`
+	Hidden         bool            `json:"hidden"`
+	Redirect       *OptionRedirect `json:"redirect,omitempty"`
+	ClusterMetrics clusterMetrics  `json:"clusterMetrics"`


Suggested change

ClusterMetrics clusterMetrics `json:"clusterMetrics"`

ClusterMetrics clusterMetrics `json:"clusterMetrics,omitempty"`

andyatmiami · 2025-07-01T17:47:58Z

workspaces/backend/internal/models/workspacekinds/types.go

+	Labels         []OptionLabel   `json:"labels"`
+	Hidden         bool            `json:"hidden"`
+	Redirect       *OptionRedirect `json:"redirect,omitempty"`
+	ClusterMetrics clusterMetrics  `json:"clusterMetrics"`


Suggested change

ClusterMetrics clusterMetrics `json:"clusterMetrics"`

ClusterMetrics clusterMetrics `json:"clusterMetrics,omitempty"`

andyatmiami · 2025-07-01T20:12:51Z

workspaces/backend/internal/models/workspacekinds/funcs.go

@@ -60,6 +62,10 @@ func NewWorkspaceKindModelFromWorkspaceKind(wsk *kubefloworgv1beta1.WorkspaceKin
 		Hidden:             ptr.Deref(wsk.Spec.Spawner.Hidden, false),
 		Icon:               iconRef,
 		Logo:               logoRef,
+		// TODO: add namespaceMetrics


Suggested change

// TODO: add namespaceMetrics

// TODO: in the future will need to support including exactly one of clusterMetrics or namespaceMetrics based on request context

workspaces/backend/internal/models/workspacekinds/funcs.go

andyatmiami · 2025-07-01T20:43:53Z

workspaces/backend/internal/models/workspacekinds/funcs.go

+func buildOptionMetricsMap(metrics []kubefloworgv1beta1.OptionMetric) map[string]int32 {
+	resultMap := make(map[string]int32)
+	for _, metric := range metrics {
+		resultMap[metric.Id] = metric.Workspaces
+	}
+	return resultMap
+}


I am legitimately torn here on a subtle and potentially in-this-context-unimportant implementation decision 😇

for _, metric := range metrics will perform a copy on the elements defined in metrics

To avoid the copy - you can declare the for loop as: for i := range metrics

However, I am then unsure how the body of the loop should be constructed

avoid copy (readable): metric := &metrics[i]

as this is a pointer - if in the future someone modified this implementation for some reason to change this value - it would also change the actual element in the slice

avoid copy (less readable): resultMap[metrics[i].Id] = metrics[i].Workspaces

less likely for someone in future to "accidentally" re-assign a value - but also not as visually pleasing to read

explicit copy: metric := metrics[i]

intent more clear as compared to for _, metric := range metrics

as these structs are small - copy overhead not a concern

given these objects we are handling are inherently small (and bounded to be reasonably sure they will always be small) - the code here as written is sufficient.

…ma response Signed-off-by: rafriat <[email protected]>

andyatmiami · 2025-07-11T12:39:58Z

/lgtm

github-project-automation bot added this to Kubeflow Notebooks May 21, 2025

github-project-automation bot moved this to Needs Triage in Kubeflow Notebooks May 21, 2025

google-oss-prow bot requested review from kimwnasptd and thesuperzapper May 21, 2025 11:53

google-oss-prow bot added the size/S label May 21, 2025

google-oss-prow bot added the ok-to-test label May 21, 2025

roee1313 force-pushed the RHOAIENG-25098 branch from e751a98 to df2debf Compare May 27, 2025 13:53

google-oss-prow bot added size/L and removed size/S labels May 27, 2025

roee1313 force-pushed the RHOAIENG-25098 branch 2 times, most recently from b47474c to 15e589e Compare May 28, 2025 11:55

andyatmiami suggested changes May 29, 2025

View reviewed changes

roee1313 force-pushed the RHOAIENG-25098 branch from 15e589e to 59637e6 Compare June 3, 2025 11:48

google-oss-prow bot added size/M and removed size/L labels Jun 3, 2025

roee1313 force-pushed the RHOAIENG-25098 branch from 59637e6 to 3d88f7a Compare June 3, 2025 12:27

andyatmiami reviewed Jun 9, 2025

View reviewed changes

roee1313 force-pushed the RHOAIENG-25098 branch 4 times, most recently from 0e43df9 to f8c3234 Compare June 10, 2025 13:06

google-oss-prow bot assigned andyatmiami Jun 10, 2025

google-oss-prow bot added the lgtm label Jun 10, 2025

andyatmiami mentioned this pull request Jun 11, 2025

[TASK] Frontend should leverage workspaceCounts attribute in /workspacekinds API response #418

Open

andyatmiami suggested changes Jun 16, 2025

View reviewed changes

google-oss-prow bot added size/L and removed lgtm size/M labels Jun 16, 2025

roee1313 force-pushed the RHOAIENG-25098 branch 3 times, most recently from ceec598 to 88d0e77 Compare June 25, 2025 07:55

andyatmiami reviewed Jul 1, 2025

View reviewed changes

workspaces/backend/internal/models/workspacekinds/funcs.go Show resolved Hide resolved

andyatmiami reviewed Jul 1, 2025

View reviewed changes

workspaces/backend/internal/models/workspacekinds/funcs.go Show resolved Hide resolved

andyatmiami reviewed Jul 1, 2025

View reviewed changes

google-oss-prow bot added area/backend area - related to backend components area/v2 area - version - kubeflow notebooks v2 labels Jul 6, 2025

feat(ws): Notebooks 2.0 // Backend // add count to workspacekind sche…

11a3b8e

…ma response Signed-off-by: rafriat <[email protected]>

roee1313 force-pushed the RHOAIENG-25098 branch from 9775a37 to 11a3b8e Compare July 6, 2025 08:46

thesuperzapper changed the title ~~feat(ws): Notebooks 2.0 // Backend // add count to workspacekind sche…~~ feat(ws): add ws counts to backend wsk model Jul 10, 2025

andyatmiami approved these changes Jul 11, 2025

View reviewed changes

google-oss-prow bot added the lgtm label Jul 11, 2025

	ClusterMetrics clusterMetrics `json:"clusterMetrics"`
	ClusterMetrics clusterMetrics `json:"clusterMetrics,omitempty"`

	// TODO: add namespaceMetrics
	// TODO: in the future will need to support including exactly one of clusterMetrics or namespaceMetrics based on request context

feat(ws): add ws counts to backend wsk model #368

Are you sure you want to change the base?

feat(ws): add ws counts to backend wsk model #368

Uh oh!

Conversation

roee1313 commented May 21, 2025

Uh oh!

google-oss-prow bot commented May 21, 2025

Uh oh!

jiridanek commented May 21, 2025

Uh oh!

andyatmiami left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

google-oss-prow bot commented May 29, 2025

Uh oh!

roee1313 commented Jun 7, 2025

Uh oh!

andyatmiami Jun 9, 2025

Choose a reason for hiding this comment

Uh oh!

roee1313 Jun 10, 2025

Choose a reason for hiding this comment

Uh oh!

andyatmiami commented Jun 10, 2025

Uh oh!

andyatmiami commented Jun 10, 2025

Uh oh!

andyatmiami left a comment

Choose a reason for hiding this comment

Uh oh!

andyatmiami Jul 1, 2025

Choose a reason for hiding this comment

Uh oh!

andyatmiami Jul 1, 2025

Choose a reason for hiding this comment

Uh oh!

andyatmiami Jul 1, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

andyatmiami Jul 1, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

andyatmiami Jul 11, 2025

Choose a reason for hiding this comment

Uh oh!

andyatmiami commented Jul 11, 2025

Uh oh!

Uh oh!

andyatmiami left a comment •

edited

Loading

andyatmiami Jul 1, 2025 •

edited

Loading