From 6352bbede77d51cde74012542a7b2a73daa030ec Mon Sep 17 00:00:00 2001 From: synkd Date: Fri, 22 Nov 2024 15:08:10 -0500 Subject: [PATCH 1/2] Use session-scoped inventory by default Currently, Manifester syncs its inventory multiple times while generating a manifest to ensure that the inventory data matches with the corresponding data in RHSM. For some use cases, though, this behavior is not desirable. Notably, in a CI scenario in which multiple workers share the same `username_prefix` setting, syncing the inventory with RHSM prevents those workers from running `manifester delete --all` at the end of each pipeline, as this could delete allocations that are still being used by other workers. This PR addresses this issue by converting the default inventory behavior to not sync from RHSM unless a sync is explicitly invoked. --- manifester/commands.py | 3 ++- manifester/helpers.py | 42 +++++++++++++++++++++++++++++++++------- manifester/manifester.py | 13 +++++++------ 3 files changed, 44 insertions(+), 14 deletions(-) diff --git a/manifester/commands.py b/manifester/commands.py index 04569a1..a0d09f6 100644 --- a/manifester/commands.py +++ b/manifester/commands.py @@ -80,7 +80,8 @@ def inventory(details, sync, offline_token): border = "-" * 38 if sync: helpers.update_inventory( - Manifester(minimal_init=True, offline_token=offline_token).subscription_allocations + Manifester(minimal_init=True, offline_token=offline_token).subscription_allocations, + sync=True, ) inv = helpers.load_inventory_file(Path(settings.inventory_path)) if not details: diff --git a/manifester/helpers.py b/manifester/helpers.py index 0470cfa..8ded10f 100644 --- a/manifester/helpers.py +++ b/manifester/helpers.py @@ -171,15 +171,43 @@ def load_inventory_file(file): return yaml.load(f, Loader=yaml.FullLoader) or [] -def update_inventory(inventory_data): +def _dump_inventory_file(inventory_path, inventory): + """Write inventory data to local inventory file.""" + with inventory_path.open("w") as inventory_file: + yaml.dump(inventory, inventory_file, allow_unicode=True) + + +def _update_inventory(inventory_path, inventory, allocation): + """Add new allocation, remove and recreate inventory file.""" + inventory.append(allocation) + inventory_path.unlink() + inventory_path.touch() + _dump_inventory_file(inventory_path, inventory) + + +def update_inventory(inventory_data, sync=False, remove=False, uuid=None): """Replace the existing inventory file with current subscription allocations.""" inventory_path = Path(settings.inventory_path) - if load_inventory_file(inventory_path): - inventory_path.unlink() - inventory_path.touch() - if inventory_data != []: - with inventory_path.open("w") as inventory_file: - yaml.dump(inventory_data, inventory_file, allow_unicode=True) + if sync: + if load_inventory_file(inventory_path): + inventory_path.unlink() + inventory_path.touch() + if inventory_data != []: + _dump_inventory_file(inventory_path, inventory_data) + elif remove: + inv = load_inventory_file(inventory_path) + _dump_inventory_file(inventory_path, [alloc for alloc in inv if uuid not in alloc["uuid"]]) + else: + current_allocation = next( + iter([alloc for alloc in inventory_data if alloc["uuid"] == uuid]) + ) + inv = load_inventory_file(inventory_path) + uuids = [alloc["uuid"] for alloc in inv] + if current_allocation["uuid"] not in uuids: + _update_inventory(inventory_path, inv, current_allocation) + else: + inv = [alloc for alloc in inv if uuid not in alloc["uuid"]] + _update_inventory(inventory_path, inv, current_allocation) def fake_http_response_code(good_codes=None, bad_codes=None, fail_rate=0): diff --git a/manifester/manifester.py b/manifester/manifester.py index 010e879..1fabc0a 100644 --- a/manifester/manifester.py +++ b/manifester/manifester.py @@ -185,7 +185,7 @@ def create_subscription_allocation(self): f"Subscription allocation created with name {self.allocation_name} " f"and UUID {self.allocation_uuid}" ) - update_inventory(self.subscription_allocations) + update_inventory(self.subscription_allocations, uuid=self.allocation_uuid) return self.allocation_uuid def delete_subscription_allocation(self, uuid=None): @@ -203,7 +203,9 @@ def delete_subscription_allocation(self, uuid=None): cmd_args=[f"{self.allocations_url}/{uuid if uuid else self.allocation_uuid}"], cmd_kwargs=data, ) - update_inventory(self.subscription_allocations) + update_inventory( + self.subscription_allocations, remove=True, uuid=uuid if uuid else self.allocation_uuid + ) return response def add_entitlements_to_allocation(self, pool_id, entitlement_quantity): @@ -322,7 +324,7 @@ def process_subscription_pools(self, subscription_pools, subscription_data): f"{subscription_data['name']} to the allocation." ) self._active_pools.append(match) - update_inventory(self.subscription_allocations) + update_inventory(self.subscription_allocations, uuid=self.allocation_uuid) break elif add_entitlements.status_code == SUCCESS_CODE: logger.debug( @@ -330,7 +332,7 @@ def process_subscription_pools(self, subscription_pools, subscription_data): f"{subscription_data['name']} to the allocation." ) self._active_pools.append(match) - update_inventory(self.subscription_allocations) + update_inventory(self.subscription_allocations, uuid=self.allocation_uuid) break else: raise RuntimeError( @@ -409,7 +411,7 @@ def trigger_manifest_export(self): manifest.uuid = self.allocation_uuid.uuid else: manifest.uuid = self.allocation_uuid - update_inventory(self.subscription_allocations) + update_inventory(self.subscription_allocations, uuid=self.allocation_uuid) return manifest def get_manifest(self): @@ -437,4 +439,3 @@ def __enter__(self): def __exit__(self, *tb_args): """Deletes subscription allocation on teardown unless using CLI.""" self.delete_subscription_allocation() - update_inventory(self.subscription_allocations) From 89488f920f8fc4df1823b273c92cddeb5d249f08 Mon Sep 17 00:00:00 2001 From: synkd Date: Mon, 2 Dec 2024 16:00:42 -0500 Subject: [PATCH 2/2] Get unit tests into passing state --- manifester/manifester.py | 11 ++++------- tests/test_manifester.py | 25 ++++++++++++++++++------- 2 files changed, 22 insertions(+), 14 deletions(-) diff --git a/manifester/manifester.py b/manifester/manifester.py index 1fabc0a..13ca11a 100644 --- a/manifester/manifester.py +++ b/manifester/manifester.py @@ -170,7 +170,9 @@ def create_subscription_allocation(self): cmd_kwargs=allocation_data, ).json() logger.debug(f"Received response {self.allocation} when attempting to create allocation.") - self.allocation_uuid = self.allocation["body"]["uuid"] + self.allocation_uuid = ( + self.allocation.uuid if self.is_mock else self.allocation["body"]["uuid"] + ) if self.simple_content_access == "disabled": simple_retry( self.requester.put, @@ -196,8 +198,6 @@ def delete_subscription_allocation(self, uuid=None): "proxies": self.manifest_data.get("proxies"), "params": {"force": "true"}, } - if self.is_mock: - self.allocation_uuid = self.allocation_uuid.uuid response = simple_retry( self.requester.delete, cmd_args=[f"{self.allocations_url}/{uuid if uuid else self.allocation_uuid}"], @@ -407,10 +407,7 @@ def trigger_manifest_export(self): local_file.write_bytes(manifest.content) manifest.path = local_file manifest.name = self.manifest_name - if self.is_mock: - manifest.uuid = self.allocation_uuid.uuid - else: - manifest.uuid = self.allocation_uuid + manifest.uuid = self.allocation_uuid update_inventory(self.subscription_allocations, uuid=self.allocation_uuid) return manifest diff --git a/tests/test_manifester.py b/tests/test_manifester.py index 3180395..e566675 100644 --- a/tests/test_manifester.py +++ b/tests/test_manifester.py @@ -82,11 +82,13 @@ + "".join(random.choices(string.ascii_letters, k=8)), "type": "Satellite", "version": f"{MANIFEST_DATA['sat_version']}", - "entitlementQuantity": sum(d["quantity"] for d in MANIFEST_DATA["subscription_data"]), - "url": f"{MANIFEST_DATA['url']['allocations']}/{SUB_ALLOCATION_UUID}", + "entitlementsAttachedQuantity": sum( + d["quantity"] for d in MANIFEST_DATA["subscription_data"] + ), "simpleContentAccess": f"{MANIFEST_DATA['simple_content_access']}", } - ] + ], + "status_code": 200, } @@ -148,9 +150,9 @@ def get(self, *args, **kwargs): self.pool_response["body"] += SUB_POOL_RESPONSE["body"] return self if args[0].endswith("allocations") and self._has_offset: - if kwargs["params"]["offset"] != 50: + if kwargs["params"]["offset"] != 100: self.allocations_response = {"body": []} - for _x in range(50): + for _x in range(100): self.allocations_response["body"].append( { "uuid": f"{uuid.uuid4().hex}", @@ -197,6 +199,15 @@ def delete(self, *args, **kwargs): self._good_codes = [204] return self + def __repr__(self): + """Return a string representation of the RhsmApiStub instance.""" + inner = ", ".join( + f"{k}={v}" + for k, v in self.__dict__.items() + if not k.startswith("_") and not callable(v) + ) + return f"{self.__class__.__name__}({inner})" + def test_basic_init(): """Test that manifester can initialize with the minimum required arguments.""" @@ -211,7 +222,7 @@ def test_create_allocation(): """Test that manifester's create_subscription_allocation method returns a UUID.""" manifester = Manifester(manifest_category=MANIFEST_DATA, requester=RhsmApiStub(in_dict=None)) allocation_uuid = manifester.create_subscription_allocation() - assert allocation_uuid.uuid == SUB_ALLOCATION_UUID + assert allocation_uuid == SUB_ALLOCATION_UUID def test_negative_simple_retry_timeout(): @@ -300,7 +311,7 @@ def test_invalid_sat_version(): def test_update_inventory(): """Test that inventory file is populated with expected contents after updating.""" manifester = Manifester(manifest_category=MANIFEST_DATA, requester=RhsmApiStub(in_dict=None)) - update_inventory(manifester.subscription_allocations) + update_inventory(manifester.subscription_allocations, sync=True, uuid=SUB_ALLOCATION_UUID) assert ( load_inventory_file(Path(MANIFEST_DATA["inventory_path"])) == SUB_ALLOCATIONS_RESPONSE["body"]