Code review comment for lp:~allenap/launchpad/ec2-test-race-bug-422433

Hi Gavin,

a very nice branch! Just one suggestion:

> + def delete_previous_security_groups(self):
> + """Delete previously used security groups, if found."""
> + def try_delete_group(group):
> + try:
> + group.delete()
> + except EC2ResponseError, e:
> + if e.code != 'InvalidGroup.InUse':
> + raise
> + self.log('Cannot delete; security group '
> + '%r in use.\n' % group.name)
> + now = datetime.utcnow()
> + for group in self.conn.get_all_security_groups():
> + session_name = EC2SessionName(group.name)
> + if session_name in (self.name, self.name.base):
> + self.log('Deleting security group %r\n' % group.name)
> + try_delete_group(group)
> + elif session_name.base == self.name.base:
> + if session_name.expires is None:
> + self.log('Found security group %r without creation '
> + 'date; leaving.\n' % group.name)
> + elif session_name.expires >= now:
> + self.log('Found recent security group %r; '
> + 'leaving\n' % group.name)
> + else:
> + self.log('Deleting expired security '
> + 'group %r\n' % group.name)
> + try_delete_group(group)
> + else:
> + self.log('Found other security group %r; '
> + 'leaving.\n' % group.name)
> +

> + def delete_previous_key_pairs(self):
> + """Delete previously used keypairs, if found."""
> + def try_delete_key_pair(key_pair):
> + try:
> + key_pair.delete()
> + except EC2ResponseError, e:
> + if e.code != 'InvalidKeyPair.NotFound':
> + if e.code == 'AuthFailure':
> + # Inserted because of previous support issue.
> + self.log(
> + 'POSSIBLE CAUSES OF ERROR:\n'
> + ' Did you sign up for EC2?\n'
> + ' Did you put a credit card number in your AWS '
> + 'account?\n'
> + 'Please doublecheck before reporting a '
> + 'problem.\n')
> + raise
> + self.log('Cannot delete; key pair not '
> + 'found %r\n' % key_pair.name)
> + now = datetime.utcnow()
> + for key_pair in self.conn.get_all_key_pairs():
> + session_name = EC2SessionName(key_pair.name)
> + if session_name in (self.name, self.name.base):
> + self.log('Deleting key pair %r\n' % key_pair.name)
> + try_delete_key_pair(key_pair)
> + elif session_name.base == self.name.base:
> + if session_name.expires is None:
> + self.log('Found key pair %r without creation date; '
> + 'leaving.\n' % key_pair.name)
> + elif session_name.expires >= now:
> + self.log('Found recent key pair %r; '
> + 'leaving\n' % key_pair.name)
> + else:
> + self.log('Deleting expired key pair %r\n' % key_pair.name)
> + try_delete_key_pair(key_pair)
> + else:
> + self.log('Found other key pair %r; '
> + 'leaving.\n' % key_pair.name)

The for loops in both methods are nearly identical; the only differences I see are that the log messages contain "key pair" or "secuity group", and that try_delete_key_pair() or try_delete_security_group() is called. Would it make sense to move the code starting at "if session_name" and ending at "else: self.log(..)" into a common method?