6:22 p.m.
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
6:32 p.m.
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(a)nyc.rr.com>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(a)lists.fedorahosted.org
https://lists.fedorahosted.org/mailman/listinfo/func
--
Steve Salevan
steve(a)tumblr.com
3:55 p.m.
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(a)nyc.rr.com>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(a)lists.fedorahosted.org
> https://lists.fedorahosted.org/mailman/listinfo/func
--
Steve Salevan
steve(a)tumblr.com
12:42 p.m.
On Friday, August 24, 2012 07:22:32 PM Marcus Lauer wrote:
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.
Now that I have taken another look at the code I see that this is not
true. With my patch "copyfile" still sends all of the data from the overlord
to the minion even if it does not write it.
Any minions on which the file has not changed should not run the
"append" or later parts of the copyfile. Unfortunately I do not know enough
about func to implement this properly. So for now they do run "append" but do
not actually append any data to the file on the minion (which was not
previously truncated).
--
Marcus
3:29 p.m.
New subject: PATCH (Re: Changes to copyfile functionality)
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(a)nyc.rr.com>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(a)lists.fedorahosted.org
> > https://lists.fedorahosted.org/mailman/listinfo/func
>
> --
> Steve Salevan
> steve(a)tumblr.com
7:01 p.m.
New subject: PATCH (Re: Changes to copyfile functionality)
Attached is an updated version of this patch which makes two more
changes to the CopyFile minion.
One major change is that it now tries to use the pyxattr library to
copy extended attributes (including SELinux contexts) from the target file to
the tempfile. Pyxattr is not a core library but it is packaged for RHEL5 and
6. If this library is not available then it falls back to calling chcon.
The other major change is that it explicitly looks for a SELinux
config file. If one is present then it will consider a failure to copy the
SELinux contexts from the target file to the tempfile to be an error and will
exit. Previously it just kept going when this failed.
The remaining changes are organizational, e.g. moving the "from
subprocess import..." to the top.
--
Marcus
On Monday, September 03, 2012 04:29:28 PM Marcus Lauer wrote:
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(a)nyc.rr.com>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(a)lists.fedorahosted.org
> > > https://lists.fedorahosted.org/mailman/listinfo/func
> >
> > --
> > Steve Salevan
> > steve(a)tumblr.com
1:01 p.m.
New subject: PATCH (Re: Changes to copyfile functionality)
I really should have made the "retainperms" argument to copyfile.send
optional. This small patch does so by assigning it a default value of
"False". The patch should be applied after my previous patch of September
18th.
--
Marcus
On Tuesday, September 18, 2012 08:01:41 PM Marcus Lauer wrote:
Attached is an updated version of this patch which makes two
more
changes to the CopyFile minion.
One major change is that it now tries to use the pyxattr library to
copy extended attributes (including SELinux contexts) from the target file
to the tempfile. Pyxattr is not a core library but it is packaged for
RHEL5 and 6. If this library is not available then it falls back to
calling chcon.
The other major change is that it explicitly looks for a SELinux
config file. If one is present then it will consider a failure to copy the
SELinux contexts from the target file to the tempfile to be an error and
will exit. Previously it just kept going when this failed.
The remaining changes are organizational, e.g. moving the "from
subprocess import..." to the top.
--
Marcus
On Monday, September 03, 2012 04:29:28 PM Marcus Lauer wrote:
> 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(a)nyc.rr.com>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(a)lists.fedorahosted.org
> > > > https://lists.fedorahosted.org/mailman/listinfo/func
> > >
> > > --
> > > Steve Salevan
> > > steve(a)tumblr.com
4229
days inactive
4265
days old
6 comments
2 participants
participants (2)
-
Marcus Lauer -
Steve Salevan