Run preflights in manager UI / API #2234

sgalsaleh · 2025-06-03T14:56:50Z

What this PR does / why we need it:

Added ability to run preflight checks in the manager UI/API, including status monitoring, error handling, and rerun capabilities.

Which issue(s) this PR fixes:

SC-124343
SC-124239
SC-124379
SC-124345

Does this PR require a test?

Yes

Does this PR require a release note?

NONE

Does this PR require documentation?

NONE

…new-installers

…ers' into run-preflights-in-manager-ui

github-actions · 2025-06-04T18:53:41Z

This PR has been released (on staging) and is available for download with a embedded-cluster-smoke-test-staging-app license ID.

Online Installer:

curl "https://staging.replicated.app/embedded/embedded-cluster-smoke-test-staging-app/ci/appver-dev-ff2d783" -H "Authorization: $EC_SMOKE_TEST_LICENSE_ID" -o embedded-cluster-smoke-test-staging-app-ci.tgz

Airgap Installer (may take a few minutes before the airgap bundle is built):

curl "https://staging.replicated.app/embedded/embedded-cluster-smoke-test-staging-app/ci-airgap/appver-dev-ff2d783?airgap=true" -H "Authorization: $EC_SMOKE_TEST_LICENSE_ID" -o embedded-cluster-smoke-test-staging-app-ci.tgz

Happy debugging!

handle preflights error add rerun add start install btn

…hts-in-manager-ui

JGAntunes

Haven't finished reviewing but submitting what I have right now.

api/Makefile

JGAntunes · 2025-06-05T10:56:26Z

api/api.go

+		installController, err := install.NewInstallController(
+			install.WithRuntimeConfig(api.rc),
+			install.WithLogger(api.logger),
+			install.WithHostUtils(api.hostUtils),
+			install.WithMetricsReporter(api.metricsReporter),
+			install.WithReleaseData(api.releaseData),
+			install.WithLicenseFile(api.licenseFile),
+			install.WithAirgapBundle(api.airgapBundle),
+		)


We should likely review some of these struct properties to understand if they need to be pointers to structs and we're making that choice deliberately or not. Right now I'm worried that we receive pointers to structs that we then pass to the controllers (and maybe these can even end up being used by things such as managers). If some end up unintentionally mutating state we can be in trouble trying to debug a particularly airy bug.

api/client/client_test.go

JGAntunes · 2025-06-05T12:49:58Z

api/install.go

 		a.logError(r, err, "failed to set installation config")
 		a.jsonError(w, r, err)
 		return
 	}

-	a.getInstall(w, r)
+	a.getInstallInstallationStatus(w, r)


Should we be returning just the InstallationStatus? Maybe it's ok for now but I'm wondering if it would be worthwile sending the config too. Maybe it's the same object entirely so no point in that I guess? 🤔

why would the client need the config if it just sent it to configure the installation? i would assume it should get the status as a confirmation that it's "running" now

If we end up mutating the config in some way and we want to let the client know (e.g. this doesn't really apply here because the client probably doesn't need to know about it but imagine something like pod-cidr and service-cidr which is calculated by the API). But we can definitely cross that bridge if we ever get there.

api/install.go

api/controllers/install/installation.go

api/internal/managers/installation/manager.go

api/internal/managers/installation/status.go

JGAntunes · 2025-06-05T16:41:43Z

api/internal/managers/installation/status.go

+func (m *installationManager) isRunning() (bool, error) {
+	status, err := m.GetStatus()
+	if err != nil {
+		return false, err
+	}
+	return status.State == types.StateRunning, nil
+}


I don't think this will be a problem right now (and maybe it will never be?), but I wonder if we'll ever need to make this an atomic operation at the store level to protect us from potential problems in the future? Something like m.installationStore.IsStatus(types.StateRunning)

hmm, i might agree that it should ideally be atomic, but probably not at the store level. IMO, the store shouldn't have such logic and should simply be an interface for CRUD operations.

We talked it over and we both agreed it makes sense to do this at the manager level 👍

Follow up ticket - https://app.shortcut.com/replicated/story/124680

api/internal/managers/preflight/hostpreflight.go

JGAntunes · 2025-06-05T17:34:11Z

api/install.go

 		a.logError(r, err, "failed to set installation config")
 		a.jsonError(w, r, err)
 		return
 	}

-	a.getInstall(w, r)
+	a.getInstallInstallationStatus(w, r)


If we end up mutating the config in some way and we want to let the client know (e.g. this doesn't really apply here because the client probably doesn't need to know about it but imagine something like pod-cidr and service-cidr which is calculated by the API). But we can definitely cross that bridge if we ever get there.

