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

Avoid sharing database name between testcase classes. #4285

Merged
merged 2 commits into from
Nov 6, 2017
Merged

Avoid sharing database name between testcase classes. #4285

merged 2 commits into from
Nov 6, 2017

Conversation

tseaver
Copy link
Contributor

@tseaver tseaver commented Oct 30, 2017

Abrupt shutdown (maybe CircleCI auto-cancellation?) can cause them not to be cleaned up.

@tseaver tseaver added api: spanner Issues related to the Spanner API. priority: p1 Important issue which blocks shipping the next release. Will be fixed prior to next release. testing labels Oct 30, 2017
@tseaver tseaver requested a review from dhermes October 30, 2017 22:39
@googlebot googlebot added the cla: yes This human has signed the Contributor License Agreement. label Oct 30, 2017
@dhermes
Copy link
Contributor

dhermes commented Oct 30, 2017

@tseaver These tests end up with a 409 Conflict. We'll pick this back up tomorrow?

@dhermes
Copy link
Contributor

dhermes commented Oct 31, 2017

Based on the success of

it's clear that

  • the failures were caused by a failed clean-up of old tests
  • this fix is still insufficient, since the 409 was caused when this build ran at the same time as another

@tseaver
Copy link
Contributor Author

tseaver commented Oct 31, 2017

The "obvious" fix would be to use a per-CI-run instance (using our normal unique_resource_id() helper, which is what we did prior to #3951. However, the Spanner folks throttle us at a maximum of 3 instances, and we have to keep one (gcp-streaming-systests) around, pre-populated. That means that even if we mangle the instance name, and never screw up the cleanup, we can have at most two CI runs happening at one time.

We could try mangling the database names, rather than the instance name: it is possible that we wouldn't hit a different quota limit for databases-within-an-instance.

@dhermes
Copy link
Contributor

dhermes commented Oct 31, 2017

@tseaver Could we check if it exists before using it in a class setup, then sleep (or fail the test) if it already exists?

@tseaver
Copy link
Contributor Author

tseaver commented Oct 31, 2017

@lukesneeringer Or we could ask again to get our quota raised: they acted like it was impossible back before I capitulated and put in #3951.

@tseaver
Copy link
Contributor Author

tseaver commented Oct 31, 2017

@dhermes We really don't want to share the database between test runs: they could step on each other in lots of ways beyond the creation bit.

@tseaver
Copy link
Contributor Author

tseaver commented Oct 31, 2017

@dhermes https://circleci.com/gh/GoogleCloudPlatform/google-cloud-python/3964 is the run which will actually exercise the system tests from 2a66746.

@dhermes
Copy link
Contributor

dhermes commented Oct 31, 2017

We really don't want to share the database between test runs: they could step on each other in lots of ways beyond the creation bit.

I wasn't suggesting we do. I was suggesting we either

  • wait until the class knows it has created the database
  • give up if the database already exists (would need to use unittest.TestCase.skipTest at run time vs. unittest.skipIf at import time)

@dhermes
Copy link
Contributor

dhermes commented Oct 31, 2017

@tseaver You should rebase this PR so that your "Declare target packages (changed packages and dependencies)." doesn't end up with packages you don't care about.

Abrupt shutdown (maybe CircleCI auto-cancellation?) can cause them not to be cleaned up.
@tseaver
Copy link
Contributor Author

tseaver commented Nov 1, 2017

I'm not sure I get why the rebase, but I've done it blindly.

@dhermes
Copy link
Contributor

dhermes commented Nov 1, 2017

@tseaver If you'd like to understand how "Declare Target Packages" works:

In particular, get_target_packages.py does a somewhat naive diff against master. So if your branch is behind master, you end up with a diff against HEAD vs. your merge base (this is actually the "right" idea). The files in the diff determine which packages get tested.

@tseaver
Copy link
Contributor Author

tseaver commented Nov 6, 2017

The Appveyor varioure is the same (bogus) "invalid version number" error. Any other reason not to merge this?

@dhermes
Copy link
Contributor

dhermes commented Nov 6, 2017

@tseaver If you rebase from master, it'll go away (I fixed in #4313).

@dhermes
Copy link
Contributor

dhermes commented Nov 6, 2017

But go ahead and merge, no need to re-run tests.

@tseaver tseaver merged commit 1844684 into googleapis:master Nov 6, 2017
@tseaver tseaver deleted the spanner-fix-system-tests-borked branch November 6, 2017 16:07
chemelnucfin pushed a commit to chemelnucfin/google-cloud-python that referenced this pull request Nov 6, 2017
Abrupt shutdown (maybe CircleCI auto-cancellation?) can cause them not to be cleaned up.
chemelnucfin pushed a commit to chemelnucfin/google-cloud-python that referenced this pull request Nov 6, 2017
Abrupt shutdown (maybe CircleCI auto-cancellation?) can cause them not to be cleaned up.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api: spanner Issues related to the Spanner API. cla: yes This human has signed the Contributor License Agreement. priority: p1 Important issue which blocks shipping the next release. Will be fixed prior to next release. testing
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants