Skip to content
This repository has been archived by the owner on Jan 24, 2024. It is now read-only.

SEP 7 - Change requisites/excludes #10

Closed
wants to merge 3 commits into from

Conversation

waynew
Copy link
Contributor

@waynew waynew commented Apr 9, 2019

What is says on the tin - and as always, see the full text here

@waynew waynew force-pushed the changing-requisites-excludes branch from 6131f91 to 9e3e7b6 Compare April 9, 2019 22:18
@waynew
Copy link
Contributor Author

waynew commented Apr 9, 2019

This will be SEP 7

@waynew waynew changed the title Create changing requisites/excludes SEP SEP 7 - Create changing requisites/excludes SEP Apr 9, 2019
@waynew waynew changed the title SEP 7 - Create changing requisites/excludes SEP SEP 7 - Change requisites/excludes SEP Apr 9, 2019
@waynew waynew changed the title SEP 7 - Change requisites/excludes SEP SEP 7 - Change requisites/excludes Apr 9, 2019
@max-arnold
Copy link

max-arnold commented Apr 10, 2019

Here are my thougts, from more to less specific:

  1. Have you considered using the existing config namespace (use_superseded)? The proliferation of inconsistently named one-off feature toggles is far from optimal.

  2. On a more abstract level, global feature flags like this are hard to use, because they can't be turned on a per-state basis (think of formula authors and folks who run multiple salt versions) and also because they multiply the number of cases to test. Maybe a separate requisite (like the onfail_all that was introduced by @mattp- in new requisite form: onfail_all salt#50264 and Difference between the onfail and onchanges requisites vs their new _any variants unclear salt#47015) would be better?

  3. In general, I'm not sure if this is the most pressing problem with requisites (and that it is worth to make the SEP). There are many other open issues, and also many closed as stale. I think it would be good if SaltStack allocated some systematization/refactoring/documentation/product_management/architecture_design efforts there. Some examples:

I'm not saying that these cases are more important. My point is that the issue tracker is full of useful data (often closed by a mindless stale-bot!), and with systemic approach/analysis it is possible to see multiple root causes and areas for improvements that otherwise are not obvious.

UPD: I wonder if tools like TLA+ could be used to model and exhaustively verify the state ordering algorithm.

@jfindlay
Copy link

jfindlay commented Apr 10, 2019

I like the idea of consolidating small, not quite orthogonal feature domains into a more globally ordered feature comprehension, like lining up magnetic domains in a ferromagnetic material, thus making salt more focused, comprehensible, and compelling.

I really like @max-arnold's argument about digging deeper into the philosophical mines of salt principles and inscribing a larger diameter around common patterns of user irritation and institutional quirks. I have long thought about how to best effect a positive revision in this regard. While incremental improvements are great to see and utilize (like @isbm's module.run update, for example) I suggest making a long, careful study of

  • salt's current state of affairs and its huge library of
    • github bugs/complaints
    • formulas
    • stack overflow answers
    • blog howto's
    • books
    • case studies
  • how ansible, puppet, chef, and others approach config man/remote ex
  • generally how people want/need management automation based on how the field trends, taking into account things like
    • disposable infrastructure methodologies (containering et al)
    • infrastructure management as a service
    • what engineers expect their infrastructure lifecycles to look like

and with this information design a 2.0 state grammar and DSL. I think there is enough to gain by spending the time to put in all the best features at once rather than rolling them out slowly, painfully over many years. This will also simplify how salt users and developers can expect salt to function. "Am I looking at sls 1.0 or 2.0? Actually, I can tell because the yaml is very different." Nobody has to wonder about which micro release version of salt their sls is compliant with and nobody has to worry about backwards compatibility. Formulas could be drastically simplified as a first class concept. The need for a jinja-like preprocessor could be (mostly?) eliminated. You could even write a canonical translation utility to cover the most common cases. Perhaps the state compiler itself could provide this service.

Whereas salt's foundational concepts are certainly informed by a practical and clear intuition, why not steal the best ideas from wherever you can find them a decade or more after we've learned how to devops and design a new, clean system from this knowledge? Incremental changes to the status quo will not be able to get us there.

@waynew
Copy link
Contributor Author

waynew commented Apr 10, 2019

My point is that the issue tracker is full of useful data (often closed by a mindless stale-bot!), and with systemic approach/analysis it is possible to see multiple root causes and areas for improvements that otherwise are not obvious.

@max-arnold I heartily agree!

Actually that's how this came to be - I was working my way through old issues on the issue tracker and came across this one and created the original RFC. (I was cleaning up my old PRs yesterday and that was still kicking around.)

Whereas salt's foundational concepts are certainly informed by a practical and clear intuition, why not steal the best ideas from wherever you can find them a decade or more after we've learned how to devops and design a new, clean system from this knowledge?

Definitely something we've considered. And something that is probably very worthwhile. Of course it's also not going to happen overnight, as much as we'd like to wave our hands and make it so 😛

Meanwhile, if someone is interested and has the time to make some incremental improvement 🤷‍♂️

Of course, all of that is orthogonal to this proposal 👀 I'm not sure what forum would make for a great discussion around the idea (perhaps salt-users? Maybe a Google doc? Probably not the community Slack, as the history there just kind of fades away), or if this PR was just a ripe opportunity to air some minor/general feelings, and nobody is really ready for the larger discussion. Do you have any suggestions?

@jfindlay
Copy link

@waynew, well, I'm just stuffing the suggestion box with ideas, so don't let my apparent (not actual) criticism thwart any good work.

My main argument is that while the fastidious ethos of backwards compatibility in the salt project is a great asset and virtue, I wonder if sometimes it distracts us from making more significant changes when that would be better. The latter case does not have to be undisciplined, but can rather be similar to py2->py3, where the differences are well known and well defined and the old system is kept in place for a long time.

I wonder if many of the frustrating quirks with sls can be avoided by using or writing an alternative renderer.

@Akm0d Akm0d requested review from Akm0d and removed request for Akm0d May 3, 2019 17:58
@eliasp
Copy link

eliasp commented May 20, 2019

and with this information design a 2.0 state grammar and DSL. I think there is enough to gain by spending the time to put in all the best features at once rather than rolling them out slowly, painfully over many years. This will also simplify how salt users and developers can expect salt to function. "Am I looking at sls 1.0 or 2.0? Actually, I can tell because the yaml is very different."

This is something that always bugged me a lot about SaltStack's SLS compared to e.g. Kubernetes' Objects, which always have a mandatory apiVersion or Gentoo Portage's PMS which make it far easier to handle transitions of changes of either the implementation or resource definitions.

@mattp-
Copy link

mattp- commented May 20, 2019

@eliasp i agree. another good example of hard versions controlling schema is dockerfiles and docker-compose files. Something similar could work for salt

@waynew
Copy link
Contributor Author

waynew commented May 21, 2019

Those are some interesting ideas! I think I would love to see a SEP proposing a SLS version syntax. I think there might be some good discussion around that topic.

this is considered an anti-pattern, and provide some patterns to follow for
people who think they want to take this approach.
3. Codify the existing behavior by adding a proper error/exception when exclude
excludes something that was required (should still require documentation
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like the PR, but if we go down this path instead, I'd advocate a warning instead of an error. An error would be a breaking change if someone was relying on current behavior. If an error is really desired, then have one major release use a warning with a deprecation notice first, then make it an error.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's a great point all around. Definitely a warning+deprecation notice, e.g. "This will become an error and in release ". That would handle the current case, where it's implicit behavior, as well as provide a good path going forward.

@waynew waynew added the Final Comment Period Speak now or forever hold your peace. label Sep 9, 2019
@thatch45
Copy link
Contributor

thatch45 commented Sep 9, 2019

I think that this conversation is going in multiple directions.

  1. The first and initial direction is to simply make it so that excludes remove requisites that have been excluded. This would retain backward compatibility and create more clarity when excludes are used..
    This will require some changes about where and how excludes are removed in the state system, but it is very doable.

  2. Change the state system to no longer be backward compatible to chase every issue possible. This statement might be a little heavy-handed, but nothing seems to tick users off more than breaking their states. This is a situation FAR worse than unexpected behavior.

I should also state that trying to chase expected behavior is almost always a real mess once it gets to a certain level of granularity.

In a nutshell, Salt has been made to allow for multiple state systems to exist side by side. It would be reasonable to make another state system if it were so desired if we wanted to not break states but explore another model for states. This type of detachment of code is one of the goals of Salt's loader system.

@waynew
Copy link
Contributor Author

waynew commented Sep 9, 2019

I think that this conversation is going in multiple directions.

Agreed.

My thoughts are that for now this SEP should (continue to) focus only on 1, with the idea that 2 is still a worthwhile idea, but out of scope. I'd be happy to update the SEP with a statement to the effect of:

This SEP is intentionally limited in scope. In the long term it would be great if we could design the requisite/include/exclude system based on lessons learned, but that will require an overhaul of the system and will likely not be backwards-compatible. Another SEP could be proposed for a more comprehensive solution.

if we think being that explicit is necessary.

@arizvisa
Copy link

arizvisa commented Nov 5, 2019

Sorry to jump on this proposal long after it's been discussed... but I think instead of changing the way that requisites and excludes actually work (which can likely break things or be difficult to introduce new semantics without breaking things) can be done without overhauling the entire system.

Some of these issues wrt the flexiblity of requisites can likely be solved with some kind of multiplexing-module in the salt.states namespace. It appears that the majority of the state application logic is defined as chunks grouped into lowstates, and then handled by the salt.state.State class.

This State class, though, can likely be re-instantiated with a subset of already compiled chunks or running states, and then said "multiplex" module can apply the specified states and check their results to determine how to proceed. Those running states and their lowstates also appear to be injected into the globals of the module by the call method of this class, so it might not require too much research or hackery rather than overhauling the whole thing. (There's some special cases in that class too like for the cmd lowstate)

The one thing that could be really screwy, though, is how to capture the results from the "multiplex" module, since it'd essentially unflatten the schema for the entire thing, and could get hairy wrt remaining idempotent if the user tries to reference a state that has been multiplexed.

Overhauling this system just makes me nervous, and it appears you guys have a pretty full plate already.

@waynew
Copy link
Contributor Author

waynew commented Dec 6, 2019

After some further discussion with the core team it looks like there is an actual bug, as well as some non-buggy, but could-be-improved behavior. So I will update the original things, and withdraw this as it's not actually needed.

@waynew waynew closed this Dec 6, 2019
@waynew waynew added Withdrawn Author has decided to no longer pursue the SEP in its current state. and removed Final Comment Period Speak now or forever hold your peace. labels Jan 24, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Withdrawn Author has decided to no longer pursue the SEP in its current state.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants