From 6431fae12ca5977873de93856e443d3ef77079ad Mon Sep 17 00:00:00 2001 From: Anton Smorodskyi Date: Thu, 4 Jan 2024 17:28:58 +0100 Subject: [PATCH 1/2] Unify exceptions handling during VPC deletion There was two different attempts to handle known exceptions in VPC deletion. It appears that having both of them cause collisions. So I unify them into one. --- ocw/lib/ec2.py | 25 +++++++++++-------------- 1 file changed, 11 insertions(+), 14 deletions(-) diff --git a/ocw/lib/ec2.py b/ocw/lib/ec2.py index d52a2b3c..6d35e34f 100644 --- a/ocw/lib/ec2.py +++ b/ocw/lib/ec2.py @@ -162,6 +162,15 @@ def delete_vpc(self, region: str, vpc, vpc_id: str): self.log_info('Delete VPC={}', vpc_id) self.ec2_resource(region).meta.client.delete_vpc(VpcId=vpc_id) return None + except ClientError as ex: + # Our cleanup is not perfect yet so often at this stage we have VPC's + # which has dependencies still and we don't want to have emails about this known problem + # See https://docs.aws.amazon.com/AWSEC2/latest/APIReference/errors-overview.html for more details + if 'is currently in use.' in ex.response['Error']['Message']: + self.log_dbg(traceback.format_exc()) + self.log_info(ex.response['Error']) + return None + return f"[{vpc_id}] {type(ex).__name__} on VPC deletion. {traceback.format_exc()}" except Exception as ex: return f"[{vpc_id}] {type(ex).__name__} on VPC deletion. {traceback.format_exc()}" @@ -173,15 +182,7 @@ def delete_vpc_subnets(self, vpc) -> None: self.log_info(f'Deletion of {interface} skipped due to dry_run mode') else: self.log_info(f'Deleting {interface}') - try: - interface.delete() - except ClientError as exc: - # From https://docs.aws.amazon.com/AWSEC2/latest/APIReference/errors-overview.html - # If a network interface is in use, you may also receive the InvalidParameterValue error. - if exc.response['Error']['Code'] == 'InvalidParameterValue': - self.log_info(exc.response['Error']) - continue - raise + interface.delete() if self.dry_run: self.log_info(f'Deletion of {subnet} skipped due to dry_run mode') else: @@ -279,7 +280,6 @@ def cleanup_vpcs(self) -> None: vpc_errors = [] vpc_notify = [] vpc_locked = [] - vpc_known_exception = "botocore.exceptions.ClientError: An error occurred (DependencyViolation)" for region in self.all_regions: response = self.ec2_client(region).describe_vpcs(Filters=[{'Name': 'isDefault', 'Values': ['false']}]) self.log_dbg(f"Found {len(response['Vpcs'])} VPC's in {region}") @@ -297,10 +297,7 @@ def cleanup_vpcs(self) -> None: del_responce = self.delete_vpc(region, resource_vpc, vpc_id) if del_responce is not None: self.log_err(del_responce) - # Our cleanup is not perfect yet so often at this stage we have VPC's - # which has dependencies still and we don't want to have emails about this known problem - if vpc_known_exception not in del_responce: - vpc_errors.append(del_responce) + vpc_errors.append(del_responce) elif not self.dry_run: vpc_locked.append(f'{vpc_id} (OwnerId={response_vpc["OwnerId"]}) in {region} is locked') self.report_cleanup_results(vpc_errors, vpc_notify, vpc_locked) From 7fcfaae486fbb314f688dd4ef4a53bdd1041df08 Mon Sep 17 00:00:00 2001 From: Anton Smorodskyi Date: Thu, 4 Jan 2024 17:31:59 +0100 Subject: [PATCH 2/2] Remove optional parameter because sometimes we do not have it --- ocw/lib/ec2.py | 3 +-- tests/test_ec2.py | 2 +- 2 files changed, 2 insertions(+), 3 deletions(-) diff --git a/ocw/lib/ec2.py b/ocw/lib/ec2.py index 6d35e34f..a14b258b 100644 --- a/ocw/lib/ec2.py +++ b/ocw/lib/ec2.py @@ -256,8 +256,7 @@ def delete_routing_tables(self, region: str, vpc_id: str) -> None: else: self.log_info(f"Delete route {route_table['RouteTableId']}") self.log_dbg(route) - self.ec2_client(region).delete_route(RouteTableId=route_table['RouteTableId'], - DestinationCidrBlock=route['DestinationCidrBlock']) + self.ec2_client(region).delete_route(RouteTableId=route_table['RouteTableId']) if route_table['Associations'] == []: if self.dry_run: self.log_info(f"{route_table['RouteTableId']} routing table will not be deleted due to dry_run mode") diff --git a/tests/test_ec2.py b/tests/test_ec2.py index 7723caa8..c401b09e 100644 --- a/tests/test_ec2.py +++ b/tests/test_ec2.py @@ -170,7 +170,7 @@ def disassociate_route_table(self, AssociationId): def describe_route_tables(self, Filters): return MockedEC2Client.routing_tables - def delete_route(self, RouteTableId, DestinationCidrBlock): + def delete_route(self, RouteTableId): if RouteTableId == '2': MockedEC2Client.delete_route_called = True