Skip to content

Fix too-many-open-files leak in availability worker and version lookup #21

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.

Sign up for GitHub

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

Merged
pdmars merged 2 commits into from
Aug 12, 2026
Merged

Fix too-many-open-files leak in availability worker and version lookup #21

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
16747dd
Fix too-many-open-files leak in availability worker and version lookup
pdmars Aug 12, 2026
daba085
Address PR feedback: isolate release cache from versioning cache; rem…
pdmars Aug 12, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 5 additions & 3 deletions reference_api/availability/worker.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,9 +33,12 @@ async def _site_loop(
site_timeout: float,
error_backoff: float,
) -> None:
client: BlazarClient | None = None
while True:
try:
await _sync_site(cache, site_id, cloud_name, site_timeout)
if client is None:
client = BlazarClient(cloud_name)
Comment on lines +39 to +40

@msherman64 msherman64 Aug 12, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

good fix, I think there's a longstanding footgun where openstack "connection" objects aren't garbage collected.

await _sync_site(cache, site_id, client, site_timeout)
await asyncio.sleep(poll_interval)
except Exception: # pylint: disable=broad-exception-caught
LOG.exception("Availability sync failed for site %s, backing off", site_id)
Expand All @@ -45,14 +48,13 @@ async def _site_loop(
async def _sync_site(
cache: AvailabilityCache,
site_id: str,
cloud_name: str,
client: BlazarClient,
site_timeout: float,
) -> None:
LOG.info("Starting availability sync for site %s", site_id)
loop = asyncio.get_running_loop()

def _fetch():
client = BlazarClient(cloud_name)
return client.list_host_allocations()

nodes, known_uuids, unavailable_uuids = await asyncio.wait_for(
Expand Down
5 changes: 3 additions & 2 deletions reference_api/main.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -157,8 +157,9 @@ def _warmup_flavor_clients(ref_dir: Path) -> None:
def _fetch_flavor_availability(
cloud_name: str, flavor_id: str, start_date: datetime, end_date: datetime
) -> list[dict]:
client = _site_clients.get(cloud_name) or BlazarClient(cloud_name)
return client.get_flavor_availability(flavor_id, start_date, end_date)
if cloud_name not in _site_clients:
_site_clients[cloud_name] = BlazarClient(cloud_name)
return _site_clients[cloud_name].get_flavor_availability(flavor_id, start_date, end_date)


@lru_cache(maxsize=1)
Expand Down
2 changes: 1 addition & 1 deletion reference_api/services/utils.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -209,4 +209,4 @@ def build_paginated_response(

def get_version(repo_root: Path) -> Optional[str]:
"""Gets the repository version for a given repo_root."""
return filesystem.get_version(repo_root)
return filesystem.get_release_and_timestamp(repo_root).get("version")

@msherman64 msherman64 Aug 12, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I'm not sure exactly what this change does?

In the old path, it calls git_versioning.get_version(), which creates a new Repo() object on each call

and in the new path, it's calling get_release_and_timestamp, which also creates a new Repo on each call, but has a @cached decorator, so I guess it should reuse that cached object.

is that the only difference?

@msherman64 msherman64 Aug 12, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

it seems like nothing else calls git_versioning.get_version() after this, so we can probably just remove that method too.

@msherman64 msherman64 Aug 12, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

it looks like the other methods (list_versions, get_version_info) also use the same cache, but could be distinct per site/node, and fill the cache up?

we might want to use a different cache for the get_release_and_timestamp because it depends only on the repo_path.

@pdmars pdmars Aug 12, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Good points.

  1. yes main reason was to use the cache
  2. good catch, removed
  3. good point, moved it to its own cache

4 changes: 0 additions & 4 deletions reference_api/storage/filesystem.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -133,10 +133,6 @@ def read_flavor(ref_dir: Path, site_id: str, flavor_id: str) -> Optional[Dict]:
return _read_json(ref_dir / f"sites/{site_id}/flavors/{flavor_id}.json")


def get_version(repo_path: Path) -> Optional[str]:
return git_versioning.get_version(repo_path)


def list_versions(
repo_path: Path, dir_path: Optional[Path] = None
) -> List[Dict]:
Expand Down
9 changes: 2 additions & 7 deletions reference_api/storage/git_versioning.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,12 +10,7 @@


git_cache: LRUCache = LRUCache(maxsize=1024)


def get_version(repo_path: Path) -> Optional[str]:
"""Return the git HEAD sha for the provided repo_path"""
repo = Repo(repo_path, search_parent_directories=True)
return repo.head.commit.hexsha
_release_cache: LRUCache = LRUCache(maxsize=8)


def _get_relative_dir_path(repo_root: Path, dir_path: Path) -> Optional[str]:
Expand Down Expand Up @@ -132,7 +127,7 @@ def get_version_info(
return None


@cached(git_cache)
@cached(_release_cache)
def get_release_and_timestamp(repo_path: Path) -> Dict[str, Optional[str]]:
"""Get the current release (HEAD sha) and timestamp for the repo."""
result: Dict[str, Optional[str]] = {"version": None, "timestamp": None}
Expand Down
6 changes: 0 additions & 6 deletions tests/test_storage_filesystem.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,12 +33,6 @@ def test_read_cluster(mock_ref_dir):
assert cluster.get("uid") == "chameleon"


def test_version_helpers(mock_ref_dir):
v = filesystem.get_version(mock_ref_dir)
# may be None in test env but should not raise
assert True


def test_list_nodes(mock_ref_dir):
nodes = filesystem.list_nodes(mock_ref_dir, "uc", "chameleon")
assert nodes and len(nodes) == 2
Expand Down
Loading