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

Refactor and improvements for "transactional-updates" module #61188

Merged

Conversation

meaksh
Copy link
Contributor

@meaksh meaksh commented Nov 5, 2021

What does this PR do?

This PR changes the way the transactional-update module is working internally in order to run Salt actions inside new transactions.

The previous architecture used the Salt SSH approach, using the SSH wrappers, Salt thin and state.pkg in order to execute a Salt action inside a transaction. We found this is a bit problematic, and produce some issues particularly when dealing with complex/fancy states like:

# this is example1.sls

my_test_state:
  module.run:
    - name: state.apply

The above state produce the following error when running in a transactional system:

slem51:~ # salt-call state.apply example1
local:
----------
          ID: test_example1
    Function: module.run
        Name: state.apply
      Result: False
     Comment: Module function state.apply executed
     Started: 10:42:19.295456
    Duration: 71.843 ms
     Changes:   
              ----------
              ret:
                  ----------
                  no_|-states_|-states_|-None:
                      ----------
                      __run_num__:
                          0
                      changes:
                          ----------
                      comment:
                          No Top file or master_tops data matches found. Please see master log for details.
                      name:
                          No States
                      result:
                          False

Summary for local
------------
Succeeded: 0 (changed=1)
Failed:    1
------------
Total states run:     1
Total run time:  71.843 ms

Similarly with this other state, where filerefs appears as missing (will force us to use extra_filerefs for this particular call):

# this is example2.sls

run_test2:
  module.run:
    - name: state.apply
    - mods: tests2

and

# this is tests2.sls

something_test:
  cmd.run:
    - name: date

In this case we are getting:

slem51:~ # salt-call state.apply example2
local:
----------
          ID: run_test2
    Function: module.run
        Name: state.apply
      Result: True
     Comment: Module function state.apply executed
     Started: 10:45:22.012088
    Duration: 89.861 ms
     Changes:   
              ----------
              ret:
                  - No matching sls found for 'tests2' in env 'base'

Summary for local
------------
Succeeded: 1 (changed=1)
Failed:    0
------------
Total states run:     1

What this PR does in order to solve this problems is the following:

  1. Use a normal "salt-call" execution instead SSH approach to execute a Salt action inside a new transaction.
  2. Make use of concurrent flag to allow execution of the underlying "salt-call" in case of states execution.
  3. Implements --no-return-event for "salt-call" to avoid getting duplicated events on the Salt event bus.
  4. Solved the above issues about missing filerefs or master top data.
  5. Fixed a problem with state.highstate not acting according to concurrent flag described in Salt documentation.

This brings some benefits:

  • No need to struggle with "extra_filerefs" for those states that contains references that are not included in the state.pkg.
  • More transparent approach. Should cause less troubles and work with complex states.

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.

@meaksh meaksh requested a review from a team as a code owner November 5, 2021 11:01
@meaksh meaksh requested review from krionbsd and aplanas and removed request for a team November 5, 2021 11:01
@meaksh
Copy link
Contributor Author

meaksh commented Nov 5, 2021

I notice there are few tests failures, I'm pushing necessary fixes

@garethgreenaway garethgreenaway added the Phosphorus v3005.0 Release code name and version label Apr 20, 2022
twangboy
twangboy previously approved these changes May 2, 2022
changelog/61188.fixed Outdated Show resolved Hide resolved
salt/utils/parsers.py Show resolved Hide resolved
salt/modules/state.py Show resolved Hide resolved
salt/modules/transactional_update.py Show resolved Hide resolved
salt/modules/transactional_update.py Show resolved Hide resolved
@meaksh
Copy link
Contributor Author

meaksh commented Jun 1, 2022

@Ch3LL thanks for the review! 👍

I've pushed some of the requested changes and did some comments about the parameter deprecations.

@github-actions
Copy link

github-actions bot commented Jun 1, 2022

Hi! I'm your friendly PR bot!

You might be wondering what I'm doing commenting here on your PR.

Yes, as a matter of fact, I am...

I'm just here to help us improve the documentation. I can't respond to
questions or anything, but what I can do, I do well!

Okay... so what do you do?

I detect modules that are missing docstrings or "CLI Example" on existing docstrings!
When I was created we had a lot of these. The documentation for these
modules need some love and attention to make Salt better for our users.

So what does that have to do with my PR?

I noticed that in this PR there are some files changed that have some of these
issues. So I'm leaving this comment to let you know your options.

Okay, what are they?

Well, my favorite, is that since you were making changes here I'm hoping that
you would be the most familiar with this module and be able to add some other
examples or fix any of the reported issues.

If I can, then what?

Well, you can either add them to this PR or add them to another PR. Either way is fine!

Well... what if I can't, or don't want to?

That's also fine! We appreciate all contributions to the Salt Project. If you
can't add those other examples, either because you're too busy, or unfamiliar,
or you just aren't interested, we still appreciate the contributions that
you've made already.

Whatever approach you decide to take, just drop a comment here letting us know!

Detected Issues (click me)
Check Known Missing Docstrings...........................................Failed
- hook id: invoke
- duration: 1.57s
- exit code: 1

/home/runner/.cache/pre-commit/repo8mv9kx9s/py_env-python3/lib/python3.9/site-packages/distutils_hack/init.py:30: UserWarning: Setuptools is replacing distutils.
warnings.warn("Setuptools is replacing distutils.")
The function 'get_pauses' on 'salt/modules/state.py' does not have a 'CLI Example:' in it's docstring
The function 'apply
' on 'salt/modules/state.py' does not have a 'CLI Example:' in it's docstring
The function 'test' on 'salt/modules/state.py' does not have a 'CLI Example:' in it's docstring
Found 3 errors


Thanks again!

@Ch3LL Ch3LL merged commit 844753f into saltstack:master Jun 1, 2022
@meaksh meaksh deleted the master-transactional-update-enhacements branch June 2, 2022 08:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Phosphorus v3005.0 Release code name and version
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants