Create a subunit2sql based sql repository type #16
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This commit adds a new repository type for using subunit2sql. It performs the basic operations, but with a few limitations: 1.There is no attachment support in either directions (into the db or out of the db). 2. There is no sanity check on whether the db is created or not. This means that if stestr init has not been run for the provided repo url it'll fail in an unexpected way when it tries to use the repository. 3. There is no count() function defined for the repository, meaning the stestr stats command will not work. This is due to a limitation in the subunit2sql api as of v1.8.0 where there is no exposed API function to get the number of runs. 4. When writing to the repository the entire contents are stored in memory until the run is complete. This means 2 things stestr will have at least 2 complete copies of the stream in memory while processing it. Also if there is a power failure, hard shutoff, or the process is killed while running tests there will be no partial stream stored in the repository. But, other than those issues it works fine. This is just a starting point and won't be enabled by default for some time.
This commit adds a new api to get a repository and corresponding CLI options to set the repository type and the repository url.
The sql repository type is still experimental. Until it's stable enough for production use we should print a warning when creating a sql repo type.
When the sql repository type is used the stestr last command would encounter an issue trying to calculate the id for the previous run. The file repository type used an interger so to get the previous run id it would just subtract 1. However the sql repository type uses a uuid so this does not work. This commit works around this by not using the previous run data for now. Eventually we'll add a function to the repository abstract class to get the previous run id.
This commit adds a release note for adding a sql repository type and cli support for switching repositories.
This commit adds documentation for using different repository types and also api documentation for the sql repository type.
This commit updates the default url for the sql repository type to be a bit more sane.
This commit fixes a small typo in the sql repository class in the get_subunit_stream() function when passing in a list of test_runs. This was being hit in the stestr failing path.
|
@masayukig any thoughts on this? It's still pretty rough I think, but it's getting closer. Feel free to add on to it as necessary. |
This commit adds unit tests for verifying the functionality of the stestr.repository.util function to get the default repo urls. In the process it also fixes a bug where the function would fail in an unexpected manner when an invalid repo_type was returned.
This commit add .stestr.sqlite file to .gitignore. We should not track this file.
This commit fixes temp_dir path in TestUtil. In some environments, its temporally directory is in a symbolic directory. Therefore, sometimes the directory path is not same as a result of os.get_cwd(). In macOS at least, I faced this issue like this: -- Directory -- $ ls -ld /var lrwxr-xr-x 1 root wheel 11 9 24 07:50 /var -> private/var/ -- Test Error -- testtools.matchers._impl.MismatchError: !=: reference = '/var/folders/q4/xxxxx/T/tmpljfBqV' actual = '/private/var/folders/q4/xxxxx/T/tmpljfBqV'
|
I'm actually facing a weird thing like below, the test was passed, but something weird strings were shown. This only occurs from second runs. I never see this at a first run. Any thoughts? |
|
Hmm, I guess I never ran it more than once at a time, or I just didn't notice. :) That's a binary subunit v2 stream, my guess is it's leaking to stdout from somewhere in the new code path. Although, I'm not sure from where. |
|
Thanks, hmm, I'll dig the code, anyway :) |
mtreinish
commented
Jan 6, 2017
| # NOTE(mtreinish): test_run_metadata is not guaranteed to be | ||
| # present for the test run | ||
| metadata = test.get('metadata', None) | ||
| write_subunit.write_test(output, test['start_time'], |
There was a problem hiding this comment.
I suspect this is where the subunit stream is leaking to stdout, or something related to this call. From my quick scan, which is difficult to do from my phone (but so much easier than on gerrit) this gets called from get_test via inside run.py
But I'm not really sure, it's just a shot in the dark really. I'll take a more detailed look tomorrow when I'm in front of my computer again.
There was a problem hiding this comment.
4af17b6
should fix it. I was on the right track last night, but it was actually the get_test() itself at fault, not the subunit generation happening here.
Fix subunit2sql
The sql repository type requires subunit2sql to work, so we should ensure that we install it.
Previously the get_test() method in the _Subunit2SqlRun class was based heavily on the version in the file repository type. However the mistake there was while get_subunit_stream() returned a v2 stream, get_test was assuming a v1 stream (which is why the get_subunit_stream() call is commented out there) However in the sql repository type only v2 is ever used. So using the calls assuming a v1 stream caused the v2 stream generated by _Subunit2SqlRun.get_subunit_stream() to just passthrough the subunit library calls() to stdout. (because that's the default behavior in subunit) This commit fixes this issue by using ByteStreamToStreamResult(), which is the correct class, to convert the subunit v2 stream into a Result object. This fixes both the subunit v2 leaking to stdout as well as any of the operations which involved using historical data from the repository.
|
Before we merge this I'd like to try and start fixing (even if they're in progress) as many of the limitations outlined in: 2dd3341 as we can. So to start I've added a count api to subunit2sql in https://review.openstack.org/#/c/417519/ to address limitation number 3. Once that subunit2sql patch is included in a release we can easily update count() in the stestr.repository.sql.Repository class to do the right thing |
This commit adds a check to the sql repository type's RepositoryFactory. The check just runs the get_ids_for_all_tests() subunit2sql db api function. This function was chosen because it is read only and will not raise an exception in the presence of an empty database. (but it still requires the schema) If the function does not raise an operation error from sqlalchemy that means stestr was able to communicate to the database and the subunit2sql was present. But, if an operation error is raised that means either stestr can't connect to the DB or the schema isn't present (or up to date). This should be sufficient for the check during the RepositoryFactory's open() function.
|
So looking at the outstanding todos from the first commit. I think we're good to merge this after we fix the partial stream story, issue # 4 from the first commit msg. The attachments are going to be tricky and can be a goal for removing the experimental flag (the subunit2sql is likely broken there) |
The subunit2sql code being used to populate the tests table (which is where the running mean of test times is queried from) strip the attrs from the test_ids. However the lookup side in the Repository code was still using a test_ids with attrs. This commit fixes this so lookups will actually match.
Previously the sql repository type would wait for the subunit stream to complete before it would start writing the subunit2sql DB. This would potentially cause an issue if the stream was aborted for any reason during the run there would be no partial data stored anywhere. This commit solves this by writing data to the DB as it comes in over the subunit. An empty run is created at startTestRun in the result stream, each test_run is added to the DB as it's finished, and the run_time is added to the run at the end during stopTestRun. This solves the issue, the only cost is there is more DB activity required to accomplish this. Since the run also needs to be updated for each test (to keep the counters up to date).
|
The last patch added real time db updates, I think this should be good enough to merge now, let me know what you think. |
|
yeah, agree, I also think it's good to merge :) |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This introduces a new repository type sql, based on subunit2sql. Long term this will be the default, but for right now it's still under development and should be considered experimental. This also fixes #10 and #11