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

"top_file_merging_strategy: same" is functionally identical to specifying an environment #35045

Closed
GarthSnyder opened this issue Jul 28, 2016 · 7 comments · Fixed by #35822
Closed
Assignees
Labels
Bug broken, incorrect, or confusing behavior Core relates to code central or existential to Salt fixed-pls-verify fix is linked, bug author to confirm fix P1 Priority 1 severity-medium 3rd level, incorrect or bad functionality, confusing and lacks a work around State-Compiler ZRELEASED - Carbon
Milestone

Comments

@GarthSnyder
Copy link

GarthSnyder commented Jul 28, 2016

[Please see also #34975. You'll need to set top_file_merging_strategy: same on the master to see this effect, as this option currently has no effect on a minion.]

The docs for top_file_merging_strategy say "...When set to same, then for each environment, only that environment's top file is processed, with the others being ignored. For example, only the dev environment's top file will be processed for the dev environment, and any SLS targets defined for dev in the base environment's (or any other environment's) top file will be ignored. If an environment does not have a top file, then the top file from the default_top config parameter will be used as a fallback."

That doesn't appear to be true; the default_top is applied in all cases when the minion doesn't specify an environment of its own, regardless of whether a corresponding top file exists on the master.

See the following code within __get_tops() in salt/state.py:

        if self.opts['top_file_merging_strategy'] == 'same' and \
        not self.opts['environment']:
            if not self.opts['default_top']:
                raise SaltRenderError('Top file merge strategy set to same, but no default_top '
                          'configuration option was set')
            self.opts['environment'] = self.opts['default_top']

The effect is that under top_file_merging_strategy: same, a minion always has a pinned environment. The effects of specifying an environment are more restrictive than just those documented for top_file_merging_strategy: same. Only the pinned environment's top file will be read, and only the section matching the environment will be seen by the minion.

I'm not sure whether the issue is with the documentation or with the code, but as it stands now, there does not seem to be any difference between top_file_merging_strategy: same and environment: base. The code and documentation for top_file_merging_strategy and default_top could simply be dropped without loss of generality.

As an example, consider the following set of environment trees:

$ tree
.
├── base
│   ├── pillar
│   └── salt
│       ├── basebase.sls
│       ├── devbase.sls
│       ├── prodbase.sls
│       └── top.sls
├── dev
│   ├── pillar
│   └── salt
│       ├── basedev.sls
│       ├── devdev.sls
│       ├── proddev.sls
│       └── top.sls
└── prod
    ├── pillar
    └── salt
        ├── baseprod.sls
        ├── devprod.sls
        ├── prodprod.sls
        └── top.sls

Each state file is named after the referencing environment + the referenced environment. For example, dev/salt/proddev.sls is a state mentioned only in the dev section of prod/salt/top.sls.

base/salt/top.sls contains:

base:
  '*':
    - basebase
development:
  '*':
    - basedev
production:
  '*':
    - baseprod

I used slightly different matching patterns in each top file so they wouldn't collide, but otherwise the general layout is the same for all of them.

With this configuration and the client set to top_file_merging_strategy: same, I would expect the following highstate:

$ sudo salt '*' state.show_top
hoecackle:
    ----------
    base:
        - basebase
    development:
        - devdev
    production:
        - prodprod

The actual highstate is:

$ sudo salt '*' state.show_top
Password:
hoecackle:
    ----------
    base:
        - basebase

$ sudo salt '*' test.versions_report
hoecackle:
Salt Version:
Salt: 2016.3.1-147-gf23e8c5

    Dependency Versions:
               cffi: Not Installed
           cherrypy: Not Installed
           dateutil: 2.5.0
              gitdb: Not Installed
          gitpython: Not Installed
              ioflo: Not Installed
             Jinja2: 2.8
            libgit2: Not Installed
            libnacl: Not Installed
           M2Crypto: Not Installed
               Mako: Not Installed
       msgpack-pure: Not Installed
     msgpack-python: 0.4.7
       mysql-python: Not Installed
          pycparser: Not Installed
           pycrypto: 2.6.1
             pygit2: Not Installed
             Python: 2.7.12 (default, Jul  5 2016, 01:21:33)
       python-gnupg: Not Installed
             PyYAML: 3.11
              PyZMQ: 15.2.0
               RAET: Not Installed
              smmap: Not Installed
            timelib: Not Installed
            Tornado: 4.3
                ZMQ: 4.1.4

    System Versions:
               dist:   
            machine: amd64
            release: 10.2-RELEASE-p18
             system: FreeBSD
            version: Not Installed
@Ch3LL
Copy link
Contributor

Ch3LL commented Aug 1, 2016

@GarthSnyder I am also seeing the same behavior in 2016.3.1. Given my understanding, with teh following test case:

[root@thecakeisalie ~]# cat /srv/salt/top.sls 
base:
  '*':
    - base
[root@thecakeisalie ~]# cat /srv/salt/dev/top.sls 
dev:
  '*':
    - dev
file_roots:
  base:
    - /srv/salt/
  dev:
    - /srv/salt/dev
top_file_merging_strategy: same
[root@thecakeisalie ~]# salt '*' state.show_top
ch3ll-minion:
    ----------
    base:
        - base

I would expect it to look like:

[root@thecakeisalie ~]# salt '*' state.show_top
ch3ll-minion:
    ----------
    base:
        - base
    dev:
        - dev

I have to specify the saltenv in order to call the different environments top files. This might be related to #30434 but without using gitfs

@Ch3LL Ch3LL added Bug broken, incorrect, or confusing behavior P3 Priority 3 labels Aug 1, 2016
@Ch3LL
Copy link
Contributor

Ch3LL commented Aug 1, 2016

ping @terminalmage am I understanding the usage of same merging strategy? From the docs here I would expect it to work differently then it is. Just wanted to double check that I understand it correctly and accurately labeled this as a bug. Thanks

@Ch3LL Ch3LL added severity-medium 3rd level, incorrect or bad functionality, confusing and lacks a work around Core relates to code central or existential to Salt labels Aug 1, 2016
@Ch3LL Ch3LL added this to the Approved milestone Aug 1, 2016
@terminalmage
Copy link
Contributor

@Ch3LL Sorry about the lack of response, I should be able to look into this on Tuesday or Wednesday.

@terminalmage
Copy link
Contributor

Working on a fix this morning.

@meggiebot meggiebot added P1 Priority 1 and removed P3 Priority 3 labels Aug 12, 2016
@meggiebot meggiebot modified the milestones: C 5, Approved Aug 12, 2016
@meggiebot
Copy link

We'll need to make sure we do docs here based on the strategy that tom and erik outlined

@frozenfoxx
Copy link

frozenfoxx commented Aug 23, 2016

+1, encountered this issue myself. When I specify saltenv it seems to just ignore the alternate environment and just runs the base target. My layout is identical to the issue above.

@terminalmage
Copy link
Contributor

terminalmage commented Aug 26, 2016

This is fixed in #35822. @thatch45 and I discussed this and decided that changes in top file merging should not be made in a point release, so this fix is going into the develop branch and will be part of the Carbon release (currently scheduled for late-September).

#35744 was opened yesterday to add corrective information to the docs regarding top file merging behavior, and to let users know that the behavior is slated to be fixed in the Carbon release.

@terminalmage terminalmage added the fixed-pls-verify fix is linked, bug author to confirm fix label Aug 30, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug broken, incorrect, or confusing behavior Core relates to code central or existential to Salt fixed-pls-verify fix is linked, bug author to confirm fix P1 Priority 1 severity-medium 3rd level, incorrect or bad functionality, confusing and lacks a work around State-Compiler ZRELEASED - Carbon
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants