Bug 1814397: fix wrongful backup of files not originally on the system #1593

yuqi-zhang · 2020-03-26T16:23:41Z

Adds to #1586

Add a check for backwards compatibility: basically if the file
doesn't exist in /usr/etc/ and no rpm is claming it, we assume
that the orig file came from a wrongful backup of a MachineConfig
file instead of a RHCOS file.

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>

pkg/daemon/update.go

runcom · 2020-03-27T12:56:35Z

manually upgraded from 4.2 to 4.4 (with this patch) and it worked just fine, I'm gonna test all the way from 4.1 to fully verify this, otherwise, this replaces #1586

/approve

openshift-ci-robot · 2020-03-27T12:57:28Z

@yuqi-zhang: This pull request references Bugzilla bug 1814397, which is valid. The bug has been updated to refer to the pull request using the external bug tracker.

3 validation(s) were run on this bug

bug is open, matching expected state (open)
bug target release (4.5.0) matches configured target release for branch (4.5.0)
bug is in the state POST, which is one of the valid states (NEW, ASSIGNED, ON_DEV, POST, POST)

In response to this:

Bug 1814397: update.go: add extra checks when restoring .orig files

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 · 2020-03-27T12:58:30Z

The additional patches here are also backported already to #1588 - we'd need a 4.3 patch and 4.2 patch anyway but those can wait till we unblock 4.4 as this patch effectively fixes the upgrade.

runcom · 2020-03-27T13:01:04Z

/retest

yuqi-zhang · 2020-03-27T13:36:03Z

Ah, thanks for the fix! Feel free to squash the commits together

pkg/daemon/update.go

yuqi-zhang · 2020-03-27T19:26:36Z

Pushed new commit:
Squashed fixes
Added comments and changed naming for clarity
Add new fix for files that exist in /etc

Manually tested to be good for both new and existing scenarios

pkg/daemon/update.go

yuqi-zhang · 2020-03-27T20:32:32Z

Modified error msg

Now I think about it technically I think the logic is wrong.

/hold

yuqi-zhang · 2020-03-27T20:41:21Z

/hold cancel

After some more thought I think this is alright. To be clear if we have an .orig file, the conditions it will restore is that it must both be a) managed by an RPM and b) exist in the original ostree (if it was in /etc).

Is there a scenario where we have an rpm drop in a file in /etc but it would not exist in /usr/etc? Conversely, is there a scenario where it exists in /usr/etc but no rpm is claiming it?

runcom · 2020-03-30T10:56:48Z

Is there a scenario where we have an rpm drop in a file in /etc but it would not exist in /usr/etc?

I think this is only possible if you install an rpm after initial bootstrap + MCO right?

Conversely, is there a scenario where it exists in /usr/etc but no rpm is claiming it?

I think yes, say you installed an RPM with a spec that doesn't claim ownership of a file (it happens a lot, trust me...) but still ships the file under /etc

runcom · 2020-03-30T10:57:25Z

/retest

yuqi-zhang · 2020-03-30T18:45:54Z

I can confirm this passes the tests to be added in #1590

I can also add a test to ensure existing .origs are properly pruned as a separate PR. For now I have done manual testing and can confirm this should fix those cases.

openshift-ci-robot · 2020-04-01T09:00:36Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: kikisdeliveryservice, runcom, sinnykumari, yuqi-zhang

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:

~~OWNERS~~ [kikisdeliveryservice,runcom,sinnykumari,yuqi-zhang]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

openshift-bot · 2020-04-01T09:30:23Z

/retest