From 51358ca60af85bec62674e646f31f85453b3e0c6 Mon Sep 17 00:00:00 2001 From: Dusty Mabe Date: Thu, 6 Feb 2025 11:01:32 -0500 Subject: [PATCH 1/2] cmd-coreos-prune: fix skipping of images action This detection wasn't working properly and causing the `images` action to get run even for builds where the images had already been pruned. --- src/cmd-coreos-prune | 13 ++++++------- 1 file changed, 6 insertions(+), 7 deletions(-) diff --git a/src/cmd-coreos-prune b/src/cmd-coreos-prune index eca94168b3..cca179de98 100755 --- a/src/cmd-coreos-prune +++ b/src/cmd-coreos-prune @@ -148,16 +148,15 @@ def main(): # Skip if the action has been handled previously for the build if action in previous_cleanup: - # If we are in here then there has been some previous cleanup of - # this type run for this build. For all types except `images` we - # can just continue. - if action != "images": - print(f"\t\tBuild {build_id} has already had {action} pruning completed") - continue + print(f"\t\tBuild {build_id} has already had {action} pruning completed") + continue + # We have to handle the "images" action separately because the key in the previous + # cleanup is "images-kept" and not "images" + if action == 'images' and 'images-kept' in previous_cleanup: # OK `images` has been pruned before, but we need to check # that all the images were pruned that match the current policy. # i.e. there may be additional images we need prune - elif set(images_to_keep) == set(previous_cleanup.get("images-kept", [])): + if set(images_to_keep) == set(previous_cleanup.get("images-kept", [])): print(f"\t\tBuild {build_id} has already had {action} pruning completed") continue From bb83fccd9e7639014e5c394a0220ae5c3838d6ca Mon Sep 17 00:00:00 2001 From: Dusty Mabe Date: Thu, 6 Feb 2025 11:02:46 -0500 Subject: [PATCH 2/2] cmd-coreos-prune: update error handling for s3.delete_object Apparently delete object won't return a NoSuchKey error if the object doesn't exist. I stumbled upon this because our handling for the images action meant we were calling prune_images() even for builds that had already had images pruned and the logs were telling me it was deleting the images again when it should have been telling me ``` XYZ.vmdk already pruned ``` Turns out the delete_object returns a `204 Success (No Content)` response when the key doesn't exist. https://docs.aws.amazon.com/AmazonS3/latest/API/API_DeleteObject.html#API_DeleteObject_RequestSyntax Let's just delete this code in here since it's dead code and can be confusing to future code readers. The prior commit to this one will try to ensure we don't end up in this code path if the images have already been pruned for a build. --- src/cmd-coreos-prune | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-) diff --git a/src/cmd-coreos-prune b/src/cmd-coreos-prune index cca179de98..d867ab475e 100755 --- a/src/cmd-coreos-prune +++ b/src/cmd-coreos-prune @@ -410,10 +410,11 @@ def prune_images(s3, build, images_to_keep, dry_run, bucket, prefix): s3.delete_object(Bucket=bucket, Key=image_prefix) print(f"\t\t\tPruned {name} image for {build.id} for {build.arch}") except botocore.exceptions.ClientError as e: - if e.response['Error']['Code'] == 'NoSuchKey': - print(f"\t\t\t{bucket}/{image_prefix} already pruned.") - else: - errors.append(e) + # Note that even if the object doesn't exist the delete_object() + # will still return Success (`204 Success (No Content)`) so we + # don't need to handle that error case here. + # https://docs.aws.amazon.com/AmazonS3/latest/API/API_DeleteObject.html#API_DeleteObject_RequestSyntax + errors.append(e) if errors: print(f"\t\t\tFound errors when pruning images for {build.id}:") for e in errors: