pkg/daemon: do not delete host files when deleting an MC #797
Conversation
openshift-ci-robot
added
size/L
|
Thinking of any way this could break upgrades/backwards compatibily now lol |
openshift-ci-robot
added
the
needs-rebase
|
@runcom Are you going to get back to this PR? Pretty important bug I think, also I realized in later discussion that this would help us support the Ignition |
yep, been dragged and buried into something else (mainly bugzillas and next features), I'll get back on this shortly. |
openshift-ci-robot
removed
the
needs-rebase
|
Updated and ready for another round of reviews now |
Deploying an MC changing a file and later deleting it causes the file to be deleted (it could have been already on disk i.e. shipped by an RPM). Following Colin's suggestion, this patch adds a backup mechanism which, when deleting an MC, causes the old file to be restored. Added an e2e test as well. Signed-off-by: Antonio Murdaca <runcom@linux.com>
|
Image build failing: https://openshift-gce-devel.appspot.com/build/origin-ci-test/pr-logs/pull/openshift_machine-config-operator/797/pull-ci-openshift-machine-config-operator-master-images/2698 |
yup, error on my side :P |
|
A brief search turns up https://github.com/docker/docker-ce/blob/9f7430572607260b058cc69c03da91278beac746/components/engine/daemon/graphdriver/copy/copy.go#L30 which looks reasonable if we ever want to copy some code to do this in-process. Rust also recently gained a good implementation of this FWIW. |
I'm familiar with that piece of code, also willing to use that, I'm just worried about the maintenance status of anything within Docker/Moby. |
| // the orig file is already there and we avoid creating a new one to preserve the real default | ||
| return nil | ||
| } | ||
| if out, err := exec.Command("cp", "-a", "--reflink=auto", fpath, origFileName(fpath)).CombinedOutput(); err != nil { |
There was a problem hiding this comment.
Of course if this is interrupted in the middle we'll end up with an incomplete backup but...let's just get this in!
There was a problem hiding this comment.
The libglnx code I wrote for this properly clones to an O_TMPFILE which also makes it superior to what the Rust libstd is doing. It's a bit surprising actually cp doesn't have a --tmpfile flag or something, would probably be easy to implement.
If only calling C from golang wasn't unspeakably awful...
Anyways we can circle back to polishing this bit later.
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: cgwalters, 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 |
6 participants
Deploying an MC changing a file and later deleting it causes the file to
be deleted (it could have been already on disk i.e. shipped by an RPM).
Following Colin's suggestion, this patch adds a backup mechanism which,
when deleting an MC, causes the old file to be restored.
Added an e2e test as well.
closes #782
Signed-off-by: Antonio Murdaca runcom@linux.com