ore/aws: Handle missing SnapshotID

[gursewak1997]

Added a check for when the snapshotID is None in the RemoveBySnapshotTag function. If snapshotID is None, the function now attempts to extract the snapshot ID from the associated AMI. If the AMI cannot be described, it is logged as pruned, and the function returns without error.

[dustymabe]

I think rather than changing this function we should change the input --snapshot to accept a new special value for snapshot ID of detectFromAMI.

If this value was passed in then we would:

• First determine if AMI exists
• If AMI exists determine snapshot ID from it and set that value

then we can continue with the rest of the code

[dustymabe]

we would then change the calling GC code to pass in --snapshot detectFromAMI if no snapshot was set in the json.

[dustymabe]

just provide a default value to .get() and it will do what you want here.

>>> { 'name': 'abcdefg' }.get("name", 'default')
'abcdefg'
>>> { 'name': 'abcdefg' }.get("blank", 'default')
'default'

let's also add a comment here with a little more explanation on what we are doing.. i.e. If we are dealing with an old manifest where the snapshot ID isn't stored then let's instruct ore to detect the snapshot ID from the AMI.

[dustymabe]

maybe this should be

Suggested change

	if snapshotID == "detectFromAMI" {
	if snapshotID == "detectFromAMI" && amiID != "" {

[dustymabe]

I think we need to check the err != nil case, right?

[gursewak1997]

The function FindSnapshot has multiple scenarios. Basically, its either snapshot is nil or err!=nil if nothing is found.

[dustymabe]

the FindSnapshot function has several errors that it can throw:

• unable to describe snapshots:
• found multiple matching snapshots
• unable to describe import tasks
• couldn't describe snapshot from import task
• found multiple matching import tasks

in all of those cases you can't assume there is no snapshot. You can only say there is definitely no snapshot if snapshot and err are both nil.

[gursewak1997]

Understood, but they all mean we are not getting any snapshots from the AMI i.e. we won't be handling each scenario differently right?
From what I see, the scenario when we get a snapshot from AMI, the snapshot!=nil and err==nil. If anything else, it doesn't help us.

[gursewak1997]

I can possibly update the error message if needed. Something less aggressive like Failed to find any snapshot for ami-123

[dustymabe]

It depends on how we want to handle it. If there was an AMI then there definitely is a snapshot and if we can't find a snapshot ID then we probably just want to error here. I mean we don't expect for an error to happen so bubbling that up to the user would be useful? or at least printing a warning (probably the better option).

[dustymabe]

basically we should handle the err != nil case even if it's only informational:

if err != nil {
   print warning; do nothing
} else if snapshot == nil {
   print log message; do nothing
} else {
   snapshotID = snapshot
}

[gursewak1997]

	if err != nil {
		fmt.Fprintf(os.Stderr, "Warning: Unable to find any snapshot for %s.\n", amiID)
		os.Exit(0)
	} else if snapshot == nil {
		fmt.Fprintf(os.Stderr, "No valid snapshot found for %s.\n", amiID)
		os.Exit(0)
	}
	snapshotID = snapshot.SnapshotID

Something like this works? We shouldn't need the else statement since that would just mean that we have a valid snapshot.

[dustymabe]

that looks pretty good. I would actually say that we hit an error but we are going to continue. fmt.Fprintf(os.Stderr, "Warning: Encountered error when searching for snapshot for %s: %s. Continuing..\n", amiID, err)

also I wouldn't do os.Stderr for the non warning log message.

[dustymabe]

LGTM