Skip to content
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
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
35 changes: 35 additions & 0 deletions pkg/daemon/daemon.go
Original file line number Diff line number Diff line change
Expand Up @@ -198,6 +198,11 @@ const (
// currentConfigPath is where we store the current config on disk to validate
// against annotations changes
currentConfigPath = "/etc/machine-config-daemon/currentconfig"
// postOSUpdateEtcFilesMarkerPath is written before a reboot that follows an
// OS update with MC-managed /etc files. On the next first-run it signals that
// those files must be re-applied, because the ostree 3-way merge may have
// overwritten them when their content matched the old OS base.
postOSUpdateEtcFilesMarkerPath = "/etc/machine-config-daemon/post-os-update-etc-files"
// bootstrapConfigDiffPath is where we store the current config on disk to validate
// against annotations changes
bootstrapConfigDiffPath = "/etc/machine-config-daemon/bootstrapconfigdiff"
Expand Down Expand Up @@ -2190,6 +2195,36 @@ func (dn *Daemon) checkStateOnFirstRun() error {

}

// If a previous OS update left a marker, re-apply MC-managed /etc files
// before validation. The ostree 3-way merge silently overwrites MC-managed
// files whose content matches the old OS base, causing a spurious content
// mismatch. Re-applying here, after the merge has run, restores the correct
// content so validation passes.
if _, err := os.Stat(postOSUpdateEtcFilesMarkerPath); err != nil && !os.IsNotExist(err) {
return fmt.Errorf("checking post-OS-update marker: %w", err)
} else if err == nil {
klog.Infof("Post-OS-update marker found; re-applying MC /etc files to correct 3-way merge overwrites")
Comment thread
coderabbitai[bot] marked this conversation as resolved.
if err := os.Remove(postOSUpdateEtcFilesMarkerPath); err != nil {

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.

In case of an error with the re-write below, we removed this marker already and gets into a bad state. I guess if something fails we're probably in a bad state anyways, but probably better to move this marker deletion after the write? Unless you're explicitly wanting the MCD not to retry this?

Also, I think the function is runs in checkStateOnFirstRun runs every restart of the MCD. I guess it's conceivable that the MCD somehow restarts after writing the marker, we delete this here, and a reboot happens. In practice though I don't think we really should ever get into that state? (Also, this means we're potentially re-writing it more than once, but I guess not really a problem?)

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

In case of an error with the re-write below, we removed this marker already and gets into a bad state. I guess if something fails we're probably in a bad state anyways, but probably better to move this marker deletion after the write? Unless you're explicitly wanting the MCD not to retry this?

Code rabbit pointed out this concern also; here is my response to the 🐰 #6409 (comment). In my mind, if we fail this then the customer gets a degrade from the content mismatch and can resolve it through known work arounds. I'm happy to retry if we want, but I kind of figured if it fails, we might want to give humans the notice and option to investigate and fix themselves.

Also, I think the function is runs in checkStateOnFirstRun runs every restart of the MCD. I guess it's conceivable that the MCD somehow restarts after writing the marker, we delete this here, and a reboot happens. In practice though I don't think we really should ever get into that state? (Also, this means we're potentially re-writing it more than once, but I guess not really a problem?)

I'm not sure about if or when we might get into this state, as I never considered it. However, if we re-write more than once, I don't see that as being a big issue since we would be writing the desired content.

In general, I could see this not being the best solution as we discussed in Slack. However, it seems like the cleanest and safest approach for now. Long term, I think an rpm-ostree fix is the best approach, but since the issue is surfacing through supported MCO operations, I think the customer may not love that answer. 🙂

klog.Errorf("Failed to remove post-OS-update marker: %v", err)
}
newIgnConfig, err := ctrlcommon.ParseAndConvertConfig(state.currentConfig.Spec.Config.Raw)
if err != nil {
return fmt.Errorf("parsing current config for /etc re-apply after OS update: %w", err)
}
Comment thread
coderabbitai[bot] marked this conversation as resolved.
var etcFiles []ign3types.File
for _, f := range newIgnConfig.Storage.Files {
if strings.HasPrefix(f.Path, "/etc/") {
etcFiles = append(etcFiles, f)
}
}
if len(etcFiles) > 0 {
klog.Infof("Re-applying %d MC /etc files after OS update", len(etcFiles))
if err := dn.writeFiles(etcFiles, false); err != nil {
return fmt.Errorf("re-applying /etc files after OS update: %w", err)
}
}
}

if err := dn.validateOnDiskStateOrImage(state.currentConfig, state.currentImage); err != nil {
dn.nodeWriter.Eventf(corev1.EventTypeWarning, "OnDiskStateValidationFailed", "%s", err.Error())
// Start the config drift monitor even when there's pre-existing drift
Expand Down
17 changes: 17 additions & 0 deletions pkg/daemon/update.go
Original file line number Diff line number Diff line change
Expand Up @@ -1302,6 +1302,23 @@ func (dn *Daemon) update(oldConfig, newConfig *mcfgv1.MachineConfig, skipCertifi
}
}
}()
var etcFiles []ign3types.File
for _, f := range newIgnConfig.Storage.Files {
if strings.HasPrefix(f.Path, "/etc/") {
etcFiles = append(etcFiles, f)
}
}
if osUpdate && len(etcFiles) > 0 {
// Signal checkStateOnFirstRun to re-apply these files after the reboot.
// The ostree 3-way /etc merge runs at boot time and can overwrite
// MC-managed files whose content matches the old OS base; writing to
// the staged deployment before the reboot would not survive the merge.
// The marker causes a post-reboot re-apply that corrects any overwrites.
if err := writeFileAtomicallyWithDefaults(postOSUpdateEtcFilesMarkerPath, []byte{}); err != nil {
return fmt.Errorf("writing post-OS-update marker: %w", err)
}
klog.Infof("Wrote post-OS-update marker; %d managed /etc files will be re-applied on next boot", len(etcFiles))
}
} else {
klog.Info("updating the OS on non-CoreOS nodes is not supported")
}
Expand Down