Merge pull request #342 from CloudVE/aws-integration-test-runtime

Cut AWS integration suite runtime and stop credentials expiring mid-run

.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

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

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 @@ class AWSVMTypeService(BaseVMTypeService):
             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 @@ class AWSVMTypeService(BaseVMTypeService):
             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 @@ class AWSVMTypeService(BaseVMTypeService):
         # 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 @@ class AWSDnsZoneService(BaseDnsZoneService):
             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 @@ class AWSDnsRecordService(BaseDnsRecordService):
         # 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 @@ class AWSDnsRecordService(BaseDnsRecordService):
         # 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)

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

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