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

[master] Define "__virtualname__" for transactional_update module #64369

Merged
merged 4 commits into from Aug 18, 2023

Conversation

meaksh
Copy link
Contributor

@meaksh meaksh commented May 26, 2023

What does this PR do?

On openSUSE MicroOS and SLE Micro, transactional systems, this prevent problems with "LazyLoader" when importing transactional_update module, which was wrongly exposing functions for this module under state.* on Salt >= 3005.

Previous Behavior

The following exception is triggered by salt minion when calling state.apply:

   The minion function caused an exception: Traceback (most recent call last):
      File "/usr/lib/python3.10/site-packages/salt/minion.py", line 1939, in _thread_return
        return_data = minion_instance._execute_job_function(
      File "/usr/lib/python3.10/site-packages/salt/minion.py", line 1898, in _execute_job_function
        return_data = self.executors[fname](opts, data, func, args, kwargs)
      File "/usr/lib/python3.10/site-packages/salt/loader/lazy.py", line 149, in __call__
        return self.loader.run(run_func, *args, **kwargs)
      File "/usr/lib/python3.10/site-packages/salt/loader/lazy.py", line 1230, in run
        return self._last_context.run(self._run_as, _func_or_method, *args, **kwargs)
      File "/usr/lib/python3.10/site-packages/salt/loader/lazy.py", line 1245, in _run_as
        return _func_or_method(*args, **kwargs)
      File "/usr/lib/python3.10/site-packages/salt/executors/transactional_update.py", line 123, in execute
        opts, data, __salt__[DELEGATION_MAP[fun]], args, kwargs
      File "/usr/lib/python3.10/site-packages/salt/loader/context.py", line 78, in __getitem__
        return self.value()[item]
      File "/usr/lib/python3.10/site-packages/salt/loader/lazy.py", line 336, in __getitem__
        super().__getitem__(item)  # try to get the item from the dictionary
      File "/usr/lib/python3.10/site-packages/salt/utils/lazy.py", line 105, in __getitem__
        raise KeyError(key)
    KeyError: 'transactional_update.apply'

New Behavior

The state.apply works as expected.

Merge requirements satisfied?

[NOTICE] Bug fixes or features added to Salt require tests.

Commits signed with GPG?

Yes

Please review Salt's Contributing Guide for best practices.

See GitHub's page on GPG signing for more information about signing commits with GPG.

This prevent problems with LazyLoader when importing this module,
which was wrongly exposing functions for this module under "state.*"
@meaksh meaksh requested a review from a team as a code owner May 26, 2023 14:15
@meaksh meaksh requested review from cmcmarrow and removed request for a team May 26, 2023 14:15
@salt-project-bot-prod-environment salt-project-bot-prod-environment bot changed the title Define "__virtualname__" for transactional_update module [master] Define "__virtualname__" for transactional_update module May 26, 2023
@meaksh meaksh temporarily deployed to ci May 26, 2023 15:56 — with GitHub Actions Inactive
@meaksh meaksh temporarily deployed to ci May 26, 2023 15:56 — with GitHub Actions Inactive
@meaksh meaksh temporarily deployed to ci May 26, 2023 15:56 — with GitHub Actions Inactive
@meaksh meaksh temporarily deployed to ci May 26, 2023 17:10 — with GitHub Actions Inactive
@meaksh meaksh temporarily deployed to ci May 26, 2023 17:10 — with GitHub Actions Inactive
@meaksh meaksh temporarily deployed to ci May 26, 2023 17:10 — with GitHub Actions Inactive
@meaksh meaksh temporarily deployed to ci May 26, 2023 17:32 — with GitHub Actions Inactive
@meaksh meaksh temporarily deployed to ci May 26, 2023 17:32 — with GitHub Actions Inactive
@meaksh meaksh temporarily deployed to ci May 26, 2023 17:32 — with GitHub Actions Inactive
Copy link
Contributor

@Ch3LL Ch3LL left a comment

Choose a reason for hiding this comment

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

Is this error only showing up on openSUSE MicroOS and SLE Micro, transactional systems ? If so why is that the case?

I'm wondering if we can add test coverage here easily or not.

This will certainly need a changelog though.

@meaksh
Copy link
Contributor Author

meaksh commented Jun 28, 2023

@Ch3LL sorry for the delay answering here.

Is this error only showing up on openSUSE MicroOS and SLE Micro, transactional systems ? If so why is that the case?

Yes, that is the case atm. The "transactional_update" execution module is only available on transactional systems, where transactional-update CLI is available.

I think this might have some relation with the usage of the "transactional_update" executor, which is configured by default in Salt configuration for transactional systems:

# Enable the transactional_update executor
module_executors:
  - transactional_update
  - direct_call

This executor [1], contains a set of modules and functions that must be executed via "transactional_update" module, as they are expected to make changes on the readonly system and therefore a new transaction/snapshot is required.

One of these function is state.apply of course. Other modules goes directly to direct_call, i.a. test.ping.

As mentioned, without defining this __virtualname__, and maybe because of this extra executor layer, the LazyLoader seems to mess (in Salt >= 3005) and functions from transactional_update execution module end up being exposed as state.*.

One note: the transactional_update executor module does import some things from salt.modules.state, and also import salt.client.ssh.state and salt.client.ssh.wrapper.state. Not sure if that could be confusing the LazyLoader mechanism.

I'm wondering if we can add test coverage here easily or not.

It doesn't seems straight forward, as it is not really clear yet what is triggering this behavior.

This will certainly need a changelog though.

I've added it now 👍

[1] https://github.com/saltstack/salt/blob/master/salt/executors/transactional_update.py

@meaksh meaksh temporarily deployed to ci June 28, 2023 10:08 — with GitHub Actions Inactive
@meaksh meaksh temporarily deployed to ci June 28, 2023 10:08 — with GitHub Actions Inactive
@meaksh meaksh temporarily deployed to ci June 28, 2023 10:08 — with GitHub Actions Inactive
@meaksh meaksh temporarily deployed to ci June 28, 2023 10:23 — with GitHub Actions Inactive
@meaksh meaksh temporarily deployed to ci June 28, 2023 10:31 — with GitHub Actions Inactive
@meaksh meaksh temporarily deployed to ci June 28, 2023 10:54 — with GitHub Actions Inactive
@meaksh meaksh temporarily deployed to ci June 28, 2023 15:17 — with GitHub Actions Inactive
@meaksh meaksh temporarily deployed to ci June 28, 2023 15:17 — with GitHub Actions Inactive
@meaksh meaksh temporarily deployed to ci June 28, 2023 15:17 — with GitHub Actions Inactive
@meaksh meaksh temporarily deployed to ci June 28, 2023 15:17 — with GitHub Actions Inactive
@meaksh meaksh temporarily deployed to ci June 28, 2023 15:17 — with GitHub Actions Inactive
@meaksh meaksh temporarily deployed to ci June 28, 2023 15:17 — with GitHub Actions Inactive
@meaksh meaksh requested a review from Ch3LL June 30, 2023 11:50
@Ch3LL Ch3LL merged commit d3f204e into saltstack:master Aug 18, 2023
309 checks passed
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

Successfully merging this pull request may close these issues.

None yet

4 participants