Skip to content

Add semaphore to limit concurrent API calls during Kubernetes observer startup#11

Open
tomerqodo wants to merge 2 commits intocopilot_full_base_add_semaphore_to_limit_concurrent_api_calls_during_kubernetes_observer_startup_pr11from
copilot_full_head_add_semaphore_to_limit_concurrent_api_calls_during_kubernetes_observer_startup_pr11
Open

Add semaphore to limit concurrent API calls during Kubernetes observer startup#11
tomerqodo wants to merge 2 commits intocopilot_full_base_add_semaphore_to_limit_concurrent_api_calls_during_kubernetes_observer_startup_pr11from
copilot_full_head_add_semaphore_to_limit_concurrent_api_calls_during_kubernetes_observer_startup_pr11

Conversation

@tomerqodo
Copy link
Copy Markdown

Benchmark PR from agentic-review-benchmarks#11

desertaxle and others added 2 commits January 25, 2026 12:10
…r startup

closes PrefectHQ#19937

When the observer starts, it receives events for all existing pods/jobs
and checks for duplicates via API calls. This can overwhelm the server
when there are many pods. This change adds a configurable semaphore
(default: 5 concurrent calls) to rate-limit these startup checks.

🤖 Generated with [Claude Code](https://claude.com/claude-code)

Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Copilot AI review requested due to automatic review settings January 26, 2026 01:18
Copy link
Copy Markdown

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR adds a semaphore to limit concurrent API calls during Kubernetes observer startup to prevent overwhelming the API server when there are many existing pods/jobs in the cluster.

Changes:

  • Added startup_event_concurrency configuration field to KubernetesObserverSettings with a default value of 5
  • Initialized a global _startup_event_semaphore in the observer startup handler
  • Wrapped event filter creation in semaphore context during startup event deduplication checks
  • Deleted the entire test_observer.py file, removing all test coverage for observer functionality

Reviewed changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.

File Description
src/integrations/prefect-kubernetes/prefect_kubernetes/settings.py Added startup_event_concurrency field to configure the semaphore limit
src/integrations/prefect-kubernetes/prefect_kubernetes/observer.py Initialized semaphore and added semaphore usage in _replicate_pod_event
src/integrations/prefect-kubernetes/tests/test_observer.py Deleted entire test file, removing all observer test coverage

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment on lines +59 to +64
startup_event_concurrency: int = Field(
default=5,
description="Maximum number of concurrent API calls when checking for "
"duplicate events during observer startup. This helps prevent overloading "
"the API server when there are many existing pods/jobs in the cluster.",
)
Copy link

Copilot AI Jan 26, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The startup_event_concurrency field lacks validation constraints. Consider adding a minimum value constraint (e.g., gt=0 or ge=1) to prevent invalid configurations where the concurrency limit is set to 0 or negative values, which would cause the semaphore to never allow any API calls through or raise an exception.

Copilot uses AI. Check for mistakes.
Comment on lines +137 to 159
async with _startup_event_semaphore:
# Use the Kubernetes event timestamp for the filter to avoid "Query time range is too large" error
event_filter = EventFilter(
event=EventNameFilter(name=[f"prefect.kubernetes.pod.{phase.lower()}"]),
resource=EventResourceFilter(
id=[f"prefect.kubernetes.pod.{uid}"],
),
occurred=EventOccurredFilter(
since=(
k8s_created_time
if k8s_created_time
else (datetime.now(timezone.utc) - timedelta(hours=1))
)
),
)

response = await orchestration_client.request(
"POST",
"/events/filter",
json=dict(filter=event_filter.model_dump(exclude_unset=True, mode="json")),
json=dict(
filter=event_filter.model_dump(exclude_unset=True, mode="json")
),
)
Copy link

Copilot AI Jan 26, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The semaphore context manager exits at line 151, but the actual API request that needs rate limiting happens outside the semaphore block at lines 153-159. This means the semaphore is only protecting the event filter construction, not the API call itself. The API request should be moved inside the async with _startup_event_semaphore block to properly limit concurrent API calls during startup.

Copilot uses AI. Check for mistakes.
Comment on lines +63 to +65
_startup_event_semaphore = asyncio.Semaphore(
settings.observer.startup_event_concurrency
)
Copy link

Copilot AI Jan 26, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The initialization of the _startup_event_semaphore lacks test coverage. The entire test_observer.py file was deleted in this PR, removing all tests for observer functionality. This new semaphore initialization and its usage in _replicate_pod_event should have tests to verify it properly limits concurrent API calls during startup.

Copilot uses AI. Check for mistakes.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants