search

saltstack-formulas / zabbix-formula

http://docs.saltstack.com/en/latest/topics/development/conventions/formulas.html
Other
50 stars 121 forks source link

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

Open OrangeDog opened 4 years ago

OrangeDog commented 4 years ago

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'
myii commented 4 years ago

@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:

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

Try setting the pillar like this:

https://github.com/saltstack-formulas/zabbix-formula/blob/8291d7c77d1061eaba1aa74884a9a037dd60a3bc/test/salt/pillar/default.sls#L4-L7

myii commented 4 years ago

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 commented 4 years ago

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 commented 4 years ago

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

OrangeDog commented 4 years ago

No I didn't. image

myii commented 4 years ago

@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:

OrangeDog commented 4 years ago

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

myii commented 4 years ago

@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:

Various repos configured for 4.4:

Various 4.4 packages installed:

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

OrangeDog commented 4 years ago

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 commented 4 years ago

@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:

OrangeDog commented 4 years ago

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

pillar.get:
{
    "local": {
        "lookup": {
            "version_repo": "4.4"
        }
    }
}
config.get:
{
    "local": {
        "psk": "..."
    }
}
myii commented 4 years ago

@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:

https://github.com/saltstack-formulas/zabbix-formula/blob/8291d7c77d1061eaba1aa74884a9a037dd60a3bc/zabbix/map.jinja#L11

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

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

myii commented 4 years ago

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

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 commented 4 years ago

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 commented 4 years ago

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 commented 4 years ago

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 commented 4 years ago

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 commented 4 years ago

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 commented 4 years ago

Looks like that fixes it.

myii commented 4 years ago

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 commented 4 years ago

Why can't it always be recurse?

myii commented 4 years ago

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.

At other points, there were other issues raised including:

OrangeDog commented 4 years ago

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 commented 4 years ago

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 commented 4 years ago

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).