attachment
0001-Ticket-48402-allow-plugins-to-detect-a-restore-or-im.patch

Hi Ludwig,

bak2db could take "-n backend" option. Do you think it matters?
$ bak2db backupDirectory -n backend

If it does not, your implementation looks good to me.

Replying to [comment:3 nhosoi]:

Hi Ludwig,

bak2db could take "-n backend" option. Do you think it matters?
$ bak2db backupDirectory -n backend

Interesting point, it is not handled by my patch and running it gave some unexpected log messages regarding the restore file.

I am not sure how important this option is, in fact I consider it dangerous. When only one backend is restored the result can be an inconsistent environment:
- if the retro cl is enabled and you restore useroot, the RC can contain changes which are not in the database
- the txn logs are taken from the backup, other backends might contain LSNs which are newer than in the txn log, and for me the consequences are not predictable

Hi,

I don't see anywhere where we remove the .restore or .import file. Shouldn't we remove these once the plugins have completed their actions based on SLAPI_BE_FLAG_POST_RESTORE or SLAPI_BE_FLAG_POST_IMPORT? IE after all plugins have finished starting, and we are about to process requests.

This would mean with the current patch, once we do an import or restore, every startup after the fact acts like it's been imported / restored.

You also write status strings to the .restore / .import file (which I really like, it gives us some debug tracing)

However, you have in ldbm_back_ldif2ldbm( Slapi_PBlock *pb ) the condition, we can't write the import file.

IMO this should be cause to terminate the import, and raise big errors. If we can't flag to the DB it's been imported, we need to not do the import.

With dblayer_file_check we can tell if a backend has failed to import or restore, yet we don't really act on this in the DS. Shouldn't we raise issues to the admin in the case we detect this?

Hope that this helps.

The import or restore files are deleted in dblayer_check_file() by calling PR_Delete(fname)

If the restore file cannot be written the restore is aborted:

{{{

make .restore not accessable by server user:

chown root:root /var/lib/dirsrv/slapd-lucy1/.restore

try restore:

bak2db /var/lib/dirsrv/slapd-lucy1/bak/lucy1-2016_02_03_15_04_20 -Z lucy1

[08/Feb/2016:15:48:45 +0100] - Failed to open file: /var/lib/dirsrv/slapd-lucy1/db/../.restore, error: -5966
[08/Feb/2016:15:48:45 +0100] - archive2db: failed to write restore file.

}}}

for the import file I will try to do the same, thanks for catching this

Replying to [comment:4 lkrispen]:

Interesting point, it is not handled by my patch and running it gave some unexpected log messages regarding the restore file.

I am not sure how important this option is, in fact I consider it dangerous. When only one backend is restored the result can be an inconsistent environment:
- if the retro cl is enabled and you restore useroot, the RC can contain changes which are not in the database
- the txn logs are taken from the backup, other backends might contain LSNs which are newer than in the txn log, and for me the consequences are not predictable

I'd agree it is not a popular feature... It was introduced to skip replica initialization when a replica gets broken and the database is huge. We don't hear the type of requirements much these days.

Since it has a possibility to turn the system to the inconsistent status, we rather should not encourage it? Maybe, we could doc it as deprecated as well as issue a warning when it's used?

attachment
0001-Ticket-48402-v2-allow-plugins-to-detect-a-restore-or.patch

Looks good.

It might not be necessary, but it'd be safer to unset the 2 bits in ldbm_instance_stopall if any?

define SLAPI_BE_FLAG_POST_IMPORT 0x100 / backend was imported /

define SLAPI_BE_FLAG_POST_RESTORE 0x200 / startup after restore /

Or they are supposed to be unset by someone else?

Hmm, I don't see ldbm_instance_stopall() getting called.

But the question lets me think if there is a need to also set the flag in backend state changes, eg after online import or total init.
Plugins can register for notifications of be state changes, but cannot really determine what was going on in between

Replying to [comment:10 lkrispen]:

Hmm, I don't see ldbm_instance_stopall() getting called.

Wow... I didn't know that... :)

But the question lets me think if there is a need to also set the flag in backend state changes, eg after online import or total init.
Plugins can register for notifications of be state changes, but cannot really determine what was going on in between

"in between" means between the moment restore/import is done and the moment the bits are set in ldbm_instance_startall? You are thinking to close the gap? (In advance, sorry if I might have misunderstood completely... :p)

sorry if I was unclear. With "in between" I mean the following:

a plugin can register for backend state change, it will receive a notification that a backend is going offline or that a backend is coming online, but it doesn't know why the backend was offline, what happend between going offline and coming online.

The import/restore files are written by offline import/restore.
For online import the backend goes offline/online, and I think import is the only one triggerring this, but setting the flag could make it more clear

attachment
0001-Ticket-48402-v3-allow-plugins-to-detect-a-restore-or.patch

Sorry, could you please tell me (probably once more) how the new flags SLAPI_BE_FLAG_POST_ are being used? I see the bits are set, but do not see check them (slapi_be_is_flag_set) or unset them (slapi_be_unset_flag)... Or are they for the future use?
{{{
$ egrep SLAPI_BE_FLAG_POST_ 0001-Ticket-48402-v3-allow-plugins-to-detect-a-restore-or.patch
+ slapi_be_set_flag(inst->inst_be, SLAPI_BE_FLAG_POST_RESTORE);
+ slapi_be_set_flag(inst->inst_be, SLAPI_BE_FLAG_POST_IMPORT);
+ slapi_be_set_flag(inst->inst_be, SLAPI_BE_FLAG_POST_IMPORT);
+ slapi_be_set_flag(be, SLAPI_BE_FLAG_POST_IMPORT);
+#define SLAPI_BE_FLAG_POST_IMPORT 0x100 / backend was imported /
+#define SLAPI_BE_FLAG_POST_RESTORE 0x200 / startup after restore */
}}}
Thanks!

This change is to enable plugins to check if the database is reloaded, it is eg required for ticket 48380 where sync repl needs to invalidate cookies if the database is reloaded.
So there is no plugin using it yet, but sync repl will.
The flags are reset at restart, they just indicate that the database was reimported/restored before startup or an online import had happened.

Thanks for the details, Ludwig!

Hi Ludwig,

after 900c32f I can't install instance using setup-ds.pl (I'm using build from master):

{{{
...
Directory Manager DN [cn=Directory Manager]:
Password:
Password (confirm):
Could not import LDIF file '/tmp/ldifl6QWVe.ldif'. Error: 65280. Output: importing data ...
[25/Aug/2016:04:01:51.670383593 -0400] ldbm_back_ldif2ldbm: failed to write import file

Error: Could not create directory server instance 'qeos-14'.
Exiting . . .
Log file is '/tmp/setupoGOdHs.log'
}}}

The failure during install is that in the database entry the path to the database is not set.
The attached patch enhances the template.

attachment
0001-provide-backend-dir-in-suffix-template.patch

ack

Note: upstream only (1.3.5 and newer) at this time -- 2016/9/6.

Closing this ticket with milestone 1.3.5.14.

Commit of enhancement:

commit 900c32f

Fix of regression:

commit 2fb3c27

389-ds-base is moving from Pagure to Github. This means that new issues and pull requests
will be accepted only in 389-ds-base's github repository.

This issue has been cloned to Github and is available here:
- https://github.com/389ds/389-ds-base/issues/1733

If you want to receive further updates on the issue, please navigate to the github issue
and click on subscribe button.

Thank you for understanding. We apologize for all inconvenience.