feat(package): reorg version-locking salt packages #525

mdschmitt · 2021-11-24T21:37:19Z

PR progress checklist (to be filled in by reviewers)

Changes to documentation are appropriate (or tick if not required)
Changes to tests are appropriate (or tick if not required)
Reviews completed

What type of PR is this?

Primary type

[build] Changes related to the build system
[chore] Changes to the build process or auxiliary tools and libraries such as documentation generation
[ci] Changes to the continuous integration configuration
[feat] A new feature
[fix] A bug fix
[perf] A code change that improves performance
[refactor] A code change that neither fixes a bug nor adds a feature
[revert] A change used to revert a previous commit
[style] Changes that do not affect the meaning of the code (white-space, formatting, missing semi-colons, etc.)

Secondary type

[docs] Documentation changes
[test] Adding missing or correcting existing tests

Does this PR introduce a `BREAKING CHANGE`?

Possibly. I'm not 100% sure how pkg.installed's "hold" parameter functions cross-platform. My testing was on an AmazonLinux 2 box, and I don't see anything specific in the integration tests for this particular item..

Related issues and/or pull requests

Describe the changes you're proposing

Add the ability to version-lock the yum packages for Salt, if desired.

Pillar / config required to test the proposed changes

On a RHEL-based system:

---
salt:
  version: 3004
  pin_version: True

Debug log showing how the proposed changes work

Documentation checklist

Updated the README (e.g. Available states).
Updated pillar.example.

Testing checklist

Included in Kitchen (i.e. under state_top).
Covered by new/existing tests (e.g. InSpec, Serverspec, etc.).
Updated the relevant test pillar.

Additional context

aboe76

Looks good as far as I can tell, this is based on my experience with rhel like systems.

hatifnatt · 2021-12-02T20:47:10Z

salt/cloud.sls

@@ -24,6 +29,9 @@ salt-cloud:
    {%- if salt_settings.version is defined %}
    - version: {{ salt_settings.version }}
    {%- endif %}
+    {%- if salt_settings.pin_version %}
+    - hold: True
+    {%- endif %}


Don't you think you need to also add update_holds: True here and in other similar places to allow upgrade or downgrade?

update_holds (bool) --
If True, and this function would update the package version, any packages which are being held will be temporarily unheld so that they can be updated. Otherwise, if this function attempts to update a held package, the held package(s) will be skipped and the state will fail. By default, this parameter is set to False.
Supported on YUM/DNF & APT based systems.

Currently you don't provide any way to unhold packages via formula.

hatifnatt · 2021-12-02T21:09:24Z

Version hold is looks very similar with version pinning but it is not exactly the same thing. If I'm not mistaken hold will only hold explicitly installed packages, and pin can pin packages by mask - in current case it's salt*
That's the main reason why in my original RP #458 I'm using pinning and not hold. By using pinning you can successfully downgrade all salt packages

allow package downgrade, simple version specification is not enough for downgrade because related packages won't be downgraded, i.e. if you try to downgrade from version 3000 to 2019.2.3 with apt-get install salt-minion=2019.2.* you will face error because apt will try to install version 3000 of salt-common

I'm almost certain that is not possible by simple hold.

salt/api.sls

salt/pin.sls

salt/master.sls

mdschmitt · 2022-03-30T21:49:18Z

@hatifnatt maybe you could explain the difference between "pinning" and "hold"? On the surface, they sound like the same thing... I'm having a hard time finding anything in the official Salt module index specifically about "pinning."

mdschmitt · 2022-03-30T22:17:30Z

Regarding hold being able to address wildcards like "salt*" I think that it can like so:

[me@myserver pluginconf.d]$ sudo yum versionlock add salt\*
Loaded plugins: extras_suggestions, langpacks, priorities, update-motd, versionlock
Adding versionlock on: 0:salt-master-3004-1.amzn2
Adding versionlock on: 0:salt-3004-1.amzn2
Adding versionlock on: 0:salt-minion-3004-1.amzn2
versionlock added: 3

hatifnatt · 2022-03-30T22:35:46Z

#525 (comment)
I don't think I can explain better than I have already did in message above.

Regarding hold being able to address wildcards like "salt*" I think that it can like so:

But can you hold package prior installation? Does hold help you with downgrade? Main reason why I'm using pinning - it helps with downgrade. Also in the formula you only set hold for, let's call it "main" packages i.e. salt-minion or salt-master, salt-common still won't be on hold.

hatifnatt · 2022-03-30T22:54:32Z

@myii can you please help with failing test? I don't think it's related with PR itself but I'm not familiar with all kitchen stuff.

mdschmitt · 2022-03-31T06:29:02Z

@hatifnatt
I don't know for certain whether or not hold helps with downgrade but I would assume that it handles that case. Maybe someone else knows? I don't have the infra in place to try it out at the moment. :/

I think my question is more along the lines of...should I make this PR a breaking change and replace all the pin_version stuff with the "native" Salt functionality provided by hold? I don't know how to handle the fact that we wouldn't hold salt-common, but I feel like the hard-coded salt* wildcard and /etc/apt/preferences.d/salt file is a little less flexible... What do you think?

Btw, your point about update_holds was totally spot-on. Thanks for that.

hatifnatt

Addition of hold.sls is definitely an improvement, otherwise formula can fail on systems where versionlock plugin is not installed, but in the rest previous version was better and cleaner.

hatifnatt · 2022-03-31T11:27:33Z

pillar.example

-  # Pin version provided under 'version' key by using apt-pinning
-  # available only on Debian family OS-es
-  pin_version: false


I don't see any reason why you need to delete pinning in favor to hold. Both options can coexist without any trouble.

hatifnatt · 2022-03-31T11:44:19Z

salt/api.sls

+  {%- if salt_settings.hold_version is defined %}
+    - hold: {{ salt_settings.hold_version }}
+  {%- endif %}
+  {%- if salt_settings.update_holds is defined %}
+    - update_holds: {{ salt_settings.update_holds }}
+  {%- endif %}


Suggested change

{%- if salt_settings.hold_version is defined %}

- hold: {{ salt_settings.hold_version }}

{%- endif %}

{%- if salt_settings.update_holds is defined %}

- update_holds: {{ salt_settings.update_holds }}

{%- endif %}

- hold: {{ salt_settings.get('hold_version', False) }}

- update_holds: {{ salt_settings.get('update_holds', False ) }}

I would suggest something like this - less Jinja - better readability.
But even better is to preserve defaults in defaults.yaml and wait for comment about failing pipelines from @myii

hatifnatt · 2022-03-31T11:49:37Z

salt/master.sls

@@ -2,7 +2,7 @@
 {%- from tplroot ~ "/map.jinja" import salt_settings with context %}
 {%- from tplroot ~ "/libtofs.jinja" import files_switch with context %}

-{% if salt_settings.pin_version and salt_settings.version and grains.os_family|lower == 'debian' %}
+{% if salt_settings.hold_version is defined and salt_settings.install_packages %}


And again don't replace pin with hold, just add hold.

salt/master.sls

hatifnatt · 2022-03-31T11:52:08Z

salt/minion.sls

+{% if salt_settings.hold_version is defined and salt_settings.install_packages %}
 include:
-  - .pin
+  - .hold
 {% endif %}


And there you are replacing .pin with .hold

hatifnatt · 2022-03-31T11:58:03Z

salt/hold.sls

+salt-install-hold-plugin:
+  pkg.installed:
+  {% if grains.osmajorrelease > 7 %}
+    - name: python3-dnf-plugin-versionlock
+  {% else %}
+    - name: yum-plugin-versionlock
+  {% endif %}


I would suggest to put package name into variable and read it from osfingermap file. osmajorrelease based condition is only applicable for RedHat derivatives I suppose it will fail for SUSE.

mdschmitt · 2022-03-31T21:10:01Z

Excellent feedback. Give me a little bit to try to clean this up.

mdschmitt · 2022-03-31T21:17:08Z

I think you're right about the legibility and "less Jinja" suggestion when you say I should do this:

     - hold: {{ salt_settings.get('hold_version', False) }}
     - update_holds: {{ salt_settings.get('update_holds', False ) }}

But unfortunately, if the necessary yum plugin is not installed, having hold and/or update_holds defined in the state resource will outright fail the pkg.installed resource. Example: https://gitlab.com/saltstack-formulas/salt-formula/-/jobs/2272883059#L1879

If you see another cleaner way to do it, I'm all ears.

Should we just install the respective versionlock package regardless of any pillar settings in this formula? That seems a little heavy-handed..

hatifnatt · 2022-04-01T17:15:06Z

You can have less Jinja something like this:

{% if salt_settings.install_packages and (salt_settings.hold_version or salt_settings.update_holds) %}
include:
  - {{ tplroot }}.hold
{% endif %}

...

salt-master:
    {% if salt_settings.install_packages %}
  pkg.installed:
    ...
    {%- if salt_settings.hold_version or salt_settings.update_holds %}
    - hold: {{ salt_settings.get('hold_version', False) }}
    - update_holds: {{ salt_settings.get('update_holds', False ) }}
    {%- endif %}
    - require:
    {%- if salt_settings.hold_version or salt_settings.update_holds %}
      - sls: {{ tplroot }}.hold
    {%- endif %}

This way versionlock plugin will be installed prior main packages and hold and update_holds values will be added to state only if one of them is true.
Note, this code is not verified, so indentation can be incorrect, but I hope you get the idea. Also note - sls: {{ tplroot }}.hold - you can't require sls with relative path.

mdschmitt · 2023-08-24T02:40:53Z

Nevermind. This is more trouble than it's worth.

mdschmitt requested review from hatifnatt and a team as code owners November 24, 2021 21:37

mdschmitt changed the title ~~feat(redhat): allow version-locking salt packages~~ feat(package): reorg version-locking salt packages Nov 24, 2021

aboe76 approved these changes Dec 2, 2021

View reviewed changes

hatifnatt reviewed Dec 2, 2021

View reviewed changes

hatifnatt requested changes Dec 2, 2021

View reviewed changes

salt/api.sls Outdated Show resolved Hide resolved

salt/pin.sls Outdated Show resolved Hide resolved

salt/master.sls Outdated Show resolved Hide resolved

hatifnatt approved these changes Mar 30, 2022

View reviewed changes

hatifnatt requested changes Mar 31, 2022

View reviewed changes

feat(package): allow for salt package version hold

11b017e

mdschmitt marked this pull request as draft April 2, 2022 04:09

mdschmitt closed this Aug 24, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(package): reorg version-locking salt packages #525

feat(package): reorg version-locking salt packages #525

mdschmitt commented Nov 24, 2021 •

edited

Loading

aboe76 left a comment

hatifnatt Dec 2, 2021 •

edited

Loading

hatifnatt commented Dec 2, 2021

mdschmitt commented Mar 30, 2022

mdschmitt commented Mar 30, 2022

hatifnatt commented Mar 30, 2022

hatifnatt commented Mar 30, 2022

mdschmitt commented Mar 31, 2022

hatifnatt left a comment

hatifnatt Mar 31, 2022

hatifnatt Mar 31, 2022

hatifnatt Mar 31, 2022

hatifnatt Mar 31, 2022

hatifnatt Mar 31, 2022

mdschmitt commented Mar 31, 2022

mdschmitt commented Mar 31, 2022 •

edited

Loading

hatifnatt commented Apr 1, 2022

mdschmitt commented Aug 24, 2023

feat(package): reorg version-locking salt packages #525

feat(package): reorg version-locking salt packages #525

Conversation

mdschmitt commented Nov 24, 2021 • edited Loading

PR progress checklist (to be filled in by reviewers)

What type of PR is this?

Primary type

Secondary type

Does this PR introduce a BREAKING CHANGE?

Related issues and/or pull requests

Describe the changes you're proposing

Pillar / config required to test the proposed changes

Debug log showing how the proposed changes work

Documentation checklist

Testing checklist

Additional context

aboe76 left a comment

Choose a reason for hiding this comment

hatifnatt Dec 2, 2021 • edited Loading

Choose a reason for hiding this comment

hatifnatt commented Dec 2, 2021

mdschmitt commented Mar 30, 2022

mdschmitt commented Mar 30, 2022

hatifnatt commented Mar 30, 2022

hatifnatt commented Mar 30, 2022

mdschmitt commented Mar 31, 2022

hatifnatt left a comment

Choose a reason for hiding this comment

hatifnatt Mar 31, 2022

Choose a reason for hiding this comment

hatifnatt Mar 31, 2022

Choose a reason for hiding this comment

hatifnatt Mar 31, 2022

Choose a reason for hiding this comment

hatifnatt Mar 31, 2022

Choose a reason for hiding this comment

hatifnatt Mar 31, 2022

Choose a reason for hiding this comment

mdschmitt commented Mar 31, 2022

mdschmitt commented Mar 31, 2022 • edited Loading

hatifnatt commented Apr 1, 2022

mdschmitt commented Aug 24, 2023

mdschmitt commented Nov 24, 2021 •

edited

Loading

Does this PR introduce a `BREAKING CHANGE`?

hatifnatt Dec 2, 2021 •

edited

Loading

mdschmitt commented Mar 31, 2022 •

edited

Loading