Resources, Providers and Runners, oh my - A tale of concerns


#1

Hi all,

I’m trying to wrap my head around something, and I think I’m a bit
stuck. While working on CHEF-606 [1] (which was a direct result of
trying to fix CHEF-605 [2]) I ran across something I can only describe
as mixed concerns.

Chef::Runner has a similar functionality as Chef::Resource, they both
have a run_action method, but other than I expected, Runner#run_action
doesn’t invoke Resource#run_action, it pretty much duplicates its
functionality. The difference seems to be that when the Runner builds
a provider instance it hands in some of its own instance variables
(@collection, @definitions, @cookbook_loader) which the Resource knows
nothing about, but Providers do. The Resource on the other hand only
builds Providers without references to these, and its run_action
method seems only to be used when running Resources directly from
Recipes.

That’s where the concerns start to fall apart for me. I’d argue that
the Runner shouldn’t have to know anything about Providers, especially
when all the logic dealing with them is already implemented in the
Resource. But there’s cases where the mentioned instance variables are
expected to be set. That code seems to be mostly dealing with
lightweight resources, as they are the only tests that failed. The
code in RecipeDefinitionDSLCore references them, as far as I could
tell it’s used to create ad-hoc Recipes or something like that,
correct me if I’m wrong.

If I consider this a chain, where the Runner only knows about
Resources, and the Resources are the only spot where the code deals
with invoking Providers, there’s a blind spot in the middle. Providers
need access to objects the Resource doesn’t know anything about. The
question is if the Resource should know them, or if it should just
hand them through. We started to move some code around, and play with
some ideas, as can be seen in [3], but we’re not happy with it. It
basically hands the required objects into the Resource where they’re
just handed over to the Provider. The Resource though comes with its
own instance variable @collection which doesn’t seem to have any
relation to the one the Provider refers to.

I’m not exactly sure how to proceed on this, so I’m asking for some
input, because maybe we missed something along the way, or maybe
someone has other ideas how that can be fixed.

For me, putting all the code that deals with running an action and
notifying subscribed actions belongs into the Resource, it just seems
to be the right place for it. But the fact that there’s a mix of
concerns here makes me think there’s a bigger issue here.

Let me know, what you think.

Thanks!

Cheers, Mathias
[1] https://tickets.opscode.com/browse/CHEF-606
[2] https://tickets.opscode.com/browse/CHEF-605
[3] http://github.com/peritor/chef/tree/CHEF-606-WIP

http://paperplanes.de | http://holgarific.net
http://twitter.com/roidrage


#2

Adam originally wrote that code, and I’d like to wait for him to chime
in. He’ll be out of town for a couple more days, so I wanted to make
sure someone responded now to let you know someone will follow up with
real substance in a day or two.

I agree that a clean separation of concerns makes sense, but I’m
loathe to assert too quickly without Adam giving us his reasons behind
the current form.

Cheers,
Chris

On Mon, Oct 12, 2009 at 11:50 AM, Mathias Meyer meyer@paperplanes.de wrote:

Hi all,

I’m trying to wrap my head around something, and I think I’m a bit
stuck. While working on CHEF-606 [1] (which was a direct result of
trying to fix CHEF-605 [2]) I ran across something I can only describe
as mixed concerns.

Chef::Runner has a similar functionality as Chef::Resource, they both
have a run_action method, but other than I expected, Runner#run_action
doesn’t invoke Resource#run_action, it pretty much duplicates its
functionality. The difference seems to be that when the Runner builds
a provider instance it hands in some of its own instance variables
(@collection, @definitions, @cookbook_loader) which the Resource knows
nothing about, but Providers do. The Resource on the other hand only
builds Providers without references to these, and its run_action
method seems only to be used when running Resources directly from
Recipes.

That’s where the concerns start to fall apart for me. I’d argue that
the Runner shouldn’t have to know anything about Providers, especially
when all the logic dealing with them is already implemented in the
Resource. But there’s cases where the mentioned instance variables are
expected to be set. That code seems to be mostly dealing with
lightweight resources, as they are the only tests that failed. The
code in RecipeDefinitionDSLCore references them, as far as I could
tell it’s used to create ad-hoc Recipes or something like that,
correct me if I’m wrong.

If I consider this a chain, where the Runner only knows about
Resources, and the Resources are the only spot where the code deals
with invoking Providers, there’s a blind spot in the middle. Providers
need access to objects the Resource doesn’t know anything about. The
question is if the Resource should know them, or if it should just
hand them through. We started to move some code around, and play with
some ideas, as can be seen in [3], but we’re not happy with it. It
basically hands the required objects into the Resource where they’re
just handed over to the Provider. The Resource though comes with its
own instance variable @collection which doesn’t seem to have any
relation to the one the Provider refers to.

I’m not exactly sure how to proceed on this, so I’m asking for some
input, because maybe we missed something along the way, or maybe
someone has other ideas how that can be fixed.

For me, putting all the code that deals with running an action and
notifying subscribed actions belongs into the Resource, it just seems
to be the right place for it. But the fact that there’s a mix of
concerns here makes me think there’s a bigger issue here.

Let me know, what you think.

Thanks!

Cheers, Mathias
[1] https://tickets.opscode.com/browse/CHEF-606
[2] https://tickets.opscode.com/browse/CHEF-605
[3] http://github.com/peritor/chef/tree/CHEF-606-WIP

http://paperplanes.de | http://holgarific.net
http://twitter.com/roidrage


Christopher Brown, VP of Engineering
Opscode, Inc.
T: (425) 281-0727, E: cb@opscode.com
Twitter, IRC, Github: skeptomai


#3

Ohai Chefs!
Mathias,
To follow up on our conversation on IRC today, what you’re seeing in the
LWRP spec failures results from the resources being initialized differently
in the specs than they would be if created via the DSL. In the DSL, you see
resources initialized this way:

args << @collection
args << @node
resource = Chef::Resource.const_get(resource_camel_case_name).new(*args)

But in the failing specs, you see this:

Chef::Resource::LwrpFoo.new(“morpheus”)

When you moved Runner#build_provider to Resource#build_provider, it became
necessary for the specs to pass the resource collection to the resources
when initializing. So the issue can be fixed by changing the specs to more
closely match the behavior that would occur in a real chef run, for example:

rc = Chef::ResourceCollection.new
injector = Chef::Resource::LwrpFoo.new(“morpheus”, rc)

By the way, should you encounter errors due to @definitions being undefined
somewhere, there are fixes for that in the works, so I’d recommend waiting
for those fixes to hit master so we’re not stepping on each others’ toes.

As far as cleanly separating responsibilities, I’m all for it, and I’d say
the amount of effort required to understand the code as it is now is good
evidence that the effort is worthwhile.

HTH,
Dan DeLeo/kallistec

On Mon, Oct 12, 2009 at 4:38 PM, Christopher Brown cb@opscode.com wrote:

Adam originally wrote that code, and I’d like to wait for him to chime
in. He’ll be out of town for a couple more days, so I wanted to make
sure someone responded now to let you know someone will follow up with
real substance in a day or two.

I agree that a clean separation of concerns makes sense, but I’m
loathe to assert too quickly without Adam giving us his reasons behind
the current form.

Cheers,
Chris

On Mon, Oct 12, 2009 at 11:50 AM, Mathias Meyer meyer@paperplanes.de
wrote:

Hi all,

I’m trying to wrap my head around something, and I think I’m a bit
stuck. While working on CHEF-606 [1] (which was a direct result of
trying to fix CHEF-605 [2]) I ran across something I can only describe
as mixed concerns.

Chef::Runner has a similar functionality as Chef::Resource, they both
have a run_action method, but other than I expected, Runner#run_action
doesn’t invoke Resource#run_action, it pretty much duplicates its
functionality. The difference seems to be that when the Runner builds
a provider instance it hands in some of its own instance variables
(@collection, @definitions, @cookbook_loader) which the Resource knows
nothing about, but Providers do. The Resource on the other hand only
builds Providers without references to these, and its run_action
method seems only to be used when running Resources directly from
Recipes.

That’s where the concerns start to fall apart for me. I’d argue that
the Runner shouldn’t have to know anything about Providers, especially
when all the logic dealing with them is already implemented in the
Resource. But there’s cases where the mentioned instance variables are
expected to be set. That code seems to be mostly dealing with
lightweight resources, as they are the only tests that failed. The
code in RecipeDefinitionDSLCore references them, as far as I could
tell it’s used to create ad-hoc Recipes or something like that,
correct me if I’m wrong.

If I consider this a chain, where the Runner only knows about
Resources, and the Resources are the only spot where the code deals
with invoking Providers, there’s a blind spot in the middle. Providers
need access to objects the Resource doesn’t know anything about. The
question is if the Resource should know them, or if it should just
hand them through. We started to move some code around, and play with
some ideas, as can be seen in [3], but we’re not happy with it. It
basically hands the required objects into the Resource where they’re
just handed over to the Provider. The Resource though comes with its
own instance variable @collection which doesn’t seem to have any
relation to the one the Provider refers to.

I’m not exactly sure how to proceed on this, so I’m asking for some
input, because maybe we missed something along the way, or maybe
someone has other ideas how that can be fixed.

For me, putting all the code that deals with running an action and
notifying subscribed actions belongs into the Resource, it just seems
to be the right place for it. But the fact that there’s a mix of
concerns here makes me think there’s a bigger issue here.

Let me know, what you think.

Thanks!

Cheers, Mathias
[1] https://tickets.opscode.com/browse/CHEF-606
[2] https://tickets.opscode.com/browse/CHEF-605
[3] http://github.com/peritor/chef/tree/CHEF-606-WIP

http://paperplanes.de | http://holgarific.net
http://twitter.com/roidrage


Christopher Brown, VP of Engineering
Opscode, Inc.
T: (425) 281-0727, E: cb@opscode.com
Twitter, IRC, Github: skeptomai


#4

On Tue, Oct 13, 2009 at 3:18 AM, Daniel DeLeo dan@kallistec.com wrote:

Ohai Chefs!
Mathias,
To follow up on our conversation on IRC today, what you’re seeing in the
LWRP spec failures results from the resources being initialized differently
in the specs than they would be if created via the DSL. In the DSL, you see
resources initialized this way:
args << @collection
args << @node
resource = Chef::Resource.const_get(resource_camel_case_name).new(*args)
But in the failing specs, you see this:
Chef::Resource::LwrpFoo.new(“morpheus”)

When you moved Runner#build_provider to Resource#build_provider, it became
necessary for the specs to pass the resource collection to the resources
when initializing. So the issue can be fixed by changing the specs to more
closely match the behavior that would occur in a real chef run, for example:
rc = Chef::ResourceCollection.new
injector = Chef::Resource::LwrpFoo.new(“morpheus”, rc)
By the way, should you encounter errors due to @definitions being undefined
somewhere, there are fixes for that in the works, so I’d recommend waiting
for those fixes to hit master so we’re not stepping on each others’ toes.
As far as cleanly separating responsibilities, I’m all for it, and I’d say
the amount of effort required to understand the code as it is now is good
evidence that the effort is worthwhile.

All that sounds great, Daniel, and it looks like we’re halfway there
with these fixes and the potentially broken specs. We talked about it
a bit more two days ago, and the last remaining problem apart from the
things you mentioned above was basically the cookbook_loader. In the
DSL core that’s handed over to a Chef::Recipe instance where it’s used
for include_recipe.

The definitions issue was apparently fixed with [1], and Daniel
improved quite a lot of the code that’s depending on @definitions in
his methodnotmissing branch [2].

As far as what I learned during our chat was that the cookbook_loader
could easily be created lazily, maybe not even only in this case, but
whenever it’s required. I just noticed that Daniel already changed it
to work this way in his branch [3].

So to sum up, it seems that most of the underlying issues were already
fixed, awesome!

The question is, how do they get in. Daniel mentioned they’re too big
for a 0.7.12 release, personally I’d love to see them in the release
after that if possible. I’ll play around with Daniel’s changes on our
fork to see how they work with the changes we made for CHEF-606 and
CHEF_605.

If I forgot anything, please feel free to add it.

Thanks, guys!

Cheers, Mathias

[1] http://github.com/opscode/chef/commit/03214fa3eae3f5ccfdceb405fd66660566b005fb
[2] http://github.com/danielsdeleo/chef/commits/methodnotmissing
[3] http://github.com/danielsdeleo/chef/commit/f0d6bf123632723ac7fb36c0f66b42f0fb8c75d1

http://paperplanes.de | http://holgarific.net
http://twitter.com/roidrage