Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

refactor: Use vars/RedHat_N.yml symlink for CentOS, Rocky, Alma wherever possible #203

Merged
merged 1 commit into from
Oct 25, 2024

Conversation

richm
Copy link
Contributor

@richm richm commented Oct 18, 2024

We have a lot of requests to support Rocky and Alma in various system roles. The
first part of adding support is adding vars/ files for these platforms. In
almost every case, for a given major version N, the vars file RedHat_N.yml can
be used for CentOS, Rocky, and Alma. Rather than making a copy of the
RedHat_N.yml file, just use a symlink to reduce size and maintenance burden, and
standardize this across all system roles for consistency.

NOTE: There is no Alma or Rocky version 7 or less.

NOTE: OracleLinux is not a strict clone, so we are not going to do this for
OracleLinux at this time. Support for OracleLinux will need to be done in
separate PRs. For more information, see
linux-system-roles/cockpit#130

Question: Why not just use ansible_facts["os_family"] == "RedHat"?

Answer: This is what Ansible uses as the RedHat os_family:
https://github.com/ansible/ansible/blob/1e6ffc1d02559a26def6c9c3b07baf27032865a2/lib/ansible/module_utils/facts/system/distribution.py#L511
There are a lot of distributions in there. I know that Fedora is not a clone of
RHEL, but it is very closely related. Most of the others are not clones, and it
would generally not work to replace ansible_distribution in ['CentOS', 'Fedora',
'RedHat'] with ansible_facts['os_family'] == 'RedHat' (but it would probably
work in specific cases with specific distributions). For example, OracleLinux
is in there, and we know that doesn't generally work. The only ones we can be
pretty sure about are RedHat, CentOS, Fedora, AlmaLinux, and Rocky.

Question: Does my role really need this because it should already work on
RHEL clones?

Answer: Maybe not - but:

  • it doesn't hurt anything
  • it's there if we need it in the future
  • the role will be inconsistent with the other system roles if we don't have this

Question: Why do I need the tests/vars/rh_distros_vars.yml file? Doesn't
the test load the vars from the role?

Answer: No, the test does not load the vars from the role until the role is
included, and many tests use version and distribution before including the role.

Question: Do we need to change the code now to use the new variables?

Answer: No, not now, in subsequent PRs, hopefully by Alma and Rocky users.

Note that there may be more work to be done to the role to fully support Rocky
and Alma. Many roles have conditionals like this:

some_var: "{{ 'some value' if ansible_distribution in ['CentOS', 'RedHat'] else 'other value' }}"
another_var: "{{ 'some value' if ansible_distribution in ['CentOS', 'Fedora', 'RedHat'] else 'other value' }}"

...

- name: Do something
  when: ansible_distribution in ['CentOS', 'RedHat']
  ...
- name: Do something else
  when: ansible_distribution in ['CentOS', 'Fedora', 'RedHat']
  ...

Adding Rocky and AlmaLinux to these conditionals will have to be done
separately. In order to simplify the task, some new variables are being
introduced:

__$rolename_rh_distros:
  - AlmaLinux
  - CentOS
  - RedHat
  - Rocky

__$rolename_rh_distros_fedora: "{{ __$rolename_rh_distros + ['Fedora'] }}"

__$rolename_is_rh_distro: "{{ ansible_distribution in __$rolename_rh_distros }}"
__$rolename_is_rh_distro_fedora: "{{ ansible_distribution in __$rolename_rh_distros_fedora }}"

Then the conditionals can be rewritten as:

some_var: "{{ 'some value' if __$rolename_is_rh_distro else 'other value' }}"
another_var: "{{ 'some value' if __$rolename_is_rh_distro_fedora else 'other value' }}"

...

- name: Do something
  when: __$rolename_is_rh_distro | bool
  ...
- name: Do something else
  when: __$rolename_is_rh_distro_fedora | bool
  ...

For tests - tests that use such conditionals will need to use vars_files or
include_vars to load the variables that are defined in
tests/vars/rh_distros_vars.yml:

vars_files:
  - vars/rh_distros_vars.yml

We don't currently have CI testing for Rocky or Alma, so someone wanting to run
tests on those platforms would need to change the test code to use these.

@richm richm requested a review from ptoscano as a code owner October 18, 2024 22:15
@richm richm force-pushed the centos-rocky-alma-symlinks branch from ba9dfcf to 69313e0 Compare October 18, 2024 22:32
@ptoscano
Copy link
Collaborator

Hm is this code actually needed in the rhc role?

Looking at the code, I see only a couple of places of "potential interest":

$ git grep -n -C 3 CentOS
[...]
tests/tasks/setup_candlepin.yml-64-          - --publish
tests/tasks/setup_candlepin.yml-65-          - 8080:8080
tests/tasks/setup_candlepin.yml-66-          - "{{ '--privileged'
tests/tasks/setup_candlepin.yml:67:            if (ansible_distribution in ['CentOS', 'RedHat']
tests/tasks/setup_candlepin.yml-68-                and ansible_distribution_major_version | int < 8)
tests/tasks/setup_candlepin.yml-69-            else '' }}"
tests/tasks/setup_candlepin.yml-70-          - ghcr.io/ptoscano/candlepin-unofficial
--
tests/tests_environments.yml-84-        - name: Check the enabled environments
tests/tests_environments.yml-85-          when:
tests/tests_environments.yml-86-            - >-
tests/tests_environments.yml:87:              ansible_distribution not in ["CentOS", "RedHat"]
tests/tests_environments.yml-88-              or (ansible_distribution == "RedHat"
tests/tests_environments.yml-89-                  and ansible_distribution_version is version("8.6", ">="))
tests/tests_environments.yml:90:              or (ansible_distribution == "CentOS"
tests/tests_environments.yml-91-                  and ansible_distribution_major_version | int >= 8)
tests/tests_environments.yml-92-          block:
tests/tests_environments.yml-93-            - name: Get enabled environments
  • nothing in the actual code of the role, which thus should work just fine; the registration is performed in any case, and Insights is explicitly enabled only on RHEL
  • setup_candlepin.yml: this is the procedure for running the Candlepin container using podman during the tests:
    • it adds --privileged to the command line argument of podman in case the distro is older than 8 (so 7.x, in practice)
    • neither Rocky nor Alma have 7.x versions
    • while Oracle does exist on EL, I don't know what version of podman they ship
    • this setup is not done in case the test is run against an external Candlepin (see tests/README.md)
  • tests_environments.yml: the block checks that the distro is "new enough" to ship subscription-manager with the support for multiple environments:
    • due to the condition, any other EL distro will actually run the "new enough" code, which is still OK
    • tweaks are needed in case the test is run on Rocky/Alma/etc older than 8.6

Summary:

  • there is no impact on the code of the role
  • the tests should actually run properly on EL distros >= 8.6 already
  • the bits added to vars/main.yml are in practice not necessary
  • all I would potentially use from this PR is the __rhc_rh_distros variable for the tests only

What I'm thinking about is something simpler:

  • add include_vars loading in tests/tasks/setup_candlepin.yml and tests/tasks/setup_insights.yml (or maybe even directly in tests/tasks/setup_test_data.yml), so that tests/vars/*.yml files would be automatically used in any test
  • add a simple el_distros variable in tests/vars/main.yml
  • use el_distros in the two files described above

Rich, what do you think?

@richm
Copy link
Contributor Author

richm commented Oct 21, 2024

Hm is this code actually needed in the rhc role?

Maybe not? But

Looking at the code, I see only a couple of places of "potential interest":

$ git grep -n -C 3 CentOS
[...]
tests/tasks/setup_candlepin.yml-64-          - --publish
tests/tasks/setup_candlepin.yml-65-          - 8080:8080
tests/tasks/setup_candlepin.yml-66-          - "{{ '--privileged'
tests/tasks/setup_candlepin.yml:67:            if (ansible_distribution in ['CentOS', 'RedHat']
tests/tasks/setup_candlepin.yml-68-                and ansible_distribution_major_version | int < 8)
tests/tasks/setup_candlepin.yml-69-            else '' }}"
tests/tasks/setup_candlepin.yml-70-          - ghcr.io/ptoscano/candlepin-unofficial
--
tests/tests_environments.yml-84-        - name: Check the enabled environments
tests/tests_environments.yml-85-          when:
tests/tests_environments.yml-86-            - >-
tests/tests_environments.yml:87:              ansible_distribution not in ["CentOS", "RedHat"]
tests/tests_environments.yml-88-              or (ansible_distribution == "RedHat"
tests/tests_environments.yml-89-                  and ansible_distribution_version is version("8.6", ">="))
tests/tests_environments.yml:90:              or (ansible_distribution == "CentOS"
tests/tests_environments.yml-91-                  and ansible_distribution_major_version | int >= 8)
tests/tests_environments.yml-92-          block:
tests/tests_environments.yml-93-            - name: Get enabled environments
* nothing in the actual code of the role, which thus should work just fine; the registration is performed in any case, and Insights is explicitly enabled only on RHEL

Right.

* `setup_candlepin.yml`: this is the procedure for running the Candlepin container using podman during the tests:
  
  * it adds `--privileged` to the command line argument of `podman` in case the distro is older than 8 (so 7.x, in practice)
  * neither Rocky nor Alma have 7.x versions

Which has also been pointed out by @jjelen linux-system-roles/ssh#163 (review)

  * while Oracle does exist on EL, I don't know what version of `podman` they ship

Adding OracleLinux to this list is problematic because it does not appear to be a strict clone, which is why I did not add it here - see linux-system-roles/cockpit#130

  * this setup is not done in case the test is run against an external Candlepin (see `tests/README.md`)

* `tests_environments.yml`: the block checks that the distro is "new enough" to ship subscription-manager with the support for multiple environments:
  
  * due to the condition, any other EL distro will actually run the "new enough" code, which is still OK
  * tweaks are needed in case the test is run on Rocky/Alma/etc older than 8.6

I guess it doesn't matter for now because we only run the tests on RedHat, CentOS, and Fedora. If someone wants to run tests on Alma et. al., they will need to edit the tests. But at least this PR gives them the code needed to do that.

Summary:

* there is no impact on the code of the role

agreed - but I would prefer to merge this PR anyway for the reasons given above (and removing the Alma and Rocky links for versions < 8)

* the tests should actually run properly on EL distros >= 8.6 already

Probably yes, but I would prefer to make that explicit, if/when someone wants to run tests on Alma et. al.

* the bits added to `vars/main.yml` are in practice not necessary

agreed - but I would prefer to merge this PR anyway for the reasons given above

* all I would potentially use from this PR is the `__rhc_rh_distros` variable for the tests only

What I'm thinking about is something simpler:

* add `include_vars` loading in `tests/tasks/setup_candlepin.yml` and `tests/tasks/setup_insights.yml` (or maybe even directly in `tests/tasks/setup_test_data.yml`), so that `tests/vars/*.yml` files would be automatically used in any test

I would prefer not to modify the tests unless someone wants to run the tests on Alma et. al., and then I would prefer that person would submit a PR.

* add a simple `el_distros` variable in `tests/vars/main.yml`

* use `el_distros` in the two files described above

Rich, what do you think?

I would prefer to merge this PR anyway for the reasons given above (and removing the Alma and Rocky links for versions < 8)

@richm richm force-pushed the centos-rocky-alma-symlinks branch from 69313e0 to 8e7a8f0 Compare October 21, 2024 16:30
@richm richm self-assigned this Oct 21, 2024
@richm richm force-pushed the centos-rocky-alma-symlinks branch from 8e7a8f0 to 234ccd0 Compare October 21, 2024 17:12
@richm
Copy link
Contributor Author

richm commented Oct 21, 2024

See the Q&A section in the updated description

…ver possible

We have a lot of requests to support Rocky and Alma in various system roles. The
first part of adding support is adding `vars/` files for these platforms. In
almost every case, for a given major version N, the vars file RedHat_N.yml can
be used for CentOS, Rocky, and Alma.  Rather than making a copy of the
RedHat_N.yml file, just use a symlink to reduce size and maintenance burden, and
standardize this across all system roles for consistency.

NOTE: There is no Alma or Rocky version 7 or less.

NOTE: OracleLinux is not a strict clone, so we are not going to do this for
OracleLinux at this time.  Support for OracleLinux will need to be done in
separate PRs. For more information, see
linux-system-roles/cockpit#130

**Question**: Why not just use `ansible_facts["os_family"] == "RedHat"`?

**Answer**:  This is what Ansible uses as the RedHat os_family:
https://github.com/ansible/ansible/blob/1e6ffc1d02559a26def6c9c3b07baf27032865a2/lib/ansible/module_utils/facts/system/distribution.py#L511
There are a lot of distributions in there. I know that Fedora is not a clone of
RHEL, but it is very closely related. Most of the others are not clones, and it
would generally not work to replace ansible_distribution in ['CentOS', 'Fedora',
'RedHat'] with ansible_facts['os_family'] == 'RedHat' (but it would probably
work in specific cases with specific distributions).  For example, OracleLinux
is in there, and we know that doesn't generally work.  The only ones we can be
pretty sure about are `RedHat`, `CentOS`, `Fedora`, `AlmaLinux`, and `Rocky`.

**Question**: Does my role really need this because it should already work on
RHEL clones?

**Answer**: Maybe not - but:

* it doesn't hurt anything
* it's there if we need it in the future
* the role will be inconsistent with the other system roles if we don't have this

**Question**: Why do I need the `tests/vars/rh_distros_vars.yml` file?  Doesn't
the test load the vars from the role?

**Answer**: No, the test does not load the vars from the role until the role is
included, and many tests use version and distribution before including the role.

**Question**: Do we need to change the code now to use the new variables?

**Answer**: No, not now, in subsequent PRs, hopefully by Alma and Rocky users.

Note that there may be more work to be done to the role to fully support Rocky
and Alma.  Many roles have conditionals like this:

```yaml
some_var: "{{ 'some value' if ansible_distribution in ['CentOS', 'RedHat'] else 'other value' }}"
another_var: "{{ 'some value' if ansible_distribution in ['CentOS', 'Fedora', 'RedHat'] else 'other value' }}"

...

- name: Do something
  when: ansible_distribution in ['CentOS', 'RedHat']
  ...
- name: Do something else
  when: ansible_distribution in ['CentOS', 'Fedora', 'RedHat']
  ...
```

Adding Rocky and AlmaLinux to these conditionals will have to be done
separately. In order to simplify the task, some new variables are being
introduced:

```yaml
__$rolename_rh_distros:
  - AlmaLinux
  - CentOS
  - RedHat
  - Rocky

__$rolename_rh_distros_fedora: "{{ __$rolename_rh_distros + ['Fedora'] }}"

__$rolename_is_rh_distro: "{{ ansible_distribution in __$rolename_rh_distros }}"
__$rolename_is_rh_distro_fedora: "{{ ansible_distribution in __$rolename_rh_distros_fedora }}"
```

Then the conditionals can be rewritten as:

```yaml
some_var: "{{ 'some value' if __$rolename_is_rh_distro else 'other value' }}"
another_var: "{{ 'some value' if __$rolename_is_rh_distro_fedora else 'other value' }}"

...

- name: Do something
  when: __$rolename_is_rh_distro | bool
  ...
- name: Do something else
  when: __$rolename_is_rh_distro_fedora | bool
  ...
```

For tests - tests that use such conditionals will need to use `vars_files` or
`include_vars` to load the variables that are defined in
`tests/vars/rh_distros_vars.yml`:

```yaml
vars_files:
  - vars/rh_distros_vars.yml
```

We don't currently have CI testing for Rocky or Alma, so someone wanting to run
tests on those platforms would need to change the test code to use these.

Signed-off-by: Rich Megginson <[email protected]>
@richm richm force-pushed the centos-rocky-alma-symlinks branch from 234ccd0 to b469170 Compare October 23, 2024 14:21
@richm richm merged commit 57cd177 into main Oct 25, 2024
8 checks passed
@richm richm deleted the centos-rocky-alma-symlinks branch October 25, 2024 18:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants