don't mutate the new resource

[lamont-granquist]

this line violates the rule that we never mutate the new resource.

in Chef 12 this causes real problems because if a file resource is
notified (run twice) then the rendered content is loaded into the
new resource and if the file resource content changes (i.e. its a
template with logic which changes when its notified) then it will
cause a failure because the rendered content will not match the
'requested' checksum. where the 'requested' checksum is actually
the checksum loaded by this line.

fundamentally we have three different states that we're trying to
track:

• current state
• initial state
• requested state

the removed line tries to use the @new_resource for reporting initial
state, which then bleeds over into what chef thinks is requested state
in the next invokation.

without constructing a third @initial_resource and using that for
resource reporting comparison we can't solve this problem "right".

the tradeoff is we either break reporting here or break chef-client
runs. this patch sacrifices reporting in order to make chef-client
work.

fixes #3168

[lamont-granquist]

@chef/client-engineers @chef/client-core assuming travis passes this needs review.

this is a good example of the 'whys' behind the principle that we should never be updating the new resource from within the provider (in this case because notifications may invoke the the resource again and we wind up mutating what was 'requested').

this might be an API breaking change to reporting but we have three options:

1. break reporting of checksum diffs (this change)
2. leave this broken behavior in the chef-client (do nothing)
3. do a fairly epic change to introduce a third resource associated with providers that tracks initial state and replaces the new_resource for the purposes of reporting.

that last one should ideally get done, but its both a very large change and potentially breaking.

[jaymzh]

well... this is a bug we run into often. Thanks for tracking it down.

Also... I imagine this will break some very serious assumptions.

Also, we already do this a LOT in Chef::Provider::Package::Yum

[danielsdeleo]

So, a compromise: use a different parameter name for content checksums that has no meaning in the file resource DSL for reporting purposes, patch the resource reporter to use that.

[lamont-granquist]

That's a little hacky, but I guess its on par with how hacky it is now...

[lamont-granquist]

and @jaymzh now you know why that's bad and can look at fixing it before it turns into bug reports...

[danielsdeleo]

That's a little hacky, but I guess its on par with how hacky it is now...

IMO, there's a ton of places where the resource object does a lot more than describe the desired final state of a component, e.g., specifying the path to a package installed from disk, setting start/stop/restart commands on a service, etc. all describe how not what and conceptually are more like provider customizations. But we deal with it because the other options are not pragmatic. What's one more hack between friends? 😎

[lamont-granquist]

Well, the new_resource is the user-requested state, though. When you mutate that, you lose the accurate intent of the user, which is what causes these issues when it gets reused and you've lost that accurate intent. Which is different from strictly declarative state.

[lamont-granquist]

@danielsdeleo what do you think about this? I went ahead and solved the Chef::Resource#state problem by just renaming the method, then i can override it in the File resource class.

[jaym]

that's cool

[danielsdeleo]

Not the most heartwarming code, but fixes the bug and doesn't break anything else (AFICT). 👍

[danielsdeleo]

Just to be clear, my comment, "Not the most heartwarming code" is referring to the hacky solution we have to put up with because of the constraints mentioned in this thread.

[lamont-granquist]

Lol, yep.

@chef/client-core @chef/client-engineers should be ready for review.

[jaymzh]

LGTM
👍

There's other resources that use file though (like cookbook_file and template)... I looked through the code and I don't see anything that would be affected by these changes.. but something to think about.

[lamont-granquist]

Everything inherits so it should be all good. The if managing_content? bit is already a hook that gets overridden by those resources to define what semantics are for if they're dropping content or not so that logic is already protected against inheritance.

[lamont-granquist]

And I just checked for the use of new_resource.checksum and its either used in managing_content? or its used in current_resource_matches_target_checksum? in the remote_file/content.rb code which both happen before the code path which changed. I don't see any consumers of that who execute after the code path that changed that would need to be changed to use new_resource.final_checksum.

[btm]

I was looking for something in Travis and saw this failed in master after being merged (and running the integration tests that aren't run on PRs). But I can't tell that it was related, could be an intermittent failure? Gotta pay attention to something else right now.

https://travis-ci.org/chef/chef/jobs/60869991

[lamont-granquist]

looks like travis can't run tests against PRs after they're merged or something which is why there's a red x here. the failure on master looks like some unrelated networking related hiccup.

[btm]

The travis failure on master is unrelated, but it's not a hiccup. All of the builds on master (which run integration tests) have been failing for around three weeks. I've described it over here: #3323