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. Aborting the entire catalog also makes remediation with Puppet (when puppet could be used for such) impossible, which seems super bad to me. > 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. 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. > > >> If there's agreement on this I'll file the appropriate tickets. If >> there are other ideas of how we should handle prefetch failures I'd >> also love to hear them. >> > > Thanks for bringing these issues up! Sometimes innocuous looking tickets > bloom into huge "how did this ever work?" conversations and this is kinda > one of those :) > > Kylo > > >> >> Thanks, >> Branan Riley >> Puppet Labs Software Engineer >> >> [1] PUP-3656 >> [2] See https://groups.google.com/forum/#!topic/puppet-dev/QMTJli2oabc >> >> -- >> 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/CADWDnrmDGO%3DR__fc0YxB4ywcGAbcOnASMKBw%2BPdPusGoG_nbog%40mail.gmail.com >> <https://groups.google.com/d/msgid/puppet-dev/CADWDnrmDGO%3DR__fc0YxB4ywcGAbcOnASMKBw%2BPdPusGoG_nbog%40mail.gmail.com?utm_medium=email&utm_source=footer> >> . >> For more options, visit https://groups.google.com/d/optout. >> > > > > -- > 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/CALsUZFHGNJH1fc1yy%3D8-J-5frY6JqRUHc%2BOe7w30%2B062K%2Bm%2Bjg%40mail.gmail.com > <https://groups.google.com/d/msgid/puppet-dev/CALsUZFHGNJH1fc1yy%3D8-J-5frY6JqRUHc%2BOe7w30%2B062K%2Bm%2Bjg%40mail.gmail.com?utm_medium=email&utm_source=footer> > . > For more options, visit https://groups.google.com/d/optout. > -- 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/CADWDnrkFRow49M1SDm0wnZfC5W%2BePRjvw-S0FwSOcNxQOSaymA%40mail.gmail.com. For more options, visit https://groups.google.com/d/optout.