api/controllers/install/installation.go

JGAntunes · 2025-06-05T17:45:24Z

api/controllers/install/node.go

+	if preflightStatus.State != types.StateFailed && preflightStatus.State != types.StateSucceeded {
+		return fmt.Errorf("host preflight checks did not complete")
+	}


In that case I think we should have something within the Status type, something like isFinal that we can call and that would return a bool telling us if the given state is final or not. It's more scoped and we can keep it all under one place.

JGAntunes · 2025-06-05T17:54:34Z

api/controllers/install/node.go

+	if preflightStatus.State == types.StateFailed && c.metricsReporter != nil {
+		preflightOutput, err := c.GetHostPreflightOutput(ctx)
+		if err != nil {
+			return fmt.Errorf("get install host preflight output: %w", err)
+		}
+		if preflightOutput != nil {
+			c.metricsReporter.ReportPreflightsFailed(ctx, preflightOutput)
+		}
+	}
+
+	// TODO: implement node setup
+
+	return nil
+}


Talked it over with @sgalsaleh we should gate this at the API level, requiring some property to be sent over to the API.

JGAntunes · 2025-06-05T18:03:54Z

api/controllers/install/installation.go

+	// update the runtime config
+	c.rc.SetDataDir(config.DataDirectory)
+	c.rc.SetLocalArtifactMirrorPort(config.LocalArtifactMirrorPort)
+	c.rc.SetAdminConsolePort(config.AdminConsolePort)


Talked it over with @sgalsaleh and we agreed that ideally we would only need the runtime config towards the end of the installation flow. We even debated about the installation config being the runtime config essentially. However a good milestone to have would be to aim to entirely work with the installation config initially and towards the end of the flow map it out to a runtime config (this would already be helpful).

For now though:

but maybe we add a UpdateFromInstallationConfig method to the runtimeconfig interface

This would already be super helpful since it would mean we have a single place we can track back in our code where we're mutating the runtime config.

JGAntunes · 2025-06-05T18:13:01Z

api/controllers/install/status.go

+}
+
+func (c *InstallController) GetStatus(ctx context.Context) (*types.Status, error) {
+	return c.install.Status, nil


We talked it over and maybe something to look for in the future would be to have generic stores (disk and memory) that we can pass over to each of the individual stores (likely using generics).

pkg-new/hostutils/initialize.go

JGAntunes · 2025-06-05T18:23:36Z

api/internal/managers/installation/status.go

+func (m *installationManager) isRunning() (bool, error) {
+	status, err := m.GetStatus()
+	if err != nil {
+		return false, err
+	}
+	return status.State == types.StateRunning, nil
+}


We talked it over and we both agreed it makes sense to do this at the manager level 👍

emosbaugh · 2025-06-05T20:07:30Z

api/internal/managers/installation/config.go

+		return fmt.Errorf("set running status: %w", err)
+	}
+
+	go m.configureForInstall(context.Background(), config)


Can you add a comment for your reasoning for background here so that it is not mistakenly changed in the future?

…ager-ui

api/internal/managers/preflight/hostpreflight.go

JGAntunes · 2025-06-06T10:02:57Z

api/internal/managers/preflight/store.go

+func (s *MemoryStore) IsRunning() bool {
+	s.mu.RLock()
+	defer s.mu.RUnlock()
+
+	return s.hostPreflight.Status.State == types.StateRunning
+}


We should follow a similar pattern to what we described here and have this be a mutexed method at the manager level - #2234 (comment)

Follow up ticket - https://app.shortcut.com/replicated/story/124680

JGAntunes · 2025-06-06T10:05:29Z

api/types/preflight.go

