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

Fix two O(n^2) performance bugs in the sysctl.present state #58732

Merged
merged 12 commits into from
Feb 3, 2021

Conversation

asomers
Copy link
Contributor

@asomers asomers commented Oct 14, 2020

What does this PR do?

Fixes two O(n^2) performance bugs in the sysctl.present state

Previous Behavior

Two bugs:

  • When test=True, the sysctl.present state would call the sysctl module to get the values of every_single_sysctl. Sometimes there are tens of thousands of those. And it would do that again, for every_single sysctl.present state.
  • The sysctl.show function would split the output of the sysctl command line-by-line. For multiline values, it would reassemble them by appending to a string, which is O(n^2).

New Behavior

  • Now the sysctl.present state will call sysctl.get instead of sysctl.show, only looking up a single value.
  • sysctl.show concatenates strings using str.join, which is much faster.

Merge requirements satisfied?

Commits signed with GPG?

Yes

Tests run?

yes

@asomers asomers requested a review from a team as a code owner October 14, 2020 23:44
@asomers asomers requested review from waynew and removed request for a team October 14, 2020 23:44
@ghost ghost requested a review from Ch3LL October 14, 2020 23:44
@asomers
Copy link
Contributor Author

asomers commented Oct 16, 2020

Fixing the tests is blocked by #58746 .

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.

Can you also add a changelog entry as documented here: https://docs.saltstack.com/en/master/topics/development/changelog.html

salt/states/sysctl.py Outdated Show resolved Hide resolved
@Ch3LL Ch3LL requested a review from krionbsd October 22, 2020 11:35
@waynew
Copy link
Contributor

waynew commented Oct 22, 2020

Hey @asomers thanks for the PR!

I've been OOO for a bit and am just catching up, apologies for the wait! I'll be reviewing this PR as soon as I can, thanks for your patience!

@asomers
Copy link
Contributor Author

asomers commented Oct 22, 2020

Thanks @waynew . But please review #58746 first. That PR blocks this one.

When looking up a single sysctl, just look up that single one.  Don't
look up every sysctl in the system.
@asomers
Copy link
Contributor Author

asomers commented Nov 13, 2020

I rebased to incorporate the changes from PR #58746 , and I also fixed @Ch3LL's objection. This should be good to review now.

@asomers
Copy link
Contributor Author

asomers commented Nov 14, 2020

On one particular server that I manage, this change reduces the time for state.apply test=True from 126 minutes down to less than 6 minutes. On another server, I previously timed that operation as taking more than 5 hours, but I haven't rerun it with this patch yet.

@asomers
Copy link
Contributor Author

asomers commented Nov 17, 2020

I added the changelog entry as @Ch3LL requested.

krionbsd
krionbsd previously approved these changes Nov 17, 2020
@krionbsd krionbsd added the Aluminium Release Post Mg and Pre Si label Nov 17, 2020
tests/unit/modules/test_freebsd_sysctl.py Outdated Show resolved Hide resolved
Copy link
Contributor

@waynew waynew left a comment

Choose a reason for hiding this comment

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

Looks good, just a couple of minor (potential) improvements.

self.assertDictEqual(sysctl.present(name, value), ret)
with patch.dict(sysctl.__salt__, {"sysctl.show": mock_config}):
mock_get = MagicMock(return_value=old_value)
with patch.dict(sysctl.__salt__, {"sysctl.get": mock_get}):
Copy link
Contributor

Choose a reason for hiding this comment

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

suggestion: Given the number of places where we're mocking stuff like sysctl.get, we should probably import the function and use create_autospec to create these, if we can.

suggestion: deeply nested with statements can be avoided by rewriting like so:

mock_get = create_autospec(sysctl.get, return_value=old_value)
patch_opts = patch.dict(sysctl.__opts__, {"test": True})
patch_salt = patch.dict(sysctl.__salt__, {"sysctl.show": mock_config, "sysctl.get", mock_get})

with patch_opts, patch_salt:
    self.assertDictEqual(...)

This could be done throughout the tests in this PR

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 can't use create_autospec, because there is no sysctl.get object. And the sysctl.__salt__ dict is empty until I put a mock inside.

Copy link
Contributor

Choose a reason for hiding this comment

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

🤔 if there's no sysctl.get, what is called with __salt__['sysctl.get']?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The mock object that gets put there by patch.dict

Copy link
Contributor

Choose a reason for hiding this comment

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

Right, but normally it exists /somewhere/, right? Or the code wouldn't work when calling via the command line.

That's the function to autospec

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'm sure that it normally exists somewhere. But I can't figure out where. Does it have something to do with __virtualname__ and __virtual__()? Are modules like these not loaded during the state unit tests?

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah, that's where the confusion is 🙃

Normally __salt__ will contain a 'module.function' for everything in salt/modules/, but def __virtual__(): does allow you to change the name. This is useful for cases where you need different behavior based on operating system or even underlying library.

In /this/ case, there are several files in modules/*_sysctl.py that could apply. Fortunately they all have the same API, so any one of them could apply. I'm not sure if there's a better way for create_autospec, though 🤔

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 guessed it must be something like that. But in this case, there's literally nothing in sysctl.__salt__:

(Pdb) p sysctl.__salt__
{}

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah - that's because within the (unit) test suites we avoid loading all the things 🙃

elif name in configured and name in current:
if str(value).split() == __salt__["sysctl.get"](name).split():
elif name in configured and current != "":
if str(value).split() == current.split():
Copy link
Contributor

Choose a reason for hiding this comment

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

question: As I look at this if/elif chain would it be possible to rewrite this a bit clearer, perhaps by shuffling the order/indenting things?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok. I made the chain a little deeper but less long now.

salt/states/sysctl.py Outdated Show resolved Hide resolved
@asomers
Copy link
Contributor Author

asomers commented Dec 4, 2020

The test failures look unrelated.

@sagetherage sagetherage added this to the Aluminium milestone Dec 4, 2020
@asomers
Copy link
Contributor Author

asomers commented Dec 14, 2020

@waynew there's still one test failure, but it looks unrelated to this PR. What is the next step?

Copy link
Contributor

@waynew waynew left a comment

Choose a reason for hiding this comment

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

@asomers good question. Hopefully this will be fixed in the main branch - pinged the core team to ask. I'm approving it but we may need to rebase/merge master to get that test green (or re-run the suite if it's an infrastructure issue)

@asomers
Copy link
Contributor Author

asomers commented Dec 14, 2020

Thanks Wayne. Just ping me if I need to do anything.

@asomers
Copy link
Contributor Author

asomers commented Jan 25, 2021

@waynew @Ch3LL is there anything else we need to do there?

@Ch3LL
Copy link
Contributor

Ch3LL commented Feb 3, 2021

Should be good, just need to get the tests green. A lot of cleanup on pytest has occurred recently so i'll merge to master and should be good to go after that.

@Ch3LL Ch3LL merged commit 7542044 into saltstack:master Feb 3, 2021
@asomers asomers deleted the sysctl_on2 branch April 21, 2021 15:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Aluminium Release Post Mg and Pre Si
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants