Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

PR - Issue 50545 - Port repl-monitor.pl to lib389 CLI #3669

Closed
389-ds-bot opened this issue Sep 13, 2020 · 20 comments
Closed

PR - Issue 50545 - Port repl-monitor.pl to lib389 CLI #3669

389-ds-bot opened this issue Sep 13, 2020 · 20 comments
Labels
merged Migration flag - PR pr Migration flag - PR

Comments

@389-ds-bot
Copy link

389-ds-bot commented Sep 13, 2020

Cloned from Pagure Pull-Request: https://pagure.io/389-ds-base/pull-request/50614

  • Created at 2019-09-19 18:46:31 by spichugi (@droideck)
  • Merged at 2019-09-26 09:17:26

Description: Add a new command to 'dsconf replication' CLI.
'dsconf replication monitor' generates a report which
shows the replication topology to which the instance does belong.

Additional arguments:
-c or --connection [CONNECTION [CONNECTION ...]]
The connection values for monitoring other not
connected topologies. The format:
'host:port:binddn:bindpwd'. You can use regex for host
and port.You can set bindpwd to * and it will be
requested at the runtime.
-a or --alias [ALIAS [ALIAS ...]]
If a host:port is assigned an alias, then the alias
instead of host:port will be displayed in the output.
The format: alias=host:port

Resolves: #3601

Reviewed by: ?

@389-ds-bot 389-ds-bot added merged Migration flag - PR pr Migration flag - PR labels Sep 13, 2020
@389-ds-bot
Copy link
Author

Comment from spichugi (@droideck) at 2019-09-19 18:47:31

Only one minor thing I want to add - .dsrc processing.
But feel free to review the actual tool now. It is working and it has all the logic.

@389-ds-bot
Copy link
Author

Comment from spichugi (@droideck) at 2019-09-19 18:51:19

Also, I've added a function get_consumer_replicas but I haven't used it in the code because it would make the logic more complex and harder to understand.
But it can be used by someone in the future, so I've decided to leave it there.

@389-ds-bot
Copy link
Author

Comment from mreynolds (@mreynolds389) at 2019-09-20 15:21:01

We should also have an option to read in a connection's bind password from a file (that would make things easier in the UI).

@389-ds-bot
Copy link
Author

Comment from mreynolds (@mreynolds389) at 2019-09-23 18:05:28

Supplier: localhost:5555
------------------------
Replica Root: dc=example,dc=com
Replica ID: 33
Max CSN: 5d88ece0000000210000
-
Status for agreement: "to master" (localhost:389)

What is the line with the "-"? Can it be removed?

@389-ds-bot
Copy link
Author

Comment from mreynolds (@mreynolds389) at 2019-09-23 18:13:42

dsconf localhost replication monitor --connection localhost:389:cn=dm:password --connection localhost:5555:cn=dm:password 

Enter a bind DN for localhost:389: 

Why am I being prompted for a password since I've already provided one?

@389-ds-bot
Copy link
Author

Comment from mreynolds (@mreynolds389) at 2019-09-23 18:18:12

Ok, so this is where that "-" was coming from. At first I thought it was a "value" without an "attribute". If you are looking for a separator, then perhaps it should many dashes"------------------------------", otherwise it's confusing. Or, even something with a title like "---- Agreement Details ----"

@389-ds-bot
Copy link
Author

Comment from mreynolds (@mreynolds389) at 2019-09-23 18:22:39

dsconf localhost replication monitor --connection localhost:389:cn=dm:password --connection localhost:5555:cn=dm:password

Enter a bind DN for localhost:389:

Why am I being prompted for a password since I've already provided one?

Okay I see I misused the CLI. It expects only one "--connection" parameter. I think we should change this so it's a one-to-one relationship. One arg per connection:

# dsconf localhost replication monitor -c <connection> -c <connection> -c <connection>

@389-ds-bot
Copy link
Author

Comment from mreynolds (@mreynolds389) at 2019-09-23 18:26:18

Also :-) The old script accepted a config file which was just a list of connections. I think dsconf should accept a config/connection file as well, and it should accept the same format used in the old tool so customers can just reuse the same file.

@389-ds-bot
Copy link
Author

Comment from spichugi (@droideck) at 2019-09-24 01:22:09

rebased onto 61180519a9e02739c84996e58b5dd8a27ebb4635

@389-ds-bot
Copy link
Author

Comment from spichugi (@droideck) at 2019-09-24 01:30:31

We should also have an option to read in a connection's bind password from a file (that would make things easier in the UI).

Added.

What is the line with the "-"? Can it be removed?

Sure. I just copied the thing from the original report but I agree it looks cleaner without it. Removing.

dsconf localhost replication monitor --connection localhost:389:cn=dm:password --connection localhost:5555:cn=dm:password
Enter a bind DN for localhost:389:
Why am I being prompted for a password since I've already provided one?

Okay I see I misused the CLI. It expects only one "--connection" parameter. I think we should change this so it's a one-to-one relationship. One arg per connection:
dsconf localhost replication monitor -c -c -c

You can specify multiple args like this:

# dsconf localhost replication monitor -c <connection> <connection> <connection>

I think the option you propose will make the CLI more confusing... argparse originally uses nargs="*" which gives you - -c [CONNECTION [CONNECTION ...]] help usage. Which is consistent and more compact.

Also :-) The old script accepted a config file which was just a list of connections. I think dsconf should accept a config/connection file as well, and it should accept the same format used in the old tool so customers can just reuse the same file.

Yep, just added. Though I've changed the format a bit because I use existing ~/.dsrc functionality that I got working. And it uses ConfigParser which has its limitations... (for example, there is no elegant way to specify multiple arguments - what I've chosen is lesser evil)

Example:

[repl-monitor-connection]
connection1 = server1.example.com:38901:cn=Directory manager:*
connection2 = server2.example.com:38901:cn=Directory manager:[~/pwd.txt]
connection3 = hub1.example.com:.*:cn=Directory manager:password

[repl-monitor-alias]
M1 = server1.example.com:38901
M2 = server1.example.com:38902
H1 = hub1.example.com:38902

Please, review.

@389-ds-bot
Copy link
Author

Comment from spichugi (@droideck) at 2019-09-24 01:37:17

rebased onto 416c5c7689025736121f4ad2bd5011b3343941f6

@389-ds-bot
Copy link
Author

Comment from mreynolds (@mreynolds389) at 2019-09-24 15:51:44

You can specify multiple args like this:
dsconf localhost replication monitor -c

I think the option you propose will make the CLI more confusing... argparse originally uses nargs="*" which gives you - -c [CONNECTION [CONNECTION ...]] help usage. Which is consistent and more compact.

Then change the long arg to "--connections" so it's more obvious it takes multiple values.

Yep, just added. Though I've changed the format a bit because I use existing /.dsrc functionality that I got working. And it uses ConfigParser which has its limitations... (for example, there is no elegant way to specify multiple arguments - what I've chosen is lesser evil)
Example:
[repl-monitor-connection]
connection1 = server1.example.com:38901:cn=Directory manager:*
connection2 = server2.example.com:38901:cn=Directory manager:[
/pwd.txt]
connection3 = hub1.example.com:.*:cn=Directory manager:password

Does the connection name matter?

@389-ds-bot
Copy link
Author

Comment from spichugi (@droideck) at 2019-09-24 16:00:58

Then change the long arg to "--connections" so it's more obvious it takes multiple values.

Sure, makes sense. It was another thing I've copied from the original but we better change it, yeah.

Does the connection name matter?

Nope

@389-ds-bot
Copy link
Author

Comment from spichugi (@droideck) at 2019-09-26 00:37:15

1 new commit added

  • Replace connection and alias with its plurals

@389-ds-bot
Copy link
Author

Comment from spichugi (@droideck) at 2019-09-26 00:41:38

Fixed. Please, review.

Also, the How-To docs are on review too:
https://github.com/marcus2376/389wiki/pull/16

@389-ds-bot
Copy link
Author

Comment from mreynolds (@mreynolds389) at 2019-09-26 00:54:56

Thanks, ack

@389-ds-bot
Copy link
Author

Comment from spichugi (@droideck) at 2019-09-26 08:55:12

rebased onto edf23ac5e6001cad52dfd9f81416123ca79b1f92

@389-ds-bot
Copy link
Author

Comment from spichugi (@droideck) at 2019-09-26 09:16:16

rebased onto 761dd65

@389-ds-bot
Copy link
Author

Comment from spichugi (@droideck) at 2019-09-26 09:17:26

Pull-Request has been merged by droideck

@389-ds-bot
Copy link
Author

Patch
50614.patch

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
merged Migration flag - PR pr Migration flag - PR
Projects
None yet
Development

No branches or pull requests

1 participant