Skip to content

Add cluster pre-open hooks + main-process proxy lifecycle#781

Open
yolossn wants to merge 5 commits into
headlamp-downstreamfrom
hl-baremetal-core
Open

Add cluster pre-open hooks + main-process proxy lifecycle#781
yolossn wants to merge 5 commits into
headlamp-downstreamfrom
hl-baremetal-core

Conversation

@yolossn

@yolossn yolossn commented Jul 21, 2026

Copy link
Copy Markdown
Member

Description

Core changes to support AKS Hybrid & Edge (Arc-connected) clusters, which connect through a local az connectedk8s proxy.

Two parts:

  1. Proxy lifecycle (app layer, AKS-specific) — start, stop, and kill-on-quit for the az connectedk8s proxy processes, tracked per cluster.

  2. Cluster pre-open hooks (upstreamable:) — a generic hook that runs before a cluster opens, which plugins can use to prepare a cluster: set up auth, refresh credentials, resume a stopped cluster, start a proxy, etc. The promise gates entry — pending shows a connecting popup, reject surfaces the error, resolve opens the cluster.

Why the app layer

The proxy lifecycle has to live in the app layer — the plugin can't drive it completely. az connectedk8s proxy (and its arcProxy child) is a long-lived process that must survive renderer reloads, stay stoppable by cluster afterwards, and be killed on app quit. The renderer loses its handle on reload and can't run teardown on quit, so only the main process can own start/stop/kill; the plugin just triggers it over IPC.

Testing

Test together with the plugin PR: #782. Test cases are already part of #782

Type of Change

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • Documentation update
  • Performance improvement
  • Code refactoring
  • CI/CD changes
  • Other: **___**

Copilot AI review requested due to automatic review settings July 21, 2026 04:32

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Adds a “cluster pre-open” extension point so plugins can asynchronously prepare a cluster (e.g., start an Arc proxy) before cluster views/auth checks run, and adds Electron main-process lifecycle management for az connectedk8s proxy so it can be started/stopped per cluster and killed on app quit.

Changes:

  • Introduces ClusterPreOpenHook registration + Redux state to track “preparing” clusters and progress messages.
  • Gates cluster route rendering/auth probing behind pre-open preparation, and suppresses the “Lost connection” banner during preparation.
  • Adds main-process IPC + tracking to start/stop AKS Hybrid & Edge proxies per cluster and kill them on before-quit.

Reviewed changes

Copilot reviewed 8 out of 8 changed files in this pull request and generated 6 comments.

Show a summary per file
File Description
frontend/src/redux/clusterProviderSlice.ts Adds pre-open hook types + Redux state (preOpenHooks, preparing) and actions to track preparation.
frontend/src/plugin/registry.tsx Exposes registerClusterProviderPreOpen for plugins to register pre-open hooks.
frontend/src/components/common/AlertNotification.tsx Suppresses the “Lost connection” banner while the current cluster is preparing.
frontend/src/components/App/RouteSwitcher.tsx Runs pre-open hooks before rendering cluster views / probing auth; shows a connecting dialog + progress.
frontend/src/components/App/Home/ClusterContextMenu.tsx Ensures built-in Delete isn’t hidden just because plugins added menu items.
app/electron/runCmd.ts Tracks proxy child processes per cluster; adds IPC to start/stop proxy; provides kill-on-quit utilities.
app/electron/preload.ts Allows renderer to send the new start/stop proxy IPC messages.
app/electron/main.ts Kills any running proxies on app before-quit.

Comment thread frontend/src/components/App/RouteSwitcher.tsx
Comment thread frontend/src/plugin/registry.tsx Outdated
Comment thread frontend/src/plugin/registry.tsx
Comment thread frontend/src/components/common/AlertNotification.tsx
Comment thread app/electron/runCmd.ts
Comment thread app/electron/runCmd.ts
Copilot AI review requested due to automatic review settings July 21, 2026 05:47
@yolossn
yolossn force-pushed the hl-baremetal-core branch from 70cfb04 to 2462e23 Compare July 21, 2026 05:47

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 9 out of 9 changed files in this pull request and generated 5 comments.

Comment thread frontend/src/redux/clusterProviderSlice.ts
Comment thread frontend/src/redux/clusterProviderSlice.ts
Comment thread frontend/src/components/common/AlertNotification.tsx
Comment thread app/electron/runCmd.ts Outdated
Comment thread frontend/src/components/App/RouteSwitcher.tsx
Copilot AI review requested due to automatic review settings July 21, 2026 06:20
@yolossn
yolossn force-pushed the hl-baremetal-core branch from 2462e23 to 1037cf5 Compare July 21, 2026 06:20

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 35 out of 35 changed files in this pull request and generated 2 comments.

Comment thread frontend/src/redux/clusterProviderSlice.ts Outdated
Comment thread app/electron/runCmd.ts Outdated
Copilot AI review requested due to automatic review settings July 21, 2026 06:32
@yolossn
yolossn force-pushed the hl-baremetal-core branch from 1037cf5 to a0d76d5 Compare July 21, 2026 06:32

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 35 out of 35 changed files in this pull request and generated 4 comments.

