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

Initial PR for extension module migration #67

Closed
wants to merge 0 commits into from

Conversation

thatch45
Copy link
Contributor

A request to migrate the modules inside the Salt repo into external repos for better maintenance

@thatch45 thatch45 requested a review from a team as a code owner July 26, 2022 17:17
@thatch45 thatch45 requested review from waynew and removed request for a team July 26, 2022 17:17
@OrangeDog
Copy link

Speed up PR merges and reviews through faster tests and a smaller core code base.

However, the tests will no longer catch when a core change breaks something in a module. Trading test time for more bugs and time spent investigating and fixing them.

@thatch45
Copy link
Contributor Author

Yes, this is a trade-off that I feel is worth considering. Given the time spend on pushing PRs through, I feel that the preservation of more time and the distribution of testing should free up more time to address these bugs and issues. But it is a resource shuffle we are cognizant of.

@damon-atkins
Copy link

damon-atkins commented Jul 28, 2022

Moving away from "batteries included" increase the complexity of installing the minion. Unless also extensions can be installed on the Master, and the minions fetches any extensions on start-up (possible pip dependencies as well).

Currently including batteries solves:

  • No Internet(proxy) access required on minion
  • Single Package Install (almost).
  • Any SLS (modules) will work which is not dependant on pip install of a dependency first.
  • SLS can be applied immediately the minion is started for the first time in general.

Possible solution for when "batteries" are removed is documentation of the best approach to cover the above operational issues (and how to set it up).

On side note, thought we might of seen __idem__['test.echo']('foo') same as __salt__['test.echo']('foo'). Or some other way to expand idem/pop, and include it into salt more seamlessly, so salt is remote caller for idem (idem use this way would still have the above issues). A bit like salt master can call ansible on a minion but more integrated.

@kfdm
Copy link

kfdm commented Jul 28, 2022

I tend to list multiple repos under file_roots and deploy via git checkouts, but I suppose it could also be built off of spm

@waynew
Copy link
Contributor

waynew commented Jul 28, 2022

Speed up PR merges and reviews through faster tests and a smaller core code base.

However, the tests will no longer catch when a core change breaks something in a module. Trading test time for more bugs and time spent investigating and fixing them.

That's definitely a valid concern. Honestly I'm more concerned about extension modules stomping on each other, though.

But those are still problems we (can) have today. I've seen a number of bugs that were tracked down to a custom module that conflicted with a pre-existing Salt module. And some of our modules don't have a whole lot of tests today.

The way that I envision this working is that extension module maintainers would test their module against the current LTS (if that happens), main release, and development branch of Salt. Similar to how Python modules can use nox or tox to test against multiple versions of Python. So one might have the following nox test setups:

python -m nox -e test-salt-lts
python -m nox -e test-salt-latest
python -m nox -e test-salt-unstable

Something to that effect. Then if bugs are detected that should not be then the module maintainers can file an upstream bug with Salt. Of course if it's something that's properly deprecated and then removed from the main Salt codebase then obviously the module should correctly handle that, but hopefully that should be few and far between at this point (most of the removals and deprecations I'm aware of have been driven by underlying modules changing or other changes in the underlying systems, not just Salt changing)

I would like to see some much clearer guides/documentation for how to extract a module from Salt and turn it into an extension, though, as a requisite for this effort. I've done some with the https://github.com/saltstack/salt-extension project, but it's probably closer to 80% there than 100%, and there isn't a super great tutorial/walkthrough that we can point folks to, either.

@max-arnold
Copy link

max-arnold commented Jul 31, 2022

I'm cautiously positive on this, but it all depends on how the transition is going to be executed.

Plus, there are a couple of aspects that will be hard to address:

  1. Reduced code quality (or more variation in quality) for external modules. This could happen due to lack of time/energy to write unit tests (as it happens now for some PRs), less stringent code reviews, etc
  2. Less integration testing. External module maintainers might not be able to track the core updates very closely. Core could break some 3rd party modules (the feedback loop for such regressions will be longer)
  3. Decreased usability for end users, who will have to assemble Salt Minion out of many independently released and versioned components, that might also have dependency conflicts. Upgrades will be even more painful. More people will have to invest time and resources into the toil of building their own Salt packages
  4. For an end user it will be harder to identify/file bugs and find documentation, because Salt codebase will be fragmented across many repos. The visibility and discoverability of these components is crucial

@thatch45
Copy link
Contributor Author

thatch45 commented Aug 1, 2022

Thanks Max, let me try to discuss a few of these

  1. I think that we will get better overall code quality - because we have a lot of modules that are not actively maintained that we can get out of the way. We can focus on the right modules. I also think that allowing modules to be fixed and cleaned up in isolation will allow more rapid development and fixes. This is till a concern - as with everything in here it is a balancing act and I feel strongly that overall we will get better code quality in the right places
  2. We should consider a validation process allowing external modules to continually validate against HEAD as as well as development branches.
  3. We want the core library that is sent out to still cover everything that a core user would use, the core lib will still be very large with about one third of existing modules staying in there
  4. This is a problem we are working on for Idem as well, since it is far more fragmented than Salt will be after we push this SEP. But it is a complicated issue!

@waynew
Copy link
Contributor

waynew commented Aug 1, 2022

@max-arnold I would say that we're both in the same boat 🙂

  1. Is definitely one of my major concerns as well - (un)fortunately, that comes with the territory, though. By shifting modules to community control it's a double-edged sword. It enables communities to control their own level of code quality, but it also puts the onus on them. For modules that are still under the care of the Salt team, my experience working on saltext.vmware is that not being concerned with the entire Salt codebase/test suite drastically simplifies some things. But it also does add some complexity because we end out needing to check against multiple versions of Salt. So... trade-offs 🙃 But generally each module can maintain its own release cycle, which I think far outweighs any of the complexity.
  2. This is basically one of the problems that has bitten us a couple of times with saltext.vmware. But again, not having to wait for Salt's release cycle is 🖤
  3. This is one of the areas in building out the Salt Extension Ecosystem, and specifically salt's support of it that we need to do some work. Whether it's building specific functionality or just improved documentation. As Tom points out, there will still be a not-insignificant core of Salt. But this is definitely an area that I expect we'll be learning a lot of lessons about 🙃
  4. 💯 - this is something very important to me and something I want to help solve. I have a lot of ideas around it, but I think is going to require a lot of documentation and maybe some tooling.

@max-arnold
Copy link

I think @myii can add something valuable here given his experience with using salt formulas as an integration test suite for Salt

@ITJamie
Copy link

ITJamie commented Aug 4, 2022

Is https://github.com/saltstack/salt-extension still envisioned as part of this?
If so it would be nice to see it get some love as its currently producing salt extension templates which don't pass nox tests on creation.

saltstack/salt-extension#15 for error with current template output

Also to the point someone else made. It would be really nice if extensions installed on the master synced down any states/modules/utils needed to the minion side. It feels like a bit of a regression to have to also install the same extensions on the minions.

@max-arnold
Copy link

max-arnold commented Jan 12, 2023

A little note: currently the https://extensions.saltproject.io index is not being updated. When the time to put this SEP into action will arrive, it also makes sense to allocate some development resources to fix and improve the extension index. See this Slack thread for more context on what is broken.

@dgengtek
Copy link

dgengtek commented Jun 20, 2023

Maybe this move could be the groundwork for some better support of "extensions" which aren't just write a function which expects these arguments but instead expect some data structure as in and output from these extensions. Which would potentially allow users to write their extensions in whatever language they want.

And in the far future maybe include other stuff like states, renderers, since these also "basically" are outputting state in a data structure.

The reason for saying this is because I doubt this will be pleasant for anyone since there is a lot of salts magic objects, variables.

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

Successfully merging this pull request may close these issues.

8 participants