feat(ws): Notebooks 2.0 // Backend // Envtest creation #403

yehudit1987 · 2025-06-03T11:29:43Z

This PR solve issue #380

envtest setup.
Factory for clients creations - with that mocked/real manager as well as rest.Config.

google-oss-prow · 2025-06-03T11:29:49Z

[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 thesuperzapper 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

andyatmiami · 2025-06-09T19:11:13Z

workspaces/backend/cmd/main.go

+	kubeconfig.QPS = float32(cfg.ClientQPS)
+	kubeconfig.Burst = cfg.ClientBurst


What is the significance of setting this values here (irrespective of --enable-envtest flag) - when inside the Factory we only define these values for "real" k8s:

https://github.com/kubeflow/notebooks/pull/403/files#diff-a2c11aad15235c5cf40f6f268eb2b16c83b864d7c8e254284825d1d9671cdbe2R99-R100

Feels like these lines shouldn't be here (and any needs to set them should be handled within GetManagerAndConfig function) 🤔

Totally agree, my bad#1

andyatmiami · 2025-06-09T19:14:52Z

workspaces/backend/internal/k8sclientfactory/client_factory.go

+		}
+		f.logger.Info("Local dev K8s API (envtest) is ready.", "host", cfg.Host)
+
+		if testEnvInstance != nil {


What could cause testEnvInstance to be nil at this point in the code?

If this is a possible outcome...

seems wrong to print the "ready" message on L81

seems like there should be an else block helping user understand there could be an issue (or should we exit application at that point?)

For now you are correct that testEnvInstance is not suppose to be nil at that point.
In case there will be any changes in the future with StartLocalDevEnvironment it's more robust (or so I believe) to still verify that option. But again you are right that else is needed here. Fix that, let me know what you think.

andyatmiami · 2025-06-09T19:16:52Z

workspaces/backend/internal/k8sclientfactory/client_factory.go

+func (f *ClientFactory) GetClient(ctx context.Context) (client.Client, func(), error) {
+	mgr, _, cleanup, err := f.GetManagerAndConfig(ctx)
+	if err != nil {
+		return nil, cleanup, err


I wonder if err is defined here.. if we should attempt to invoke the cleanup function at this point.. instead of returning it to the calling code (presumably where it must be invoked)... as then forgetting to invoke cleanup in calling code could cause a problem?

You are absolutely correct. Although this function currently not being used, it will cause problems in the future.
So I have added a check to the error case for cleanup exitance. If it's exist it will be invoked.

andyatmiami · 2025-06-09T19:33:02Z

workspaces/backend/localdev/envsetup.go

+	ctrl.Log.Info("Local dev environment stopped.")
+}
+
+func getProjectRoot() (string, error) {


I am still myself upskilling my knowledge in Golang - so take what I say with a grain of salt!

I worry the reliance on os.Getwd() could lead to strange failure patterns...

From (quick) research - I feel something like the following may be more robust - but open to discussion!

import ( "go/build" "golang.org/x/mod/modfile" ) func getProjectRoot() (string, error) { // First try to find the module root if modRoot := os.Getenv("GOMOD"); modRoot != "" { return filepath.Dir(modRoot), nil } // Fall back to build.Import pkg, err := build.Import("github.com/kubeflow/notebooks", "", build.FindOnly) if err != nil { return "", err } return pkg.Dir, nil }

example:

➜ backend/ git:(feat/#380) $ go env GOMOD /Users/astonebe/Development/Code/GitHub/kubeflow-notebooks/workspaces/backend/go.mod

I see what you are saying. The use of os.Getwd() sets the dependency on the user which is in general less safe.
I did some changes there (hope you'll find them acceptable), but still running that code via the make file is either way possible just from backend path.

If there is other aspect, please share with me so I'll learn.

andyatmiami · 2025-06-09T19:45:01Z

workspaces/backend/localdev/envtest_helper.go

+	return nil
+}
+
+func loadAndCreateWorkspaceKindsFromDir(ctx context.Context, cl client.Client,


FOR DISCUSSION - NOT ADVOCATING FOR CHANGES QUITE YET

I wonder if instead of relying on YAML files that exist in source control.. coupled then with all this application logic.. if it would just be easier to adopt an approach similar to what the controller is doing in its tests

example: https://github.com/kubeflow/notebooks/blob/notebooks-v2/workspaces/controller/internal/controller/suite_test.go#L183

If we defined everything programmatically... I imagine the code to construct the workspaces would be easier to understand/implement as we can rely on const values (or at minimum variables defined purely within the application logic) and not need to parse out values from querying the control plan..

Thoughts?

In general I think that separating the YAML's (the source) from the code (the actions) is much cleaner but I'm open for discussion of course.
YAML way:
pros:

Separate responsibilities

Easy to apply changes

Can be re-used (not sure where but still).

Structs way:
pros:

Types safety (although we will not push PR's with YAML's breaking the go).

No file system dependency (although it's not time consuming).

Simple data flow.

Any other aspects you think about?

It could totally be not a real concern - I just worry about code drift here and its not as obvious/intuitive (i think) to see these manifests outside the controller directory and they are not necessarily guaranteed to get applied on any given PR.. such that some change could conceivably get merged that "breaks" this... whereas if we planned to tie this into helper functions that used throughout the code base (including in controller tests) - that is a more robust way to safeguard against future potential breakages.

admittedly that is probably a fringe concern - as the CRD almost assuredly is going to remain fairly static and/or backwards-compatible..

lets see if anyone else has some thoughts

andyatmiami · 2025-06-09T19:46:55Z

workspaces/backend/localdev/envtest_helper.go

+}
+
+// createInitialResources creates namespaces, WorkspaceKinds, and Workspaces.
+func createInitialResources(ctx context.Context, cl client.Client) error {


Given the initial limited scope of this work - I am comfortable with the answer being "we can address this in a follow up item" ...

But with that disclaimer in mind - I was expecting to see something around volumes here - since the Workspaces being created have reference to a volume.

Thoughts?

Let me handle this in this PR.

… envtest Signed-off-by: Yehudit Kerido <[email protected]>

github-actions · 2025-08-11T00:33:44Z

This pull request has been automatically marked as stale because it has not had recent activity.
It will be closed if no further activity occurs.
Thank you for your contributions.

Members may comment /lifecycle frozen to prevent this pull request from being marked as stale.

github-project-automation bot added this to Kubeflow Notebooks Jun 3, 2025

github-project-automation bot moved this to Needs Triage in Kubeflow Notebooks Jun 3, 2025

google-oss-prow bot requested review from kimwnasptd and thesuperzapper June 3, 2025 11:29

google-oss-prow bot added the size/XXL label Jun 3, 2025

yehudit1987 force-pushed the feat/#380 branch 9 times, most recently from 1f03d2b to 16622f3 Compare June 5, 2025 10:06

andyatmiami reviewed Jun 9, 2025

View reviewed changes

yehudit1987 marked this pull request as draft June 10, 2025 11:03

google-oss-prow bot added the do-not-merge/work-in-progress label Jun 10, 2025

yehudit1987 force-pushed the feat/#380 branch 2 times, most recently from 0e8d9e4 to 3648186 Compare June 11, 2025 07:53

yehudit1987 marked this pull request as ready for review June 11, 2025 07:55

google-oss-prow bot removed the do-not-merge/work-in-progress label Jun 11, 2025

feat(ws): Notebooks 2.0 // Backend // Backend can read data served by…

670b172

… envtest Signed-off-by: Yehudit Kerido <[email protected]>

yehudit1987 force-pushed the feat/#380 branch from 3648186 to 670b172 Compare June 11, 2025 08:31

github-actions bot added the lifecycle/stale label Aug 11, 2025

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

feat(ws): Notebooks 2.0 // Backend // Envtest creation #403

feat(ws): Notebooks 2.0 // Backend // Envtest creation #403

Uh oh!

yehudit1987 commented Jun 3, 2025

Uh oh!

google-oss-prow bot commented Jun 3, 2025

Uh oh!

andyatmiami Jun 9, 2025

Uh oh!

yehudit1987 Jun 10, 2025

Uh oh!

andyatmiami Jun 9, 2025

Uh oh!

yehudit1987 Jun 10, 2025

Uh oh!

andyatmiami Jun 9, 2025

Uh oh!

yehudit1987 Jun 10, 2025

Uh oh!

andyatmiami Jun 9, 2025

Uh oh!

yehudit1987 Jun 10, 2025

Uh oh!

andyatmiami Jun 9, 2025

Uh oh!

yehudit1987 Jun 10, 2025

Uh oh!

andyatmiami Jun 10, 2025

Uh oh!

andyatmiami Jun 9, 2025

Uh oh!

yehudit1987 Jun 10, 2025

Uh oh!

github-actions bot commented Aug 11, 2025

Uh oh!

Uh oh!

		kubeconfig.QPS = float32(cfg.ClientQPS)
		kubeconfig.Burst = cfg.ClientBurst

feat(ws): Notebooks 2.0 // Backend // Envtest creation #403

Are you sure you want to change the base?

feat(ws): Notebooks 2.0 // Backend // Envtest creation #403

Uh oh!

Conversation

yehudit1987 commented Jun 3, 2025

Uh oh!

google-oss-prow bot commented Jun 3, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

github-actions bot commented Aug 11, 2025

Uh oh!

Uh oh!