[BUG] breaking change in 0.21.3 (version_repo) #135

OrangeDog · 2020-03-23T11:02:47Z

Your setup

Formula commit hash / release tag

0.21.3

Versions reports (master & minion)

Salt Version:
           Salt: 2019.2.3

Dependency Versions:
           cffi: Not Installed
       cherrypy: Not Installed
       dateutil: 2.6.1
      docker-py: Not Installed
          gitdb: 2.0.3
      gitpython: 2.1.8
          ioflo: Not Installed
         Jinja2: 2.10
        libgit2: 0.26.0
        libnacl: Not Installed
       M2Crypto: 0.32.0
           Mako: Not Installed
   msgpack-pure: Not Installed
 msgpack-python: 0.5.6
   mysql-python: Not Installed
      pycparser: Not Installed
       pycrypto: 2.6.1
   pycryptodome: Not Installed
         pygit2: 0.26.2
         Python: 3.6.9 (default, Nov  7 2019, 10:44:02)
   python-gnupg: 0.4.1
         PyYAML: 3.12
          PyZMQ: 16.0.2
           RAET: Not Installed
          smmap: 2.0.3
        timelib: Not Installed
        Tornado: 4.5.3
            ZMQ: 4.2.5

System Versions:
           dist: Ubuntu 18.04 bionic
         locale: ISO-8859-1
        machine: x86_64
        release: 4.15.0-91-generic
         system: Linux
        version: Ubuntu 18.04 bionic

Pillar / config used

zabbix:
  lookup:
    version_repo: '4.2'

Bug details

Describe the bug

Upgrading from 0.21.2 to 0.21.3 changed how the repo is configured.
It's now trying to use the default, which does not exist

The repository 'https://repo.zabbix.com/zabbix/2.2/ubuntu bionic Release' does not have a Release file.

Steps to reproduce the bug

See above.

Expected behaviour

No changes to existing behaviour on minor/patch releases.

Attempts to fix the bug

Had a look at the code and tried this pillar instead, but it had no effect.

zabbix:
  version_repo: '4.2'

The text was updated successfully, but these errors were encountered:

myii · 2020-03-23T11:17:25Z

@OrangeDog I'm certain that this problem actually exists in 0.21.2 as well, since absolutely nothing changed in the map to 0.21.3 -- I tested that extensively:

https://travis-ci.org/github/myii/zabbix-formula/builds/665299638

The issue is the upstream repo itself. 2.2 is ancient and only available for precise and trusty:

https://repo.zabbix.com/zabbix/2.2/ubuntu/dists/

Try setting the pillar like this:

zabbix-formula/test/salt/pillar/default.sls

Lines 4 to 7 in 8291d7c

    
           zabbix: 
        
             # Overrides map.jinja 
        
             lookup: 
        
               version_repo: '4.4'

myii · 2020-03-23T11:18:21Z

Let me just add, the formula definitely needs updating from 2.2 as the default. There's already an active discussion about that here: #123. Just need someone to propose an appropriate PR.

OrangeDog · 2020-03-23T11:23:06Z

I'm certain that this problem actually exists in 0.21.2 as well

No. I just reverted back to that and the problem went away.

Try setting the pillar like this

That's exactly what I did, as I put in the report above.

myii · 2020-03-23T11:23:55Z

@OrangeDog You've missed the lookup from the pillar.

myii · 2020-03-23T11:29:42Z

@OrangeDog Apologies, I saw the last entry in your original report. Well, this is going to be fun to resolve, we've got the same configuration (ubuntu-1804-2019-2-py3) working in Travis for 0.21.3:

https://travis-ci.com/github/saltstack-formulas/zabbix-formula/jobs/300744317

OrangeDog · 2020-03-23T11:50:20Z

Hmm, I don't know what else would cause a difference, assuming your tests aren't broken in some way.

myii · 2020-03-23T11:59:39Z

@OrangeDog We've got debug logging active, so you can see exactly what's rendered. So the key places clearly show what's going on.

Rendered pillar data:

https://travis-ci.com/github/saltstack-formulas/zabbix-formula/jobs/300744317#L638-L645

Various repos configured for 4.4:

Various 4.4 packages installed:

InSpec tests passing for the specific package versions installed for 4.4:

https://travis-ci.com/github/saltstack-formulas/zabbix-formula/jobs/300744317#L4089-L4093

OrangeDog · 2020-03-23T12:11:00Z

I also have the zabbix module properties set on this host. Maybe that messes up the merging now?
They already have to use different yaml syntax.

zabbix.url: ...
zabbix.user: ...

zabbix:
  lookup:
    version_repo: '4.4'

myii · 2020-03-23T13:04:19Z

@OrangeDog The one potential difference is the use of config.get vs. pillar.get. Can you share any differences in the output of the following:

salt <minion> pillar.get zabbix
salt <minion> config.get zabbix

OrangeDog · 2020-03-23T13:13:19Z

On my currently working system (the psk is in a grain).

pillar.get:
{
    "local": {
        "lookup": {
            "version_repo": "4.4"
        }
    }
}

config.get:
{
    "local": {
        "psk": "..."
    }
}

myii · 2020-03-23T13:34:32Z

@OrangeDog Back when we first started using config.get, there was a question about the merge strategy to use. We did consider allowing that to be configurable (see the Jinja snippet in this comment). Would you mind testing the following change:

zabbix-formula/zabbix/map.jinja

Line 11 in 8291d7c

{%- set _config = salt['config.get']('zabbix', default={}) %}

-{%- set _config = salt['config.get']('zabbix', default={}) %}
+{%- set _config = salt['config.get']('zabbix', default={}, merge='recurse')

OrangeDog · 2020-03-23T14:09:09Z

Using that commit hasn't had any apparent effect: the state still tries to use the 2.2 repo.

myii · 2020-03-23T14:14:34Z

@OrangeDog So is there any difference between the output of these two commands?

salt <minion> config.get zabbix
salt <minion> config.get zabbix merge=recurse

If not, would you mind sharing the layout of how you're providing that grain, so that I can test it at my end?

OrangeDog · 2020-03-23T14:24:48Z

With merge=recurse, it includes the "lookup": {"version_repo": "4.4"}.
The grain is set via the jinja in this state:

/etc/zabbix/psk:
  file.managed:
    - contents:
      - {{ salt['grains.get_or_set_hash']('zabbix:psk', length=64, chars='0123456789abcdef') }}
    - user: {{ zabbix.user }}
    - mode: 400
    - watch_in:
      - service: zabbix-agent

OrangeDog · 2020-03-23T14:28:47Z

That sls also includes stuff from the formula, in case that makes a difference.

{% from "zabbix/map.jinja" import zabbix %}

include:
  - zabbix.agent.conf
  - zabbix.agent.repo

myii · 2020-03-23T14:37:26Z

With merge=recurse, it includes the "lookup": {"version_repo": "4.4"}.

@OrangeDog Excellent, we're on the right path then.

Using that commit hasn't had any apparent effect: the state still tries to use the 2.2 repo.

Do you mean you tried commit 8291d7c? That doesn't contain merge='recurse'. That was to indicate the line that needs to be changed accordingly:

-{%- set _config = salt['config.get']('zabbix', default={}) %}
+{%- set _config = salt['config.get']('zabbix', default={}, merge='recurse')

OrangeDog · 2020-03-23T14:38:47Z

Oh, yes, I configured the master to use that commit instead of a tag.
You want me to pull the source and patch it instead?

myii · 2020-03-23T14:39:48Z

Oh, yes, I configured the master to use that commit instead of a tag.
You want me to pull the source and patch it instead?

Yes, please.

OrangeDog · 2020-03-23T15:04:18Z

Looks like that fixes it.

myii · 2020-03-23T15:10:57Z

Thanks, that answers the question about whether we need the merge strategy to be configurable for config.get. Now we need to figure out the right way to go about this, since we're using the (implicit) default of merge=None across numerous formulas now. I reckon we're going to have to keep that as the default and then allow the strategy to be configured in an appropriate way.

OrangeDog · 2020-03-23T15:13:15Z

Why can't it always be recurse?

myii · 2020-03-23T15:25:55Z

Why can't it always be recurse?

That's what I started with when first introducing config.get but various issues came up during discussions, including the fact that merge wasn't working at all on salt-ssh.

https://freenode.logbot.info/saltstack-formulas/20190515#c2178747
- Initial question
https://freenode.logbot.info/saltstack-formulas/20190515#c2179031-c2179052
- Problems on salt-ssh

At other points, there were other issues raised including:

Becoming too confusing with values merged in from multiple sources
- The idea was that config.get still allows you to pick a single source, rather than just the pillar
Performance concerns

OrangeDog · 2020-03-23T15:34:02Z

It would be workable if it were None (I'd just have to rename my grain), but the release notes didn't explain this (seemingly major) change.

myii · 2020-03-23T15:44:20Z

It would be workable if it were None (I'd just have to rename my grain), but the release notes didn't explain this (seemingly major) change.

I'm going to send a note to the main contributors explaining that pillar.get => config.get will now have to be considered a BREAKING CHANGE, from a semantic-release perspective. That will allow us to add the appropriate instructions that will reach the changelog.

OrangeDog · 2020-03-23T16:30:16Z

Thanks. Also +1 to allowing a merge. I imagine many people would want to split their config, keeping secrets in pillar but moving the other bits somewhere more performant (even though I've currently got almost the opposite).

OrangeDog added the bug label Mar 23, 2020

This comment has been minimized.

Sign in to view

myii mentioned this issue Mar 23, 2020

feat(map.jinja): load configuration values from configurable sources saltstack-formulas/template-formula#186

Closed

19 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[BUG] breaking change in 0.21.3 (version_repo) #135

[BUG] breaking change in 0.21.3 (version_repo) #135

OrangeDog commented Mar 23, 2020

myii commented Mar 23, 2020

myii commented Mar 23, 2020

OrangeDog commented Mar 23, 2020

myii commented Mar 23, 2020

This comment has been minimized.

myii commented Mar 23, 2020

OrangeDog commented Mar 23, 2020

myii commented Mar 23, 2020

OrangeDog commented Mar 23, 2020 •

edited

Loading

myii commented Mar 23, 2020

OrangeDog commented Mar 23, 2020 •

edited

Loading

myii commented Mar 23, 2020

OrangeDog commented Mar 23, 2020

myii commented Mar 23, 2020

OrangeDog commented Mar 23, 2020 •

edited

Loading

OrangeDog commented Mar 23, 2020

myii commented Mar 23, 2020

OrangeDog commented Mar 23, 2020

myii commented Mar 23, 2020

OrangeDog commented Mar 23, 2020

myii commented Mar 23, 2020 •

edited

Loading

OrangeDog commented Mar 23, 2020

myii commented Mar 23, 2020

OrangeDog commented Mar 23, 2020

myii commented Mar 23, 2020

OrangeDog commented Mar 23, 2020 •

edited

Loading

[BUG] breaking change in 0.21.3 (version_repo) #135

[BUG] breaking change in 0.21.3 (version_repo) #135

Comments

OrangeDog commented Mar 23, 2020

Your setup

Formula commit hash / release tag

Versions reports (master & minion)

Pillar / config used

Bug details

Describe the bug

Steps to reproduce the bug

Expected behaviour

Attempts to fix the bug

myii commented Mar 23, 2020

myii commented Mar 23, 2020

OrangeDog commented Mar 23, 2020

myii commented Mar 23, 2020

This comment has been minimized.

myii commented Mar 23, 2020

OrangeDog commented Mar 23, 2020

myii commented Mar 23, 2020

OrangeDog commented Mar 23, 2020 • edited Loading

myii commented Mar 23, 2020

OrangeDog commented Mar 23, 2020 • edited Loading

myii commented Mar 23, 2020

OrangeDog commented Mar 23, 2020

myii commented Mar 23, 2020

OrangeDog commented Mar 23, 2020 • edited Loading

OrangeDog commented Mar 23, 2020

myii commented Mar 23, 2020

OrangeDog commented Mar 23, 2020

myii commented Mar 23, 2020

OrangeDog commented Mar 23, 2020

myii commented Mar 23, 2020 • edited Loading

OrangeDog commented Mar 23, 2020

myii commented Mar 23, 2020

OrangeDog commented Mar 23, 2020

myii commented Mar 23, 2020

OrangeDog commented Mar 23, 2020 • edited Loading

OrangeDog commented Mar 23, 2020 •

edited

Loading

OrangeDog commented Mar 23, 2020 •

edited

Loading

OrangeDog commented Mar 23, 2020 •

edited

Loading

myii commented Mar 23, 2020 •

edited

Loading

OrangeDog commented Mar 23, 2020 •

edited

Loading