feat: implement activity rules in controller by siyuanfoundation · Pull Request #1279 · kubeflow/notebooks

siyuanfoundation · 2026-07-24T21:52:23Z

closes: #867

This PR implements the following:

Implement activity probing via Jupyter API polling and pod exec scripts
Evaluate activity rules to determine when workspaces should be paused
Integrate with reconciliation to schedule probes and enforce culling decisions

Tested with tilt using JupyterLab WorkspaceKind.

verified eligibleAfter is properly populated, and the workspace is paused after being idle exceeding the secondsSinceActive
workspace is restarted and back running.
workspace is not paused if workspace stays active, and when activity is resumed in time before the next probe.

google-oss-prow · 2026-07-24T21:52:29Z

[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 andyatmiami for approval. For more information see the Kubernetes Code Review Process.

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

Details

Needs approval from an approver in each of these files:

workspaces/backend/OWNERS
workspaces/controller/OWNERS

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

siyuanfoundation · 2026-07-27T17:09:53Z

/retest

Signed-off-by: siyuanfoundation <sizhang@google.com>

siyuanfoundation · 2026-07-27T18:30:48Z

/cc @andyatmiami @thesuperzapper

christian-heusel · 2026-07-28T11:11:29Z

/retitle feat: implement activity rules in controller

christian-heusel

Hey @siyuanfoundation, this is really awesome stuff, thanks for pushing this forward 🔥

I don't have any big comments, but noted down a few things that struck me as odd or small improvements, feel free to apply as sensible to you. Of course for the final review the WG leads and potentially other controller experts can chime in! 🤗

Generally I found that we still mix the naming from culling and activityProbe, I think that should be standardized before merge 😄

christian-heusel · 2026-07-28T14:34:28Z

+		// allow multiple Workspaces to be reconciled (and probed) in parallel so that a
+		// slow activity probe does not block other Workspaces' reconciliation
+		MaxConcurrentReconciles: maxConcurrentReconciles,


I looked into this a bit because I feared that allowing concurrent reconciles might introduce a race, however I convinced myself that this is not an issue since both the ws and i.e. the sts will collapse to the same key (namespace/X) in the workqueue and there is only one key being worked on at the time.

Is this assesment correct?

christian-heusel · 2026-07-28T17:58:25Z

 		return ctrl.Result{}, nil
 	}

+	// snapshot the Workspace as fetched, so the culling logic can issue a minimal


Suggested change

// snapshot the Workspace as fetched, so the culling logic can issue a minimal

// snapshot the Workspace as fetched, so the activityProbe logic can issue a minimal

christian-heusel · 2026-07-28T17:59:29Z

Given that the feature was renamed to activityProbe this should most likely also be reflected in the filename and throughout it (i.e. in reconcileActivityCulling, etc.)

workspace_activity.go probably a good name... and yes.. culling is a specific feature that can be configured with Activity Probe + Activity Rules - we we'd want files and functions ideally to not reference culling in the name..

user defines activity probe(s) and activity rule(s) - and an outcome of those configurations could be that a workspace is culled.. but in future - if/as we add other "outcomes" - we wouldn't want Culling in any named files/functions as that would get confusing..

christian-heusel · 2026-07-28T18:24:17Z

+		runningDurationMs := now - lastRunningTime
+		if runningDurationMs < int64(minRunningSeconds)*millisPerSecond {
+			return false, eligibleAfter
+		}


Maybe we could do something like this and get rid of the millisPerSecond?

runningDuration := time.Duration(now-lastRunningTime) * time.Millisecond if runningDuration < time.Duration(minRunningSeconds)*time.Second { return false, eligibleAfter }

christian-heusel · 2026-07-28T18:26:55Z

+	if lastActivity <= 0 {
+		return 0
+	}
+	return lastActivity + int64(secondsSinceActive)*millisPerSecond


Suggested change

return lastActivity + int64(secondsSinceActive)*millisPerSecond

return lastActivity + (time.Duration(secondsSinceActive) * time.Second).Milliseconds()

