Bug #2527

Re: git: build: implement automatic world backups

Added by c.turner1 over 1 year ago. Updated over 1 year ago.

Status:ClosedStart date:
Priority:NormalDue date:
Assignee:marino% Done:

0%

Category:-
Target version:-

Description

On 02/17/13 14:43, John Marino wrote:

> Additionally, every time the "installworld" target is executed, the same
> directories will be automatically backed up at the location of
> ${MAKEOBJDIRPREFIX}/world_backup . These directories could be restored
> with the new make target "restoreworld-auto".

Bug: This breaks on NFS object trees as the copy tries to set flags,
which can't be done over NFS

Also,

1) getting an 'off button' back for this would be swell
2) build(7) should probably be updated with whatever the final result is.

personally, I'd prefer this was optional behavior with an 'on' switch,
(e.g. it seems sort of like a 'developer setting' like CFLAGS, etc)

but, that's me.

sorry to back seat drive with no attched patch - lifes a bit hectic ATM.

Cheers,

- Chris

backupworld.update.20130406.patch Magnifier (12.3 KB) thomas.nikolajsen, 04/06/2013 03:32 PM

0001-Makefile.inc-Rework-automatic-backup-bug-2527.patch Magnifier - Tweaked TN patch (12.5 KB) marino, 04/16/2013 01:06 AM

History

#1 Updated by marino over 1 year ago

The "off" switch used to be there, but it was removed. It can be brought back.
The copy is made with "-pRP". It looks like the first switch is responsible for the flags that must be changed before deletion. The issue is that these flags need to be preserved in case the "restore world" option is used. I'm not sure what to do other than tell people that use NFS that they must turn off backups.

There's also a problem with those that build as non-root and install as root. The next build fails because the non-root user can't remove the root-owned timestamp file. I want to solve this by making this relatively unimportant file use 777 permission so any user can delete it. Matt didn't like this idea but personally I see nothing wrong with it.

#2 Updated by thomas.nikolajsen over 1 year ago

This issue should be solved before 3.4 release,
as installworld currently fails when using NFS MAKEOBJDIRPREFIX (e.g. /usr/obj).

At least backupworld-auto should be disabled by default.

Patch for updating backupworld build(7) feature attached;
it also describes & solves a few more problems w/ current backupworld.

-thomas

#3 Updated by marino over 1 year ago

The original incarnation of this is similar to your patch, but dillon asked for it to be reworked and simplified. Now it's blown up in complexity again.

On the "makefile" patch, IIRC, you're only supposed to document targets that you can actually run. You can't "make backupworld-auto-clean" for instance, so it's would only appear on Makefile.inc

On quick glance, it seems that by removing the timestamp to solve one problem, you reintroduced another problem: The auto-backup (if enabled) will occur every time you "make installworld". In the case where you buildworld once and "make installworld" many times, this is undesired.

The use of a tarball is probably an improvement especially if it allows backup of make, cp, etc, and I suspect it's a key in dealing with the chflags/nfs issue.

