Add IdeSession API kind and reconciler (#169)#172
Draft
karolz-ms wants to merge 1 commit into
Draft
Conversation
Introduce a new top-level DCP Kubernetes resource, `IdeSession`, that lets
a user/tool spin up a stand-alone IDE debug session for workloads that are
not modeled as Executables (e.g. Blazor WebAssembly clients).
API
---
* New `IdeSession` kind with:
- `Spec.LaunchConfigurations string` -- pass-through JSON array of
launch configurations (same shape as the
`executable.../launch-configurations` annotation).
- `Spec.DesiredState IdeSessionState` -- one of `""` (Initial),
`Running`, `Stopped`. Session does not start until the user sets
it to `Running`; once `Stopped` it cannot go back to `Running`.
- `Status.State IdeSessionState` -- one of `""`, `Starting`,
`Running`, `Stopping`, `Stopped`, `Failed`. Canonical
transition is Initial -> Starting -> Running -> Stopping -> Stopped;
any non-initial state may also transition directly to `Failed`.
- `Status.Message`, `Status.SessionID`, `Status.PID`,
`Status.ExitCode`, `Status.StdOutFile`, `Status.StdErrFile`,
`Status.StartupTimestamp`, `Status.FinishTimestamp`.
* Validation enforces the restricted `Spec.DesiredState` set, requires
a non-empty JSON array of launch configurations, and rejects
`LaunchConfigurations` changes after creation.
Refactor (prerequisite)
-----------------------
The IDE plumbing previously inside `IdeExecutableRunner` (handshake,
WebSocket notification stream, request/response payloads, run-session
lifecycle) is extracted into a new `internal/ide` package with a small
public surface (`ide.Client`, `ide.SessionHandler`,
`ide.StartSessionRequest` / `StartSessionResult`). `IdeExecutableRunner`
is rewritten on top of `ide.Client`, dropping ~1500 lines of duplicated
logic. The new `IdeSession` controller reuses the same client.
Controller
----------
`IdeSessionReconciler` follows the established DCP pattern:
* embeds `ReconcilerBase` and uses the state-initializer map approach
(one initializer per state),
* stores per-session run info in an `ObjectStateMap` keyed by
namespaced name (primary) and IDE session ID (secondary), with a
transient `starting-<UID>` key while the start request is in flight,
* uses two bounded `WorkQueue`s (startup / stop) for async IDE calls,
* feeds IDE notifications back into the reconciler via deferred map ops
plus `ScheduleReconciliation` so all state mutations are observed by
the next reconciliation pass.
Wiring
------
* `run_controllers.go` constructs an `ide.Client` once and shares it
between the existing IDE executable runner factory and the new
`IdeSessionReconciler`.
* `internal/apiserver/apiserver.go` registers the new kind.
Tests
-----
* Unit tests for the API type (state validity, transition rules,
validation webhook behavior).
* 9 integration tests covering: stays-Initial-without-DesiredRunning,
start-then-stop, startup failure, IDE-side termination, clean
zero-exit termination, deletion while active, rejection of
launch-config change, propagation of launch configurations to the
IDE, and stdout-file recording. A `TestIdeSessionClient` fake in
`internal/testutil/ctrlutil` lets tests drive the IDE lifecycle.
Bug worth calling out: `manageIdeSession` only ran `Update` on the
run map when `runInfo` was non-nil at entry, but
`manageIdeSessionInitial` pre-Store'd a fresh run info with
`state=Initial` and then mutated the local copy to Starting /
`startQueued=true`. Because the original slot was nil, the mutated copy
was never persisted, so the next reconciliation pass kept seeing
`state=Initial` and re-enqueued the startup. Fixed by having the
initial-state initializer bootstrap the map entry with the FINAL
(post-mutation) run info itself; `manageIdeSession` still only does
`Update` when there was already a stored entry.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Contributor
There was a problem hiding this comment.
Pull request overview
This PR introduces a new cluster-scoped IdeSession Kubernetes resource and controller to manage stand-alone IDE debug sessions (not tied to Executable), and refactors IDE protocol handling into a shared internal/ide client that’s reused by both the IdeSession controller and the IDE Executable runner.
Changes:
- Adds
IdeSessionAPI type (validation, OpenAPI, deepcopy/model name generation) and registers it for log streaming. - Implements
IdeSessionReconciler+ in-memory run tracking, plus integration/unit tests and a fake IDE session client for test driving lifecycle. - Extracts IDE execution protocol handling into
internal/ideand rewritesIdeExecutableRunnerto use the sharedide.Client.
Reviewed changes
Copilot reviewed 20 out of 23 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| test/integration/test_env_common.go | Adds IdeSessionController flag for integration test environment wiring. |
| test/integration/standard_test_env.go | Wires IdeSessionReconciler into the standard integration test env and exposes TestIdeSessionClient. |
| test/integration/ide_session_controller_test.go | Adds integration coverage for IdeSession lifecycle, immutability, propagation, and log-file recording. |
| test/integration/controllers_common_test.go | Plumbs TestIdeSessionClient into shared integration test globals. |
| pkg/generated/openapi/zz_generated.openapi.go | Registers OpenAPI schemas for the new IdeSession types. |
| internal/testutil/ctrlutil/test_ide_session_client.go | Adds a fake IdeSessionClient for integration tests. |
| internal/ide/session.go | Introduces per-session internal state + dispatch gating for IDE notifications. |
| internal/ide/protocol.go | Moves protocol types/constants into the new ide package and defines the client-facing interfaces/types. |
| internal/ide/protocol_test.go | Updates protocol tests to the new ide package. |
| internal/ide/notification_handler.go | Refactors the WS notification handler into the new ide package and internalizes receiver APIs. |
| internal/ide/connection.go | Refactors IDE connection handshake/request construction into the new ide package. |
| internal/ide/client.go | Adds shared ide.Client that performs handshake, starts/stops sessions, and dispatches notifications. |
| internal/exerunners/ide_run_data.go | Removes the old IDE runner per-run state implementation (superseded by internal/ide). |
| internal/exerunners/ide_executable_runner.go | Rewrites IDE executable runner to delegate to the shared ide.Client. |
| internal/dcpctrl/commands/run_controllers.go | Creates and shares a single ide.Client; wires the new IdeSessionReconciler. |
| internal/apiserver/apiserver.go | Registers IdeSession as a log-streamable resource. |
| controllers/ide_session_run_info.go | Adds in-memory run-info model + state transition application to status. |
| controllers/ide_session_controller.go | Implements the IdeSession reconciler, queues, run map, and IDE notification bridge. |
| api/v1/zz_generated.model_name.go | Adds OpenAPI model names for IdeSession types. |
| api/v1/zz_generated.deepcopy.go | Adds deepcopy implementations for IdeSession types. |
| api/v1/ide_session_types.go | Adds the IdeSession API type, validation, log subresource, and scheme registration. |
| api/v1/ide_session_types_test.go | Adds unit tests for IdeSession validation and state transition helpers. |
| api/v1/groupversion_info.go | Registers IdeSession GVR and type with the API server. |
Files not reviewed (2)
- api/v1/zz_generated.deepcopy.go: Language not supported
- api/v1/zz_generated.model_name.go: Language not supported
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+245
to
+262
| // If runInfo is nil, we need to bootstrap a new entry in the run map. We must Store | ||
| // the run info AFTER beginIdeSessionStartup has applied its mutations (state and the | ||
| // startQueued latch), otherwise the next reconciliation pass would observe a stale | ||
| // initial-state entry and would re-enqueue the startup. The caller (manageIdeSession) | ||
| // only Update()s the map when runInfo was non-nil on entry, so the bootstrap path | ||
| // has to perform its own Store(). | ||
| bootstrap := runInfo == nil | ||
| if bootstrap { | ||
| runInfo = NewIdeSessionRunInfo(s) | ||
| } | ||
|
|
||
| runInfo.State = apiv1.IdeSessionStateStarting | ||
| log.V(1).Info("Beginning IdeSession startup") | ||
| change := r.beginIdeSessionStartup(s, runInfo, log) | ||
|
|
||
| if bootstrap { | ||
| r.runs.Store(s.NamespacedName(), runInfoStateKey(runInfo), runInfo.Clone()) | ||
| } |
Comment on lines
+401
to
+421
| finalizeStartup := func(failureErr error) { | ||
| if failureErr != nil { | ||
| pendingUpdate.State = apiv1.IdeSessionStateFailed | ||
| pendingUpdate.Message = failureErr.Error() | ||
| pendingUpdate.FinishTimestamp = metav1.NowMicro() | ||
| } | ||
| runMap := r.runs | ||
| r.runs.QueueDeferredOp(name, func(_ types.NamespacedName, _ ideSessionStateKey, _ *apiv1.IdeSession) { | ||
| // Find the entry under whatever key it lives under and rekey if needed. | ||
| currentKey, _ := runMap.BorrowByNamespacedName(name) | ||
| newKey := currentKey | ||
| if pendingUpdate.SessionID != "" { | ||
| newKey = ideSessionStateKey(pendingUpdate.SessionID) | ||
| } | ||
| if currentKey == newKey { | ||
| _ = runMap.Update(name, currentKey, pendingUpdate) | ||
| } else { | ||
| _ = runMap.UpdateChangingStateKey(name, currentKey, newKey, pendingUpdate) | ||
| } | ||
| }) | ||
| r.ScheduleReconciliation(name) |
|
|
||
| pendingUpdate.State = apiv1.IdeSessionStateRunning | ||
| finalizeStartup(nil) | ||
| startRes.ConfirmHandlerReady() |
Comment on lines
+306
to
+320
| if spec.LaunchConfigurations == "" { | ||
| errorList = append(errorList, field.Required( | ||
| specPath.Child("launchConfigurations"), | ||
| "launchConfigurations is required.")) | ||
| } else { | ||
| // We do not interpret the launch configurations payload, but we do verify it parses as | ||
| // JSON so users can correct obviously-malformed input before the IDE rejects it. | ||
| var parsed json.RawMessage | ||
| if err := json.Unmarshal([]byte(spec.LaunchConfigurations), &parsed); err != nil { | ||
| errorList = append(errorList, field.Invalid( | ||
| specPath.Child("launchConfigurations"), | ||
| spec.LaunchConfigurations, | ||
| fmt.Sprintf("launchConfigurations must be valid JSON: %v", err))) | ||
| } | ||
| } |
Comment on lines
+164
to
+168
| if rawRequest, dumpErr := httputil.DumpRequest(httpReq, true); dumpErr == nil { | ||
| log.V(1).Info("Sending IDE run session request", "Request", string(rawRequest)) | ||
| } else { | ||
| log.V(1).Info("Sending IDE run session request", "URL", httpReq.URL) | ||
| } |
Comment on lines
+199
to
+207
| confirmed := atomic.Bool{} | ||
| confirmReady := func() { | ||
| confirmed.Store(true) | ||
| } | ||
|
|
||
| result := &ide.StartSessionResult{ | ||
| SessionID: sessionID, | ||
| ConfirmHandlerReady: confirmReady, | ||
| } |
Collaborator
Author
|
AI opened the PR without asking me 😑 Still working on it |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #169.
Introduces a new top-level DCP Kubernetes resource,
IdeSession, that lets a user/tool spin up a stand-alone IDE debug session for workloads that are not modeled asExecutables (e.g. Blazor WebAssembly clients).API
New
IdeSessionkind with:Spec.LaunchConfigurations string— pass-through JSON array of launch configurations (same shape as theexecutable.usvc-dev.developer.microsoft.com/launch-configurationsannotation).Spec.DesiredState IdeSessionState— one of""(Initial),Running,Stopped. Session does not start until the user sets it toRunning; onceStoppedit cannot go back toRunning.Status.State IdeSessionState— one of"",Starting,Running,Stopping,Stopped,Failed. Canonical transition isInitial → Starting → Running → Stopping → Stopped; any non-initial state may also transition directly toFailed.StoppedandFailedare terminal.Status.Message,Status.SessionID,Status.PID,Status.ExitCode,Status.StdOutFile,Status.StdErrFile,Status.StartupTimestamp,Status.FinishTimestamp.Validation enforces the restricted
Spec.DesiredStateset, requires a non-empty JSON array of launch configurations, and rejectsLaunchConfigurationschanges after creation.Refactor (prerequisite)
The IDE plumbing previously inside
IdeExecutableRunner(handshake, WebSocket notification stream, request/response payloads, run-session lifecycle) is extracted into a newinternal/idepackage with a small public surface:ide.Client— owns the connection to the IDE.ide.SessionHandler— receives per-session notifications.ide.StartSessionRequest/ide.StartSessionResult.IdeExecutableRunneris rewritten on top ofide.Client, dropping ~1500 lines of duplicated logic. The newIdeSessioncontroller reuses the same client.Controller
IdeSessionReconcilerfollows the established DCP pattern:ReconcilerBaseand uses the state-initializer-map approach (one initializer per state).ObjectStateMapkeyed by namespaced name (primary) and IDE session ID (secondary), with a transientstarting-<UID>key while the start request is in flight.WorkQueues (startup / stop) for async IDE calls.ScheduleReconciliationso all state mutations are observed by the next reconciliation pass.Wiring
run_controllers.goconstructs anide.Clientonce and shares it between the existing IDE executable runner factory and the newIdeSessionReconciler.internal/apiserver/apiserver.goregisters the new kind.Tests
test/integration/ide_session_controller_test.gocovering:TestIdeSessionClientfake ininternal/testutil/ctrlutillets tests drive the IDE lifecycle (start, log/message/process notifications, run-end, startup-failure injection).Bug worth calling out
While bringing up the controller,
manageIdeSessiononly ranUpdateon the run map whenrunInfowas non-nil at entry. ButmanageIdeSessionInitialpre-Store'd a fresh run info withstate=Initialand then mutated the local copy toStarting/startQueued=true. Because the original slot was nil, the mutated copy was never persisted, so the next reconciliation pass kept seeingstate=Initialand re-enqueued the startup. Fixed by having the initial-state initializer bootstrap the map entry byStoreing the FINAL (post-mutation) run info itself;manageIdeSessionstill only doesUpdatewhen there was already a stored entry.Verification
go build ./...cleango vet ./...cleanmake lintclean (0 issues)go test ./... -parallel 8all packages pass (the defaultparallel 32inmake testcauses resource contention on this Windows host and times out a few infrastructure packages; running them individually or withparallel 8they all pass)