Handle errors from cluster_status #3735
Draft
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
kgaillot
reviewed
Nov 18, 2024
Use pcmk_unpack_scheduler_input instead.
… fails. This function can return a couple error codes, most notably when called on input with a feature set that is newer than the latest supported. In that case, the caller should return its own error instead f trying to continue on with an unpopulated scheduler object. This prevents a cascade of error messages.
Also, there's no need to do any error reporting. pcmk__config_err will have already called crm_err in this case.
The error message is hidden and only gets displayed if -V is given on the command line. Adding config error/warning handlers will cause the error to be displayed regardless. This could have been implemented in a couple ways, and there's tradeoffs here. I've chosen to duplicate what's happening in crm_verify, but instead of checking for verbosity (which is a global variable in that file), I'm checking out->is_quiet. This means that if you do `crm_simulate -Q`, you won't see the error message but you will get an error return code. This also means that `crm_simulate -Q -VVVV...`, you still won't see the error message. This may be a bug, but I'm not sure who would do that and I also think these sorts of problems are pervasive in our command line tools. Fix T521
This is just like the previous patch for crm_simulate, complete with all the same problems regarding -Q and -V.
This is just like the previous patch to crm_simulate. However, one additional problem here is that it relies on using the deprecated -Q command line option. On the other hand, I think this is okay because we have a lot of work to do straightening out these sorts of options for all our command line tools. This is just one more thing we'll have to deal with at that time.
This takes care of all callers of pcmk__output_cluster_status and pcmk__status. pcmk_status would also be affected, but at the moment there are no users of that function and anyway the config error handlers aren't public API.
The point of this is to allow it to return the value from unpack_cib, which is returning the value from cluster_status. This allows us to check whether that function hit the too-new feature set CIB condition.
…tions. This takes care of most callers - the ones in the daemons are unlikely to be a problem. This allows catching the too-new schema condition in various other tools and displaying an error message to the user. Note that a couple other callers don't need to check the return value. I've added comments explaining why.
* Remove the leading function name from various messages. This was most commonly "unpack_resources". * In the XML output format, move various messages from text that gets printed out to the XML output itself. This does end up with somewhat weird output with status="0" message="OK" followed by some error messages. * Add a couple warnings to crm_resource output.
clumens
force-pushed
the
cluster_status-errors
branch
from
07e8301 to
b4f4b75
Compare
kgaillot
reviewed
Dec 3, 2024
|
|
||
| va_start(ap, msg); | ||
| pcmk__assert(vasprintf(&buf, msg, ap) > 0); | ||
| if (!out->is_quiet(out)) { |
There was a problem hiding this comment.
might as well just un-deprecate -Q
| @@ -1730,6 +1757,7 @@ WARNING: Creating rsc_location constraint 'cli-ban-dummy-on-node1' with a score | |||
| =#=#=#= End test: Move a resource from its existing location - OK (0) =#=#=#= | |||
| * Passed: crm_resource - Move a resource from its existing location | |||
| =#=#=#= Begin test: Clear out constraints generated by --move =#=#=#= | |||
| warning: More than one node entry has name 'node1' | |||
There was a problem hiding this comment.
Doesn't look like it, though I don't yet see what could be causing that.
| @@ -1,3 +1,6 @@ | |||
| error: Ignoring invalid node_state entry without id | |||
| warning: Ignoring failure timeout (10s) for rsc_pcmk-2 because it conflicts with on-fail=block | |||
| warning: Ignoring failure timeout (10s) for rsc_pcmk-4 because it conflicts with on-fail=block | |||
There was a problem hiding this comment.
Most of the errors/warnings in the test cases are things we really should fix in the test case :(
(some may be testing the error though)
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
I'm a little undecided on this patch at the moment - check out the changes to regression test output in the last patch. I think some of that could be mitigated by not printing the warnings at all. The errors are a little trickier. We want to print them out if we're not verbose (that's the entire point of the related issue) but that means we get all of them.