Skip to content

OCPBUGS-52302: MCO-1713: Update MachineConfigNode resource to include on cluster image mode fields #5282

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

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

OCPBUGS-52302: MCO-1713: Update MachineConfigNode resource to include on cluster image mode fields #5282

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
749f9b7
bump(api & client-go): bump api to commit 2acafd4 and client-go to co…
isabella-janssen Sep 12, 2025
b77f1c9
controller, daemon, and upgrade monitor: update MachineConfigNode upd…
isabella-janssen Sep 12, 2025
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
4 changes: 2 additions & 2 deletions go.mod
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,8 +34,8 @@ require (
github.com/onsi/gomega v1.36.2
github.com/opencontainers/go-digest v1.0.0
github.com/openshift-eng/openshift-tests-extension v0.0.0-20250722101414-8083129ab8f9
github.com/openshift/api v0.0.0-20250811150514-cc869c87a7f0
github.com/openshift/client-go v0.0.0-20250811163556-6193816ae379
github.com/openshift/api v0.0.0-20250911131931-2acafd4d1ed2
github.com/openshift/client-go v0.0.0-20250911202206-1bc0cb0da03b
github.com/openshift/library-go v0.0.0-20250911074910-e2c18d5abc3a
github.com/openshift/runtime-utils v0.0.0-20230921210328-7bdb5b9c177b
github.com/prometheus/client_golang v1.22.0
Expand Down
8 changes: 4 additions & 4 deletions go.sum
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -591,10 +591,10 @@ github.com/opencontainers/selinux v1.12.0 h1:6n5JV4Cf+4y0KNXW48TLj5DwfXpvWlxXplU
github.com/opencontainers/selinux v1.12.0/go.mod h1:BTPX+bjVbWGXw7ZZWUbdENt8w0htPSrlgOOysQaU62U=
github.com/openshift-eng/openshift-tests-extension v0.0.0-20250722101414-8083129ab8f9 h1:4ZeSM80DVCb5WWB3Q/fyCI9jYXAl9bfrGnFvFONqzN4=
github.com/openshift-eng/openshift-tests-extension v0.0.0-20250722101414-8083129ab8f9/go.mod h1:6gkP5f2HL0meusT0Aim8icAspcD1cG055xxBZ9yC68M=
github.com/openshift/api v0.0.0-20250811150514-cc869c87a7f0 h1:K/EiQZE4lBzGMvk7APzYWRuRUJtfwaD5QGRVcny2J1M=
github.com/openshift/api v0.0.0-20250811150514-cc869c87a7f0/go.mod h1:SPLf21TYPipzCO67BURkCfK6dcIIxx0oNRVWaOyRcXM=
github.com/openshift/client-go v0.0.0-20250811163556-6193816ae379 h1:Xr47DBqFVjpLdU4BTtCS5l2XojbRYap2FIPdSj8YYzU=
github.com/openshift/client-go v0.0.0-20250811163556-6193816ae379/go.mod h1:HouQRy4JgvTBpxcyw1YSD/Lp+wjOaUrxjWFHlMtZsk8=
github.com/openshift/api v0.0.0-20250911131931-2acafd4d1ed2 h1:orbYgUTUMs2asHZhT792jeXlVzOqGFaGo8FbD9ihnsE=
github.com/openshift/api v0.0.0-20250911131931-2acafd4d1ed2/go.mod h1:SPLf21TYPipzCO67BURkCfK6dcIIxx0oNRVWaOyRcXM=
github.com/openshift/client-go v0.0.0-20250911202206-1bc0cb0da03b h1:VQpSjWE8jmsPj+EXB+XABTLmDgg9xtT8/fudB/31/aI=
github.com/openshift/client-go v0.0.0-20250911202206-1bc0cb0da03b/go.mod h1:w7sV33ASK/HcuEb0Ll9qvChZdJwNwqo8GocVAnd7fVY=
github.com/openshift/kubernetes v1.30.1-0.20250716113245-b94367cabf3e h1:M5BrUTglTltZjcRz5ouJBqSw0a60p760Bl520ndOGS0=
github.com/openshift/kubernetes v1.30.1-0.20250716113245-b94367cabf3e/go.mod h1:GwUMe2E0Dqe2YN/Nkg9QWNBktqiTR7y+HFxcIWKshXI=
github.com/openshift/kubernetes/staging/src/k8s.io/api v0.0.0-20250716113245-b94367cabf3e h1:Y70IDoOnCCKQT4lIJxx2KkTifLuqD/vjRrzo1DxZ/iw=
Expand Down
16 changes: 16 additions & 0 deletions pkg/controller/common/layered_node_state.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -321,3 +321,19 @@ func (l *LayeredNodeState) CheckNodeCandidacyForUpdate(layered bool, pool *mcfgv

return true
}

// GetDesiredAnnotationsFromMachineOSConfig gets the desired config version and desired image values from the associated MOSC and MOSB
func (l *LayeredNodeState) GetDesiredAnnotationsFromMachineConfigPool(mcp *mcfgv1.MachineConfigPool) (desriedConfig string) {
return mcp.Spec.Configuration.Name
}

// GetDesiredAnnotationsFromMachineOSConfig gets the desired config version and desired image values from the associated MOSC and MOSB
func (l *LayeredNodeState) GetDesiredAnnotationsFromMachineOSConfig(mosc *mcfgv1.MachineOSConfig, mosb *mcfgv1.MachineOSBuild) (desriedConfig, desiredImage string) {
desiredImage = ""
moscs := NewMachineOSConfigState(mosc)
if moscs.HasOSImage() {
desiredImage = moscs.GetOSImage()
}

return mosb.Spec.MachineConfig.Name, desiredImage
}
14 changes: 13 additions & 1 deletion pkg/controller/node/node_controller.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@ import (
"time"

helpers "github.com/openshift/machine-config-operator/pkg/helpers"
"github.com/openshift/machine-config-operator/pkg/upgrademonitor"

configv1 "github.com/openshift/api/config/v1"
features "github.com/openshift/api/features"
Expand Down Expand Up @@ -1288,14 +1289,19 @@ func (ctrl *Controller) updateCandidateNode(mosc *mcfgv1.MachineOSConfig, mosb *
}

lns := ctrlcommon.NewLayeredNodeState(oldNode)
desiredConfig := ""
desiredImage := ""
if !layered {
lns.SetDesiredStateFromPool(pool)
// If pool is not layered, the desired image annotation is removed (see the `delete`
// call in `SetDesiredStateFromPool`), so only the desired config version must be set.
desiredConfig = lns.GetDesiredAnnotationsFromMachineConfigPool(pool)
} else {
lns.SetDesiredStateFromMachineOSConfig(mosc, mosb)
desiredConfig, desiredImage = lns.GetDesiredAnnotationsFromMachineOSConfig(mosc, mosb)
}

// Set the desired state to match the pool.

newData, err := json.Marshal(lns.Node())
if err != nil {
return err
Expand All @@ -1306,6 +1312,12 @@ func (ctrl *Controller) updateCandidateNode(mosc *mcfgv1.MachineOSConfig, mosb *
return nil
}

// Populate the desired config version and image annotations in the node's MCN
err = upgrademonitor.UpdateMachineConfigNodeSpecDesiredAnnotations(ctrl.fgHandler, ctrl.client, nodeName, desiredConfig, desiredImage)
if err != nil {
klog.Errorf("error populating MCN for desired config version and image updates: %v", err)
}

klog.V(4).Infof("Pool %s: layered=%v node %s update is needed", pool.Name, layered, nodeName)
patchBytes, err := strategicpatch.CreateTwoWayMergePatch(oldData, newData, corev1.Node{})
if err != nil {
Expand Down
123 changes: 123 additions & 0 deletions pkg/daemon/update.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2567,11 +2567,38 @@ func (dn *Daemon) updateLayeredOS(config *mcfgv1.MachineConfig) error {
}
}

// For image mode status reporting we need the node's MCP association to populate its MCN
imageModeStatusReportingEnabled := dn.fgHandler != nil && dn.fgHandler.Enabled(features.FeatureGateImageModeStatusReporting)
pool := ""
if imageModeStatusReportingEnabled {
pool, err = helpers.GetPrimaryPoolNameForMCN(dn.mcpLister, dn.node)
if err != nil {
return err
}
}

if isOsImagePresent {
if err := dn.NodeUpdaterClient.RebaseLayeredFromContainerStorage(newURL); err != nil {
return fmt.Errorf("failed to update OS from local storage: %s: %w", newURL, err)
}
} else {
// Report ImagePulledFromRegistry condition as unknown (pulling)
if imageModeStatusReportingEnabled {
err := upgrademonitor.GenerateAndApplyMachineConfigNodes(
&upgrademonitor.Condition{State: mcfgv1.MachineConfigNodeImagePulledFromRegistry, Reason: string(mcfgv1.MachineConfigNodeImagePulledFromRegistry), Message: fmt.Sprintf("Pulling OS image %s from registry", newURL)},
nil,
metav1.ConditionUnknown,
metav1.ConditionFalse,
dn.node,
dn.mcfgClient,
dn.fgHandler,
pool,
)
if err != nil {
klog.Errorf("Error setting ImagePulledFromRegistry condition to unknown: %v", err)
}
}

// Workaround for OCPBUGS-43406, retry the remote rebase with backoff,
// such that if we happen to update while the CoreDNS pod is being restarted,
// the next retry should succeed if no other issues are present.
Expand All @@ -2588,8 +2615,41 @@ func (dn *Daemon) updateLayeredOS(config *mcfgv1.MachineConfig) error {
}
return true, nil
}); err != nil {
// Report ImagePulledFromRegistry condition as false (failed)
if imageModeStatusReportingEnabled {
err = upgrademonitor.GenerateAndApplyMachineConfigNodes(
&upgrademonitor.Condition{State: mcfgv1.MachineConfigNodeImagePulledFromRegistry, Reason: string(mcfgv1.MachineConfigNodeImagePulledFromRegistry), Message: fmt.Sprintf("Failed to pull OS image %s from registry: %v", newURL, err)},
nil,
metav1.ConditionFalse,
metav1.ConditionFalse,
dn.node,
dn.mcfgClient,
dn.fgHandler,
pool,
)
if err != nil {
klog.Errorf("Error setting ImagePulledFromRegistry condition to false: %v", err)
}
}
return fmt.Errorf("Failed to update OS to %s after retries: %w", newURL, err)
}

// Report ImagePulledFromRegistry condition as true (success)
if imageModeStatusReportingEnabled {
err := upgrademonitor.GenerateAndApplyMachineConfigNodes(
&upgrademonitor.Condition{State: mcfgv1.MachineConfigNodeImagePulledFromRegistry, Reason: string(mcfgv1.MachineConfigNodeImagePulledFromRegistry), Message: fmt.Sprintf("Successfully pulled OS image %s from registry", newURL)},
nil,
metav1.ConditionTrue,
metav1.ConditionFalse,
dn.node,
dn.mcfgClient,
dn.fgHandler,
pool,
)
if err != nil {
klog.Errorf("Error setting ImagePulledFromRegistry condition to true: %v", err)
}
}
}

return nil
Expand Down Expand Up @@ -2727,6 +2787,7 @@ func (dn *Daemon) reboot(rationale string) error {
return nil
}

//nolint:gocyclo
Copy link
Contributor

Choose a reason for hiding this comment

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

unrelated tou your chang and not blocker: This nolint (and the one in generateAndApplyMachineConfigNodes) is relevant, upgrade_monitor is, at least IMHO (and some basic software metrics, like cyclomatic complexity, maximum annidation, max method/function length, max number of parameters, etc), unmaintanable. As a new team member reading that code for the first time, I felt really unconfident interpreting it and I'd feel like that if I need to touch or interact with it.
I'm not 100% how the golang linter computes cyclomatic complexity and maybe this new nolint is only required because the extra ifs required to evaluate the FeatureGate.

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 agree with your thoughts here. I've created MCO-1902 to track this need and make sure it does not get lost. Thanks for bringing this up!

func (dn *CoreOSDaemon) applyLayeredOSChanges(mcDiff machineConfigDiff, oldConfig, newConfig *mcfgv1.MachineConfig) (retErr error) {
// Override the computed diff if the booted state differs from the oldConfig
// https://issues.redhat.com/browse/OCPBUGS-2757
Expand All @@ -2738,13 +2799,75 @@ func (dn *CoreOSDaemon) applyLayeredOSChanges(mcDiff machineConfigDiff, oldConfi
var osExtensionsContentDir string
var err error

// For image mode status reporting we need the node's MCP association to populate its MCN
imageModeStatusReportingEnabled := dn.fgHandler != nil && dn.fgHandler.Enabled(features.FeatureGateImageModeStatusReporting)
pool := ""
if imageModeStatusReportingEnabled {
pool, err = helpers.GetPrimaryPoolNameForMCN(dn.mcpLister, dn.node)
if err != nil {
return err
}
}

if newConfig.Spec.BaseOSExtensionsContainerImage != "" && (mcDiff.osUpdate || mcDiff.extensions || mcDiff.kernelType) && !mcDiff.oclEnabled {
// TODO(jkyros): the original intent was that we use the extensions container as a service, but that currently results
// in a lot of complexity due to boostrap and firstboot where the service isn't easily available, so for now we are going
// to extract them to disk like we did previously.

// Report ImagePulledFromRegistry condition as unknown (pulling)
if imageModeStatusReportingEnabled {
err := upgrademonitor.GenerateAndApplyMachineConfigNodes(
&upgrademonitor.Condition{State: mcfgv1.MachineConfigNodeImagePulledFromRegistry, Reason: string(mcfgv1.MachineConfigNodeImagePulledFromRegistry), Message: fmt.Sprintf("Pulling extensions image %s from registry", newConfig.Spec.BaseOSExtensionsContainerImage)},
nil,
metav1.ConditionUnknown,
metav1.ConditionFalse,
dn.node,
dn.mcfgClient,
dn.fgHandler,
pool,
)
if err != nil {
klog.Errorf("Error setting ImagePulledFromRegistry condition to unknown: %v", err)
}
}

if osExtensionsContentDir, err = ExtractExtensionsImage(newConfig.Spec.BaseOSExtensionsContainerImage); err != nil {
// Report ImagePulledFromRegistry condition as false (failed)
if imageModeStatusReportingEnabled {
err := upgrademonitor.GenerateAndApplyMachineConfigNodes(
&upgrademonitor.Condition{State: mcfgv1.MachineConfigNodeImagePulledFromRegistry, Reason: string(mcfgv1.MachineConfigNodeImagePulledFromRegistry), Message: fmt.Sprintf("Failed to pull extensions image %s from registry: %v", newConfig.Spec.BaseOSExtensionsContainerImage, err)},
nil,
metav1.ConditionFalse,
metav1.ConditionFalse,
dn.node,
dn.mcfgClient,
dn.fgHandler,
pool,
)
if err != nil {
klog.Errorf("Error setting ImagePulledFromRegistry condition to false: %v", err)
}
}
return err
}

// Report ImagePulledFromRegistry condition as true (success)
if imageModeStatusReportingEnabled {
err := upgrademonitor.GenerateAndApplyMachineConfigNodes(
&upgrademonitor.Condition{State: mcfgv1.MachineConfigNodeImagePulledFromRegistry, Reason: string(mcfgv1.MachineConfigNodeImagePulledFromRegistry), Message: fmt.Sprintf("Successfully pulled extensions image %s from registry", newConfig.Spec.BaseOSExtensionsContainerImage)},
nil,
metav1.ConditionTrue,
metav1.ConditionFalse,
dn.node,
dn.mcfgClient,
dn.fgHandler,
pool,
)
if err != nil {
klog.Errorf("Error setting ImagePulledFromRegistry condition to true: %v", err)
}
}

// Delete extracted OS image once we are done.
defer os.RemoveAll(osExtensionsContentDir)

Expand Down
Loading