SEP: Expose utility functions to states, but not to CLI or template context

[terminalmage]

No description provided.

[OrangeDog]

Can we examine why __utils__ is planned for removal, and whether perhaps

• a) it shouldn't be, and this can then use it
• b) that implies that option 2 is also a bad idea

[Ch3LL]

@s0undt3ch and @dwoz can you take a look at this

[s0undt3ch]

As you might guess from my comments, I'm against the SEP.

Every problem that this SEP tries to address, can be addressed today.
We do not need additional complexity in Salt.

[s0undt3ch]

Let me start by pointing out that 10 years is a long time ago, things have changed. The main idea, never supposed to include platform-specific code is still valid, when possible.

[s0undt3ch]

When not possible, there are still ways to achieve it without code duplication.

[terminalmage]

Then name them?

[s0undt3ch]

It was named

#70 (comment)

[s0undt3ch]

There are other ways to maintain consistency on __virtual__ checks.

[terminalmage]

Then name them?

[s0undt3ch]

It was named

#70 (comment)

[s0undt3ch]

So, the __virtual__ can be defined in the utility module and imported into the remote-execution module

Precisely, this is the way to avoid duplication.
Shouldn't use any salt dunders, or, if the function needs access to them, pass them in directly.

[terminalmage]

But it doesn't avoid duplication when it comes to invoking imported code in the state module. As I've mentioned both in the original Salt issue and in this SEP, you end up needing to duplicate the logic from the __virtual__ everywhere you want to use this imported code.

[s0undt3ch]

You import the util module that implements the virtual logic, and then, on each of the module needing that check, you call that function.

We're not arguing about adding import statements are we?

[s0undt3ch]

And we're also not arguing about adding a function call in each of the virtual functions right?

[terminalmage]

No and no. What is at issue here is the duplication of OS checks in state modules. Yes, it is a 10-year-old opinion. Pointing out that it's an old opinion is not a refutation of the opinion, it's just a non sequitur. There was a reason behind it, and that reason is that the loader is the right solution for running code specific to a given platform.

There are cases where simply importing a function from another module is a viable solution. Simpler state modules for which the execution module counterpart is not a virtual module are a great candidate for simply importing a function and running it. But pkg (and to a lesser extent, pkgrepo) are very different. There are more than 20 virtual pkg modules, representing almost as many different package managers. Many have functionality that is unique to them, which the pkg states have to be able to support.

Since the very early days of Salt, the way we have accounted for that is to use the loader. If a specific piece of functionality is only applicable to a single platform, Salt just checks if that function is in the loader. I gave several examples of this technique in this post, only to be told that the way Salt has been doing this since time immemorial is wrong, and should be considered "technical debt that needs to be fixed".

To be clear, the method required to "fix" this code in the pkg and pkgrepo states requires replacing loader membership checks with a reproduction of the same logic from the execution module's __virtual__ method, and also including a check in the minion opts to see if the pkg virtual has been assigned using providers. For example, that means replacing this:

if "pkg.check_db" not in __salt__:

with this:

import salt.modules.ebuildpkg

if not (
    (salt.modules.ebuildpkg.HAS_PORTAGE and __grains__["os"] == "Gentoo")
    or __opts__.get('providers', {}).get('pkg') == 'ebuildpkg'
):

That is the alternative to a loader membership check. That is what you are arguing in favor of. You have to reproduce everything that the loader is already doing for you.

Loader membership checks are the right way to handle these cases. This SEP is an attempt to find a way to balance this truth with the desire of the project not to further expose unnecessary functions to the loader.

[s0undt3ch]

To be clear, you will always be able to do:

if "pkg.check_db" not in __salt__:

This was never in question, unless check_db is a function which serves no user facing usefulness, which, in that case, should be moved to a utils module.

You can still create a utility module with all the required checks to reduce the verboseness of:

import salt.modules.ebuildpkg

if not (
    (salt.modules.ebuildpkg.HAS_PORTAGE and __grains__["os"] == "Gentoo")
    or __opts__.get('providers', {}).get('pkg') == 'ebuildpkg'
):

