Skip to content

feat: preflight storage needed to perform airgap install #2336

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 28 commits into
base: main
Choose a base branch
from

Conversation

banjoh
Copy link
Member

@banjoh banjoh commented Jun 18, 2025

What this PR does / why we need it:

Add host preflight checks to ensure nodes have enough storage to perform an airgap install when installing a new node, and when joining a node to an existing cluster. Separate preflights have been added for workers and controller type nodes.

Pending tasks
  • Running manual tests with a real airgap install to justify airgap multiplier. Its 2x at the moment, but might increase to 3x
  • Manual tests with new build to test the new preflights

Which issue(s) this PR fixes:

sc-123579

Does this PR require a test?

  • Automated tests to new changes added
  • Manual tests to check if preflight works

Does this PR require a release note?

Add host preflight to check storage needed to perform airgap install

Does this PR require documentation?

NONE

@banjoh banjoh marked this pull request as draft June 18, 2025 12:19
Signed-off-by: Evans Mungai <[email protected]>
Copy link

github-actions bot commented Jun 18, 2025

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-9f3562b" -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-9f3562b?airgap=true" -H "Authorization: $EC_SMOKE_TEST_LICENSE_ID" -o embedded-cluster-smoke-test-staging-app-ci.tgz

Happy debugging!

@banjoh banjoh marked this pull request as ready for review June 19, 2025 12:58
// Calculate airgap storage space requirement (2x uncompressed size for controller nodes)
var controllerAirgapStorageSpace string
if c.airgapBundle != "" {
airgapInfo, err := airgap.AirgapInfoFromPath(c.airgapBundle)
Copy link
Member

@emosbaugh emosbaugh Jun 19, 2025

Choose a reason for hiding this comment

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

could we move this to the flag parsing code in preRunInstall so it is only run once and fails much earlier? Then make airgapInfo a property of InstallController like airgapBundle.

Copy link
Member Author

Choose a reason for hiding this comment

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

This method gets called by postInstallRunHostPreflights which is a http request served by the controller. At which point does preRunInstall come in?

Copy link
Member

Choose a reason for hiding this comment

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

preRunInstall is called in the cli and stores properties for later use in the InstallCmdFlags struct. You can add an airgapInfo property and pass this to the api server and store it as a property similar to airgapBundle on the API and InstallController structs.

banjoh added 2 commits June 19, 2025 15:19
Signed-off-by: Evans Mungai <[email protected]>
Signed-off-by: Evans Mungai <[email protected]>
@banjoh banjoh force-pushed the evansmungai/sc-123579/take-airgap-bundle-size-into-account-when branch 2 times, most recently from 520aa03 to 5e53eca Compare June 19, 2025 17:06
banjoh added 14 commits June 19, 2025 18:07
Signed-off-by: Evans Mungai <[email protected]>
Signed-off-by: Evans Mungai <[email protected]>
Signed-off-by: Evans Mungai <[email protected]>
…unt-when' of github.com:replicatedhq/embedded-cluster into evansmungai/sc-123579/take-airgap-bundle-size-into-account-when
Signed-off-by: Evans Mungai <[email protected]>
Signed-off-by: Evans Mungai <[email protected]>
Signed-off-by: Evans Mungai <[email protected]>
Signed-off-by: Evans Mungai <[email protected]>
Comment on lines 102 to 110
var airgapInfo *kotsv1beta1.Airgap
if m.airgapBundle != "" {
var err error
airgapInfo, err = airgap.AirgapInfoFromPath(m.airgapBundle)
if err != nil {
return fmt.Errorf("failed to get airgap info: %w", err)
}
}

Copy link
Member

Choose a reason for hiding this comment

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

Can you follow the same pattern described here where you make airgapInfo a property of the manager and pass it with the function WithAirgapInfo?

Comment on lines 100 to 106
if flags.airgapBundle != "" {
var err error
airgapInfo, err = airgap.AirgapInfoFromPath(flags.airgapBundle)
if err != nil {
return fmt.Errorf("failed to get airgap info: %w", err)
}
}
Copy link
Member

Choose a reason for hiding this comment

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

Is there any reason this is not in the preRunInstall function as suggested? That way it exists with the rest of the flag parsing code and will work for both install and install run-preflights command.

Copy link
Member Author

Choose a reason for hiding this comment

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

Isn't InstallCmdFlags meant for CLI flags? That's why I did not move the new flag there.

Copy link
Member Author

Choose a reason for hiding this comment

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

I'll create a new flag like the rest

@@ -426,6 +435,7 @@ func runManagerExperienceInstall(ctx context.Context, flags InstallCmdFlags, rc
ManagerPort: flags.managerPort,
License: flags.licenseBytes,
AirgapBundle: flags.airgapBundle,
AirgapInfo: airgapInfo,
Copy link
Member

Choose a reason for hiding this comment

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

Is there some reason you chose not to make airgapInfo a property of InstallCmdFlags as suggested? That way you will be following the existing pattern and dont have to pass it as an additional argument to many functions.

Comment on lines 48 to 54
if flags.airgapBundle != "" {
var err error
airgapInfo, err = airgap.AirgapInfoFromPath(flags.airgapBundle)
if err != nil {
return fmt.Errorf("failed to get airgap info: %w", err)
}
}
Copy link
Member

Choose a reason for hiding this comment

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

Move this to preRunInstall

Comment on lines 142 to 147
// read file from path
var err error
airgapInfo, err = airgap.AirgapInfoFromPath(flags.airgapBundle)
if err != nil {
return fmt.Errorf("failed to get airgap bundle versions: %w", err)
}
Copy link
Member

Choose a reason for hiding this comment

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

Move this to preRunInstall

@@ -218,21 +228,28 @@ func (m *infraManager) installK0s(ctx context.Context, rc runtimeconfig.RuntimeC
return k0sCfg, nil
}

func (m *infraManager) recordInstallation(ctx context.Context, kcli client.Client, license *kotsv1beta1.License, rc runtimeconfig.RuntimeConfig) (*ecv1beta1.Installation, error) {
func (m *infraManager) recordInstallation(ctx context.Context, kcli client.Client, license *kotsv1beta1.License, rc runtimeconfig.RuntimeConfig, airgapInfo *kotsv1beta1.Airgap) (*ecv1beta1.Installation, error) {
Copy link
Member

Choose a reason for hiding this comment

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

this will no longer need to be an argument as infraManager will have a property airgapInfo

@@ -98,6 +99,14 @@ func (m *infraManager) install(ctx context.Context, rc runtimeconfig.RuntimeConf
return fmt.Errorf("parse license: %w", err)
}

if m.airgapBundle != "" {
var err error
m.airgapInfo, err = airgap.AirgapInfoFromPath(m.airgapBundle)
Copy link
Member

Choose a reason for hiding this comment

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

this is passed to the api though, why extract it from the bundle again?

Copy link
Member

Choose a reason for hiding this comment

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

it makes it hard to test the api without an airgap bundle

Copy link
Member Author

@banjoh banjoh Jun 24, 2025

Choose a reason for hiding this comment

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

That's a good call out. The API server has the struct, so it can be passed down to the manager from the controller.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants