Set inject_facts_as_vars to false to speed up ansible

[fungusakafungus]

What type of PR is this?
/kind feature

What this PR does / why we need it:
According to discussion in ansible/ansible#73654 (comment), disabling inject_facts_as_vars makes ansible go faster for some people. We do have a lot of tasks to skip and for every task/every host the vars need to be resolved.

This PR replaces all usages of ansible_<fact_name> with ansible_facts.<fact_name> and disables inject_facts_as_vars in ansible.cfg and in tests.

Which issue(s) this PR fixes:

Related to ansible/ansible#73654

Special notes for your reviewer:
While this has been a nice exercise for me, this PR does not bring considerable improvement, at least in my limited testing.
Probably because we do carefully limit the number of facts we gather, so there's not much to gain here. I would still like to inform you, that this approach might be useful in the future, if we have to use more facts.

Does this PR introduce a user-facing change?:
This is probably not going to be released.

[k8s-ci-robot]

Hi @fungusakafungus. Thanks for your PR.

I'm waiting for a kubernetes-sigs member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

[cristicalin]

This is actually a great exercise to force us to keep the code clean and separate facts from inventory variables. I like it quite a lot and the CI seems to agree.

This has a 👍 from me.

@floryut @oomichi what do you think?

[oomichi]

This is actually a great exercise to force us to keep the code clean and separate facts from inventory variables. I like it quite a lot and the CI seems to agree.

This has a 👍 from me.

@floryut @oomichi what do you think?

This seems for making Ansible speed up.
By comparing the other job running times, it is not easy to say this performance is better than the existing ones.
For example, the following is by comparing with #8932

Job	Existing	This
packet_almalinux8-calico	21m	19m
packet_almalinux8-calico-remove-node	30m	26m
packet_almalinux8-crio	22m	27m
packet_almalinux8-docker	19m	19m
packet_centos7-calico-ha-once-localhost	23m	31m
packet_centos7-flannel-addons-ha	28m	33m
packet_debian10-calico	20m	26m
packet_debian10-calico-upgrade	29m	24m
packet_debian10-docker	18m	23m

There might be a bigger factor than this inject_facts_as_vars thing at the CI.

[cristicalin]

Great comparison @oomichi but our CI does not quite yield predictable run times, depending on the number of parallel jobs there can actually be severe contention leading to time variance between runs of the same job. Additionally we fetch a lot of assets from the internet during a job run which can also cause unpredictable run times.

I'm not sure how we could stop the CI from picking up other jobs to at least remove the noisy neighbour effect but there are some weird discrepancies. Since vagrant jobs spin up a dedicated host comparing those might give us more comparable numbers.

[fungusakafungus]

I'd expect the highest impact on ci jobs with the most hosts and the most tasks, even if those are skipped tasks.

[cristicalin]

The CI jobs are usually 2 node jobs or 3 node jobs (HA ones) so it would be difficult to use this criteria.

[oomichi]

My concern isdisabling inject_facts_as_vars makes ansible go faster for some people from the pull request message.
If we merge this pull request without some proof/result of the performance improvement, the other people would try reverting this pull request for the performance deterioration.

This pull request is big and the revert is not easy after taking time.
I'd like to avoid such situation if possible.

[cristicalin]

My concern isdisabling inject_facts_as_vars makes ansible go faster for some people from the pull request message. If we merge this pull request without some proof/result of the performance improvement, the other people would try reverting this pull request for the performance deterioration.

This pull request is big and the revert is not easy after taking time. I'd like to avoid such situation if possible.

Totally agree with you on this @oomichi , I was just pointing out that in order to qualify the difference of runtimes we need some more precise measurements.

@fungusakafungus I'm wondering if we could make the code change without affecting ansible.cfg. We could compare the impact of that change alone then we could advise users to try the ansible.cfg modification and test for themselves.

[fungusakafungus]

@cristicalin

I'm wondering if we could make the code change without affecting ansible.cfg. We could compare the impact of that change alone then we could advise users to try the ansible.cfg modification and test for themselves.

Sure, I can hold off on that part. But I'll leave it in the tests/ansible.cfg

@oomichi

My concern is disabling inject_facts_as_vars makes ansible go faster for some people from the pull request message.

I'm quite sure it makes it faster for everyone, it is only visible for some people. That's what I meant.

One the other side, this change might break something for some people, in case they use facts like ansible_distribution(such facts will not be available anymore) in their own playbooks, but with kubespray's ansible.cfg. We do not recommend this usage on https://kubespray.io/#/docs/integration, we recommend using submodules instead.

This pull request is big and the revert is not easy after taking time.

I will be around to revert it if needed. Most of it is done with sed anyway, and

git grep -l ansible_facts | xargs gsed -i -e 's/\([^.]\)ansible_facts\./\1ansible_/g' -e "s/ansible_facts']\['/ansible_/g"

basically reverts it, leaving a small diff to be reverted manually (or by git if still possible)

[fungusakafungus]

Rebased again and squashed and rephrased commits

[fungusakafungus]

rebased again

[cristicalin]

Currently affected by CI issues: #8984

[fungusakafungus]

I've produced a little oneliner to check for facts injected as vars I'm using to keep this fresh:
https://gist.github.com/fungusakafungus/c36632a272249493c3ab4fcd8b60e091

[k8s-ci-robot]

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: fungusakafungus
Once this PR has been reviewed and has the lgtm label, please assign miouge1 for approval by writing /assign @miouge1 in a comment. For more information see:The Kubernetes Code Review Process.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

• OWNERS

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

[k8s-ci-robot]

@fungusakafungus: PR needs rebase.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

[k8s-triage-robot]

The Kubernetes project currently lacks enough contributors to adequately respond to all issues and PRs.

This bot triages issues and PRs according to the following rules:

• After 90d of inactivity, lifecycle/stale is applied
• After 30d of inactivity since lifecycle/stale was applied, lifecycle/rotten is applied
• After 30d of inactivity since lifecycle/rotten was applied, the issue is closed

You can:

• Mark this issue or PR as fresh with /remove-lifecycle stale
• Mark this issue or PR as rotten with /lifecycle rotten
• Close this issue or PR with /close
• Offer to help out with Issue Triage

Please send feedback to sig-contributor-experience at kubernetes/community.

/lifecycle stale

[k8s-triage-robot]

The Kubernetes project currently lacks enough active contributors to adequately respond to all issues and PRs.

This bot triages issues and PRs according to the following rules:

• After 90d of inactivity, lifecycle/stale is applied
• After 30d of inactivity since lifecycle/stale was applied, lifecycle/rotten is applied
• After 30d of inactivity since lifecycle/rotten was applied, the issue is closed

You can:

• Mark this issue or PR as fresh with /remove-lifecycle rotten
• Close this issue or PR with /close
• Offer to help out with Issue Triage

Please send feedback to sig-contributor-experience at kubernetes/community.

/lifecycle rotten