Skip to content

feat: check computed schematic ID to avoid unnecessary machine upgrades #1897

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.

Sign up for GitHub

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

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from
Draft

feat: check computed schematic ID to avoid unnecessary machine upgrades #1897

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions hack/test/integration.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -95,6 +95,8 @@ if [[ "${CI:-false}" == "true" ]]; then
WIREGUARD_IP=172.20.0.1
fi

# TODO: to cover more cases of (unwanted) machine upgrades, populate all schematics in this test with as many different options as possible, e.g., overlays, SecureBootCustomization and so on.

# Prepare schematic with kernel args
KERNEL_ARGS_SCHEMATIC=$(
cat <<EOF
Expand Down
94 changes: 50 additions & 44 deletions internal/backend/runtime/omni/controllers/omni/schematic_configuration.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -53,10 +53,10 @@ func NewSchematicConfigurationController(imageFactoryClient ImageFactoryClient)
UnmapMetadataFunc: func(schematicConfiguration *omni.SchematicConfiguration) *omni.MachineStatus {
return omni.NewMachineStatus(resources.DefaultNamespace, schematicConfiguration.Metadata().ID())
},
TransformExtraOutputFunc: func(ctx context.Context, r controller.ReaderWriter, _ *zap.Logger,
TransformExtraOutputFunc: func(ctx context.Context, r controller.ReaderWriter, logger *zap.Logger,
machineStatus *omni.MachineStatus, schematicConfiguration *omni.SchematicConfiguration,
) error {
status, err := helper.reconcile(ctx, r, machineStatus, schematicConfiguration)
status, err := helper.reconcile(ctx, r, machineStatus, schematicConfiguration, logger)
if err != nil {
return err
}
Expand Down Expand Up @@ -135,11 +135,8 @@ type schematicConfigurationHelper struct {
// Reconcile implements controller.QController interface.
//
//nolint:gocognit,gocyclo,cyclop
func (helper *schematicConfigurationHelper) reconcile(
ctx context.Context,
r controller.ReaderWriter,
ms *omni.MachineStatus,
schematicConfiguration *omni.SchematicConfiguration,
func (helper *schematicConfigurationHelper) reconcile(ctx context.Context, r controller.ReaderWriter, ms *omni.MachineStatus,
schematicConfiguration *omni.SchematicConfiguration, logger *zap.Logger,
) (*omni.MachineExtensionsStatus, error) {
if ms.TypedSpec().Value.Schematic == nil {
return nil, xerrors.NewTaggedf[qtransform.SkipReconcileTag]("schematic information is not yet available")
Expand Down Expand Up @@ -202,7 +199,7 @@ func (helper *schematicConfigurationHelper) reconcile(

overlay := getOverlay(ms.TypedSpec().Value.Schematic)

kernelArgs, err := determineKernelArgs(ctx, ms, r)
kernelArgs, kernelArgsUnchanged, err := determineKernelArgs(ctx, ms, r)
if err != nil {
return nil, err
}
Expand Down Expand Up @@ -230,6 +227,22 @@ func (helper *schematicConfigurationHelper) reconcile(
return nil, err
}

schematicUnchanged := customization.unchanged && kernelArgsUnchanged
idMismatch := id != ms.TypedSpec().Value.Schematic.FullId

// We do a safety check here: if we did not customize anything but the schematic ID appears to have changed,
// we preserve the existing schematic ID to avoid unnecessary machine reboot.
// This is an indicator of a bug, we should find out the cause of the discrepancy and fix it.
if schematicUnchanged && idMismatch {
logger.Warn("schematic ID mismatch detected despite no changes in schematic configuration, preserving existing schematic ID to avoid unnecessary machine reboot",
zap.String("machine_schematic_id", ms.TypedSpec().Value.Schematic.FullId),
zap.String("computed_schematic_id", id),
)

id = ms.TypedSpec().Value.Schematic.FullId
kernelArgs = ms.TypedSpec().Value.Schematic.KernelArgs
}

if _, err = safe.ReaderGetByID[*omni.Schematic](ctx, r, id); err != nil {
if !state.IsNotFoundError(err) {
return nil, err
Expand All @@ -242,7 +255,7 @@ func (helper *schematicConfigurationHelper) reconcile(

schematicConfiguration.TypedSpec().Value.SchematicId = id
schematicConfiguration.TypedSpec().Value.KernelArgs = kernelArgs
machineExtensionsStatus.TypedSpec().Value.Extensions = computeMachineExtensionsStatus(ms, &customization)
machineExtensionsStatus.TypedSpec().Value.Extensions = computeMachineExtensionsStatus(ms, customization.extensionsList, customization.detectedExtensions)

return machineExtensionsStatus, nil
}
Expand Down Expand Up @@ -305,7 +318,7 @@ func newMachineCustomization(ms *omni.MachineStatus, cluster *omni.Cluster, exts

detected, err := getDetectedExtensions(cluster, ms)
if err != nil {
return mc, err
return machineCustomization{}, err
}

mc.detectedExtensions = detected
Expand All @@ -314,7 +327,7 @@ func newMachineCustomization(ms *omni.MachineStatus, cluster *omni.Cluster, exts

clusterVersion, err := semver.ParseTolerant(cluster.TypedSpec().Value.TalosVersion)
if err != nil {
return mc, err
return machineCustomization{}, err
}

if mc.machineExtensions != nil {
Expand All @@ -340,6 +353,8 @@ func newMachineCustomization(ms *omni.MachineStatus, cluster *omni.Cluster, exts
mc.extensionsList = initialState.Extensions
}

mc.unchanged = slices.Equal(ms.TypedSpec().Value.Schematic.Extensions, mc.extensionsList)

return mc, nil
}

Expand All @@ -351,42 +366,43 @@ type machineCustomization struct {
extensionsList []string
detectedExtensions []string
meta []schematic.MetaValue
unchanged bool
}

func determineKernelArgs(ctx context.Context, machineStatus *omni.MachineStatus, r controller.Reader) ([]string, error) {
func determineKernelArgs(ctx context.Context, machineStatus *omni.MachineStatus, r controller.Reader) (args []string, unchanged bool, err error) {
if _, kernelArgsInitialized := machineStatus.Metadata().Annotations().Get(omni.KernelArgsInitialized); !kernelArgsInitialized {
return nil, xerrors.NewTaggedf[qtransform.SkipReconcileTag]("kernel args are not yet initialized")
return nil, false, xerrors.NewTaggedf[qtransform.SkipReconcileTag]("kernel args are not yet initialized")
}

updateSupported, err := kernelargs.UpdateSupported(machineStatus)
if err != nil {
return nil, err
return nil, false, err
}

if !updateSupported {
if machineStatus.TypedSpec().Value.Schematic == nil {
return nil, xerrors.NewTaggedf[qtransform.SkipReconcileTag]("machine schematic is not yet initialized")
return nil, false, xerrors.NewTaggedf[qtransform.SkipReconcileTag]("machine schematic is not yet initialized")
}

// keep them unchanged - take the current args as-is
return machineStatus.TypedSpec().Value.Schematic.KernelArgs, nil
return machineStatus.TypedSpec().Value.Schematic.KernelArgs, true, nil
}

kernelArgs, err := kernelargs.GetUncached(ctx, r, machineStatus.Metadata().ID())
if err != nil && !state.IsNotFoundError(err) {
return nil, err
return nil, false, err
}

args, ok, err := kernelargs.Calculate(machineStatus, kernelArgs)
if err != nil {
return nil, err
return nil, false, err
}

if !ok {
return nil, xerrors.NewTaggedf[qtransform.SkipReconcileTag]("extra kernel args for machine %q are not yet initialized", machineStatus.Metadata().ID())
return nil, false, xerrors.NewTaggedf[qtransform.SkipReconcileTag]("extra kernel args for machine %q are not yet initialized", machineStatus.Metadata().ID())
}

return args, nil
return args, false, nil
Copy link

Copilot AI Nov 20, 2025

Choose a reason for hiding this comment

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

The unchanged return value should be true when the calculated kernel args match the existing kernel args in the machine status. Currently, it always returns false for the normal calculation path, which could prevent the safety check at line 230 from working correctly when kernel args haven't actually changed.

Consider comparing the calculated args with existing args:

unchanged := slices.Equal(args, machineStatus.TypedSpec().Value.Schematic.KernelArgs)
return args, unchanged, nil
Suggested change
return args, false, nil
unchanged := slices.Equal(args, machineStatus.TypedSpec().Value.Schematic.KernelArgs)
return args, unchanged, nil

Copilot uses AI. Check for mistakes.
}

func (mc *machineCustomization) isDetected(value string) bool {
Expand Down Expand Up @@ -427,19 +443,11 @@ func detectExtensions(initialVersion, currentVersion semver.Version) ([]string,
return nil, nil
}

func computeMachineExtensionsStatus(ms *omni.MachineStatus, customization *machineCustomization) []*specs.MachineExtensionsStatusSpec_Item {
var (
installedExtensions set.Set[string]
requestedExtensions set.Set[string]
)

if ms.TypedSpec().Value.Schematic != nil {
installedExtensions = xslices.ToSet(ms.TypedSpec().Value.Schematic.Extensions)
}
func computeMachineExtensionsStatus(ms *omni.MachineStatus, extensionsList, detectedExtensions []string) []*specs.MachineExtensionsStatusSpec_Item {
var installedExtensions, requestedExtensions set.Set[string]

if customization != nil {
requestedExtensions = xslices.ToSet(customization.extensionsList)
}
installedExtensions = xslices.ToSet(ms.TypedSpec().Value.Schematic.Extensions)
requestedExtensions = xslices.ToSet(extensionsList)

allExtensions := set.Values(set.Union(
installedExtensions,
Expand All @@ -454,18 +462,16 @@ func computeMachineExtensionsStatus(ms *omni.MachineStatus, customization *machi
phase specs.MachineExtensionsStatusSpec_Item_Phase
)

if customization != nil {
switch {
// detected extensions should always be pre installed
case customization.isDetected(extension):
phase = specs.MachineExtensionsStatusSpec_Item_Installed

immutable = true
case installedExtensions.Contains(extension) && !requestedExtensions.Contains(extension):
phase = specs.MachineExtensionsStatusSpec_Item_Removing
case !installedExtensions.Contains(extension) && requestedExtensions.Contains(extension):
phase = specs.MachineExtensionsStatusSpec_Item_Installing
}
switch {
// detected extensions should always be pre installed
case slices.Contains(detectedExtensions, extension):
phase = specs.MachineExtensionsStatusSpec_Item_Installed

immutable = true
case installedExtensions.Contains(extension) && !requestedExtensions.Contains(extension):
phase = specs.MachineExtensionsStatusSpec_Item_Removing
case !installedExtensions.Contains(extension) && requestedExtensions.Contains(extension):
phase = specs.MachineExtensionsStatusSpec_Item_Installing
}

statusExtensions = append(statusExtensions,
Expand Down
Loading