Repository navigation
OCPBUGS-105508: Re-apply MC /etc files after OS update to fix ostree 3-way merge overwrites #6409
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -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" | ||
|
|
@@ -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") | ||
| if err := os.Remove(postOSUpdateEtcFilesMarkerPath); err != nil { | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
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.
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) | ||
| } | ||
|
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 | ||
|
|
||
Uh oh!
There was an error while loading. Please reload this page.