Bug#841025: [Piuparts-devel] Bug#841025: Hard links patch for bug #754878 makes little sense

[Holger Levsen]

control: tags -1 + pending

Hi Alexander,

On Mon, Oct 17, 2016 at 11:13:08AM +0200, Alexander Thomas wrote:
> >> 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.
 
hehe, very nice. I assume this is esaturnus.com :)

> 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.

thanks for explaining and doing the change like this! Much appreciated.
As the whole bug+patch! :)

On Mon, Oct 17, 2016 at 11:33:46AM +0200, Alexander Thomas wrote:
> > Regarding your use case: why don't you use chroot tarballs? IMO, tar xfz
> > might be faster than cp -a.
> We benchmarked it and on our trusty old build server, cp -a was faster.

interesting.

> Here's a small update that mentions --hard-link in --existing-chroot.

thanks a lot, I've finally merged this into the develop branch. And sorry
for the delay & "btw" I do plan to do a release in the next two weeks or so.
This year. :)


-- 
cheers,
Holger


signature.asc
Description: Digital signature

Bug#841025: Hard links patch for bug #754878 makes little sense

[Alexander Thomas]

On Mon, Oct 17, 2016 at 11:25 AM, Andreas Beckmann  wrote:
> On 2016-10-17 11:13, Alexander Thomas wrote:
>> 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
>
> Looks ok from my point, maybe the documentation of --existing-chroot
> could mention the new option.
>
> Regarding your use case: why don't you use chroot tarballs? IMO, tar xfz
> might be faster than cp -a.

We benchmarked it and on our trusty old build server, cp -a was faster.

Here's a small update that mentions --hard-link in --existing-chroot.

Regards,

-- 
Alexander Thomas
diff --git a/piuparts.1.txt b/piuparts.1.txt
index a857f4d..80ae746 100644
--- a/piuparts.1.txt
+++ b/piuparts.1.txt
@@ -90,7 +90,7 @@ The tarball can be created with the '-s' option, or you can use one that *pbuild
 *-e* 'dirname', *--existing-chroot*='dirname'::
   Use the specified directory as source for the new chroot, instead of building
   a new one with debootstrap. This is similar to '--basetgz', but the contents
-  are not archived.
+  are not archived. See also the --hard-link option.
 
 *--distupgrade-to-testdebs*::
   Use the "testdebs" repository to override the packages in the distupgrade
@@ -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

Bug#841025: Hard links patch for bug #754878 makes little sense

[Andreas Beckmann]

On 2016-10-17 11:13, Alexander Thomas wrote:
> 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

Looks ok from my point, maybe the documentation of --existing-chroot
could mention the new option.

Regarding your use case: why don't you use chroot tarballs? IMO, tar xfz
might be faster than cp -a.

Andreas

Bug#841025: Hard links patch for bug #754878 makes little sense

[Alexander Thomas]

On Mon, Oct 17, 2016 at 1:08 AM, Holger Levsen  wrote:
> package: piuparts
> submitter: Alexander Thomas 
> x-debbugs-cc: Alexander Thomas 
>
> 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
 

Bug#841025: Hard links patch for bug #754878 makes little sense

[Holger Levsen]

package: piuparts
submitter: Alexander Thomas 
x-debbugs-cc: Alexander Thomas 

Hi Alexander,

thanks for your bug report, I turned it into one so we don't loose track
of it!

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?


> 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 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.

Right now I'm too tired to actually look at the code itself… Git patches
are also much welcome too! ;-)

> -- 
> Alexander Thomas

Thanks again! :)


-- 
cheers,
Holger


signature.asc
Description: Digital signature