Bug 1814397: pkg/daemon: fix deletion of stale files #1586
Conversation
openshift-ci-robot
added
the
approved
|
@cgwalters ptal 🙏 |
We have a serious bug in how we backup "original" files and restore them. Here, "original" means files that ship with RHCOS. Think of a default Chrony or another system daemon configuration file. When the MCD kicks in and writes to those files, we want to backup the original one (the shipped-with-RHCOS) in order to restore it if a user deletes the MC that modified it (this was the initial bug reported in GitHub at openshift#782). However, that patch that fixed openshift#782 was causing the following; if you shipped a file with just _one_ MC, removing it would wipe it out and that works. However, if you modified that file later again with another MC, a backup file will be created for the first MC, and when deleting the file by deleting the second MC, it will restore the initial file shipped with the first MC instead of wiping it out completely which it should have since that file was never meant to be backed up because it wasn't on RHCOS from the beginning. This patch now differentiates between files that are already on RHCOS (on-disk so to speak) and files that are shipped with an MC. For the former, the MCD will create a backup as it's doing today, for the latter instead, the MCD creates a placeholder file that tells it to just get rid of the file altogether (along with adding all the necessary checks and actions in order to create those backup files). The issue popped up on upgrade paths where the new manifests rendered by the MCO don't contain a certain file. The MCD notices that and go ahead trying to remove the file. It however notices that a backup file (which was created for an MC shipped file and later other MC have modified it) is there and tries to restore it (also failing with invalid cross-link device error, but that's another issue which I'm fixing here as well by using cp directly). Really hoping all the above makes sense. Signed-off-by: Antonio Murdaca <runcom@linux.com>
runcom
changed the title
openshift-ci-robot
added
the
bugzilla/valid-bug
|
@runcom: This pull request references Bugzilla bug 1814397, which is valid. The bug has been moved to the POST state. The bug has been updated to refer to the pull request using the external bug tracker. 3 validation(s) were run on this bug
In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
|
@runcom: This pull request references Bugzilla bug 1814397, which is valid. 3 validation(s) were run on this bug
In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
|
/retest |
There was a problem hiding this comment.
However, if you modified that file later again with another MC, a backup
file will be created for the first MC
I don't understand this. Why do we backup the file if it's not actually newly modified? Isn't that the root problem here?
When deleting the second MC, shouldn't it notice that the desired file contents have a new target and reconcile it back to the first MC?
| // create a noorig file that tells the MCD that the file wasn't present on disk in RHCOS | ||
| // so it can just remove it when deleting stale data, as opposed as restoring a file | ||
| // that was shipped _with_ RHCOS (e.g. a default chrony config). |
There was a problem hiding this comment.
This is somewhat of a corner case, but: does this correctly handle an upgrade whose base OSTree introduces a config file which we previously had modified? We'd write the noorig file, but then if it becomes stale would just nuke it entirely instead of restoring the base one, right?
This would be a lot easier if we used the /usr/etc as the canonical "host" defaults, but yeah I understand we need to support traditional systems too and it's easier to have the same path for both.
There was a problem hiding this comment.
Thinking more on this: unless I'm misunderstanding, I'm not sure if using noorig is the right approach since it's signaling something that can change. One could imagine the opposite scenario as well: a file we previously shipped in the host and modified with an MC is now dropped out of the host; once you nuke the MC, do we restore the original file instead of deleting it?
There was a problem hiding this comment.
does this correctly handle an upgrade whose base OSTree introduces a config file which we previously had modified?
Hm, now I think about it, if we are rebooting into a new oscontainer with a new version of a file, how are we consolidating that today?
And yeah this would be another ugly scenario. Correct me if I'm wrong but I think this would only happen if we somehow created a new MC and the OSTree to write the same file right?
| } | ||
| glog.V(2).Infof("Removing file %q completely", f.Path) | ||
| } else if _, err := os.Stat(origFileName(f.Path)); err == nil { | ||
| if out, err := exec.Command("cp", "-a", "--reflink=auto", origFileName(f.Path), f.Path).CombinedOutput(); err != nil { |
There was a problem hiding this comment.
Minor: I see the exec.Command("cp", ...) is used elsewhere too. Could probably use a factoring out to make it cleaner to call.
There was a problem hiding this comment.
Right now if we want to fix existing clusters we need to instead iterate over all versions of non-renderd MCs to see whether the file at any point was written, and if not, wipe from disk. This is ugly but this proposed solution won't fix existing clusters.
As a side note do you know why we don't back up systemd units
| return nil | ||
| } | ||
| if _, err := os.Stat(fpath); err != nil { |
There was a problem hiding this comment.
So while this would work with newly created clusters, existing clusters would still run into this problem after we upgrade to this version, since we already lost the fact that the file originally didn't exist
There was a problem hiding this comment.
This will be covered by patching release-1 (in this case 4.3) - see #1587 <- this patch takes care of a best effort to understand which file was on the host/shipped by an rpm like chrony and the like
No that's not the problem. This issue only occurs when
This backup originally is meant to fix scenarios like:
|
there's a BZ we need to fix to fix that - it's tracked for now and you're right, we should be doing that for systemd units (and dropins as well) |
That's what I mean though. If we fix just that, then the current orig approach still works fine (notwithstanding the issues mentioned here) and this whole issue goes away, right? |
that's what this does right? it makes sure to create the backup file only if the file isn't already there |
|
@runcom: The following tests failed, say
Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here. |
|
It's worth looking at how ostree handles the base problem here. The way it works is we always checkout the new defaults, then calculate a diff between current defaults and current, and apply that diff to the new. It's a lot like The most important thing here is we're not explicitly trying to delete files that no longer apply - we're always starting from the new base. It's also how ostree handles the base Whereas this is trying to mutate the current state directly into a new state - a lot like how On a related topic, this "mutate the current" is also extremely hard to do transactionally - neither I think the long term what we want to do is capture what I'll call the "local diff" - any changes made on the node that are not part of the MCO. Then on upgrades, we:
Then in no case do we need to even care about a diff versus the old MCO state, dealing with But this is harder to do while we need to care about !CoreOS. |
|
It should be really pretty easy to put together a set of unit tests for this right? Or perhaps better, a more "blackbox" test that runs in a pod: Basically |
There was a problem hiding this comment.
Had a chat with @runcom about this, and I think we're on the same page. I had trouble trying to formulate what bothered me, but I think I can pinpoint it now: creating an .mcdnoorig isn't actually necessary; the condition we care about (whether we're doing a modification on top of another MC) can be calculated by just analyzing the generated MCs since we have both the old and new generated configs at hand.
And given that .mcdnoorig files don't actually track the right thing (as per discussions in #1586 (comment)), it wasn't clear to me if we were doing more harm than good. I think I've convinced myself though that the amount of harm is about the same. :)
Anyway, I think this approach works too as a stopgap and I appreciate that it's easier to achieve than doing another diff as described above.
| return nil | ||
| } | ||
| if _, err := os.Stat(fpath); err != nil { |
There was a problem hiding this comment.
Should check specifically for ENOENT here: https://golang.org/pkg/os/#IsNotExist
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: jlebon, runcom The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
|
Did we close this PR by mistake 🤔 |
|
This has been superceded by #1593 |
We have a serious bug in how we backup "original" files and restore them.
Here, "original" means files that ship with RHCOS. Think of a default Chrony
or another system daemon configuration file. When the MCD kicks in and writes
to those files, we want to backup the original one (the shipped-with-RHCOS) in order
to restore it if a user deletes the MC that modified it (this was the initial bug reported
in GitHub at #782).
However, that patch that fixed #782
was causing the following; if you shipped a file with just one MC, removing it would wipe it out and that works.
However, if you modified that file later again with another MC, a backup
file will be created for the first MC, and when deleting the file by deleting the second MC, it will
restore the initial file shipped with the first MC instead of wiping it out completely which
it should have since that file was never meant to be backed up because it wasn't on RHCOS from the beginning.
This patch now differentiates between files that are already on RHCOS (on-disk so to speak)
and files that are shipped with an MC. For the former, the MCD will create a backup as it's doing today,
for the latter instead, the MCD creates a placeholder file that tells it to just get rid
of the file altogether (along with adding all the necessary checks and actions in order to create those backup files).
The issue popped up on upgrade paths where the new manifests rendered by the MCO don't contain a certain file.
The MCD notices that and go ahead trying to remove the file. It however notices that a backup file
(which was created for an MC shipped file and later other MC have modified it) is there and tries to restore it (also failing
with invalid cross-link device error, but that's another issue which I'm fixing here as well by using cp directly).
Really hoping all the above makes sense. Otherwise, an e2e can be consulted here #1590
Signed-off-by: Antonio Murdaca runcom@linux.com