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

Unable to locate package on debian stretch #253

Closed
JoeDupuis opened this issue Mar 4, 2019 · 16 comments
Closed

Unable to locate package on debian stretch #253

JoeDupuis opened this issue Mar 4, 2019 · 16 comments

Comments

@JoeDupuis
Copy link

JoeDupuis commented Mar 4, 2019

On debian stretch 9.6 if I run the postgres state with defaults, it crash on postgresql-server with


      ID: postgresql-server
Function: pkg.installed
  Result: False
 Comment: Problem encountered installing package(s). Additional info follows:

          errors:
              - Running scope as unit: run-r95b568b09a624343a63bc95286832a39.scope
                E: Unable to locate package u'postgresql-10'
 Started: 11:51:14.014073
Duration: 2731.392 ms
 Changes:

It works if i set the package name directly with:

pkg: 'postgresql-10'
pkg_client: 'postgresql-client-10'

@vutny
Copy link
Contributor

vutny commented Mar 4, 2019

This is due to incompatible changes in how Salt renders Jinja in 2019.02 release.
See saltstack/salt#51925 and the release notes for details.

I guess many formulas including this one need to be patched.

@myii
Copy link
Member

myii commented Mar 4, 2019

As discussed in our Slack/IRC/Matrix room, we've got a bit of a problem:

17:51 We've got another problem.
17:51 <docs.saltstack.com/en/latest/topics/jinja/index.html#tojson>
17:51 "New in version 2018.3.3,2019.2.0."
17:52 If we start making wholesale changes across our formulas, we're going to start breaking older minions, which are still officially supported.
17:52 @javierbertoli ^^.
17:52 <noel.mcloughlin> Yeah that is risk.
17:53 <noel.mcloughlin> Regarding my problem, why has salt 2019.2 regressed?
17:53 <docs.saltstack.com/en/develop/topic…-compatible-change-to-yaml-renderer>
17:54 <noel.mcloughlin> Ahh
17:54 <noel.mcloughlin> Aaaahhhh
17:54 We need an alternative plan.
17:54 Either we use the tojson, only if the minion is >;= 2019.2.
17:55 Or we find an alternative, that may or may not be available.
17:55 @aboe76 We need to work out a plan here across all of the formulas that are going to have to be updated.

@myii
Copy link
Member

myii commented Mar 5, 2019

@vutny Tossed around a few more ideas in the discussion beyond the snippet quoted above. Tried quite a few different things. It appears that the only realistic solution, to cover until the time tojson becomes ubiquitous, is to use the | json renderer. That's the only thing that's guaranteed to work, for older minions as well as in the future (no signs of deprecation). Ultimately, once 2019.2 becomes the oldest officially supported version of Salt on Oct 31, 2020, then we can switch to the | tojson Jinja filter.

@vutny
Copy link
Contributor

vutny commented Mar 5, 2019

@myii I don't really understand why SaltStack and the community are marketing tojson().

If we take a look at Jinja upstream docs, it's clearly stated the the filter is intended to use within HTML, not YAML (or SLS) at all.
That works most of the times, but designed for totally different use case. And it would play tricks on you when trying to serialize something more complex than alphanumeric string.

On the other hand, Salt provides their own serialization routines to use specially for SLS rendering:
https://docs.saltstack.com/en/latest/ref/renderers/all/salt.renderers.jinja.html

I found it obvious that the yaml() formatting filter should be applied for SLS files containing YAML, and the json() -- on the code written in JSON.

I think we could push it as a standard for outputting data structures inside Salt states.

@myii
Copy link
Member

myii commented Mar 5, 2019

@vutny Thanks for your reply. Some of what I've got is from looking into things but most of it is from (discussions with) others.

@myii I don't really understand why SaltStack and the community are marketing tojson().

You've already linked to the 2019.2 release notes above. Essentially, in terms of considering Jinja filters alone, then the upstream |tojson filter is superior to the |json_encode_dict and |json_encode_list filters that were previously used. It also passes the implementation to upstream Jinja, instead of having to maintain any unnecessary code duplication.

The example in that section prescribes using |tojson, which is therefore the official upstream guidelines. The community doesn't have to adopt those guidelines but we need to be able to justify why we are differing from it.

If we take a look at Jinja upstream docs, it's clearly stated the the filter is intended to use within HTML, not YAML (or SLS) at all.
That works most of the times, but designed for totally different use case. And it would play tricks on you when trying to serialize something more complex than alphanumeric string.

I think this is an exceptionally important point you are raising. Having concrete examples of any serialisation issues would help form these justifications. However, I would be very surprised to find out that upstream SaltStack didn't run a battery of tests beforehand, to ensure that the |tojson filter wouldn't introduce breakages into existing uses of SaltStack.

On the other hand, Salt provides their own serialization routines to use specially for SLS rendering:
https://docs.saltstack.com/en/latest/ref/renderers/all/salt.renderers.jinja.html

I found it obvious that the yaml() formatting filter should be applied for SLS files containing YAML, and the json() -- on the code written in JSON.

There are a number of points that need to be considered here, which I don't have the answers to:

  1. Why isn't this being pushed as the official guidelines -- is there a negative cost to using these?
    1. For example, does using an additional renderer (e.g. |json) have a performance cost compared to using the |tojson filter?
  2. This specific part of the code was first implemented 6 years ago and hasn't been getting much recent attention -- is this actively maintained as opposed to passively maintained (even just to be kept up-to-date with recent changes being made to JSON and YAML)?
  3. Using |json is preferred to |yaml:
    1. YAML is slow compared to JSON, as mentioned by a number of others.
    2. @aboe76 has started an issue about it as well.
    3. AFAIK, |json will work in all cases that |yaml does.
    4. These formulas also need to be able to support implementations that use JSON instead of YAML.

One additional point about performance issues. We've been having extensive discussions about the problem with these formulas being heavily reliant on pillars -- which becomes the greatest load on Salt masters. So there's a lot of focus on how we can minimise performance costs in all aspects of our formulas.

I think we could push it as a standard for outputting data structures inside Salt states.

I agree with you but not for the same reasons. For me, it's simply because there is no viable alternative right now. The only available solution at the current time is to use the |json renderer. This can be re-evaluated at around Oct 31, 2020, to work out whether it is worth adopting the |tojson filter at that time.


As a side note, your viewpoints are excellent as always @vutny. A lot of decision-making discussions are taking place on our Slack/IRC/Matrix room. Please try to join us whenever you can -- it will help us steer in the right direction.

@myii
Copy link
Member

myii commented Mar 5, 2019

@twistedjoe Sorry for hijacking your issue! You're not being ignored but due to this breaking change in 2019.2, we're trying to work out the best way to deal with this across all of SaltStack Formulas.

In the meantime, do you mind testing with this to see if it works for you?

- pkgs: {{ pkgs }}

Please change this line to:

     - pkgs: {{ pkgs | json }} 

And then try again. Please let us know if that works for you.

@aboe76
Copy link
Member

aboe76 commented Mar 5, 2019

@myii plus the {{ bla | json }} has been used in more then one formula already:

  • docker-formula
  • apache-formula
  • nfs-formula
  • uwsgi-formula
  • systemd-formula
  • postfix-formula
  • php-formula
  • node-formula
  • nginx-formula
  • libvirt-formula
  • hostsfile-formula
  • firewalld-formula

@vutny
Copy link
Contributor

vutny commented Mar 6, 2019

TL;DR

Guys, don't get me wrong, I'm totally happy with json() as it does the job well on simple things like list of packages to install.

My motivation for the discussion

I just got a habit to use yaml() because I stepped on edge cases with json() couple of times...
Think of a long multiline string coming from external data source, which could have any content.

Let it be a shell script for example. Having it encoded in JSON looks ugly when doing state.show_sls or salt-call -l debug. And it is sometimes impossible to determine clearly if that's an issue in the incoming data, or with its representation.

Things you need to aware of are quotes and backslashes, and especially when they're going together.
YAML has lots of reserved characters which JSON simply doesn't care of, like: |, >, &, * , % and others. Plus special combos: ---, ... and !!.
It's very hard to craft an artificial example illustrating this problem, but when you encounter it, that's super non-intuitive. Debugging becomes nightmare, as things look very obscure.

I know it's bad idea to operate raw data in SLS files, sometimes there's no other choice. And since we usually merge Pillars with defaults, it could be anything by mistake or by purpose.

@myii Regarding the code to support Salt's serialization, you could see dumb wrappers around json.dumps() and yaml.dump() exposed via Jinja. So we rely on Python's stdlib and PyYAML, instead of Django-like magic on top developed for HTML pages.

Considering performance, since the default layer of Salt renderers are in use (jinja|yaml), there is no difference if SLS contains JSON or YAML, because everything would be parsed effectively as YAML.
Until you modify a shebang and convert everything to JSON.

@myii
Copy link
Member

myii commented Mar 6, 2019

TL;DR

Guys, don't get me wrong, I'm totally happy with json() as it does the job well on simple things like list of packages to install.

@vutny TL;DR: Didn't get you wrong at all, this feedback was welcome. You've raised important points that need to be considered.

My motivation for the discussion

I just got a habit to use yaml() because I stepped on edge cases with json() couple of times...
Think of a long multiline string coming from external data source, which could have any content.

Let it be a shell script for example. Having it encoded in JSON looks ugly when doing state.show_sls or salt-call -l debug. And it is sometimes impossible to determine clearly if that's an issue in the incoming data, or with its representation.

I think we approach this from a similar viewpoint. I've used |yaml extensively myself for my own projects.

Things you need to aware of are quotes and backslashes, and especially when they're going together.
YAML has lots of reserved characters which JSON simply doesn't care of, like: |, >, &, * , % and others. Plus special combos: ---, ... and !!.
It's very hard to craft an artificial example illustrating this problem, but when you encounter it, that's super non-intuitive. Debugging becomes nightmare, as things look very obscure.

I know it's bad idea to operate raw data in SLS files, sometimes there's no other choice. And since we usually merge Pillars with defaults, it could be anything by mistake or by purpose.

Now this is an action point. I believe we could really benefit from collecting examples that cause breakages, both ways (i.e. |json and |yaml). If we could integrate this with our testing (and docs), it will save us a lot of hassle.

@myii Regarding the code to support Salt's serialization, you could see dumb wrappers around json.dumps() and yaml.dump() exposed via Jinja. So we rely on Python's stdlib and PyYAML, instead of Django-like magic on top developed for HTML pages.

I only glanced over the serialisation code while also casting an eye over the new |tojson filter; thanks for pointing out how the implementation works.

Considering performance, since the default layer of Salt renderers are in use (jinja|yaml), there is no difference if SLS contains JSON or YAML, because everything would be parsed effectively as YAML.
Until you modify a shebang and convert everything to JSON.

This is where things get a little tricky. While that's the default, these formulas could end up being used in environments where that specifically is being avoided. YAML's idiosyncrasies are documented. Using |yaml in those situations could lead to other breakages. This doesn't necessitate that using |json is always the solution, though.


Update: This discussion wouldn't be complete without linking to the available renderers. @aboe76 was mentioning (not seriously) dson in our Slack/IRC/Matrix room -- is it time for the switch now?!

@aboe76
Copy link
Member

aboe76 commented Mar 6, 2019

@myii don't support dson please I can't bear the thought of having to support templates with
such and wow

@myii
Copy link
Member

myii commented Mar 6, 2019

@aboe76 This is it! The conversion to dson begins today! At least when things fail, we'll be entertained by messages like wow and boom! -- not that I've checked this, of course!

@noelmcloughlin
Copy link
Member

Here is link to comprehensive Eriksson-Halberg study of performance & syntax of JSON and YAML:
http://www.csc.kth.se/utbildning/kth/kurser/DD143X/dkand11/Group2Mads/Rapport_Malin_Eriksson_Viktor_Hallberg.pdf

One statement captures the essence of the problem... speed sacrifices robustness and visa versa.

One general explanation behind the YAML implementation’s seemingly bad performance is that it does a more thorough work in the general serialization process. This includes inspecting objects, inserting tags for custom data types and taking advantage of the alternative string block notations in YAML.

See also http://summit.yaml.io/the-yaml-summit-wiki.html
I was also reading somewhere about JSON-LD .. first I heard of that one.

@JoeDupuis
Copy link
Author

@myii Nah it's fine :p
I'll try to test it next time I rebuild the staging server.

@JoeDupuis
Copy link
Author

Switching to pkgs | json does fix the problem. Thank you.

@myii
Copy link
Member

myii commented Mar 8, 2019

@twistedjoe Thanks for testing, I've sent through PR #254 to push through that fix.

@myii
Copy link
Member

myii commented Jul 28, 2019

I just got a habit to use yaml() because I stepped on edge cases with json() couple of times...
Think of a long multiline string coming from external data source, which could have any content.

@vutny Just reporting back that I just got burnt by this as well -- but I did say that I've used | yaml in my own formulas previously! Using either | json or | tojson was adding quotes to numerical keys that I was relying upon in a loop (to still be numbers, not strings).

So, this issue isn't going to go away any time soon. | tojson is going to cause the same problems.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

5 participants