From 07d8a633b1c6f42ca683af24ddfeb7a035c9c0b0 Mon Sep 17 00:00:00 2001 From: Rich Megginson Date: Wed, 29 Nov 2023 13:52:56 -0700 Subject: [PATCH] refactor: improve support for ostree systems The dependency on `ansible.utils.update_fact` is causing issue with some users who now must install that collection in order to run the role, even if they do not care about ostree. The fix is to stop trying to set `ansible_facts.pkg_mgr`, and instead force the use of the ostree package manager with the `package:` module `use:` option. The strategy is - on ostree systems, set the flag `__$ROLENAME_is_ostree` if the system is an ostree system. The flag will either be undefined or `false` on non-ostree systems. Then, change every invocation of the `package:` module like this: ```yaml - name: Ensure required packages are present package: name: "{{ __$ROLENAME_packages }}" state: present use: "{{ (__$ROLENAME_is_ostree | d(false)) | ternary('ansible.posix.rhel_rpm_ostree', omit) }}" ``` This should ensure that the `use:` parameter is not used if the system is non-ostree. The goal is to make the ostree support as unobtrusive as possible for non-ostree systems. The user can also set `__$ROLENAME_is_ostree: true` in the inventory or play if the user knows that ostree is being used and wants to skip the check. Or, the user is concerned about the performance hit for ostree detection on non-ostree systems, and sets `__$ROLENAME_is_ostree: false` to skip the check. The flag `__$ROLENAME_is_ostree` can also be used in the role or tests to include or exclude tasks from being run on ostree systems. This fix also improves error reporting in the `get_ostree_data.sh` script when included roles cannot be found. Signed-off-by: Rich Megginson --- .ansible-lint | 2 -- .ostree/get_ostree_data.sh | 29 +++++++++++++++++++---------- meta/collection-requirements.yml | 1 - tasks/main.yml | 6 ++++++ tasks/set_vars.yml | 18 ++++++------------ tests/tasks/setup.yml | 22 +++++++++------------- tests/tests_ntp_ptp.yml | 2 ++ tests/tests_ptp_multi.yml | 2 ++ tests/tests_ptp_single.yml | 2 ++ 9 files changed, 46 insertions(+), 38 deletions(-) diff --git a/.ansible-lint b/.ansible-lint index bf155a5..2c7c8df 100644 --- a/.ansible-lint +++ b/.ansible-lint @@ -24,5 +24,3 @@ exclude_paths: - examples/roles/ mock_roles: - linux-system-roles.timesync -mock_modules: - - ansible.utils.update_fact diff --git a/.ostree/get_ostree_data.sh b/.ostree/get_ostree_data.sh index 7c32524..cec08b0 100755 --- a/.ostree/get_ostree_data.sh +++ b/.ostree/get_ostree_data.sh @@ -2,7 +2,6 @@ set -euo pipefail -role_collection_dir="${ROLE_COLLECTION_DIR:-fedora/linux_system_roles}" ostree_dir="${OSTREE_DIR:-"$(dirname "$(realpath "$0")")"}" if [ -z "${4:-}" ] || [ "${1:-}" = help ] || [ "${1:-}" = -h ]; then @@ -29,7 +28,7 @@ if [ "$pkgtype" = testing ]; then fi get_rolepath() { - local ostree_dir role rolesdir roles_parent_dir + local ostree_dir role rolesdir roles_parent_dir coll_path pth ostree_dir="$1" role="$2" roles_parent_dir="$(dirname "$(dirname "$ostree_dir")")" @@ -47,16 +46,22 @@ get_rolepath() { fi done # look elsewhere - if [ -n "${ANSIBLE_COLLECTIONS_PATHS:-}" ]; then - for pth in ${ANSIBLE_COLLECTIONS_PATHS//:/ }; do - rolesdir="$pth/ansible_collections/$role_collection_dir/roles/$role/.ostree" - if [ -d "$rolesdir" ]; then - echo "$rolesdir" - return 0 - fi + coll_path="${ANSIBLE_COLLECTIONS_PATH:-}" + if [ -z "$coll_path" ]; then + coll_path="${ANSIBLE_COLLECTIONS_PATHS:-}" + fi + if [ -n "${coll_path}" ]; then + for pth in ${coll_path//:/ }; do + for rolesdir in "$pth"/ansible_collections/*/*_system_roles/roles/"$role"/.ostree; do + if [ -d "$rolesdir" ]; then + echo "$rolesdir" + return 0 + fi + done done fi - return 1 + 1>&2 echo ERROR - could not find role "$role" - please use ANSIBLE_COLLECTIONS_PATH + exit 2 } get_packages() { @@ -75,6 +80,10 @@ get_packages() { roles="$(cat "$rolefile")" for role in $roles; do rolepath="$(get_rolepath "$ostree_dir" "$role")" + if [ -z "$rolepath" ]; then + 1>&2 echo ERROR - could not find role "$role" - please use ANSIBLE_COLLECTIONS_PATH + exit 2 + fi get_packages "$rolepath" done fi diff --git a/meta/collection-requirements.yml b/meta/collection-requirements.yml index 4f4842b..0110bbe 100644 --- a/meta/collection-requirements.yml +++ b/meta/collection-requirements.yml @@ -2,4 +2,3 @@ --- collections: - ansible.posix - - ansible.utils diff --git a/tasks/main.yml b/tasks/main.yml index c2de01e..0592e55 100644 --- a/tasks/main.yml +++ b/tasks/main.yml @@ -62,6 +62,8 @@ package: name: chrony state: present + use: "{{ (__timesync_is_ostree | d(false)) | + ternary('ansible.posix.rhel_rpm_ostree', omit) }}" when: - timesync_mode != 2 - timesync_ntp_provider == 'chrony' @@ -70,6 +72,8 @@ package: name: ntp state: present + use: "{{ (__timesync_is_ostree | d(false)) | + ternary('ansible.posix.rhel_rpm_ostree', omit) }}" when: - timesync_mode != 2 - timesync_ntp_provider == 'ntp' @@ -78,6 +82,8 @@ package: name: linuxptp state: present + use: "{{ (__timesync_is_ostree | d(false)) | + ternary('ansible.posix.rhel_rpm_ostree', omit) }}" when: timesync_mode != 1 - name: Gather package facts diff --git a/tasks/set_vars.yml b/tasks/set_vars.yml index 883ee88..de9a170 100644 --- a/tasks/set_vars.yml +++ b/tasks/set_vars.yml @@ -5,23 +5,17 @@ when: __timesync_required_facts | difference(ansible_facts.keys() | list) | length > 0 -- name: Ensure correct package manager for ostree systems - vars: - ostree_pkg_mgr: ansible.posix.rhel_rpm_ostree - ostree_booted_file: /run/ostree-booted - when: ansible_facts.pkg_mgr | d("") != ostree_pkg_mgr +- name: Determine if system is ostree and set flag + when: not __timesync_is_ostree is defined block: - name: Check if system is ostree stat: - path: "{{ ostree_booted_file }}" + path: /run/ostree-booted register: __ostree_booted_stat - - name: Set package manager to use for ostree - ansible.utils.update_fact: - updates: - - path: ansible_facts.pkg_mgr - value: "{{ ostree_pkg_mgr }}" - when: __ostree_booted_stat.stat.exists + - name: Set flag to indicate system is ostree + set_fact: + __timesync_is_ostree: "{{ __ostree_booted_stat.stat.exists }}" - name: Set platform/version specific variables include_vars: "{{ lookup('first_found', ffparams) }}" diff --git a/tests/tasks/setup.yml b/tests/tasks/setup.yml index 223cc36..c7442bf 100644 --- a/tests/tasks/setup.yml +++ b/tests/tasks/setup.yml @@ -1,27 +1,23 @@ --- # common test setup tasks -- name: Ensure correct package manager for ostree systems - vars: - ostree_pkg_mgr: ansible.posix.rhel_rpm_ostree - ostree_booted_file: /run/ostree-booted - when: ansible_facts.pkg_mgr | d("") != ostree_pkg_mgr +- name: Determine if system is ostree and set flag + when: not __timesync_is_ostree is defined block: - name: Check if system is ostree stat: - path: "{{ ostree_booted_file }}" + path: /run/ostree-booted register: __ostree_booted_stat - - name: Set package manager to use for ostree - ansible.utils.update_fact: - updates: - - path: ansible_facts.pkg_mgr - value: "{{ ostree_pkg_mgr }}" - when: __ostree_booted_stat.stat.exists + - name: Set flag to indicate system is ostree + set_fact: + __timesync_is_ostree: "{{ __ostree_booted_stat.stat.exists }}" - name: Ensure iproute for gathering default_ipv4 fact package: name: iproute # for fact gathering for ip facts state: present + use: "{{ (__timesync_is_ostree | d(false)) | + ternary('ansible.posix.rhel_rpm_ostree', omit) }}" - name: Ensure ansible_facts used by test setup: @@ -45,4 +41,4 @@ meta: end_host when: - __timesync_ostree_unsupported | d(false) - - ansible_facts.pkg_mgr | d() == "ansible.posix.rhel_rpm_ostree" + - __timesync_is_ostree | d(false) diff --git a/tests/tests_ntp_ptp.yml b/tests/tests_ntp_ptp.yml index e62723c..c6b5b15 100644 --- a/tests/tests_ntp_ptp.yml +++ b/tests/tests_ntp_ptp.yml @@ -38,6 +38,8 @@ package: name: ethtool state: present + use: "{{ (__timesync_is_ostree | d(false)) | + ternary('ansible.posix.rhel_rpm_ostree', omit) }}" - name: Check if SW/HW timestamping is supported command: ethtool -T "{{ ansible_default_ipv4['interface'] }}" diff --git a/tests/tests_ptp_multi.yml b/tests/tests_ptp_multi.yml index c118a73..ad7b6cd 100644 --- a/tests/tests_ptp_multi.yml +++ b/tests/tests_ptp_multi.yml @@ -32,6 +32,8 @@ package: name: ethtool state: present + use: "{{ (__timesync_is_ostree | d(false)) | + ternary('ansible.posix.rhel_rpm_ostree', omit) }}" - name: Check if SW/HW timestamping is supported command: ethtool -T "{{ ansible_default_ipv4['interface'] }}" diff --git a/tests/tests_ptp_single.yml b/tests/tests_ptp_single.yml index 8e44003..8f3e61d 100644 --- a/tests/tests_ptp_single.yml +++ b/tests/tests_ptp_single.yml @@ -23,6 +23,8 @@ package: name: ethtool state: present + use: "{{ (__timesync_is_ostree | d(false)) | + ternary('ansible.posix.rhel_rpm_ostree', omit) }}" - name: Check if SW/HW timestamping is supported command: ethtool -T "{{ ansible_default_ipv4['interface'] }}"