Since part #1 of my three-part plan is proving troublesome I moved
ahead on #2.
Although part #3 of my original plan borrows some code from #1 it does
not require #1. If this patch is accepted then I will move ahead on #3.
This patch changes copyfile in two ways:
1. Copyfile now opens a tempfile on the minion and appends data to this
tempfile, then moves the tempfile over the target file after all of the data
has been appended.
The advantage of this is that updating the target file is atomic, or
as close to it as possible in python. This minimizes the chance of some
program accessing an empty or half-complete file. If the file in question
were a server config file... surely you can imagine how this could be a
problem.
The tempfiles are created in the same directory as the target file
(for security) and have the same name as the target file but with the
extension ".functmp". Permissions are set on this file during the "open"
stage, while it is still zero-size.
2. Copyfile has a new option called "retainperms" ("--retainperms" at the
command line). It is "False" by default.
Currently func tries to give the new file on the minion the same
attributes (permissions and uid/gid) as the file on the overlord. The SELinux
context of the new file is not set (and does not change). This overrides this
behavior to make the permissions, flags, uid, gid and SELinux contexts of the
new file identical to the existing file (if any) on the minion.
This is intended to deal with the situation in which the file on the
overlord has different permissions than the files on the minions and the user
does not want to overwrite them. Perhaps the overlord computer does not have
the same users and groups as the minions? I figured this might be useful for
someone.
On a related note, does anyone think it would be worthwhile to have
copyfile copy the SELinux contexts of the source file to the target? How
about making command-line options to allow the user to specify the desired
mode, uid, gid, contexts, etc.?
--
Marcus
On Saturday, August 25, 2012 04:55:09 PM Marcus Lauer wrote:
> Attached are my original patch and a git-patch which contains the
> same code. These implement changes I called #1 in my previous e-mail.
>
> #2 doesn't seem too hard. CopyFile.open will have to open a
> tempfile rather than the target file. It already returns the filepath on
> success. Append already uses the return value of CopyFile.open as the file
> to append to. After appending has finished there needs to be a
> CopyFile.move function called by the overlord which takes both paths and
> moves the tempfile over the source file. Making sure the
> ownership/permissions/SELinux attributes of the target file do not change
> could be is important so had better be careful about that.
>
> Once this is in place #3 is almost trivial. The overlord needs to
> calculate the MD5 of the source file. The code for doing this is also
> needed by #1 and is included in my patches. That MD5 should be passed to
> the CopyFile.move function. It should calculate the MD5 of the tempfile
> and if they don't match just delete the tempfile and return an error
> instead of moving it. I figure that this MD5 check should be optional so I
> will also create a flag to enable it.
>
> Can anyone see any issues with this general design? If not I'll get
> working on it when I have the time.
>
> --
> Marcus
>
> On Friday, August 24, 2012 07:32:14 PM you wrote:
> > Copyfile could most certainly use some TLC, so let me know if I can
> > help,
> > that would certainly be of help if you'd like to work on that. The list
> > has indeed changed to lists.fedorahosted.com, so definitely use that
> > one.
> >
> > Would you mind shooting me your original patch as well? Thanks very
> > much!>
> > On Fri, Aug 24, 2012 at 7:22 PM, Marcus Lauer
<marcuslauer@xxxxxxxxxx>wrote:
> > > It seems to me that there are at least three
> > > improvements which
> > >
> > > should be made to copyfile:
> > >
> > > 1. If the file on the overlord and minion are the same, that file
> > > should not
> > > be copied. This saves bandwidth/processing time.
> > >
> > > 2. Overwriting the file on the minion should be atomic, e.g. the
> > > file on the
> > > overlord should be copied to a tempfile on the minion then moved to
> > > overwrite
> > > the target file as a final step. Right now if funcd dies or
> > > something
> > > goes wrong mid-copy you end up with half a file on the minion.
> > >
> > > 3. There should be an option (off by default) such that if the file
> > > which
> > > has
> > > been copied over to the minion is not identical to the file on the
> > > overlord it
> > > is re-copied, or at least does not overwrite the file on the minion.
> > > This won't work for some files (busy logfiles) but for most files it
> > > seems like an
> > > extra bit of reliability which most people would want in most
> > > situations.
> > >
> > > I submitted a patch (though not a git-patch... my
> > > bad!)
> > > back in
> > >
> > > January 2011 which implements #1, but imperfectly. The overlord
> > > still
> > > has to
> > > send a (small) packet to the minion for each 60KB chunk of target
> > > file.
> > >
> > > Would anyone be bothered terribly if I worked on #2
> > > and
> > > #3? While
> > >
> > > I am
> > > at it I will try to find a way to fix up my patch for #1 as well.
> > > There is some overlap between these patches. Both will involve
> > > calculating an MD5 hash
> > > of the file on the overlord, for example.
> > >
> > > P.S. Is the new mailing list on fedorahosted.org
> > > live
> > > yet? I am
> > >
> > > sending this message to both just in case.
> > >
> > > --
> > > Marcus
> > > _______________________________________________
> > > func mailing list
> > > func@xxxxxxxxxxxxxxxxxxxxxx
> > > https://lists.fedorahosted.org/mailman/listinfo/func
> >
> > --
> > Steve Salevan
> > steve@xxxxxxxxxx>From 8af5bac2039459124b0084ab9fba445c3f56733e Mon Sep 17 00:00:00 2001
From: Marcus Lauer <marcuslauer@xxxxxxxxxx>
Date: Mon, 3 Sep 2012 13:38:25 -0400
Subject: [PATCH 1/4] Make copyfile write to a tempfile on the minion then
move the file at the end
---
func/minion/modules/copyfile.py | 85 +++++++++++++++++++++++++++++--------
func/overlord/modules/copyfile.py | 6 ++-
2 files changed, 71 insertions(+), 20 deletions(-)
diff --git a/func/minion/modules/copyfile.py b/func/minion/modules/copyfile.py
index 61c6a4f..8281747 100644
--- a/func/minion/modules/copyfile.py
+++ b/func/minion/modules/copyfile.py
@@ -26,12 +26,12 @@ import time
import shutil
import func_module
-
+from func import utils as func_utils
class CopyFile(func_module.FuncModule):
version = "0.0.1"
- api_version = "0.0.2"
+ api_version = "0.0.3"
description = "Allows for smart copying of a file."
def _checksum_blob(self, blob):
@@ -40,7 +40,6 @@ class CopyFile(func_module.FuncModule):
return thissum.hexdigest()
def checksum(self, thing):
-
CHUNK=2**16
thissum = hashlib.new('sha1')
if os.path.exists(thing):
@@ -62,7 +61,9 @@ class CopyFile(func_module.FuncModule):
if not os.path.exists(dirpath):
os.makedirs(dirpath)
- # Create empty file
+ # Create an empty tempfile in the target directory.
+ writetarget = filepath + '.functmp'
+
try:
fo = open(filepath, 'w')
fo.close()
@@ -71,33 +72,81 @@ class CopyFile(func_module.FuncModule):
# XXX logger output here
return -1
- try:
- # we could intify the mode here if it's a string
- if mode:
- os.chmod(filepath, mode)
- if uid != -1 or gid != -1:
- os.chown(filepath, uid, gid)
- except (IOError, OSError), e:
- return -1
+ """
+ Set the mode and ownership of the tempfile if requested.
+ If not set them to match the target file if the target
+ file exists.
+ """
+ if mode or uid != -1 or gid != -1:
+ try:
+ # we could intify the mode here if it's a string
+ if mode:
+ os.chmod(filepath, mode)
+ if uid != -1 or gid != -1:
+ os.chown(filepath, uid, gid)
+ except (IOError, OSError), e:
+ return -1
+ elif os.path.exists(filepath):
+ # Copy the mode of the existing file.
+ shutil.copystat(filepath,writetarget)
- return filepath
+ # Copy the uid/gid of the existing file.
+ fileinfo = os.stat(filepath)
+ os.chown(writetarget,fileinfo[4],fileinfo[5])
- def append(self, filepath, filebuf):
- if not os.path.exists(filepath):
- # file disaperead
+ # Copy the SELinux attribs of the existing file.
+ import subprocess
+ try:
+ subprocess.check_call(["chcon", "--reference=" + filepath, writetarget])
+ except (OSError), e:
+ # Do nothing. Presumably SELinux is not enabled on this system.
+
+ return writetarget
+
+ def append(self, open_result, filebuf):
+ hostname = func_utils.get_hostname_by_route()
+ writetarget = open_result[hostname]
+
+ # If the result of the open function was 0 (or -1), do not append, return the same value.
+ if type(writetarget) == type(0):
+ return writetarget
+
+ if not os.path.exists(writetarget):
+ # file disappeared
return -1
try:
- fo = open(filepath, 'a')
+ fo = open(writetarget, 'a')
fo.write(filebuf.data)
fo.close()
del fo
except (IOError, OSError), e:
- # XXX logger output here
+ # Clean up by removing the tempfile, then return an error.
+ os.unlink(writetarget)
return -1
return 1
+ def move(self, filepath, append_result)
+ hostname = func_utils.get_hostname_by_route()
+ writetarget = append_result[hostname]
+
+ # If the result of the append function was 0 (or -1), just return the same value.
+ if type(filepath) == type(0):
+ return filepath
+
+ if not os.path.exists(filepath):
+ # file disappeared
+ return -1
+
+ try:
+ shutil.move(writetarget,filepath)
+ except (IOError, OSError), e:
+ # Clean up by removing the tempfile, then return an error.
+ os.unlink(writetarget)
+ return -1
+
+ return 1
def copyfile(self, filepath, filebuf, mode=0644, uid=0, gid=0, force=None):
# -1 = problem file was not copied
diff --git a/func/overlord/modules/copyfile.py b/func/overlord/modules/copyfile.py
index 9f385a9..9a2bf2b 100644
--- a/func/overlord/modules/copyfile.py
+++ b/func/overlord/modules/copyfile.py
@@ -19,13 +19,15 @@ class copyfile(overlord_module.BaseModule):
uid = st.st_uid
gid = st.st_gid
- self.parent.run("copyfile", "open", [remotepath, mode, uid, gid])
+ open_result = self.parent.run("copyfile", "open", [remotepath, mode, uid, gid])
while True:
data=f.read(bufsize)
if data:
- self.parent.run("copyfile", "append", [remotepath, xmlrpclib.Binary(data)])
+ append_result = self.parent.run("copyfile", "append", [open_result, xmlrpclib.Binary(data)])
else:
break
+ self.parent.run("copyfile", "move", [remotepath, append_result])
+
return True
--
1.7.7
>From 1f16288e1f47e20ef4e35029eda8dd7d6fdab28d Mon Sep 17 00:00:00 2001
From: Marcus Lauer <marcuslauer@xxxxxxxxxx>
Date: Mon, 3 Sep 2012 13:40:15 -0400
Subject: [PATCH 2/4] Make copyfile write to a tempfile on the minion then
move the file at the end
---
func/minion/modules/copyfile.py | 13 +++++++------
func/overlord/modules/copyfile.py | 1 +
2 files changed, 8 insertions(+), 6 deletions(-)
diff --git a/func/minion/modules/copyfile.py b/func/minion/modules/copyfile.py
index 8281747..b8e6d99 100644
--- a/func/minion/modules/copyfile.py
+++ b/func/minion/modules/copyfile.py
@@ -65,7 +65,7 @@ class CopyFile(func_module.FuncModule):
writetarget = filepath + '.functmp'
try:
- fo = open(filepath, 'w')
+ fo = open(writetarget, 'w')
fo.close()
del fo
except (IOError, OSError), e:
@@ -100,6 +100,7 @@ class CopyFile(func_module.FuncModule):
subprocess.check_call(["chcon", "--reference=" + filepath, writetarget])
except (OSError), e:
# Do nothing. Presumably SELinux is not enabled on this system.
+ pass
return writetarget
@@ -125,17 +126,17 @@ class CopyFile(func_module.FuncModule):
os.unlink(writetarget)
return -1
- return 1
+ return writetarget
- def move(self, filepath, append_result)
+ def move(self, filepath, append_result):
hostname = func_utils.get_hostname_by_route()
writetarget = append_result[hostname]
# If the result of the append function was 0 (or -1), just return the same value.
- if type(filepath) == type(0):
- return filepath
+ if type(writetarget) == type(0):
+ return writetarget
- if not os.path.exists(filepath):
+ if not os.path.exists(writetarget):
# file disappeared
return -1
diff --git a/func/overlord/modules/copyfile.py b/func/overlord/modules/copyfile.py
index 9a2bf2b..148d5b1 100644
--- a/func/overlord/modules/copyfile.py
+++ b/func/overlord/modules/copyfile.py
@@ -5,6 +5,7 @@ import sys
import xmlrpclib
from func.overlord import overlord_module
+import func.minion.modules.copyfile as copyfile
class copyfile(overlord_module.BaseModule):
def send(self, localpath, remotepath, bufsize=60000):
--
1.7.7
>From 67c89de4ec644f4f8ad695750f05abd4b883d4e2 Mon Sep 17 00:00:00 2001
From: Marcus Lauer <marcuslauer@xxxxxxxxxx>
Date: Mon, 3 Sep 2012 15:49:12 -0400
Subject: [PATCH 3/4] More work on making copyfile write to a tempfile on the
minion
---
func/minion/modules/copyfile.py | 46 +++++++++++++++------------------
func/overlord/cmd_modules/copyfile.py | 4 ++-
func/overlord/modules/copyfile.py | 4 +-
3 files changed, 26 insertions(+), 28 deletions(-)
diff --git a/func/minion/modules/copyfile.py b/func/minion/modules/copyfile.py
index b8e6d99..582e21b 100644
--- a/func/minion/modules/copyfile.py
+++ b/func/minion/modules/copyfile.py
@@ -56,7 +56,7 @@ class CopyFile(func_module.FuncModule):
return thissum.hexdigest()
- def open(self, filepath, mode=None, uid=-1, gid=-1):
+ def open(self, filepath, retainperms, mode=None, uid=-1, gid=-1):
dirpath = os.path.dirname(filepath)
if not os.path.exists(dirpath):
os.makedirs(dirpath)
@@ -72,35 +72,31 @@ class CopyFile(func_module.FuncModule):
# XXX logger output here
return -1
- """
- Set the mode and ownership of the tempfile if requested.
- If not set them to match the target file if the target
- file exists.
- """
- if mode or uid != -1 or gid != -1:
+ if retainperms:
+ # Make the attributes of the tempfile match the target file.
+ if os.path.isfile(filepath):
+ # Copy the mode of the existing file.
+ shutil.copystat(filepath,writetarget)
+
+ # Copy the uid/gid of the existing file.
+ fileinfo = os.stat(filepath)
+ os.chown(writetarget,fileinfo[4],fileinfo[5])
+
+ # Copy the SELinux attribs of the existing file.
+ import subprocess
+ refstring = "--reference=" + filepath
+ selinx_result = subprocess.call(["chcon", refstring, writetarget], stdout=None, stderr=None)
+
+ else:
+ # Set the mode and ownership of the file to match the server.
try:
# we could intify the mode here if it's a string
if mode:
- os.chmod(filepath, mode)
+ os.chmod(writetarget, mode)
if uid != -1 or gid != -1:
- os.chown(filepath, uid, gid)
+ os.chown(writetarget, uid, gid)
except (IOError, OSError), e:
- return -1
- elif os.path.exists(filepath):
- # Copy the mode of the existing file.
- shutil.copystat(filepath,writetarget)
-
- # Copy the uid/gid of the existing file.
- fileinfo = os.stat(filepath)
- os.chown(writetarget,fileinfo[4],fileinfo[5])
-
- # Copy the SELinux attribs of the existing file.
- import subprocess
- try:
- subprocess.check_call(["chcon", "--reference=" + filepath, writetarget])
- except (OSError), e:
- # Do nothing. Presumably SELinux is not enabled on this system.
- pass
+ return -1
return writetarget
diff --git a/func/overlord/cmd_modules/copyfile.py b/func/overlord/cmd_modules/copyfile.py
index 3f2263d..3796515 100644
--- a/func/overlord/cmd_modules/copyfile.py
+++ b/func/overlord/cmd_modules/copyfile.py
@@ -38,6 +38,8 @@ class CopyFile(base_command.BaseCommand):
action="store_true")
self.parser.add_option("-v", "--verbose", dest="verbose",
action="store_true")
+ self.parser.add_option("", "--retainperms", dest="retainperms",
+ action="store_true")
def handleOptions(self, options):
self.verbose = self.options.verbose
@@ -50,4 +52,4 @@ class CopyFile(base_command.BaseCommand):
self.server_spec = self.parentCommand.server_spec
self.getOverlord()
- return self.overlord_obj.local.copyfile.send(self.options.filename, self.options.remotepath)
+ return self.overlord_obj.local.copyfile.send(self.options.filename, self.options.remotepath, self.options.retainperms)
diff --git a/func/overlord/modules/copyfile.py b/func/overlord/modules/copyfile.py
index 148d5b1..bef3740 100644
--- a/func/overlord/modules/copyfile.py
+++ b/func/overlord/modules/copyfile.py
@@ -8,7 +8,7 @@ from func.overlord import overlord_module
import func.minion.modules.copyfile as copyfile
class copyfile(overlord_module.BaseModule):
- def send(self, localpath, remotepath, bufsize=60000):
+ def send(self, localpath, remotepath, retainperms, bufsize=60000):
try:
f = open(localpath, "r")
except IOError, e:
@@ -20,7 +20,7 @@ class copyfile(overlord_module.BaseModule):
uid = st.st_uid
gid = st.st_gid
- open_result = self.parent.run("copyfile", "open", [remotepath, mode, uid, gid])
+ open_result = self.parent.run("copyfile", "open", [remotepath, retainperms, mode, uid, gid])
while True:
data=f.read(bufsize)
--
1.7.7
>From 9988429a94e613dae6f33c7383797cc7ec922a05 Mon Sep 17 00:00:00 2001
From: Marcus Lauer <marcuslauer@xxxxxxxxxx>
Date: Mon, 3 Sep 2012 16:02:04 -0400
Subject: [PATCH 4/4] Make False the default for new retainperms flag in
copyfile
---
func/overlord/modules/copyfile.py | 2 +-
1 files changed, 1 insertions(+), 1 deletions(-)
diff --git a/func/overlord/modules/copyfile.py b/func/overlord/modules/copyfile.py
index bef3740..6ac97a6 100644
--- a/func/overlord/modules/copyfile.py
+++ b/func/overlord/modules/copyfile.py
@@ -8,7 +8,7 @@ from func.overlord import overlord_module
import func.minion.modules.copyfile as copyfile
class copyfile(overlord_module.BaseModule):
- def send(self, localpath, remotepath, retainperms, bufsize=60000):
+ def send(self, localpath, remotepath, retainperms=False, bufsize=60000):
try:
f = open(localpath, "r")
except IOError, e:
--
1.7.7
_______________________________________________
func mailing list
func@xxxxxxxxxxxxxxxxxxxxxx
https://lists.fedorahosted.org/mailman/listinfo/func
