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 a watch method to Workflow::Runner for event driven updates #95

Merged
merged 22 commits into from
Feb 13, 2024

Conversation

agrare
Copy link
Member

@agrare agrare commented Sep 12, 2023

Add a method for getting update driven notifications about floe events

Get all updates indefinitely:

Floe::Workflow::Runner.docker_runner.wait { |runner_context| puts runner_context }

Get only create/update events for 30 seconds then break out:

Floe::Workflow::Runner.wait(:timeout => 30, :events => %i[create update]) { |runner_context| puts runner_context }

TODO

  • Kubernetes watches
  • Docker events
  • Podman events
  • Return a compatible runner context from the wait
  • Add a way to terminate the wait loop (accept a timeout option)
  • Update Floe::Workflow.wait to use Floe::Workflow::Runner#wait events
  • Update exe/floe to use Floe::Workflow.wait

@agrare agrare requested a review from Fryguy as a code owner September 12, 2023 20:07
@agrare agrare changed the title Add a watch method to Workflow::Runner for event driven updates [WIP] Add a watch method to Workflow::Runner for event driven updates Sep 12, 2023
@miq-bot miq-bot added the wip label Sep 12, 2023
@agrare agrare force-pushed the runner_watch branch 3 times, most recently from 31e0a78 to 53d0403 Compare November 6, 2023 16:18
lib/floe/workflow/state.rb Outdated Show resolved Hide resolved
@agrare agrare force-pushed the runner_watch branch 2 times, most recently from 364dc19 to 8bff3b9 Compare November 6, 2023 16:45
@kbrock
Copy link
Member

kbrock commented Nov 6, 2023

I'm getting distracted by Rename State#run_wait to just #wait
Can you pull that out to a separate PR?
It is a slam dunk.

Lol. Code climate is asking for you to add back please_hold(). I'm ok with it gone.
I had tried hard to find a way to get the complexity there down. But 4 variables that are basically the same thing just plain make it complicated for the machine (and simple for you and me)

@agrare
Copy link
Member Author

agrare commented Nov 6, 2023

Lol. Code climate is asking for you to add back please_hold(). I'm ok with it gone.

Ahh that makes so much more sense, was wondering why we pulled that out into a separate method when it was only called in one place.

@agrare
Copy link
Member Author

agrare commented Nov 6, 2023

I'm getting distracted by Rename State#run_wait to just #wait
Can you pull that out to a separate PR?

👍 yes great idea

@agrare agrare closed this Nov 6, 2023
@agrare agrare reopened this Nov 6, 2023
@agrare agrare force-pushed the runner_watch branch 3 times, most recently from cb254e9 to 2ab742c Compare November 6, 2023 19:50
@Fryguy Fryguy added the enhancement New feature or request label Jan 24, 2024
@Fryguy
Copy link
Member

Fryguy commented Feb 8, 2024

Overall LGTM - only questions I have left are about possible JSON parsing failures as we are parsing the notices line by line.

Comment on lines +113 to +118
if timeout.to_i > 0
timeout_thread = Thread.new do
sleep(timeout)
watcher.finish
end
end
Copy link
Member

Choose a reason for hiding this comment

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

not thrilled with this.
But it is a very common pattern (i.e.: feels like go code)

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah it is my preference for any method call that could block indefinitely to take a timeout option, otherwise this is what you have to do.

kubeclient.watch_pods(:timeout => 30.seconds) would fix this :)

lib/floe/workflow/runner/kubernetes.rb Outdated Show resolved Hide resolved
@miq-bot
Copy link
Member

miq-bot commented Feb 13, 2024

Checked commits agrare/floe@45c905b~...9369eef with ruby 2.7.8, rubocop 1.56.3, haml-lint 0.51.0, and yamllint
7 files checked, 0 offenses detected
Everything looks fine. 🏆

@agrare
Copy link
Member Author

agrare commented Feb 13, 2024

@kbrock @Fryguy this should be ready to go now

ready = []
run_until = Time.now.utc + timeout if timeout.to_i > 0
ready = []
queue = Queue.new
Copy link
Member

Choose a reason for hiding this comment

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

Is there a concurrency concern here? i.e. should we be using something from concurrent-ruby, or is Queue thread safe enough?

Copy link
Member Author

Choose a reason for hiding this comment

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

Is Queue not thread-safe? We use it in ManageIQ core across threads in a few places

Copy link
Member Author

Choose a reason for hiding this comment

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

The docs for Queue show examples using multiple threads

Copy link
Member

Choose a reason for hiding this comment

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

I can never remember hence my question - Some thing in Ruby are thread safe, others are partially and others are not. Otherwise there wouldn't be a Concurrent::Hash :(

Copy link
Member

@Fryguy Fryguy left a comment

Choose a reason for hiding this comment

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

Super-high-level I'm good with this PR (have one non-blocking question above).

In the future I wonder if it makes more sense to use something like concurrent-ruby Promises or some other abstraction. I do worry that there are hidden synchronization bugs here cause threads are hard.

@agrare
Copy link
Member Author

agrare commented Feb 13, 2024

In the future I wonder if it makes more sense to use something like concurrent-ruby Promises or some other abstraction. I do worry that there are hidden synchronization bugs here cause threads are hard.

So one of my biggest issues with Queue is there is no "timed-dequeue" so I had to add an extra thread that just did sleep+queue.put(nil). I was able to get around this by adding a Concurrent::Semaphore that has try_acquire with a timeout parameter but it started looking gross.

@Fryguy Fryguy merged commit bca985e into ManageIQ:master Feb 13, 2024
4 of 5 checks passed
@agrare agrare deleted the runner_watch branch February 13, 2024 22:34
agrare added a commit that referenced this pull request Feb 19, 2024
Changed
- Default to wait indefinitely (#157)
- Create docker runners factory and add scheme (#152)
- Add a watch method to Workflow::Runner for event driven updates (#95)

Fixed
- Fix waiting on extremely short durations (#160)
- Fix wait state missing finish (#159)
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.

4 participants