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

Nondeterminism in Pillar #19332

Closed
QuinnyPig opened this Issue Jan 4, 2015 · 15 comments

Comments

@QuinnyPig
Contributor

QuinnyPig commented Jan 4, 2015

Remember: conflicting keys will be overwritten in a non-deterministic manner!

Is there an advantage to this that I'm not seeing? Being able to strategically override environment-wide settings with node-specific settings seems to be rather valuable; as long as the value in which keys are added to the current run is lexicographically nondeterministic, that's going to pose significant challenges.

Am I missing something obvious? If so feel free to close/WONTFIX/stop babbling.

@basepi

This comment has been minimized.

Show comment
Hide comment
@basepi

basepi Jan 5, 2015

Collaborator

I think it technically is deterministic -- it's going to merge the same way each time with the same pillar data. It's likely an issue of not using ordered dictionaries for that data yet, and so the way things are overwritten will be dependent on Python's ordering and dictionary handling. (With some YAML unexpected behavior thrown in)

We just need to nail down this behavior and document it.

Collaborator

basepi commented Jan 5, 2015

I think it technically is deterministic -- it's going to merge the same way each time with the same pillar data. It's likely an issue of not using ordered dictionaries for that data yet, and so the way things are overwritten will be dependent on Python's ordering and dictionary handling. (With some YAML unexpected behavior thrown in)

We just need to nail down this behavior and document it.

@rallytime

This comment has been minimized.

Show comment
Hide comment
@rallytime

rallytime Mar 20, 2015

Contributor

I think this issue was referenced inadvertently.

Contributor

rallytime commented Mar 20, 2015

I think this issue was referenced inadvertently.

@rallytime rallytime reopened this Mar 20, 2015

@cachedout cachedout closed this in 56142ad Mar 21, 2015

@QuinnyPig

This comment has been minimized.

Show comment
Hide comment
@QuinnyPig

QuinnyPig Mar 23, 2015

Contributor

I'm still unconvinced this solves the root issue. Am I missing something obvious?

Contributor

QuinnyPig commented Mar 23, 2015

I'm still unconvinced this solves the root issue. Am I missing something obvious?

@rallytime

This comment has been minimized.

Show comment
Hide comment
@rallytime

rallytime Mar 23, 2015

Contributor

Apologies here @QuinnyPig! This issue was mistakenly referenced in a commit, and as the commits gets merged, backported, merged-forward, etc, it keeps closing. Github is trying to be too helpful. :)

Contributor

rallytime commented Mar 23, 2015

Apologies here @QuinnyPig! This issue was mistakenly referenced in a commit, and as the commits gets merged, backported, merged-forward, etc, it keeps closing. Github is trying to be too helpful. :)

@rallytime rallytime reopened this Mar 23, 2015

@QuinnyPig

This comment has been minimized.

Show comment
Hide comment
@QuinnyPig

QuinnyPig Mar 23, 2015

Contributor

No worries. I vote that @UtahDave just fixes it already. :-)

Contributor

QuinnyPig commented Mar 23, 2015

No worries. I vote that @UtahDave just fixes it already. :-)

@QuinnyPig

This comment has been minimized.

Show comment
Hide comment
@QuinnyPig

QuinnyPig Jun 24, 2015

Contributor

Yay, movement!

Contributor

QuinnyPig commented Jun 24, 2015

Yay, movement!

@fernandoacorreia

This comment has been minimized.

Show comment
Hide comment
@fernandoacorreia

fernandoacorreia Nov 17, 2015

I ran into this warning on the documentation when researching how to implement configuration overrides.

The implementation seems to be deterministic, at least for certain cases, and I would like to request that the documentation should be updated to state the cases for which deterministic behavior can be expected (so that the existing, deterministic behavior does not suddenly go away in a new version).

The use case that I'm working on is to provide "base" pillar files and formulas that can be copied to a target host (e.g. via git clone or unzipping), and that some of the "default" pillar settings can be overridden for that particular deployment in a separate file. This way, a set of generic scripts can be used across several deployments, which have some custom settings, and when the generic scripts are changed, they can be updated across those deployments, without losing the customized settings.

An example of the current (deterministic) behavior, with a default Salt installation (using the "smart" merging strategy):

root@ip-172-31-46-214:/srv/pillar# ls
defaults.sls  overrides.sls  top.sls

root@ip-172-31-46-214:/srv/pillar# cat top.sls 
base:
  '*':
    - defaults
    - overrides

root@ip-172-31-46-214:/srv/pillar# cat defaults.sls 
service:
  package-name: bind9
  version: 9
  password: DEFAULT

root@ip-172-31-46-214:/srv/pillar# cat overrides.sls 
service:
  version: 8
  password: SECRET

root@ip-172-31-46-214:/srv/pillar# salt-call pillar.get service
local:
    ----------
    package-name:
        bind9
    password:
        SECRET
    version:
        8

In short, the request is to document that behavior, where the value of a key defined in an included file will override the previous value of the same key.

Note: There's a related issue (#24501) which seems to be about a more complex case, where the ordering of different environments in a top.sls file is non-deterministic. There's also an older issue (#1432) which seems to be about the mappings in a top.sls file not being read in order (this one may have been fixed).

fernandoacorreia commented Nov 17, 2015

I ran into this warning on the documentation when researching how to implement configuration overrides.

The implementation seems to be deterministic, at least for certain cases, and I would like to request that the documentation should be updated to state the cases for which deterministic behavior can be expected (so that the existing, deterministic behavior does not suddenly go away in a new version).

The use case that I'm working on is to provide "base" pillar files and formulas that can be copied to a target host (e.g. via git clone or unzipping), and that some of the "default" pillar settings can be overridden for that particular deployment in a separate file. This way, a set of generic scripts can be used across several deployments, which have some custom settings, and when the generic scripts are changed, they can be updated across those deployments, without losing the customized settings.

An example of the current (deterministic) behavior, with a default Salt installation (using the "smart" merging strategy):

root@ip-172-31-46-214:/srv/pillar# ls
defaults.sls  overrides.sls  top.sls

root@ip-172-31-46-214:/srv/pillar# cat top.sls 
base:
  '*':
    - defaults
    - overrides

root@ip-172-31-46-214:/srv/pillar# cat defaults.sls 
service:
  package-name: bind9
  version: 9
  password: DEFAULT

root@ip-172-31-46-214:/srv/pillar# cat overrides.sls 
service:
  version: 8
  password: SECRET

root@ip-172-31-46-214:/srv/pillar# salt-call pillar.get service
local:
    ----------
    package-name:
        bind9
    password:
        SECRET
    version:
        8

In short, the request is to document that behavior, where the value of a key defined in an included file will override the previous value of the same key.

Note: There's a related issue (#24501) which seems to be about a more complex case, where the ordering of different environments in a top.sls file is non-deterministic. There's also an older issue (#1432) which seems to be about the mappings in a top.sls file not being read in order (this one may have been fixed).

@cachedout

This comment has been minimized.

Show comment
Hide comment
@cachedout

cachedout Nov 17, 2015

Contributor

@twangboy or @dmurphy18 Are one of you able to look at this, per the above request of @thatch45 ?

Contributor

cachedout commented Nov 17, 2015

@twangboy or @dmurphy18 Are one of you able to look at this, per the above request of @thatch45 ?

@twangboy twangboy modified the milestones: B 8, Approved Nov 17, 2015

@dmurphy18

This comment has been minimized.

Show comment
Hide comment
@dmurphy18

dmurphy18 Nov 17, 2015

Contributor

@twangboy You'll have to look, I am neck deep packaging.

Contributor

dmurphy18 commented Nov 17, 2015

@twangboy You'll have to look, I am neck deep packaging.

@QuinnyPig

This comment has been minimized.

Show comment
Hide comment
@QuinnyPig

QuinnyPig Nov 17, 2015

Contributor

This matches what I'm seeing / using in production here. Sorry I haven't followed up; Pillar top files work on a last-match basis. If you can confirm this is always the case / there's no way to uncover overridden values, then this becomes a purely documentation issue.

Contributor

QuinnyPig commented Nov 17, 2015

This matches what I'm seeing / using in production here. Sorry I haven't followed up; Pillar top files work on a last-match basis. If you can confirm this is always the case / there's no way to uncover overridden values, then this becomes a purely documentation issue.

@justinta justinta added this to the Approved milestone Nov 17, 2015

@twangboy twangboy assigned twangboy and unassigned UtahDave Nov 18, 2015

@twangboy twangboy modified the milestones: B 8, Approved Nov 18, 2015

@twangboy twangboy added the Stretch label Nov 18, 2015

@twangboy twangboy modified the milestones: B 7, B 8 Nov 23, 2015

@twangboy twangboy removed the Stretch label Nov 30, 2015

@twangboy

This comment has been minimized.

Show comment
Hide comment
@twangboy

twangboy Dec 3, 2015

Contributor

@cachedout In this specific configuration (#19332 (comment)) it appears the last one wins if there is a conflict.

So with the following top.sls overrides wins:

base:
  '*':
    - defaults
    - overrides

If you switch the order like so then defaults wins:

base:
  '*':
    - overrides
    - defaults

In the same comment he references #24501 which is set up a little differently. It looks like it should be using saltenv but it doesn't. I need somebody (@UtahDave) to explain that one to me.

Contributor

twangboy commented Dec 3, 2015

@cachedout In this specific configuration (#19332 (comment)) it appears the last one wins if there is a conflict.

So with the following top.sls overrides wins:

base:
  '*':
    - defaults
    - overrides

If you switch the order like so then defaults wins:

base:
  '*':
    - overrides
    - defaults

In the same comment he references #24501 which is set up a little differently. It looks like it should be using saltenv but it doesn't. I need somebody (@UtahDave) to explain that one to me.

@twangboy twangboy referenced this issue Dec 3, 2015

Merged

Fix #19332 #29400

@twangboy twangboy closed this in d965d00 Dec 9, 2015

cro added a commit to cro/salt that referenced this issue Jan 12, 2016

Fix saltstack#19332
Clarified precedence of pillar values
@rsuarezsoto

This comment has been minimized.

Show comment
Hide comment
@rsuarezsoto

rsuarezsoto Apr 6, 2016

Sorry to comment on a closed issue, but I've been bitten by something that I think is related to this. Should I open a new issue instead of commenting here, just say so, please.

Currently it seems that ext_pillars (in particular, gitfs pillars) are not parsed in the order by which they're listed in the configuration, but rather by the order of their hashed directories in the saltmaster's cache. I.e., let's say you have this configuration:

ext_pillar:
  - git:
    - master git@gitserver.com:pillars1.git
    - master git@gitserver.com:pillars2.git

The saltmaster will fetch these repositories and cache them in two directories, whose names will be hashes (e.g, "6141fcbdf8d4a54de0d3ba39fe66f405"). When processing them, it will use the order given by these hashes, even if it's not the same that the one in the configuration file.

Ideally, the repositories would be processed in the order that I listed them so I could know which were going to have preference (for pillar overriding). As it is now, if there are conflicting pillars, I don't know if the ones "winning" will be those in the first repository or those in the second.

I was trying to do something alike to what @fernandoacorreia was doing, using gitfs pillars instead of local ones, and this makes it impossible. If I can't fix an order to process gitfs pillars, my only hope of them working like I want them is pure luck. And I've been running out of luck lately ;-)

Thanks in advance.

rsuarezsoto commented Apr 6, 2016

Sorry to comment on a closed issue, but I've been bitten by something that I think is related to this. Should I open a new issue instead of commenting here, just say so, please.

Currently it seems that ext_pillars (in particular, gitfs pillars) are not parsed in the order by which they're listed in the configuration, but rather by the order of their hashed directories in the saltmaster's cache. I.e., let's say you have this configuration:

ext_pillar:
  - git:
    - master git@gitserver.com:pillars1.git
    - master git@gitserver.com:pillars2.git

The saltmaster will fetch these repositories and cache them in two directories, whose names will be hashes (e.g, "6141fcbdf8d4a54de0d3ba39fe66f405"). When processing them, it will use the order given by these hashes, even if it's not the same that the one in the configuration file.

Ideally, the repositories would be processed in the order that I listed them so I could know which were going to have preference (for pillar overriding). As it is now, if there are conflicting pillars, I don't know if the ones "winning" will be those in the first repository or those in the second.

I was trying to do something alike to what @fernandoacorreia was doing, using gitfs pillars instead of local ones, and this makes it impossible. If I can't fix an order to process gitfs pillars, my only hope of them working like I want them is pure luck. And I've been running out of luck lately ;-)

Thanks in advance.

@basepi

This comment has been minimized.

Show comment
Hide comment
@basepi

basepi Apr 8, 2016

Collaborator

@rsuarezsoto I recommend creating a new issue and linking back to this one.

Collaborator

basepi commented Apr 8, 2016

@rsuarezsoto I recommend creating a new issue and linking back to this one.

@rsuarezsoto

This comment has been minimized.

Show comment
Hide comment
@rsuarezsoto

rsuarezsoto Apr 11, 2016

Done, issue #32471 . Thanks!

rsuarezsoto commented Apr 11, 2016

Done, issue #32471 . Thanks!

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