lb-healthmonitor-create doesn't recognize the timeout parameter

[John Schwarz]

Using the CLI command 'neutron lb-healthmonitor-create --delay 3 --type HTTP --max-retries 3 --timeout 3' doesn't work:

$ neutron lb-healthmonitor-create --delay 3 --type HTTP --max-retries 3 --timeout 3
usage: neutron lb-healthmonitor-create [-h] [-f {shell,table,value}]
                                       [-c COLUMN] [--max-width <integer>]
                                       [--prefix PREFIX]
                                       [--request-format {json,xml}]
                                       [--tenant-id TENANT_ID]
                                       [--admin-state-down]
                                       [--expected-codes EXPECTED_CODES]
                                       [--http-method HTTP_METHOD]
                                       [--url-path URL_PATH] --delay DELAY
                                       --max-retries MAX_RETRIES --timeout
                                       TIMEOUT --type {PING,TCP,HTTP,HTTPS}
neutron lb-healthmonitor-create: error: argument --timeout is required

Multiple variations of this command were tried - no success.

[John Schwarz (jschwarz)]

This is caused by a patch [1] adding a '--timeout' to neutronclient which overrides the one defined in healthmonitor [2].

A fix of 2 lines can be made to the healthmonitor (renaming '--timeout' to '--monitor-timeout', but this will break backward compatibility in the CLI.
Kevin, what do you think?

[1] https://review.openstack.org/#/c/105386/
[2] https://github.com/openstack/python-neutronclient/blob/master/neutronclient/neutron/v2_0/lb/healthmonitor.py (L77)

[Doug Wiegley (dougwig)]

I see two issues here.

1) In traditional unix commands with named subcommands, optargs before the subcommand apply to the command, and after apply to the subcommand. We are violating that. (git is an example.)

2) Our CLI tests have a hole, since --timeout is a REQUIRED parameter for that lb command.

[Kyle Mestery (mestery)]

This is a serious issue, and we need a way to address the immediate issue of the brokeness, and a longer term fix as Doug indicates.

[Kevin Benton (kevinbenton)]

Revert my commit for now. I will rename the option I added to http timeout.

[Kevin Benton (kevinbenton)]

Or I will look at fixing the ordering issue.
On Aug 13, 2014 11:11 AM, "Kevin Benton" <email address hidden> wrote:

> Revert my commit for now. I will rename the option I added to http
> timeout.
>
> --
> You received this bug notification because you are subscribed to the bug
> report.
> https://bugs.launchpad.net/bugs/1353536
>
> Title:
> lb-healthmonitor-create doesn't recognize the timeout parameter
>
> Status in Python client library for Neutron:
> Confirmed
>
> Bug description:
> Using the CLI command 'neutron lb-healthmonitor-create --delay 3
> --type HTTP --max-retries 3 --timeout 3' doesn't work:
>
> $ neutron lb-healthmonitor-create --delay 3 --type HTTP --max-retries 3
> --timeout 3
> usage: neutron lb-healthmonitor-create [-h] [-f {shell,table,value}]
> [-c COLUMN] [--max-width
> <integer>]
> [--prefix PREFIX]
> [--request-format {json,xml}]
> [--tenant-id TENANT_ID]
> [--admin-state-down]
> [--expected-codes EXPECTED_CODES]
> [--http-method HTTP_METHOD]
> [--url-path URL_PATH] --delay
> DELAY
> --max-retries MAX_RETRIES
> --timeout
> TIMEOUT --type
> {PING,TCP,HTTP,HTTPS}
> neutron lb-healthmonitor-create: error: argument --timeout is required
>
> Multiple variations of this command were tried - no success.
>
> To manage notifications about this bug go to:
> https://bugs.launchpad.net/python-neutronclient/+bug/1353536/+subscriptions
>

[OpenStack Infra (hudson-openstack)]

Fix proposed to branch: master
Review: https://review.openstack.org/114005

[Kevin Benton (kevinbenton)]

Hi John, can you test that patch I just proposed? I don't have a full devstack deployment handy but it should prevent the main parser from catching the timeout param from the subcommand.

[Doug Wiegley (dougwig)]

He's in Israel. I'll give it a try.

[Doug Wiegley (dougwig)]

The regression is fixed:

$ neutron lb-healthmonitor-create --delay 5 --max-retries 5 --timeout 5 --type PING
Created a new health_monitor:
+----------------+--------------------------------------+
| Field | Value |
+----------------+--------------------------------------+
| admin_state_up | True |
| delay | 5 |
| id | d7a2c0bd-b5d8-44c8-a44e-d1ab324fd61f |
| max_retries | 5 |
| pools | |
| tenant_id | 4575adf4a43142aeafb5280cd3d9a4ef |
| timeout | 5 |
| type | PING |
+----------------+--------------------------------------+

[Kevin Benton (kevinbenton)]

The only thing I am worried about with this approach is that options to the top level neutron command like --os-username will now not work if they are added after the subcommand.

Kyle, do you think this fix is okay, or should we just revert my original patch and rename the top-level timeout something else?

[Doug Wiegley (dougwig)]

Yes, I had exactly the same concern. Wasn't sure if that was enough to have us stuck with shared options forever.

[John Schwarz (jschwarz)]

With a (small) delay, I can confirm the fix is also working for me.
As a long term solution I offer to change '--timeout' to '--os-timeout'. Then the parameter will be in-line with most of the other top-level parameters (and maybe also see about adding os to '--retries' and '--insecure'?)

[Kevin Benton (kevinbenton)]

It got its name from matching the Nova client.
https://github.com/openstack/python-novaclient/blob/master/novaclient/shell.py#L271

The fix I proposed should work fine if we are fine with requiring correct
grouping and should match other unix command/subcommand semantics like
pointed out.
On Aug 14, 2014 2:16 AM, "John Schwarz" <email address hidden> wrote:

> With a (small) delay, I can confirm the fix is also working for me.
> As a long term solution I offer to change '--timeout' to '--os-timeout'.
> Then the parameter will be in-line with most of the other top-level
> parameters (and maybe also see about adding os to '--retries' and
> '--insecure'?)
>
> --
> You received this bug notification because you are subscribed to the bug
> report.
> https://bugs.launchpad.net/bugs/1353536
>
> Title:
> lb-healthmonitor-create doesn't recognize the timeout parameter
>
> Status in Python client library for Neutron:
> In Progress
>
> Bug description:
> Using the CLI command 'neutron lb-healthmonitor-create --delay 3
> --type HTTP --max-retries 3 --timeout 3' doesn't work:
>
> $ neutron lb-healthmonitor-create --delay 3 --type HTTP --max-retries 3
> --timeout 3
> usage: neutron lb-healthmonitor-create [-h] [-f {shell,table,value}]
> [-c COLUMN] [--max-width
> <integer>]
> [--prefix PREFIX]
> [--request-format {json,xml}]
> [--tenant-id TENANT_ID]
> [--admin-state-down]
> [--expected-codes EXPECTED_CODES]
> [--http-method HTTP_METHOD]
> [--url-path URL_PATH] --delay
> DELAY
> --max-retries MAX_RETRIES
> --timeout
> TIMEOUT --type
> {PING,TCP,HTTP,HTTPS}
> neutron lb-healthmonitor-create: error: argument --timeout is required
>
> Multiple variations of this command were tried - no success.
>
> To manage notifications about this bug go to:
> https://bugs.launchpad.net/python-neutronclient/+bug/1353536/+subscriptions
>

[OpenStack Infra (hudson-openstack)]

Fix proposed to branch: master
Review: https://review.openstack.org/115109

[OpenStack Infra (hudson-openstack)]

Reviewed: https://review.openstack.org/115109
Committed: https://git.openstack.org/cgit/openstack/python-neutronclient/commit/?id=98d2135dbdcc0210328db57dbb7c8fbd28f596be
Submitter: Jenkins
Branch: master

commit 98d2135dbdcc0210328db57dbb7c8fbd28f596be
Author: Kevin Benton <email address hidden>
Date: Mon Aug 18 13:54:01 2014 -0700

    Rename --timeout param to --http-timeout

    The --timeout parameter was already used by the
    load balancer subcommand so adding it as a top-level
    parameter broke the load balancer commands. This
    patch renames the new --timeout top-level command
    to --http-timeout so it doesn't conflict with the
    existing load balancer commands.

    DocImpact

    Closes-Bug: #1353536
    Change-Id: I3d1dd9537e546191c5905e0aa5415de5308d9c7e

[OpenStack Infra (hudson-openstack)]

Change abandoned by Kyle Mestery (<email address hidden>) on branch: master
Review: https://review.openstack.org/114005
Reason: This change is old enough and hasn't seen any updates since August 14, 2014. Abandoning it, please revive it if you plan to work on it again.