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 Do and DoSync runners #226

Merged
merged 18 commits into from
Oct 5, 2022
Merged

Add Do and DoSync runners #226

merged 18 commits into from
Oct 5, 2022

Conversation

groovemonkey
Copy link
Collaborator

@groovemonkey groovemonkey commented Sep 6, 2022

Overview

Change the runner interface so Run returns a collection of ops.
Products store the results of these ops under the id of the runner.

Add “do” runner that can accept a list of runners and calls .Run() on each in a goroutine.
Return all results as []op.Op
We’ll need to change the runner return to []op.Op

Add “do-sync” runner that can accept a list of runners and calls .Run() for each in order.
Check the Status of the result op and if it’s not Success, abort returning all existing ops.

Notes

  • runners don't currently log. that breaks logging -- to get around this, I'm passing the product logger (which is really the agent logger) into Do/DoSync.

  • Host and nomad runners run inside of a Do runner (async): return []runner.Runner{runner.NewDo(l, runners)}

  • I've written this intentionally so that Do and DoSync can be nested -- it ain't pretty, but it's fine:

// runners := []runner.Runner{} // whatever product runners you start with
syncRunners := []runner.Runner{
runner.NewCommander("uname -a", "string", cfg.Redactions),
runner.NewCommander("echo foobar", "string", cfg.Redactions),
}
// Execute asynchronously: Do inside of DoSync
allRunners := []runner.Runner{runner.NewDoSync(l, append(syncRunners, runner.NewDo(l, runners)))}
return allRunners, nil
  • Do/DoSync runners could add their own "Op" result to the end of the op-slice they return from all their runners implemented in dosync.go). Is that how we want to do this?

product/product.go Outdated Show resolved Hide resolved
runner/do.go Outdated
for i, r := range d.Runners {
d.log.Info("running operation", "runner", r.ID())
go func(m *sync.Mutex, wg *sync.WaitGroup, sm *map[int][]op.Op, r Runner, ridx int) {
// Dereference pointer TODO(is there a better way?)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Since the map isn't needed until we have the lock, an alternative way (not sure it's better or worse, but it avoids a variable assignment) would be to do something like (*sm)[ridx] = o after the lock.

@mkcp mkcp force-pushed the dcohen/experiment-do-dosync branch from a5547ff to 8f57209 Compare September 28, 2022 18:51
@mkcp mkcp force-pushed the dcohen/experiment-do-dosync branch from 4fd5c3d to 5727ef6 Compare September 28, 2022 19:46
@mkcp mkcp marked this pull request as ready for review September 28, 2022 19:47
Copy link
Contributor

@mkcp mkcp left a comment

Choose a reason for hiding this comment

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

Just realized I forgot to actually approve the branch, as discussed. Dave and I have working on this together and have iterated through two approaches for returning collections of results. Constraining Op.Result to a map[string]any allows us to build around nested trees of ops, and was easier to work with than []op.Op. do.Do and do.Sync leverage the results changes to return their result with multiple ops, and each previous runner has been changed to return results in a semantic key.

@mkcp mkcp merged commit a7b93b7 into main Oct 5, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants