chore: [WIP] experimental parallel kokoro system test#17608
chore: [WIP] experimental parallel kokoro system test#17608chalmerlowe wants to merge 12 commits into
Conversation
There was a problem hiding this comment.
Code Review
This pull request introduces parallel execution for system tests in .kokoro/system.sh with concurrency limits and log segregation, and isolates gcloud configurations per run using unique temporary directories. The feedback suggests robust cleanup of the temporary directories using shell traps to prevent leaks if commands fail under set -e, sanitizing package names used in directory creation, and refactoring the duplicate process reaping logic into a helper function to improve maintainability.
|
Applied Do Not Merge, simply because we will want to remove the throw-away comments just prior to merging. |
|
Something in this PR is off. Hold off on reviewing it. |
|
|
||
| printf '%s\n' "${PACKAGES_TO_TEST[@]}" | xargs -P "$MAX_JOBS" -I {} bash -c 'run_package_test "{}" > "$LOG_DIR/{}.log" 2>&1 || touch "$LOG_DIR/{}.failed"' | ||
|
|
||
| reap_parallel_results || RETVAL=1 |
There was a problem hiding this comment.
It looks like right now, the main cloud-devrel/client-libraries/python/googleapis/google-cloud-python/presubmit/system target always returns 0. Is that intended? The individual sub-targets are still reporting properly, but it would probably be better to have the main target fail if any others do
Maybe the exit ${RETVAL} line was removed by mistake?
There was a problem hiding this comment.
Fixed. We accidentally deleted it.
| local CLOUDSDK_CONFIG="${gcloud_config_dir}" | ||
|
|
||
| # 🪤 TRAP: Ensure cleanup of THIS specific temp dir on exit of this subshell | ||
| trap 'rm -rf "$gcloud_config_dir"' EXIT |
There was a problem hiding this comment.
Out of curiosity, is this required? I assumed the environment would just be destroyed after completion
There was a problem hiding this comment.
It may not be required, but felt like best practice: don't want space issues to bog things down mid test if we end up changing a lot of packages.
| export -f run_package_test | ||
| export system_test_script PROJECT_ROOT KOKORO_GFILE_DIR | ||
|
|
||
| printf '%s\n' "${PACKAGES_TO_TEST[@]}" | xargs -P "$MAX_JOBS" -I {} bash -c 'run_package_test "{}" > "$LOG_DIR/{}.log" 2>&1 || touch "$LOG_DIR/{}.failed"' |
There was a problem hiding this comment.
nit: This is a pretty dense line. A comment explaining that this triggers run_package_test for each package, and writes to LOG_DIR would be helpful
There was a problem hiding this comment.
Done: We broke the line into steps and added some comments here and elsewhere in the script.
| MAX_JOBS=${MAX_JOBS:-4} | ||
|
|
||
| # Temporary directory for clean log segregation | ||
| LOG_DIR=$(mktemp -d -t test-logs-XXXXXX) |
There was a problem hiding this comment.
IIRC, the expectation is to write individual package logs to ${KOKORO_ARTIFACTS_DIR}/${pkg}/sponge_log.log".
In theory, this will persist the logs as an artifact, which would let us view logs for each individual target under the "Target Log" tab.
This doesn't work properly in the default BTX view. But if you click through to Sponge, it renders nicely there (invocation details > Sponge URL)
I don't think this really matters, but consider saving individual logs to that artifact directory any way, just in case BTX starts working for us some day
There was a problem hiding this comment.
This is a good point, I was not sure why my logs weren't showing up where I expected them to be and this was a great question. Helped me a lot.
We added logic to write to KOKORO_ARTIFACTS_DIR by default and to fallback to a local dir if needed during a local test (I know we don't do many of those cause: "system tests", but it felt like good practice to have it).
|
This is blocked on: #17826 |
## The Problem Two unit tests in `google-cloud-bigquery` (`test_result_w_retry_wo_state` and `test_result_w_custom_retry`) were failing intermittently with a `TimeoutError` or `RetryError`. This flakiness occurs because the tests use a very short inner retry deadline of 0.1 seconds. Under heavy load (such as during parallel testing or on constrained continuous integration runners), this deadline can be exceeded, causing the test to fail confusingly. ## The Solution Increased the `deadline` parameter for the custom retry objects in both tests from 0.1 seconds to 1.0 seconds. This provides enough buffer for the test assertions to complete without timing out, aligning with other similar tests in the suite. ## Notes to Reviewers - This change only affects unit tests and does not alter production code. - This issue is blocking: - #17642 - #17608 Here is a snippet of the failure: ```python def test_result_w_retry_wo_state(global_time_lock): from google.cloud.bigquery.retry import DEFAULT_GET_JOB_TIMEOUT begun_job_resource = helpers._make_job_resource( job_id=JOB_ID, project_id=PROJECT, location="EU", started=True ) done_job_resource = helpers._make_job_resource( job_id=JOB_ID, project_id=PROJECT, location="EU", started=True, ended=True, ) conn = helpers.make_connection( exceptions.NotFound("not normally retriable"), begun_job_resource, exceptions.NotFound("not normally retriable"), done_job_resource, ) client = helpers._make_client(project=PROJECT, connection=conn) job = google.cloud.bigquery.job._AsyncJob( google.cloud.bigquery.job._JobReference(JOB_ID, PROJECT, "EU"), client ) custom_predicate = mock.Mock() custom_predicate.return_value = True custom_retry = google.api_core.retry.Retry( predicate=custom_predicate, initial=0.001, maximum=0.001, deadline=0.1, ) > assert job.result(retry=custom_retry) is job ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ tests/unit/job/test_async_job_retry.py:115: _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ google/cloud/bigquery/job/base.py:1047: in result return super(_AsyncJob, self).result(timeout=timeout, retry=retry) ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ .nox/unit-3-12-test_type-unit/lib/python3.12/site-packages/google/api_core/future/polling.py:256: in result self._blocking_poll(timeout=timeout, retry=retry, polling=polling) _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ self = _AsyncJob<project=test-project, location=EU, id=test-job-id> timeout = None retry = <google.api_core.retry.retry_unary.Retry object at 0x7ff2beda32c0> polling = <google.api_core.retry.retry_unary.Retry object at 0x7ff2beda01d0> def _blocking_poll(self, timeout=_DEFAULT_VALUE, retry=None, polling=None): """Poll and wait for the Future to be resolved.""" if self._result_set: return polling = polling or self._polling if timeout is not PollingFuture._DEFAULT_VALUE: polling = polling.with_timeout(timeout) try: polling(self._done_or_raise)(retry=retry) except exceptions.RetryError: > raise concurrent.futures.TimeoutError( f"Operation did not complete within the designated timeout of " f"{polling.timeout} seconds." ) E TimeoutError: Operation did not complete within the designated timeout of None seconds. ```
Warning
Something in this PR is off. Hold off on reviewing it.
Note
Results
Baseline: 2 hours 5 mins
With parallelization: 42 mins
This is blocked on: #17826