Skip to content

fix(perf): Avoid server-side tag filtering in listEC2Runners DescribeInstances call - #5328

Open
wadherv wants to merge 2 commits into
github-aws-runners:mainfrom
wadherv:issue_5327
Open

fix(perf): Avoid server-side tag filtering in listEC2Runners DescribeInstances call#5328
wadherv wants to merge 2 commits into
github-aws-runners:mainfrom
wadherv:issue_5327

Conversation

@wadherv

@wadherv wadherv commented Sep 2, 2026

Copy link
Copy Markdown
Contributor

Description

The Scale-Up Lambda's runner-lookup call (listEC2Runners in lambdas/libs/compute-providers/aws/ec2/src/control-plane/runners.ts) dominated total invocation latency. X-Ray traces showed total invocation duration consistently ~35s, while the actual provisioning logic (credential lookups, CreateFleet) was sub-2s — DescribeInstances alone accounted for ~93% of the invocation (~24–33s), reproducing consistently across 10+ sampled traces over a 24-hour window.

Root-cause investigation ruled out the usual suspects:

  • Not pagination — result sets are in the hundreds, well under AWS's ~1,000-per-page default.
  • Not Lambda networking (VPC/ENI) — the same delay reproduced identically calling the same API locally, outside the Lambda's VPC, via both boto3 and the AWS CLI.
  • Not match count — a query with fewer filters but fewer matches still ran 5x faster.
  • Each additional tag:ghr:* filter (environment, Type, Owner, Application, optionally orphan) pays the same tag-index lookup cost server-side; the current implementation stacks up to four.
  • Tried two AWS APIs purpose-built for tag lookups (DescribeTags, Resource Groups Tagging API) — both performed the same or worse. This confirmed the fix isn't "use a different tag-lookup API," it's "avoid server-side tag filtering in this call entirely."

This PR changes listEC2Runners to fetch by instance-state-name only (a fast, native attribute) and filter by tag in application code afterward, instead of asking EC2 to filter by tags server-side:

// Before: EC2 filters by instance-state-name AND up to 4 tag:ghr:* filters
const ec2Filters = constructFilters(filters);
const instances = await ec2.send(new DescribeInstancesCommand({ Filters: ec2Filters }));

// After: EC2 filters by instance-state-name only; tags matched client-side
const stateFilter = [{ Name: 'instance-state-name', Values: ec2Statuses }];
const tagFilters = constructTagFilters(filters);
const instances = await ec2.send(new DescribeInstancesCommand({ Filters: stateFilter }));
const matched = filterInstancesByTags(instances, tagFilters);

Since Instance.Tags is already returned on every instance in the DescribeInstances response, no extra API calls are needed — the tag data is just filtered in-process instead of server-side. This is an internal implementation change only: listEC2Runners's signature and the Ec2ListRunnerFilters input shape are unchanged, so no changes were needed in any of its callers (pool.ts, scale-up.ts, scale-down.ts).

Measured locally with real data: state-only fetch took 6.4s for ~950 instances, with client-side tag filtering afterward costing under 2ms — a ~5x improvement over the ~24–33s baseline.

Test Plan

  • runners.test.ts updated: filter-assertion tests now confirm only instance-state-name is sent to DescribeInstancesCommand, and new tests verify correct client-side inclusion/exclusion for environment, Type+Owner, orphan, and Application tag combinations (including instances missing the Application tag being correctly excluded).
  • vitest run on runners.test.ts: 71/71 passing.
  • Full compute-providers package test suite: 10 files / 272 tests passing (no ripple effects in pool.ts, scale-up.ts, scale-down.ts or their tests).
  • functions/control-plane/src/pool/pool.test.ts: 20/20 passing.
  • tsc --noEmit and eslint clean on both changed files.
  • Confirmed no docs//README.md content describes this internal filtering behavior, so no doc updates needed.

Related Issues

#5327

@wadherv
wadherv requested a review from a team as a code owner September 2, 2026 05:29
@edersonbrilhante

Copy link
Copy Markdown
Contributor

There is a plan to use dynamodb for caching the instance Metadata. My concern with your change is for aws regions with multiple installations, runner types and heavy usage.

@wadherv

wadherv commented Sep 2, 2026

Copy link
Copy Markdown
Contributor Author

@edersonbrilhante Thanks for the feedback. I understand there are plans to introduce DynamoDB-based caching, although I'm not sure when that work will be available.
Could you elaborate on the concerns around regions with multiple installations, runner types, and heavy usage?
We're currently running 8-9 runner labels with approximately 800-900 active runners on average, and one of the motivations behind this change is the cost and latency associated with repeated DescribeInstances API calls at that scale. The current provisioning lifecycle is slow enough that we have to rely on over-provisioning via pool lambdas to ensure runners are available when users need them. Reducing the dependency on frequent DescribeInstances calls helps improve provisioning latency and reduce the amount of excess capacity we need to maintain. If there are specific scenarios where you believe this approach could cause issues, I'd be happy to understand them better and make adjustments if needed.

@wadherv

wadherv commented Sep 2, 2026

Copy link
Copy Markdown
Contributor Author

Another observation from our production environment is that the current implementation does not scale linearly. As the number of runners grows, DescribeInstances latency increases and provisioning performance degrades accordingly. Smaller deployments are less impacted because the API responses are significantly faster at lower instance counts.

@edersonbrilhante

Copy link
Copy Markdown
Contributor

Thanks for giving the environment context.

There is this opened PR #5281 to use dynamodb as caching strategy

And I am working in some drafts like this one to allow extract the ssm and ec2 tags filter approach to use storage plugin that would allow expands the scalabity issues(ssm and ec2 api limits)

I am working in different PRs to allow multiple providers:

  • computer provider: ec2, microvm, etc
  • orchestration provider: webhook(scale up,scale down,pool), scaleset(using GitHub Actions Runner Scale Set APIs)
  • storage provider(ssm, dynamodb, etc):
    • config
    • runner metadata
    • runner registration
    • caching orchestration

About "when", I hope soon. in my lab I already made work runner backed in microvm and and the orchestration of ec2 via scaleset. I paused because I am waiting for reviews in a stacked PR. I plan to resume this week.

I am waiting this PR get merged #5312, So I can resume the work in the providers.

I started a discussion in #5316, I would appreciate feedbacks

@edersonbrilhante

Copy link
Copy Markdown
Contributor

@wadherv Update:

Once the PRs in stacked PR #5367 are reviewed and merged. I will be able to request review in microvm and scaleset feature.

To support microvm I needed to decouple the ssm from scale/up and pool, moving it to a storage provider with a contract interface. By doing it, now we have space to add dynamodb as alternative to cache/config/metadata.

Hope soon we can fix this issue with aws api :)

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.

2 participants