(#22191) When a transaction is cancelled, mark remaining resources as skipped#1839
(#22191) When a transaction is cancelled, mark remaining resources as skipped#1839joshcooper wants to merge 5 commits intopuppetlabs:masterfrom
Conversation
This commit adds the ability to pass a cancelled resource handler callback to the traverse method such that if the traversal is cancelled, then the callback is invoked once for each resource that is ready, but not yet evaluated. In addition, the resource is "finished", which unblocks its dependents, so that the callback can handle those as well. While writing tests for this, we found a bug in the main while loop. Previously, the loop was removing the next ready resource, and then checking if the loop had been cancelled. But if the loop was ever cancelled, the removed resource was effectively lost. This commit reverses the order so that we only remove the next ready resource if we intend to evaluate it. Paired-with: Patrick Carlisle <patrick@puppetlabs.com>
Previously, if a transaction was cancelled while processing resources, the report did not contain any information about the skipped resources. This commit modifies the transaction to supply a cancelled_resource_handler callback, which if invoked, will generate a resource status object for the resource, and mark the status as skipped. This commit also updates the test helper to clear the run_status global state before each test. Paired-with: Patrick Carlisle <patrick@puppetlabs.com>
We were calling compile_to_ral twice unnecessarily, presumably a bad merge or copy and paste.
Puppet generates and applies several catalogs during a single run. There is the main (host_config) catalog, either downloaded from the master, or locally applied. However, there are several other internally generated catalogs used to manage internal state, such as file permissions from defaults.rb. Basically anytime `Puppet::Settings.use` is called. Previously, if puppet was asked to stop, `Puppet::Application.stop!`, then it would cancel the current transaction, as well as any future transaction. As a result, we may intentionally cancel the main (host_config) catalog, but then unintentionally skip the transactions that follow, e.g. writing the rrdgraph report. This commit modifies the transaction to only cancel the evaluation loop if we are evaluating the main (host_config) catalog and a stop has been requested. All other internal catalogs will always be processed.
There was a problem hiding this comment.
Hah! That might make our tests faster 👍
Prefer the current American spelling from the past 30 years.
|
CLA signed by all contributors. |
|
I don't suppose there is any way that you could also stamp in some data about why the resource was skipped? Because that would be unbelievably useful: |
|
@cprice404 What would you want to see here? We can provide some incredibly simple information in the first pass, like transaction was stopped, but not really by what. |
|
If there was a way that I could tell that a "skipped" resource was skipped because of a failed dependency as opposed to some other reason, that alone would be very useful. If we could also identify the particular dependency that failed and pass that along, that would be amazing. Would really open a lot of doors for things we could do in the PE UI. May be out of scope for this pull request, but I figured it couldn't hurt to ask since it seemed somewhat related. |
|
@cprice404 Currently, when we mark a resource as skipped, the report resource status looks like: Notify[after]: !ruby/object:Puppet::Resource::Status
resource: Notify[after]
file: "C:/work/modules/puppetlabs-reboot/multi.pp"
line: 3
change_count: 0
out_of_sync_count: 0
tags:
- notify
- after
- class
time: 2013-08-16 09:32:35.366326 -07:00
events: []
out_of_sync: false
changed: false
resource_type: Notify
title: after
skipped: true
failed: false
containment_path:
- Stage[main]
- ""
- Notify[after]
So it only contains skipped: true but no reason why. I'd think we'd want to add an event to explain that, but I think that's outside the scope of this PR. |
|
@joshcooper right. I'm dealing with those skips a lot in puppetdb, and looking forward to a time when I can do something more meaningful with them. Totally understand if that's out of scope for now, though. Just thought I'd ask :) |
There was a problem hiding this comment.
Won't this flood the logs with the same debug message again and again? It seems like it should indicate which resource is being skipped.
There was a problem hiding this comment.
debug is being called on the resource
|
Rebased (and ended up skipping the fixup of the spec helper code because that was fixed in another commit) and merged as 3f666c2 |
|
Awesome! 👍 |
6 participants
Previously, if puppet was asked to stop prior to completing the transaction(s), it would omit the remaining resources from the report, which gave the impression that it applied a partial catalog.
These commits modify the transaction to mark those resources as skipped. It also fixes a bug in the transaction loop that could cause a resource (and its dependents) to be "lost" (neither ready, overly deferred or cancelled).