search

kubernetes-sigs / cli-utils

This repo contains binaries that built from libraries in cli-runtime.
Apache License 2.0
156 stars 78 forks source link

Change the behavior of hanlding a WaitTask timeout #451

Closed haiyanmeng closed 3 years ago

haiyanmeng commented 3 years ago

Today, when a WaitTask timeout happens, the WaitTask sends the TimeoutError on the TaskChannel. After receiving the TimeoutError, baseRunner.run terminates immediately by returning the error to its caller (Applier.Run or Destroyer.Run). The caller then sends the error onto the EventChannel and terminates.

With this PR, when a WaitTask timeout happens, the WaitTask sends a WaitType Event including the TimeoutError on the EventChannel, and then sends an empty TaskResult on the TaskChannel. An empty TaskResult suggests that the task finished successfully, and therefore baseRunner.run would continue instead of terminate.

The motivation of this change is to make sure that cli-utils only terminates on fatal errors (such as inventory-related errors, and ApplyOptions creation errors). A WaitTask timeout may not always mean a fatal error (it may happen because the StatusPoller has not finished polling everything, or some but not all the resources have not reached the desired status), and therefore should not terminate cli-utils.

haiyanmeng commented 3 years ago

/cc @Liujingfang1 @karlkfi @janetkuo /assign @seans3 @mortent

mortent commented 3 years ago

/approve

haiyanmeng commented 3 years ago

/cc @ash2k

haiyanmeng commented 3 years ago

@ash2k , let me know if this change breaks your use case.

haiyanmeng commented 3 years ago

@seans3 , @mortent , @karlkfi , I updated the PR based on our last discussion. PTAL.

ash2k commented 3 years ago

@haiyanmeng I haven't checked the code closely, but I think it shouldn't affect me. Thanks for the ping.

k8s-ci-robot commented 3 years ago

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: haiyanmeng, mortent, seans3

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files: - ~~[OWNERS](https://github.com/kubernetes-sigs/cli-utils/blob/master/OWNERS)~~ [mortent,seans3] Approvers can indicate their approval by writing `/approve` in a comment Approvers can cancel approval by writing `/approve cancel` in a comment