Bug 1286704

+++ This bug was initially created as a clone of Bug #1220791 +++

+++ This bug is a RHEV-M zstream clone. The original bug is: +++
+++   https://bugzilla.redhat.com/show_bug.cgi?id=1217402. +++
+++ Requested by "sbonazzo" +++
======================================================================



----------------------------------------------------------------------
Following comment by jbelka on April 30 at 09:57:47, 2015

Created attachment 1020521 [details]
logs

Description of problem:

unable to restore if backup contains read only user for DWH DB access, found hardly while restoring our long running brq-setup (existing since 3.0).

# engine-backup --mode=restore --file=backup --log=restore.log
Preparing to restore:
- Unpacking file 'backup'
Restoring:
- Files
- Engine database 'engine'
- DWH database 'ovirt_engine_history'
FATAL: Errors while restoring database ovirt_engine_history

...
ERROR:  role "readonlyuser" does not exist
REVOKE
REVOKE
GRANT
ERROR:  role "readonlyuser" does not exist
2015-04-30 11:46:56 924: FATAL: Errors while restoring database ovirt_engine_history

Version-Release number of selected component (if applicable):
3.5.1

How reproducible:
100%

Steps to Reproduce:
1. install 3.3 rhevm, rhevm-dwh, rhevm-reports, create read-only user for dwh db access
2. upgrade env to 3.4 -> 3.5.1
3. engine-backup --mode=backup ... (save for latest somewhere)
4. clean os, *install only* 3.5.1 rhevm, rhevm-dwh, rhevm-reports
5. get the backup, engine-backup --mode=restore ...

Actual results:
restore fails because dwh ro db user does not exist

Expected results:
should work or at least it should do some pre-check for all prereqs existing

Additional info:

----------------------------------------------------------------------
Following comment by jbelka on April 30 at 10:14:41, 2015

fyi https://bugzilla.redhat.com/show_bug.cgi?id=1092388

----------------------------------------------------------------------
Following comment by didi on May 03 at 05:57:45, 2015

Already fixed in 3.6 by the fix for bug 1212752.

The fix was trivial - just passing '--no-owner'. We might want to note somewhere, though, that with this, restore will not anymore (try to, and fail to) restore other users and grants. Thus, if you have in the db more than the minimum set of users/grants created by engine-setup, it's up to you to backup/restore these if needed.

----------------------------------------------------------------------
Following comment by didi on May 03 at 05:59:48, 2015

Moving back to NEW for consideration whether we want this in 3.5.

----------------------------------------------------------------------
Following comment by didi on May 12 at 13:07:01, 2015

Moving to modified, as it's already fixed in 3.6, see comment 2.

--- Additional comment from Jiri Belka on 2015-07-14 10:44:21 EDT ---

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',

--- Additional comment from Jiri Belka on 2015-07-14 10:45:00 EDT ---



--- Additional comment from Yedidyah Bar David on 2015-07-14 12:53:25 EDT ---

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

--- Additional comment from Jiri Belka on 2015-07-15 09:55:17 EDT ---

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.

--- Additional comment from Yedidyah Bar David on 2015-07-15 10:05:49 EDT ---

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.

--- Additional comment from Eli Mesika on 2015-07-15 10:11:25 EDT ---

(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

--- Additional comment from Yaniv Dary on 2015-07-21 18:35:05 EDT ---

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.

--- Additional comment from Yedidyah Bar David on 2015-07-22 02:20:53 EDT ---

(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)?

--- Additional comment from Yaniv Dary on 2015-07-23 03:47:37 EDT ---

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

--- Additional comment from Yedidyah Bar David on 2015-08-04 03:22:38 EDT ---

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.

--- Additional comment from Sandro Bonazzola on 2015-08-04 03:27:24 EDT ---

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

--- Additional comment from Yaniv Dary on 2015-08-09 11:38:51 EDT ---

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.

--- Additional comment from Yedidyah Bar David on 2015-09-06 09:29:06 EDT ---

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

--- Additional comment from Yaniv Dary on 2015-09-22 04:22:57 EDT ---

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

--- Additional comment from Red Hat Bugzilla Rules Engine on 2015-10-19 06:58:00 EDT ---

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.

--- Additional comment from Red Hat Bugzilla Rules Engine on 2015-10-26 10:34:11 EDT ---

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.

--- Additional comment from Yedidyah Bar David on 2015-11-16 10:40:33 EST ---

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