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 SLS compilation does not behave the same as Docs describe #12483

Closed
driskell opened this Issue May 2, 2014 · 64 comments

Comments

@driskell
Copy link
Contributor

driskell commented May 2, 2014

Hi

(This is taken from my comments on closed issue #5440 - @basepi recommended a new issue so here it is! Thanks.)

I have 4 environments, "base", "qa", "dev", "master", in that order defined in file_roots. These are branches in a git and I have my own deployment of this to the salt server (I do not use gitfs).

Each has a version of the top.sls file containing all environments (as you'd expect since it same file, just different revisions different branches)

The documentation describes that if an environment has info in top.sls in "base", it will always override all other top.sls. This is not the case - and checking the code there is no distinction between "base" and any other name, and no specific decision on ordering (and it seems alphabetical) - so in my case, even though in the top.sls and file_roots, "qa" is second down, because it is last alphabetically, that overrides all other top.sls files in all other environments.

Based on the docs, it seems to give the idea that "base" is a special name and is always authoritative. It also explicitly says that if an environment is defined in multiple top.sls files - the definition in the top.sls for that environment will be preferred. But the code seems to say otherwise.

The code seems to simply merge and overwrite, in alphabetical order. Which contradicts entirely the documentation.

For reference, from http://docs.saltstack.com/en/latest/ref/states/top.html:

The following is what lead me to think "base" was a special name (it doesn't say the FIRST environment wins, it says the "base" environment wins.)

21.24.15.3. HOW TOP FILES ARE COMPILED
...
1 . The base environment's top file is processed first. Any environment which is defined in the base top.sls as well as another environment's top file, will use the instance of the environment configured in base and ignore all other instances. In other words, the base top file is authoritative when defining environments.

The following is the bit saying a top.sls in an environment is authoritative for that environment if it contains config for it:

3 . For environments other than base, the top file in a given environment will be checked for a section matching the environment's name. If one is found, then it is used. Otherwise, the remaining (non-base) environments will be checked in alphabetical order. In the below example, the qa section in /srv/salt/dev/top.sls will be ignored, but if /srv/salt/qa/top.sls were cleared or removed, then the states configured for the qa environment in /srv/salt/dev/top.sls will be applied.

Although if I'm honest, point 3 does seem to contradict point 2 (below) - I would've expected point 2 to at least make an exception for point 3 - or point 3 come first

2 . If, for some reason, the base environment is not configured in the base environment's top file, then the other environments will be checked in alphabetical order. The first top file found to contain a section for the base environment wins, and the other top files' base sections are ignored. So, provided there is no base section in the base top file, with the below two top files the dev environment would win out, and the common.centos SLS would not be applied to CentOS hosts.

@basepi:

@driskell Could I convince you to open a new issue and insert all of this awesome information and detective work into that new issue? I think what we will want to do is bring the code in line with the documentation (I still haven't tested this myself, so I haven't verified that I see the same thing you do) if possible. I think the documentation's way is a good way, but we definitely need to get them in line with each other one way or another.

I agree the documentation is (mostly) a good way, and the behaviour should match. Assuming I haven't read the doc wrong though, and "base" is meant to be special and authoritative and overriding, then I think that should be configurable so I can call it something else, so maybe a "main_env: base" option with that as default.

Thanks guys!

@basepi basepi added Bug labels May 2, 2014

@basepi basepi added this to the Approved milestone May 2, 2014

@basepi

This comment has been minimized.

Copy link
Collaborator

basepi commented May 2, 2014

Thanks for filing this!

driskell added a commit to driskell/salt that referenced this issue May 5, 2014

Fix top.sls compilation (saltstack#12483) so it matches the behaviour…
… description in documentation section 21.24.15.3 exactly

driskell added a commit to driskell/salt that referenced this issue May 5, 2014

Fix top.sls compilation (saltstack#12483) so it matches the behaviour…
… description in documentation section 21.24.15.3 exactly
@driskell

This comment has been minimized.

Copy link
Contributor

driskell commented May 5, 2014

Proposed a fix if it helps!

@basepi

This comment has been minimized.

Copy link
Collaborator

basepi commented May 5, 2014

Awesome! I'll leave review of that to @thatch45. =)

@cachedout

This comment has been minimized.

Copy link
Collaborator

cachedout commented May 13, 2014

Flagged as 'Fixed Pending Verification' while we wait for that review.

@driskell

This comment has been minimized.

Copy link
Contributor

driskell commented Jun 11, 2014

OK so the fix is not viable as I expected - though I think it was a good POC of what the docs say.

Has there been any discussions on what should happen? Or what recommended approach for me to take?

I think I can rename my environments aDev bQA cProd and things will work better, but that just seems a hack. Or I would need to take my top.sls out of Git and hold them separately, so that each one only contains directives for servers in its environments (thus merging doesn't overwrite anything). But I still worry about someone with access to QA accidentally using a production server name instead of QA and it overriding - so I'd have to use the aDev bQA cProd still.

Maybe I'm approaching this completely wrong though.

@basepi

This comment has been minimized.

Copy link
Collaborator

basepi commented Jun 11, 2014

I think what it comes down to is that we've never really had testing in place for this kind of workflow. In general we just recommend that people don't have conflicts across environments for their top.sls files, because it tends to be implicit, rather than explicit, which in my opinion isn't good (and is harder to maintain).

So we just need to deep dive this and either change the documentation or get the actual compilation process in line with the documentation.

@driskell

This comment has been minimized.

Copy link
Contributor

driskell commented Jun 20, 2014

It seems that #9616 is a variation of this issue?

Please let me know if you start discussing this. I am more than willing to accept something where you have an option to enable the top compilation described in the docs.

I think it is important that not only states can be version controlled, but top.sls also. This workflow I used is exactly how I would proceed when using GitFS backend. But I do require top.sls to be in each branch and version controlled, so changes can be promoted along with state changes. Otherwise I end up merging in state changes from QA to Production but have to prevent top.sls merging too somehow... not even sure how you do this without a dirty hack :-(

Jason

@driskell

This comment has been minimized.

Copy link
Contributor

driskell commented Jun 20, 2014

Also - I'm willing to help implement the proposed solution when one is decided.

@basepi

This comment has been minimized.

Copy link
Collaborator

basepi commented Jun 20, 2014

No worries, we'll keep you posted. Just need to find a good time to have a discussion about this.

@eyj

This comment has been minimized.

Copy link
Contributor

eyj commented Nov 24, 2014

I would strongly suggest that a note is added to the docs right now that they are incorrect. We just set up a new system that strongly relied on the behaviour described in the documentation, and now have to find out that is not implemented that way.

@basepi

This comment has been minimized.

Copy link
Collaborator

basepi commented Nov 24, 2014

You're right, we need to update the docs, especially since it's taken so long for us to have time to investigate this.

@tehmaspc

This comment has been minimized.

Copy link

tehmaspc commented Nov 25, 2014

ditto; been scratching my head as to why my env ordering isn't working.
thanks for updating the docs - but finding that the docs say one thing and saltstack does something else is a sad trend i keep running into :(

cheers

@basepi

This comment has been minimized.

Copy link
Collaborator

basepi commented Nov 25, 2014

@tehmaspc Thanks for the feedback. We've recently hired some services people and should have more core engineering time in the coming months to fix issues like this and get the general bug count under control.

@tehmaspc

This comment has been minimized.

Copy link

tehmaspc commented Nov 26, 2014

@basepi - awesome; for the record Helium fixed up a bunch of things for us - so it's not all bad :)

cheers,

@driskell

This comment has been minimized.

Copy link
Contributor

driskell commented Nov 26, 2014

For anyone arriving here needing a limited alternative, I name the per-env top.sls "_top.sls" in the repository. And I made "top.sls" a symbolic link to "../../base/states/_top.sls" (adjust as necessary)
This makes the top.sls in every env point to the _top.sls in the base environment, always. And you keep the ability to merge top changes between environments. Of course, it means base always wins so not exactly a workaround - just a limited alternative.

It'll be good to get something in place to fix it up completely though! I'm watching in the sidelines if you need any help.

@tehmaspc

This comment has been minimized.

Copy link

tehmaspc commented Nov 26, 2014

Thanks @driskell - I'll give that a shot; I think I'm having the issue as xcorvis on the other issue linked here: #5440 ; but I need to peruse that better. I did notice yesterday after trying to debug on one of my minion that it indeed was pulling env info from an env I didn't expect and everything I'm trying to do formula wise is via the git backend. Hopefully I can find a solution that works because I'm trying to create a base, dev, stg, prd environment setup like yourself and I have been trying to put a separate top.sls file in each of these env dirs.

@driskell

This comment has been minimized.

Copy link
Contributor

driskell commented Nov 26, 2014

@tehmaspc The current functionality just browses the environment top.sls in order, overwriting envs as it goes along. So it will probably pull the top.sls data from "stg" as it's alphabetically last. For me it was pulling qa.

@driskell

This comment has been minimized.

Copy link
Contributor

driskell commented Nov 26, 2014

@tehmaspc I can see that other ticket was 2013 - I think when I last looked at this it used to be random (unordered dictionary) so probably based on some hash of some sort, but at some point the code was changed to be an ordered dictionary. So if you tried to reproduce #5440 now it might be it's reverse alphabetical order.

@jmelfi

This comment has been minimized.

Copy link

jmelfi commented Jun 4, 2015

+1 @BIAndrews @jamiemoore

We use puppet currently to deploy changing values from our code checked out of git branches (single repo). Puppet determines from the node itself what env it should be. We have two repos for puppet and hiera with branches and this works well.

It would be beneficial to be able to have something similar for salt where the salt repo can logically define the envs and pillars can provide the abstracted data. Really hoping for a change and update here since we are beginning our evaluation of continuing with puppet or another solution. This feature problem removes salt for us for now.

@cachedout

This comment has been minimized.

Copy link
Collaborator

cachedout commented Jun 11, 2015

Whew, this issue took a long time to read! ;]

There are a lot of solid suggestions here and good feedback. The use cases surrounding version-controlled SLS files in multiple environments are especially compelling and make me feel like we need to add additional behavior to Salt to facilitate workflows where it makes sense to have per-environment top files.

Here is what I would like to propose:

  1. We adopt a merge strategy configuration option similar to the pillar merge strategy that's currently controlled by pillar_source_merging_strategy. For the interested, this code is fairly straightforward to follow. The default merging strategy would not change. Somebody would have to really come up with a compelling reason to do so, since it would break many existing installations, in my opinion.

  2. Like @basepi, I like this suggestion from @clinta of being able to allow each environment to only use high data for its own environment defined in the top file present in that environment, disregarding all others. I propose making this an additional merge strategy in the configuration option described above.

  3. Finally, I propose an additional merge strategy option that would allow an environment to be selected from which all high data would be taken from. This could even default to base if we like. This would only use the selected environment as the authoritative source for high data, disregarding all others.

Does this proposal adequately cover the use cases outlined? All suggestions or feedback are very welcome. :]

@garo

This comment has been minimized.

Copy link

garo commented Jun 12, 2015

The features @cachedout suggested sounds good.

Now I know this feature request isn't directly related to this issue but please also consider if #24622 could be implemented at the same time for pillars.

@cachedout

This comment has been minimized.

Copy link
Collaborator

cachedout commented Jun 12, 2015

Hi @garo. I'll take a look at it and see. Thanks. We'll track the progress of that as a separate feature request, though. Follow #24622 for progress.

Anybody else have feedback before we get to writing this?

@cachedout

This comment has been minimized.

Copy link
Collaborator

cachedout commented Jun 16, 2015

OK, I just about have a PR ready to go. Here are the changes I propose making:

After reviewing this in more detail, I think @driskell had the right idea in his earlier proposal. Therefore, I've done a few things:

  1. Configuration files are now rendered as ordered dictionaries. I have added a new configuration file option called ordered_top_files as suggested by @driskell . It defaults to False, but when it is turned on, it will process environments in the order they are written in the configuration file.

  2. I have added a configuration option called top_file_merging_strategy. This can be set to either merge or same (currently). If set to merge, the current behaviour is preserved. However, this can be influenced by the ordered_top_files setting above, in what should hopefully be an obvious way. If it's not, please let me know. :]

I did not implement fallback options because I feel like environments are already very difficult to reason about and I feel like adding fallback behaviour will only increase that complexity. Differing opinions are welcome of course. :]

  1. I created a default_top option. If this is set, and no environment is speficially requested (via configuration or via a saltenv argument) and the same value is applied to the top_file_merging_strategy option, this is the environment that will be used.

Thoughts? I'll work on cleaning this up and submitting a PR, probably tomorrow.

@mathom

This comment has been minimized.

Copy link

mathom commented Jun 18, 2015

@cachedout I'd be very interested in seeing your PR. Being able to disable the topfile merging would greatly simplify our environment setup and test process.

@cachedout

This comment has been minimized.

Copy link
Collaborator

cachedout commented Jun 19, 2015

@mathom I was just waiting for any feedback. :] I am planning on getting this cleaned up and submitted today. Thanks for being willing to review it. Much appreciated.

@jmelfi

This comment has been minimized.

Copy link

jmelfi commented Jun 19, 2015

@cachedout I will love to test this.

I assume that top_file_merging_strategy being set to merge by default and ordered_top_files being set to True would pull them from the top down.

Using this method, we would need to define the most generic first and then the most custom at the bottom to get a proper merge overwrite or is this resolved otherwise with the change to using ordered dictionaries? I'm interested in seeing how much would need to be altered to use different branches in our git repositories to deploy our code base from a single top file for definition.

Thanks for all the work put in as this is a large problem that is quite complex from what I can understand.

@cachedout

This comment has been minimized.

Copy link
Collaborator

cachedout commented Jun 22, 2015

@jmelfi I ended up running into a problem with ordered_top_files and so I'm changing it to support an order: flag for each env in file_roots. In the end, I think this is better since depending on ordering in a YAML dictionary is just asking for trouble. To answer your question though, yes, with the order flag set in your envs, the behaviour you describe is what should happen.

@cachedout

This comment has been minimized.

Copy link
Collaborator

cachedout commented Jun 23, 2015

OK @jmelfi and @mathom. Have a look at #24878 and let me know what you think.

@jmelfi

This comment has been minimized.

Copy link

jmelfi commented Jun 23, 2015

@cachedout I'll give it a review over the next hour or so. Thanks for the update!

@cachedout

This comment has been minimized.

Copy link
Collaborator

cachedout commented Jun 23, 2015

@jmelfi Super. Be sure to configure things as described in the PR. It won't (or shouldn't) change behaviour out of the box. I'll be here if you have questions. Thanks for the testing help!

@DanyC97

This comment has been minimized.

Copy link

DanyC97 commented Jul 24, 2015

wow, this was a long issue to read :)) Would love to see this merged soon, thx for the effort!

@cachedout

This comment has been minimized.

Copy link
Collaborator

cachedout commented Aug 11, 2015

I have merged this fix into develop and it should be backported into the 2015.8 branch in the next day or two.

@basepi

This comment has been minimized.

Copy link
Collaborator

basepi commented Aug 11, 2015

👍

@rallytime

This comment has been minimized.

Copy link
Contributor

rallytime commented Aug 11, 2015

Back-port is in #26214. :)

@jmelfi

This comment has been minimized.

Copy link

jmelfi commented Aug 11, 2015

@cachedout sorry for a late reply here. I was able to get the sorting to work properly after using the PR work. Thanks you guys for this hard work. I've been working the last week on moving from our puppet code completely into saltstack.

You guys are awesome!

@cachedout

This comment has been minimized.

Copy link
Collaborator

cachedout commented Aug 11, 2015

@jmelfi I'm so glad! Thanks for testing this. For the time being, I am going to close this issue since we're using it to track progress toward our release. That doesn't mean, however, that people can't continue to post feedback here. We'll re-open it if we need to. Thanks again!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment