Bug 1817455: [release-4.4] fix wrongful backup of files not originally on the system #1609
Conversation
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>
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. In this case we delete the `orig` file and don't back up. Also change the name to noOrigFileStamp for clarity. Signed-off-by: Yu Qi Zhang <jerzhang@redhat.com>
Signed-off-by: Antonio Murdaca <runcom@linux.com>
openshift-ci-robot
added
the
bugzilla/invalid-bug
|
@runcom: This pull request references Bugzilla bug 1817455, which is invalid:
Comment DetailsIn 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. |
openshift-ci-robot
added
the
approved
|
/retest |
|
/retest |
|
/skip |
|
/bugzilla refresh |
openshift-ci-robot
added
bugzilla/valid-bug
|
@sinnykumari: This pull request references Bugzilla bug 1817455, which is valid. The bug has been updated to refer to the pull request using the external bug tracker. 6 validation(s) were run on this bug
DetailsIn 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. |
|
/lgtm |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: ashcrow, runcom, sinnykumari The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
|
/bugzilla refresh |
|
@runcom: This pull request references Bugzilla bug 1817455, which is valid. 6 validation(s) were run on this bug
DetailsIn 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. |
ashcrow
added
the
cherry-pick-approved
|
/retest Please review the full test history for this PR and help us cut down flakes. |
|
@runcom: The following test 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. DetailsInstructions 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. |
|
/retest Please review the full test history for this PR and help us cut down flakes. |
|
@runcom: Some pull requests linked via external trackers have merged: openshift/machine-config-operator#1609. The following pull requests linked via external trackers have not merged:
DetailsIn 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. |
|
/bugzilla refresh |
|
@runcom: All pull requests linked via external trackers have merged: openshift/machine-config-operator#1609. Bugzilla bug 1817455 has been moved to the MODIFIED state. DetailsIn 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. |
7 participants
cherrypick of #1593