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

Check --await and --timeout interaction #2124

Closed
fyrchik opened this issue Dec 1, 2022 · 6 comments · Fixed by #2135
Closed

Check --await and --timeout interaction #2124

fyrchik opened this issue Dec 1, 2022 · 6 comments · Fixed by #2135
Assignees
Labels
neofs-cli NeoFS CLI application issues U2 Seriously planned
Milestone

Comments

@fyrchik
Copy link
Contributor

fyrchik commented Dec 1, 2022

Check how they interact and describe the behavior.
Maybe add --await-timeout

@carpawell
Copy link
Member

carpawell commented Dec 5, 2022

Now we wait for ~120 seconds before saying "timeout: *smth* has not been *actioned* on sidechain"

I think we can go three ways:

  1. Keep it as it is now: (--timeout is for network operations only and pooling timeout is a const);
  2. Make pooling timeout as max(120, --timeout) (if timeout is set);
  3. Add a new --await-timeout, just +1 flag.

I'm somewhere b/w 1 and 3, did not think that --timeout could be interpreted as a pooling timeout before, and do not like adding more flags that mean nothing (neofs-container create can be called with --await-timeout 5 while the block time is 15s so what the expectations are?)

/cc @fyrchik, @cthulhu-rider, @anikeev-yadro, @realloc, @acid-ant

@acid-ant
Copy link
Contributor

acid-ant commented Dec 6, 2022

I vote for 3, we can specify in help value for default timeout 15s to warn about using 5.
Also we can provide option to configure number of retries and sleep interval between each retries. Like --await-retries and --await-sleep-timeout

@cthulhu-rider
Copy link
Contributor

cthulhu-rider commented Dec 6, 2022

I'd leave single timeout for the command since user doesn't care what's going on inside. Awaiting option just tells to finish successfully if container is persisted only, so it doesn't have any relation to timeout. With --await we can clarify the output when request is sent but the operation hasn't persisted yet (I don't remember how is it going now).

@cthulhu-rider cthulhu-rider self-assigned this Dec 6, 2022
cthulhu-rider pushed a commit to cthulhu-rider/neofs-node that referenced this issue Dec 6, 2022
In previous implementation `--timeout` flag affected RPC timeouts only.
Since from the user perspective each command (even async) is monolithic,
it doesn't make sense to tune RPC timeout. On the contrary, the user
understands the whole command timeout setting in the CLI.

Build context with timeout in command's `Run` function. Use the context
for all client operations.

Signed-off-by: Leonard Lyubich <ctulhurider@gmail.com>
cthulhu-rider pushed a commit to cthulhu-rider/neofs-node that referenced this issue Dec 6, 2022
In previous implementation `--timeout` flag affected RPC timeouts only.
Since from the user perspective each command (even async) is monolithic,
it doesn't make sense to tune RPC timeout. On the contrary, the user
understands the whole command timeout setting in the CLI.

Build context with timeout in command's `Run` function. Use the context
for all client operations.

Signed-off-by: Leonard Lyubich <ctulhurider@gmail.com>
@fyrchik
Copy link
Contributor Author

fyrchik commented Dec 8, 2022

  1. What about using --await 2m, i.e. always using explicit timeout?
  2. What about making --await and --await-timeout mutually exclusive?
  3. Or just use both --await and --await-timeout.

@carpawell
Copy link
Member

IMO, the best for us is 1. if it is possible to do without breaking compatibility.

@roman-khimov
Copy link
Member

  1. Timeout should be global, per CLI operation. Internals are opaque to the user, he can't know how many of different requests are being made for his specific operation.
  2. --await should work as it should work, just waiting for event/VUB (not some specific time), irrespective of --timeout.
  3. Sane defaults should be used to cover most of the regular --await cases. Default timeout with --await may differ from the one without it.
  4. Commands should make it clear that the request was successfully sent when that's the case, but they failed to --await.
  5. Other than that, insufficient non-default timeouts are user's responsibility.

cthulhu-rider added a commit to cthulhu-rider/neofs-node that referenced this issue Jun 7, 2023
There is a need to limit execution time of some commands. In Cobra CLI,
the best way to do this is to set command context with timeout.

Add `commonflag.AcceptTimeout` function over `cobra.Command` which
registers `--timeout` flag and precedes `Run` call with time limited
context setting. As before, timeout defaults to 15s.

Refs nspcc-dev#2124.

Signed-off-by: Leonard Lyubich <leonard@morphbits.io>
cthulhu-rider added a commit to cthulhu-rider/neofs-node that referenced this issue Jun 7, 2023
After recent changes, each command has context with particular global
timeout. There is a need to perform network communication within this
context.

Make `client.GetSDKClient` function to accept `context.Context`. Pass
context parameter to the `client.Client.Dial()`. Make deadline of client
call and single stream message to the context deadline.  Make
`client.GetSDKClientByFlag` to pass underlying `cobra.Command.Context()`
to `GetSDKClient`.

Refs nspcc-dev#2124.

Signed-off-by: Leonard Lyubich <leonard@morphbits.io>
cthulhu-rider added a commit to cthulhu-rider/neofs-node that referenced this issue Jun 7, 2023
Previously internal NeoFS API client used by CLI used context without
deadline. Since some of the commands are time limited, there is a need
to execute client operations within particular context.

Add `context.Context` parameter to all op functions of internal client
package. Pass `cobra.Command.Context()` to them in all `Run` scripts.

Refs nspcc-dev#2124.

Signed-off-by: Leonard Lyubich <leonard@morphbits.io>
cthulhu-rider added a commit to cthulhu-rider/neofs-node that referenced this issue Jun 7, 2023
Previously some NeoFS CLI commands like `container create` waited for
the operation persistence for a fixed period of time, decoupled from the
command timeout specified by `--timeout` flag. The wait process should
terminate when the command timeout expires.

Use command context as a waiting stop in contract write ops (`put`,
`delete`, `set-eacl`).

Closes nspcc-dev#2124.

Signed-off-by: Leonard Lyubich <leonard@morphbits.io>
cthulhu-rider added a commit to cthulhu-rider/neofs-node that referenced this issue Jun 7, 2023
Previously internal NeoFS API client used by CLI used context without
deadline. Since some of the commands are time limited, there is a need
to execute client operations within particular context.

Add `context.Context` parameter to all op functions of internal client
package. Pass `cobra.Command.Context()` to them in all `Run` scripts.

Refs nspcc-dev#2124.

Signed-off-by: Leonard Lyubich <leonard@morphbits.io>
cthulhu-rider added a commit to cthulhu-rider/neofs-node that referenced this issue Jun 7, 2023
Previously some NeoFS CLI commands like `container create` waited for
the operation persistence for a fixed period of time, decoupled from the
command timeout specified by `--timeout` flag. The wait process should
terminate when the command timeout expires.

Use command context as a waiting stop in contract write ops (`put`,
`delete`, `set-eacl`).

Closes nspcc-dev#2124.

Signed-off-by: Leonard Lyubich <leonard@morphbits.io>
cthulhu-rider added a commit to cthulhu-rider/neofs-node that referenced this issue Jun 7, 2023
Previously some NeoFS CLI commands like `container create` waited for
the operation persistence for a fixed period of time, decoupled from the
command timeout specified by `--timeout` flag. The wait process should
terminate when the command timeout expires.

Use command context as a waiting stop in contract write ops (`put`,
`delete`, `set-eacl`).

Closes nspcc-dev#2124.

Signed-off-by: Leonard Lyubich <leonard@morphbits.io>
cthulhu-rider added a commit to cthulhu-rider/neofs-node that referenced this issue Jun 7, 2023
After recent changes, each command has context with particular global
timeout. There is a need to perform network communication within this
context.

Make `client.GetSDKClient` function to accept `context.Context`. Pass
context parameter to the `client.Client.Dial()`. Make deadline of client
call and single stream message to the context deadline.  Make
`client.GetSDKClientByFlag` to pass underlying `cobra.Command.Context()`
to `GetSDKClient`.

Refs nspcc-dev#2124.

Signed-off-by: Leonard Lyubich <leonard@morphbits.io>
cthulhu-rider added a commit to cthulhu-rider/neofs-node that referenced this issue Jun 7, 2023
Previously internal NeoFS API client used by CLI used context without
deadline. Since some of the commands are time limited, there is a need
to execute client operations within particular context.

Add `context.Context` parameter to all op functions of internal client
package. Pass `cobra.Command.Context()` to them in all `Run` scripts.

Refs nspcc-dev#2124.

Signed-off-by: Leonard Lyubich <leonard@morphbits.io>
cthulhu-rider added a commit to cthulhu-rider/neofs-node that referenced this issue Jun 7, 2023
Previously some NeoFS CLI commands like `container create` waited for
the operation persistence for a fixed period of time, decoupled from the
command timeout specified by `--timeout` flag. The wait process should
terminate when the command timeout expires.

Use command context as a waiting stop in contract write ops (`put`,
`delete`, `set-eacl`).

Closes nspcc-dev#2124.

Signed-off-by: Leonard Lyubich <leonard@morphbits.io>
cthulhu-rider added a commit to cthulhu-rider/neofs-node that referenced this issue Jun 13, 2023
Some CLI commands like `container create` are async-await. Previously,
default execution timeout was 15s for such commands. Since more than one
RPC is needed for theses commands (e.g. creation request and polling),
there is a need to increase default timeout in waiting mode.

Add `GetCommandContextWithAwait` and use it in `create`, `delete` and
`set-eacl` commands of `container` section. Make command to time out
after 1m if `--await` flag is set while `--timeout` one is omitted.

Closes nspcc-dev#2124.

Signed-off-by: Leonard Lyubich <leonard@morphbits.io>
cthulhu-rider added a commit to cthulhu-rider/neofs-node that referenced this issue Jun 13, 2023
Some CLI commands like `container create` are async-await. Previously,
default execution timeout was 15s for such commands. Since more than one
RPC is needed for theses commands (e.g. creation request and polling),
there is a need to increase default timeout in waiting mode.

Add `GetCommandContextWithAwait` and use it in `create`, `delete` and
`set-eacl` commands of `container` section. Make command to time out
after 1m if `--await` flag is set while `--timeout` one is omitted.

Closes nspcc-dev#2124.

Signed-off-by: Leonard Lyubich <leonard@morphbits.io>
cthulhu-rider added a commit to cthulhu-rider/neofs-node that referenced this issue Jun 14, 2023
Some CLI commands like `container create` are async-await. Previously,
default execution timeout was 15s for such commands. Since more than one
RPC is needed for theses commands (e.g. creation request and polling),
there is a need to increase default timeout in waiting mode.

Add `GetCommandContextWithAwait` and use it in `create`, `delete` and
`set-eacl` commands of `container` section. Make command to time out
after 1m if `--await` flag is set while `--timeout` one is omitted.

Closes nspcc-dev#2124.

Signed-off-by: Leonard Lyubich <leonard@morphbits.io>
cthulhu-rider added a commit to cthulhu-rider/neofs-node that referenced this issue Jun 14, 2023
Some CLI commands like `container create` are async-await. Previously,
default execution timeout was 15s for such commands. Since more than one
RPC is needed for theses commands (e.g. creation request and polling),
there is a need to increase default timeout in waiting mode.

Add `GetCommandContextWithAwait` and use it in `create`, `delete` and
`set-eacl` commands of `container` section. Make command to time out
after 1m if `--await` flag is set while `--timeout` one is omitted.

Closes nspcc-dev#2124.

Signed-off-by: Leonard Lyubich <leonard@morphbits.io>
cthulhu-rider added a commit to cthulhu-rider/neofs-node that referenced this issue Jun 14, 2023
Some CLI commands like `container create` are async-await. Previously,
default execution timeout was 15s for such commands. Since more than one
RPC is needed for theses commands (e.g. creation request and polling),
there is a need to increase default timeout in waiting mode.

Add `GetCommandContextWithAwait` and use it in `create`, `delete` and
`set-eacl` commands of `container` section. Make command to time out
after 1m if `--await` flag is set while `--timeout` one is omitted.

Closes nspcc-dev#2124.

Signed-off-by: Leonard Lyubich <leonard@morphbits.io>
@roman-khimov roman-khimov modified the milestones: v0.37.0, v0.38.0 Jun 15, 2023
cthulhu-rider added a commit to cthulhu-rider/neofs-node that referenced this issue Jun 16, 2023
Some CLI commands like `container create` are async-await. Previously,
default execution timeout was 15s for such commands. Since more than one
RPC is needed for theses commands (e.g. creation request and polling),
there is a need to increase default timeout in waiting mode.

Add `GetCommandContextWithAwait` and use it in `create`, `delete` and
`set-eacl` commands of `container` section. Make command to time out
after 1m if `--await` flag is set while `--timeout` one is omitted.

Closes nspcc-dev#2124.

Signed-off-by: Leonard Lyubich <leonard@morphbits.io>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
neofs-cli NeoFS CLI application issues U2 Seriously planned
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants