`salt_testenv`: Enable Tumbleweed deployments and clean up #1849

agraul · 2025-06-04T16:19:53Z

This pull request enables salt_testenv Tumbleweed deployments by fixing the password-based root login over SSH (used by terraform to run salt on the target) and using venv-salt-minion during provisioning, which allows the removal of Salt packages without interfering with the provisioning itself.

In a secondary step I did a bit of clean-up. I used a bit of Jinja to remove repetition from our salt_classic_package.sls. This serves as an example, we can do the same for more if we like it.

m-czernek

So I'm giving a cautious +1 :)

From the perspective of Tumbleweed and SSH access, this is easy +1
From the perspective of refactoring, two things happened at once:
- Removing non-SUSE salt-classic code paths
- Refactoring remaining code paths

From that perspective, this seems good to me as well, though the refactoring is surprisingly big IMHO and difficult to review for corner cases.

I have tested, for example, that the repos correspond to the repos in our salt CI testsuite. As a side note, I'm missing the salttest repo, but I couldn't find it in the sumaform, so I expect we add that somewhere in the CI.

It would be good to try this with our CI first to ensure we haven't missed some corner cases, but I can also see merging this now and fixing CI errors if there are any afterwards.

I'll leave that decision up to you :)

m-czernek · 2025-06-13T08:03:51Z

salt/salt_testenv/salt_classic_package.sls

-
-{% if grains['osfullname'] == 'SL-Micro' %}
-{% set repo_path = 'SLMicro' + grains['osrelease_info'][0]|string + grains['osrelease_info'][1]|string %}
+{% from "salt_testenv/lib.sls" import sle15_module_repos with context -%}


Open question: Since we're refactoring this, should we split this sls file into two sls files, one for the repos part be and the other one for the pkgs part?

Good question, for me it's okay either way. I don't have a strong preference either way.

m-czernek · 2025-06-13T08:04:42Z

salt/salt_testenv/salt_classic_package.sls

-    - name: transactional-update -c -n pkg in --capability python3-salt-testsuite python3-salt-test python3-salt
-{% else %}
-  {# HACK: we call zypper manually to ensure right packages are installed regardless upgrade/downgrade #}
+    - name: transactional-update --continue --non-interactive --drop-if-no-change pkg remove {{ installed_salt_pkgs|join(" ")}}


Love this change! Keep one-letter options for personal CLI use, but here, it costs us nothing to expand these for better readability.

m-czernek · 2025-06-13T08:08:14Z

salt/salt_testenv/lib.sls

+
+{# Build repository url for a SLE 15 Module.
+
+Args:


Not sure what I think of creating a sls library. I totally understand the intent, i.e. in code, we'd create small functions so that we don't have to declare things over and over again.

But, when we'll need to change this, it'll be more difficult to understand these macros than if we just declared the repos directly.

In other words, this:

development_tools_repo_pool: pkgrepo.managed: - baseurl: http://{{ grains.get("mirror") | default("dist.nue.suse.com/ibs", true) }}/SUSE/Products/SLE-Module-Development-Tools/{{ repo_path }}/x86_64/product/ - refresh: True ... repeated X times...

Seems clearer and easier to understand/change than:

{% for module in ["development_tools", "HPC", "containers" ] -%} {{ sle15_module_repos(module, "dist.nue.suse.com/ibs") }} {% endfor -%}

Perhaps this stems for me, personally, from wanting to do as little "clever" things as possible in markup languages (in SLS, Jinja, ...).

(Feel free to resolve, I'm not blocking this, just offering a point of discussion - I'm happy to try this :) )

Thank you for your perspective, that's exactly the discussion I wanted to have. If you only look at one set of repos I agree, it's a lot easier to read and understand if we do not use a macro. For this case, we need to compare the expanded macro, not just one example. I found it overwhelming to have the same structure repeated so often.

{% for module in ["development_tools", "HPC", "containers" ] -%} {{ sle15_module_repos(module, "dist.nue.suse.com/ibs") }} {% endfor -%} {% if grains["osrelease_info"][1]|int >= 6 -%} {% for module in ["python3", "systems_management"] -%} {{ sle15_module_repos(module, "dist.nue.suse.com/ibs") }} {% endfor -%}

vs

development_tools_repo_pool: pkgrepo.managed: - baseurl: http://dist.nue.suse.com/ibs/SUSE/Products/SLE-Module-Development-Tools/15-SP6/x86_64/product/ - refresh: true development_tools_repo_updates: pkgrepo.managed: - baseurl: http://dist.nue.suse.com/ibs/SUSE/Updates/SLE-Module-Development-Tools/15-SP6/x86_64/update/ - refresh: true HPC_repo_pool: pkgrepo.managed: - baseurl: http://dist.nue.suse.com/ibs/SUSE/Products/SLE-Module-HPC/15-SP6/x86_64/product/ - refresh: true HPC_repo_updates: pkgrepo.managed: - baseurl: http://dist.nue.suse.com/ibs/SUSE/Updates/SLE-Module-HPC/15-SP6/x86_64/update/ - refresh: true containers_repo_pool: pkgrepo.managed: - baseurl: http://dist.nue.suse.com/ibs/SUSE/Products/SLE-Module-Containers/15-SP6/x86_64/product/ - refresh: true containers_repo_updates: pkgrepo.managed: - baseurl: http://dist.nue.suse.com/ibs/SUSE/Updates/SLE-Module-Containers/15-SP6/x86_64/update/ - refresh: true python3_repo_pool: pkgrepo.managed: - baseurl: http://dist.nue.suse.com/ibs/SUSE/Products/SLE-Module-Python3/15-SP6/x86_64/product/ - refresh: true python3_repo_updates: pkgrepo.managed: - baseurl: http://dist.nue.suse.com/ibs/SUSE/Updates/SLE-Module-Python3/15-SP6/x86_64/update/ - refresh: true systems_management_repo_pool: pkgrepo.managed: - baseurl: http://dist.nue.suse.com/ibs/SUSE/Products/SLE-Module-Systems-Management/15-SP6/x86_64/product/ - refresh: true systems_management_repo_updates: pkgrepo.managed: - baseurl: http://dist.nue.suse.com/ibs/SUSE/Updates/SLE-Module-Systems-Management/15-SP6/x86_64/update/ - refresh: true

In my personal opinion, it's harder to notice the differences in the second case.

agraul · 2025-06-13T11:44:29Z

Thank you for taking a look, much appreciated!

From the perspective of refactoring, two things happened at once:

Removing non-SUSE salt-classic code paths

There was no non-SUSE classic code path. The old SLS file started with {% if grains['os'] == 'SUSE' %}, rendering the old SLS for e.g. Alma was always empty. If that's easier to see now, it's a win in my book.

[root@salt-shaker-products-testing-almalinux8-bundle ~]# venv-salt-call --local slsutil.renderer /root/salt/salt_testenv/salt_classic_package.sls 
local:
    ----------

(if you add default_renderer=jinja you see the non-stripped whitespace in the rendered yml but nothing more)

Refactoring remaining code paths

From that perspective, this seems good to me as well, though the refactoring is surprisingly big IMHO and difficult to review for corner cases.

Can you expand a bit on this? Which parts are bigger than you expected? Do you have any corner cases in mind? The goal of the refactoring was to make it easier to understand and reason about. If this goes in the wrong direction, we shouldn't merge it. I kept the commits separately for this purpose.

I have tested, for example, that the repos correspond to the repos in our salt CI testsuite. As a side note, I'm missing the salttest repo, but I couldn't find it in the sumaform, so I expect we add that somewhere in the CI.

I wasn't aware of salttest. I wonder what its purpose is, thank you for pointing it out.

It would be good to try this with our CI first to ensure we haven't missed some corner cases, but I can also see merging this now and fixing CI errors if there are any afterwards.

I'll leave that decision up to you :)

Might be easier to merge here than to re-configure or copy-and-edit all Jenkins jobs. We can wait a few day and merge when we don't change Salt to isolate problem sources.

srbarrios · 2025-07-02T12:34:53Z

backend_modules/libvirt/host/user_data.yaml

      enabled: true
      gpgcheck: false
      name: tools_pool_repo

+packages:


We avoid using packages in cloud-init for a good reason.
It was related to how cloud-init automatically update all the packages installed on the system, when using packages. And we don't want that, as we want to be sure we also have a pristine image using the packages from the ISO.
I would need to dig into old PRs to demonstrate it, but that's what I remember.

I think that behavior is fine for Tumbleweed. It doesn't really make sense to update old snapshots. But good to know we don't use the declarative approach for a reason, would be nice if that was documented :)

I take care of the dist upgrade in the Salt state instead, if you prefer that.

We find out that it was hard to have a deterministic environment if the packages after deployment were changing without our control. That was the main reason for that change.

So, if we plan to cover this system in our BV, better if we don't run a dist upgrade, but instead, we download a newer image whenever we decide.

Still, if you need it in your tests, we indeed have a variable in sumaform to perform this in salt states.

I'm not sure that approach will work well for Tumbleweed, but I'll change the default.update instead.

backend_modules/libvirt/host/user_data.yaml

Tumbleweed has enabled SELinux nowadays. The old way of enabling root login does not work anymore. Instead we need to install an additional package that contains a config file with the correct SELinux context.

We only want the Salt RPMs from our repository installed, not from the distribution. This is done by removing all python3*-salt packages before installing what we want from our devel repos

A lot of repetition is avoided by using jinja features. This is a test, if we like using it we can expand it to more of sumaform. I wanted to use less `cmd.run` and hit a known bug for salt-call in MicroOS, where `--local` is not passed. I left the intended code in the file, commented out, until we fix Salt.

They are already present in the tumbleweedo image. We can't add them again in repos.os, Salt refuses to add a repo when another repo with the same baseurl already exists.

meaksh

Let's give it a try, thanks 👍

agraul requested a review from a team as a code owner June 4, 2025 16:19

m-czernek approved these changes Jun 13, 2025

View reviewed changes

srbarrios requested changes Jul 2, 2025

View reviewed changes

srbarrios reviewed Jul 2, 2025

View reviewed changes

backend_modules/libvirt/host/user_data.yaml Show resolved Hide resolved

agraul force-pushed the fix-tw-provisioning branch from 5a34416 to 84926d1 Compare July 4, 2025 12:30

agraul added 5 commits July 4, 2025 15:12

Tumbleweed: Allow SSH login as root with password

00210a7

Tumbleweed has enabled SELinux nowadays. The old way of enabling root login does not work anymore. Instead we need to install an additional package that contains a config file with the correct SELinux context.

salt_testenv: Install correct Salt RPMs

d593208

We only want the Salt RPMs from our repository installed, not from the distribution. This is done by removing all python3*-salt packages before installing what we want from our devel repos

user_data.yml: strip template whitespace

4f7b4ba

TW: install venv-salt-minion

e6cadc8

agraul force-pushed the fix-tw-provisioning branch from 84926d1 to 9184002 Compare July 4, 2025 13:14

Do not disable Tumbleweed OS repos

81787b4

They are already present in the tumbleweedo image. We can't add them again in repos.os, Salt refuses to add a repo when another repo with the same baseurl already exists.

agraul force-pushed the fix-tw-provisioning branch from 9184002 to 81787b4 Compare July 4, 2025 13:33

m-czernek requested a review from meaksh July 29, 2025 12:01

meaksh approved these changes Jul 30, 2025

View reviewed changes

srbarrios approved these changes Jul 30, 2025

View reviewed changes

meaksh merged commit af5ab20 into uyuni-project:master Jul 31, 2025
2 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

`salt_testenv`: Enable Tumbleweed deployments and clean up #1849

`salt_testenv`: Enable Tumbleweed deployments and clean up #1849

Uh oh!

agraul commented Jun 4, 2025 •

edited

Loading

Uh oh!

m-czernek left a comment

Uh oh!

m-czernek Jun 13, 2025

Uh oh!

agraul Jul 2, 2025

Uh oh!

m-czernek Jun 13, 2025

Uh oh!

m-czernek Jun 13, 2025

Uh oh!

agraul Jun 13, 2025

Uh oh!

agraul commented Jun 13, 2025

Uh oh!

srbarrios Jul 2, 2025

Uh oh!

agraul Jul 2, 2025

Uh oh!

srbarrios Jul 3, 2025 •

edited

Loading

Uh oh!

agraul Jul 4, 2025

Uh oh!

Uh oh!

meaksh left a comment

Uh oh!

Uh oh!

Uh oh!

salt_testenv: Enable Tumbleweed deployments and clean up #1849

salt_testenv: Enable Tumbleweed deployments and clean up #1849

Uh oh!

Conversation

agraul commented Jun 4, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

m-czernek left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

agraul commented Jun 13, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

srbarrios Jul 3, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

meaksh left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

`salt_testenv`: Enable Tumbleweed deployments and clean up #1849

`salt_testenv`: Enable Tumbleweed deployments and clean up #1849

agraul commented Jun 4, 2025 •

edited

Loading

srbarrios Jul 3, 2025 •

edited

Loading