fail... rhevm-tools-3.5.4-1.1.el6ev.noarch

# engine-backup --mode=restore --file=backup --log=restore.log --db-host=jb-test.rhev.lab.eng.brq.redhat.com --db-name=engine --db-user=engine --db-password=SECRET --dwh-db-host=jb-test.rhev.lab.eng.brq.redhat.com --dwh-db-name=ovirt_engine_history --dwh-db-user=engine_history --dwh-db-password=SECRET --reports-db-host=jb-test.rhev.lab.eng.brq.redhat.com --reports-db-name=engine_reports --reports-db-user=engine_reports --reports-db-password=SECRET

...
ALTER TABLE
WARNING:  no privileges could be revoked for "public"
REVOKE
WARNING:  no privileges could be revoked for "public"
REVOKE
WARNING:  no privileges were granted for "public"
GRANT
WARNING:  no privileges were granted for "public"
GRANT
ERROR:  role "rouser" does not exist
REVOKE
REVOKE
GRANT
ERROR:  role "rouser" does not exist
...

[root@jb-test ~]# sed -n '534,+4p' `which engine-backup`
                -U "${user}" \
                -h "${host}" \
                -p "${port}" \
                --no-owner \
                --no-privileges \
[root@jb-test ~]# sed -n '578,+4p' /usr/share/ovirt-engine/setup/ovirt_engine_setup/engine_common/database.py
                '-p', str(self.environment[self._dbenvkeys['port']]),
                '-U', self.environment[self._dbenvkeys['user']],
                '-d', self.environment[self._dbenvkeys['database']],
                '--no-owner',
                '--no-privileges',

Created attachment 1051874 [details]
restore.log

(In reply to Jiri Belka from comment #1)
> fail... rhevm-tools-3.5.4-1.1.el6ev.noarch

Was backup taken with this version too?

If yes, please attach it. Otherwise, please re-check with a backup taken with it.

The following flow works:

- 3.3 -> 3.4 -> 3.5.3-1 -> vt16.2
  > do backup (engine-backup contains the diffs)
- install clean vt16.2
  > do restore

Although following flow does not work:

- 3.3 -> 3.4 -> 3.5.3-1
  > do backup
- install vt16.1/vt16.2 (both have diffs)
  > do restore

Thus it's not possible to do restore inside minor versions, ie. 3.5.3-1 -> 3.5.4-1.1.

Please decide if this is acceptable, thx.

Workaround:

Before trying again to restore, create in the database the missing users.

If they are not needed, they can be removed after restore.

(In reply to Jiri Belka from comment #4)
> The following flow works:
> 
> - 3.3 -> 3.4 -> 3.5.3-1 -> vt16.2
>   > do backup (engine-backup contains the diffs)
> - install clean vt16.2
>   > do restore
> 
> Although following flow does not work:
> 
> - 3.3 -> 3.4 -> 3.5.3-1
>   > do backup
> - install vt16.1/vt16.2 (both have diffs)
>   > do restore
> 
> Thus it's not possible to do restore inside minor versions, ie. 3.5.3-1 ->
> 3.5.4-1.1.
> 
> Please decide if this is acceptable, thx.

If we want to fix this too, we'll have to patch the dumped sql file and remove relevant stuff from it (grant commands etc). I think this is error prone and recommend that users that need to restore backups from a previous version use the above workaround.

(In reply to Yedidyah Bar David from comment #5)


> If we want to fix this too, we'll have to patch the dumped sql file and
> remove relevant stuff from it (grant commands etc). I think this is error
> prone and recommend that users that need to restore backups from a previous
> version use the above workaround.

+1

We need to let the user know we are dropping the users.
Even better would be to provide a list of user we are about to drop.
We need to give the user a option to no drop users if he created them in advance.

(In reply to Yaniv Dary from comment #7)
> We need to let the user know we are dropping the users.
> Even better would be to provide a list of user we are about to drop.
> We need to give the user a option to no drop users if he created them in
> advance.

You want to revert current change? Keep it but add options etc for 3.5.5 (should probably be in another bug)?

(In reply to Yedidyah Bar David from comment #8)
> (In reply to Yaniv Dary from comment #7)
> > We need to let the user know we are dropping the users.
> > Even better would be to provide a list of user we are about to drop.
> > We need to give the user a option to no drop users if he created them in
> > advance.
> 
> You want to revert current change? Keep it but add options etc for 3.5.5
> (should probably be in another bug)?

Keep the change and improve based on it. We have a note in for it anyhow.

Writing down some comments, since I will likely not finish handling this prior to going out for a vacation.

1. I do not think we should check and report about extra permissions. As discussed above, this is a bit hacky. If we do want to do that, we can do something like:

To get a list of extra users that got granted:

pg_dump -s "${DWH_DB_USER}" | sed -n 's/^GRANT .* TO \(.*\);$/\1/p' | grep -E -v "^${DWH_DB_USER}\$|^postgres$|^PUBLIC$" | sort -u

If it's empty, nothing special needs to be done.

2. We need to decide about our default, which depends on whether we decide to check (1.) above or not. Basically we have two options:

2.1. Keep 3.5.4 behavior as default - just don't dump permissions, and add an option to override this, say '--keep-permissions'.

If we do this and decide to do (1.), we can of course warn or abort if we find any, suggesting to use this option.

2.2. Revert 3.5.4 behavior - by default dump permissions, unless a new option passed '--no-permissions'.

Here, too, we can warn accordingly - if using default and no '-no-permission', tell the user that on restore he'll need to first create these accounts (from output of (1.)).

3. Note that the fix will be different between 3.5 and 3.6. In 3.6 we added an option (which is the default) to use the 'custom' dump format, which is restored by pg_restore (and not psql). With this format, these options are not passed during dump, but during restore. This also means that a user does not have to choose beforehand.

Yaniv, according to comment #10, how should we proceed?

2.2. looks the best, do not drop anything unless the user uses the flag '-no-permissions'. Restore should fail nicely on issues and ask user to use flag if he doesn't want to restore users.

(In reply to Yaniv Dary from comment #12)
> 2.2. looks the best, do not drop anything unless the user uses the flag
> '-no-permissions'. Restore should fail nicely on issues and ask user to use
> flag if he doesn't want to restore users.

I didn't say such an option exist, and I am against adding it. Let me repeat my "2.2.":

In 3.5, and in 3.6 if using the plain format:

On backup: We check with (1.) if there are extra grants and warn accordingly, unless passed '-no-permissions'.

On restore: No special treatment. That is, if user didn't pass '-no-permissions' on backup, and had extra grants, restore will fail (not "nicely").

In principle we can do even more complex things, such as check on restore if the extra users exist, and if not fail nicely. I'd say this is probably crossing the line and becoming too complex (imho). Something in the middle is to check if extra grants were dumped (but not if they are in the db) and fail (nicely), unless passed something like '--force' or whatever.

In 3.6 with the custom format:

On backup: no special treatment

On restore: Like 3.5 above, but all checks etc are done on restore. That is, the user does not have to decide during backup whether to dump extra permissions or not.

(In reply to Yedidyah Bar David from comment #13)
> (In reply to Yaniv Dary from comment #12)
> > 2.2. looks the best, do not drop anything unless the user uses the flag
> > '-no-permissions'. Restore should fail nicely on issues and ask user to use
> > flag if he doesn't want to restore users.
> 
> I didn't say such an option exist, and I am against adding it. Let me repeat
> my "2.2.":
> 
> In 3.5, and in 3.6 if using the plain format:
> 
> On backup: We check with (1.) if there are extra grants and warn
> accordingly, unless passed '-no-permissions'.
> 
> On restore: No special treatment. That is, if user didn't pass
> '-no-permissions' on backup, and had extra grants, restore will fail (not
> "nicely").
> 
> In principle we can do even more complex things, such as check on restore if
> the extra users exist, and if not fail nicely. I'd say this is probably
> crossing the line and becoming too complex (imho). Something in the middle
> is to check if extra grants were dumped (but not if they are in the db) and
> fail (nicely), unless passed something like '--force' or whatever.

The above sounds good, but I would use '--with-permissions' not '--force'.

> 
> In 3.6 with the custom format:
> 
> On backup: no special treatment
> 
> On restore: Like 3.5 above, but all checks etc are done on restore. That is,
> the user does not have to decide during backup whether to dump extra
> permissions or not.

Same as above should apply. Use '--with-permissions' to get the extra permissions.

Target release should be placed once a package build is known to fix a issue. Since this bug is not modified, the target version has been reset. Please use target milestone to plan a fix for a oVirt release.

Target release should be placed once a package build is known to fix a issue. Since this bug is not modified, the target version has been reset. Please use target milestone to plan a fix for a oVirt release.

(In reply to Yaniv Dary from comment #14)
> (In reply to Yedidyah Bar David from comment #13)
> > (In reply to Yaniv Dary from comment #12)
> > > 2.2. looks the best, do not drop anything unless the user uses the flag
> > > '-no-permissions'. Restore should fail nicely on issues and ask user to use
> > > flag if he doesn't want to restore users.
> > 
> > I didn't say such an option exist, and I am against adding it. Let me repeat
> > my "2.2.":
> > 
> > In 3.5, and in 3.6 if using the plain format:
> > 
> > On backup: We check with (1.) if there are extra grants and warn
> > accordingly, unless passed '-no-permissions'.
> > 
> > On restore: No special treatment. That is, if user didn't pass
> > '-no-permissions' on backup, and had extra grants, restore will fail (not
> > "nicely").
> > 
> > In principle we can do even more complex things, such as check on restore if
> > the extra users exist, and if not fail nicely. I'd say this is probably
> > crossing the line and becoming too complex (imho). Something in the middle
> > is to check if extra grants were dumped (but not if they are in the db) and
> > fail (nicely), unless passed something like '--force' or whatever.
> 
> The above sounds good, but I would use '--with-permissions' not '--force'.

But that's the point! With '--with-permissions' you imply there is also
'--without-permissions'. But during restore time, you can't choose that
(without messing with the dump file). '--force' implies just that: You asked
to dump with permissions (or this was the default, we still have to decide),
and now, if you want to restore, you have to make sure you created these users
beforehand, then force engine-backup to try to restore even though it's "risky"
(meaning, will fail if some users are missing, or you do not have permissions
for that).

We can add '--with-permissions' during *backup* time. Please see
bug 1212752 comment 5 and all of it for the discussion we already had.

I don't mind trying to restore with such extra grants and see if the
default permissions are enough for that. I didn't try that yet, IIRC.
If they are not, do we just give up? We can obviously spend more time
on whether we'll tell the user (--help? Doc?) how to give it also
permissions for processing these grant statements.

I still think that's out-of-scope for this tool. Please see also the current
doc text of current bug. I now checked and see that in 3.5 we also have
such content [1]. Perhaps we should ask the doc team to update that part,
and/or the backup/restore guide [2].

[1] https://access.redhat.com/documentation/en-US/Red_Hat_Enterprise_Virtualization/3.5/html/Administration_Guide/sect-History_Database.html#Allowing_Read_Only_Access_to_the_History_Database
[2] https://access.redhat.com/documentation/en-US/Red_Hat_Enterprise_Virtualization/3.5/html/Administration_Guide/chap-Backups.html#sect-Backing_Up_and_Restoring_the_Red_Hat_Enterprise_Virtualization_Manager

> 
> > 
> > In 3.6 with the custom format:
> > 
> > On backup: no special treatment
> > 
> > On restore: Like 3.5 above, but all checks etc are done on restore. That is,
> > the user does not have to decide during backup whether to dump extra
> > permissions or not.
> 
> Same as above should apply. Use '--with-permissions' to get the extra
> permissions.

Here, with the custom format, we can do that. But not with the plain format,
which was the only one in 3.5. But see above - I think this will fail anyway,
didn't try yet.

Due to the release of recent 3.6.0 major release of the oVirt project, and upcoming release of its first maintenance release 3.6.1, the last 3.5.z release will be 3.5.6.
We recommend anyone that is need for a fix in 3.5.z to upgrade to 3.6 to get all new fixes

Re-targeting to 3.6.2 not being a blocker for 3.6.1

Pushed 49584 following a private discussion with Yaniv. Copying current commit message:
=========================================================================
If backing up with the custom dump format (which is the default), do
backup with owner and privileges, thus partially reverting I78fa18fe0d.
During restore, the user must pass one of '--restore-permissions' and
'--no-restore-permissions'.

With plain dump format, permissions are not backed up nor restored.
=========================================================================

If we decide to add such a mandatory option during restore of custom dump,
might as well add a companion '--backup-permissions' or '--no-backup-permissions'
during backup with plain dump format.

If we do that, and even if we don't, it might make sense to allow passing
a different one for each db - e.g. users might want to backup engine with
plain dump (which might be perceived as having an advantage since it's
plain text) and dwh with custom format (which is faster to restore, thus
more important for dwh which is potentially huge).

Had another thought - it might be not that difficult to add an option to
read a custom format and restore only the schema ('-s'), and emit a warning
when using '--no-restore-permissions'. This will allow to default to
'--no-restore-permissions', and users that do that and get the warning will
be able to run again with the option to restore just the schema (which will
be quick). Didn't test this, though.

Gave up for now on restoring only extra grants, couldn't find a simple way to do that with pg_restore.

Current patch is basically as previous comment 19, and also changes engine-setup accordingly (backup permissions, restore them on rollback). Also made engine-backup output a note about not backing up permissions when using plain dump format.

ok, rhevm-tools-3.6.3-0.1.el6.noarch

- without read-only user, restore with '--restore-permissions' does not work
- with read-only user, restore with '--restore-permissions' does work
- --no-restore-permissions works ok

Now noticed that doc text was updated (nor published?). Copying current (outdated) doc text here, and changing it (not updating Cause and Consequence).

Old (pre- comment 19) doc text:

Cause: 

engine-backup used pg_dump's default, which is to dump (also) ownership of objects and grants on them to other users.

Consequence: 

If the dump included grants to users that existed on the backed up database, but do not exist on the database we restore to, restore will fail.

Fix: 

engine-backup now does not backup or (try to) restore any ownership/grants, so only the defaults apply (each db user owns the objects in its schema, and no other user has access to them).

Result: 

restore now succeeds even if the database that was backed up had extra users with grants on one or more of the applications' objects. These are not restored, and the user should manually reconfigure these if relevant.

Note to doc team:

In 3.3 (only), dwh setup asked whether to create a read-only db user allowing remote access to the dwh database.

In 3.4 and later, we did not have similar functionality.

In 3.2, we also didn't have it, but we did have instructions [1] about how to do that manually. I now see that we also have them in 3.5 [2].

We might want to write something similar for 3.4+, perhaps as a KB article.

Note that 3.4 requires a bit more manual configuration than 3.5 - 3.5 configures postgresql.conf 'listen_address=*', which 3.4 does not.

In any case, as explained above, such users, whether created manually or by 3.3's dwh-setup (which was later upgraded), are not backed up or restored by engine-backup.

[1] https://access.redhat.com/documentation/en-US/Red_Hat_Enterprise_Virtualization/3.2/html/Administration_Guide/Allowing_Read_Only_Access_to_the_History_Database.html

[2] https://access.redhat.com/documentation/en-US/Red_Hat_Enterprise_Virtualization/3.5/html/Administration_Guide/sect-History_Database.html#Allowing_Read_Only_Access_to_the_History_Database