bonclay7 / aws-amicleaner

Cleanup your old unused ami and related snapshots
MIT License
362 stars 132 forks source link

Wrong AMI deletion when using multiple values in --excluded-mapping-values option #95

Open adriananeci opened 6 years ago

adriananeci commented 6 years ago

When I try to pass multiple values for --excluded-mapping-values parameter I get duplicate AMIs marked for deletion on one hand and also multiple AMIs which should be excluded are wrongly added in AMIs to clean on the other hand. amicleaner --mapping-key tags --mapping-values Scope --excluded-mapping-values dev_ready test_ready --full-report --keep-previous 0

Assuming that I have 14 AMIs with 'testing' Scope tag and 14 AMis with 'dev_ready', when I try to cleanup all 'testing' AMIs I get 28(all 14 AMIs ID are duplicated) AMIs to be removed in testing group but also few AMIs in dev_ready group which is abnormal since I excluded 'dev_ready' Scope tags.

jhaage-jama commented 5 years ago

I ran into this same issue. It looks like the logic here is to see if the excluded_mapping_value is not in the mapping_value (i.e. not an excluded value) then add it to the candidates. The problem with the current logic is that each AMI needs to run through each excluded mapping value. For one or more of the excluded mapping values you pass in it won't match and then will add that AMI to the candidates list. It also has the unfortunate side effect of duplicating any AMIs that weren't going to be excluded in the first place.

I worked around this by changing the strategy and pre-populating the candidates list (same as it does when there are no --excluded-mapping-values provided) but then removing any AMIs from the list if they match any of the excluded values.

This seems to fix the two issues I was seeing: 1) Excluded AMIs still show up in the list of candidates to delete 2) Candidates to delete are duplicated

The bit of logic that handles this is in core.py, within the map_candidates method. Here's the section of this method I modified:

if mapping_strategy.get("excluded"): mapping_list = candidates_map.get(mapping_value) or [] mapping_list.append(ami) candidates_map[mapping_value] = mapping_list for excluded_mapping_value in mapping_strategy.get("excluded"): if excluded_mapping_value in mapping_value: print("Excluding AMI from candidates list based on exclusion: {}".format(mapping_value)) mapping_list = candidates_map.get(mapping_value) or [] mapping_list.remove(ami) candidates_map[mapping_value] = mapping_list