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

Clear cached provider properties after transition #8

Merged

Conversation

Sharpie
Copy link
Member

@Sharpie Sharpie commented Apr 5, 2017

A transition resource does the equivalent of an out-of-band
puppet resource ... operation. This means that the catalog will end up
containg a resource with a stale state if pre-fetching was triggered before
the transition. An example of this is a package resource in the catalot that
the agent thinks is present, because it was when pre-fetch happened, but is
actually absent because of a Transition. This patch clears cached state from
the catalog resource so that it will be re-fetched when the resource
is evaluated.

@natemccurdy
Copy link

I had an AIX 7.1 box that was failing to upgrade the puppet-agent package via the puppet_agent module, and this totally fixed it 👍

@@ -34,6 +34,10 @@ def transition

# TODO: Find a better way to log the results ???

# Clean cached state from the catalog resource since it was just
# altered.
catalog_resource.provider.instance_variable_set(:@property_hash, {})
Copy link
Member Author

Choose a reason for hiding this comment

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

I'm not the biggest fan of this since it does manipulate an internal instance variable. But, it seems like the only approach that will preserve existing references to the provider instance.

Copy link

Choose a reason for hiding this comment

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

Is there another way that you could use set to achieve the same goal?

https://github.com/puppetlabs/puppet/blob/master/lib/puppet/provider.rb#L535-L544

Copy link
Member Author

Choose a reason for hiding this comment

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

I looked at set, but the problem there is that it can update properties, but not remove them. So, set can't be used to purge stale data in order to force a re-load.

@geoffnichols
Copy link

Ping @puppetlabs/modules for review.

@Sharpie
Copy link
Member Author

Sharpie commented Apr 10, 2017

@puppetlabs/modules Anything else needed on this one, or is it good for a merge+release? AIX upgrades are completely broken without something to address the pre-fetched state issue.

@Sharpie
Copy link
Member Author

Sharpie commented Apr 12, 2017

After chatting with Hunner a bit, we've decided to see if re-triggering prefetch for a single resource would work using something like:

unless catalog_resource.provider.nil?
  provider_class = catalog_resource.provider.class
  provider_class.prefetch({ catalog_resource.name => catalog_resource}) if provider_class.respond_to?(:prefetch)
end

This would refresh the prefetched state using the same process that initially loaded it. I'll see if this approach is workable and update the PR.

A transition resource does the equivalent of an out-of-band
`puppet resource ...` operation. This means that the catalog will end up
containg a resource with a stale state if pre-fetching was triggered before
the transition. An example of this is a package resource in the catalot that
the agent thinks is present, because it was when pre-fetch happened, but is
actually absent because of a Transition. This patch clears cached state from
the catalog resource so that it will be re-fetched when the resource
is evaluated.
@Sharpie Sharpie force-pushed the modules-4656-clear-catalog-resource-state branch from 85a5526 to 1d17e9c Compare April 12, 2017 18:46
@Sharpie
Copy link
Member Author

Sharpie commented Apr 12, 2017

PR updated with a more robust implementation that uses prefetch.

@geoffnichols
Copy link

ping @puppetlabs/modules for another review after @Sharpie's update

Copy link
Contributor

@hunner hunner left a comment

Choose a reason for hiding this comment

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

This looks like it should handle standard and prefetching resources for ensuring present, changing properties, and ensuring absent. 👍

Copy link
Contributor

@reidmv reidmv left a comment

Choose a reason for hiding this comment

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

This looks good to me. I haven't dived into prefetch internals to know if there are any booby traps that come from re-fetching state, but I'm 👍 to the concept.

@eputnam eputnam merged commit 48c111c into puppetlabs:master Apr 13, 2017
@abottchen
Copy link

This patch also just resolved an issue with transition upgrading Splunk on Solaris.

@Sharpie Sharpie deleted the modules-4656-clear-catalog-resource-state branch April 14, 2017 19:19
natemccurdy added a commit to natemccurdy/puppetlabs-puppet_agent that referenced this pull request Apr 19, 2017
This increases the minimum version requirement for the
puppetlabs-transition module from 0.1.0 to 0.1.1.

There was a very important bug fix in 0.1.1 that allows puppet-agent
to be upgraded on AIX servers. Prior versions were unable to
automatically update puppet-agent due to inconsistent state caching.

See: puppetlabs/puppetlabs-transition#8
natemccurdy added a commit to natemccurdy/puppetlabs-puppet_agent that referenced this pull request May 9, 2017
This increases the minimum version requirement for the
puppetlabs-transition module from 0.1.0 to 0.1.1.

There was a very important bug fix in 0.1.1 that allows puppet-agent
to be upgraded on AIX servers. Prior versions were unable to
automatically update puppet-agent due to inconsistent state caching.

See: puppetlabs/puppetlabs-transition#8
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

10 participants