Comment thread frontend/src/redux/clusterProviderSlice.ts
Comment thread app/electron/runCmd.ts
Comment thread app/electron/runCmd.ts
Comment thread frontend/src/components/App/RouteSwitcher.tsx Outdated
Copilot AI review requested due to automatic review settings July 21, 2026 06:43
@yolossn
yolossn force-pushed the hl-baremetal-core branch from a0d76d5 to c68f5bb Compare July 21, 2026 06:43

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 35 out of 35 changed files in this pull request and generated 2 comments.

Comments suppressed due to low confidence (1)

frontend/src/redux/clusterProviderSlice.ts:52

  • The JSDoc for clusterConf says it can be null, but the exported type is unknown (non-nullable). This makes hook authors handle a value the type system claims can’t happen, and can cause unnecessary narrowing/casts in plugins. Make the type reflect the documented nullability.

Comment thread app/electron/runCmd.ts Outdated
Comment thread frontend/src/components/App/RouteSwitcher.tsx Outdated
Copilot AI review requested due to automatic review settings July 21, 2026 07:12
@yolossn
yolossn force-pushed the hl-baremetal-core branch from c68f5bb to aa84f0d Compare July 21, 2026 07:12

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 35 out of 35 changed files in this pull request and generated no new comments.

Comments suppressed due to low confidence (1)

app/electron/runCmd.ts:143

  • stopProxyForCluster schedules a new SIGKILL timeout and registers a new exit listener every time it’s called. If a user/plugin issues repeated Stop requests (double-click, retries), this can stack multiple timers/listeners for the same child process (eventually causing MaxListeners warnings and redundant signals). Consider making the delayed SIGKILL fallback idempotent per tracked proxy.
  const killTimer = setTimeout(() => {
    if (entry.child.exitCode === null && entry.child.signalCode === null) {
      signalChildEntry(entry, 'SIGKILL');
    }
  }, 4000);

@illume illume left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some questions:

  • who upstreams this into headlamp, and what happens then?
  • Can we support this in headlamp without a plugin?
    • What about other proxies/tunnels? Maybe we can support ssh tunnels at the same time, and just leave this in headlamp.
  • When we remove az command, what happens then?
    • there was some investigation into using APIs instead
  • Did you think about in-cluster support?
  • desktop API can be called by other plugins/code
    • consider scriptjs
  • atomic commits, and commits messages need bodies
  • The PR description doesn't describe, no testing instructions, and there's no link to issues/PRs etc
  • new component states should have storybook states
  • some of this AKS desktop proxy code in headlamp looks in the wrong place

@yolossn
yolossn force-pushed the hl-baremetal-core branch from aa84f0d to 0a49313 Compare July 22, 2026 08:41
Copilot AI review requested due to automatic review settings July 22, 2026 13:48

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 38 out of 38 changed files in this pull request and generated 1 comment.

Comments suppressed due to low confidence (1)

frontend/src/redux/clusterProviderSlice.ts:53

  • ClusterPreOpenContext.clusterConf is documented as "... or null if unavailable" and AuthRoute can pass null when it can’t find a config. Consider reflecting that in the exported type so hook authors know to handle the null case explicitly (instead of relying on unknown to implicitly include it).

Comment thread app/electron/runCmd.ts Outdated
@yolossn

yolossn commented Jul 22, 2026

Copy link
Copy Markdown
Member Author

Some questions:

  • who upstreams this into headlamp, and what happens then?

I can do it once the feature is merged, or it can also be part of the usual Headlamp ↔ aksd rebase.

  • Can we support this in headlamp without a plugin?

If you mean the preOpen hook. Yes, it lives in core and any plugin can register against it; nothing about it is AKS-specific. Could you expand on the question a bit so I answer the right thing? Happy to go deeper if you meant something else.

  • What about other proxies/tunnels? Maybe we can support ssh tunnels at the same time, and just leave this in headlamp.

Happy to keep the core lifecycle generic and drop the AKS-specific naming. But SSH/port-forward as first-class tunnels is a separate design, so I'd keep it out of scope here and do it as a follow-up if there's demand.

  • When we remove az command, what happens then?
    there was some investigation into using APIs instead

Doing the proxy over the API instead of the az command isn't straightforward — it's a websocket relay, not a plain REST call. I've also been told the proxy is only an interim access mechanism on the Hybrid/Edge cluster side; they plan to move away from the proxy-command dependency themselves. So I've kept the transport decoupled from the lifecycle here, which means when that migration happens we swap the transport without reworking start/stop/kill.

  • Did you think about in-cluster support?

Not possible by design. The approach relies on a machine-local az login and a local loopback proxy — in a shared/in-cluster deployment that would mean one user's credentials serving everyone and one user's proxy being reachable by others, i.e. a data leak. So it's desktop-only and single-user by nature.

  • desktop API can be called by other plugins/code
    consider scriptjs

The scriptjs/pluginRunCommand pattern is fire-and-forget command execution — it works when the CLI owns its own lifecycle, i.e. start and stop are independent, stateless calls and the process daemonizes itself.

The Arc proxy is different. az connectedk8s proxy is a long-lived foreground process with no stop command — to stop it you have to kill the exact process you spawned (and its arcProxy child). So something persistent has to hold that process handle to stop it later or kill it on app quit. The renderer can't: it loses the handle on reload and can't run teardown on quit. That's why start/stop/kill lives in the app layer keyed by cluster

  • atomic commits, and commits messages need bodies

The commits already has message bodies, broke down the commits into smaller ones.

  • The PR description doesn't describe, no testing instructions, and there's no link to issues/PRs etc
    new component states should have storybook states

Added storybook wherever necessary and updated the PR description.

  • some of this AKS desktop proxy code in headlamp looks in the wrong place

The proxy lifecycle has to live in the app layer — the plugin can't drive it completely. It's a long-lived OS process (az connectedk8s proxy + its arcProxy child) that must survive renderer reloads, stay stoppable by cluster afterwards, and be killed on app quit. The renderer loses its handle on reload and can't run teardown on quit, so only the main process can own start/stop/kill; the plugin just triggers it over IPC. Like the other AKS-specific commits already part of headlamp-downstream. The AKS specifics here are by design for the fork, not code meant to go upstream, so it's consistent with how we already scope AKS-specific changes. The pieces I did intend for upstream are split out and tagged as upstreamable

yolossn added 5 commits July 22, 2026 19:57
Adds main-process handlers to start, stop, and force-stop (on app quit) the
AKS Hybrid/Edge proxy child processes, tracked per cluster.

Also hardens that lifecycle:
- Windows: force-kill the child tree with taskkill /T /F (no POSIX process
  groups; SIGTERM and SIGKILL both map to a forced tree kill).
- Keep a stopped proxy tracked until its child actually exits, so a quick
  Start after Stop can't spawn a second proxy that fights over the port.
- Cancel the SIGKILL fallback once the child exits, avoiding a signal to a
  reused PID.
- Only lifecycle-manage the real 'az connectedk8s proxy' invocation, and
  reserve/settle in-flight starts so back-to-back start/stop IPC can't leak
  or outlive a proxy.

Signed-off-by: yolossn <sannagaraj@microsoft.com>
Adds a preOpen hook that plugins register via registerClusterProviderPreOpen.
A hook runs before a cluster's views render and its promise gates entry:
while pending the app shows a connecting state, a rejection surfaces the
error with a retry, and resolving opens the cluster. Plugins use it for
async preparation - auth/credential setup, telemetry, resuming a stopped
cluster, or starting a local proxy.

Includes the redux state (preOpenHooks plus a 'preparing' map of progress
messages), the RouteSwitcher connecting UI, and suppressing the transient
'Lost connection' banner while a cluster is being prepared.

Signed-off-by: yolossn <sannagaraj@microsoft.com>
…menu item

The built-in Delete item was gated on 'no plugin menu items registered', so
registering any cluster provider menu item hid Delete for every cluster. The
gate assumed a provider that adds menu items also provides its own delete,
but plugin menu items are additive. Show Delete based on the cluster's
source/permissions instead.

Signed-off-by: yolossn <sannagaraj@microsoft.com>
Regenerated translation catalogs for the new connecting/preparing strings
added by the cluster preOpen hook UI.

Signed-off-by: yolossn <sannagaraj@microsoft.com>
Extract the connecting popup out of RouteSwitcher into a pure
ClusterPreparingDialog so its states are storybook-able, and add stories
for it and for the AlertNotification suppressed state.

Signed-off-by: yolossn <sannagaraj@microsoft.com>
@yolossn
yolossn force-pushed the hl-baremetal-core branch from 832c8b9 to 00e219f Compare July 22, 2026 14:48
Copilot AI review requested due to automatic review settings July 22, 2026 14:48

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 41 out of 41 changed files in this pull request and generated 1 comment.

Comments suppressed due to low confidence (1)

app/electron/runCmd.ts:140

  • stopProxyForCluster can be called multiple times for the same tracked child (e.g. repeated IPC Stop events). Each call currently schedules a new SIGKILL fallback timer and adds another once('exit', ...) listener, which can lead to unnecessary timers/listener accumulation and repeated kill attempts.

Consider making Stop idempotent per tracked proxy by storing (and reusing) a single pending kill timer on the tracked entry, and returning early when a stop is already in progress.

  // Force-kill fallback a few seconds later, only if the child hasn't already
  // exited — avoids ESRCH noise and, more importantly, signalling a reused PID.
  // The timer is cancelled when the child exits (the common case after SIGTERM).
  const killTimer = setTimeout(() => {
    if (entry.child.exitCode === null && entry.child.signalCode === null) {

Comment thread frontend/src/redux/clusterProviderSlice.ts
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants