Repository navigation
Conversation
There was a problem hiding this comment.
Code Review
This pull request simplifies the development and testing environment by removing legacy Kokoro and Cloud Build configurations, updating dependencies to use local editable installs, and recommending uv in the documentation. Crucially, it enhances the integration test suite in tests/integration/conftest.py with a region fallback mechanism to handle 503 ServiceUnavailable stockouts and registers a SIGTERM handler for reliable session cleanup, backed by new unit tests. The review feedback suggests making the exception-traversing helper more robust against non-exception types and wrapping the signal registration in try-except blocks to prevent failures in multi-threaded test environments.
| def _is_service_unavailable(exc): | ||
| """Return True if exc or its chained cause/context is 503 ServiceUnavailable.""" | ||
| current = exc | ||
| visited = set() | ||
| while current is not None and id(current) not in visited: | ||
| visited.add(id(current)) | ||
| if isinstance(current, core_exceptions.ServiceUnavailable): | ||
| return True | ||
| current = current.__cause__ or current.__context__ | ||
| return False |
There was a problem hiding this comment.
To make _is_service_unavailable more robust and adhere to defensive programming practices, we should ensure that current is an instance of BaseException before attempting to access its __cause__ or __context__ attributes. If a user or library manually assigns a non-exception object to __cause__ or __context__, accessing these attributes on a non-exception type in subsequent iterations would raise an AttributeError. Checking isinstance(current, BaseException) prevents this potential crash.
| def _is_service_unavailable(exc): | |
| """Return True if exc or its chained cause/context is 503 ServiceUnavailable.""" | |
| current = exc | |
| visited = set() | |
| while current is not None and id(current) not in visited: | |
| visited.add(id(current)) | |
| if isinstance(current, core_exceptions.ServiceUnavailable): | |
| return True | |
| current = current.__cause__ or current.__context__ | |
| return False | |
| def _is_service_unavailable(exc): | |
| """Return True if exc or its chained cause/context is 503 ServiceUnavailable.""" | |
| current = exc | |
| visited = set() | |
| while isinstance(current, BaseException) and id(current) not in visited: | |
| visited.add(id(current)) | |
| if isinstance(current, core_exceptions.ServiceUnavailable): | |
| return True | |
| current = current.__cause__ or current.__context__ | |
| return False |
| previous_sigterm = signal.getsignal(signal.SIGTERM) | ||
|
|
||
| @pytest.fixture(scope="session") | ||
| def _handle_sigterm(signum, frame): | ||
| _terminate_active_session() | ||
| if callable(previous_sigterm): | ||
| previous_sigterm(signum, frame) | ||
| raise KeyboardInterrupt("Received SIGTERM during integration tests") | ||
|
|
||
| signal.signal(signal.SIGTERM, _handle_sigterm) | ||
| try: | ||
| yield os.environ | ||
| finally: | ||
| _terminate_active_session() | ||
| signal.signal(signal.SIGTERM, previous_sigterm) |
There was a problem hiding this comment.
In Python, signal.signal can only be called from the main thread of the process. If these integration tests are executed in a multi-threaded test runner or environment where this fixture runs on a background thread, calling signal.signal will raise a ValueError. Additionally, signal.getsignal can occasionally return None (e.g., if the handler was registered outside of Python), which would cause signal.signal to raise a TypeError during restoration. Wrapping these calls in try-except (ValueError, TypeError) blocks ensures the test suite remains robust and does not crash unexpectedly.
def _handle_sigterm(signum, frame):
_terminate_active_session()
if callable(previous_sigterm):
previous_sigterm(signum, frame)
raise KeyboardInterrupt("Received SIGTERM during integration tests")
previous_sigterm = None
try:
previous_sigterm = signal.getsignal(signal.SIGTERM)
signal.signal(signal.SIGTERM, _handle_sigterm)
except (ValueError, TypeError):
pass
try:
yield os.environ
finally:
_terminate_active_session()
if previous_sigterm is not None:
try:
signal.signal(signal.SIGTERM, previous_sigterm)
except (ValueError, TypeError):
pass
Summary
Modernizes the repository's CI workflows, integration test resiliency, packaging metadata, and developer documentation:
1. Integration Test Resiliency (
tests/integration/conftest.py)_create_session_with_region_fallback, which retries shared session creation acrossGOOGLE_CLOUD_FALLBACK_REGIONS(defaulting tous-central1,us-east1,us-west1whenMANAGED_SPARK_CONNECT_SUBNETis a short name and no custom regional endpoint is set) when Dataproc returns503 ServiceUnavailable(includingServiceUnavailablewrapped inRuntimeErrorbyManagedSparkSession.Builder). Updates_region_state["region"]so all downstream fixtures (test_region,session_name,session_controller_client,session_template_controller_client) stay pinned to the region that succeeded.SIGTERM/SIGINTsession cleanup: Registers aSIGTERMhandler andfinally:cleanup hooks calling an idempotent_terminate_active_session()helper (falling back toSessionControllerClient.terminate_sessionif a session is stillCREATINGwhen a workflow run is cancelled).tests/unit/test_integration_conftest.py): Covers candidate region selection, wrapped503 ServiceUnavailablefallback, non-503 error propagation, and idempotent in-flight session termination.2. GitHub Actions Hardening,
/gcbrunSupport & Dependabot (.github/)/gcbrun(.github/workflows/integration-tests.yaml):pushtomain,workflow_dispatch, and same-repopull_requestevents (excludingdependabot[bot])./gcbrun(or/gcbrun <sha>) comments on fork PRs: verifies collaboratoradmin/maintain/writepermissions and TOCTOU commit timestamps before checking out the PR head SHA, and reports pending/final commit statuses back to the PR head SHA.vars.*) & secret masking:vars.GCP_REGIONandvars.GCP_SUBNET(with fallback tosecrets.*), and masksproject_idandclient_emailextracted fromsecrets.GCP_SA_KEYvia::add-mask::.tests.yaml,pyink.yaml,lint-readme.yaml,integration-tests.yaml):permissions: contents: read,persist-credentials: falseonactions/checkout, job/workflowconcurrencygroups withcancel-in-progress, explicittimeout-minutes, and full 40-char commit SHA action pins (0findings underzizmor)..github/dependabot.yml): Adds weekly groupedgithub-actionsupdates.3. Legacy CI Removal & Packaging Modernization
.kokoro/,.trampolinerc,cloudbuild/, and.dockerignorefiles.[build-system]topyproject.toml, removes deprecated[bdist_wheel] universal = 1fromsetup.cfg, and updatesrequirements-dev.txt,DEVELOPING.md, and.env.example.