On Mon, Oct 17, 2016 at 1:08 AM, Holger Levsen <hol...@layer-acht.org> wrote:
> package: piuparts
> submitter: Alexander Thomas <alexander.tho...@esaturnus.com>
> x-debbugs-cc: Alexander Thomas <alexander.tho...@esaturnus.com>
>
> Hi Alexander,
>
> thanks for your bug report, I turned it into one so we don't loose track
> of it!
All right, thanks!
> On Fri, Oct 14, 2016 at 03:53:49PM +0200, Alexander Thomas wrote:
>> In July 2014, a patch was included to use `cp -al` instead of `cp -ax`
>> when the -e option is used and the chroot is on the same filesystem as
>> where piuparts is run. I wonder why this was included without making
>> it optional, or maybe why it was included at all.
>
> a quick grep in todays piuparts for "cp -al" (and for "cp" too) reveal no
> hits.
>
> can you confirm that piuparts 0.72 is affected?
Yes, the relevant code is around line 876 in piuparts.py:
if os.stat(dirname).st_dev == os.stat(self.name).st_dev:
cmd += ["-al"]
logging.debug("Hard linking %s to %s" % (dirname, self.name))
else:
cmd += ["-ax"]
logging.debug("Copying %s into %s" % (dirname, self.name))
>> Hard-linking the chroot makes little sense unless there is a guarantee
>> that none of the existing files will be modified. The only difference
>> with just running the test in the provided chroot itself, is that new
>> or deleted files will not be reflected in the original directory.
>> Modifications to existing files however will be reflected in the
>> original chroot, because of the hard links. Because the whole point of
>> piuparts testing is to detect unexpected changes, at some point a
>> naughty package will clobber existing files, and this change will
>> persist.
>> Maybe the submitter of the patch mistook `cp -al` as a copy-on-write,
>> or didn't mind that this would make the -e option of piuparts
>> destructive.
>>
>> In our setup,
>
> could you maybe expand a little bit on this, I'm curious (and having
> users is motivating!
We install our software by bundling its components in Debian packages.
When we release a patch, we distribute an updated set of packages that
are served from a local repository within the system. This allows to
simply do a dist-upgrade on all of the VMs and servers to apply the
patch. Of course we want to make sure such upgrades don't do anything
unexpected, therefore packages must pass a piuparts test before they
are released.
>> we perform multiple separate runs of piuparts, and it is
>> essential to start from a pristine copy of the same chroot every time.
>> We use the -e option to avoid having to re-generate and customize the
>> chroot for every test, or unpack the same tarball over and over again.
>> It is essential that the original chroot is not modified. Relying on
>> hard links method breaks this workflow unless we wrap every test
>> inside yet another cp -ax, which again makes everything slower.
>>
>> If anyone sees a valid use case for the hard linked chroot, an option
>> to enable it separately should be added. Otherwise this patch should
>> be reverted.
>
> reading #754878 it tells me this code should only be used with the
> --existing-chroot option.
Indeed.
> Right now I'm too tired to actually look at the code itself… Git patches
> are also much welcome too! ;-)
Because I don't know what was the use case for the submitter of that
hard-linking patch, it is safest to preserve it, but make it optional
with a new --hard-link switch that is only relevant together with
--existing-chroot. This is also very easy to implement, it only
requires one extra test on a boolean. I have attached a patch that
does exactly this.
--
Alexander Thomas
diff --git a/piuparts.1.txt b/piuparts.1.txt
index a857f4d..5a9c4d0 100644
--- a/piuparts.1.txt
+++ b/piuparts.1.txt
@@ -111,6 +111,11 @@ The tarball can be created with the '-s' option, or you can use one that *pbuild
Takes a comma separated list of package names and can be given multiple
times.
+*--hard-link*::
+ When the --existing-chroot option is used, and the source directory is on the
+ same filesystem, hard-link files instead of copying them. This is faster, but
+ any modifications to files will be reflected in the originals.
+
*-i* 'filename', *--ignore*='filename'::
Add a filename to the list of filenames to be ignored when comparing changes before and after installation. By default, piuparts ignores files that always change during a package installation and uninstallation, such as *dpkg* status files. The filename should be relative to the root of the chroot (e.g., _var/lib/dpkg/status_). This option can be used as many times as necessary.
diff --git a/piuparts.py b/piuparts.py
index f69c955..dbf2b18 100644
--- a/piuparts.py
+++ b/piuparts.py
@@ -178,6 +178,7 @@ class Settings:
self.lvm_snapshot_size = "1G"
self.adt_virt = None
self.existing_chroot = None
+ self.hard_link = False
self.schroot = None
self.end_meta = None
self.save_end_meta = None
@@ -873,7 +874,7 @@ class Chroot:
"""Create chroot from an existing one."""
# if on same device, make hard link
cmd = ["cp"]
- if os.stat(dirname).st_dev == os.stat(self.name).st_dev:
+ if settings.hard_link and os.stat(dirname).st_dev == os.stat(self.name).st_dev:
cmd += ["-al"]
logging.debug("Hard linking %s to %s" % (dirname, self.name))
else:
@@ -2768,6 +2769,11 @@ def parse_command_line():
"chroot, instead of building a new one with " +
"debootstrap")
+ parser.add_option("--hard-link", default=False,
+ action='store_true',
+ help="When using --existing-chroot, and the source dir is on the same"
+ "filesystem, hard-link files instead of copying them.")
+
parser.add_option("-i", "--ignore", action="append", metavar="FILENAME",
default=[],
help="Add FILENAME to list of filenames to be " +
@@ -3017,6 +3023,7 @@ def parse_command_line():
settings.lvm_volume = opts.lvm_volume
settings.lvm_snapshot_size = opts.lvm_snapshot_size
settings.existing_chroot = opts.existing_chroot
+ settings.hard_link = opts.hard_link
settings.schroot = opts.schroot
settings.end_meta = opts.end_meta
settings.save_end_meta = opts.save_end_meta
