From e5637f6092e0655fed05baadb78ce1e5eaf98a9d Mon Sep 17 00:00:00 2001 From: Nuwan Goonasekera <2070605+nuwang@users.noreply.github.com> Date: Fri, 31 Jul 2026 19:44:14 +0530 Subject: [PATCH] Cut AWS integration suite runtime and stop credentials expiring mid-run The AWS cloud integration job took ~59 minutes and started failing its last tests with RequestExpired. The OIDC session defaults to one hour and the credentials reach tox as static environment variables, so botocore cannot refresh them mid-run. Cleanup handlers need working credentials too, so the failing tests also leaked the instances and images they had created. Request a 3 hour OIDC session instead of the 1 hour default. This requires the IAM role's MaxSessionDuration to permit the longer session. Memoise the AWS VM type catalogue per availability zone. EC2 has no server-side paging for instance types, so list() materialises the whole catalogue and pages it client-side, and it previously refetched that catalogue on every call -- one DescribeInstanceTypeOfferings walk plus a DescribeInstanceTypes call per 100 types. That made walking a full listing quadratic in API calls: the 1343 types offered in us-east-1a, paged at the tests' result limit of 5, cost ~4300 calls where 14 suffice. This was the bulk of the runtime; test_vm_types_standard alone took ~55 minutes. Poll Route53 record changes every 5s rather than boto3's 30s default, while keeping the same ~30 minute ceiling. Measured against Route53, changes reached INSYNC inside the first poll interval every time, so a test making four record changes spent 120s asleep for work that finished in seconds. --- .github/workflows/integration-cloud.yaml | 6 + CHANGELOG.rst | 31 +++++ cloudbridge/providers/aws/services.py | 59 ++++++-- tests/test_aws_dns_waiters.py | 141 +++++++++++++++++++ tests/test_aws_vm_types.py | 166 +++++++++++++++++++++++ 5 files changed, 393 insertions(+), 10 deletions(-) create mode 100644 tests/test_aws_dns_waiters.py create mode 100644 tests/test_aws_vm_types.py diff --git a/.github/workflows/integration-cloud.yaml b/.github/workflows/integration-cloud.yaml index dda14f14..454cd362 100644 --- a/.github/workflows/integration-cloud.yaml +++ b/.github/workflows/integration-cloud.yaml @@ -94,6 +94,12 @@ jobs: with: role-to-assume: ${{ secrets.AWS_OIDC_ROLE_ARN }} aws-region: us-east-1 + # The action exports static session credentials that tox inherits as + # env vars — botocore cannot refresh them mid-run. The default 1h + # session expired partway through the ~59min AWS suite, failing the + # last tests with `RequestExpired`. 3h leaves headroom as the suite + # grows. Requires the IAM role's MaxSessionDuration to be >= this. + role-duration-seconds: 10800 - name: Run tox id: tox diff --git a/CHANGELOG.rst b/CHANGELOG.rst index 717eeaea..ec191253 100644 --- a/CHANGELOG.rst +++ b/CHANGELOG.rst @@ -1,3 +1,34 @@ +4.3.1 - unreleased +------------------ + +## Fixes +* **AWS VM type listings no longer refetch the whole catalogue for every + page.** EC2 offers no server-side paging for instance types, so + ``AWSVMTypeService.list`` materialises the full catalogue and pages it + client-side. It previously refetched that catalogue on every call - one + ``DescribeInstanceTypeOfferings`` walk plus a ``DescribeInstanceTypes`` call + per 100 types, about 14 API calls - which made walking the pages of a full + listing quadratic in API calls. Walking all 1343 types offered in + ``us-east-1a`` at a result limit of 5 cost roughly 4300 API calls; it now + costs 14. The catalogue is memoised per availability zone for the lifetime + of the provider. +* **AWS DNS record changes no longer wait a full 30 seconds each.** Creating + or deleting a record blocks until Route53 reports the change INSYNC, using + boto3's ``resource_record_sets_changed`` waiter. That waiter polls every 30 + seconds by default, so a change that propagated in a few seconds still cost + a full 30. Measured against Route53, INSYNC was reached inside the first + poll interval every time, making the granularity the entire cost. The + waiter now polls every 5 seconds while keeping the same ~30 minute ceiling. + +## Build and CI +* The AWS cloud integration job now requests a 3 hour OIDC session instead of + relying on the 1 hour default. The credentials are exported to tox as static + environment variables and cannot be refreshed mid-run, so a suite that ran + past the hour failed its remaining tests with ``RequestExpired`` - and, + because cleanup handlers need working credentials too, leaked the instances + and images those tests had created. Requires the IAM role's + ``MaxSessionDuration`` to permit the longer session. + 4.3.0 - July 11, 2026 (sha 863d0c8297e74e62a72b98643952f9a923807b7b) -------------------------------------------------------------------- diff --git a/cloudbridge/providers/aws/services.py b/cloudbridge/providers/aws/services.py index 53fd9e34..337b3d07 100644 --- a/cloudbridge/providers/aws/services.py +++ b/cloudbridge/providers/aws/services.py @@ -1023,6 +1023,9 @@ class AWSVMTypeService(BaseVMTypeService): def __init__(self, provider: CloudProvider) -> None: super(AWSVMTypeService, self).__init__(provider) + # Raw instance type dicts, keyed by availability zone. See + # _get_catalogue for why this is memoised. + self._catalogue: dict[str | None, list[dict[str, Any]]] = {} @dispatch(event="provider.compute.vm_types.get", priority=BaseVMTypeService.STANDARD_EVENT_PRIORITY) @@ -1039,15 +1042,12 @@ def get(self, vm_type: str) -> VMType | None: else: raise e - @dispatch(event="provider.compute.vm_types.list", - priority=BaseVMTypeService.STANDARD_EVENT_PRIORITY) - def list(self, limit: int | None = None, - marker: str | None = None) -> ResultList[VMType]: + def _fetch_catalogue(self, zone: str | None) -> list[dict[str, Any]]: client = cast("AWSCloudProvider", self.provider).ec2_conn.meta.client vmt_list_resp = client.describe_instance_type_offerings( LocationType='availability-zone', Filters=[{'Name': 'location', - 'Values': [self.provider.zone_name]}], + 'Values': [zone]}], # MaxResults is set to max value (1000) # and client-side pagination is used **trim_empty_params({'MaxResults': 1000, 'NextToken': None})) @@ -1056,7 +1056,7 @@ def list(self, limit: int | None = None, vmt_list_resp = client.describe_instance_type_offerings( LocationType='availability-zone', Filters=[{'Name': 'location', - 'Values': [self.provider.zone_name]}], + 'Values': [zone]}], **trim_empty_params( {'MaxResults': 1000, 'NextToken': vmt_list_resp.get("NextToken")})) @@ -1067,12 +1067,41 @@ def list(self, limit: int | None = None, # describe_instance_types call can get at most 100 types at once chunks = [vmt_list_names[x:x + 100] for x in range(0, len(vmt_list_names), 100)] - raw_types = [] + raw_types: list[dict[str, Any]] = [] for chunk in chunks: raw_chunk = client.describe_instance_types( InstanceTypes=chunk).get('InstanceTypes') raw_types.extend(raw_chunk) - cb_types = [AWSVMType(cast("AWSCloudProvider", self.provider), t) for t in raw_types] + return raw_types + + def _get_catalogue(self) -> list[dict[str, Any]]: + """ + Return the raw instance type catalogue for the provider's zone, + fetching it at most once per zone. + + EC2 has no server-side paging for instance types, so ``list()`` must + materialise the whole catalogue and page it client-side. Fetching it + costs one ``DescribeInstanceTypeOfferings`` walk plus one + ``DescribeInstanceTypes`` call per 100 types — around 14 calls for a + real region. Without memoisation every page of a ``list(marker=...)`` + walk repeats all of that, making a full walk quadratic in API calls: + 1343 types at a result limit of 5 costs ~4300 calls instead of ~14. + + The catalogue is static for the lifetime of a provider, so it is held + per zone (a provider may be cloned to another zone, which genuinely + offers a different set of types). + """ + zone = self.provider.zone_name + if zone not in self._catalogue: + self._catalogue[zone] = self._fetch_catalogue(zone) + return self._catalogue[zone] + + @dispatch(event="provider.compute.vm_types.list", + priority=BaseVMTypeService.STANDARD_EVENT_PRIORITY) + def list(self, limit: int | None = None, + marker: str | None = None) -> ResultList[VMType]: + cb_types = [AWSVMType(cast("AWSCloudProvider", self.provider), t) + for t in self._get_catalogue()] return ClientPagedResultList(self.provider, cb_types, limit=limit, marker=marker) @@ -1674,6 +1703,14 @@ def delete(self, dns_zone: DnsZone | str) -> None: client.delete_hosted_zone(Id=dns_zone.aws_id) +# Route53 reports record changes INSYNC within seconds, but boto3's +# resource_record_sets_changed waiter polls every 30s by default, so every +# record change costs a full 30s of sleep no matter how fast it propagated. +# Poll often enough that the granularity stops dominating, while keeping the +# same ~30 minute ceiling for changes that genuinely are slow. +DNS_CHANGE_WAITER_CONFIG = {'Delay': 5, 'MaxAttempts': 360} + + class AWSDnsRecordService(BaseDnsRecordService): def __init__(self, provider: CloudProvider) -> None: @@ -1765,7 +1802,8 @@ def create(self, dns_zone: DnsZone | str, name: str, type: str, # waiting, this is skipped for mock tests. if not cast("AWSCloudProvider", self.provider).PROVIDER_ID == 'mock': waiter = client.get_waiter('resource_record_sets_changed') - waiter.wait(Id=response.get('ChangeInfo').get('Id')) + waiter.wait(Id=response.get('ChangeInfo').get('Id'), + WaiterConfig=DNS_CHANGE_WAITER_CONFIG) return cast(DnsRecord, self.get(dns_zone, name + ":" + type)) def delete(self, dns_zone: DnsZone | str, @@ -1793,4 +1831,5 @@ def delete(self, dns_zone: DnsZone | str, # waiting, this is skipped for mock tests. if not cast("AWSCloudProvider", self.provider).PROVIDER_ID == 'mock': waiter = client.get_waiter('resource_record_sets_changed') - waiter.wait(Id=response.get('ChangeInfo').get('Id')) + waiter.wait(Id=response.get('ChangeInfo').get('Id'), + WaiterConfig=DNS_CHANGE_WAITER_CONFIG) diff --git a/tests/test_aws_dns_waiters.py b/tests/test_aws_dns_waiters.py new file mode 100644 index 00000000..bdc62dce --- /dev/null +++ b/tests/test_aws_dns_waiters.py @@ -0,0 +1,141 @@ +""" +Unit tests for how ``AWSDnsRecordService`` waits on Route53 changes. + +Creating or deleting a record blocks until Route53 reports the change INSYNC. +boto3's ``resource_record_sets_changed`` waiter polls every 30 seconds by +default, so a change that propagates in a few seconds still costs a full 30 -- +and a test that makes four record changes pays 120 seconds of pure sleep. +Measured against real Route53, INSYNC was reached within the first poll +interval every time, making the granularity the entire cost. + +These tests drive the real botocore waiter against a simulated clock, so they +assert on how long we *would* sleep without actually sleeping. +""" +import unittest +from unittest import mock + +import botocore.client +import botocore.waiter +from botocore.exceptions import WaiterError + +from cloudbridge.providers.aws import AWSCloudProvider +from cloudbridge.providers.aws.resources import AWSDnsRecord +from cloudbridge.providers.aws.resources import AWSDnsZone +from cloudbridge.providers.aws.services import AWSDnsRecordService + +# Simulated seconds before Route53 reports INSYNC. Real-world measurement put +# this comfortably inside one 30s poll interval. +INSYNC_AFTER = 6.0 +BOTO_DEFAULT_DELAY = 30.0 +# The waiter's ceiling must stay at roughly 30 minutes however it is polled. +REQUIRED_CEILING = 1700.0 + +ZONE = {'Id': '/hostedzone/Z1EXAMPLE', 'Name': 'example.com.'} +RECORD = {'Name': 'sub.example.com.', 'Type': 'CNAME', 'TTL': 500, + 'ResourceRecords': [{'Value': 'hello.com.'}]} + + +class _Route53Sim: + """Canned Route53 responses driven by a simulated clock.""" + + def __init__(self, insync_after=INSYNC_AFTER): + self.insync_after = insync_after + self.clock = 0.0 + self.sleeps = [] + self.get_change_calls = 0 + + def api(self, operation_name, params): + if operation_name == 'ChangeResourceRecordSets': + return {'ChangeInfo': {'Id': '/change/C1', 'Status': 'PENDING'}} + if operation_name == 'GetChange': + self.get_change_calls += 1 + status = ('INSYNC' if self.clock >= self.insync_after + else 'PENDING') + return {'ChangeInfo': {'Id': '/change/C1', 'Status': status}} + if operation_name == 'ListResourceRecordSets': + return {'ResourceRecordSets': [RECORD], 'IsTruncated': False} + raise AssertionError('unexpected operation: ' + operation_name) + + def sleep(self, secs): + self.sleeps.append(secs) + self.clock += secs + + @property + def total_wait(self): + return sum(self.sleeps) + + +def _provider(): + return AWSCloudProvider({'aws_access_key': 'dummy', + 'aws_secret_key': 'dummy', + 'aws_zone_name': 'us-east-1a'}) + + +def _run(sim, fn): + """Run fn with Route53 stubbed and the waiter's clock simulated.""" + with mock.patch.object(botocore.client.BaseClient, '_make_api_call', + lambda self, op, params: sim.api(op, params)), \ + mock.patch.object(botocore.waiter.time, 'sleep', sim.sleep): + return fn() + + +class AWSDnsWaiterTestCase(unittest.TestCase): + + def setUp(self): + self.provider = _provider() + self.svc = AWSDnsRecordService(self.provider) + self.zone = AWSDnsZone(self.provider, ZONE) + self.record = AWSDnsRecord(self.provider, self.zone, RECORD) + + def test_create_does_not_burn_a_full_poll_interval_on_a_fast_change(self): + sim = _Route53Sim() + + _run(sim, lambda: self.svc.create( + self.zone, 'sub.example.com.', 'CNAME', 'hello.com', ttl=500)) + + self.assertLess( + sim.total_wait, BOTO_DEFAULT_DELAY, + "A change that went INSYNC after %ss cost %ss of sleep; the " + "waiter is still polling at boto3's %ss default" + % (INSYNC_AFTER, sim.total_wait, BOTO_DEFAULT_DELAY)) + + def test_delete_does_not_burn_a_full_poll_interval_on_a_fast_change(self): + sim = _Route53Sim() + + _run(sim, lambda: self.svc.delete(self.zone, self.record)) + + self.assertLess( + sim.total_wait, BOTO_DEFAULT_DELAY, + "A change that went INSYNC after %ss cost %ss of sleep; the " + "waiter is still polling at boto3's %ss default" + % (INSYNC_AFTER, sim.total_wait, BOTO_DEFAULT_DELAY)) + + def test_waiter_polls_until_the_change_is_actually_insync(self): + """Faster polling must not mean giving up early.""" + sim = _Route53Sim(insync_after=47.0) + + _run(sim, lambda: self.svc.create( + self.zone, 'sub.example.com.', 'CNAME', 'hello.com', ttl=500)) + + self.assertGreaterEqual(sim.clock, 47.0, + "Returned before the change was INSYNC") + self.assertGreater(sim.get_change_calls, 1) + + def test_waiter_ceiling_is_still_about_thirty_minutes(self): + """Polling more often must not shrink how long we are willing to + wait -- a genuinely slow change should still be given ~30 minutes + before the waiter gives up.""" + sim = _Route53Sim(insync_after=float('inf')) + + with self.assertRaises(WaiterError): + _run(sim, lambda: self.svc.create( + self.zone, 'sub.example.com.', 'CNAME', 'hello.com', ttl=500)) + + self.assertGreaterEqual( + sim.total_wait, REQUIRED_CEILING, + "Waiter gave up after only %ss of simulated waiting" + % sim.total_wait) + + +if __name__ == '__main__': + unittest.main() diff --git a/tests/test_aws_vm_types.py b/tests/test_aws_vm_types.py new file mode 100644 index 00000000..7869c16e --- /dev/null +++ b/tests/test_aws_vm_types.py @@ -0,0 +1,166 @@ +""" +Unit tests for ``AWSVMTypeService`` catalogue retrieval. + +AWS has no server-side pagination for VM types, so ``list()`` materialises the +whole catalogue and hands it to ``ClientPagedResultList`` for client-side +paging. Fetching that catalogue is expensive: one +``DescribeInstanceTypeOfferings`` walk plus a ``DescribeInstanceTypes`` call +per 100 types (~14 calls for a real region). Re-fetching it for every page +makes walking the full list quadratic in API calls, so these tests pin the +catalogue down to a single fetch per zone. + +Exercised against an in-memory fake connection so they run in CI without +cloud credentials. +""" +import unittest + +from cloudbridge.providers.aws import AWSCloudProvider + +# Enough types to force several pages at the tests' small result limit, and +# more than one 100-type DescribeInstanceTypes chunk. +CATALOGUE_SIZE = 250 +CHUNK_SIZE = 100 +PAGE_SIZE = 5 + + +class _FakeEC2Client: + """Records every catalogue call made against it.""" + + def __init__(self, types_by_zone): + self._types_by_zone = types_by_zone + self.offering_calls = [] # zone per call + self.describe_type_calls = [] # list of requested type names + + def describe_instance_type_offerings(self, **kwargs): + zone = kwargs['Filters'][0]['Values'][0] + self.offering_calls.append(zone) + names = self._types_by_zone[zone] + return {'InstanceTypeOfferings': [{'InstanceType': n} for n in names]} + + def describe_instance_types(self, **kwargs): + requested = kwargs['InstanceTypes'] + self.describe_type_calls.append(requested) + return {'InstanceTypes': [{'InstanceType': n, + 'CurrentGeneration': True, + 'VCpuInfo': {'DefaultVCpus': 2}} + for n in requested]} + + +class _FakeEC2Conn: + def __init__(self, client): + self.meta = type('_Meta', (), {'client': client})() + + +def _make_provider(zone, types_by_zone): + provider = AWSCloudProvider({ + 'aws_access_key': 'dummy', + 'aws_secret_key': 'dummy', + 'aws_zone_name': zone, + 'default_result_limit': PAGE_SIZE, + }) + client = _FakeEC2Client(types_by_zone) + # ec2_conn is a lazily-populated property backed by this attribute. + provider._ec2_conn = _FakeEC2Conn(client) + return provider, client + + +def _zone_types(prefix, count=CATALOGUE_SIZE): + return ['{0}.type{1}'.format(prefix, i) for i in range(count)] + + +class AWSVMTypeCatalogueTestCase(unittest.TestCase): + + def setUp(self): + self.zone = 'us-east-1a' + self.types = {self.zone: _zone_types('a')} + + def _calls_for_one_catalogue_fetch(self): + expected_chunks = -(-CATALOGUE_SIZE // CHUNK_SIZE) # ceil div + return 1, expected_chunks + + def test_single_list_fetches_catalogue_once(self): + provider, client = _make_provider(self.zone, self.types) + + provider.compute.vm_types.list() + + offerings, chunks = self._calls_for_one_catalogue_fetch() + self.assertEqual(len(client.offering_calls), offerings) + self.assertEqual(len(client.describe_type_calls), chunks) + + def test_repeated_list_calls_reuse_the_catalogue(self): + provider, client = _make_provider(self.zone, self.types) + + provider.compute.vm_types.list() + provider.compute.vm_types.list() + provider.compute.vm_types.list() + + offerings, chunks = self._calls_for_one_catalogue_fetch() + self.assertEqual( + len(client.offering_calls), offerings, + "Catalogue offerings should be fetched once and reused, but were " + "fetched %s times" % len(client.offering_calls)) + self.assertEqual( + len(client.describe_type_calls), chunks, + "Instance type details should be fetched once and reused, but " + "%s calls were made" % len(client.describe_type_calls)) + + def test_paging_through_all_pages_does_not_refetch_catalogue(self): + """The cost of walking every page must not scale with the page count. + + This is the access pattern used by the standard-behaviour test helper + (``check_list``) and it is what made the AWS suite take ~50 minutes. + """ + provider, client = _make_provider(self.zone, self.types) + + result = provider.compute.vm_types.list() + pages = 1 + while result.is_truncated: + result = provider.compute.vm_types.list(marker=result.marker) + pages += 1 + + self.assertEqual(pages, -(-CATALOGUE_SIZE // PAGE_SIZE), + "Expected to walk the whole catalogue") + offerings, chunks = self._calls_for_one_catalogue_fetch() + self.assertEqual( + len(client.offering_calls), offerings, + "Walking %s pages refetched the offerings %s times" + % (pages, len(client.offering_calls))) + self.assertEqual( + len(client.describe_type_calls), chunks, + "Walking %s pages made %s DescribeInstanceTypes calls; the " + "catalogue should be fetched once" + % (pages, len(client.describe_type_calls))) + + def test_list_still_returns_the_full_catalogue_contents(self): + """Caching must not change what callers observe.""" + provider, client = _make_provider(self.zone, self.types) + + seen = [] + result = provider.compute.vm_types.list() + seen.extend(t.id for t in result) + while result.is_truncated: + result = provider.compute.vm_types.list(marker=result.marker) + seen.extend(t.id for t in result) + + self.assertEqual(seen, self.types[self.zone]) + + def test_catalogue_is_keyed_by_zone(self): + """A provider cloned to another zone must not reuse the first zone's + catalogue — zones genuinely offer different instance types.""" + other_zone = 'us-east-1b' + types = {self.zone: _zone_types('a'), other_zone: _zone_types('b')} + provider, client = _make_provider(self.zone, types) + + first = [t.id for t in provider.compute.vm_types.list()] + + provider.config['aws_zone_name'] = other_zone + provider._zone_name = other_zone + second = [t.id for t in provider.compute.vm_types.list()] + + self.assertEqual(first, types[self.zone][:PAGE_SIZE]) + self.assertEqual(second, types[other_zone][:PAGE_SIZE]) + self.assertEqual(client.offering_calls, [self.zone, other_zone]) + + +if __name__ == '__main__': + unittest.main()