| Message ID | 20171117004110.23143-1-luizluca@gmail.com |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | [LEDE-DEV] base-files: do not backup unchanged files | expand |
After applied, the list of files in backup in a fresh installation change from:
root@LEDE:~# sysupgrade -l
/etc/config/dhcp
/etc/config/dropbear
/etc/config/firewall
/etc/config/network
/etc/config/system
/etc/dropbear/dropbear_rsa_host_key
/etc/group
/etc/hosts
/etc/inittab
/etc/opkg/keys/5151f69420c3f508
/etc/opkg/keys/603b85163caded45
/etc/opkg/keys/72a57f2191b211e0
/etc/opkg/keys/792d9d9b39f180dc
/etc/opkg/keys/9ef4694208102c43
/etc/opkg/keys/b5043e70f9a75cde
/etc/opkg/keys/dace9d4df16896bf
/etc/opkg/keys/dd6de0d06bbd3d85
/etc/passwd
/etc/profile
/etc/rc.local
/etc/shadow
/etc/shells
/etc/sysctl.conf
/etc/sysctl.d/local.conf
to:
root@LEDE:~# sysupgrade -l
/etc/config/dhcp
/etc/config/firewall
/etc/config/network
/etc/config/system
/etc/dropbear/dropbear_rsa_host_key
This solves a problem that, on a upgrade, LEDE/OpenWrt always use
/etc/profile (and others) way back from the first
LEDE/OpenWrt installation, even if the user never touched those files.
Even files listed in "opkg list-changed-conffiles" might be skipped if
they are equal to /rom version. For example,
some packages (dnsmasq) add a new user/group, changing /etc/passwd. If
the changed /etc/passwd is in /rom (changes done while
building the image), it will not be in backup.
Regards,
---
Luiz Angelo Daros de Luca, Me.
luizluca@gmail.com
> On 17 Nov 2017, at 00:41, luizluca@gmail.com wrote: > > From: Luiz Angelo Daros de Luca <luizluca@gmail.com> > > Only backup /aaa/bbb/ccc if /rom/aaa/bbb/ccc does not exist > or /aaa/bbb/ccc is different from /rom/aaa/bbb/ccc. > > Signed-off-by: Luiz Angelo Daros de Luca <luizluca@gmail.com> > --- > package/base-files/files/sbin/sysupgrade | 6 ++++-- > 1 file changed, 4 insertions(+), 2 deletions(-) > <snip> You need to bump PKG_RELEASE in base-files/Makefile Cheers, Kevin
On 2017-11-17 02:07, Luiz Angelo Daros de Luca wrote: > After applied, the list of files in backup in a fresh installation change from: > > root@LEDE:~# sysupgrade -l > /etc/config/dhcp > /etc/config/dropbear > /etc/config/firewall > /etc/config/network > /etc/config/system > /etc/dropbear/dropbear_rsa_host_key > /etc/group > /etc/hosts > /etc/inittab > /etc/opkg/keys/5151f69420c3f508 > /etc/opkg/keys/603b85163caded45 > /etc/opkg/keys/72a57f2191b211e0 > /etc/opkg/keys/792d9d9b39f180dc > /etc/opkg/keys/9ef4694208102c43 > /etc/opkg/keys/b5043e70f9a75cde > /etc/opkg/keys/dace9d4df16896bf > /etc/opkg/keys/dd6de0d06bbd3d85 > /etc/passwd > /etc/profile > /etc/rc.local > /etc/shadow > /etc/shells > /etc/sysctl.conf > /etc/sysctl.d/local.conf > > to: > > root@LEDE:~# sysupgrade -l > /etc/config/dhcp > /etc/config/firewall > /etc/config/network > /etc/config/system > /etc/dropbear/dropbear_rsa_host_key > <snip> Hi, Did you also account that some of the files will still be backupped (even when unchanged) as they are also referenced here? : [ Casino BB | node-10 ] cd /lib/upgrade/keep.d/ [ Casino BB | node-10 ] ls base-files netdata sudo base-files-essential openvpn uboot-envtools dbus opkg isc-dhcp-server-ipv4 ppp [ Casino BB | node-10 ] cat * /etc/config/ /etc/config/network /etc/config/system /etc/crontabs/ /etc/dropbear/ /etc/profile.d /etc/sysctl.d/ # Essential files that will be always kept /etc/hosts /etc/inittab /etc/group /etc/passwd /etc/profile /etc/shadow /etc/shells /etc/sysctl.conf /etc/rc.local /etc/dbus-1/session.conf /etc/dbus-1/system.conf /etc/dhcpd.conf /etc/netdata/ /etc/openvpn/ /etc/opkg/keys/ /etc/ppp/ip-down /etc/ppp/ip-up /etc/ppp/ipv6-down /etc/ppp/ipv6-up /etc/sudoers.d/ /etc/config/ubootenv /etc/fw_env.config [ Casino BB | node-10 ] Only "sysupgrade -n" will change this behaviour, but in that case, *nothing* is saved at all. Koen
On 17 November 2017 at 01:41, <luizluca@gmail.com> wrote: > From: Luiz Angelo Daros de Luca <luizluca@gmail.com> > > Only backup /aaa/bbb/ccc if /rom/aaa/bbb/ccc does not exist > or /aaa/bbb/ccc is different from /rom/aaa/bbb/ccc. > > Signed-off-by: Luiz Angelo Daros de Luca <luizluca@gmail.com> > --- > package/base-files/files/sbin/sysupgrade | 6 ++++-- > 1 file changed, 4 insertions(+), 2 deletions(-) > > diff --git a/package/base-files/files/sbin/sysupgrade b/package/base-files/files/sbin/sysupgrade > index 359f21f51c..0085dbe07e 100755 > --- a/package/base-files/files/sbin/sysupgrade > +++ b/package/base-files/files/sbin/sysupgrade > @@ -101,8 +101,10 @@ add_uci_conffiles() { > local file="$1" > ( find $(sed -ne '/^[[:space:]]*$/d; /^#/d; p' \ > /etc/sysupgrade.conf /lib/upgrade/keep.d/* 2>/dev/null) \ > - -type f -o -type l 2>/dev/null; > - opkg list-changed-conffiles ) | sort -u > "$file" > + $(opkg list-changed-conffiles) \ > + \( -type f -o -type l \) \ > + \( \( -exec test -e /rom{} \; -exec cmp -s {} /rom{} \; \) -o -print \) 2>/dev/null; > + ) | sort -u > "$file" "opkg list-changed-conffiles" should have already filtered by that (but obviously didn't), so the issue should be fixed at the source instead of being worked around. Regards Jonas
On 11/17/2017 10:14 AM, Jonas Gorski wrote: > On 17 November 2017 at 01:41, <luizluca@gmail.com> wrote: >> From: Luiz Angelo Daros de Luca <luizluca@gmail.com> >> >> Only backup /aaa/bbb/ccc if /rom/aaa/bbb/ccc does not exist >> or /aaa/bbb/ccc is different from /rom/aaa/bbb/ccc. >> >> Signed-off-by: Luiz Angelo Daros de Luca <luizluca@gmail.com> >> --- >> package/base-files/files/sbin/sysupgrade | 6 ++++-- >> 1 file changed, 4 insertions(+), 2 deletions(-) >> >> diff --git a/package/base-files/files/sbin/sysupgrade b/package/base-files/files/sbin/sysupgrade >> index 359f21f51c..0085dbe07e 100755 >> --- a/package/base-files/files/sbin/sysupgrade >> +++ b/package/base-files/files/sbin/sysupgrade >> @@ -101,8 +101,10 @@ add_uci_conffiles() { >> local file="$1" >> ( find $(sed -ne '/^[[:space:]]*$/d; /^#/d; p' \ >> /etc/sysupgrade.conf /lib/upgrade/keep.d/* 2>/dev/null) \ >> - -type f -o -type l 2>/dev/null; >> - opkg list-changed-conffiles ) | sort -u > "$file" >> + $(opkg list-changed-conffiles) \ >> + \( -type f -o -type l \) \ >> + \( \( -exec test -e /rom{} \; -exec cmp -s {} /rom{} \; \) -o -print \) 2>/dev/null; >> + ) | sort -u > "$file" > > "opkg list-changed-conffiles" should have already filtered by that > (but obviously didn't), so the issue should be fixed at the source > instead of being worked around. `opkg list-changed-conffiles` does filter for changed files - but the files listed in /etc/sysupgrade.conf or /lib/upgrade/keep.d are backed up unconditionally. I'm not sure which behaviour the average admin would expect - but I'm not happy about the different handling of config files managed by opkg and the other file lists. I don't like adding overlay-specific logic here, sysupgrade should work "the same" on all targets, with or without overlay. Maybe we can rather filter out unchanged files *after* the sysupgrade (after a major config backup rework I guess - but I heard people are thinking about a generic tar-based backup approach for sysupgrades). Another issue with this patch: it breaks for filenames with spaces in the opkg list-changed-conffiles output (might or might not be an actual issue, but something to keep in mind). Matthias
On 17 November 2017 at 11:35, Matthias Schiffer <mschiffer@universe-factory.net> wrote: > On 11/17/2017 10:14 AM, Jonas Gorski wrote: >> On 17 November 2017 at 01:41, <luizluca@gmail.com> wrote: >>> From: Luiz Angelo Daros de Luca <luizluca@gmail.com> >>> >>> Only backup /aaa/bbb/ccc if /rom/aaa/bbb/ccc does not exist >>> or /aaa/bbb/ccc is different from /rom/aaa/bbb/ccc. >>> >>> Signed-off-by: Luiz Angelo Daros de Luca <luizluca@gmail.com> >>> --- >>> package/base-files/files/sbin/sysupgrade | 6 ++++-- >>> 1 file changed, 4 insertions(+), 2 deletions(-) >>> >>> diff --git a/package/base-files/files/sbin/sysupgrade b/package/base-files/files/sbin/sysupgrade >>> index 359f21f51c..0085dbe07e 100755 >>> --- a/package/base-files/files/sbin/sysupgrade >>> +++ b/package/base-files/files/sbin/sysupgrade >>> @@ -101,8 +101,10 @@ add_uci_conffiles() { >>> local file="$1" >>> ( find $(sed -ne '/^[[:space:]]*$/d; /^#/d; p' \ >>> /etc/sysupgrade.conf /lib/upgrade/keep.d/* 2>/dev/null) \ >>> - -type f -o -type l 2>/dev/null; >>> - opkg list-changed-conffiles ) | sort -u > "$file" >>> + $(opkg list-changed-conffiles) \ >>> + \( -type f -o -type l \) \ >>> + \( \( -exec test -e /rom{} \; -exec cmp -s {} /rom{} \; \) -o -print \) 2>/dev/null; >>> + ) | sort -u > "$file" >> >> "opkg list-changed-conffiles" should have already filtered by that >> (but obviously didn't), so the issue should be fixed at the source >> instead of being worked around. > > `opkg list-changed-conffiles` does filter for changed files The issue seems to be a bit more complex than at first sight. There are at least two issues causing all /rom files treated as modified: 1) The hashing of opkg seems to be broken: # opkg -V2 list-changed-conffiles ... conffile_has_been_modified: Conffile /etc/ppp/options: old chk=851bf20b58373edaba24255bb5c8abf86288379f6f0d99c72c01b76cee56a7b7 new chk=a0509db666fc831a3f9332ca6e911ba9a32e7f1ce13733e539a719f0413794b9 # sha256sum /etc/ppp/options a0509db666fc831a3f9332ca6e911ba9a32e7f1ce13733e539a719f0413794b9 /etc/ppp/options # grep "/etc/ppp/options" /usr/lib/opkg/status /etc/ppp/options a0509db666fc831a3f9332ca6e911ba9a32e7f1ce13733e539a719f0413794b9 (old chk is the calculated checksum of the existing file, new chk is the one according to status) So while the one from status is properly read, the generated one doesn't match the existing file at all. 2.) We *do* modify some files during offline root generation after the base package had been installed. E.g. we modify the /etc/passwd and /etc/groups for all users and groups required by packages. So these being treated as modified shouldn't come as a surprise. The first issue is an obvious bug, and needs to be fixed in opkg. The second issue is a bit more complicated. One option to fix would be to regenerate the checksums at the end of the rootfs-creation, one option would be to the manual check like this patch does. The question for implications is if there are parts that rely on files being implicitly backed up, and can there be issues when only doing partial backups. On a first glance it seems that adding a user to an existing group will still modify the /etc/groups, so /etc/passwd and /etc/users will always be backed up at the same time, thus group ids cannot go out of sync (in case they are dynamically allocated). > - but the files > listed in /etc/sysupgrade.conf or /lib/upgrade/keep.d are backed up > unconditionally. I'm not sure which behaviour the average admin would > expect - but I'm not happy about the different handling of config files > managed by opkg and the other file lists. > > I don't like adding overlay-specific logic here, sysupgrade should work > "the same" on all targets, with or without overlay. Maybe we can rather > filter out unchanged files *after* the sysupgrade (after a major config > backup rework I guess - but I heard people are thinking about a generic > tar-based backup approach for sysupgrades). This is IMHO a bit of a philosophical question - if one specifies additional files to backup, does this mean we should back them up in any case, or should we second guess the user and check if they were present before and were modified since then? Maybe we need a new flag for that (meaning "full config backup" or "modified-only"). I have to admit, I always assumed that if I tell sysupgrade to backup the config, it backs up the full config, and not only the modified parts. But that might be only me ;-) Although, the help says: -b | --create-backup <file> create .tar.gz of files specified in sysupgrade.conf then exit. Does not flash an image. If file is '-', i.e. stdout, verbosity is set to 0 (i.e. quiet). and the sysupgrade.conf says: ## This file contains files and directories that should ## be preserved during an upgrade. Which sort of implies to me that it creates a full backup - it doesn't mention anywhere that it will skip unmodified files. Not that I'm against doing the diff-version, just that the texts should be updated as well in that case. Regards Jonas
On 17 November 2017 at 13:23, Jonas Gorski <jonas.gorski@gmail.com> wrote: > On 17 November 2017 at 11:35, Matthias Schiffer > <mschiffer@universe-factory.net> wrote: >> On 11/17/2017 10:14 AM, Jonas Gorski wrote: >>> On 17 November 2017 at 01:41, <luizluca@gmail.com> wrote: >>>> From: Luiz Angelo Daros de Luca <luizluca@gmail.com> >>>> >>>> Only backup /aaa/bbb/ccc if /rom/aaa/bbb/ccc does not exist >>>> or /aaa/bbb/ccc is different from /rom/aaa/bbb/ccc. >>>> >>>> Signed-off-by: Luiz Angelo Daros de Luca <luizluca@gmail.com> >>>> --- >>>> package/base-files/files/sbin/sysupgrade | 6 ++++-- >>>> 1 file changed, 4 insertions(+), 2 deletions(-) >>>> >>>> diff --git a/package/base-files/files/sbin/sysupgrade b/package/base-files/files/sbin/sysupgrade >>>> index 359f21f51c..0085dbe07e 100755 >>>> --- a/package/base-files/files/sbin/sysupgrade >>>> +++ b/package/base-files/files/sbin/sysupgrade >>>> @@ -101,8 +101,10 @@ add_uci_conffiles() { >>>> local file="$1" >>>> ( find $(sed -ne '/^[[:space:]]*$/d; /^#/d; p' \ >>>> /etc/sysupgrade.conf /lib/upgrade/keep.d/* 2>/dev/null) \ >>>> - -type f -o -type l 2>/dev/null; >>>> - opkg list-changed-conffiles ) | sort -u > "$file" >>>> + $(opkg list-changed-conffiles) \ >>>> + \( -type f -o -type l \) \ >>>> + \( \( -exec test -e /rom{} \; -exec cmp -s {} /rom{} \; \) -o -print \) 2>/dev/null; >>>> + ) | sort -u > "$file" >>> >>> "opkg list-changed-conffiles" should have already filtered by that >>> (but obviously didn't), so the issue should be fixed at the source >>> instead of being worked around. >> >> `opkg list-changed-conffiles` does filter for changed files > > The issue seems to be a bit more complex than at first sight. > > There are at least two issues causing all /rom files treated as modified: > > 1) The hashing of opkg seems to be broken: > > # opkg -V2 list-changed-conffiles > ... > conffile_has_been_modified: Conffile /etc/ppp/options: > old chk=851bf20b58373edaba24255bb5c8abf86288379f6f0d99c72c01b76cee56a7b7 > new chk=a0509db666fc831a3f9332ca6e911ba9a32e7f1ce13733e539a719f0413794b9 > # sha256sum /etc/ppp/options > a0509db666fc831a3f9332ca6e911ba9a32e7f1ce13733e539a719f0413794b9 > /etc/ppp/options > # grep "/etc/ppp/options" /usr/lib/opkg/status > /etc/ppp/options > a0509db666fc831a3f9332ca6e911ba9a32e7f1ce13733e539a719f0413794b9 > > (old chk is the calculated checksum of the existing file, new chk is > the one according to status) > > So while the one from status is properly read, the generated one > doesn't match the existing file at all. Okay, found the issue, the sha256sum calculation was broken for big endian systems. With it fixed it's a much more managable list (with a freshly booted system): root@LEDE:/# opkg list-changed-conffiles /etc/group /etc/passwd /etc/shadow /etc/config/dhcp /etc/dropbear/dropbear_rsa_host_key root@LEDE:/# Regards Jonas
Kevin, > You need to bump PKG_RELEASE in base-files/Makefile I missed that one. I'll fix it. Stefen, > Is this really needed? Just try to remove package/base-files/files/lib/ upgrade/keep.d/base-files-essential and your problem will be gone. I think this file should be only added to the image if opkg is not installed. > > https://lists.openwrt.org/pipermail/openwrt-devel/2013-February/018742.html Removing base-files-essential will only "solve" the problem for those files listed there (but not for /etc/sysctl.d/local.conf). I'm trying to change the logic and not the input of "what is interesting for backup". Koen, > Did you also account that some of the files will still be backupped (even when unchanged) as they are also referenced here? : > > [ Casino BB | node-10 ] cd /lib/upgrade/keep.d/ AFAIK, this patch applies to all files mentioned /etc/sysupgrade.conf, /lib/ upgrade/keep.d/ or opkg list-changed-conffiles Matthias, > I don't like adding overlay-specific logic here, sysupgrade should work > "the same" on all targets, with or without overlay. Maybe we can rather > filter out unchanged files *after* the sysupgrade (after a major config > backup rework I guess - but I heard people are thinking about a generic > tar-based backup approach for sysupgrades). it really depends on /rom, which is present when overlay is in use. However, if space is not a problem, a simple copy of pristine / into /rom would work. In the future, OpenWrt might use a FS that has snapshots, which could also provide /rom. > Another issue with this patch: it breaks for filenames with spaces in the > opkg list-changed-conffiles output (might or might not be an actual issue, > but something to keep in mind). Yes, I'll break. I noticed that but the only solution that I imaged consists of a hacky multiline script like: "set --; while read line; do set -- '$@" "$line"; done <"$output of opkg..." I'll fix it. Jonas, > 1) The hashing of opkg seems to be broken: Sorry, I don't have this problem. Anyway, the patch helped fix a bug (bonus). > 2.) We *do* modify some files during offline root generation after the base package had been installed. I don't think this as an issue. Build process do change files but I imagine that a sysadmin might also want to generate a custom image with changed files. IMHO, packages must still consider those files changed. > The question for implications is if there are parts that rely on files > being implicitly backed up, and can there be issues when only doing > partial backups. On a first glance it seems that adding a user to an > existing group will still modify the /etc/groups, so /etc/passwd and > /etc/users will always be backed up at the same time, thus group ids > cannot go out of sync (in case they are dynamically allocated). > >> - but the files >> listed in /etc/sysupgrade.conf or /lib/upgrade/keep.d are backed up >> unconditionally. I'm not sure which behaviour the average admin would >> expect - but I'm not happy about the different handling of config files >> managed by opkg and the other file lists. >> >> I don't like adding overlay-specific logic here, sysupgrade should work >> "the same" on all targets, with or without overlay. Maybe we can rather >> filter out unchanged files *after* the sysupgrade (after a major config >> backup rework I guess - but I heard people are thinking about a generic >> tar-based backup approach for sysupgrades). > > This is IMHO a bit of a philosophical question - if one specifies > additional files to backup, does this mean we should back them up in > any case, or should we second guess the user and check if they were > present before and were modified since then? Maybe we need a new flag > for that (meaning "full config backup" or "modified-only"). I'm having difficult to think on a case where the sysadmin would like to copy a file that was already in the original image into the new system. I do use a different modified-only backup solution that depends on overlay and opkg. I'm cleaning it up before sending the patch. It simply backups all files in overlay, except for those files that comes from packages, but including "opkg list- changed-conffiles" ones. It is a "'sysupgrade -c' on steroids". This overlay backup together with a batch installation of all extra packages previously installed was able to restore my most complex installation with no extra intervention. > I have to admit, I always assumed that if I tell sysupgrade to backup > the config, it backs up the full config, and not only the modified > parts. But that might be only me ;-) If the user asked to backup a file, I agree that it should be in the backup. Maybe /etc/sysupgrade.conf should be included unconditionally. However, the user never asked to include /etc/profile. It is the default behavior from keep.d files. I got into this because a custom script that depends on /etc/profile.d simply failed for some LEDE routers. Same HW, same LEDE version, different behavior. I only isolated the problem after doing a full FS diff, detecting that /etc/profile was still from OpenWRT BB era. This is not how a "normal" linux distribution would work. RPM/DEB will quietly replace unchanged config files on an upgrade. In the case of /etc/profile, as it is more code than config, I guess they will quietly replace even if modified. BTW, this is an open issue (but not reported until now). Any software that depends on /etc/profile.d will not work on any LEDE system upgraded from OpenWRT because /etc/profile is outdated. An old /etc/profile gave me headaches but other outdated files in backup list might break other things for other sysadmin. This patch will not fix that but it could prevent new cases. > Although, the help says: > > -b | --create-backup <file> > create .tar.gz of files specified in sysupgrade.conf > then exit. Does not flash an image. If file is '-', > i.e. stdout, verbosity is set to 0 (i.e. quiet). > > and the sysupgrade.conf says: > > ## This file contains files and directories that should > ## be preserved during an upgrade. > > Which sort of implies to me that it creates a full backup - it doesn't > mention anywhere that it will skip unmodified files. Not that I'm > against doing the diff-version, just that the texts should be updated > as well in that case. Sure, they need to be updated as well. Regards, Luiz Angelo
diff --git a/package/base-files/files/sbin/sysupgrade b/package/base-files/files/sbin/sysupgrade index 359f21f51c..0085dbe07e 100755 --- a/package/base-files/files/sbin/sysupgrade +++ b/package/base-files/files/sbin/sysupgrade @@ -101,8 +101,10 @@ add_uci_conffiles() { local file="$1" ( find $(sed -ne '/^[[:space:]]*$/d; /^#/d; p' \ /etc/sysupgrade.conf /lib/upgrade/keep.d/* 2>/dev/null) \ - -type f -o -type l 2>/dev/null; - opkg list-changed-conffiles ) | sort -u > "$file" + $(opkg list-changed-conffiles) \ + \( -type f -o -type l \) \ + \( \( -exec test -e /rom{} \; -exec cmp -s {} /rom{} \; \) -o -print \) 2>/dev/null; + ) | sort -u > "$file" return 0 }