On Wed, Jul 22, 2015 at 1:37 PM, Branan Riley <[email protected]> wrote:
> On Wed, Jul 22, 2015 at 12:44 PM Kylo Ginsberg <[email protected]> > wrote: > >> On Tue, Jul 21, 2015 at 12:40 PM, Branan Riley <[email protected]> >> wrote: >> >>> <This is email 2/2 inspired by PUP-4813> >>> >>> With puppet 4, we made a change to how prefetch failures are handled >>> by the transaction[1]. This is actually a pretty good change, as it >>> prevents puppet from some nasty misbehaviors. >>> >>> I still don't think it's the "correct" behavior, though. Aborting the >>> entire catalog because one resource type is broken doesn't seem like >>> the best behavior, especially when it comes to reporting[2] >>> >>> I feel like the correct behavior is something like this: >>> >>> * If an exception escapes a prefetch, mark all resources of that type >>> in the catalog as failed and do not try to apply them >>> >> >> Background for a question: >> >> Wrt exceptions in prefetch, as of 4.0 there is a distinction between the >> expected/supported exceptions (there are two: Puppet::MissingCommand and >> LoadError) and everything else. >> >> I'm not sure this is *super* well documented, but it is mentioned as a >> breaking change in the 4.0 release notes: >> >> >> https://docs.puppetlabs.com/puppet/4.0/reference/release_notes.html#stuff-a-lot-of-people-will-notice >> >> and the rescue in question is here: >> >> >> https://github.com/puppetlabs/puppet/blob/810624fe6f88d520339173c0e811c5ede4009fac/lib/puppet/transaction.rb#L322 >> >> So with that in mind, is the proposal here about the handling of the two >> supported exceptions or the handling of other exceptions? >> > >> For the two supported exceptions, this seems like a reasonable behavior >> change and could also speed up catalog application in this failure case. >> > >> For *other* exceptions, I'm not sure. I walked away from PUP-3656 >> thinking that those two exceptions were now part of the contract we're >> specifying for providers. If so, a provider that throws any *other* >> exception has a bug which should be fixed, and we could be making matters >> worse by trying to soldier on. I.e. this seems like a situation where it's >> better to fail hard and the user should file a bug, upgrade/downgrade the >> module, etc. >> > > I would say this should be for ALL exceptions. We absolutely should stop > puppet from soldiering on in ways that could cause problems, but I don't > think trying to apply other resource types in the catalog is one of those > ways. Prefetch failure should be a localized catastrophe, not a global one. > > The move away from rescuing everything was definitely good, but aborting > the entire transaction when only a single type is unprefetchable is a bit > of an over-reaction. We know that instances of that type (really just ones > of that type/provider pair) are bad, and we absolutely shouldn't apply > them. The rest of the catalog is still fine, though. > > The two exceptions that are supported allow a Puppet run to install any > packages/gems that a provider might need in order to prefetch. Skipping > only the resources that would have been prefetched instead of the entire > catalog serves that same purpose. > I can see this argument. I'm still concerned that this would mean we aren't surfacing bugs that really should be exposed to the light of day. I suppose we could report the one case with a Warning and the other with an Error. But assuming we go ahead and do something like this (I see I'm the only one with the slightest reservation :)), I'm thinking we should audit how puppet handles exceptions in providers during catalog application but *outside* prefetch. It would be nice to be consistent throughout catalog application. > Aborting the entire catalog also makes remediation with Puppet (when > puppet could be used for such) impossible, which seems super bad to me. > Is it impossible? Couldn't you remediate with a catalog that doesn't include resources that will tickle prefetch from that provider? Not trying to quibble (i.e. yes, that could still be a PITA), but making sure I'm not missing something. > > >> Btw (and perhaps I should have asked this from the get-go before opining >> above): what was the exception in PUP-4813 that precipitated this thread? >> > > The bad catalog from that ticket set the `target` field of mount to a > directory, which causes some sort of IO exception to be raised when > ParsedFile tries to access that directory as a regular file. > Ooh fun. So we should fix that to be more robust. Separate ticket than this thread though. > > This is what inspired my bullet point below - ParsedFile should be smart > enough to know which resources have a bad target and which are OK, and > appropriately fail only the ones it can't work with. > > >> >>> * Make it possible to fail resources from *within* a prefetch. This >>> lets a sufficiently smart prefetch handle the case when only some of >>> its resources are hosed (parsedfile resources with multiple targets, >>> for example, might be only partially unfetchable) >>> >> >> How would this work? A new hook for providers to implement? If so, it'd >> be nice to flesh out a couple examples use cases so that we can think >> through the API with something concrete. >> > > It's probably already "doable" by having prefetch mark the returned > provider instances as bad in some way, and the provider implementation for > flush could raise an exception. I'd prefer to see something at the > transaction layer, though. I'll think through what this might look like > more. > Well, definitely some charm to not adding new methods that we have to sprinkle respond_to? tests around for. So the former approach might just fly. I'd also like to think through whether there are additional motivating use cases. Kylo -- Kylo Ginsberg | [email protected] | irc: kylo | twitter: @kylog *PuppetConf 2015 <http://2015.puppetconf.com/> is coming to Portland, Oregon! Join us October 5-9.* *Register now to take advantage of the Early Bird discount <https://www.eventbrite.com/e/puppetconf-2015-october-5-9-tickets-13115894995?discount=EarlyBird> * *—**save $249!* -- You received this message because you are subscribed to the Google Groups "Puppet Developers" group. To unsubscribe from this group and stop receiving emails from it, send an email to [email protected]. To view this discussion on the web visit https://groups.google.com/d/msgid/puppet-dev/CALsUZFHyPv0mGjeE5Xhn-sv0WYXJ2QbDzY802sBe2JJQR%3DLT5g%40mail.gmail.com. For more options, visit https://groups.google.com/d/optout.
