Use FQCN for roles where available #1021
Conversation
|
Build succeeded.
|
|
To help review of this change, here is the new test's event data, with everything removed except what I find potentially relevant. [
{
"event": "playbook_on_start",
},
{
"event": "playbook_on_play_start",
},
{
"event": "playbook_on_task_start",
"event_data": {
"task": "debug",
"task_action": "debug",
"role": "groovy.peanuts.hello",
"name": "groovy.peanuts.hello : debug",
"resolved_action": "ansible.builtin.debug"
}
},
{
"event": "runner_on_start",
"event_data": {
"task": "debug",
"task_action": "debug",
"role": "groovy.peanuts.hello",
"host": "testhost",
}
},
{
"event": "runner_on_ok",
"event_data": {
"task": "debug",
"task_action": "debug",
"role": "groovy.peanuts.hello",
"host": "testhost",
}
},
{
"event": "playbook_on_stats",
}
] |
That sounds like a bug in core where the data is not persisting somehow. All references to the task object should have that attribute. I'd file an issue to get it looked at. |
|
@sivel we already discussed the diff to do that diff --git a/ansible_runner/display_callback/callback/awx_display.py b/ansible_runner/display_callback/callback/awx_display.py
index aa29f39..0aee596 100644
--- a/ansible_runner/display_callback/callback/awx_display.py
+++ b/ansible_runner/display_callback/callback/awx_display.py
@@ -409,6 +409,7 @@ class CallbackModule(DefaultCallbackModule):
task=(task.name or task.action),
task_uuid=str(task._uuid),
task_action=task.action,
+ resolved_action=getattr(task, 'resolved_action', ''),
task_args='',
)
try:
@@ -554,7 +555,6 @@ class CallbackModule(DefaultCallbackModule):
name=task.get_name(),
is_conditional=is_conditional,
uuid=task_uuid,
- resolved_action=getattr(task, 'resolved_action', '')
)
with self.capture_event_data('playbook_on_task_start', **event_data):
super(CallbackModule, self).v2_playbook_on_task_start(task, is_conditional)This was probably not done, because it's unclear what the objective is, or what the standards for runner's event_data is. I know it's been said many times that this is a stable API, so not changing long-standing fields (to some reasonable extent) is an objective. I am fudging that in this PR by changing the content of For the overall objective, how should the JSON data I posted above look, with task and role FQCNs? What is If The |
|
I put up #1023 to address the issue of the code coverage decline. |
|
@AlanCoding @sivel task: task name, whatever user writes, e.g. "Install httpd module" task_action: module used, not sure if it can be other resource too, e.g. "yum" (based on what user inputs, so can be "ansible.builtin.yum") resolved_action: the FQCN of resource (we see mainly modules in events), e.g. ansible.builtin.yum role: role name, based on what user inputs, it can be role name, FQCN, path to /tmp/xy/roles/role_x, relative path, etc. Based on the above, it seems it'd be consistent to have My assumption is also that if the content doesn't come from a collection the As for the main usecase, we need a good way to detect:
We should be able to nicely get that by parsing the FQCN attributes, counting on schema always being |
|
This diff changes the role field to be FQCN, and I believe it should work for analytics needs. |
|
@AlanCoding I don't see test on what happens when the role is standalone (not part of a collection). I think with 1 field it might be harder to recognize? Or will it be enough to just say, if the format is So for me, separate field will have more consistency and easier processing. But I am ok with whatever you guys decide, if we can detect role as part of collection, role without collection and no role. |
|
That's right! There's a pre-existing test for a role that's not from a collection. The new test uses a make-believe role from a collection, To make things painfully complicated, I didn't think of the point you raised. It's true, this role name cannot distinguish between a collection and a dotted path (or name). Maybe you were trying to make this point earlier and I wasn't getting it.
I don't think anyone wants to make a decision. Say we add another field for the role FQCN. This probably needs to go in all events that currently have What would we call the new field? |
|
@AlanCoding |
|
Build succeeded.
|
This is beyond the scope of the discussion so far, so I tested it manually. Using a local module in
I was designing the |
|
^ maybe I'm not so confident in that. I have a little bit of hesitancy about adding that change onto this. What we have now for the role case we care about looks like: [
{
"event": "playbook_on_start",
},
{
"event": "playbook_on_play_start",
},
{
"event": "playbook_on_task_start",
"event_data": {
"task": "debug",
"task_action": "debug",
"role": "hello",
"resolved_role": "groovy.peanuts.hello",
"name": "groovy.peanuts.hello : debug",
"resolved_action": "ansible.builtin.debug"
}
},
{
"event": "runner_on_start",
"event_data": {
"task": "debug",
"task_action": "debug",
"role": "hello",
"resolved_role": "groovy.peanuts.hello",
"host": "testhost",
}
},
{
"event": "runner_on_ok",
"event_data": {
"task": "debug",
"task_action": "debug",
"role": "hello",
"resolved_role": "groovy.peanuts.hello",
"host": "testhost",
}
},
{
"event": "playbook_on_stats",
}
] |
|
@AlanCoding I think the Lets chat with @sivel when he's back. Ideally the |
|
I'm a bit behind in here, and was having trouble following what if anything was needed of me. @Ladas provided these topics:
I'd say that is fine. You can still use
I guess this just depends on the goals of how the data will be used. The core code doesn't concern itself with whether
I think this is the right thing to do. I don't think it should only be present in |
|
@sivel right for 3.): One of our usecases is to recognize if the content comes from collection or not. Are you saying we're not able to do that? Or are you saying that matching VALID_FQCR_RE = re.compile(r'^\w+(.\w+){2,}$') guarantees the resource is from a collection? From @AlanCoding comment #1021 (comment), it wasn't clear to me if we're e.g. able to craft standalone role, that would match the regexp. |
|
Technically you could poke As such, there isn't really a straight forward way to know if someone just created If you do poke at |
|
@AlanCoding @sivel Sounds like using private methods that can become backwards incompatible is not a great idea. I think that if we'll put there FQCN in a format I assume the installed_collections sent in ansible/awx@0e80f66, should have a list of all valid collections, so this approach should be viable. |
Is there not? It sounded like you were converging on:
Having both of these, you could get the collection of the role without much more complex references to the list of installed collections. The approach of comparing to installed collections won't event work for my test case because I'm using a collection in source control next to the playbook. My concern about |
|
|
|
Well then, honestly, the current diff of this PR remains my preference unless someone has a highly specific change to request. |
|
@AlanCoding @sivel if there is no other way, I think that checking resolved_role with So I am fine with the current approach. @AlanCoding do I need to file an issue for |
|
@Ladas why don't you subtract the "role" from "resolved_role", from the test case: "role": "hello",
"resolved_role": "groovy.peanuts.hello",Like you're saying, there's no other correct way to do this. In the current implementation, any task that has "resolved_role" will also have "role" (not necessarily the other way around, when a role isn't from a collection).
No, that's currently implemented in this PR. |
| @@ -409,6 +409,7 @@ def set_task(self, task, local=False): | |||
| task=(task.name or task.action), | |||
| task_uuid=str(task._uuid), | |||
| task_action=task.action, | |||
| resolved_action=getattr(task, 'resolved_action', ''), | |||
There was a problem hiding this comment.
Any reason not to make the default value here task_action instead of empty string? resolved_action isn't guaranteed to be a module/action FQCN if the actual module run was a legacy library module anyway, so in the "old core" case where the value is missing, it seems like presenting a less-accurate value is preferable to no value at all...
There was a problem hiding this comment.
@nitzmahone I think both options should be ok. The processing side will likely do a regexp check for FQCN format on resolved_action, if it won't match we'll use task_action.
There was a problem hiding this comment.
The only option that could make more sense to me would be to exclude resolved_action if it's the same as task.
I pushed a commit that does the original suggestion here, just fallback to the task.action.
|
Unable to freeze job graph: Job ansible-buildset-registry does not specify a run playbook |
|
Unable to freeze job graph: Job ansible-buildset-registry does not specify a run playbook |
|
For purposes of final approval, I want to make sure I present the final data structure here. Script for doing this: import json
import sys
import ansible_runner.interface
KEEP_KEYS = ['task', 'task_action', 'role', 'resolved_role', 'name', 'resolved_action']
r = ansible_runner.interface.run(
private_data_dir=sys.argv[-2],
playbook=sys.argv[-1]
)
events = []
for event in r.events:
event_data = event['event_data']
d = {"event": event['event']}
for key in KEEP_KEYS:
if key in event_data:
d[key] = event_data[key]
events.append(d)
print(json.dumps(events, indent=2))Running that for the new test case [
{
"event": "playbook_on_start"
},
{
"event": "playbook_on_play_start",
"name": "all"
},
{
"event": "playbook_on_task_start",
"task": "debug",
"task_action": "debug",
"role": "hello",
"resolved_role": "groovy.peanuts.hello",
"name": "groovy.peanuts.hello : debug",
"resolved_action": "ansible.builtin.debug"
},
{
"event": "runner_on_start",
"task": "debug",
"task_action": "debug",
"role": "hello",
"resolved_role": "groovy.peanuts.hello",
"resolved_action": "ansible.builtin.debug"
},
{
"event": "runner_on_ok",
"task": "debug",
"task_action": "debug",
"role": "hello",
"resolved_role": "groovy.peanuts.hello",
"resolved_action": "ansible.builtin.debug"
},
{
"event": "playbook_on_stats",
"resolved_role": "groovy.peanuts.hello",
"resolved_action": "ansible.builtin.debug"
}
]Now that I look at it, the playbook_on_stats looks a bit problematic, but I don't believe that changes with this PR in any way. |
|
No wait, that was a real implementation problem, will push a fix. |
|
Unable to freeze job graph: Job ansible-buildset-registry does not specify a run playbook |
|
Updated structure [
{
"event": "playbook_on_start"
},
{
"event": "playbook_on_play_start",
"name": "all"
},
{
"event": "playbook_on_task_start",
"task": "debug",
"task_action": "debug",
"role": "hello",
"resolved_role": "groovy.peanuts.hello",
"name": "groovy.peanuts.hello : debug",
"resolved_action": "ansible.builtin.debug"
},
{
"event": "runner_on_start",
"task": "debug",
"task_action": "debug",
"role": "hello",
"resolved_role": "groovy.peanuts.hello",
"resolved_action": "ansible.builtin.debug"
},
{
"event": "runner_on_ok",
"task": "debug",
"task_action": "debug",
"role": "hello",
"resolved_role": "groovy.peanuts.hello",
"resolved_action": "ansible.builtin.debug"
},
{
"event": "playbook_on_stats"
}
] |
|
@AlanCoding this looks great 👍 |
|
Looks like CI may be fixed in #1034 EDIT: pushed again to pick up CI fixes. |
|
Build succeeded.
|
|
I have no idea about this new failure |
|
Build succeeded.
|
|
last CI failure I mentioned looks like it was flake, it's passing now. |
Connect #1019
Keeping track of the event data from these test cases is hard. Events go:
The bolded event types have historically had the
roleentry inside of the event_data dict.The new
resolved_actionkey is only present on "playbook_on_task_start". This is a break in pattern from other keys liketask, which occurs through all events where that task is active, similar torole, those 3 bolded events. I tried to raise this point in #995, although without the full testing and context. Here's the full event data:https://gist.github.com/AlanCoding/325a84d4121249188a573ad25249cda7
The
ben.jsonfile corresponds to the existing test that this follows the pattern of, andpeanut.jsonis for the test added here.Speaking in broad strokes,
taskandrolekeys are contextual in nature. A role will run multiple tasks, and everything in that context will be marked with that role. However,resolved_actiongives the FQCN of a task only at the start of the task. This doesn't leave good options for the role FQCN. Introduce a new key, sure, but would this key be contextual or not? If it is non-contextual likeresolved_action, then it would only appear in "playbook_on_task_start", which is really weird for a role, since the role has multiple tasks. So that would leave it as weird semi-contextual thing. To make it contextual, it would become inconsistent with the implementation ofresolved_action.So here I'm testing the waters to see if there's any pushback for the "cheap" option. Make the
rolekey FQCN if it's a collection.