+// HostPreflight represents the host preflight checks state
+type HostPreflight struct {
+	Titles []string             `json:"titles"`
+	Output *HostPreflightOutput `json:"output"`
+	Status *Status              `json:"status"`


Should this be HostPreflights instead? E.g. the response is InstallHostPreflightsStatusResponse

Follow up ticket created - https://app.shortcut.com/replicated/story/124682/rename-hostpreflight-to-hostpreflights

cmd/installer/cli/api.go

JGAntunes · 2025-06-06T10:18:43Z

cmd/installer/cli/api.go

+	if hostname != "" {
+		return fmt.Sprintf("https://%s:%v", hostname, port)
+	}
+	ipaddr := runtimeconfig.TryDiscoverPublicIP()


Not a "now problem" but I was looking through this method the other day, is there any reason why this lives in the runtimeconfig package currently? Seems totally unrelated unless I'm missing something.

no reason, was thinking the same. we should move it

Created follow up ticket -https://app.shortcut.com/replicated/story/124684

operator/charts/embedded-cluster-operator/Chart.yaml

pkg/helpers/fs_test.go

JGAntunes

Overall lgtm. There's some feedback that remains to be addressed but given this PR is blocking a lot of work it's something that we can create follow up PRs/tickets to do.

sgalsaleh · 2025-06-06T14:04:24Z

Merging. Will address feedback in follow up PRs. Also, the failing test is known to be flaky.

miaawong and others added 15 commits May 29, 2025 16:28

wip preflgihts ui

3af8ff1

update types to match api preflight structure

088dcdd

add comment

f4dd47a

Merge branch 'main' into miawong/sc-124239/preflight-ui-step-for-the-…

86ac03e

…new-installers

fix for loading web

548468b

Merge branch 'main' into miawong/sc-124239/preflight-ui-step-for-the-…

1d964e6

…new-installers

f

e4035b6

Merge branch 'main' into miawong/sc-124239/preflight-ui-step-for-the-…

d90cd90

…new-installers

fix the web dev

b82dbfc

Run host preflights in manager UI

f0dca98

Merge branch 'miawong/sc-124239/preflight-ui-step-for-the-new-install…

a15bd63

…ers' into run-preflights-in-manager-ui

f

c77250a

f

d2d10d2

preflight checks

45643ba

[skip ci] testing

2d1b7eb

sgalsaleh added the type::feature label Jun 3, 2025

f

3825c4e

sgalsaleh force-pushed the run-preflights-in-manager-ui branch from ad9ed44 to 3825c4e Compare June 3, 2025 18:55

sgalsaleh added 7 commits June 3, 2025 15:28

f

56ce746

runtimeconfig interface

3db1168

Merge branch 'main' into run-preflights-in-manager-ui

a8c01e1

f

b04f5fc

f

e932601

fix dagger build

c617d9c

fix dagger build

6ca3f08

miaawong added 4 commits June 4, 2025 15:19

add polling for installation/status

5bc987c

handle preflights error add rerun add start install btn

update

11d558b

f

5810a7e

f

cd592a4

miaawong and others added 7 commits June 4, 2025 15:19

f

cb8f113

fix dbus error

c91c351

[skip ci] f

5763381

Merge branch 'miawong/preflights-manager-ui-cleanup' into run-preflig…

4adddd9

…hts-in-manager-ui

[skip ci] no-op

34fbd09

fix ui

9acd699

more ui improvements

3b78a3e

sgalsaleh marked this pull request as ready for review June 5, 2025 00:12

sgalsaleh requested review from emosbaugh and JGAntunes June 5, 2025 00:12

sgalsaleh changed the title ~~Run preflights in manager UI~~ Run preflights in manager UI / API Jun 5, 2025

miaawong added 3 commits June 5, 2025 11:08

update complete step wording

464411d

f

b23c198

clean up success message

d5d92e6

JGAntunes reviewed Jun 5, 2025

View reviewed changes

sgalsaleh added 4 commits June 5, 2025 10:26

address feedback

87ad72c

regenerate swagger

39b45fe

fix tests

4d77b19

no-op

4204e58

JGAntunes reviewed Jun 5, 2025

View reviewed changes

emosbaugh reviewed Jun 5, 2025

View reviewed changes

Merge remote-tracking branch 'origin/main' into run-preflights-in-man…

ff2d783

…ager-ui

JGAntunes reviewed Jun 6, 2025

View reviewed changes

JGAntunes approved these changes Jun 6, 2025

View reviewed changes

sgalsaleh merged commit 9bf21ed into main Jun 6, 2025
245 of 252 checks passed

sgalsaleh deleted the run-preflights-in-manager-ui branch June 6, 2025 14:04

sgalsaleh mentioned this pull request Jun 6, 2025

Address feedback from new API preflights PR #2255

Merged

sgalsaleh restored the run-preflights-in-manager-ui branch June 6, 2025 14:08

Run preflights in manager UI / API #2234

Run preflights in manager UI / API #2234

Uh oh!

Conversation

sgalsaleh commented Jun 3, 2025 • edited by miaawong Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What this PR does / why we need it:

Which issue(s) this PR fixes:

Does this PR require a test?

Does this PR require a release note?

Does this PR require documentation?

Uh oh!

github-actions bot commented Jun 4, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

JGAntunes left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

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!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

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!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

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!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

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!

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!

Uh oh!

Uh oh!

JGAntunes left a comment

Choose a reason for hiding this comment

Uh oh!

sgalsaleh commented Jun 3, 2025 •

edited by miaawong

Loading

github-actions bot commented Jun 4, 2025 •

edited

Loading