christian-heusel · 2026-07-28T18:59:56Z

+func EvaluateActivityRule[T any](
+	rules []kubefloworgv1beta1.ActivityRule,
+	namespaceLabels, podConfigLabels map[string]string,
+	getEffect func(kubefloworgv1beta1.ActivityRuleEffect) *T,
+) ActivityRuleDecision[T] {


I'm curious, why did you decide to make EvaluateActivityRule() generic? 🤔 AFAIU it can currently only ever be a bool?

christian-heusel · 2026-07-28T19:05:38Z

+type ProbeResult struct {
+	// StartTime is the time the probe was started (UNIX epoch in milliseconds).
+	StartTime int64
+
+	// EndTime is the time the probe completed (UNIX epoch in milliseconds).
+	EndTime int64
+
+	// Result is the outcome of the probe (Success, Failure, or Timeout).
+	Result kubefloworgv1beta1.WorkspaceProbeResult
+
+	// Message is a human-readable message about the probe result.
+	Message string
+
+	// LastActivity is the activity timestamp determined by the probe (UNIX epoch in
+	// milliseconds). It is nil when the probe did not succeed or when the activity timestamp
+	// should not be updated.
+	LastActivity *int64
+}


Any reason you chose the time-related fields in ProbeResult to be of type int64? I would have imagined that since we use this type internally as a helper its easier to use the native go time type and then convert via UnixMilli(int64) / time.UnixMilli() as needed 🤔

christian-heusel · 2026-07-28T19:20:55Z

+	shellCommand := fmt.Sprintf(
+		"cat > %[1]s && chmod +x %[1]s && OUTPUT_JSON_PATH=%[2]s %[1]s; rc=$?; rm -f %[1]s; "+
+			"if [ $rc -ne 0 ]; then exit $rc; fi; "+
+			"if [ -f %[2]s ]; then cat %[2]s; rm -f %[2]s; fi",
+		scriptPath, outputPath,
+	)
+	command := []string{"/bin/sh", "-c", shellCommand}


Even though scriptPath can not contain spaces ([/.a-z0-9-]) we should apply proper variable quoting in this section, since paths generally can have spaces.

$ shellcheck test.sh In test.sh line 2: cat > $1 && chmod +x $1 && OUTPUT_JSON_PATH=$2 $1; rc=$?; rm -f $1; ^-- SC2086 (info): Double quote to prevent globbing and word splitting. ^-- SC2086 (info): Double quote to prevent globbing and word splitting. ^-- SC2086 (info): Double quote to prevent globbing and word splitting. Did you mean: cat > "$1" && chmod +x "$1" && OUTPUT_JSON_PATH=$2 $1; rc=$?; rm -f "$1"; In test.sh line 4: if [ -f $2 ]; then cat $2; rm -f $2; fi ^-- SC2086 (info): Double quote to prevent globbing and word splitting. ^-- SC2086 (info): Double quote to prevent globbing and word splitting. ^-- SC2086 (info): Double quote to prevent globbing and word splitting. Did you mean: if [ -f "$2" ]; then cat "$2"; rm -f "$2"; fi For more information: https://www.shellcheck.net/wiki/SC2086 -- Double quote to prevent globbing ...

christian-heusel · 2026-07-29T18:19:57Z

/assign

andyatmiami

@siyuanfoundation ... don't let number of comments here "get it twisted" - this is a very impressive contribution and its obvious the effort/attention you put into this - so thank you.

Aside from the comments I have explicitly called out in the review - I also wanetd to flag a general pattern..

There are 15+ error and result message strings scattered across RunJupyterProbe, RunPodExecProbe, runProbe, and parsePodExecOutput, many sharing repeated prefixes like "Jupyter probe failed: " and "PodExec probe failed: ". Extracting the prefixes and common messages as constants at the top of the file (alongside the existing maxProbeResponseBytes, probeContainerName, jupyterStatusPath constants) would:
- Make the full message contract visible at a glance
- Reduce prefix duplication
- Make it easier to keep messaging consistent if new probe types are added

ℹ️ As always - feel free to question/push back on any of these comments... I did a lot of staring at call stacks while reviewing - so its possible I am just "confused" on some of my feedback - keep that in mind 😇

andyatmiami · 2026-08-02T14:40:51Z

 	Config *config.EnvConfig
+
+	// PodExecutor executes activity probe scripts inside Workspace Pods.
+	// If nil, podExec probes are skipped (used when exec is not configured, e.g. in some tests).


This comment seems misleading - as if I am reading this code corrected... we return a Failure result:

https://github.com/kubeflow/notebooks/pull/1279/changes#diff-b87d913cf3d02827ba01d1b701b76b068f60741f6082cdf72a78830ad462209aR208-R215

Perhaps that is technically "skipping" - but maybe a little more specificity here would be good

andyatmiami · 2026-08-02T14:43:58Z

 // +kubebuilder:rbac:groups=apps,resources=statefulsets,verbs=create;delete;get;list;patch;update;watch
 // +kubebuilder:rbac:groups=core,resources=events,verbs=get;list;watch
 // +kubebuilder:rbac:groups=core,resources=configmaps,verbs=get;list;watch
+// +kubebuilder:rbac:groups=core,resources=namespaces,verbs=get;list;watch


Maybe for @thesuperzapper to weigh in here...

It makes sense we'd use the controller-runtime's cached client here.. but we are also now introducing a shared informer that list+watches all namespaces cluster-wide..

Are we concerned about cardinality here?

andyatmiami · 2026-08-02T14:45:44Z

 	}
 	workspace.Status = workspaceStatus

+	// reconcile the activity probe and culling rules


Suggested change

// reconcile the activity probe and culling rules

// reconcile the activity probe and activity rules

andyatmiami · 2026-08-02T14:53:40Z

+	if workspaceState == kubefloworgv1beta1.WorkspaceStateRunning {
+		if workspace.Status.State != kubefloworgv1beta1.WorkspaceStateRunning || status.LastRunningTime == 0 {
+			status.LastRunningTime = metav1.Now().UnixMilli()
+			status.Activity = kubefloworgv1beta1.WorkspaceActivity{}
+		}
+	}


Doesn't seem like we need nested-ifs here (and perhaps a helper function to implement the conditional would be beneficial and a more nature place to add these helpful inline comments..

andyatmiami · 2026-08-02T14:55:44Z

workspace_activity.go probably a good name... and yes.. culling is a specific feature that can be configured with Activity Probe + Activity Rules - we we'd want files and functions ideally to not reference culling in the name..

user defines activity probe(s) and activity rule(s) - and an outcome of those configurations could be that a workspace is culled.. but in future - if/as we add other "outcomes" - we wouldn't want Culling in any named files/functions as that would get confusing..

andyatmiami · 2026-08-02T20:32:01Z

+	if ns.Labels == nil {
+		return map[string]string{}, nil
+	}
+	return ns.Labels, nil


The Namespace object is from the controller-runtime cache. While current versions deep-copy on Get, returning maps.Clone(ns.Labels) would make the safety guarantee explicit and future-proof.

andyatmiami · 2026-08-02T20:34:44Z

+
+// selectorMatches converts a metav1.LabelSelector and evaluates it against the given labels.
+func selectorMatches(selector *metav1.LabelSelector, lbls map[string]string) (bool, error) {
+	sel, err := metav1.LabelSelectorAsSelector(selector)


EvaluateActivityRule calls metav1.LabelSelectorAsSelector() per rule per reconcile, parsing and compiling the selector each time. For a small number of rules this is fine, but with thousands of workspaces all referencing the same WorkspaceKind, the same selectors are recompiled on every reconcile. Not a blocker — selector compilation is fast — but at extreme scale (10K+ workspaces, frequent probes), caching compiled selectors per WorkspaceKind would eliminate redundant work.

andyatmiami · 2026-08-02T20:36:20Z

+	probeInterval := time.Duration(ptr.Deref(activityProbe.ProbeIntervalSeconds, kubefloworgv1beta1.DefaultProbeIntervalSeconds)) * time.Second
+
+	// gather the labels used by activity rule matching.
+	namespaceLabels, err := r.getNamespaceLabels(ctx, workspace.Namespace)


ℹ️ getNamespaceLabels is called on every reconcile regardless of whether any activityRules define a matchNamespace.

I just want to transparently call this out - but I don't think the additional complexity is warranted to try and defer this call - as then we'd seemingly be required to iterate rules 2x... negating any desired performance benefit.

andyatmiami · 2026-08-02T20:37:49Z

+
+		matched, err := activityRuleMatches(rule.Match, namespaceLabels, podConfigLabels)
+		if err != nil {
+			// treat an invalid selector as non-matching (validation should prevent this).


Part of me would want at least a debug log (or something) here to alert us if we someone entered this logic branch.... but then threading a logger into this function seems overkill...

WDYT?

andyatmiami · 2026-08-02T20:39:25Z

+			TTY:       false,
+		}, scheme.ParameterCodec)
+
+	exec, err := remotecommand.NewSPDYExecutor(e.RestConfig, http.MethodPost, req.URL())


⚠️ This is another Claude review flag - I'd prefer to get your opinion here - and push back if/as necessary!

This project currently targets Kubernetes 1.31+ and uses client-go v0.31.0, where WebSocket-based exec is GA. This proposed implementation uses remotecommand.NewSPDYExecutor (the legacy SPDY protocol), which still works but is the older path. The Kubernetes ecosystem is moving toward WebSocket exec, and some managed Kubernetes offerings may eventually deprecate the SPDY exec endpoint. Consider using NewFallbackExecutor which tries WebSocket first and falls back to SPDY for broader compatibility

github-project-automation Bot added this to Kubeflow Notebooks Jul 24, 2026

github-project-automation Bot moved this to Needs Triage in Kubeflow Notebooks Jul 24, 2026

google-oss-prow Bot added the area/backend area - related to backend components label Jul 24, 2026

google-oss-prow Bot requested review from andyatmiami and thesuperzapper July 24, 2026 21:52

google-oss-prow Bot added area/controller area - related to controller components area/v2 area - version - kubeflow notebooks v2 size/XXL labels Jul 24, 2026

siyuanfoundation force-pushed the culling branch from edfc6ba to 21eeeec Compare July 27, 2026 16:04

feat: implment activity rules in controller

6d0cf15

Signed-off-by: siyuanfoundation <sizhang@google.com>

siyuanfoundation force-pushed the culling branch from 21eeeec to 6d0cf15 Compare July 27, 2026 17:34

google-oss-prow Bot changed the title ~~feat: implment activity rules in controller~~ feat: implement activity rules in controller Jul 28, 2026

christian-heusel reviewed Jul 28, 2026

View reviewed changes

google-oss-prow Bot assigned christian-heusel Jul 29, 2026

andyatmiami requested changes Aug 2, 2026

View reviewed changes

google-oss-prow Bot assigned andyatmiami Aug 2, 2026

	// snapshot the Workspace as fetched, so the culling logic can issue a minimal
	// snapshot the Workspace as fetched, so the activityProbe logic can issue a minimal

	return lastActivity + int64(secondsSinceActive)*millisPerSecond
	return lastActivity + (time.Duration(secondsSinceActive) * time.Second).Milliseconds()

	// reconcile the activity probe and culling rules
	// reconcile the activity probe and activity rules

Uh oh!

Conversation

siyuanfoundation commented Jul 24, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

google-oss-prow Bot commented Jul 24, 2026

Uh oh!

siyuanfoundation commented Jul 27, 2026

Uh oh!

siyuanfoundation commented Jul 27, 2026

Uh oh!

christian-heusel commented Jul 28, 2026

Uh oh!

christian-heusel left a comment

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!

christian-heusel commented Jul 29, 2026

Uh oh!

andyatmiami left a comment

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!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

siyuanfoundation commented Jul 24, 2026 •

edited

Loading