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()