Antoni Segura Puimedon has uploaded a new change for review.
Change subject: Added utility to ensure that files written to the file system happen atomically. ......................................................................
Added utility to ensure that files written to the file system happen atomically.
The only way in python to make a file be written in an atomic way is to take advantage of the fact that in POSIX filesystems the rename operation is atomic. Thus, we write the context to a temporary file on the same filesystem, sync on the file descriptor and finally do the atomic file rename to the original file we wanted to write.
For more info: http://stackoverflow.com/questions/2333872/atomic-writing-to-file-with-pytho... http://stackoverflow.com/questions/7433057/is-rename-without-fsync-safe
Change-Id: Ibecd61d6746231a5a8cb17bad9a3302b01454f27 Signed-off-by: Antoni S. Puimedon asegurap@redhat.com --- M vdsm/configNetwork.py M vdsm/utils.py 2 files changed, 17 insertions(+), 1 deletion(-)
git pull ssh://gerrit.ovirt.org:29418/vdsm refs/changes/56/7656/1
diff --git a/vdsm/configNetwork.py b/vdsm/configNetwork.py index 58117f8..b943574 100755 --- a/vdsm/configNetwork.py +++ b/vdsm/configNetwork.py @@ -369,7 +369,8 @@ '''Backs up the previous contents of the file referenced by fileName writes the new configuration and sets the specified access mode.''' self._backup(fileName) - open(fileName, 'w').write(configuration) + with utils.atomicWrite(fileName) as f: + f.write(configuration) os.chmod(fileName, 0664) try: selinux.restorecon(fileName) diff --git a/vdsm/utils.py b/vdsm/utils.py index 5e2d4e5..bb8a842 100644 --- a/vdsm/utils.py +++ b/vdsm/utils.py @@ -36,6 +36,7 @@ import fcntl import functools import stat +from contextlib import contextmanager
import ethtool
@@ -829,3 +830,17 @@
def __unicode__(self): return unicode(self.cmd) + +@contextmanager +def atomicWrite(file): + '''Context manager that makes the write happen to a temporary file and if + and only it is successful, overwrites the original file. It creates the + temporary file in the same directory of the destination file, as rename is + only atomic within the same filesystem.''' + tempFile = file+'vdsm_temp' + f = open(tempFile, 'w') + yield f + f.flush() + os.fsync(f.fileno()) + f.close() + os.rename(tempFile, file)
-- To view, visit http://gerrit.ovirt.org/7656 To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: newchange Gerrit-Change-Id: Ibecd61d6746231a5a8cb17bad9a3302b01454f27 Gerrit-PatchSet: 1 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Antoni Segura Puimedon asegurap@redhat.com
Antoni Segura Puimedon has posted comments on this change.
Change subject: Added utility to ensure that files written to the file system happen atomically. ......................................................................
Patch Set 1:
I guess it would make more sense to make separate commits for adding the context manager and other commits for adding use of it in several places.
-- To view, visit http://gerrit.ovirt.org/7656 To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment Gerrit-Change-Id: Ibecd61d6746231a5a8cb17bad9a3302b01454f27 Gerrit-PatchSet: 1 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Antoni Segura Puimedon asegurap@redhat.com Gerrit-Reviewer: Antoni Segura Puimedon asegurap@redhat.com
oVirt Jenkins CI Server has posted comments on this change.
Change subject: Added utility to ensure that files written to the file system happen atomically. ......................................................................
Patch Set 1:
Build Successful
http://jenkins.ovirt.info/job/patch_vdsm_unit_tests/795/ : SUCCESS
-- To view, visit http://gerrit.ovirt.org/7656 To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment Gerrit-Change-Id: Ibecd61d6746231a5a8cb17bad9a3302b01454f27 Gerrit-PatchSet: 1 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Antoni Segura Puimedon asegurap@redhat.com Gerrit-Reviewer: Antoni Segura Puimedon asegurap@redhat.com Gerrit-Reviewer: Dan Kenigsberg danken@redhat.com Gerrit-Reviewer: Igor Lvovsky ilvovsky@redhat.com Gerrit-Reviewer: oVirt Jenkins CI Server
Dan Kenigsberg has posted comments on this change.
Change subject: Added utility to ensure that files written to the file system happen atomically. ......................................................................
Patch Set 1: I would prefer that you didn't submit this
(3 inline comments)
the configNet use case is a bit complex due to ovirt-node's use of bind-mounted files; and we do backups already. So I'm not quick with taking this patch - let's be sure we've taken care of all scenarios.
.................................................... File vdsm/utils.py Line 831: def __unicode__(self): Line 832: return unicode(self.cmd) Line 833: Line 834: @contextmanager Line 835: def atomicWrite(file): "file" is a built-in class. let's choose another name. Line 836: '''Context manager that makes the write happen to a temporary file and if Line 837: and only it is successful, overwrites the original file. It creates the Line 838: temporary file in the same directory of the destination file, as rename is Line 839: only atomic within the same filesystem.'''
Line 836: '''Context manager that makes the write happen to a temporary file and if Line 837: and only it is successful, overwrites the original file. It creates the Line 838: temporary file in the same directory of the destination file, as rename is Line 839: only atomic within the same filesystem.''' Line 840: tempFile = file+'vdsm_temp' this means that two threads cannot take "atomicWrite" concurrently. better add randomness to the name. Line 841: f = open(tempFile, 'w') Line 842: yield f Line 843: f.flush() Line 844: os.fsync(f.fileno())
Line 842: yield f Line 843: f.flush() Line 844: os.fsync(f.fileno()) Line 845: f.close() Line 846: os.rename(tempFile, file) I think there were issues with renaming of bind-mounted files on ovirt-node.
-- To view, visit http://gerrit.ovirt.org/7656 To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment Gerrit-Change-Id: Ibecd61d6746231a5a8cb17bad9a3302b01454f27 Gerrit-PatchSet: 1 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Antoni Segura Puimedon asegurap@redhat.com Gerrit-Reviewer: Antoni Segura Puimedon asegurap@redhat.com Gerrit-Reviewer: Dan Kenigsberg danken@redhat.com Gerrit-Reviewer: Igor Lvovsky ilvovsky@redhat.com Gerrit-Reviewer: oVirt Jenkins CI Server
Antoni Segura Puimedon has posted comments on this change.
Change subject: Added utility to ensure that files written to the file system happen atomically. ......................................................................
Patch Set 1: (3 inline comments)
I agree that there is no hurry for this. I just thought that it is a good way to increase the robustness when writing configuration files that will maybe avoid us some rollback in crash scenarios.
.................................................... File vdsm/utils.py Line 831: def __unicode__(self): Line 832: return unicode(self.cmd) Line 833: Line 834: @contextmanager Line 835: def atomicWrite(file): Agreed. How about 'targetFile'? Line 836: '''Context manager that makes the write happen to a temporary file and if Line 837: and only it is successful, overwrites the original file. It creates the Line 838: temporary file in the same directory of the destination file, as rename is Line 839: only atomic within the same filesystem.'''
Line 836: '''Context manager that makes the write happen to a temporary file and if Line 837: and only it is successful, overwrites the original file. It creates the Line 838: temporary file in the same directory of the destination file, as rename is Line 839: only atomic within the same filesystem.''' Line 840: tempFile = file+'vdsm_temp' Done Line 841: f = open(tempFile, 'w') Line 842: yield f Line 843: f.flush() Line 844: os.fsync(f.fileno())
Line 842: yield f Line 843: f.flush() Line 844: os.fsync(f.fileno()) Line 845: f.close() Line 846: os.rename(tempFile, file) When I finish with the bug I'll setup an oVirt node to test it. By bind-mounted do you refer to mount -o bind?
-- To view, visit http://gerrit.ovirt.org/7656 To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment Gerrit-Change-Id: Ibecd61d6746231a5a8cb17bad9a3302b01454f27 Gerrit-PatchSet: 1 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Antoni Segura Puimedon asegurap@redhat.com Gerrit-Reviewer: Antoni Segura Puimedon asegurap@redhat.com Gerrit-Reviewer: Dan Kenigsberg danken@redhat.com Gerrit-Reviewer: Igor Lvovsky ilvovsky@redhat.com Gerrit-Reviewer: oVirt Jenkins CI Server
Antoni Segura Puimedon has posted comments on this change.
Change subject: Added utility to ensure that files written to the file system happen atomically. ......................................................................
Patch Set 2:
There is still the pending issue of the renaming issue in bind mounted file. I'll work on that when time allows.
-- To view, visit http://gerrit.ovirt.org/7656 To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment Gerrit-Change-Id: Ibecd61d6746231a5a8cb17bad9a3302b01454f27 Gerrit-PatchSet: 2 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Antoni Segura Puimedon asegurap@redhat.com Gerrit-Reviewer: Antoni Segura Puimedon asegurap@redhat.com Gerrit-Reviewer: Dan Kenigsberg danken@redhat.com Gerrit-Reviewer: Igor Lvovsky ilvovsky@redhat.com Gerrit-Reviewer: Livnat Peer lpeer@redhat.com Gerrit-Reviewer: oVirt Jenkins CI Server
Dan Kenigsberg has posted comments on this change.
Change subject: Added utility to ensure that files written to the file system happen atomically. ......................................................................
Patch Set 2: (1 inline comment)
.................................................... File vdsm/utils.py Line 860: '''Context manager that makes the write happen to a temporary file and if Line 861: and only it is successful, overwrites the original file. It creates the Line 862: temporary file in the same directory of the destination file, as rename is Line 863: only atomic within the same filesystem.''' Line 864: tempFile = targetFile+'_vdsm_'+randomStr(6) nitpick, before I forget: new code should bet pep8 ready. surround "+" with spaces. Line 865: f = open(tempFile, 'w') Line 866: yield f Line 867: f.flush() Line 868: os.fsync(f.fileno())
-- To view, visit http://gerrit.ovirt.org/7656 To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment Gerrit-Change-Id: Ibecd61d6746231a5a8cb17bad9a3302b01454f27 Gerrit-PatchSet: 2 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Antoni Segura Puimedon asegurap@redhat.com Gerrit-Reviewer: Antoni Segura Puimedon asegurap@redhat.com Gerrit-Reviewer: Dan Kenigsberg danken@redhat.com Gerrit-Reviewer: Igor Lvovsky ilvovsky@redhat.com Gerrit-Reviewer: Livnat Peer lpeer@redhat.com Gerrit-Reviewer: oVirt Jenkins CI Server
Giuseppe Vallarelli has posted comments on this change.
Change subject: Added utility to ensure that files written to the file system happen atomically. ......................................................................
Patch Set 4: I would prefer that you didn't submit this
(1 inline comment)
See comment.
.................................................... File vdsm/utils.py Line 37: import functools Line 38: import stat Line 39: import glob Line 40: from contextlib import contextmanager Line 41: from storage.misc import randomStr You should not depend on storage module. Line 42: Line 43: import ethtool Line 44: Line 45: import constants
-- To view, visit http://gerrit.ovirt.org/7656 To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment Gerrit-Change-Id: Ibecd61d6746231a5a8cb17bad9a3302b01454f27 Gerrit-PatchSet: 4 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Antoni Segura Puimedon asegurap@redhat.com Gerrit-Reviewer: Antoni Segura Puimedon asegurap@redhat.com Gerrit-Reviewer: Dan Kenigsberg danken@redhat.com Gerrit-Reviewer: Giuseppe Vallarelli gvallare@redhat.com Gerrit-Reviewer: Livnat Peer lpeer@redhat.com Gerrit-Reviewer: oVirt Jenkins CI Server
Giuseppe Vallarelli has posted comments on this change.
Change subject: Added utility to ensure that files written to the file system happen atomically. ......................................................................
Patch Set 4: (1 inline comment)
.................................................... File vdsm/utils.py Line 855: def __unicode__(self): Line 856: return unicode(self.cmd) Line 857: Line 858: @contextmanager Line 859: def atomicWrite(targetFile): You're decorating the open builtin behavior, you should use the same interface of open, by passing filename and mode, what do you think? Line 860: '''Context manager that makes the write happen to a temporary file and if Line 861: and only it is successful, overwrites the original file. It creates the Line 862: temporary file in the same directory of the destination file, as rename is Line 863: only atomic within the same filesystem.'''
-- To view, visit http://gerrit.ovirt.org/7656 To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment Gerrit-Change-Id: Ibecd61d6746231a5a8cb17bad9a3302b01454f27 Gerrit-PatchSet: 4 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Antoni Segura Puimedon asegurap@redhat.com Gerrit-Reviewer: Antoni Segura Puimedon asegurap@redhat.com Gerrit-Reviewer: Dan Kenigsberg danken@redhat.com Gerrit-Reviewer: Giuseppe Vallarelli gvallare@redhat.com Gerrit-Reviewer: Livnat Peer lpeer@redhat.com Gerrit-Reviewer: oVirt Jenkins CI Server
Antoni Segura Puimedon has posted comments on this change.
Change subject: Added utility to ensure that files written to the file system happen atomically. ......................................................................
Patch Set 4: (1 inline comment)
.................................................... File vdsm/utils.py Line 855: def __unicode__(self): Line 856: return unicode(self.cmd) Line 857: Line 858: @contextmanager Line 859: def atomicWrite(targetFile): I didn't do that cause it is only for writing. I guess I could allow write and write binary modes. Line 860: '''Context manager that makes the write happen to a temporary file and if Line 861: and only it is successful, overwrites the original file. It creates the Line 862: temporary file in the same directory of the destination file, as rename is Line 863: only atomic within the same filesystem.'''
Antoni Segura Puimedon has abandoned this change.
Change subject: Added utility to ensure that files written to the file system happen atomically. ......................................................................
Abandoned
vdsm-patches@lists.fedorahosted.org