What I'm arguing for is not adding yet another complexity layer which will have to be maintained by the core team.
My position regarding this should be expected as the submitter of #66

Your counter-argument is that "virtual" state modules do a lot of checks, and switching that to use imported utilities will make it more complex.
You are correct, it will be more complex, for virtual modules. But it will be way less complex for non virtual modules.
The virtual state modules vs non virtual state modules ratio is quite low, so, extra work there is acceptable, if the overall complexity lowers.

This is my point of view.

[terminalmage]

To be clear, you will always be able to do:

if "pkg.check_db" not in __salt__:

This was never in question, unless check_db is a function which serves no user facing usefulness, which, in that case, should be moved to a utils module.

You can still create a utility module with all the required checks to reduce the verboseness of:

import salt.modules.ebuildpkg

if not (
    (salt.modules.ebuildpkg.HAS_PORTAGE and __grains__["os"] == "Gentoo")
    or __opts__.get('providers', {}).get('pkg') == 'ebuildpkg'
):

You'll still need to reproduce all the work the loader is already doing to detect OS and use of providers. Moving this code to a utils module doesn't do anything to fix the code duplication, it just hides it.

Your counter-argument is that "virtual" state modules do a lot of checks, and switching that to use imported utilities will make it more complex. You are correct, it will be more complex, for virtual modules. But it will be way less complex for non virtual modules. The virtual state modules vs non virtual state modules ratio is quite low, so, extra work there is acceptable, if the overall complexity lowers.

It's not "extra work". It's duplicating code, and then hoping that you remember to update that duplicated logic in the future if/when the code in the execution module drifts. Adding an attribute lookup to the loader is a less complex and more robust solution.

Arguing that there are a lot more non-virtual modules than virtual modules is another non-sequitur. They won't have the attribute, and will act the same way as they did before. The __virtual_aliases__ attribute in execution modules has been used only a few times, such as when transitioning a newer (i.e. "ng") implementation of a module to replace the original module (e.g. dockerng.py becoming dockermod.py), and allowing the old "ng" module name to continue to work for a few releases to allow people to transition.

The ratio of modules that have used __virtual_aliases__ to those that have not is even smaller than the ratio of virtual modules to non-virtual modules. That fact didn't make it a bad idea to add __virtual_aliases__, and likewise the ratio of virtual to non-virtual modules is not an argument against this proposed augmentation of the loader.

What I'm arguing for is not adding yet another complexity layer which will have to be maintained by the core team.

Am I unqualified to help maintain Salt? I was a core team member before I was employed by SaltStack, and I didn't lose my ability or inclination to contribute when I stopped being employed by SaltStack.

EDIT: Additionally, who if not the core team will have to keep in sync all the code duplication that will be added if this SEP is not implemented?

[s0undt3ch]

You'll still need to reproduce all the work the loader is already doing to detect OS and use of providers. Moving this code to a utils module doesn't do anything to fix the code duplication, it just hides it.

Not quite, there's no duplication on the utils module.

It's not "extra work". It's duplicating code, and then hoping that you remember to update that duplicated logic in the future if/when the code in the execution module drifts. Adding an attribute lookup to the loader is a less complex and more robust solution.

Not if you just call a utils function in the virtual functions. You update logic in one place, it works on all places using that function.

Arguing that there are a lot more non-virtual modules than virtual modules is another non-sequitur. They won't have the attribute, and will act the same way as they did before. The virtual_aliases attribute in execution modules has been used only a few times, such as when transitioning a newer (i.e. "ng") implementation of a module to replace the original module (e.g. dockerng.py becoming dockermod.py), and allowing the old "ng" module name to continue to work for a few releases to allow people to transition.

The ratio of modules that have used virtual_aliases to those that have not is even smaller than the ratio of virtual modules to non-virtual modules. That fact didn't make it a bad idea to add virtual_aliases, and likewise the ratio of virtual to non-virtual modules is not an argument against this proposed augmentation of the loader.

__virtual_aliases__ does not add to the current maintenance burden of Salt.

Am I unqualified to help maintain Salt? I was a core team member before I was employed by SaltStack, and I didn't lose my ability or inclination to contribute when I stopped being employed by SaltStack.

Erik, seriously?
If that's what you understood, I'm sorry, but I'll clarify.

Me, as paid maintainer of Salt, have the obvious obligation to fix any issues that come in.
Any Salt contributor, former employee or not, does not have the obligation to fix issues. As always, any fixes contributed by the community are more than welcomed by the core team, and, in fact, community contributed fixes increase the bug fix count allowing the core team to address other issues.

[terminalmage]

You'll still need to reproduce all the work the loader is already doing to detect OS and use of providers. Moving this code to a utils module doesn't do anything to fix the code duplication, it just hides it.

Not quite, there's no duplication on the utils module.

It's not "extra work". It's duplicating code, and then hoping that you remember to update that duplicated logic in the future if/when the code in the execution module drifts. Adding an attribute lookup to the loader is a less complex and more robust solution.

Not if you just call a utils function in the virtual functions. You update logic in one place, it works on all places using that function.

The issue is is not that the __virtual__ needs to do the same logic that something in a utils module would do, it's that a state module would need to duplicate the work the loader is already doing (i.e. OS checks, minion opts checks) to decide whether or not it should execute an imported function (irrespective of whether it is imported from the execution module or from a utils module).

And the example I showed was really just a best-case scenario. The functions in lowpkg for RPM serve no purpose on the CLI and would be candidates for moving to a utils module as part of the effort to "reduce technical debt". To decide whether or not to call them from the state module, you'd need to create an if statement that encompasses OS checks for RedHat and SUSE, as well as checking if lowpkg has been explicitly set to either yumpkg or zypper via providers.

Unless, of course, you have a solution for removing these OS and minion opts checks from the state module?

Arguing that there are a lot more non-virtual modules than virtual modules is another non-sequitur. They won't have the attribute, and will act the same way as they did before. The virtual_aliases attribute in execution modules has been used only a few times, such as when transitioning a newer (i.e. "ng") implementation of a module to replace the original module (e.g. dockerng.py becoming dockermod.py), and allowing the old "ng" module name to continue to work for a few releases to allow people to transition.
The ratio of modules that have used virtual_aliases to those that have not is even smaller than the ratio of virtual modules to non-virtual modules. That fact didn't make it a bad idea to add virtual_aliases, and likewise the ratio of virtual to non-virtual modules is not an argument against this proposed augmentation of the loader.

__virtual_aliases__ does not add to the current maintenance burden of Salt.

This is just moving the goalposts.

There will be an additional maintenance burden to maintain the additional OS and opts checks in the state modules and keep them in sync.

Let's assume your best-case scenario, in which the OS check exists in one place, within a utils module, where it is invoked by the execution module's __virtual__ and the state module (let's ignore for now that if #66 is implemented, the utils modules won't have access to the grains used for OS checks). You still have to check providers, in order to catch those edge cases where the virtual module is manually being assigned. That check still needs to be in the state module. It "adds to the maintenance burden of salt", so to speak.

By any measure, augmenting the loader is the less-complex solution. It doesn't require code duplication in the state modules. It automatically responds to changes in the execution modules. And it lets us continue to use loader membership to decide whether or not to run platform-specific code.

Am I unqualified to help maintain Salt? I was a core team member before I was employed by SaltStack, and I didn't lose my ability or inclination to contribute when I stopped being employed by SaltStack.

Erik, seriously? If that's what you understood, I'm sorry, but I'll clarify.

Me, as paid maintainer of Salt, have the obvious obligation to fix any issues that come in. Any Salt contributor, former employee or not, does not have the obligation to fix issues. As always, any fixes contributed by the community are more than welcomed by the core team, and, in fact, community contributed fixes increase the bug fix count allowing the core team to address other issues.

Any SEP will result in some future maintenance burden. In this case, not doing the SEP will entail its own, even uglier maintenance burden.

The excuse that the core team will have an additional maintenance burden can be used to suppress any SEP. If that's the case, then what are we even doing here?

[s0undt3ch]

There will be an additional maintenance burden to maintain the additional OS and opts checks in the state modules and keep them in sync.

Let's assume your best-case scenario, in which the OS check exists in one place, within a utils module, where it is invoked by the execution module's virtual and the state module (let's ignore for now that if #66 is implemented, the utils modules won't have access to the grains used for OS checks). You still have to check providers, in order to catch those edge cases where the virtual module is manually being assigned. That check still needs to be in the state module. It "adds to the maintenance burden of salt", so to speak.

A proper utils functions can do that check, and if it needs access to __opts__ or any other salt dunder, they should be explicitly passed.
The utility functions alone, should be enough.

Any SEP will result in some future maintenance burden. In this case, not doing the SEP will entail its own, even uglier maintenance burden.

Depends on the perspective. Might be ugly, but less magical, and thus, more predictable, testable, maintainable.

The excuse that the core team will have an additional maintenance burden can be used to suppress any SEP. If that's the case, then what are we even doing here?

I can't reject a SEP alone, and I'm not talking on behalf of the whole core team.

[s0undt3ch]

Suggested change

	[alternatives]: #alternatives
	This SEP already contains two potential implementations. The alternative to
	implementing this SEP would be to duplicate the logic from the `__virtual__` at
	every location where utility functions need to be used, which is an open
	invitation for the two (or more) copies of that logic to drift from one
	another.
	[alternatives]: #alternatives
	This SEP already contains two potential implementations. The alternative to
	implementing this SEP would be to duplicate the logic from the `__virtual__` at
	every location where utility functions need to be used, which is an open
	invitation for the two (or more) copies of that logic to drift from one
	another.

This is not an alternative. I've pointed out how it should be done in the previous comment.

[s0undt3ch]

Suggested change

	- For [Option 1](#option-1-augmentation-of-lazyloader), Salt once had a test
	which parsed all docstrings in every execution module, ensuring that CLI
	examples were included. I'm not sure if this test still exists, but utility
	functions would not need CLI examples and this test case (if still present)
	would need to be updated to account for this fact.
	- For [Option 1](#option-1-augmentation-of-lazyloader), Salt once had a test
	which parsed all docstrings in every execution module, ensuring that CLI
	examples were included. I'm not sure if this test still exists, but utility
	functions would not need CLI examples and this test case (if still present)
	would need to be updated to account for this fact.

There isn't a test case, but there is a pre-commit hook.

[terminalmage]

No longer a concern with my proposed implementation from #70 (comment), since the utility functions now remain prefixed with underscores.

[terminalmage]

Here's a patch that implements this SEP, including automated tests to confirm it.

I even changed the implementation from the initial proposal. The utility functions are kept as private funcs, meaning that we don't have to worry about modifying the docs to hide them, nor deal with modifying the pre-commit check which looks for CLI examples in public functions. Even better!

(Note that this wouldn't be the full implementation. Since the functions argument is no longer used in salt.loader.states(), a full implementation would probably involve removing those arguments from invocations of salt.loader.states() and updating related test cases.)

The added functionality is simple, and the test cases are similarly easy-to-understand.

Using my implementation:

• We can continue to rely on loader membership (as Salt has for more than 10 years) to check if there is a platform-specific function implemented for a given platform.
• Utility functions in execution modules can be invoked from state modules without them being unnecessarily exposed to the CLI.
• We don't have to add additional layers of abstraction to prevent code duplication.
• We don't need to need to manually perform opts checks to recreate the providers functionality already provided by the loader.

Without my implementation, the position of the project is that loader membership checks are "techincal debt". To fix that technical debt, each platform-specific function referenced in the pkg/pkgrepo state modules, which is not useful on the CLI, would need to be moved to a utils module (in many cases creating a whole new utils module for this purpose). Since this "fix" would remove that function from the CLI, this removal would require that the function be deprecated prior to removal. Additionally, to prevent code duplication in the __virtual__ function, the platform checks done in the __virtual__ would also need to be moved to a utils module. This method also requires that we pass in __opts__ and __grains__ to all invocations of the utils function that performs the platform check, both in the execution module's __virtual__ and in any place where we want to do a platform check in the state module. (NOTE: This wouldn't technically be necessary unless #66 is implemented. But, if we don't pass in the __opts__ and __grains__, and then #66 is approved and implemented, part of the implementation of #66 would require that we revisit the code in these utils modules. Each function that needs __opts__ or __grains__ would need be updated so that __opts__ and __grains__ are passed in, and all invocations of these functions would also need to be updated to pass them. A number of test cases would also need to be updated.)

Please correct me if any of the details in the above paragraph are inaccurate, I don't want to be seen as misrepresenting things.

It has been argued that my proposal adds to Salt's maintenance burden. I've shown with my patch just how elegant this solution is, and how easy it is to test. It also saves the project a lot of technical debt, as loader membership checks would not need to be removed.

The alternative appears to be far more burdensome. If I am mistaken in my analysis of the alternative, I welcome being proven wrong, because the more-elegant solution will win and Salt will be better for it.

[cmcmarrow]

@terminalmage
What do you think of this idea, where we move the check to its own util.

salt.utils.portage_loader.py

import os
import sys

HAS_PORTAGE = False
try:
    import portage

    HAS_PORTAGE = True
except ImportError:
    if os.path.isdir("/usr/lib/portage/pym"):
        try:
            # In a virtualenv, the portage python path needs to be manually added
            sys.path.insert(0, "/usr/lib/portage/pym")
            import portage

            HAS_PORTAGE = True
        except ImportError:
            pass


def portage_status():
    return HAS_PORTAGE and __grains__["os"] == "Gentoo"

salt/modules/ebuildpkg.py

import copy
import datetime
import logging
import os
import re

import salt.utils.args
import salt.utils.compat
import salt.utils.data
import salt.utils.functools
import salt.utils.path
import salt.utils.pkg
import salt.utils.systemd
import salt.utils.versions
from salt.exceptions import CommandExecutionError, MinionError
import salt.utils.portage_loader
try:
    import portage
except ImportError:
    pass

log = logging.getLogger(__name__)

# Define the module's virtual name
__virtualname__ = "pkg"


def __virtual__():
    """
    Confirm this module is on a Gentoo based system
    """
    if salt.utils.portage_loader.portage_status():
        return __virtualname__
    return (
        False,
        "The ebuild execution module cannot be loaded: either the system is not Gentoo"
        " or the portage python library is not available.",
    )
# ...

salt.utils.other.py

import salt.utils.portage_loader
try:
    import portage
except ImportError:
    pass

# Define the module's virtual name
__virtualname__ = "other"


def __virtual__():
    """
    Confirm this module is on a Gentoo based system
    """
    if salt.utils.portage_loader.portage_status():
        return __virtualname__
    return (
        False,
        "No Portage"
    )
# ...

[terminalmage]

Given that this function would presumably need to be called from other places that are unrelated a virtual function, perhaps a better name for the function would be has_portage.

[cmcmarrow]

I dont see anything wrong with the name change, plus we can add portage_virtual

salt.utils.has_portage.py

import os
import sys

HAS_PORTAGE = False
try:
    import portage

    HAS_PORTAGE = True
except ImportError:
    if os.path.isdir("/usr/lib/portage/pym"):
        try:
            # In a virtualenv, the portage python path needs to be manually added
            sys.path.insert(0, "/usr/lib/portage/pym")
            import portage

            HAS_PORTAGE = True
        except ImportError:
            pass


def portage_status():
    return HAS_PORTAGE and __grains__["os"] == "Gentoo"


def portage_virtual(virtual_name, failed_message=None):
    if portage_status():
        return virtual_name
    if failed_message is None:
        failed_message = f"{virtual_name} cannot be loaded: either the system is not Gentoo\n or the portage python library is not available."
    return False, failed_message

import copy
import datetime
import logging
import os
import re

import salt.utils.args
import salt.utils.compat
import salt.utils.data
import salt.utils.functools
import salt.utils.path
import salt.utils.pkg
import salt.utils.systemd
import salt.utils.versions
from salt.exceptions import CommandExecutionError, MinionError
import salt.utils.portage_loader
try:
    import portage
except ImportError:
    pass

log = logging.getLogger(__name__)

# Define the module's virtual name
__virtualname__ = "pkg"


def __virtual__():
    return salt.utils.portage_virtual(__virtualname__)
# ...