Allow ordered operations via plugin order #627

johnSchnake · 2019-03-11T18:54:25Z

What this PR does / why we need it:
This change introduces the concept of plugin order. It
is backwards compatible since the default value is 0
which can execute with without precondition.

A plugin can only be executed after all plugins of lower
order have reported results.

Signed-off-by: John Schnake [email protected]

Which issue(s) this PR fixes
Fixes #39

Special notes for your reviewer:

Release note:

Add support for ordered operations via plugin "order" field.

This change introduces the concept of plugin order. It is backwards compatible since the default value is 0 which can execute with without precondition. A plugin can only be executed after all plugins of lower order have reported results. Fixes #39 Signed-off-by: John Schnake <[email protected]>

johnSchnake · 2019-03-11T19:09:16Z

pkg/plugin/aggregation/run.go

+	go func() {
+		pluginsRun := map[int]bool{}
+		for {
+			for index, p := range plugins {


Previously we just synchronously started each plugin and now we just iterate over them checking if we can start them or not.

uses a map to check if we started each plugin (just using index of the plugin as a key)

didnt want to alter the slice of plugins since that is used elsewhere

Where do we keep status of each plugin? Could you create a struct to store all the bits about the plugin (i.e. current status, start time, etc).

johnSchnake · 2019-03-11T19:10:10Z

pkg/plugin/aggregation/run.go

+			}
+
+			// Sleep for a shorter period before rechecking plugins.
+			time.Sleep(10 * time.Second)


Arbitrary; just created as poc. Would make a constant but honestly 10s seems fine to me since its just doing some iterations on a map its not an expensive operation to do every few sec. Especially since the # of plugins is usually very small

johnSchnake · 2019-03-11T19:10:18Z

pkg/plugin/aggregation/run.go

@@ -189,6 +217,11 @@ func Run(client kubernetes.Interface, plugins []plugin.Interface, cfg plugin.Agg
 	}
 }

+func canStartPlugin(a *Aggregator, p plugin.Interface) bool {
+	logrus.Infof("schnake: p.order %v current order: %v", p.GetOrder(), a.currentOrder())


johnSchnake · 2019-03-11T19:10:50Z

pkg/plugin/aggregation/run.go

@@ -189,6 +217,11 @@ func Run(client kubernetes.Interface, plugins []plugin.Interface, cfg plugin.Agg
 	}
 }

+func canStartPlugin(a *Aggregator, p plugin.Interface) bool {


Even though the logic is small I wanted to put it here so that it would be easier to test/manipulate in the future.

johnSchnake · 2019-03-11T19:56:05Z

@akutz what do you think about simple support of ordered operations like this?

See the ticket for the reasoning I had on doing a small version like this; tldr is that I think this unlocks a lot of options for users for minimal effort. I think implementing a full/robust dependency logic and linking runs together will be about 50x harder and (as of now) I've not heard of someone saying they need it (as opposed to just stating they needed ordered ops)

akutz

Hi @johnSchnake,

what do you think about simple support of ordered operations like this?

I think I tried to do this once for muxers as part of HTTP request chain logic, and maintaining a dependency graph is a huge headache. A simple non-branching order is much easier to handle 👍

akutz · 2019-03-13T21:08:07Z

pkg/plugin/aggregation/run.go

-		if err = p.Run(client, cfg.AdvertiseAddress, cert); err != nil {
-			return errors.Wrapf(err, "error running plugin %v", p.GetName())
+	go func() {
+		pluginsRun := map[int]bool{}


Hi @johnSchnake,

Suggesting the following:

pluginsRun := map[int]struct{}

Anonymous (or even empty) structs do not consume any memory in Go. If you're using a map as an addressable flag set, anonymous structs should be used as the value type. Dave Cheney wrote a great piece on this a few years back.

@akutz , thanks. Will do. I'm aware of the pattern, I think this is just a bit left over from iterations on the theme.

akutz · 2019-03-13T21:09:59Z

pkg/plugin/aggregation/run.go

+				// If unable to run plugin, wait.
+				if !canStartPlugin(aggr, p) {
+					logrus.Info("schnake: cant start plugin, continue")
+					continue


Hi @johnSchnake,

This continue has no sleep, and neither does the loop's beginning. Thus if the plug-in is unable to start, and there's only one plug-in, it will immediately try again. Is this desired?

👍 no, thanks for the find. When fixing up the PR I'll be sure to move the sleep (or something) around appropriately.

akutz · 2019-03-13T21:11:22Z

pkg/plugin/aggregation/run.go

-			return errors.Wrapf(err, "error running plugin %v", p.GetName())
+	go func() {
+		pluginsRun := map[int]bool{}
+		for {


Hi @johnSchnake,

You may want to consider using a Go context and channels to select on a channel buffered to the size of the plug-ins and ctx.Done in case the operation is cancelled.

Hi @akutz , that is an interesting idea but would that actually be necessary here since all this is running in containers in the cluster? A "cancel" in that context would mean the user is stopping the run and we are just blowing up the container entirely, right? Or is there another way you can see that being used?

Hi @johnSchnake,

I didn't have any clear idea on how this was being used. When I see the for { pattern I usually ask myself if it should be cancellable, and if so, does it make sense to use a Go context. You know the call stack better than I do, so please do whatever you think makes sense.

akutz · 2019-03-13T21:12:17Z

pkg/plugin/aggregation/run.go

@@ -189,6 +217,11 @@ func Run(client kubernetes.Interface, plugins []plugin.Interface, cfg plugin.Agg
 	}
 }

+func canStartPlugin(a *Aggregator, p plugin.Interface) bool {


Hi @johnSchnake,

Just curious, why isn't this a private function on *Aggregator?

No reason; 6 on one hand half a dozen on the other IMO. Under the hood its all the same so it just came down to my random train of thought at the moment of writing it.

akutz · 2019-03-13T21:13:37Z

pkg/plugin/driver/base.go

@@ -66,6 +66,11 @@ func (b *Base) GetName() string {
 	return b.Definition.Name
 }

+// GetIOrder returns the order of this Job plugin.


Hi @johnSchnake,

// GetIOrder -> // GetOrder

Also, consider rewriting the function signature as func (b Base) GetOrder() int since there is no reason for the function to be able to write any values to the address of b. A read on a copy will suffice and offers better protection.

👍 on the rename.

As far as the pointer, I agree with the unencessity since it is a read. However, go best practice states that you shouldn't have a mixture of pointer/value receivers for a given type. Since other pointer receiver methods exist (and I wasn't planning on changing them all, nor do I think I could with all the implications because this is used by all the other plugin types). (also fwiw I hate linking to the tour as a 'best practice' reference but I can't find the original link I'm thinking of. We can talk more about this if it is a serious concern to you)

Hi @johnSchnake,

I'm not overly concerned about this, but as long as there's a clear pattern to when things read and when things write, I think it's fine. All of GoVmomi does it. Read functions use byCopy receivers, and write functions use byAddress receivers.

I don't feel strongly either way. Just noticed it.

akutz · 2019-03-13T21:14:10Z

pkg/plugin/driver/base.go

+func (b *Base) GetOrder() int {
+	return b.Definition.Order
+}
+
 // GetSecretName gets a name for a secret based on the plugin name and session ID.
 func (b *Base) GetSecretName() string {


Hi @johnSchnake,

Consider rewriting the function signature as func (b Base) GetSecretName() string since there is no reason for the function to be able to write any values to the address of b. A read on a copy will suffice and offers better protection.

akutz · 2019-03-13T21:14:51Z

pkg/plugin/driver/job/job.go

@@ -63,7 +63,10 @@ func NewPlugin(dfn plugin.Definition, namespace, sonobuoyImage, imagePullPolicy
 // a Job only launches one pod, only one result type is expected.
 func (p *Plugin) ExpectedResults(nodes []v1.Node) []plugin.ExpectedResult {


Hi @johnSchnake,

Consider rewriting the function signature as func func (p Plugin) ExpectedResults(nodes []v1.Node) []plugin.ExpectedResult since there is no reason for the function to be able to write any values to the address of p. A read on a copy will suffice and offers better protection.

akutz · 2019-03-13T21:28:55Z

pkg/plugin/aggregation/aggregator.go

@@ -100,6 +100,22 @@ func (a *Aggregator) isComplete() bool {
 	return true
 }

+// currentOrder returns the order of lowest order plugin which doesn't have a result yet. If all have results then -1 is returned.
+func (a *Aggregator) currentOrder() int {


Hi @johnSchnake,

Please consider rewriting the function signature as func (a Aggregator) currentOrder() since there is no reason for the function to be able to write any values to the address of a. A read on a copy will suffice and offers better protection.

I do not see where the result Order is set. It seems to be related to the plug-in order. If plug-ins have a defined order, and the number of plug-ins is known, does that also mean the number of expected results are known? If so, then you could also maintain an analogue slice of addresses of the expected results that is sorted by order and use a Search function from the stdlib sort package. It may be overkill to do so though. I don't know whether or not the list is ever sufficiently large that keeping a second copy of addresses in memory is worth a faster search result.

The list is usually very small; default is 2 and I could realistically imagine 5-10 being common for someone who is trying to run all their own custom, small plugins on each run. Even in a weird application I can't see it being an orders of magnitude higher to where we are concerned with the performance of a sort.

johnSchnake · 2019-03-15T03:02:10Z

@akutz Thank you so much for both the high and low-level review. I'm going to consolidate what I've learned and done into a design doc to clarify some design details before finalizing things in this PR (or a related one).

A little odd to do a POC, then design doc for what is such a small feature in terms of LOC but I think something like this is pretty important to get right the first time.

💯 for your time and detail.

stale · 2020-11-04T23:04:43Z

There has not been much activity here. We'll be closing this issue if there are no follow-ups within 15 days.

johnSchnake requested a review from stevesloka March 11, 2019 18:54

johnSchnake commented Mar 11, 2019

View reviewed changes

johnSchnake requested a review from akutz March 11, 2019 19:53

johnSchnake mentioned this pull request Mar 12, 2019

Allow for user ordered operations in aggregator #39

Closed

akutz approved these changes Mar 13, 2019

View reviewed changes

This was referenced Apr 17, 2019

Provide flags to disable non plugin related operations #682

Closed

Add CLI flag for aggregation timeout #686

Merged

stale bot added the misc/wontfix label Nov 4, 2020

stale bot closed this Nov 19, 2020

johnSchnake mentioned this pull request Apr 29, 2022

Allow plugin ordering #1684

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allow ordered operations via plugin order #627

Allow ordered operations via plugin order #627

johnSchnake commented Mar 11, 2019

johnSchnake Mar 11, 2019

stevesloka Mar 13, 2019

johnSchnake Mar 11, 2019

johnSchnake Mar 11, 2019

johnSchnake Mar 11, 2019

johnSchnake commented Mar 11, 2019

akutz left a comment

akutz Mar 13, 2019

johnSchnake Mar 14, 2019

akutz Mar 13, 2019

johnSchnake Mar 14, 2019

akutz Mar 13, 2019

johnSchnake Mar 14, 2019

akutz Mar 14, 2019

akutz Mar 13, 2019

johnSchnake Mar 14, 2019

akutz Mar 13, 2019

johnSchnake Mar 14, 2019

akutz Mar 14, 2019

akutz Mar 13, 2019

akutz Mar 13, 2019

akutz Mar 13, 2019

johnSchnake Mar 14, 2019

johnSchnake commented Mar 15, 2019

stale bot commented Nov 4, 2020

		@@ -63,7 +63,10 @@ func NewPlugin(dfn plugin.Definition, namespace, sonobuoyImage, imagePullPolicy
		// a Job only launches one pod, only one result type is expected.
		func (p *Plugin) ExpectedResults(nodes []v1.Node) []plugin.ExpectedResult {

Allow ordered operations via plugin order #627

Allow ordered operations via plugin order #627

Conversation

johnSchnake commented Mar 11, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

johnSchnake commented Mar 11, 2019

akutz left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

johnSchnake commented Mar 15, 2019

stale bot commented Nov 4, 2020