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

adding the node object to run_converge messages, resource schema cleanup #208

Merged
merged 1 commit into from
Jun 30, 2016

Conversation

adamleff
Copy link
Contributor

@adamleff adamleff commented Jun 28, 2016

Including the node object in run_converge messages will allow
consumers of Data Collector messages to filter converge completion
messages based on node data more easily than performing additional
queries or joins. This also reduces the number of Data Collector
messages sent by the Chef Client at the end of the run from 2 to 1.

Additionally, the resource schema included in the run_converge
message didn't match up to what was actually used by Chef Analytics
(and expected by other Chef projects) so it was cleaned up to match
appropriately.

@adamleff adamleff force-pushed the adamleff/rfc77-schema-update branch from 0381339 to 0b6308c Compare June 28, 2016 19:27
@adamleff adamleff changed the title adding the node object to run_converge messages adding the node object to run_converge messages, resource schema cleanup Jun 28, 2016
Including the node object in run_converge messages will allow
consumers of Data Collector messages to filter converge completion
messages based on node data more easily than performing additional
queries or joins.

Additionally, the resource schema included in the run_converge
message didn't match up to what was actually used by Chef Analytics
(and expected by other Chef projects) so it was cleaned up to match
appropriately.
@adamleff adamleff force-pushed the adamleff/rfc77-schema-update branch from 0b6308c to 6a56787 Compare June 28, 2016 19:45
@@ -525,6 +445,7 @@ The Run End Schema will be used by Chef Client to notify the data collection ser
"expanded_run_list",
"message_type",
"message_version",
"node",
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm still thinking about this, but do we really want to require a full node object in this message?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I (and others) think we do. A receiver of these messages is likely to want to filter by a variety of properties in order to get the run_converges they want... environment, role, platform, etc. Without knowing all the different properties a user would want and having to constantly change the schema to add them each time a new use case is discovered), putting the node object on this message makes a lot of sense (IMO).

This doesn't actually increase the amount of data the Data Collector is sending as, prior to this change, we sent two messages at the end of the run - the run_converge message, and the action message with the node object. We're proposing consolidating them so users don't have to marry the two messages on their own (via JOINs or whatever), and this reduces the end-of-run messages to a single message.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The expectation is a converge is changing the state of a node (or at least asserting the state is what it currently is), and sending the node along with the converge-is-completed message is like handing over proof of that converge. I'm actually bummed we didn't do this initially :)

Copy link
Contributor

Choose a reason for hiding this comment

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

yeah at some point we probably better want to filter or manage the contents of the node object itself, but we likely want to do that as a core feature

@btm
Copy link
Contributor

btm commented Jun 30, 2016

I didn't pay a ton of attention to this schema in the past, but it looks like you've got three messages:

  1. send the node object
  2. start a run
  3. finish a run

Is that right?

@adamleff
Copy link
Contributor Author

Correct - and we plan to stop sending the "node object" message from Chef Client but still leave the schema defined as there are other implementations that will use that schema, such as the Chef Server sending an action message when a user edits an object with knife.

@btm
Copy link
Contributor

btm commented Jun 30, 2016

And then this PR is putting the node object in #3 to avoid having to send both a #1 and #3 message at the end of the run? Yeah, sounds it.

I'm a little surprised we'd go through the trouble of having different schemas for the start and end of the run. They're feeling like really heavy use specific schemas, rather than light schemas that could be used for multiple use cases. Like a resource_updated schema that could send a message for every converged resource or every five minutes or whatever you wanted so the data collected is more real-time per a run.

Sending one message that represented the run and another that represented a node object state seemed like it made the schemas more flexible.

@adamleff
Copy link
Contributor Author

For us internally a Chef, much of it was based on reusing existing schemas that were in use for other Chef products, so we're iterating over those current schemas as the needs of our current product-under-development evolved, a product which started consuming those existing schemas initially.

I would love to see us have per-event-method schemas and allow users to consume them, but that feels like a later phase or evolution of this. For now, this PR is trying to focus on tying together node details with the completion of a converge so users can more easily and accurately filter their converge messages by node details for reporting purposes.

@lamont-granquist
Copy link
Contributor

👍

@btm
Copy link
Contributor

btm commented Jun 30, 2016

as the decider this week, this was approved at today's meeting.

@btm
Copy link
Contributor

btm commented Jun 30, 2016

We should chat elsewhere to see if we can steer this schema back towards one that is more general and project/product agnostic.

@btm btm merged commit 612943e into master Jun 30, 2016
@btm btm deleted the adamleff/rfc77-schema-update branch June 30, 2016 16:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

3 participants