Fixes #4323 - Fix wrong result classes of service_ensure_stopped which are based on service_check_running() #26
Conversation
| @@ -42,10 +41,10 @@ bundle agent service_ensure_stopped(service_name) | |||
| ifvarclass => "service_check_running_${canonified_service_name}_ok"; | |||
|
|
|||
| "placeholder" | |||
| usebundle => _classes_copy("service_check_running_${canonified_service_name}", "service_ensure_stopped_${canonified_service_name}"); | |||
| usebundle => _classes_inverted_copy("service_check_running_${canonified_service_name}", "service_ensure_stopped_${canonified_service_name}"); | |||
There was a problem hiding this comment.
Note to self: we can't merge this unil the _classes_inverted_copy PR is merged (see #23).
Then, we will have to adapt this for the new name for that bundle.
|
Rebased from master and renamed from "_classes_inverted_copy" to ''_classes_copy_invert_kept_repaired" |
| @@ -42,10 +42,10 @@ bundle agent service_ensure_stopped(service_name) | |||
| ifvarclass => "service_check_running_${canonified_service_name}_ok"; | |||
|
|
|||
| "class copy check" | |||
| usebundle => _classes_copy("service_check_running_${canonified_service_name}", "${class_prefix}"); | |||
| usebundle => _classes_copy_invert_kept_repaired("service_check_running_${canonified_service_name}", "${class_prefix"); | |||
There was a problem hiding this comment.
I realize this was a pre-existing bug, but I want to discuss it now as I just saw it, and this may compromise this bundle: surely we should only do this _classes_copy if we have !service_check_running_${canonified_service_name}_ok?
Otherwise, in the case that !service_check_running_${canonified_service_name}_ok, there are two _classes_copy to the same destination class_prefix... That's got to be messed up, right?
There was a problem hiding this comment.
After thinking about all of this, it appears to me that:
service_stopwill only create repaired classes and never kept- the conditions
service_check_running_${canonified_service_name}_okare wrong as it will call class copy twice as you noticed and this is no good.
The logic should also be:
- if
service_check_running_${canonified_service_name}_repairedthen no process exist, then we need to invert result class from repaired to kept - Otherwise if
service_check_running_${canonified_service_name}_keptso some process(es) exist, and we will need to callservice_stop. Then we will have to copy its result class repaired
Do you agree ?
There was a problem hiding this comment.
I agree with your logic.
I had never seen the service_check_running implementation, and am rather surprised that it only returns $(prefix)_{kept,repaired}. It doesn't seem right to me that "check running not ok" equates to "repaired", since no actions were taken. I suggest that we change this behaviour (probably to use {kept,error} instead, and hopefully to have all appropriate classes included prefixed ones, etc).
Therefore, I suggest that you change the current PR using only the service_check_running_${canonified_service_name}_ok class (and of course !service_check_running_${canonified_service_name}_ok when necessary).
There was a problem hiding this comment.
Therefore, I suggest that you change the current PR using only the service_check_running_${canonified_service_name}ok class (and of course !service_check_running${canonified_service_name}_ok when necessary).
Ok, so if I understand well, I'll have to make a new PR to change the behavior of service_check_running in order to have an error instead of repaired after this PR ?
There was a problem hiding this comment.
It would be good to open a bug at least, yes please.
Also I now realise I gave you the wrong class suffix in my previous comment, it should be _kept, not _Ok of course. Your acceptance tests should detect this.
|
Aside from that one comment, this looks great. |
|
Actually, I just noticed that your commit also fixes http://www.rudder-project.org/redmine/issues/4322. Could you please include that in the commit? (Fixes #4322) |
…change bundle _classes_inverted_copy() to handle error classes
…o '_classes_copy_invert_kept_repaired.cf', fix some comments, enhance tests and factorize code for class inversion
…version table and update description
…nd enhance its acceptance tests
…reate_symlink_enforce' and add two wrapper bundles 'file_create_symlink' and 'file_create_symlink_force'
…and 'file_create_symlink_force'
…valid_wrong_enforce_arg.cf'
…d which are based on service_check_running() and remove useless variable about init.d command
…ist and copy class of 'service_stop' if process was detected.
|
Argg..i wanted to rebase the PR but i've completly broken it. |
|
This PR is broken, i've opened another one: #29 |
Fixes #4323 - Fix wrong result classes of service_ensure_stopped which are based on service_check_running()
cf http://www.rudder-project.org/redmine/issues/4323