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

Add trace logging and optimize file handling in grains.core.os_data #48166

Merged
merged 56 commits into from Jun 21, 2018

Conversation

Projects
None yet
3 participants
@terminalmage
Member

terminalmage commented Jun 17, 2018

This is a continuation of the troubleshooting being done for saltstack/salt-jenkins#1000. I've narrowed the timeout to the os_data function in the core grains. To that end, this PR does the following:

  1. Adds the timestamp to log_fmt_console in the integration suite's minion config file.
  2. Adds trace logging throughout the os_data function.

In the process of adding the trace logging, I discovered a couple bugs in my recent changes to mock_open, so I resolved those. I also found some optimizations that could be made in os_data, such as removing os.path.isfile() checks before attempting to read from a file (when we can simply catch the IOError). Removing the os.path.isfile() checks exposed some shortcomings of mock_open, most importantly that the same data is returned for all filehandles opened while the patch is in effect. To resolve this, I rewrote mock_open to support distinct filehandles per call to salt.utils.files.fopen().

Additionally, I've added a bunch of unit tests to confirm that mock_open behaves as closely as possible to a real filehandle. One of the problems uncovered by my recent changes to mock_open has exposed the fact that a number of tests which use mock_open were written to conform to the limitations of mock_open, and thus those tests were not as accurate as they should have been. Having unit tests to confirm the behavior of mock_open should help ensure that tests which rely on it are more accurate.

@terminalmage

This comment has been minimized.

Member

terminalmage commented Jun 17, 2018

Some tests need updating to work with the new MockFH class I wrote. Also, a couple lint issues. Working on them now.

terminalmage added some commits Jun 14, 2018

Add timestamp to the minion's log_fmt_console
This makes salt-call show timestamps in stdout/stderr when run from
integration tests (e.g. in ShellCase.run_script).
Add a bunch of logging for linux os_data core grains
Also removed a couple isfile checks and replaced them with try/except
clauses. EAFP.
More mock_open bugfixes
This makes readline() a regular function and not an iterator. It also
makes the different side_effect functions all operate on the same
generator representing the file contents, and adds support for reading
an explicit amount of bytes from the file. The side effect functions can
now be run one after another and the mocked filehandle will work
properly.
Move lsb_release parsing into its own function
This makes mocking easier in the unit tests, and follows the convention
used for parsing the os-release file.
Separate mocked file contents per filename, not glob
This allows for multiple opens of a file matching the same glob.
Otherwise, subsequent opens would already be at EOF.
Use function for empty string
Missed this when making changes for multi-file support
mock_open: rewrite multi-file support
The initial approach tied all opened files to the same mocked
filehandle. This new approach moves all the read functions into a class,
and then uses a separate instance of that class for each opened file.
add a dict containing the handles to the mock_open return object
This allows tests to access the MockFH objects opened during the
execution of the unit test.
Make read funcs mocks so their calls can be tracked
Also add a property func as a shorthand to return all the write calls
for a given filehandle.
Use explicit config file and fix remaining mac_sysctl tests
This updates test_persist_success to work with the new mocked filehandle
support. Additionally, it fixes incorrect invocation of
`mac_sysctl.persist()`; the `config` argument is supposed to be a file
path, not a string config text.

terminalmage added some commits Jun 18, 2018

@terminalmage terminalmage force-pushed the terminalmage:salt-jenkins-1000 branch from a290f2f to 6acb4c8 Jun 18, 2018

@terminalmage

This comment has been minimized.

Member

terminalmage commented Jun 18, 2018

The failing unit tests from before should pass now. Might have a few lint failures though.

@rallytime

This comment has been minimized.

Contributor

rallytime commented Jun 18, 2018

@terminalmage Looks like there are still some unittests failing with this change, in addition to the lint failure.

terminalmage added some commits Jun 18, 2018

@terminalmage

This comment has been minimized.

Member

terminalmage commented Jun 18, 2018

Yes, those were all from newly-added tests that came in when I rebased. They should be fixed now as well.

I still need to add some more unit tests for the new mock_open, and write some documentation

terminalmage added some commits Jun 18, 2018

Operate on a copy of the read_data
This keeps us from mangling it if it contains lists, which we would be
popping items off of. We don't want to pop items from the list that was
passed in.
@rallytime

This comment has been minimized.

Contributor

rallytime commented Jun 19, 2018

@terminalmage There's one more unittest failure that is related to this:

https://jenkins.saltstack.com/job/PR/job/salt-pr-linode-cent7-py3/5836/

@rallytime

This LGTM, except for that last new test failure.

@rallytime rallytime requested a review from gtmanfred Jun 19, 2018

@terminalmage

This comment has been minimized.

Member

terminalmage commented Jun 21, 2018

re-run py3

@rallytime rallytime merged commit 51928ff into saltstack:2018.3 Jun 21, 2018

6 of 10 checks passed

continuous-integration/jenkins/pr-merge This commit cannot be built
Details
jenkins/PR/salt-pr-rs-cent7-n Pull Requests » Salt PR - RS CentOS 7 #19890 — ABORTED
Details
jenkins/PR/salt-pr-linode-ubuntu16-py3 Pull Requests » Salt PR - Linode Ubuntu16.04 - PY3 #10888 — FAILURE
Details
default Build started sha1 is merged.
Details
WIP ready for review
Details
jenkins/PR/salt-pr-clone Pull Requests » Salt PR - Clone #26120 — SUCCESS
Details
jenkins/PR/salt-pr-docs-n Pull Requests » Salt PR - Docs #18094 — SUCCESS
Details
jenkins/PR/salt-pr-linode-cent7-py3 Pull Requests » Salt PR - Linode CentOS 7 - PY3 #5917 — SUCCESS
Details
jenkins/PR/salt-pr-linode-ubuntu14-n Pull Requests » Salt PR - Linode Ubuntu14.04 #23766 — SUCCESS
Details
jenkins/PR/salt-pr-lint-n Pull Requests » Salt PR - Code Lint #22729 — SUCCESS
Details

@terminalmage terminalmage deleted the terminalmage:salt-jenkins-1000 branch Jun 25, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment