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

Add Task#Retrier incremental backoff and Wait#Timestamp #100

Merged
merged 3 commits into from
Oct 5, 2023

Conversation

kbrock
Copy link
Member

@kbrock kbrock commented Sep 21, 2023

Change Wait#running?

I feel running? should read the state to see if the task is complete but should not change the state.

This change was also introduced to be reusable by Task#wait.

Before

running? checks if we have passed the amount of time necessary. Once it is no longer running, this method sets the state["EndTime"].

After

start sets the target time when this will end. running? just checks Time.now agains the predefined value.

  • This allows all 4 of the time options to be used without evaluating them over each tick.
  • running? no longer modifies state
  • This allows us to expose the expectation for when this will finish to the workflow engine. So it can use sleep or MiqQueue#deliver_on.

Wait#SecondsPath wait ref

The existing Seconds parameter reads the wait time out of the payload.
The new SecondsPath reads the wait time out of the input.

Wait#Timestamp, Wait#TimestampPath

While the existing Seconds parameter is a delta, and the new Timestamp parameter is an absolute time.
The TimestampPath parameter is the equivalent, but the value comes from the input.

The implementation for Timestamp and TimestampPayload are not very different from Seconds. wait ref

Task#retry

A retrier specifies how many times to re-run a Task after it fails.
But the current code does not delay the retry. More information on the retry delay logic can be found in the ref

Before

A task retry does not introduce a delay so there is no incremental backoff.
The retrier does allow you to define it, but it is not used.
You will notice the TODO removed from this PR.

After

We do a non-blocking sleep similar to Wait when a task has a failure.
Since the Task#wait implementation is based upon Sleep#wait, I bundled them together.

Removed

Extracted workflow#wait_until to another PR.

@kbrock
Copy link
Member Author

kbrock commented Sep 21, 2023

update:

  • rebased to include string dates
  • storing wait as a string

update:

  • Workflow#wait_until

@kbrock
Copy link
Member Author

kbrock commented Sep 22, 2023

update:

  • rebased
  • changed comment around adding success states to make it obvious that it is a development feature

@kbrock
Copy link
Member Author

kbrock commented Sep 22, 2023

update:

  • helper method value_or_path to simplify implementation of using a path or value
  • was able to put the wait() call back into Wait#start (vs rubocop encouraged separate method)
  • test for wait_until

lib/floe/workflow/state.rb Outdated Show resolved Hide resolved
@kbrock
Copy link
Member Author

kbrock commented Sep 25, 2023

Updates:

  • rebased onto cops (had a conflict when adding timecop gem)
  • fixed cops related to hashrocket and extra newline

@Fryguy Fryguy added the enhancement New feature or request label Sep 27, 2023
@kbrock kbrock changed the title Wait Implement Task#retrier_wait, Wait#Timestamp, Wait#TimestampPath, Wait#SecondsPath Sep 28, 2023
@kbrock kbrock changed the title Implement Task#retrier_wait, Wait#Timestamp, Wait#TimestampPath, Wait#SecondsPath Implement Task#retrier_wait, Wait#Timestamp and path versions Sep 28, 2023
@kbrock kbrock changed the title Implement Task#retrier_wait, Wait#Timestamp and path versions Implement Task#retrier_wait, Wait#Timestamp{,Path} Sep 28, 2023
@kbrock kbrock changed the title Implement Task#retrier_wait, Wait#Timestamp{,Path} Add Task#retrier_wait, Wait#Timestamp{,Path} Sep 28, 2023
@Fryguy
Copy link
Member

Fryguy commented Sep 29, 2023

Can you describe the problem you are trying to solve in the OP? It;'s hard to understand what problem/feature this is trying to solve.

Saw the little refs in the OP - are those new features being added here?

@kbrock
Copy link
Member Author

kbrock commented Oct 3, 2023

update:

  • split timecop/spec changes addition to own commit (to show it is green before/after)
  • rebased
  • removed wait_until

@kbrock kbrock changed the title Add Task#retrier_wait, Wait#Timestamp{,Path} Add Task#Retrier incremental backoff and Wait#Timestamp Oct 3, 2023
Copy link
Member

@agrare agrare left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍 LGTM just needs a rebase

@miq-bot
Copy link
Member

miq-bot commented Oct 5, 2023

Checked commits kbrock/floe@7d3af17~...5eff318 with ruby 2.6.10, rubocop 1.28.2, haml-lint 0.35.0, and yamllint
8 files checked, 0 offenses detected
Everything looks fine. 👍

@agrare agrare merged commit 0c30358 into ManageIQ:master Oct 5, 2023
5 checks passed
@kbrock kbrock deleted the wait branch October 5, 2023 20:37
agrare added a commit that referenced this pull request Oct 6, 2023
Added
- Add Fail#CausePath and Fail#ErrorPath (#110)
- Add Task#Retrier incremental backoff and Wait#Timestamp (#100)

Fixed
- Combine stdout and stderr for docker and podman runners (#104)
- Don't raise an exception on task failure (#115)
- Fix task output handling (#112)
- Fix Context#input not JSON parsed (#122)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants