ome / omero-py

Python project containing Ice remoting code for OMERO
https://www.openmicroscopy.org/omero
GNU General Public License v2.0
20 stars 33 forks source link

login: add --retry=seconds option to login #283

Closed joshmoore closed 3 years ago

joshmoore commented 3 years ago

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

joshmoore commented 3 years ago

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

sbesson commented 3 years ago

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 https://github.com/ome/omero-py/pull/283#issuecomment-815500432, I tried to look at similar examples. Probably the most relevant to this conversation curl.

A few thoughts:

joshmoore commented 3 years ago
  • 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 commented 3 years ago

Any thoughts, @sbesson ?

sbesson commented 3 years ago

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:

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 commented 3 years ago

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