#8146 Fix the clean ami script to remove the amis based on date
Merged 9 months ago by mohanboddu. Opened 9 months ago by sayanchowdhury.
sayanchowdhury/releng fix-clean-ami-script  into  master

file modified
+290 -79

@@ -7,121 +7,268 @@ 

  #     Sayan Chowdhury <sayanchowdhury@fedoraproject.org>

  # Copyright (C) 2016 Red Hat Inc,

  # SPDX-License-Identifier:	GPL-2.0+

+ #

+ # The script runs as a cron job within the Fedora Infrastructure to delete

+ # the old AMIs. The permission of the selected AMIs are changed to private.

+ # This is to make sure that if someone from the community raises an issue

+ # we have the option to get the AMI back to public.

+ # After 10 days, if no complaints are raised the AMIs are deleted permanently.

+ #

+ # The complete process can be divided in couple of parts:

+ #

+ # - Fetching the data from datagrepper.

+ #   Based on the `--days` param, the script starts fetching the fedmsg messages

+ #   from datagrepper for the specified timeframe i.e. for lasts `n` days, where

+ #   `n` is the value of `--days` param. The queried fedmsg

+ #   topic `fedimg.image.upload`.

+ #

+ # - Selection of the AMIs:

+ #   After the AMIs are parsed from datagrepper. The AMIs are filtered to remove

+ #   Beta, Two-week Atomic Host and GA released AMIs.

+ #   Composes with `compose_type` set to `nightly` are picked up for deletion.

+ #   Composes which contain date in the `compose label` are also picked up for

+ #   deletion.

+ #   GA composes also have the compose_type set to production. So to distinguish

+ #   then we filter them if the compose_label have date in them. The GA

+ #   composes dont have date whereas they have the version in format of X.Y

+ #

+ # - Updated permissions of AMIs

+ #   The permissions of the selected AMIs are changed to private.

+ #

+ # - Deletion of AMIs

+ #   After 10 days, the private AMIs are deleted.

  

  from __future__ import print_function

  

  import os

+ import re

  import argparse

- import boto.ec2

+ import boto3

+ import functools

  import fedfind

  import fedfind.release

  import requests

  

- from datetime import datetime, timedelta

+ from datetime import datetime, timedelta, date

  

  import logging

+ 

  logging.basicConfig(level=logging.INFO)

  log = logging.getLogger()

  

  env = os.environ

- aws_access_key_id = os.environ.get('AWS_ACCESS_KEY')

- aws_secret_access_key = os.environ.get('AWS_SECRET_ACCESS_KEY')

+ aws_access_key_id = os.environ.get("AWS_ACCESS_KEY")

+ aws_secret_access_key = os.environ.get("AWS_SECRET_ACCESS_KEY")

  

- DATAGREPPER_URL = 'https://apps.fedoraproject.org/datagrepper/'

- NIGHTLY = 'nightly'

+ DATAGREPPER_URL = "https://apps.fedoraproject.org/datagrepper/"

+ NIGHTLY = "nightly"

  

  REGIONS = (

-     'us-east-1',

-     'us-west-2',

-     'us-west-1',

-     'eu-west-1',

-     'eu-central-1',

-     'ap-south-1',

-     'ap-southeast-1',

-     'ap-northeast-1',

-     'ap-northeast-2',

-     'ap-southeast-2',

-     'sa-east-1',

+     "us-east-1",

+     "us-east-2",

+     "us-west-2",

+     "us-west-1",

+     "eu-west-1",

+     "eu-central-1",

+     "ap-south-1",

+     "ap-southeast-1",

+     "ap-northeast-1",

+     "ap-northeast-2",

+     "ap-southeast-2",

+     "sa-east-1",

+     "ca-central-1",

+     "eu-west-2",

  )

  

  

+ def _is_timestamp_newer(timestamp1, timestamp2):

+     """ Return true if timestamp1 is newer than timestamp2

+     """

+     timestamp1_f = datetime.strptime(timestamp1, "%d%m%Y")

+     timestamp2_f = datetime.strptime(timestamp2, "%d%m%Y")

+ 

+     return timestamp1_f > timestamp2_f

+ 

+ 

  def _get_raw_url():

      """ Get the datagrepper raw URL to fetch the message from

      """

-     return DATAGREPPER_URL + '/raw'

+     return DATAGREPPER_URL + "/raw"

  

  

- def _get_nightly_amis_nd(delta, start=None, end=None):

-     """ Returns the nightly AMIs for the last n days

+ def get_page(page, delta, topic, start=None, end=None):

  

-     :args delta: last delta seconds

-     """

-     amis = []

      params = {

-         'topic': 'org.fedoraproject.prod.fedimg.image.upload',

-         'delta': delta,

-         'rows_per_page': 100,

+         "topic": topic,

+         "delta": delta,

+         "rows_per_page": 100,

+         "page": page,

      }

  

      if start:

-         params.update({'start': start})

+         params.update({"start": start})

  

      if end:

-         params.update({'end': end})

+         params.update({"end": end})

  

      resp = requests.get(_get_raw_url(), params=params)

-     messages = resp.json().get('raw_messages', [])

-     print(messages)

  

-     # Filter completed messages

-     messages = [msg['msg']

-                 for msg in messages if msg['msg']['status'] == 'completed']

+     return resp.json()

+ 

+ 

+ def _get_two_week_atomic_released_compose_id(delta, start=None, end=None):

+     """ Returns the release compose ids for last n days """

+ 

+     topic = "org.fedoraproject.prod.releng.atomic.twoweek.complete"

+     data = get_page(1, delta, topic, start, end)

+ 

+     messages = data.get("raw_messages", [])

+ 

+     for page in range(1, data["pages"]):

+         data = get_page(

+             topic=topic, page=page + 1, delta=delta, start=start, end=end

+         )

+         messages.extend(data["raw_messages"])

+ 

+     messages = [msg["msg"] for msg in messages]

  

+     released_atomic_compose_ids = []

      for msg in messages:

-         ami_id = msg['extra']['id']

-         region = msg['destination']

+         # This is to support the older-format fedmsg messages

+         if "atomic_raw" in msg:

+             released_atomic_compose_ids.append(["atomic_raw"]["compose_id"])

+         # We are just trying here multiple archs to get the compose id

+         elif "aarch64" in msg:

+             released_atomic_compose_ids.append(

+                 msg["aarch64"]["atomic_raw"]["compose_id"]

+             )

+         elif "x86_64" in msg:

+             released_atomic_compose_ids.append(

+                 msg["x86_64"]["atomic_raw"]["compose_id"]

+             )

+         elif "ppc64le" in msg:

+             released_atomic_compose_ids.append(

+                 msg["ppc64le"]["atomic_raw"]["compose_id"]

+             )

+ 

+     return set(released_atomic_compose_ids)

  

-         compose_id = msg['compose']['compose_id']

-         compose_info = fedfind.release.get_release_cid(compose_id)

+ 

+ def _get_nightly_amis_nd(delta, start=None, end=None):

+     """ Returns the nightly AMIs for the last n days

+ 

+     :args delta: last delta seconds

+     """

+     amis = []

+     released_atomic_compose_ids = _get_two_week_released_atomic_compose_id(

+         delta=delta, start=start, end=end

+     )

+ 

+     topic = "org.fedoraproject.prod.fedimg.image.publish"

+     data = get_page(1, delta, topic, start, end)

+     messages = data.get("raw_messages", [])

+ 

+     for page in range(1, data["pages"]):

+         data = get_page(

+             topic=topic, page=page + 1, delta=delta, start=start, end=end

+         )

+         messages.extend(data["raw_messages"])

+ 

+     for message in messages:

+         msg = message.get("msg")

+         ami_id = msg["extra"]["id"]

+         region = msg["destination"]

+ 

+         compose_id = msg["compose"]

+         compose_info = fedfind.release.get_release(cid=compose_id)

          compose_type = compose_info.type

+         compose_label = compose_info.label

+ 

+         # Sometimes the compose label is None

+         # and they can be blindly put in for deletion

+         if compose_label is None:

+             amis.append((compose_id, ami_id, region))

  

+         if compose_id in released_atomic_compose_ids:

+             continue

+ 

+         # Include the nightly composes

          if compose_type == NIGHTLY:

              amis.append((compose_id, ami_id, region))

+         else:

+             # Include AMIs that have date in them

+             # These are the production compose type but not GA

+             result = re.search("-(\d{8}).", compose_label)

+             if result is None:

+                 continue

+             amis.append((compose_id, ami_id, region))

  

      return amis

  

  

- def delete_amis_nd(amis, dry_run=False):

+ def delete_amis_nd(deletetimestamp, dry_run=False):

      """ Delete the give list of nightly AMIs

  

-     :args amis: list of AMIs

+     :args deletetimestamp: the timestamp for the delete 

      :args dry_run: dry run the flow

      """

-     log.info('Deleting AMIs')

+     log.info("Deleting AMIs")

      for region in REGIONS:

-         log.info('%s Starting' % region)

+         log.info("%s Starting" % region)

          # Create a connection to an AWS region

-         conn = boto.ec2.connect_to_region(

+         conn = boto3.client(

+             "ec2",

              region,

              aws_access_key_id=aws_access_key_id,

-             aws_secret_access_key=aws_secret_access_key)

-         log.info('%s: Connected' % region)

+             aws_secret_access_key=aws_secret_access_key,

+         )

+         log.info("%s: Connected" % region)

  

-         # Filter all the nightly AMIs belonging to this region

-         r_amis = [amis for _, _, r in amis if r == region]

+         response = conn.describe_images(

+             Filters=[{"Name": "tag-key", "Values": ["LaunchPermissionRevoked"]}]

+         )

+         amis = response.get("Images", [])

  

-         # Loop through the AMIs and delete then only if the launch permission

-         # for the AMIs are private.

-         for compose_id, ami_id, region in r_amis:

+         for ami in amis:

              try:

-                 ami_obj = conn.get_image(ami_id)

-                 is_launch_permitted = bool(ami_obj.get_launch_permission())

-                 if not dry_run and not is_launch_permitted:

-                     conn.deregister_image(ami_obj.id, delete_snapshot=True)

+                 ami_id = ami["ImageId"]

+                 is_launch_permitted = ami["Public"]

+                 _index = len(ami["BlockDeviceMappings"])

+                 snapshot_id = ami["BlockDeviceMappings"][0]["Ebs"]["SnapshotId"]

+                 tags = ami["Tags"]

+ 

+                 revoketimestamp = ""

+                 for tag in tags:

+                     if "LaunchPermissionRevoked" in tag.values():

+                         revoketimestamp = tag["Value"]

+ 

+                 if not revoketimestamp:

+                     log.warn(

+                         "%s ami has LaunchPermissionRevoked tag but no value"

+                         % ami_id

+                     )

+                     continue

+ 

+                 if is_launch_permitted:

+                     log.warn(

+                         "%s ami has LaunchPermissionRevoked tag "

+                         "but launch permission is still enabled" % ami_id

+                     )

+                     continue

+ 

+                 # The revoke timestamp allows us to tell how long ago an image

+                 # had permissions removed. If the permissions have been removed

+                 # for shorter than the waiting period then we can't delete it yet.

+                 if _is_timestamp_newer(revoketimestamp, deletetimestamp):

+                     continue

+ 

+                 if not dry_run:

+                     conn.deregister_image(ImageId=ami_id)

+                     conn.delete_snapshot(SnapshotId=snapshot_id)

                  else:

                      print(ami_id)

-             except:

-                 log.error('%s: %s failed' % (region, ami_id))

+             except Exception as ex:

+                 log.error("%s: %s failed\n%s" % (region, ami_id, ex))

  

  

  def change_amis_permission_nd(amis, dry_run=False):

