First of all, I think, we really should get away from this legacy methods/objects and we should use existing structures for Replicas, RUV, Agreements.
This method can be put to Agreement(DSLdapObject) and we can benefit from it. The object already has the binddn, bindpw, consumer instance information, etc.

In the later comments, I'll point out what can be changed with what.

If you need to get RUV and the nsds5AgmtMaxCSN, you can use Replica(DSLdapObject) methods (which uses RUV(DSLdapObject)).

replicas = Replicas(to_instance)
replica = replicas.get(suffix)
ruv = replica.get_ruv()
ruv.get_attr_val_utf8('nsds5AgmtMaxCSN')

Instead of using this old CSN object, I think, we can use RUV(DSLdapObject). It already has the method 'RUV().is_synced(other_ruv)'. So we can add the method 'get_time_lag' there.

I think we should put the method here, in Agreement(DSLdapObject) and name it something like 'get_status'.

The rest looks good.

Just to sum up my idea:
Current lib389 is spread over all other different modules. Sometimes it repeats itself in different forms. It makes any change harded. I think we should put the code in DSLdapObject modules only so it is always easy to find a tool when you need to implement something (or write a test).

For example, _replication.py looks redundant to me and I propose do not use it and put the code in the existing structures (probably, we don't even need a new class for CSN, we can use RUV(DSLdapObject) from Replica and put the methods we need there).

DirSrv().getConsumerMaxCSN and DirSrv().getReplAgmtStatus also are pretty ugly (you fixed some parts but the rest is still badly written). The functionality is already in DSLdapObject design. I did write how you can get MaxCSN and getReplAgmtStatus can be moved to Agreement(DSLdapObject) but modified of course so it uses DSLdapObjects.

Also, please, write docstrings for the not private methods you add to lib389 API. And maybe some basic CLI tests if you have something short in mind.

Okay, but we need to provide an external bind dn and password for all the status related functions We can not rely on the credentials in the instance object because if it's LDAPI, like in the UI, we can not contact a remote consumer.

Okay, good point!

But it may not be a bind either. It could be GSSAPI, TLS, or other. We shouldn't have "helper" wrappers like this because they limit us to narrow methods of operation, and are not composable.

We should be taking a DirSrv object, and calling what is needed from that. So I think we should change this function.

Isn't winsync dead?

As above - these should all be on a dsldapobject and we take a dirsrv object that is bound "however" it wants (ldap, gssapi, other).

How is this related to the ticket?

Isn't there a RUV object somewhere? We should avoid raw searches if possible ....

This is for connecting to remote replicas. We can not use what is in the existing Dirsrv object, it must be provided externally. There are not a lot of good options here.

I'm afraid winsync is very alive and still being used by a lot of customers. There are no plans to deprecate it. It definitely complicated the CLI :-(

Again, this is for connecting to remote replicas where we can not reused the existing credentials - not a lot of good options here

Just killing two birds with one stone...

The RUV class does raw searches - also the RUV object can not be a DSLdapObject, why? Because of some "magic" that renames the DN after you search for it. :-/ It's a corner case

Actually I'm not suing this function anymore. I wrote a new one inthe agreement class. This one is only needed for legacy replication. I'd actually like to remove it.

Improved the consumer dirsrv object (for getting repl agmt status) to use the secure port if the agmt is using LDAPS.

Also added options to create the replication manager entry when enabling replication for a suffix

And added the option to initialize an agreement after creating it.

Add docstrings to the new functions in the lib389 classes

I think this won't work correctly. It should be :param binddn:, not :param: binddn:. The same for :type binddn:. :returns: is right

In the replication.py, if we specify --bind-dn as a name 'repl_manager', it will try to put it to the replica entry nsDS5ReplicaBindDN: repl_manager and it will fail.

So, I think, we should either validate the parameter, or put to the 'cn=repl_manager,cn=config' DN.
The same for agreements (the commands there also have --bind-dn parameter.

Small nitpick, but I think, worth fixing.
When we enable replication, we set --rid. And when we set some new values, we specify --replica-id. It is a bit inconsistent.

You don't have 'disable' command because of the typo:

# Disable
agmt_disable_parser = agmt_subcommands.add_parser('enable', help='Disable replication agreement')
agmt_disable_parser.set_defaults(func=disable_agmt)

The rest seems to work fine, thank you!

In the replication.py, if we specify --bind-dn as a name 'repl_manager', it will try to put it to the replica entry nsDS5ReplicaBindDN: repl_manager and it will fail.
So, I think, we should either validate the parameter, or put to the 'cn=repl_manager,cn=config' DN.

But the parameter says "bind-dn", which means you need to use a DN, not a name. This is also clearly stated in the usage. But adding a DN validator is easy

But the parameter says "bind-dn", which means you need to use a DN, not a name. This is also clearly stated in the usage. But adding a DN validator is easy

Yeah, I think too that it is the best way. I was just trying things because 'create-manager' accepts the name (cn=NAME,cn=config). So it looked a bit inconsistent to me as 'a user'.

Changes made. So I removed the option to create the repl manager when enabling replication. It is confusing, and I think it's fine to have it as an extra step(especially since it's optional)

Ithink generally the question is "do you want dsconf replication to be a recipe process" or do you want it to be "nuts and bolts bucket of parts", perhaps that's where my issues with this change are (that I have had at the back of my mind).

IMO we need to do more "recipe" not more "nuts and bolts". No one likes setting up repl by hand, even i dread it and I'm a developer of the project. How does an admin feel?

Perhaps in the future we'll add a second repl-wizard command that does things the "recipe" way?

Sure, looks good to me! You have my ack!

Ithink generally the question is "do you want dsconf replication to be a recipe process" or do you want it to be "nuts and bolts bucket of parts", perhaps that's where my issues with this change are (that I have had at the back of my mind).
IMO we need to do more "recipe" not more "nuts and bolts". No one likes setting up repl by hand, even i dread it and I'm a developer of the project. How does an admin feel?
Perhaps in the future we'll add a second repl-wizard command that does things the "recipe" way?

I'm assuming by "recipe" you mean the server just uses default values for almost everything but host/port/suffix/protocol/etc. That's easy, and I think that's what you are recommending. I can just add that to this PR, but we still need the existing fine grained control over all settings/objects. Anyway I'd rather do it all now than later :-p

No, there is actually a class in lib389 that can do all the agreements and auth config for you with per-server binds and stuff. It's here:

https://pagure.io/389-ds-base/blob/master/f/src/lib389/lib389/replica.py#_1264

Rather than you saying 'link that machine, and do this etc', you literally just go "here are two servers, make replicate" and it does. It automates cert auth, binds, replica ID creation, makes sure change logs exist, and it can also be used in existing replication topologies. I make the topology_replica use it by default a while back, but perhaps that was undone?

So I think we need both: one cli ui for the "raw" replication bits for people who want it, but then this is just like "hey this server is ROreplica now kgo" kind of thing.

Ohhh, one more thing: It also randomly generates the passwords, so you never need to disclose them to the other server, nor do you need to know the replication account pw, as each server has a replication bind account in the suffix being replicated, so it adds security as you can revoke an indivudual host from the topology.

Think the IPA topology code, but like .. better.

No, there is actually a class in lib389 that can do all the agreements and auth config for you with per-server binds and stuff. It's here:
https://pagure.io/389-ds-base/blob/master/f/src/lib389/lib389/replica.py#_1264
Rather than you saying 'link that machine, and do this etc', you literally just go "here are two servers, make replicate" and it does. It automates cert auth, binds, replica ID creation, makes sure change logs exist, and it can also be used in existing replication topologies. I make the topology_replica use it by default a while back, but perhaps that was undone?
So I think we need both: one cli ui for the "raw" replication bits for people who want it, but then this is just like "hey this server is ROreplica now kgo" kind of thing.

I was hoping you weren't going to say that haha. Yeah that only works if you have the correct credentials for each server. So LDAPI won't work in that case - so its not a feature set the UI can use (not easily). What that really means is that it's an RFE for 1.4.1. Once the UI is wrapped up in 1.4.0, then we can definitely add this functionality!!

This breaks all replication tests on 1.3.x:

[14/Sep/2018:13:47:48.679411028 -0400] - ERR - slapi_entry_schema_check_ext - Entry "cn=replication manager,cn=config" has unknown object class "nsAccount"

@vashirov - correct, it uses the new schema that's only in 1.4.0

Ok, I opened PR#49953 to address this.

I was hoping you weren't going to say that haha. Yeah that only works if you have the correct credentials for each server. So LDAPI won't work in that case - so its not a feature set the UI can use (not easily). What that really means is that it's an RFE for 1.4.1. Once the UI is wrapped up in 1.4.0, then we can definitely add this functionality!!

Yes, you only need credentials for each server to create the agreements, but otherwise it "just works". We'll look at this for the CLI in the future.

I was hoping you weren't going to say that haha. Yeah that only works if you have the correct credentials for each server. So LDAPI won't work in that case - so its not a feature set the UI can use (not easily). What that really means is that it's an RFE for 1.4.1. Once the UI is wrapped up in 1.4.0, then we can definitely add this functionality!!

Yes, you only need credentials for each server to create the agreements, but otherwise it "just works". We'll look at this for the CLI in the future.

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 pull request has been cloned to Github as issue and is available here:
- https://github.com/389ds/389-ds-base/issues/3004

If you want to continue to work on the PR, please navigate to the github issue,
download the patch from the attachments and file a new pull request.

Thank you for understanding. We apologize for all inconvenience.