Feature indirect host counting #15802
Conversation
github-actions
bot
added
component:api
dependencies
Codecov ReportAttention: Patch coverage is
✅ All tests successful. No failed tests found. ❌ Your patch check has failed because the patch coverage (78.16%) is below the target coverage (100.00%). You can increase the patch coverage or adjust the target coverage. |
AlanCoding
requested review from
TheRealHaoLiu,
fosterseth,
pb82 and
adrisala
|
|
||
| # From this jq result (specific to a single Ansible module), get index information about this host record | ||
| if not data.get('canonical_facts'): | ||
| if not facts_missing_logged: |
There was a problem hiding this comment.
Is not this the opposite? Shouldn't it be:
| if not facts_missing_logged: | |
| if facts_missing_logged: |
There was a problem hiding this comment.
There is something wrong here, another great catch.
But let me explain more of the intent - this will process a lot of events. It's extremely possible that the event_query.yml file, which comes from the collection, is malformed. It could have any number of things wrong with it, but that error will repeat for every single event processed against that query. But also, if a job has 1M events, we don't necessarily want to exit the entire processing loop because event 928 had an error. So, if this type of error happens, it is only logged once. The variable is to say "we already logged this once", and if we have logged it once already, we will not log again.
| try: | ||
| hashable_facts = get_hashable_form(canonical_facts) | ||
| except UnhashableFacts: | ||
| if not unhashable_facts_logged: |
There was a problem hiding this comment.
Same here?
| if not unhashable_facts_logged: | |
| if unhashable_facts_logged: |
| except UnhashableFacts: | ||
| if not unhashable_facts_logged: | ||
| logger.info(f'Could not hash canonical_facts {canonical_facts}, skipping') | ||
| unhashable_facts_logged = True |
There was a problem hiding this comment.
Not following why this is changed :(
| save_indirect_host_entries(job.id, wait_for_events=True) | ||
| job_ct += 1 | ||
| if job_ct: | ||
| logger.info(f'Restarted event processing for {job_ct} jobs') |
There was a problem hiding this comment.
Is this info level? I would put this on debug and/or mention it is related to indirect host counting. As a system administrator I don't think I want to see information about how data is collected internally in my logs.
There was a problem hiding this comment.
Well it's an hourly schedule. And yeah, default logging is INFO level. Maybe this is just me, but as long as it's a single-line log, I would like to see some kind of log reporting for any schedule that's hourly or more infrequent.
awx/main/tasks/host_indirect.py
Outdated
| compiled_jq_expressions = {} # Cache for compiled jq expressions | ||
| facts_missing_logged = False | ||
| unhashable_facts_logged = False | ||
| for event in job.job_events.filter(task__in=job_event_queries.keys()).iterator(): |
There was a problem hiding this comment.
@AlanCoding I have tested this with the VMWare collection, but it looks like none of the job events match that query: the task name is either empty or doesn't match the module name.
| for data in compiled_jq.input(event.event_data['res']).all(): | ||
|
|
||
| # From this jq result (specific to a single Ansible module), get index information about this host record | ||
| if not data.get('canonical_facts'): |
There was a problem hiding this comment.
I wonder should we use the json schema here to check the response is valid or is it overkill?
In my schema proposal I am also listy device_type under facts to be mandatory, so we can sort out which groups of devices we have in https://github.com/ansible/handbook/pull/181/files#diff-aa1240cabb636a0facdb972a621d8c8f3f46ace92838bd9a6ecfb55a5d9aabd9R197
awx/main/tasks/host_indirect.py
Outdated
| # Gate running this task on the job having all events processed, not just EOF or playbook_on_stats | ||
| current_events = 0 | ||
| for _ in range(10): | ||
| current_events = job.job_events.count() |
There was a problem hiding this comment.
we had a conversation about count effectivity. Count will have to scan all events of a job, that can take a lot of time. (O(n) complexity)
Rather use timestamp comparison, just getting max(timestamp) of job's events, which if covered by index is a O(1) lookup.
There was a problem hiding this comment.
that is if this translates to select count(*) from job_events where job_id=job.job_id AND job_created=job.job_created
if you could confirm this is the query that runs
There was a problem hiding this comment.
since the job_events is partitioned. We need to make sure we use tje job_created in every query, to go into certain condition (which should be covered the Django ORM I believe)
We'd need to do a quick check what kind of index we'd need to do, to perform index scan on max(timestamp)
There was a problem hiding this comment.
We are careful to use job.job_events specifically so that it'll be filtered by job_created, and thus, that partition.
This isn't quite what we want, and I have #15811 up, but I believe there are other issues with the test suite that needs to be fixed before that will pass. That would reduce the max from 10 queries to 1.
We could skip doing this altogether in the post_run_hook, if push comes to shove. Then we'll have to wait an hour or something for every job to be processed.
Rather use timestamp comparison, just getting max(timestamp) of job's events, which if covered by index is a O(1) lookup
I don't know if that can solve any problem we have. We only ever get here if the playbook_on_stats event is processed. But this covers a much more niche case where the final event (or EOF) is processed while mid-run events are still not yet saved. Events are not processed linearly. If they were, this polling loop could be deleted outright.
…on (#15774) * Add jq dependency * Add file in progress * Add license for jq * Write test and get it passing
* Callback plugin method from cmeyers adapted to global collection list Get tests passing Mild rebranding Put behind feature flag, flip true in dev Add noqa flag * Add missing wait_for_events
* Minor import changes to collection processing in callback plugin * Move agreed location of event_query file
…#15784) * Rename the indirect host counting test file * Combine artifacts saving logic
…15796) * Document, implement, and test remaining indirect host audit fields * Fix hashing
…15798) * Wait for all event processing to finish, add fallback task * Add flag check to periodic task
* By default, do not count indirect hosts * Fix copy paste goof * Fix linter issue from base branch
#15805) prevent multiple tasks from processing the same job events, prevent periodic task from spawning another task per job
#15815) * fix: rely on resolved_action instead of task, adapt to proposed query structure * tests: update indirect host tests * update remaining queries to new format * update live test
* Remove polling loop for job finishing event processing * Make awx/main/tests/live dramatically faster (#15780)
pb82
force-pushed
the
feature_indirect-host-counting
branch
from
f83a7c1 to
cbe6d4c
Compare
|
SUMMARY
At the moment, this feature is delivering a table
IndirectManagedNodeAudit. It is generated after a job runs, and has a cleanup policy and things like that. This will be processed later on, ping @Ladas , to generate analytics, like a count of the "real" hosts that were interacted with.ISSUE TYPE
COMPONENT NAME