I would disagree (also based on Dillon's direction) that the AUTO_BACKUP is off by default.
I'm also not sure about the redefinition of the AUTO_BACKUP default. That seems to favor an NFS implementation. Wouldn't the standard case be a single machine with local backups? In that case, the default is a bit obnoxious. Not something to fall on a sword for..

I think it's a big step in the right direction.

#4 Updated by c.turner1 over 1 year ago

On 04/08/13 11:52, John Marino via Redmine wrote:

> I would disagree (also based on Dillon's direction) that the AUTO_BACKUP is off by default.

as long as it can be disabled is fine IMHO

> I'm also not sure about the redefinition of the AUTO_BACKUP default. That seems to favor an NFS implementation. Wouldn't the standard case be a single machine with local backups? In that case, the default is a bit obnoxious. Not something to fall on a sword for..

Not sure how you mean 'favoring an NFS implementation'

#5 Updated by marino over 1 year ago

A local backup wouldn't need several levels of directories filled with "hostname" and "arch" since neither will ever change. It implies that it would be backed up in a common area with multiple hostnames and archs. (and do we need to protect for a case where the there are two arches with the same hostname?). Seems like overkill for the default.

#6 Updated by marino over 1 year ago

Well, I guess hostname could change, but do we want to lose the backup when this happens?

#7 Updated by thomas.nikolajsen over 1 year ago

marino wrote:
>The original incarnation of this is similar to your patch, but dillon asked for it to be reworked
>and simplified. Now it's blown up in complexity again.
I don't agree; supplied patch simplifies (& fixes bugs): fewer lines with simple logic & better doc.

>On the "makefile" patch, IIRC, you're only supposed to document targets that you can actually run.
>You can't "make backupworld-auto-clean" for instance, so it's would only appear on Makefile.inc
The backupworld-auto-clean* targets are meant to be manual only (GC).

>On quick glance, it seems that by removing the timestamp to solve one problem, you reintroduced >another problem: The auto-backup (if enabled) will occur every time you "make installworld".
>In the case where you buildworld once and "make installworld" many times, this is undesired.
It is not perfect, but your solution was worse & doc/commit message didn't indicate what purpose was;
we need to find a better way if it is important.

>I would disagree (also based on Dillon's direction) that the AUTO_BACKUP is off by default.
>I'm also not sure about the redefinition of the AUTO_BACKUP default. That seems to favor an NFS
>implementation. Wouldn't the standard case be a single machine with local backups? In that case,
>the default is a bit obnoxious. Not something to fall on a sword for..
There is no standard case, we need to make it work for all supported setups,
especially if auto-backup will be on by default.

>I think it's a big step in the right direction.
Thanks

>A local backup wouldn't need several levels of directories filled with "hostname" and "arch"
>since neither will ever change. It implies that it would be backed up in a common area with
>multiple hostnames and archs.
>(and do we need to protect for a case where the there are two arches with the same hostname?).
>Seems like overkill for the default.

We need a simple scheme which works in all supported setups: NFS or local MAKEOBJDIRPREFIX;
there is no mechanism to change auto-backup dir, so we don't have a default.

IMO the scheme I suggest in patch matches what we need,
but if you have other suggestion, please show it.

We could introduce mechanism to change auto-backup dir, if it is needed.

Also, even for local MAKEOBJDIRPREFIX we do need TARGET_ARCH to influence auto-backup dir,
as we support cross builds.

Hostname isn't a perfect match, for what we need, but it is close, as we have no system UUID.
If hostname is changed, user can easily find backup to restore from: it is all documented.

We could turn on auto-backup on by default, when we have a worked out implementation;
I'm not sure we will get there for the release.

My intention is to commit something like the supplied patch before release,
if we don't agree on other fix; alternatively we could backout backupworld for now &
re-introduce a reworked version at a later time.

-thomas

#8 Updated by marino over 1 year ago

We will do something before release, don't worry about that.

I don't know what you mean by, there is no mechanism to change auto-backup dir, so we don't have a default.". That is simply not true IMO. The operator "?=" means define it as this *if* it's not already defined. You can predefine auto-backup dir in make.conf. So those that use NFS could define it anyway they think works. We don't need to protect for lowest common denominator, it should be defaulted to the most common case.

Regardless if the current implementation isn't the best, you've introduced a regression. In the end, we may accept that based on the full pros and cons but in general regressions should be avoided. In the other hand, I haven't figured out how to make it better yet either.

The autobackup has basically been working in most cases, it's a couple of edge cases that's the problem (NFS + build as non-root). The later can be "fixed" with your regression and the former is fixed with your patch. Since that covers the known problems, I see no problem with the release.

#9 Updated by tuxillo over 1 year ago

Hi,

The comment about the NFS users "those that use NFS could define it anyway they think works" seems a way of avoiding or bypassing the problem. In my opinion you cannot propose different setup just to leave an issue unhandled.

I also think this should be "off" by default with an "on" switch.

Best regards,
Antonio Huete

#10 Updated by c.turner1 over 1 year ago

On 04/12/13 06:28, John Marino via Redmine wrote:

> I don't know what you mean by, there is no mechanism to change auto-backup dir, so we don't have a default.". That is simply not true IMO. The operator "?=" means define it as this *if* it's not already defined. You can predefine auto-backup dir in make.conf. So those that use NFS could define it anyway they think works. We don't need to protect for lowest common denominator, it should be defaulted to the most common case.

Agreed that using '?=' provides a mechanism for cfg overrides -
however, in the current patch, auto_backup is '=''ed and not '?='d..
so maybe this is the issue?

To note - my personal typical NFS usage is:

- run a build on machine1
- if the build works, install to another 'second-tier'
machine (machine2)
- reboot machine2 and 'burn in test' or resolve any upgrade issues
- if machine2 works, then upgrade any other second-tier machines
and finally machine1

If I'm heavily tracking tip or doing local dev, the 'machine1 build
/ machine2 install' cycle might occur multiple times before installing
to the other machines, however, each individual machine1-machine2
cycle in effect performs a mini-verification of the changes between
builds, so 'fast forwarding' the other machines after a few builds
without having the backup is not very risky.

So really, for most cases, I'd only need the backup on machine2,
since it is the 'build test' machine, and wouldn't need host specific
ones - This is somewhat symmetric with a 'single obj' tree shared
over NFS - e.g. I don't build N times for N machines, but 1 time
for N machines - similarly, I only need 1 backup for N machines.

But, if this is configurable, defaulting to multiple ${HOSTNAME}
backups is not really an issue since I manage /etc/make.conf on all
nodes, and might be handy for other use cases e.g. where you have
various 'sets' of machines hooked into a build node upgraded at
different times and want to keep emergency backups during upgrades.

In my scenario, I'd probably only have the auto-backup setup on
'machine2', with the rest configured as 'off'.

Put another way, using the 'NFS centric' hostname directory path
is not necessary in my particular local NFS use case, and if this
setting is configurable, could always be configured by a user
to support multiple backups, thereby making it 'not NFS centric'
by Marinos definition of 'NFS centric' and my NFS use case :D

Anyhow, some thoughts.

Cheers,

- Chris

#11 Updated by marino over 1 year ago

Yeah, given NFS scenario, AUTO_BACKUP should be redefinable. It was defined as "=" before when it was assumed to be a local folder.

Tuxillo, the default "works". There's a 100 ways to set up an NFS mount, nobody can guess them all. The point is if somebody's particular setup works better with a different definition, they can set it.

Having an auto backup off when you don't even know it exists pretty much defeats the point. By turning it off, you clearly know it exists and have made an informed decision. It's erroring on the side of safety, aka it is the most conservative.

#12 Updated by marino over 1 year ago

I have *slightly* reworked Thomas' patch (attached). I fixed a couple of errors, I brought back "NO_BACKUP" which inhibits the automatic backup. The man pages were changed accordingly as well as edited.

I left "the regression" intact. Those that install multiple times just need to use "make NO_BACKUP=yes installworld" to avoid unwanted automatic backups. Obviously NO_BACKUP can be put in make.conf to avoid it permanently.

The AUTO_BACKUP was put back similar to how it was before (adds $DESTDIR), but now it can be overridden as desired. This simplifies it a bit. I believe this works in all cases and allowing overrides means it can be tailored even future for things like NFS mounts.

The main functional changes were left untouched. I will try to do some basic testing and commit later today. Any comments (read: major complaints) about this new patch proposal therefore need to come ASAP.

#13 Updated by c.turner1 over 1 year ago

On 04/16/13 03:06, John Marino via Redmine wrote:
> I have *slightly* reworked Thomas' patch (attached).

Works (in theory) for me - +1

Cheers,

- Chris

#14 Updated by marino over 1 year ago

  • Status changed from In Progress to Closed

Also available in: Atom PDF