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

[Alexander Thomas]

On Mon, Oct 17, 2016 at 1:08 AM, Holger Levsen <holger at layer-acht.org> wrote:
> package: piuparts
> submitter: Alexander Thomas <alexander.thomas at esaturnus.com>
> x-debbugs-cc: Alexander Thomas <alexander.thomas at 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
-------------- next part --------------
A non-text attachment was scrubbed...
Name: piuparts-optional-hard-link.patch
Type: text/x-patch
Size: 2780 bytes
Desc: not available
URL: <http://lists.alioth.debian.org/pipermail/piuparts-devel/attachments/20161017/76e694be/attachment-0003.bin>