From 7452a974c46cd5df52b8ab2400ea13f4daa3c746 Mon Sep 17 00:00:00 2001 From: danielsdeleo Date: Thu, 20 Mar 2014 09:42:21 -0700 Subject: [PATCH 1/5] Initial draft of attribute consistency proposal --- rfc006-attribute-consistency.md | 142 ++++++++++++++++++++++++++++++++ 1 file changed, 142 insertions(+) create mode 100644 rfc006-attribute-consistency.md diff --git a/rfc006-attribute-consistency.md b/rfc006-attribute-consistency.md new file mode 100644 index 00000000..5e156168 --- /dev/null +++ b/rfc006-attribute-consistency.md @@ -0,0 +1,142 @@ +# Attribute Consistency + +This proposal has two parts: + +1. Normal level attributes should not be persisted. A new API should be + added to address the persistence use case. +2. System profiling attributes should be accessed by a different API + than other node attributes + +## Normal Attribute Persistence + +Persisting 'normal' level attributes creates a confusing, surprising +special case in attribute behavior, which makes documentation and best +practices more complicated. + +This results in the following user pain points: +* learning chef attributes is more complicated, since documentation and + recommendations are more complicated to understand. +* "Phantom" attributes. Users remove an attribute, or modify a + complicated data structure, but their changes are mysteriously not + applied because the normal attributes from a previous run are not + cleared. +* Recipes that generate node data that needs to be persisted must call + `node.save` manually, to ensure that persisted data is not lost in the + event that the chef-client run subsequently fails. This causes issues + with search data consistency, increased server load, and other + problems. + +The benefit of normal attributes persistence behavior is that they are +persisted, not that they are attributes. Common use cases include +storing data generated on the node (such as autogenerated passwords) or +exposing a mechanism for triggering a different behavior in a cookbook +by changing a node attribute using the server API. These use cases would +be equally well served by having a completely separate API for +persisting node-specific data. + +### Node Specific Persistent Data API. + +When considering the user-facing API for persistent node data, we should +consider the following factors: + +* When are updates stored on the server? We could write the feature so +that any assignment of new data triggers a save, users save changes +manually, or changes are stored in bulk at the conclusion of a +chef-client run. +* Should there be any sort of conflict detection? This may be important +if simultaneous modification of data (by both chef-client and by a human +user via HTTP API) is prevalent. +* What does search look like? + +### Swag API + +```ruby +node.storage.fetch(:namespace1, :namespace2, ...) # => value or nil + +node.storage.set[:namespace1, :namespace2, ...] = value + +node.storage.clear(:namespace1, :namespace1, ...) +``` + +This was designed quickly and probably has lots of flaws. The key point +is that the `set` API uses a single method call, which allows for +values to be saved immediately as part of the call to the +`[]=(*keyspace, value)` method call. + +### Release Timing + +This proposal is a breaking change. We have a few options: + +1. Make the change all at once, for Chef 12 +2. Introduce the new storage API ASAP, issue deprecation warnings + whenever normal attributes are used in Chef 11.next, and remove + normal attribute persistence in Chef 12. +3. Make the change all at once in Chef 12, but with a configuration + option to enable the old behavior. Remove the option in Chef 13. +4. Deprecate normal attribute usage in Chef 12, and remove persistence + in Chef 13. + +One thing to note is that deprecation without behavior change +effectively makes normal level attributes unusable until the persistence +behavior is removed. + + +## Separate System Profiling Attributes + +Currently node attributes set by users (via cookbooks, etc.) share a +namespace with those collected by ohai. This is problematic for several +reasons: + +* Since ohai's attributes have the highest precedence level, any time a +new attribute is added (for example, adding a new plugin to core ohai), +there is risk that it may stomp on attributes users were already using +for cookbooks. Luckily we haven't had any major bugs arise from this, +but one can get a feel for the risk involved by imagining an ohai plugin +began providing a 'mysql' attribute. +* It mixes actual with desired state. Attributes set by roles, +cookbooks, etc. are "aspirational"--they reflect what the node should +become. Ohai's attributes are "informational"--they reflect data +collected from the actual system at some point in time. +* It mixes user-controlled namespace with system controlled namespace. +This is a conceptual issue as well as a potential source of bugs as +described above. + +### System Attributes API SWAG + +One problem here is that "system" is the obvious word to use, but this +is the name of a method defined on `Kernel`, so there could be some +negative consequences of using it. One alternative is to shorten it to +`sys`. + +#### Accessed via `node`: + +```ruby +node.sys[:kernel][:machine] +node.sys[:fqdn] +``` + +#### Accessed via delegate method: + +To reduce repetitive typing, we can delegate the `sys` method to `node` +in all places that `node` is defined in the DSL. Then you can access +these values like so: + +``` +sys[:kernel][:machine] +sys[:fqdn] +``` + +### Release Timing + +In this case there is little downside to taking a longer time to +deprecate and eventually remove the current behavior. I think the best +course of action is to deprecate access of ohai data via combined +attributes in Chef 12 and remove it in Chef 13. The new APIs for +accessing system data can be introduced at any time before Chef 12. + +As part of the deprecation process, we can announce that potential +overlap between ohai attributes and user-defined attributes will no +longer be considered a breaking change during the Chef 12 release cycle, +which achieves all of the goals of this proposal without breaking +anything immediately upon the release of Chef 12.0. + From 942a812de38cf10699f6f9eeb3a823d83f73e50a Mon Sep 17 00:00:00 2001 From: danielsdeleo Date: Mon, 15 Dec 2014 12:06:15 -0800 Subject: [PATCH 2/5] Rewrite attributes proposal * structure/formatting matches the standard * rework explanation of the motivations * Remove all breaking change stuff, this will be released as a new API alongside the old one. * Remove API examples; will debate a few API style options before adding. --- new/rfcXXX-new-attributes-api.md | 151 +++++++++++++++++++++++++++++++ rfc006-attribute-consistency.md | 142 ----------------------------- 2 files changed, 151 insertions(+), 142 deletions(-) create mode 100644 new/rfcXXX-new-attributes-api.md delete mode 100644 rfc006-attribute-consistency.md diff --git a/new/rfcXXX-new-attributes-api.md b/new/rfcXXX-new-attributes-api.md new file mode 100644 index 00000000..ed3f85c7 --- /dev/null +++ b/new/rfcXXX-new-attributes-api.md @@ -0,0 +1,151 @@ +--- +RFC: unassigned +Author: Daniel DeLeo +Status: Draft +Type: Standards Track +--- + +# New Attributes API + +This RFC proposes a new API for setting and accessing attributes in +cookbooks. The goals of this new API are: + +* Separate desired and observed state. +* Structure the syntax in a way that "trainwreck" errors + (`NoMethodError` for `[]` on `nil`) can be avoided. +* Structure the syntax in a way that attribute tracing can be + implemented simply. +* Separate node-specific persistent storage from + cookbook/role/environment attributes. +* Structure the syntax in a way that can be implemented efficiently. + +The new API is added alongside the existing API, so existing code will +work without modification. + +## Motivation + +There are several motivations for this change. + +### Separate Desired and Observed State + +In the current attributes system, system "facts" determined by Ohai are +intermixed with user attributes, which are generally used to define the +desired state of the system. For example, `node[:fqdn]` gives the +system's FQDN, as determined by Ohai at the beginning of the Chef run, +and identical syntax is used to reference a user-defined attribute, such +as `node[:mysql][:version]`. As a result, it's not possible to know +which attributes are "facts" except by committing to memory the list of +attributes provided by Ohai. Defining a new syntax where Ohai and +user-defined attributes are accessed by different methods makes cookbook +code clearer. + +Additionally, separating Ohai data from user-defined attributes +eliminates the possibility of attribute name collisions. For example, a +user might wish to set the FQDN of a host with Chef and use the `fqdn` +attribute to define the desired FQDN, or create an Ohai plugin to detect +the currently installed version of MySQL, storing this as +`node[:mysql][:version]`. With the current attributes system, the user +can't use these attribute names because they conflict with existing Ohai +or cookbook attributes. + +### Eliminate Trainwreck Errors + +The current attribute system presents node data as a nested collection +of Hash-like objects. There are some unfortunate limitations of this +model that lead to frustrating behavior for users. In particular, code +like this: + +```ruby +node[:foo][:bar][:baz] +``` + +May raise a `NoMethodError` with the message: + +``` +undefined method `[]' for nil:NilClass +``` + +Note that in this example, it's ambiguous as to which part of the +attribute structure is missing, as this error will occur in an identical +way if either `node[:foo]` or `node[:foo][:bar]` are `nil`. + +The UX of this error case cannot be significantly improved without +abandoning the nested Hash model. If an object other than `nil` was +returned for accessing a missing key, then code like +`do_something if node[:some_attribute_present]` would break (Ruby does +not allow custom false-y types), and monkey patching `nil` is +error-prone because `nil` is an immediate object (there is only one +instance). + +### Enable Attribute Tracing + +A common problem when developing cookbook code is attributes containing +unexpected values, which can be frustrating to debug. The current +implementation of attributes makes implementation of a tracing feature +difficult; in particular, it requires that attribute objects have +circular references to their parent objects so that the full attribute +path is known when setting a new attribute value. It is preferable to +avoid circular references because code that blindly walks the object +graph (such as a string inspection method) may enter an infinite loop if +it does not correctly guard against this case. Additionally, edge cases +such as "reparenting" a part of the node tree may be difficult to +implement correctly. + +Providing an attributes API where the full attribute path is known when +new attributes are set would greatly ease the implementation of the +tracing feature. + +### Separate Node-Specific Persistent Storage from Other Attributes + +So-called "normal attributes" in the current attributes API have a +unique behavior compared to other attributes in that they are persisted +between Chef Client runs. While this behavior is useful and necessary +for some use cases, it causes some frustrations for users: + +* learning chef attributes is more complicated, since documentation and + recommendations are more complicated to understand. +* "Phantom" attributes. Users remove an attribute, or modify a + complicated data structure, but their changes are mysteriously not + applied because the normal attributes from a previous run are not + cleared. +* Recipes that generate node data that needs to be persisted must call + `node.save` manually, to ensure that persisted data is not lost in the + event that the chef-client run subsequently fails. This causes issues + with search data consistency, increased server load, and other + problems. + +### Enable More Efficient Implementation + +The current implementation has poor performance when attributes are +mutated and then read frequently. Chef 12 introduces a partial cache to +mitigate the problem, but this doesn't completely solve the performance +issues in certain degenerate cases. Due to the limitations of the +existing API, it is difficult to improve the performance further. The +proposed API should allow for straightforward implementation of a +granular merged attribute cache. + +## Specification + +### Ohai Data Access + +TBD + +### Default and Override Attribute Set and Access + +TBD + +### Node-Specific Data Storage Set and Access + +TBD + +## Rationale + +This section will be updated once the specification is more complete. + +## Copyright + +This work is in the public domain. In jurisdictions that do not allow for this, +this work is available under CC0. To the extent possible under law, the person +who associated CC0 with this work has waived all copyright and related or +neighboring rights to this work. + diff --git a/rfc006-attribute-consistency.md b/rfc006-attribute-consistency.md deleted file mode 100644 index 5e156168..00000000 --- a/rfc006-attribute-consistency.md +++ /dev/null @@ -1,142 +0,0 @@ -# Attribute Consistency - -This proposal has two parts: - -1. Normal level attributes should not be persisted. A new API should be - added to address the persistence use case. -2. System profiling attributes should be accessed by a different API - than other node attributes - -## Normal Attribute Persistence - -Persisting 'normal' level attributes creates a confusing, surprising -special case in attribute behavior, which makes documentation and best -practices more complicated. - -This results in the following user pain points: -* learning chef attributes is more complicated, since documentation and - recommendations are more complicated to understand. -* "Phantom" attributes. Users remove an attribute, or modify a - complicated data structure, but their changes are mysteriously not - applied because the normal attributes from a previous run are not - cleared. -* Recipes that generate node data that needs to be persisted must call - `node.save` manually, to ensure that persisted data is not lost in the - event that the chef-client run subsequently fails. This causes issues - with search data consistency, increased server load, and other - problems. - -The benefit of normal attributes persistence behavior is that they are -persisted, not that they are attributes. Common use cases include -storing data generated on the node (such as autogenerated passwords) or -exposing a mechanism for triggering a different behavior in a cookbook -by changing a node attribute using the server API. These use cases would -be equally well served by having a completely separate API for -persisting node-specific data. - -### Node Specific Persistent Data API. - -When considering the user-facing API for persistent node data, we should -consider the following factors: - -* When are updates stored on the server? We could write the feature so -that any assignment of new data triggers a save, users save changes -manually, or changes are stored in bulk at the conclusion of a -chef-client run. -* Should there be any sort of conflict detection? This may be important -if simultaneous modification of data (by both chef-client and by a human -user via HTTP API) is prevalent. -* What does search look like? - -### Swag API - -```ruby -node.storage.fetch(:namespace1, :namespace2, ...) # => value or nil - -node.storage.set[:namespace1, :namespace2, ...] = value - -node.storage.clear(:namespace1, :namespace1, ...) -``` - -This was designed quickly and probably has lots of flaws. The key point -is that the `set` API uses a single method call, which allows for -values to be saved immediately as part of the call to the -`[]=(*keyspace, value)` method call. - -### Release Timing - -This proposal is a breaking change. We have a few options: - -1. Make the change all at once, for Chef 12 -2. Introduce the new storage API ASAP, issue deprecation warnings - whenever normal attributes are used in Chef 11.next, and remove - normal attribute persistence in Chef 12. -3. Make the change all at once in Chef 12, but with a configuration - option to enable the old behavior. Remove the option in Chef 13. -4. Deprecate normal attribute usage in Chef 12, and remove persistence - in Chef 13. - -One thing to note is that deprecation without behavior change -effectively makes normal level attributes unusable until the persistence -behavior is removed. - - -## Separate System Profiling Attributes - -Currently node attributes set by users (via cookbooks, etc.) share a -namespace with those collected by ohai. This is problematic for several -reasons: - -* Since ohai's attributes have the highest precedence level, any time a -new attribute is added (for example, adding a new plugin to core ohai), -there is risk that it may stomp on attributes users were already using -for cookbooks. Luckily we haven't had any major bugs arise from this, -but one can get a feel for the risk involved by imagining an ohai plugin -began providing a 'mysql' attribute. -* It mixes actual with desired state. Attributes set by roles, -cookbooks, etc. are "aspirational"--they reflect what the node should -become. Ohai's attributes are "informational"--they reflect data -collected from the actual system at some point in time. -* It mixes user-controlled namespace with system controlled namespace. -This is a conceptual issue as well as a potential source of bugs as -described above. - -### System Attributes API SWAG - -One problem here is that "system" is the obvious word to use, but this -is the name of a method defined on `Kernel`, so there could be some -negative consequences of using it. One alternative is to shorten it to -`sys`. - -#### Accessed via `node`: - -```ruby -node.sys[:kernel][:machine] -node.sys[:fqdn] -``` - -#### Accessed via delegate method: - -To reduce repetitive typing, we can delegate the `sys` method to `node` -in all places that `node` is defined in the DSL. Then you can access -these values like so: - -``` -sys[:kernel][:machine] -sys[:fqdn] -``` - -### Release Timing - -In this case there is little downside to taking a longer time to -deprecate and eventually remove the current behavior. I think the best -course of action is to deprecate access of ohai data via combined -attributes in Chef 12 and remove it in Chef 13. The new APIs for -accessing system data can be introduced at any time before Chef 12. - -As part of the deprecation process, we can announce that potential -overlap between ohai attributes and user-defined attributes will no -longer be considered a breaking change during the Chef 12 release cycle, -which achieves all of the goals of this proposal without breaking -anything immediately upon the release of Chef 12.0. - From 5b9b0819abdef56c2eec02c63cadb49cd75af6d3 Mon Sep 17 00:00:00 2001 From: danielsdeleo Date: Mon, 15 Dec 2014 15:26:48 -0800 Subject: [PATCH 3/5] Clarify scope of normal attribute change Also add "method missing method name collisions" to the list of motivations. --- new/rfcXXX-new-attributes-api.md | 27 ++++++++++++++++++++++++++- 1 file changed, 26 insertions(+), 1 deletion(-) diff --git a/new/rfcXXX-new-attributes-api.md b/new/rfcXXX-new-attributes-api.md index ed3f85c7..6992363f 100644 --- a/new/rfcXXX-new-attributes-api.md +++ b/new/rfcXXX-new-attributes-api.md @@ -124,6 +124,26 @@ existing API, it is difficult to improve the performance further. The proposed API should allow for straightforward implementation of a granular merged attribute cache. +### Method Missing Method Name Collisions + +The current attributes system allows attributes to be accessed via a +`method_missing` API, e.g., `node["fqdn"]` can also be accessed as +`node.fqdn`. While this makes the code appear more readable, +implementing it directly on the `node` object leads to method name +collisions which can be confusing for users. For example: + +``` +node.foo # returns node['foo'] + +node.chef_environment # returns the environment, which is an ivar on the + # node object + +node.class # returns the Chef::Node symbol +``` + +Providing a new API where attribute access occurs via arguments to +statically defined methods eliminates this source of ambiguity. + ## Specification ### Ohai Data Access @@ -136,7 +156,12 @@ TBD ### Node-Specific Data Storage Set and Access -TBD +Syntax is TBD. + +Node-specific storage will initially be implemented as a facade on top +of the existing "normal" attributes structure. In the future we may +propose additional enhancements to this API to provide pluggability, +orchestration support, and/or improved authorization behaviors. ## Rationale From 754d7a4bcbb69593394725eaa8e994a8ed37abff Mon Sep 17 00:00:00 2001 From: danielsdeleo Date: Tue, 16 Dec 2014 12:35:34 -0800 Subject: [PATCH 4/5] Move syntax "swag" examples in-repo Gists aren't great for discussion because you can't comment on a line or to a particular person, so moving this in-repo to facilitate discussion. --- new/rfcXXX-new-attributes-api-syntax.rb | 76 +++++++++++++++++++++++++ 1 file changed, 76 insertions(+) create mode 100644 new/rfcXXX-new-attributes-api-syntax.rb diff --git a/new/rfcXXX-new-attributes-api-syntax.rb b/new/rfcXXX-new-attributes-api-syntax.rb new file mode 100644 index 00000000..aad620ab --- /dev/null +++ b/new/rfcXXX-new-attributes-api-syntax.rb @@ -0,0 +1,76 @@ +## About this file: +# This is a supplementary document for the "New Attributes API" RFC. +# It is included in the pull request to facilitate conversation. When we reach +# a consensus about what shape the API should take, this document will be +# removed and the "winning" API option will be described in the "Specification" +# section of that RFC document. +# +## COPYRIGHT: +# This work is in the public domain. In jurisdictions that do not allow for this, +# this work is available under CC0. To the extent possible under law, the person +# who associated CC0 with this work has waived all copyright and related or +# neighboring rights to this work. + +## API goals: +# * API should facilitate achieving the goals stated in the RFC's motivation section. +# * Balance brevity and clarity. +# * read and write should be unambiguous +# * Mass Assignment of attributes within a "namespace" should be easy +# * Single assignment of a nested value should be easy +# +## Things to consider: +# * Should we add explicit support for "dynamic" attributes? +# * Should there be a shortcut for setting/accessing attributes in a namespace +# based on the cookbook name? + +# Single nested attr assignment + +## Array path option +default_attr(["mysql", "version"], "1.2.3") + +## Multi-arg element assignment operator option: + +default_attr["mysql", "version"] = "1.2.3" + +## Method chaining: + +default_attr("mysql", "version").to("1.2.3") + +# Mass Default assignment + +## Block w/ block variable, method chaining +defaults_for("mysql") do |mysql| + mysql.set("version").to("1.2.3") +end + +## Block with instance eval, method chaining + +defaults_for("mysql") do + set("version").to("1.2.3") +end + +## Method missing chef-sugar API: + +namespace 'apache2' do + namespace 'config' do + root '/var/www' + end +end + +# Nested Attr Reads + +## User-defined attributes via node object +node.attr("mysql", "version") + +## Same as above, but DSL delegates `attr` to `node.attr` +attr("mysql", "version") + +## Ohai data: + +node.sys("kernel", "machine") + +## Same as above but with delegation: + +sys("kernel", "machine") + + From 11f6e70f68c8141450e2a8655667828fd7a189b9 Mon Sep 17 00:00:00 2001 From: danielsdeleo Date: Fri, 12 Jun 2015 11:51:15 -0700 Subject: [PATCH 5/5] Add discussion of trainwreck issue to rationale --- new/rfcXXX-new-attributes-api.md | 53 +++++++++++++++++++++++++++++++- 1 file changed, 52 insertions(+), 1 deletion(-) diff --git a/new/rfcXXX-new-attributes-api.md b/new/rfcXXX-new-attributes-api.md index 6992363f..f6f630f5 100644 --- a/new/rfcXXX-new-attributes-api.md +++ b/new/rfcXXX-new-attributes-api.md @@ -165,7 +165,58 @@ orchestration support, and/or improved authorization behaviors. ## Rationale -This section will be updated once the specification is more complete. +### About Trainwreck Errors + +Trainwreck errors are a common problem in Chef, and the appeal of making the +error more descriptive is evident. It would be advantageous to simply improve +the error message without requiring users to migrate to a new attributes API, +but there are several limitatations of Ruby that make this impossible to do +correctly. + +In Ruby, `nil` and `false` are the only "falsey" values. Both of these objects +are "immediate," meaning that there is only one instance of them in the Ruby +VM. If one creates a subclass of `NilClass` or `FalseClass`, instances of that +subclass will _not_ be "falsey." Therefore the return value for fetching a +nonexistent key must be either `nil` or `false` or else code like +`do_something if node["key_that_may_exist"]` will not function correctly. + +Ruby does allow setting instance variables on immediate objects, e.g.: + +```ruby +nil.instance_variable_set(:@foo, "hello") +nil.instance_variable_get(:@foo) +# => "hello" +``` + +Taking advantage of this, combined with monkeypatching NilClass, it's possible +to fix the simplest case of trainwreck error. For example, if `node["a"]` is +`nil`, we could set an instance variable on `nil` to note that the last +attribute we attempted to access was "a", and handle method missing such that +`node["a"]["b"]` would explain which attribute was missing in the error +message. This approach has some unfortunate drawbacks, however. Consider the +following cases: + +```ruby +Hash.new["foo"]["bar"] +``` + +Since there is only one `nil`, this code would trigger the `method_missing` +hack. This could be mitigated by disabling the hack if the special instance +variable is not set; however, there are still cases where this approach would +produce an incorrect error. For example: + +```ruby +unless node["no_value_for_this_key"] + Hash.new["foo"]["bar"] +end +``` + +In this case, the first line would necessarily set the instance variable on +nil, but `nil[]` isn't called until the second line, where the custom error +handling would produce a completely incorrect error message stating that the +code failed because `node["no_value_for_this_key"]` was `nil`. Since many ruby +types implement `[]`, bizarre error messages could be produced in a wide +variety of cases, depending on what type the user expected to have. ## Copyright