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

salt.minion state does not run on pre 2017.7.0 minions due to pillar.filter_by #405

Open
gizahNL opened this issue Mar 26, 2019 · 10 comments

Comments

@gizahNL
Copy link

gizahNL commented Mar 26, 2019

pillar.filter_by was introduced in 2017.7.0, since 2016.11 is still in support the formula should not have hard dependencies on functionality that is not there.

@gizahNL
Copy link
Author

gizahNL commented Mar 26, 2019

diff --git a/salt/osfamilymap.yaml b/salt/osfamilymap.yaml
index 7852dee38f9882a2693d6eb3f150bf42fd287476..5c3725a339a64d5e92c86412fd3b6b65b4bb7263 100644
--- a/salt/osfamilymap.yaml
+++ b/salt/osfamilymap.yaml
@@ -3,7 +3,12 @@

{% import_yaml "salt/ospyvermap.yaml" as ospyvermap %}
{% set ospyver = salt['grains.filter_by'](ospyvermap, grain='os_family') or {} %}
-{% set py_ver_dir = salt['pillar.filter_by'](ospyver, pillar='salt:py_ver', default='py2') %}
+{% set pillar_py_ver = salt'pillar.get' %}
+{% if pillar_py_ver == 'py3' %}
+{% set py_ver_dir = ospyver.py3 %}
+{% else %}
+{% set py_ver_dir = ospyver.py2 %}
+{% endif %}

{% set osrelease = salt'grains.get' %}
{% set salt_release = salt'pillar.get' %}
diff --git a/salt/osmap.yaml b/salt/osmap.yaml
index 3d88110a2c2e7d5fa4c0660286c97d82e10cb72d..f2e66fe10705b49e18b081166dd6e181fe72a9c6 100644
--- a/salt/osmap.yaml
+++ b/salt/osmap.yaml
@@ -3,7 +3,14 @@

{% import_yaml "salt/ospyvermap.yaml" as ospyvermap %}
{% set ospyver = salt['grains.filter_by'](ospyvermap, grain='os_family') or {} %}
-{% set py_ver_dir = salt['pillar.filter_by'](ospyver, pillar='salt:py_ver', default='py2') %}
+{% set pillar_py_ver = salt'pillar.get' %}
+{% if pillar_py_ver == 'py3' %}
+{% set py_ver_dir = ospyver.py3 %}
+{% else %}
+{% set py_ver_dir = ospyver.py2 %}
+{% endif %}
+
+

{% set osrelease = salt'grains.get' %}
{% set salt_release = salt'pillar.get' %}

@myii
Copy link
Member

myii commented Mar 26, 2019

@gizahNL You're absolutely right, looking at the commit, the earliest version supported is v2017.5. But now we reach a couple of issues:

  • Looking at https://s.saltstack.com/product-support-lifecycle/, 2016.11 is only supported until May 31, 2019, just over 2 months remaining.
  • There are some suggesting that SaltStack Formulas shouldn't even have to support up to the "Extended life support ends" date and that even the "Phase 2 support ends" date is sufficient -- and in terms of maintenance of all of these formulas, that makes a lot of sense.
  • Even if we eventually settle on the "Phase 3 support ends" date, 2016.11 is out of the equation.

@gizahNL Would you be happy to prepare the PR for this change and the PR to revert this in 2 months time?
@aboe76 What are your thoughts about this?

@myii
Copy link
Member

myii commented Mar 26, 2019

@gizahNL OK, just had a significant update from upstream in our Slack/IRC/Matrix room. The discussion starting from here:

15:41 <myii> @​gtmanfred The issue of supporting older versions of Salt keeps coming up. Looking at <s.saltstack.com/product-support-lifecycle>, the oldest version still supported ("Extended life support ends") is 2016.11. But there have been suggestions that SaltStack Formulas shouldn't have to go that far and that maybe even "Phase 2" is enough. The difference in terms of maintenance is significant. What's your take on the level of su
15:43 <gtmanfred> That needs to be updated 2017.7 is now the oldest supported
15:43 <gtmanfred> @​mwilhite do you know who would update this? ^^
15:43 <myii> What phase should we be supporting up to? Phase 2, 3 or extended?
15:44 <gtmanfred> I think once something is in cve only support it can be dropped from formulas
15:44 <myii> Sounds good, I've started feeling like Phase 3 is the realistic answer.
15:44 <gtmanfred> I wouldn't actively remove support, but I would stop testing it.
15:45 <myii> Right, but that means that PRs must still consider those versions in extended support, right?
15:46 <mwilhite> im on it, i'll ping someone to get it updated. thanks
15:47 <myii> @​mwilhite So that I can report back on a current PR, do you know the date that 2016.11 went out of extended support?
15:48 <mwilhite> the date that it actually went out of extended support was when we released 2019.2. We always try to support 3 releases. So now we are supporting 2019.2 (latest), 2018.3 (revious), 2017.7 (cve only- althought we are still planning on doing one more bug fix release since we couldn't get it out before 2019.2)

Essentially, this issue is invalid. I'll leave it open for now for any remaining discussion.

@gizahNL
Copy link
Author

gizahNL commented Mar 26, 2019

I was under the impression 2016.11 would still be supported, no problem though, the patch I wrote is simple (though of course less elegant than a online pillar.filter_by) and should be easy enough to rebase for me if and when we need to pull changes in from here.
May I suggest though either mentioning in the readme or in the formula file the minimum required salt version?
Perhaps also mentioning that X is unsupported would save time dealing with problem reports like mine ;)

@myii
Copy link
Member

myii commented Mar 26, 2019

@gizahNL I wouldn't bother with the PR now, since there's no reason to make the change! In terms of documentation, there's work being done to find a better way of explaining these sorts of things. Particularly issues that affect all formulas. In the meantime, PRs improving the documentation are welcome, with important contributions getting promoted to the new documentation when it arrives.

Thanks for raising this issue in the first place.

@myii
Copy link
Member

myii commented Apr 1, 2019

@gizahNL Are you OK with this being closed?

@gboddin
Copy link

gboddin commented Apr 3, 2019

Hi, thank you for all the details @myii much appreciated !

I have to say, I would consider it normal for any other forumlas ...

I do think it makes sense however for this precise formula to support older (supported) versions of salt since it's probably gonna be used to upgrade it anyway.

At least in our case we make sure whatever minions comes in have same version of salt-minion installed, without having to alter default distro repos during boostrap ( which saves a lot of time on templating ).

It's no big deal we can keep a fork, just my 2ct that this repo is probably used as a bootstrap repo as well :)

@myii
Copy link
Member

myii commented Apr 3, 2019

@gboddin Thanks for transferring here and closing the duplicate issue. You raise some decent points and I'd like to draw in some other opinions about the right way to approach this. @gizahNL Please ignore my comment above about closing this issue, this needs to be discussed more widely.

@aboe76 @javierbertoli The general agreement for supporting older versions of Salt is to mirror upstream Salt, which is 2017.7 now. Do we apply that to this formula as well or do we make an exception here? If so, how far back? This also affects other aspects of this formula including how to provide the master and minion configs, as started in #398.

@gboddin
Copy link

gboddin commented Apr 5, 2019

A bit of copy paste from the ubuntu side :

trusty (14.04LTS) (admin): client package for salt, the distributed remote execution system [universe]
0.17.5+ds-1: all
xenial (16.04LTS) (admin): client package for salt, the distributed remote execution system [universe]
2015.8.8+ds-1: all
bionic (18.04LTS) (admin): client package for salt, the distributed remote execution system [universe]
2017.7.4+dfsg1-1: all 

The 14.04 LTS of ubuntu ends this month so we can forget 0.17, but 16.04 ( ending 2021 ) is still 2015.8, thanks for considering, I will invest a bit of time to work on this and maybe add test cases to check for salt upgrades .

@aboe76
Copy link
Member

aboe76 commented Apr 5, 2019

@gboddin There is a reason ubuntu put those packages in Universe...those are community maintained versions... not officially maintained by Ubuntu.

So these could be swapped by the repo's supported by saltstack and thus upgraded to a version saltstack supports.

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

No branches or pull requests

4 participants