Repository: ansible/awx
File: awx/main/tasks/facts.py (Python)

Code before:
                with codecs.open(filepath, 'r', encoding='utf-8') as f:
                    try:
                        ansible_facts = json.load(f)
                    except ValueError:
                        continue
                    host.ansible_facts = ansible_facts
                    host.ansible_facts_modified = now()
                    hosts_to_update.append(host)
                    system_tracking_logger.info(
                        'New fact for inventory {} host {}'.format(smart_str(host.inventory.name), smart_str(host.name)),
                        extra=dict(
                            inventory_id=host.inventory.id,
                            host_name=host.name,
                            ansible_facts=host.ansible_facts,
                            ansible_facts_modified=host.ansible_facts_modified.isoformat(),
                            job_id=job_id,
                        ),
                    )
                    log_data['updated_ct'] += 1
            else:
                log_data['unmodified_ct'] += 1
        else:
            # if the file goes missing, ansible removed it (likely via clear_facts)
            # if the file goes missing, but the host hasnot started facts, then we should not clear the facts
            if hosts_cached and host.name not in hosts_cached:
                continue
            host.ansible_facts = {}
            host.ansible_facts_modified = now()
            hosts_to_update.append(host)
            system_tracking_logger.info('Facts cleared for inventory {} host {}'.format(smart_str(host.inventory.name), smart_str(host.name)))
            log_data['cleared_ct'] += 1
        if len(hosts_to_update) > 100:
            update_hosts(hosts_to_update)
            hosts_to_update = []
    update_hosts(hosts_to_update)


Review comment (performance):
The outer iteration loop no longer makes any sense, that being `for host in hosts:`. On the surface-level you might as well change that to `for host in hosts_cached:` (and any associated changes), and get rid of the `continue` added here.

This requires changing the method signature (this is a structural change and that should not be surprising). As passing in `job.get_hosts_for_fact_cache()` for hosts carries a non-trivial cost and it is no longer needed considering what you're doing.

Given that you don't need to pass it into `finish_fact_cache`, I'm also unclear if it should be passed into `start_fact_cache`. A semi-isolated way to do that might be to pass the full `Inventory` object from the outset.

I talked a good deal with @chrismeyersfsu about further consequences and I'm not sure if the refactor should even stop there. The contract between the start/finish methods is mainly the artifacts directory, and passing around the list of cached hosts in memory (given this particular kind of toxic code path) may be less maintainable than passing it around on disk. I would seriously consider saving the data structure in a file next to the fact cache folder (both being inside of artifacts). If you did that, I also expect it would lead into removing the `facts_write_time` argument.

Code after:
                    except ValueError:
                        continue
                    host.ansible_facts = ansible_facts
                    host.ansible_facts_modified = now()
                    hosts_to_update.append(host)
                    system_tracking_logger.info(
                        'New fact for inventory {} host {}'.format(smart_str(host.inventory.name), smart_str(host.name)),
                        extra=dict(
                            inventory_id=host.inventory.id,
                            host_name=host.name,
                            ansible_facts=host.ansible_facts,
                            ansible_facts_modified=host.ansible_facts_modified.isoformat(),
                            job_id=job_id,
                        ),
                    )
                    log_data['updated_ct'] += 1
            else:
                log_data['unmodified_ct'] += 1
        else:
            # if the file goes missing, ansible removed it (likely via clear_facts)
            # if the file goes missing, but the host has not started facts, then we should not clear the facts
            host.ansible_facts = {}
            host.ansible_facts_modified = now()
            hosts_to_update.append(host)
            system_tracking_logger.info('Facts cleared for inventory {} host {}'.format(smart_str(host.inventory.name), smart_str(host.name)))
            log_data['cleared_ct'] += 1
        if len(hosts_to_update) > 100:
            update_hosts(hosts_to_update)
            hosts_to_update = []
    update_hosts(hosts_to_update)