@@ -133,61 +280,125 @@ 

      :args amis: list of AMIs

      :args dry_run: dry run the flow

      """

-     log.info('Changing permission for AMIs')

+     log.info("Changing permission for AMIs")

+     todaystimestamp = date.today().strftime("%d%m%Y")

  

      for region in REGIONS:

-         log.info('%s: Starting' % region)

+         log.info("%s: Starting" % region)

          # Create a connection to an AWS region

-         conn = boto.ec2.connect_to_region(

+         conn = boto3.client(

+             "ec2",

              region,

              aws_access_key_id=aws_access_key_id,

-             aws_secret_access_key=aws_secret_access_key)

-         log.info('%s: Connected' % region)

+             aws_secret_access_key=aws_secret_access_key,

+         )

+         log.info("%s: Connected" % region)

  

          # Filter all the nightly AMIs belonging to this region

-         r_amis = [amis for _, _, r in amis if r == region]

+         r_amis = [(c, a, r) for c, a, r in amis if r == region]

  

          # Loop through the AMIs change the permissions

          for _, ami_id, region in r_amis:

              try:

                  if not dry_run:

-                     conn.modify_image_attribute(ami_id,

-                                                 attribute='launchPermission',

-                                                 operation='remove',

-                                                 groups='all')

+                     conn.modify_image_attribute(

+                         ImageId=ami_id,

+                         LaunchPermission={"Remove": [{"Group": "all"}]},

+                     )

+                     conn.create_tags(

+                         Resources=[ami_id],

+                         Tags=[

+                             {

+                                 "Key": "LaunchPermissionRevoked",

+                                 "Value": todaystimestamp,

+                             }

+                         ],

+                     )

                  else:

                      print(ami_id)

-             except:

-                 log.error('%s: %s failed' % (region, ami_id))

+             except Exception as ex:

+                 log.error("%s: %s failed \n %s" % (region, ami_id, ex))

  

  

- if __name__ == '__main__':

+ if __name__ == "__main__":

      argument_parser = argparse.ArgumentParser()

      argument_parser.add_argument(

          "--delete",

          help="Delete the AMIs whose launch permissions have been removed",

-         action="store_true", default=False)

+         action="store_true",

+         default=False,

+     )

+     argument_parser.add_argument(

+         "--days",

+         help="Specify the number of days worth of AMI fedmsg information to fetch from datagrepper.",

+         type=int,

+     )

+     argument_parser.add_argument(

+         "--deletewaitperiod",

+         help="Specify the number of days to wait after removing launch perms before deleting",

+         type=int,

+         default=10,

+     )

+     argument_parser.add_argument(

+         "--permswaitperiod",

+         help="Specify the number of days to wait before removing launch perms",

+         type=int,

+         default=10,

+     )

      argument_parser.add_argument(

          "--change-perms",

          help="Change the launch permissions of the AMIs to private",

-         action="store_true", default=False)

+         action="store_true",

+         default=False,

+     )

      argument_parser.add_argument(

          "--dry-run",

          help="Dry run the action to be performed",

-         action="store_true", default=False)

+         action="store_true",

+         default=False,

+     )

      args = argument_parser.parse_args()

  

      if not args.delete and not args.change_perms:

-         print('Either of the argument, delete or change permission is allowed')

+         raise Exception(

+             "Either of the argument, delete or change permission is required"

+         )

  

      if args.delete and args.change_perms:

-         print('Both the argument delete and change permission is not allowed')

- 

-     if args.delete:

-         end = (datetime.now() - timedelta(days=10)).strftime('%s')

-         amis = _get_nightly_amis_nd(delta=864000, end=int(end))

-         delete_amis_nd(amis, dry_run=args.dry_run)

+         raise Exception(

+             "Both the argument delete and change permission is not allowed"

+         )

+ 

+     # Ideally, we could search through all the AMIs that ever were created but this

+     # this would create huge load on datagrepper.

+     # default to 4 weeks/ 28 days

+     days = 28

+     if args.days:

+         days = args.days

+ 

+     permswaitperiod = args.permswaitperiod

+     deletewaitperiod = args.deletewaitperiod

+ 

+     # The AMIs deleted are the nightly AMIs that are uploaded via fedimg everyday.

+     # The clean up of the AMIs happens through a cron job.

+     # The steps followed while deleting the AMIs:

+     # - The selected AMIs are made private, so that if people report issue we can make it

+     #   public again.

+     # - If no issues are reported in 10 days, the AMIs are deleted permanently.

  

      if args.change_perms:

-         amis = _get_nightly_amis_nd(delta=864000)

+         if days < permswaitperiod:

+             raise Exception(

+                 "permswaitperiod param cannot be more than days param"

+             )

+         end = (datetime.now() - timedelta(days=permswaitperiod)).strftime("%s")

+         amis = _get_nightly_amis_nd(

+             delta=86400 * (days - permswaitperiod), end=int(end)

+         )

          change_amis_permission_nd(amis, dry_run=args.dry_run)

+ 

+     if args.delete:

+         deletetimestamp = (

+             datetime.now() - timedelta(days=deletewaitperiod)

+         ).strftime("%d%m%Y")

+         delete_amis_nd(deletetimestamp, dry_run=args.dry_run)

This fixes #8126.

Additional changes made to the script:
- You can pass a additional params as days. Earlier it was hardcoded to 10 days. This is beneficial if the script fails to run over 10 days then we can pass the --days param.
- Removes images which have date in them (treats them as nightly) except for the updates composes because I'm unable to find a way to filter out the 2WA released composes.

Signed-off-by: Sayan Chowdhury sayan.chowdhury2012@gmail.com

days=10 is hardcoded here - is that right? I think it should be days=days

can we re-order these two (change perms first and then delete) and then add a nice comment about what is going on?

It took me a while to figure out what is going on and I'm still not sure it's entirely right (or maybe I misunderstand).

For each AMI:

  • (2*$days < item < $days) -> DELETE
  • ($days < item < TODAY) -> CHANGE PERMISSIONS

Is that right?

Here, 10 is the number of days the AMIs will stay with the changed perms.

This help message needs more clarity. I don't quite get exactly what value you want me to give it for days.

we need a comment at the beginning of this if/else that explains how we are determining which AMIs are OK to delete.

actually maybe better to just explain all of the logic before any of the if statements in this function.

REGIONS = (          
    'us-east-1',     
    'us-west-2',     
    'us-west-1',     
    'eu-west-1',     
    'eu-central-1',  
    'ap-south-1',    
    'ap-southeast-1',
    'ap-northeast-1',
    'ap-northeast-2',
    'ap-southeast-2',
    'sa-east-1',     
)                    

Do we have more regions now?

rebased onto e8bb003aa24db1ec202545ad378be1553e7eb7a4

9 months ago

We have nightly AMIs in two additional regions:
ca-central-1
eu-west-2

rebased onto 41b8936aaf72746038704b7b51542ef5367044b8

9 months ago

can we re-order these two (change perms first and then delete) and then add a nice comment about what is going on?

Done.

Looks good to me, one minor change may required:
we might want to make NIGHTLY lowercase at https://pagure.io/fork/sayanchowdhury/releng/blob/0ce51d0036c4b779e71efdc4fdfb0e8bae369107/f/scripts/clean-amis.py#_95 , since result of fedfind is somewhat like this for rawhide and F30 nightly:

>>> compose_info = fedfind.release.get_release(cid='Fedora-Rawhide-20190213.n.0')
>>> print compose_info.type
nightly

Also, this won't delete any of Fedora updates and updates testing AMIs, since for both cases compose_info.label starts with Update-*

(2*$days < item < $days) -> DELETE
($days < item < TODAY) -> CHANGE PERMISSIONS

for delete, its $days-10 < item < TODAY-10 -> DELETE

Looks good to me, one minor change may required:
we might want to make NIGHTLY lowercase at https://pagure.io/fork/sayanchowdhury/releng/blob/0ce51d0036c4b779e71efdc4fdfb0e8bae369107/f/scripts/clean-amis.py#_95 , since result of fedfind is somewhat like this for rawhide and F30 nightly:

compose_info = fedfind.release.get_release(cid='Fedora-Rawhide-20190213.n.0')
print compose_info.type
nightly

Yeah, NIGHTLY is the variable. The value of it is nightly.

Also, this won't delete any of Fedora updates and updates testing AMIs, since for both cases compose_info.label starts with Update-*

Yes, This is to prevent from deleting the 2WA Host released AMIs. I could not find a way to distinguish from the other AMIs.

Yeah, NIGHTLY is the variable. The value of it is nightly.

ah yes, missed it.

Also, this won't delete any of Fedora updates and updates testing AMIs, since for both cases compose_info.label starts with Update-*

Yes, This is to prevent from deleting the 2WA Host released AMIs. I could not find a way to distinguish from the other AMIs.

Compose ID of updates are usually Fedora-$releases-updates-$composedate and for updates-testing, it is Fedora-$releases-updates-testing-$composedate .
We do 2WA release from updates compose, so maybe we can filter out at compose ID level to remove AMIs from updates-testing?
@dustymabe WDYT

@sayanchowdhury One question for my clarity, does using topic org.fedoraproject.prod.fedimg.image.upload gets us to fetch AMIs for all desired regions we want AMIs to make private or delete or we should move to org.fedoraproject.prod.fedimg.image.publish topic where we have AMI IDs for all regions available?

Yeah, NIGHTLY is the variable. The value of it is nightly.

ah yes, missed it.

Also, this won't delete any of Fedora updates and updates testing AMIs, since for both cases compose_info.label starts with Update-*
Yes, This is to prevent from deleting the 2WA Host released AMIs. I could not find a way to distinguish from the other AMIs.

Compose ID of updates are usually Fedora-$releases-updates-$composedate and for updates-testing, it is Fedora-$releases-updates-testing-$composedate .
We do 2WA release from updates compose, so maybe we can filter out at compose ID level to remove AMIs from updates-testing?
@dustymabe WDYT

We can do this. But still updates composes would stay.

Let's define a new variable at the top of the script (WAIT_PERIOD=10)

i'm not sure if we should use days here at all. we basically want any AMIs that have had launch permissions removed for more than 10 days right? I almost wonder if we should be using fedmsg at all and not pure ec2 commands for this.

we should be able to remove updates-testing AMIs IMHO. Can we use any other identifying information?

looks like @sinnykumari already said this above :) - sorry @sinnykumari

If I upload an AMI today and this gets run today does that AMI get its launch permissions removed?

rebased onto 6a46caca939ed6620d2ed022b5b4bcbb097093a7

9 months ago

rebased onto 731affa16d9b413f1539a475d23b9180a2df87bc

9 months ago

rebased onto d41e4dabfbd4734f828decae3e3dadffbdaed7d7

9 months ago

@dustymabe I fixed the issue that you pointed out. Now AMIs $((today-5)-$days) < item < $(today-5) will be deleted.

Since the AMIs will be deleted after 10 days, so the $(today-15)-days < item < today -15

The benefit of using this doesn't seem to be worth it to me. We define more twice and we call it twice and all but one argument is static. Let's just call get_page() instead rather than making readers lookup what functools.partial does?

why should we include AMIs that have the date in them? What does it mean if they do have the date in them?

What does this mean? Add comment

these should be useful for the dry-run case, no?

Here are some slight modifications that would make this more useful I think:

diff --git a/scripts/clean-amis.py b/scripts/clean-amis.py
index 31d069c..b518089 100755
--- a/scripts/clean-amis.py
+++ b/scripts/clean-amis.py
@@ -265,8 +265,16 @@ if __name__ == '__main__':
         help="Delete the AMIs whose launch permissions have been removed",
         action="store_true", default=False)
     argument_parser.add_argument(
-        "--days",
-        help="Specify the number of days to run the operation",
+        "--howfarback",
+        help="How far back should we search for AMIs that need to be deleted",
+        type=int)
+    argument_parser.add_argument(
+        "--deletewaitperiod",
+        help="How long to wait after launch perms were removed before deleting",
+        type=int)
+    argument_parser.add_argument(
+        "--permswaitperiod",
+        help="Specify the number of days to wait before removing permissions",
         type=int)
     argument_parser.add_argument(
         "--change-perms",
@@ -284,9 +292,22 @@ if __name__ == '__main__':
     if args.delete and args.change_perms:
         print('Both the argument delete and change permission is not allowed')

-    days = 10
-    if args.days:
-        days = args.days
+    # Default to waiting 10 days after launch permissions have been
+    # removed before deleting
+    deletewaitperiod = 10
+    if args.deletewaitperiod:
+        deletewaitperiod
+
+    permswaitperiod = 10
+    if args.permswaitperiod:
+        permswaitperiod = args.permswaitperiod
+
+    # Ideally we could search through all the AMIs that ever were
+    # created but this would create quite the load on datagrepper.
+    # default to 4 weeks
+    howfarback = 28 # 4 weeks
+    if args.howfarback:
+        howfarback = args.howfarback

     # The AMIs deleted are the nightly AMIs that are uploaded via fedimg everyday.
     # The clean up of the AMIs happens through a cron job.
@@ -296,12 +317,20 @@ if __name__ == '__main__':
     # - If no issues are reported in 10 days, the AMIs are deleted permanently.

     if args.change_perms:
-        end = (datetime.now() - timedelta(days=5)).strftime('%s')
-        amis = _get_nightly_amis_nd(delta=86400 * days, end=int(end))
+        if howfarback < permswaitperiod:
+            raise Exception # shouldn't happen
+        end = (datetime.now() - timedelta(days=permswaitperiod)).strftime('%s')
+        amis = _get_nightly_amis_nd(delta=86400 * howfarback, end=int(end))
         change_amis_permission_nd(amis, dry_run=args.dry_run)

     if args.delete:
-        end = (datetime.now() - timedelta(days=15)).strftime('%s')
-        amis = _get_nightly_amis_nd(delta=86400 * days, end=int(end))
+        if howfarback < deletewaitperiod:
+            raise Exception # shouldn't happen
+        end = (datetime.now() - timedelta(days=deletewaitperiod)).strftime('%s')
+        amis = _get_nightly_amis_nd(delta=86400 * howfarback, end=int(end))
+        # Here we need to see how long it's been since we removed the
+        # permissions. If it has been deletewaitperiod days then we can
+        # delete. If there is no way to tell this organically then we
+        # could add a tag to the AMI with a timestamp at the same time
+        # we remove the permissions.
         delete_amis_nd(amis, dry_run=args.dry_run)
-

note the comment about possibly using tags set on the AMIs to determine when
the launch permissions were removed.

any updates for today for me to review?

1 new commit added

  • Tmp commit
9 months ago

rebased onto 95aca465273674f9b47c12160c620a317fe02d72

9 months ago

rebased onto 28179f2189a3dd32b5b2cfefa9a12d476f413139

9 months ago

rebased onto 2d0a536a680e1899acccad7a2e22bd8ac22d4efc

9 months ago

rebased onto 68aafd7315ccbfa5f51215b1a5a6db26b6bafa06

9 months ago

rebased onto 9beff8aacd778409a724b1845be4cb24a815668f

9 months ago

Specify the number of days worth of AMI fedmsg information to fetch from datagrepper.

probably need to raise an exception here?

I don't understand the 2nd if statement here and below for deletewaitperiod.

need description error message here

Need to fix function description and arg descrition here

This is a tag key/value pair we are defining here right? I would do something like Key=LaunchPermissionRevoked and Value=todaystimestamp.

this seems limiting - we will only delete images that were deleted on that day. We want to delete all the ones with permissions revoked before that day. I'm thinking we should get all images with this tag (key) and then filter the list of AMIs based on the timestamp (value).

"on or before that day" or "before that day"

This is when you want to delete same day images.

I don't know if I support deleting same day images but I guess it makes sense for development. The user should be able to pass --permswaitperiod 0, right? We should not need a special if statement for this.

"on or before that day" works for me if it does for you. but in general "off by one" shouldn't matter too much here.

I don't know if I support deleting same day images but I guess it makes sense for development. The user should be able to pass --permswaitperiod 0, right? We should not need a special if statement for this.

since it's 0, the if condition evaluated to false and the default value of 10 is set. And yes it's needed for development purpose.

"on or before that day" works for me if it does for you. but in general "off by one" shouldn't matter too much here.

+1

I don't know if I support deleting same day images but I guess it makes sense for development. The user should be able to pass --permswaitperiod 0, right? We should not need a special if statement for this.

since it's 0, the if condition evaluated to false and the default value of 10 is set. And yes it's needed for development purpose.

I see. In that case we should change things up a bit. Let's set the defaults for these values in argparse calls above and then just use args.days args.permswaitperiod and args.deletewaitperiod everywhere - WDYT?

I see. In that case we should change things up a bit. Let's set the defaults for these values in argparse calls above and then just use args.days args.permswaitperiod and args.deletewaitperiod everywhere - WDYT?

Sounds good to me

1 new commit added

  • Temp commit before squash
9 months ago

just checking - not sure what ami['Tags'] returns but i would think this should be tag.keys() and not tag.values.. Do you have an example of what tags=ami['Tags'] returns?

i would make the function args here generic.. ie timestamp1, timestamp2 and add a description: eturns true if timestamp1 is greater than or equal to timestamp2

maybe rename the function to _timestamp_greater() the readability of the later code benefits from this

this shouldn't happen. I would at least print out a warning message with information

just checking - not sure what ami['Tags'] returns but i would think this should be tag.keys() and not tag.values.. Do you have an example of what tags=ami['Tags'] returns?

[{'Key': 'LaunchPermissionRevoked', 'Value': '27022019'}]

List of dictionaries

1 new commit added

  • Another temp commit
9 months ago

add a comment before this if statement - something like

# The revoke timestamp allows us to tell how long ago an image had permissions removed
# If the permissions have been removed for shorter than the waiting period then we can't
# delete it yet.  

maybe we should change the function to be titled is_timestamp_older()

3 new commits added

  • Another temp commit
  • Temp commit before squash
  • Fix the clean ami script to remove the amis based on date
9 months ago

1 new commit added

  • Temp commit
9 months ago

i may have given you a bug - not sure if the string can span two lines here. maybe test it and see.

LGTM - other than the one comment.

@sinnykumari - can you review this in the morning?

1 new commit added

  • Format the script with black
9 months ago

rebased onto 9abfb34f4f2539febcb09ee4f608caae73f7e11f

9 months ago

i may have given you a bug - not sure if the string can span two lines here. maybe test it and see.

Yeah I had fixed that in the next commit.

rebased onto ba567ec22576d6304a0d6f028f2fbe7cb942ef43

9 months ago

Note that by this method we will be able to fetch TwoWeek compose id only for recent ones (from the time we added multi-arch support in twoweek.complete fedmsg topic). If we are interested to deleting most of the older composes, then we will have to add another condition to get composeID, sample json is https://apps.fedoraproject.org/datagrepper/id?id=2018-01994496-c703-49d6-8ffc-e99afd07c406&is_raw=true&size=extra-large

This idea came to my mind today as well, nice to see that this is already taken care of!

Added one concern above, other than that LGTM.

rebased onto 1d716c707effae0a0542a397eec5e74ab1a5beb6

9 months ago

2 new commits added

  • Format the script with black
  • Fix the clean ami script to remove the amis based on date
9 months ago

LGTM, have re-looked only into the modified content which I believe would be the "older-format fedmsg messages" section

need to set default value here

need to set default value here right?

can you verify that changing this from '-(\d{8}).' to "-(\d{8})." doesn't cause any problems?

1 new commit added

  • fixup! Fix the clean ami script to remove the amis based on date
9 months ago

Why is this seperated? Cant it be joined in the for loop below?

Use elif since same arch wont be repeated and less checks :smile:

these changes LGTM - can you address mohan's comments?

Same as the one that I asked above, why separated, rather than being the for loop below?

I have no idea on how delete amis and change ami permissions work, but other than that and the above comments I made, its LGTM.

No because the first query returns the number of pages. which is utilized in the next loop

1 new commit added

  • fixup! Fix the clean ami script to remove the amis based on date
9 months ago

LGTM. Let's squash and merge and then can you:

  1. do a test against dev account
  2. do a dry-run against prod account and share output with us in IRC?

rebased onto 44e1c85

9 months ago

Pull-Request has been merged by mohanboddu

9 months ago
Metadata