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

SEP 22: Compartmentalize salt: Use pop-grains (grainsv2) for grain collection #30

Closed
wants to merge 8 commits into from
Closed

SEP 22: Compartmentalize salt: Use pop-grains (grainsv2) for grain collection #30

Changes from all commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
03635e4
added corn SEP
Akm0d Jun 15, 2020
3dbf254
spelling fixes
Akm0d Jun 15, 2020
f94cd0e
Fix grammar
Akm0d Jun 16, 2020
18c9083
Update compartmentalize-grains-via-pop.md
Akm0d Jun 16, 2020
ffd805d
Updated based on initial discussions
Akm0d Jun 19, 2020
87ad286
Merge branch 'pop_idem' of github.com:Akm0d/salt-enhancement-proposal…
Akm0d Jun 19, 2020
ea5a4f1
testing will continue on core.py
Akm0d Jun 19, 2020
f2c8705
fixed grammar errors
Akm0d Jun 19, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
74 changes: 74 additions & 0 deletions 0022-compartmentalize-grains-via-pop.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,74 @@
- Feature Name: compartmentalize-grains-via pop
- Start Date: 2020-06-22
- SEP Status: Draft
- SEP PR: https://github.com/saltstack/salt/pull/57681
- Salt Issue: https://github.com/saltstack/salt/issues/57680

# Summary
[summary]: #summary

Grains is currently a static component of salt. We want it to be developed in a more compartmentalize fashion.
Akm0d marked this conversation as resolved.
Show resolved Hide resolved
The engineers at SaltStack have created platform-specific projects that functionally replace the grains
provided by salt with POP. We propose that this project, codenamed `grainsv2` becomes available in salt while
maintaining backwards compatibility.

# Motivation
[motivation]: #motivation

"The original implementation of grains was not designed to scale to the level that salt requires" - thatch45
Copy link
Contributor

Choose a reason for hiding this comment

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

My impression is that scaling issues currently revolve around the event bus and job storage, not the grains implementation. Can we elaborate on what we mean by scale here? How does the current implementation fail to scale? How does the proposed change make things more scalable?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@thatch45 ^^

Tom wrote POP to solve this problem and we're going to use it.

By using Plugin Oriented Programming, grains can be separated into individual projects that can be maintained
Copy link
Contributor

Choose a reason for hiding this comment

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

Are there other alternative approaches we could take instead of 'Plugin Oriented Programming'?

Copy link

@taigrr taigrr Jun 22, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

For something as important as grains to be changing, I'd like to take a more in depth look at POP in salt to understand the significance before a major migration is undertaken.

by the community safely.

This also solves the issue of `core.py` being an unmaintainable monolith. It allows platform specific grains
Copy link
Contributor

Choose a reason for hiding this comment

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

Would and alternate method be to simply split up core.py into multiple files?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Grains is the first focus of this SEP and the associated changes. It also opens the door for getting execution modules and state modules from idem in the future. For example, Mac specific execution modules could be put in idem-darwin, tested in idem-darwin, and PRs/issues can be handled by the Mac working group. Grainsv2 is the simplest and first step towards more compartmentalization of salt in this way.

Copy link
Contributor

Choose a reason for hiding this comment

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

This is one of the thing I think that should be highlighted more in the SEP. We're saying this is opening the door to door to all of these changes, but we're also saying

What will further adoption of POP mean inside of salt?  The purpose of this sep and these features is not to provide a definitive answer to this question

Which is it?

to be independently maintained.

Working group leaders and other community members focused on specific platforms can manage the release cycle for
individual idem-platform projects independently from salt.

# Design
[design]: #detailed-design

Grains are currently created by calling functions in the salt loader. We would simply need to call
external POP functions to collect grains. We need to maintain backwards compatibility with custom user grains.

- All salt grains have already been ported to `grainsv2`.
Copy link
Contributor

Choose a reason for hiding this comment

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

Are we sure that all of the changes that have taken place in recent releases are also present in the current pop version?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I have been focused on idem-cloud since March, there will need to be a code freeze of future PRs into core.py and changes in the past two months into core.py will need to be merged into the new platform.

- `grainsv2` has many more features than the salt-grains implementation, but is compatible with salt grains.
- None of the grain names have been changed, but the output of grains is more consistent/complete between platforms.
- This will resolve many bugs that are open against grains and will migrate those bugs OUT of the salt
Copy link
Contributor

Choose a reason for hiding this comment

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

Which bugs specifically?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No specific open issues, but a lot of work has been done to standardize grains between the different idem platforms. For example, idem-windows implements the oscodename grain and the gpus grain, which are missing on windows in salt. grainsv2 is more complete and consistent across all platforms

Copy link

Choose a reason for hiding this comment

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

Have PRs been opened to fix these things in the current grains?

Copy link
Contributor Author

@Akm0d Akm0d Jun 22, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

not to my knowledge.

Copy link
Contributor

Choose a reason for hiding this comment

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

This will resolve many bugs that are open against grains

conflicts directly with

No specific open issues

Are there open bugs that this solves, or not? If not, perhaps this SEP should not claim that there are. Regardless, I think this point needs to be clarified.

platform -- making salt easier to develop
- Grains in the new platform are independently fully tested.
Copy link
Contributor

Choose a reason for hiding this comment

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

Are they tested against current versions of salt?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes, this PR has a test that runs both versions of grains and does a line by line comparison of each

Copy link

Choose a reason for hiding this comment

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

Farming out implementations to outside of salt itself h bring up new continuity issues when there's no central authority to determine if for example Ubuntu or Void has the "official/accepted/blessed" format/implementation.

(Examples picked for systemd disagreement reasons)

Copy link
Contributor

@dwoz dwoz Jun 22, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

Good point, also, how would we ensure that a requires_reboot grain on one system isn't named requires_restart on another system?

Copy link
Contributor

Choose a reason for hiding this comment

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

To build on @dwoz's question, assuming that the platform-specific projects are going to be providing certain grains that used to be in core.py, what are the plans for enforcing that a given platform's idem-XXXXX project provides the grains it is supposed to?

- Development will freeze on core.py and the salt/grains folder.
- This comprises a complete rewrite of ALL grains. Backwards compatibility is expected, but we need this to get
in early so that any unintentional issues can be reet up a conversation with the relevant parties solved quickly.
- Salt releases will aggressively pin `grainsv2`/idem-platform dependencies with each release.
- Users will be to update their grains implementation to a custom idem-platform version.
- The use of `grainsv2` will be gated with a config variable; It will be disabled by default.
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we should discuss how we gate this change. Adding a config value that will be rendered un-needed in a future release doesn't seem like a great approach to me.

Copy link
Contributor

Choose a reason for hiding this comment

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

Perhaps a new value isn't necessary. We have use_superseded, which was only being used for the format change in module.run. Couldn't it be used here?

- New development for grains will be done in `grainsv2` projects and core.py will remain static.
Copy link
Contributor

Choose a reason for hiding this comment

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

So does this mean that no new core grains will be able to be added to Python 3.5 platforms (Ubuntu 16.04 LTS, etc.)?

- When `grainsv2` gains maturity in salt on the other side of the gate (and python3.5 is dropped) it will completely replace core.py
Copy link
Contributor

Choose a reason for hiding this comment

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

What indicators will be used to gauge the maturity of this new version? How can we ensure that we're not decreasing stability of salt? What tests need to be run?


## Alternatives
[alternatives]: #alternatives

Keep things the way they are. Does this mean we would suffer in stagnation? Will we be stuck maintaining a monolith.
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think it is fair to say the only alternative is maintaining a monolith. I can think of several other possibilities.

  • Simply breaking up core.py (the monolith) into multiple files inside the salt codebase.
  • Creating separate projects for grains does not inherently require the addition of POP. This could be done with simple projects using python's build in mechanisms allowing for plugin type projects. One example of this is pytest, django also has been doing this for a long time.


## Unresolved questions
[unresolved]: #unresolved-questions

What will further adoption of POP mean inside of salt? The purpose of this sep and these features is not to provide
Copy link
Contributor

Choose a reason for hiding this comment

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

While the purpose of this sep does not intend to provide an answer to this question it certainly seems to provide an opinion about it. I feel it would be pertinent to this discussion, especially because the current PR implementing this change is adding a lot more than just a grains specific library. The addition of idem-* means would be adding idem and POP into salt core.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You're right, this SEP and the accompanying PR do provide strong opinions on this, I mean to say that the future of idem in POP in salt has been discussed by a small group, but warrants a larger discussion, more firm plans, and possibly it's own SEP. With this PR we add the __hub__ to the loader, which means custom execution modules and states could make use of idem's hub... but we have discussed adding the option to specify the salt/idem engine in salt states (defaulting to the salt engine) is a possible future integration of idem into salt. This SEP opens the door for that discussion, but that is an implementation/discussion that should happen separately from grains.

Copy link
Contributor

Choose a reason for hiding this comment

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

Seems like the addition of __hub__ is inviting a lot of confusion and potential complexity to and already complex code base in regards to the loader. What problem is the addition of __hub__ intended to solve? How will developers know which one to use? What happens if we mix the use of __salt__ and __hub__?

a definitive answer to this question -- rather to introduce plugin oriented programming into salt in a very safe way.
In the future this will determine the design so that salt can be further compartmentalized and written in POP.
Copy link
Contributor

Choose a reason for hiding this comment

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

Have we had a discussion with the core team and community at large around how we want to compartmentalize salt or has this decision been made (by a small group) already?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Compartmentalizing salt is a discussion that has been had in small groups, and it will happen in larger groups before decisions are made


# Drawbacks
[drawbacks]: #drawbacks

Why should we *not* do this? Please consider:

- We will be introducing a number of new dependencies.
- The new implementation of grains/pop is written with python3.6 and later with asyncio
Copy link
Contributor

Choose a reason for hiding this comment

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

I feel the asyncio portion also warents further exploration and discussion. Especially considering the challenges we've had trying to upgrade tornado to a version which uses asyncio.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I have looked at the structure of corn.

  • idem-aix
  • idem-bsd
  • idem-solaris
  • idem-dawwin
  • idem-linux

corn/grain fqdn is the same on all platforms, why write the code 5 times?
Why is there a idem-linux vs idem-rhel and idem-debian etc I assume to save having the same code in multiple places.
Why not a idem-posix This is well documented standard which Linux/Unix is based on, and then anything outside this standard then use idem-solaris, etc.
Why not have idem-py i.e. anything which can be completed in pure python e.g. fqdn. idem-py Would exclude the use of posix functions like chmod(), chown() as they are Unix based and hence not 100% multi-platform like most of python.

Is this comment going to be addressed?

Pure python grains and idem-posix are great ideas 👍. We will create an idem-universal for pure python grains/exec/state modules and idem-posix that has the shared code between posix platforms. I am especially a fan of not having to push updates to idem's implementation of cmd.run to 7 different platform specific repos. Code that is similar between platforms should be in universal repos.

(The latest release of salt is also python3.6 and later).
- Some grains for specific OSes (such as SUSE) implement some grains differently from other platforms. We need
Copy link
Contributor

Choose a reason for hiding this comment

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

This seems like we are adding a fair amount management and community synchronization overhead. Are we sure we have thought through all of this and developed the processes needed to deal with it?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think this is an excellent point and exposes the crux of what needs to be discussed in this SEP. Broadly I've speculated that the windows working group would manage idem-windows and the mac working group would manage idem-darwin and at a certain point in the release there would be a discussion about which version of idem-darwin and idem-windows would be pinned to the next release of salt. But we do need a concrete process in place.

to set up a conversation with the relevant parties to resolve such conflicts.
- Documentation will be the heaviest lifting. We need to document how to make salt-style internal grains and generally
Copy link
Contributor

@dwoz dwoz Jun 22, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

Specifically what documentation are we lacking now? What is the plan and time-lines around getting all of the needed documentation in place?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

For grains there isn't much more documentation changes that will need to happen, but for the future of the compartmentalization of salt this is an unaddressed problem. Execution and state modules could be maintained in platform specific projects... How do we merge that documentation into salt?

available idem-grains.