diff --git a/Docs/DRY_DATA_REFACTOR.md b/Docs/DRY_DATA_REFACTOR.md new file mode 100644 index 00000000..959312b6 --- /dev/null +++ b/Docs/DRY_DATA_REFACTOR.md @@ -0,0 +1,152 @@ +# Dry Data Refactor + +This document summarizes the refactor of Cloudwash's "dry data" tracking (the +in-memory record of resources that are eligible for deletion/stop/skip during a +`--dry-run`) and the centralization of its printing/reporting logic. + +## Motivation + +Previously, `dry_data` was a single module-level dictionary in `cloudwash/utils.py`, +shared by every provider and resource entity via a plain `from cloudwash.utils import +dry_data` import. This had a few problems: + +- **Duplicate instantiation**: every resource entity (`CleanVMs`, `CleanDiscs`, ...) + reached into the same module-level dict directly, so there was no single owner of + the dry data's lifecycle. +- **Cross-region/zone leakage risk**: providers that iterate over multiple + regions/zones (AWS, Azure, GCE) reset the dict in-place between iterations + (`dry_data[items]['delete'] = []`). Sub-lists like `VMS.stop` and `VMS.skip` were + never reset, so stale entries from a previous region could leak into the next + region's dry-run report. +- **Repeated printing boilerplate**: every provider's `cleanup()` had its own + `if is_dry_run: echo_dry(dry_data); all_data.append(deepcopy(dry_data))` block. + +## Changes + +### 1. `DryData` class (`cloudwash/utils.py`) + +The module-level `dry_data` dict was replaced with a `DryData` class that +encapsulates the same structure (`VMS`, `CONTAINERS`, `DISCS`, `NICS`, `PIPS`, +`OCPS`, `RESOURCES`, `STACKS`, `IMAGES`, `PROVIDER`, `REGION`, `GROUP`, `ZONE`): + +```python +class DryData: + def __init__(self): + self._data = {...} # same layout as the old module-level dict + + def __getitem__(self, key): ... + def __setitem__(self, key, value): ... + def get(self, key, default=None): ... + def update(self, other_dict): ... + def reset_resource(self, resource_key): ... + def to_dict(self): ... +``` + +`__getitem__`/`__setitem__` keep the existing `dry_data['VMS']['delete']` call +sites working unchanged, so no consumer needed dict-vs-object gymnastics. + +Each provider's `cleanup()` now creates its **own** `DryData()` instance and passes +it down to the provider/resource entities instead of relying on a shared singleton. +This means concurrent/sequential regions or zones (and, in principle, concurrent +provider runs) never share mutable state. + +### 2. `reset_resource()` clears every sub-list, not just `delete` + +The old reset logic only cleared `delete`: + +```python +dry_data[items]['delete'] = [] +``` + +`stop`/`skip` (used by `VMS` and `CONTAINERS`) were never cleared between +regions/zones. `DryData.reset_resource()` now clears every sub-key of a resource: + +```python +def reset_resource(self, resource_key): + if resource_key in self._data: + for sub_key in self._data[resource_key]: + self._data[resource_key][sub_key] = [] +``` + +This is what actually guarantees two regions/zones can't conflict or repeat +resources in the dry-run output; it's covered by +`tests/test_dry_data.py::test_dry_data_reset_resource_clears_every_sub_list`. + +### 3. `dry_data` propagated through entities instead of re-imported + +- `cloudwash/entities/providers.py`: `providerCleanup.__init__` now accepts an + optional `dry_data` and stores it as `self.dry_data`; every resource property + (`vms`, `discs`, `nics`, `pips`, `images`, `stacks`, `ocps`, `containers`) passes + `dry_data=self.dry_data` down to the resource-entity constructor. The + `AzureCleanup`/`AWSCleanup`/`GCECleanup`/`VMWareCleanup`/`PodmanCleanup` + subclasses no longer need to redeclare an identical `__init__`. +- `cloudwash/entities/resources/*.py` (`vms.py`, `discs.py`, `nics.py`, `pips.py`, + `images.py`, `stacks.py`, `ocps.py`, `containers.py`): each `Clean*` class now + accepts `dry_data=None` and falls back to its own `DryData()` if not provided + (keeps the class usable standalone, e.g. in unit tests), using `self.dry_data` + in `_set_dry()` instead of the removed module-level import. + +### 4. Centralized dry-data printing: `print_dry_data()` + +Every provider previously repeated: + +```python +if is_dry_run: + echo_dry(dry_data) + all_data.append(deepcopy(dry_data)) +``` + +This is now a single utility function: + +```python +def print_dry_data(dry_data, is_dry_run, all_data): + """Centralized function to print dry data and append it to all_data if in dry run mode.""" + if is_dry_run: + echo_dry(dry_data.to_dict()) + all_data.append(deepcopy(dry_data.to_dict())) +``` + +`cloudwash/providers/{aws,azure,gce,podman,vmware}.py` all call +`print_dry_data(dry_data, is_dry_run, all_data)` at the end of each +region/zone/group iteration instead of duplicating the check/echo/append logic. + +### 5. `resourcewise_data()` / `echo_dry()` operate on plain dicts + +`resourcewise_data()` is invoked both with dry-run snapshots (plain dicts stored in +`all_data`) and from `create_html()` (also plain dicts). It no longer assumes its +argument is a `DryData` instance; it treats `dry_data` as the plain dict produced by +`DryData.to_dict()`, with `None` falling back to a fresh `DryData().to_dict()`. + +## Tests (`tests/test_dry_data.py`) + +- `test_dry_data_initialization` / `test_dry_data_update` / `test_dry_data_to_dict`: + basic `DryData` behavior. +- `test_dry_data_instances_are_independent`: two `DryData()` instances don't share + state. +- `test_dry_data_reset_resource_clears_every_sub_list`: `reset_resource` clears + `delete`/`stop`/`skip` together. +- `test_separate_regions_do_not_conflict_or_repeat` and + `test_no_resources_missing_across_regions`: simulate the exact region-loop + pattern used by `cloudwash/providers/aws.py` (reuse one `DryData` instance, + `reset_resource()` between regions, capture snapshots via `print_dry_data()`), + then assert: + - each captured snapshot only contains that region's resources (no + conflicting/repeating resources between regions), and + - the union of all snapshots contains every resource from every region (nothing + is silently dropped from the cleanup/dry-data output). + +## Requirements satisfied + +- [x] Introduced a `DryData` class and instantiate/pass it explicitly wherever + dry data is read or written, removing the shared module-level singleton. +- [x] Removed duplicate `DryData`/dry-data-dict instantiation across providers and + resource entities by threading a single instance through + `providerCleanup` → resource entities. +- [x] Centralized the repeated dry-run printing/reporting logic into + `print_dry_data()`, used identically by all five providers. +- [x] Fixed the underlying region/zone isolation bug (`reset_resource` not + clearing `stop`/`skip`) that could have caused conflicting/repeated + entries across regions or zones. +- [x] Added tests verifying two regions/zones produce disjoint, non-repeating + dry data and that no resource is missing from the aggregated cleanup/dry + data output. diff --git a/cloudwash/entities/providers.py b/cloudwash/entities/providers.py index 3b5002bf..1781db26 100644 --- a/cloudwash/entities/providers.py +++ b/cloudwash/entities/providers.py @@ -13,17 +13,19 @@ from cloudwash.entities.resources.vms import CleanAzureVMs from cloudwash.entities.resources.vms import CleanGCEVMs from cloudwash.entities.resources.vms import CleanVMWareVMs +from cloudwash.utils import DryData class providerCleanup: - def __init__(self, client): + def __init__(self, client, dry_data=None): self.client = client + self.dry_data = dry_data or DryData() @property def ocps(self): providerclass = self.__class__.__name__ if 'AWS' in providerclass: - return CleanAWSOcps(client=self.client) + return CleanAWSOcps(client=self.client, dry_data=self.dry_data) else: raise NotImplementedError(f'The OCPs cleanup on {providerclass} is not implemented') @@ -31,82 +33,70 @@ def ocps(self): def vms(self): providerclass = self.__class__.__name__ if 'Azure' in providerclass: - return CleanAzureVMs(client=self.client) + return CleanAzureVMs(client=self.client, dry_data=self.dry_data) elif 'AWS' in providerclass: - return CleanAWSVms(client=self.client) + return CleanAWSVms(client=self.client, dry_data=self.dry_data) elif 'GCE' in providerclass: - return CleanGCEVMs(client=self.client) + return CleanGCEVMs(client=self.client, dry_data=self.dry_data) elif 'VMWare' in providerclass: - return CleanVMWareVMs(client=self.client) + return CleanVMWareVMs(client=self.client, dry_data=self.dry_data) @property def discs(self): providerclass = self.__class__.__name__ if 'Azure' in providerclass: - return CleanAzureDiscs(client=self.client) + return CleanAzureDiscs(client=self.client, dry_data=self.dry_data) elif 'AWS' in providerclass: - return CleanAWSDiscs(client=self.client) + return CleanAWSDiscs(client=self.client, dry_data=self.dry_data) @property def nics(self): providerclass = self.__class__.__name__ if 'Azure' in providerclass: - return CleanAzureNics(client=self.client) + return CleanAzureNics(client=self.client, dry_data=self.dry_data) elif 'AWS' in providerclass: - return CleanAWSNics(client=self.client) + return CleanAWSNics(client=self.client, dry_data=self.dry_data) @property def pips(self): providerclass = self.__class__.__name__ if 'Azure' in providerclass: - return CleanAzurePips(client=self.client) + return CleanAzurePips(client=self.client, dry_data=self.dry_data) elif 'AWS' in providerclass: - return CleanAWSPips(client=self.client) + return CleanAWSPips(client=self.client, dry_data=self.dry_data) @property def images(self): providerclass = self.__class__.__name__ if 'Azure' in providerclass: - return CleanAzureImages(client=self.client) + return CleanAzureImages(client=self.client, dry_data=self.dry_data) elif 'AWS' in providerclass: - return CleanAWSImages(client=self.client) + return CleanAWSImages(client=self.client, dry_data=self.dry_data) @property def stacks(self): providerclass = self.__class__.__name__ if 'AWS' in providerclass: - return CleanAWSStacks(client=self.client) + return CleanAWSStacks(client=self.client, dry_data=self.dry_data) class AzureCleanup(providerCleanup): - def __init__(self, client): - self.client = client - super().__init__(client) + pass class AWSCleanup(providerCleanup): - def __init__(self, client): - self.client = client - super().__init__(client) + pass class GCECleanup(providerCleanup): - def __init__(self, client): - self.client = client - super().__init__(client) + pass class VMWareCleanup(providerCleanup): - def __init__(self, client): - self.client = client - super().__init__(self.client) + pass class PodmanCleanup(providerCleanup): - def __init__(self, client): - self.client = client - super().__init__(client) - @property def containers(self): - return CleanPodmanContainers(client=self.client) + return CleanPodmanContainers(client=self.client, dry_data=self.dry_data) diff --git a/cloudwash/entities/resources/containers.py b/cloudwash/entities/resources/containers.py index 4eaca9b5..66901651 100644 --- a/cloudwash/entities/resources/containers.py +++ b/cloudwash/entities/resources/containers.py @@ -1,22 +1,23 @@ from cloudwash.config import settings from cloudwash.entities.resources.base import ContainerCleanup from cloudwash.logger import logger -from cloudwash.utils import dry_data +from cloudwash.utils import DryData from cloudwash.utils import total_running_time class CleanContainers(ContainerCleanup): - def __init__(self, client): + def __init__(self, client, dry_data=None): self.client = client + self.dry_data = dry_data or DryData() self._delete = [] self._stop = [] self._skip = [] self.list() def _set_dry(self): - dry_data['CONTAINERS']['delete'] = self._delete - dry_data['CONTAINERS']['stop'] = self._stop - dry_data['CONTAINERS']['skip'] = self._skip + self.dry_data['CONTAINERS']['delete'] = self._delete + self.dry_data['CONTAINERS']['stop'] = self._stop + self.dry_data['CONTAINERS']['skip'] = self._skip def list(self): pass diff --git a/cloudwash/entities/resources/discs.py b/cloudwash/entities/resources/discs.py index abd29874..55ddd8e1 100644 --- a/cloudwash/entities/resources/discs.py +++ b/cloudwash/entities/resources/discs.py @@ -1,19 +1,20 @@ from cloudwash.config import settings from cloudwash.entities.resources.base import DiscsCleanup from cloudwash.logger import logger -from cloudwash.utils import dry_data +from cloudwash.utils import DryData class CleanDiscs(DiscsCleanup): - def __init__(self, client): + def __init__(self, client, dry_data=None): self.client = client + self.dry_data = dry_data or DryData() self._delete = [] self.list() def _set_dry(self): # VMsContainer = namedtuple('VMsCotainer', ['delete', 'stop', 'skip']) # return VMsContainer(self._delete, self._stop, self._skip) - dry_data['DISCS']['delete'] = self._delete + self.dry_data['DISCS']['delete'] = self._delete def list(self): pass diff --git a/cloudwash/entities/resources/images.py b/cloudwash/entities/resources/images.py index 811397c6..0c5c4bc5 100644 --- a/cloudwash/entities/resources/images.py +++ b/cloudwash/entities/resources/images.py @@ -1,12 +1,13 @@ from cloudwash.config import settings from cloudwash.entities.resources.base import ImagesCleanup from cloudwash.logger import logger -from cloudwash.utils import dry_data +from cloudwash.utils import DryData class CleanImages(ImagesCleanup): - def __init__(self, client): + def __init__(self, client, dry_data=None): self.client = client + self.dry_data = dry_data or DryData() self._delete = [] self._stop = [] self._skip = [] @@ -15,7 +16,7 @@ def __init__(self, client): def _set_dry(self): # ImagesContainer = namedtuple('ImagesCotainer', ['delete', 'stop', 'skip']) # return ImagesContainer(self._delete, self._stop, self._skip) - dry_data['IMAGES']['delete'] = self._delete + self.dry_data['IMAGES']['delete'] = self._delete def list(self): pass diff --git a/cloudwash/entities/resources/nics.py b/cloudwash/entities/resources/nics.py index d1ab0d9c..e3a008cd 100644 --- a/cloudwash/entities/resources/nics.py +++ b/cloudwash/entities/resources/nics.py @@ -1,17 +1,18 @@ from cloudwash.config import settings from cloudwash.entities.resources.base import NicsCleanup from cloudwash.logger import logger -from cloudwash.utils import dry_data +from cloudwash.utils import DryData class CleanNics(NicsCleanup): - def __init__(self, client): + def __init__(self, client, dry_data=None): self.client = client + self.dry_data = dry_data or DryData() self._delete = [] self.list() def _set_dry(self): - dry_data['NICS']['delete'] = self._delete + self.dry_data['NICS']['delete'] = self._delete def list(self): pass diff --git a/cloudwash/entities/resources/ocps.py b/cloudwash/entities/resources/ocps.py index b4778519..a82d3d7a 100644 --- a/cloudwash/entities/resources/ocps.py +++ b/cloudwash/entities/resources/ocps.py @@ -1,20 +1,21 @@ from cloudwash.config import settings from cloudwash.entities.resources.base import OCPsCleanup from cloudwash.utils import calculate_time_threshold -from cloudwash.utils import dry_data from cloudwash.utils import filter_resources_by_time_modified from cloudwash.utils import group_ocps_by_cluster from cloudwash.utils import OCP_TAG_SUBSTR +from cloudwash.utils import DryData class CleanOCPs(OCPsCleanup): - def __init__(self, client): + def __init__(self, client, dry_data=None): self.client = client + self.dry_data = dry_data or DryData() self._delete = [] self.list() def _set_dry(self): - dry_data['OCPS']['delete'] = self._delete + self.dry_data['OCPS']['delete'] = self._delete def list(self): pass diff --git a/cloudwash/entities/resources/pips.py b/cloudwash/entities/resources/pips.py index 7125bedc..521cb7f7 100644 --- a/cloudwash/entities/resources/pips.py +++ b/cloudwash/entities/resources/pips.py @@ -1,17 +1,18 @@ from cloudwash.config import settings from cloudwash.entities.resources.base import PipsCleanup from cloudwash.logger import logger -from cloudwash.utils import dry_data +from cloudwash.utils import DryData class CleanPips(PipsCleanup): - def __init__(self, client): + def __init__(self, client, dry_data=None): self.client = client + self.dry_data = dry_data or DryData() self._delete = [] self.list() def _set_dry(self): - dry_data['PIPS']['delete'] = self._delete + self.dry_data['PIPS']['delete'] = self._delete def list(self): pass diff --git a/cloudwash/entities/resources/stacks.py b/cloudwash/entities/resources/stacks.py index 568177bf..82caa808 100644 --- a/cloudwash/entities/resources/stacks.py +++ b/cloudwash/entities/resources/stacks.py @@ -1,18 +1,19 @@ from cloudwash.config import settings from cloudwash.entities.resources.base import StacksCleanup from cloudwash.logger import logger -from cloudwash.utils import dry_data +from cloudwash.utils import DryData from cloudwash.utils import total_running_time class CleanStacks(StacksCleanup): - def __init__(self, client): + def __init__(self, client, dry_data=None): self.client = client + self.dry_data = dry_data or DryData() self._delete = [] self.list() def _set_dry(self): - dry_data['STACKS']['delete'] = self._delete + self.dry_data['STACKS']['delete'] = self._delete def list(self): pass diff --git a/cloudwash/entities/resources/vms.py b/cloudwash/entities/resources/vms.py index 4fff04ee..531de5bd 100644 --- a/cloudwash/entities/resources/vms.py +++ b/cloudwash/entities/resources/vms.py @@ -1,13 +1,14 @@ from cloudwash.config import settings from cloudwash.entities.resources.base import VMsCleanup from cloudwash.logger import logger -from cloudwash.utils import dry_data +from cloudwash.utils import DryData from cloudwash.utils import total_running_time class CleanVMs(VMsCleanup): - def __init__(self, client): + def __init__(self, client, dry_data=None): self.client = client + self.dry_data = dry_data or DryData() self._delete = [] self._stop = [] self._skip = [] @@ -16,9 +17,9 @@ def __init__(self, client): def _set_dry(self): # VMsContainer = namedtuple('VMsCotainer', ['delete', 'stop', 'skip']) # return VMsContainer(self._delete, self._stop, self._skip) - dry_data['VMS']['delete'] = self._delete - dry_data['VMS']['stop'] = self._stop - dry_data['VMS']['skip'] = self._skip + self.dry_data['VMS']['delete'] = self._delete + self.dry_data['VMS']['stop'] = self._stop + self.dry_data['VMS']['skip'] = self._skip def list(self): pass diff --git a/cloudwash/providers/aws.py b/cloudwash/providers/aws.py index f9959827..a9e0d600 100644 --- a/cloudwash/providers/aws.py +++ b/cloudwash/providers/aws.py @@ -1,18 +1,17 @@ """ec2 CR Cleanup Utilities""" -from copy import deepcopy - from cloudwash.client import compute_client from cloudwash.config import settings from cloudwash.constants import aws_data as data from cloudwash.entities.providers import AWSCleanup from cloudwash.logger import logger from cloudwash.utils import create_html -from cloudwash.utils import dry_data -from cloudwash.utils import echo_dry +from cloudwash.utils import DryData +from cloudwash.utils import print_dry_data def cleanup(**kwargs): is_dry_run = kwargs.get("dry_run", False) + dry_data = DryData() dry_data['PROVIDER'] = "AWS" regions = settings.aws.auth.regions all_data = [] @@ -22,18 +21,16 @@ def cleanup(**kwargs): with compute_client("aws", aws_region=aws_client_region) as aws_ocp_client: if "all" in regions: regions = aws_ocp_client.list_regions() - awscleanup = AWSCleanup(client=aws_ocp_client) + awscleanup = AWSCleanup(client=aws_ocp_client, dry_data=dry_data) for region in regions: dry_data['REGION'] = region aws_ocp_client.cleaning_region = region # Emptying the dry data for previous region everytime for items in data: - dry_data[items]['delete'] = [] + dry_data.reset_resource(items) logger.info(f"\nResources from the region: {region}") awscleanup.ocps.cleanup() - if is_dry_run: - echo_dry(dry_data) - all_data.append(deepcopy(dry_data)) + print_dry_data(dry_data, is_dry_run, all_data) else: if "all" in regions: with compute_client("aws", aws_region="us-west-2") as client: @@ -42,9 +39,9 @@ def cleanup(**kwargs): dry_data['REGION'] = region # Emptying the dry data for previous region everytime for items in data: - dry_data[items]['delete'] = [] + dry_data.reset_resource(items) with compute_client("aws", aws_region=region) as aws_client: - awscleanup = AWSCleanup(client=aws_client) + awscleanup = AWSCleanup(client=aws_client, dry_data=dry_data) # Actual Cleaning and dry execution logger.info(f"\nResources from the region: {region}") if kwargs["vms"] or kwargs["_all"]: @@ -59,8 +56,6 @@ def cleanup(**kwargs): awscleanup.images.cleanup() if kwargs["stacks"] or kwargs["_all"]: awscleanup.stacks.cleanup() - if is_dry_run: - echo_dry(dry_data) - all_data.append(deepcopy(dry_data)) + print_dry_data(dry_data, is_dry_run, all_data) if is_dry_run: create_html(dry_data['PROVIDER'], all_data) diff --git a/cloudwash/providers/azure.py b/cloudwash/providers/azure.py index b45deeae..6a95be69 100644 --- a/cloudwash/providers/azure.py +++ b/cloudwash/providers/azure.py @@ -1,18 +1,17 @@ """Azure CR Cleanup Utilities""" -from copy import deepcopy - from cloudwash.client import compute_client from cloudwash.config import settings from cloudwash.constants import azure_data as data from cloudwash.entities.providers import AzureCleanup from cloudwash.logger import logger from cloudwash.utils import create_html -from cloudwash.utils import dry_data -from cloudwash.utils import echo_dry +from cloudwash.utils import DryData +from cloudwash.utils import print_dry_data def cleanup(**kwargs): is_dry_run = kwargs["dry_run"] + dry_data = DryData() dry_data['PROVIDER'] = "AZURE" regions = settings.azure.auth.regions groups = settings.azure.auth.resource_groups @@ -34,10 +33,10 @@ def cleanup(**kwargs): for group in groups: dry_data['GROUP'] = group for items in data: - dry_data[items]['delete'] = [] + dry_data.reset_resource(items) with compute_client("azure", azure_region=region, resource_group=group) as azure_client: - azurecleanup = AzureCleanup(client=azure_client) + azurecleanup = AzureCleanup(client=azure_client, dry_data=dry_data) def dry_resources(hours_old=None): dry_data["RESOURCES"]["delete"] = azure_client.list_resources_from_hours_old( @@ -83,8 +82,6 @@ def dry_resources(hours_old=None): hours_old=(sla_time / 60) ) logger.info(f"Removed Resources: \n{rres}") - if is_dry_run: - echo_dry(dry_data) - all_data.append(deepcopy(dry_data)) + print_dry_data(dry_data, is_dry_run, all_data) if is_dry_run: create_html(dry_data['PROVIDER'], all_data) diff --git a/cloudwash/providers/gce.py b/cloudwash/providers/gce.py index 3e37f1b9..f480e3fe 100644 --- a/cloudwash/providers/gce.py +++ b/cloudwash/providers/gce.py @@ -1,19 +1,18 @@ """GCE CR Cleanup Utilities""" -from copy import deepcopy - from cloudwash.client import compute_client from cloudwash.config import settings from cloudwash.constants import gce_data as data from cloudwash.entities.providers import GCECleanup from cloudwash.logger import logger from cloudwash.utils import create_html -from cloudwash.utils import dry_data -from cloudwash.utils import echo_dry +from cloudwash.utils import DryData from cloudwash.utils import gce_zones +from cloudwash.utils import print_dry_data def cleanup(**kwargs): is_dry_run = kwargs.get("dry_run", False) + dry_data = DryData() dry_data['PROVIDER'] = "GCE" zones = settings.gce.auth.get('zones', ['all']) all_data = [] @@ -29,15 +28,13 @@ def cleanup(**kwargs): for zone in zones: dry_data['ZONE'] = zone for items in data: - dry_data[items]['delete'] = [] + dry_data.reset_resource(items) gce_client.cleaning_zone = zone - gcecleanup = GCECleanup(client=gce_client) + gcecleanup = GCECleanup(client=gce_client, dry_data=dry_data) logger.info(f"\nResources from the zone: {zone}") if kwargs["vms"] or kwargs["_all"]: gcecleanup.vms.cleanup() - if is_dry_run: - echo_dry(dry_data) - all_data.append(deepcopy(dry_data)) + print_dry_data(dry_data, is_dry_run, all_data) if is_dry_run: create_html(dry_data['PROVIDER'], all_data) diff --git a/cloudwash/providers/podman.py b/cloudwash/providers/podman.py index 9658ff0b..3e562eb3 100644 --- a/cloudwash/providers/podman.py +++ b/cloudwash/providers/podman.py @@ -1,27 +1,24 @@ """PODMAN CR Cleanup Utilities""" -from copy import deepcopy - from cloudwash.client import compute_client from cloudwash.constants import container_data as data from cloudwash.entities.providers import PodmanCleanup from cloudwash.utils import create_html -from cloudwash.utils import dry_data -from cloudwash.utils import echo_dry +from cloudwash.utils import DryData +from cloudwash.utils import print_dry_data def cleanup(**kwargs): is_dry_run = kwargs.get("dry_run", False) + dry_data = DryData() dry_data['PROVIDER'] = "PODMAN" all_data = [] for items in data: - dry_data[items]['delete'] = [] + dry_data.reset_resource(items) with compute_client("podman") as podman_client: - podmancleanup = PodmanCleanup(client=podman_client) + podmancleanup = PodmanCleanup(client=podman_client, dry_data=dry_data) # Actual Cleaning and dry execution if kwargs["containers"]: podmancleanup.containers.cleanup() - if is_dry_run: - echo_dry(dry_data) - all_data.append(deepcopy(dry_data)) + print_dry_data(dry_data, is_dry_run, all_data) if is_dry_run: create_html(dry_data['PROVIDER'], all_data) diff --git a/cloudwash/providers/vmware.py b/cloudwash/providers/vmware.py index ff2740a0..bc411cb2 100644 --- a/cloudwash/providers/vmware.py +++ b/cloudwash/providers/vmware.py @@ -1,17 +1,16 @@ """VMWare CR Cleanup Utilities""" -from copy import deepcopy - from cloudwash.client import compute_client from cloudwash.constants import vmware_data as data from cloudwash.entities.providers import VMWareCleanup from cloudwash.logger import logger from cloudwash.utils import create_html -from cloudwash.utils import dry_data -from cloudwash.utils import echo_dry +from cloudwash.utils import DryData +from cloudwash.utils import print_dry_data def cleanup(**kwargs): is_dry_run = kwargs.get("dry_run", False) + dry_data = DryData() dry_data['PROVIDER'] = "VMWARE" all_data = [] if kwargs["nics"] or kwargs["_all"]: @@ -21,12 +20,10 @@ def cleanup(**kwargs): with compute_client("vmware") as vmware_client: for items in data: - dry_data[items]['delete'] = [] - vmware_cleanup = VMWareCleanup(client=vmware_client) + dry_data.reset_resource(items) + vmware_cleanup = VMWareCleanup(client=vmware_client, dry_data=dry_data) if kwargs["vms"] or kwargs["_all"]: vmware_cleanup.vms.cleanup() - if is_dry_run: - echo_dry(dry_data) - all_data.append(deepcopy(dry_data)) + print_dry_data(dry_data, is_dry_run, all_data) if is_dry_run: create_html(dry_data['PROVIDER'], all_data) diff --git a/cloudwash/utils.py b/cloudwash/utils.py index 60e3256d..dd702d7d 100644 --- a/cloudwash/utils.py +++ b/cloudwash/utils.py @@ -2,6 +2,7 @@ import importlib.resources from collections import namedtuple from datetime import datetime +from copy import deepcopy import dateparser import dominate @@ -22,51 +23,89 @@ OCP_TAG_SUBSTR = "kubernetes.io/cluster/" -_vms_dict = {"VMS": {"delete": [], "stop": [], "skip": []}} -_containers_dict = {"CONTAINERS": {"delete": [], "stop": [], "skip": []}} - -dry_data = { - "NICS": {"delete": []}, - "DISCS": {"delete": []}, - "PIPS": {"delete": []}, - "OCPS": {"delete": []}, - "RESOURCES": {"delete": []}, - "STACKS": {"delete": []}, - "IMAGES": {"delete": []}, - "PROVIDER": "", - "REGION": "", - "GROUP": "", - "ZONE": "", -} -non_rt_keys = ('provider', 'zone', 'region', 'group') -dry_data.update(_vms_dict) -dry_data.update(_containers_dict) +class DryData: + """Holds the per-run dry data for a single cleanup invocation. + + Each provider `cleanup()` call creates its own instance and passes it down to + the provider/resource entities, so concurrent or sequential regions/zones never + share (and therefore never leak into) each other's dry data. + """ + + def __init__(self): + self._data = { + "VMS": {"delete": [], "stop": [], "skip": []}, + "CONTAINERS": {"delete": [], "stop": [], "skip": []}, + "NICS": {"delete": []}, + "DISCS": {"delete": []}, + "PIPS": {"delete": []}, + "OCPS": {"delete": []}, + "RESOURCES": {"delete": []}, + "STACKS": {"delete": []}, + "IMAGES": {"delete": []}, + "PROVIDER": "", + "REGION": "", + "GROUP": "", + "ZONE": "", + } + + def get(self, key, default=None): + return self._data.get(key, default) + + def __getitem__(self, key): + return self._data[key] + + def __setitem__(self, key, value): + self._data[key] = value + + def update(self, other_dict): + self._data.update(other_dict) + + def reset_resource(self, resource_key): + """Clears every list held for a resource (e.g. delete/stop/skip) without + dropping its sub-keys, so stale entries never leak into the next region/zone. + """ + if resource_key in self._data: + for sub_key in self._data[resource_key]: + self._data[resource_key][sub_key] = [] + + def to_dict(self): + return self._data + + +non_rt_keys = ('provider', 'zone', 'region', 'group') def resourcewise_data(dry_data=None) -> dict: + """Builds the resourcewise dry data view used for logging and HTML reporting. + + :param dict dry_data: A plain dict following the `DryData.to_dict()` layout, + e.g. as produced by `print_dry_data` or stored in the `all_data` list. + Defaults to an empty/fresh `DryData` layout when not provided. + """ + data = dry_data if dry_data is not None else DryData().to_dict() resource_data = { - "provider": dry_data.get('PROVIDER'), - "region": dry_data.get('REGION'), - "group": dry_data.get('GROUP'), - "zone": dry_data.get('ZONE'), - "deletable_vms": dry_data["VMS"]["delete"], - "stopable_vms": dry_data["VMS"]["stop"], - "skipped_vms": dry_data["VMS"]["skip"], - "deletable_containers": dry_data["CONTAINERS"]["delete"], - "stopable_containers": dry_data["CONTAINERS"]["stop"], - "skipped_containers": dry_data["CONTAINERS"]["skip"], - "deletable_discs": dry_data["DISCS"]["delete"], - "deletable_nics": dry_data["NICS"]["delete"], - "deletable_images": dry_data["IMAGES"]["delete"], - "deletable_pips": dry_data["PIPS"]["delete"] if "PIPS" in dry_data else None, - "deletable_resources": dry_data["RESOURCES"]["delete"], - "deletable_stacks": dry_data["STACKS"]["delete"] if "STACKS" in dry_data else None, + "provider": data.get('PROVIDER'), + "region": data.get('REGION'), + "group": data.get('GROUP'), + "zone": data.get('ZONE'), + "deletable_vms": data["VMS"]["delete"], + "stopable_vms": data["VMS"]["stop"], + "skipped_vms": data["VMS"]["skip"], + "deletable_containers": data["CONTAINERS"]["delete"], + "stopable_containers": data["CONTAINERS"]["stop"], + "skipped_containers": data["CONTAINERS"]["skip"], + "deletable_discs": data["DISCS"]["delete"], + "deletable_nics": data["NICS"]["delete"], + "deletable_images": data["IMAGES"]["delete"], + "deletable_pips": data["PIPS"]["delete"] if "PIPS" in data else None, + "deletable_resources": data["RESOURCES"]["delete"], + "deletable_stacks": data["STACKS"]["delete"] if "STACKS" in data else None, "deletable_ocps": { ocp.resource_type: [ - r.name for r in dry_data["OCPS"]["delete"] if r.resource_type == ocp.resource_type + r.name for r in data["OCPS"]["delete"] if r.resource_type == ocp.resource_type ] - for ocp in dry_data["OCPS"]["delete"] + for ocp in data["OCPS"]["delete"] }, } return resource_data @@ -76,7 +115,7 @@ def echo_dry(dry_data=None) -> None: """Prints and Logs the per resource cleanup data on STDOUT and logfile :param dict dry_data: The deletable resources dry data of a Compute Resource, - it follows the format of module scoped `dry_data` variable in this module + following the `DryData.to_dict()` layout """ logger.info("\n=========== DRY SUMMARY ============\n") @@ -306,3 +345,10 @@ def filter_resources_by_time_modified( def delete_ocp(ocp): # WIP: add support for deletion pass + + +def print_dry_data(dry_data, is_dry_run, all_data): + """Centralized function to print dry data and append it to all_data if in dry run mode.""" + if is_dry_run: + echo_dry(dry_data.to_dict()) + all_data.append(deepcopy(dry_data.to_dict())) diff --git a/tests/test_dry_data.py b/tests/test_dry_data.py new file mode 100644 index 00000000..18398262 --- /dev/null +++ b/tests/test_dry_data.py @@ -0,0 +1,104 @@ +import unittest + +from cloudwash.constants import aws_data +from cloudwash.utils import DryData +from cloudwash.utils import print_dry_data +from cloudwash.utils import resourcewise_data + + +class TestDryData(unittest.TestCase): + def setUp(self): + self.dry_data = DryData() + + def test_dry_data_initialization(self): + self.assertEqual(self.dry_data['PROVIDER'], '') + self.assertEqual(self.dry_data['REGION'], '') + self.assertEqual(self.dry_data['GROUP'], '') + self.assertEqual(self.dry_data['ZONE'], '') + self.assertEqual(self.dry_data['VMS'], {'delete': [], 'stop': [], 'skip': []}) + self.assertEqual(self.dry_data['CONTAINERS'], {'delete': [], 'stop': [], 'skip': []}) + for resource in ('DISCS', 'NICS', 'IMAGES', 'PIPS', 'RESOURCES', 'STACKS', 'OCPS'): + self.assertEqual(self.dry_data[resource], {'delete': []}) + + def test_dry_data_update(self): + self.dry_data['PROVIDER'] = 'AWS' + self.dry_data['REGION'] = 'us-west-2' + self.assertEqual(self.dry_data['PROVIDER'], 'AWS') + self.assertEqual(self.dry_data['REGION'], 'us-west-2') + + def test_dry_data_reset_resource_clears_every_sub_list(self): + # A resource like VMS has delete/stop/skip sub-lists that must all be cleared, + # otherwise stale VMs from a previous region/zone would leak into the next one. + self.dry_data['VMS']['delete'] = ['vm1'] + self.dry_data['VMS']['stop'] = ['vm2'] + self.dry_data['VMS']['skip'] = ['vm3'] + + self.dry_data.reset_resource('VMS') + + self.assertEqual(self.dry_data['VMS'], {'delete': [], 'stop': [], 'skip': []}) + + def test_dry_data_to_dict(self): + self.dry_data['PROVIDER'] = 'AWS' + self.dry_data['REGION'] = 'us-west-2' + data_dict = self.dry_data.to_dict() + self.assertEqual(data_dict['PROVIDER'], 'AWS') + self.assertEqual(data_dict['REGION'], 'us-west-2') + + def test_dry_data_instances_are_independent(self): + dry_data1 = DryData() + dry_data1['VMS']['delete'] = ['vm1', 'vm2'] + + dry_data2 = DryData() + dry_data2['VMS']['delete'] = ['vm3', 'vm4'] + + self.assertNotEqual(dry_data1['VMS']['delete'], dry_data2['VMS']['delete']) + + def _simulate_region_loop(self, regions_vms): + """Mirrors the region loop in `cloudwash.providers.aws.cleanup`: a single DryData + instance is reused, reset per region, and each region's snapshot is captured via + `print_dry_data`, exactly as it happens for every real provider.""" + all_data = [] + for region, vms in regions_vms.items(): + self.dry_data['REGION'] = region + for resource in aws_data: + self.dry_data.reset_resource(resource) + self.dry_data['VMS'].update(vms) + print_dry_data(self.dry_data, is_dry_run=True, all_data=all_data) + return all_data + + def test_separate_regions_do_not_conflict_or_repeat(self): + regions_vms = { + 'us-east-1': {'delete': ['vm-1'], 'stop': ['vm-2'], 'skip': ['vm-3']}, + 'us-west-2': {'delete': ['vm-4'], 'stop': ['vm-5'], 'skip': ['vm-6']}, + } + all_data = self._simulate_region_loop(regions_vms) + + self.assertEqual(len(all_data), len(regions_vms)) + for snapshot, (region, vms) in zip(all_data, regions_vms.items()): + self.assertEqual(snapshot['REGION'], region) + self.assertEqual(snapshot['VMS'], vms) + + east_vms = set(sum(all_data[0]['VMS'].values(), [])) + west_vms = set(sum(all_data[1]['VMS'].values(), [])) + self.assertTrue(east_vms.isdisjoint(west_vms), "VMs from different regions must not repeat") + + def test_no_resources_missing_across_regions(self): + regions_vms = { + 'us-east-1': {'delete': ['vm-1'], 'stop': ['vm-2'], 'skip': ['vm-3']}, + 'us-west-2': {'delete': ['vm-4'], 'stop': ['vm-5'], 'skip': ['vm-6']}, + } + all_data = self._simulate_region_loop(regions_vms) + + reported_vms = set() + for snapshot in all_data: + resource_view = resourcewise_data(snapshot) + reported_vms.update(resource_view['deletable_vms']) + reported_vms.update(resource_view['stopable_vms']) + reported_vms.update(resource_view['skipped_vms']) + + expected_vms = {vm for vms in regions_vms.values() for vm_list in vms.values() for vm in vm_list} + self.assertEqual(reported_vms, expected_vms, "No VM should be dropped from the dry data output") + + +if __name__ == '__main__': + unittest.main()