Read Pillar files into OrderedDict to preserve source order

[benosman]

This pull request attempts to solve Enhancement #12161.

I'm very new to Salt, but It's something i'd like for my own states so I thought working on a patch was a good way to get familiar with the internals of SaltStack.

I've tested the functionality in both master-minion and masterless setup and it keeps the pillars in an OrdererdDict.

It does not attempt to change the 'master' values, as I don't see the need and that would mean touching the code in many places.

[ghost]

Test Failed.

If the failures are unrelated to your code, don't stress, a core developer will know these apart.
In the future, if possible, please branch off a passing commit to avoid false positives.
Refer to this link for build results: http://jenkins.saltstack.com/job/salt-pr-build/9882/

[thatch45]

Thanks @benosman ! Welcome to the project!
This looks like it is breaking some of our tests and would also break existing behavior. Some of the test failures look like the pillar data might not be consistent in all situations

[rallytime]

Re-triggering tests to get test data.

[ghost]

Test Failed.

If the failures are unrelated to your code, don't stress, a core developer will know these apart.
In the future, if possible, please branch off a passing commit to avoid false positives.
Refer to this link for build results: http://jenkins.saltstack.com/job/salt-pr-build/10086/

[benosman]

Thanks for looking at this @thatch45 & @rallytime.

I'm sure you're way ahead of me here, but I took a look at the test results myself and can see at least two that are obviously caused by this patch:

integration.client.kwarg.StdTest.test_kwarg_type
integration.shell.cp.CopyTest.test_cp_testfile

You may have worked this out for yourselves already, but I wanted to explain my thinking:

This patch does two things, one is make the Pillar class use OrderedDict which is quite a simple and unobtrusive fix. This works fine when using salt-call in local mode.

The change in payload.py, instead is a bit more invasive and was the harder one for me to locate (as a noob!). Even though the Pillar was an OrderedDict it got broadcasted to the minion as a dict. The patch makes it unpack the msgpack payload into OrdereredDicts. So even if it was a regular dict on the master it now becomes an OrderedDict.

If that is not desirable behaviour, the way around that would be to pack them differently so an OrderedDict on the master is an OrderedDict on the minion, and a dict on the master is a dict on the minion.

[thatch45]

@benosman I am sorry I have not been able to look more closely into this one yet, it has been a very crazy week and I have been sick with the flu.
If you can rebase this against develop head so it will merge cleanly then we can take another look. My main fear is that the behavior can change with older msgpack versions which may result in unexpected results on some system introducing very ghost like bugs

[benosman]

@thatch45 - sorry to hear you have been suffering with the flu - hope you are starting to feel better now!

I've rebased the patch against develop now for you to take a look.

I agree that having different behaviour when msgpack is an older version is not desirable. I need to look into msgpack deeper before I can suggest a workaround for this.

Are there any statistics available anywhere to find out what the installed base of msgpack < 0.2.0 is ?

[ghost]

Test Failed.

If the failures are unrelated to your code, don't stress, a core developer will know these apart.
In the future, if possible, please branch off a passing commit to avoid false positives.
Refer to this link for build results: http://jenkins.saltstack.com/job/salt-pr-build/10256/

[thatch45]

Looks good! Thanks @benosman !
and thanks for being patient :)

[rallytime]

@benosman This PR had to be reverted because it was causing some unusual test failures on our test suite. Since the original test data has disappeared from Jenkins on your pull request, I'll list the failures here, in case you'd like to revisit this in the future:

• integration.shell.cp.CopyTest.test_cp_testfile
• integration.client.kwarg.StdTest.test_kwarg_type
• integration.modules.lxc.LXCModuleTest.test_init
• integration.modules.lxc.LXCModuleTest.test_macvlan

Feel free to let me know if you have questions about running the test suite or anything like that.

[benosman]

@rallytime - ok understood. I was actually a bit surprised that this got merged so soon - as i thought there were still issues to discuss with it, including the tests.

I'll get myself set up with the test suite, but it may take me a couple of days to be able to look into this.

[rallytime]

No problem! I'm happy to answer any questions about the tests. Also, there's some helpful docs about running the test suite here: http://docs.saltstack.com/en/latest/topics/development/tests/

Thanks for contributing!

[aboe76]

@rallytime is this still an ongoing issue, because I have recently came across an issue which is similar.
and would like to see this feature in salt?

[rallytime]

@aboe76 I am not sure what the status of this is actually. The pull request was originally made against develop in November, but then was subsequently reverted a day or two later because of some unusual test failures as noted above. I am not sure if @benosman made another attempt at this or not. What were the issues you were seeing @aboe76 that might be related? (Perhaps it would be better to address those in the issue this pull request was attempting to fix, as those get more visibility that closed PRs usually...depends on what your issue is I suppose.)

[aboe76]

@rallytime thanks will fill the issue with more up to date information. thanks.

[benosman]

Sorry, but i've been unable to find the time to work on this recently, and i've found a workaround for my own requirements for now.

Would be keen to see it added in some form, but i think the msgpack version issue may need some discussion.