On 23 July 2015 at 03:44, Kylo Ginsberg <k...@puppetlabs.com> wrote: > On Wed, Jul 22, 2015 at 1:37 PM, Branan Riley <bra...@puppetlabs.com> wrote: >> >> On Wed, Jul 22, 2015 at 12:44 PM Kylo Ginsberg <k...@puppetlabs.com> >> wrote: >>> >>> On Tue, Jul 21, 2015 at 12:40 PM, Branan Riley <bra...@puppetlabs.com> >>> 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. >
For some relevant colour we hit this problem hard with the AWS module. We're prefetching a lot of complex data in some cases and we're going it over the wire - so it going wrong is pretty likely. We worked around that at the time by creating our own Exception inherited from Exception which does get caught by Puppet: https://github.com/puppetlabs/puppetlabs-aws/blob/master/lib/puppet_x/puppetlabs/aws.rb#L7-L23 And then raising that in cases of StandardError based exceptions being raised in prefetch. https://github.com/puppetlabs/puppetlabs-aws/blob/master/lib/puppet/provider/ec2_instance/v2.rb#L34-L36 Some more details in this issue too about the changes in 4.0. https://jira.puppetlabs.com/browse/PUP-3656 Gareth > Kylo > > -- > Kylo Ginsberg | k...@puppetlabs.com | irc: kylo | twitter: @kylog > > PuppetConf 2015 is coming to Portland, Oregon! Join us October 5-9. > Register now to take advantage of the Early Bird discount —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 puppet-dev+unsubscr...@googlegroups.com. > 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. -- Gareth Rushgrove @garethr devopsweekly.com morethanseven.net garethrushgrove.com -- 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 puppet-dev+unsubscr...@googlegroups.com. To view this discussion on the web visit https://groups.google.com/d/msgid/puppet-dev/CAFi_6y%2BxV9Gk5f_C7S4%2BQ6Oxyk5JmdNsRY%3D2XchPTRqa29ywBQ%40mail.gmail.com. For more options, visit https://groups.google.com/d/optout.
