Skip to content

Conversation

@openshift-cherrypick-robot

This is an automated cherry-pick of #1593

/assign runcom

runcom and others added 3 commits April 2, 2020 06:42
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>
@openshift-ci-robot openshift-ci-robot added the bugzilla/invalid-bug Indicates that a referenced Bugzilla bug is invalid for the branch this PR is targeting. label Apr 2, 2020
@openshift-ci-robot
Copy link
Contributor

@openshift-cherrypick-robot: This pull request references Bugzilla bug 1814397, which is invalid:

  • expected the bug to target the "4.4.0" release, but it targets "4.5.0" instead
  • expected the bug to be in one of the following states: NEW, ASSIGNED, ON_DEV, POST, POST, but it is ON_QA instead
  • expected Bugzilla bug 1814397 to depend on a bug targeting the "4.5.0" release and in one of the following states: VERIFIED, RELEASE_PENDING, CLOSED (ERRATA), but no dependents were found

Comment /bugzilla refresh to re-evaluate validity if changes to the Bugzilla bug are made, or edit the title of this pull request to link to a different bug.

Details

In response to this:

[release-4.4] Bug 1814397: fix wrongful backup of files not originally on the system

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 runcom changed the title [release-4.4] Bug 1814397: fix wrongful backup of files not originally on the system Bug 1817455: [release-4.4] fix wrongful backup of files not originally on the system Apr 2, 2020
@openshift-ci-robot
Copy link
Contributor

@openshift-cherrypick-robot: This pull request references Bugzilla bug 1817455, which is invalid:

  • expected dependent Bugzilla bug 1814397 to be in one of the following states: VERIFIED, RELEASE_PENDING, CLOSED (ERRATA), but it is ON_QA instead

Comment /bugzilla refresh to re-evaluate validity if changes to the Bugzilla bug are made, or edit the title of this pull request to link to a different bug.

Details

In response to this:

Bug 1817455: [release-4.4] fix wrongful backup of files not originally on the system

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
Copy link
Contributor

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: openshift-cherrypick-robot
To complete the pull request process, please assign ericavonb
You can assign the PR to them by writing /assign @ericavonb in a comment when ready.

The full list of commands accepted by this bot can be found here.

Details Needs approval from an approver in each of these files:

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

@runcom
Copy link
Member

runcom commented Apr 2, 2020

/retest

2 similar comments
@sinnykumari
Copy link
Contributor

/retest

@runcom
Copy link
Member

runcom commented Apr 2, 2020

/retest

@openshift-ci-robot
Copy link
Contributor

@openshift-cherrypick-robot: The following tests failed, say /retest to rerun all failed tests:

Test name Commit Details Rerun command
ci/prow/e2e-aws-scaleup-rhel7 b75e5ba link /test e2e-aws-scaleup-rhel7
ci/prow/e2e-gcp-upgrade b75e5ba link /test e2e-gcp-upgrade
ci/prow/e2e-aws b75e5ba link /test e2e-aws
ci/prow/images b75e5ba link /test images
ci/prow/e2e-gcp-op b75e5ba link /test e2e-gcp-op
ci/prow/unit b75e5ba link /test unit

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.

Details

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.

@runcom
Copy link
Member

runcom commented Apr 2, 2020

ok, the CI is failing because we're using the FCOS constant which isn't in 4.4 and I don't believe that's a needed cherry-pick so I've created #1609 - please review and merge that

@runcom runcom closed this Apr 2, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bugzilla/invalid-bug Indicates that a referenced Bugzilla bug is invalid for the branch this PR is targeting.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants