login: add --retry=seconds option to login

[joshmoore]

This should replace the wait.sh-stype scripts that exist in
multiple (especially docker) repositories.

Example output:

    $ omero sessions login -w omero root@localhost --retry=100
    Previous session expired for root on localhost:4064
    09:41:30.959913: Login retry #1 in 3s
    09:41:33.977181: Login retry #2 in 3s
    ...
    09:41:58.129038: Login retry #10 in 3s
    WARNING:omero.client:None - createSession retry: 1
    WARNING:omero.client:None - createSession retry: 2
    09:42:17.100098: Login retry #11 in 3s
    Created session for root@localhost:4064. Idle timeout: 10 min. Current group: system

see: https://github.com/ome/omero-server-docker/blob/master/test_login.sh

[sbesson]

A fairly basic test of the functionality, trying to create a session while stopping and restarting a server completed as expected:

(.venv3) bash-4.2$ omero sessions login root@localhost -w omero -C
WARNING:omero.client:..Ignoring error in client.__del__:<class 'Ice.ConnectFailedException'>
InternalException: Failed to connect: Ice.ConnectFailedException:
Network is unreachable
(.venv3) bash-4.2$ omero sessions login root@localhost -w omero -C --retry=5
WARNING:omero.client:..Ignoring error in client.__del__:<class 'Ice.ConnectFailedException'>
21:32:20.723095: Login retry #1 in 3s
WARNING:omero.client:..Ignoring error in client.__del__:<class 'Ice.ConnectFailedException'>
21:32:23.736698: Login retry #2 in 3s
WARNING:omero.client:..Ignoring error in client.__del__:<class 'Ice.ConnectFailedException'>
InternalException: Failed to connect: Ice.ConnectFailedException:
Network is unreachable
(.venv3) bash-4.2$ omero sessions login root@localhost -w omero -C --retry=60
WARNING:omero.client:..Ignoring error in client.__del__:<class 'Ice.ConnectFailedException'>
21:32:36.277700: Login retry #1 in 3s
WARNING:omero.client:..Ignoring error in client.__del__:<class 'Ice.ConnectFailedException'>
21:32:39.293634: Login retry #2 in 3s
WARNING:omero.client:..Ignoring error in client.__del__:<class 'Ice.ConnectFailedException'>
21:32:42.309487: Login retry #3 in 3s
WARNING:omero.client:..Ignoring error in client.__del__:<class 'Ice.ConnectFailedException'>
21:32:45.325290: Login retry #4 in 3s
WARNING:omero.client:..Ignoring error in client.__del__:<class 'Ice.ConnectFailedException'>
21:32:48.342630: Login retry #5 in 3s
21:32:59.326895: Login retry #6 in 3s
Created session for root@localhost:4064. Idle timeout: 10 min. Current group: system

Overall the new logic looks straightforward and I concur having a built-in way to retry a connection feels generally useful.

One inline suggestion about making the new option available outside the omero sessions login subcommand. More generally, are we expecting the new option to be part of the generic session arguments that can be passed to most CLI commands?

[sbesson]

Should this block be set up in the _configure_login instead? The immediate benefit is that the omero login alias to omero sessions login would also recognize the new retry argument.

[joshmoore]

Good point.

[joshmoore]

I'll push a refactoring for @sbesson's fairly minor issue but on the API front, all are happy?

• No other term? (--wait) No need to specify a different wait time?
• Should it be on by default? (Only if it's not an incorrect password, which will need better exception handling)
• Or is this too much of a hack and we should be doing this in an entirely different way? (like /status_check)

[sbesson]

Thanks, the option is now properly exposed

(.venv3) bash-4.2$ omero login --help
usage: /home/omero/workspace/OMERO login [-h] [-C] [-s SERVER] [-p PORT]
                                         [-g GROUP] [-u USER] [-w PASSWORD]
                                         [-k KEY] [--sudo ADMINUSER] [-q]
                                         [-t TIMEOUT] [--retry [RETRY]]
                                         [connection]

Login to a given server, and store session key locally.

USER, HOST, and PORT are set as args or in a ssh-style connection string.
PASSWORD can be entered interactively, or passed via -w (insecure!).
Alternatively, a session KEY can be passed with '-k'.
Admin users can use --sudo=ADMINUSER to login for others.

Examples:
  omero login example.com
  omero login user@example.com
  omero login user@example.com:24064
  omero login -k SESSIONKEY example.com
  omero login --sudo=root user@example

Positional Arguments:
  connection                        Connection string. See extended help for examples

Optional Arguments:
  In addition to any higher level options

  -h, --help                        show this help message and exit
  -t TIMEOUT, --timeout TIMEOUT     Timeout for session. After this many inactive seconds, the session will be closed
  --retry [RETRY]                   Number of seconds to retry login (default: no retry)
...

Thinking more of the question raised #283 (comment), I tried to look at similar examples. Probably the most relevant to this conversation curl.

A few thoughts:

• overall a retry API does not feel out-of-scope for this command
• looking at the curl documentation, I realized that having --retry RETRY mean the number of actual retries was also my natural expectation when I first tested before reading more closely the description
• in terms of additional features, the equivalent of --retry-delay <seconds> is an option. Note the exponential backoff behavior in curl (1s, 2s, 4s, 8s, 16s ... 10min) which is enabled by default unless --retry-delay is set would be interesting to consider
• for now, I would consider making --retry an opt-in via option is completely fine

[joshmoore]

• looking at the curl documentation, I realized that having --retry RETRY mean the number of actual retries was also my natural expectation when I first tested before reading more closely the description

Seems like this is the biggest issue to address since a change would be breaking.

[joshmoore]

Any thoughts, @sbesson ?

[sbesson]

Any thoughts, @sbesson ?

From my side, happy to start with a simple fixed delay and capping based on the total retry time. We can add more complex algorithm or cap the number of retries as follow-up.

I tried to spend a bit of time looking at other examples. Unfortunately, there is no real unified way to specify retry via command-line. Looking at a few examples:

• curl --retry <num> / --retry-max-time <seconds> / --retry-delay <seconds>
• wget --tries=number / --waitretry=seconds
• nslookup set retry
• openvpn –connect-retry n [max] / –connect-retry-max n
• jfrog --retries

In the absence of a compromise, maybe --retry <RETRY> is as good as any. If we decided to implement a configurable number of retries and this becomes confusing, I can imaging we can always deprecate this form in favor of more explicit names e.g. --retry-{max-time/max-count}.

[joshmoore]

Ok. Sounds like we have a path forward if we want to become more explicit. Merging and getting this released.