Made fix 1 below and pushed 315-318 to master. After much discussion with Endi, figured out how best to refactor 319. Will resubmit as 320. Thanks, Ade On Thu, 2016-06-02 at 14:10 -0500, Endi Sukma Dewata wrote:
On 6/2/2016 8:51 AM, Ade Lee wrote:
And now with the patches ..
On Thu, 2016-06-02 at 09:50 -0400, Ade Lee wrote:
Patch descriptions (in reverse order).
The final patch will need some discussion. Please review,
Ade
Some comments:
1. In SrchKey and SrchKeyForRecovery the check probably should have been like this (no closing paren):
if (filter.contains("(realm=")) {
2. If a realm is specified, I suppose it won't match any of the VLV indexes. Do the queries still work correctly without a matching VLV?
3. Could you document how to run the upgrade command in this page? http://pki.fedoraproject.org/wiki/Database_Upgrade_for_PKI_10.3.x
Note that there are still some manual upgrade procedures that also need to be executed as documented in that page.
4. In PKISubsystem.open_database() can we use bind_dn and bind_password parameter names instead of user and password (it's a DN not a user ID)? Also in DBUpgrade should we use -D, -w, and -Z for consistency with DS tools?
5. The gnu_getopt() is called with "server_id=". I think it should be "server-id=". But since it actually contains the DS instance name maybe we should use "ds-instance=" instead?
6. In the DBUpgrade.modify_kra_vlv() the os.unlink(ldif_file) probably should be moved into the finally clause above it to ensure the temporary file is deleted in all cases. Alternatively the code could be written like this:
with NamedTemporaryFile() as ldif_file: # do everything with the ldif_file here
7. In DBUpgrade.modify_schema() why not use subsystem.open_database() instead of ldapmodify? That will ensure it uses the same mechanism to connect to the LDAP server.
8. Also the DBUpgrade.modify_schema() is ignoring all schema update failures. I think we can only ignore the case where the new schema is already added. In other cases the upgrade should fail since the subsequent upgrades may depend on it.
9. I think DBUpgrade.create_realm_index() should only execute if there's a KRA subsystem in the instance. The code right now creates the index in the first subsystem's database which may not be a KRA.
subsystem = instance.subsystems[0]
10. The DBUpgrade uses db2index.pl to rebuild the indexes which means the LDAP server must run locally. To support remote LDAP server I think we need to use reindex task. We have indextasks.ldif and vlvtasks.ldif that can be used for this. See also: http://pki.fedoraproject.org/wiki/DS_Database_Indexes
11. The DBUpgrade should not ignore reindex failures since subsequent upgrades may depend on it.
12. There's a typo in the last patch's commit description.