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

Revision history for this message
Gavin Panella (allenap) wrote :

This branch attempts to resolve a race condition when starting two or
more ec2test instances at similar times. What can happen is that,
during setup, one session can delete the key pair for the other
session, or delete the security group that the other session has just
created, because a static name ("ec2-test-runner") is used to name
these objects within AWS.

Now, with this branch, a session-specific name is used to name key
pairs and security groups so that sessions don't clobber each
other. The name contains some random data to give a good guarantee of
uniqueness.

However, one useful feature of using a static name is that old stuff
gets cleared up, so the new session name also contains a expiry
timestamp after which time the object can be deleted. The methods
delete_previous_security_groups() and delete_previous_key_pairs()
respect this timestamp.

I cleaned up a big pile of lint in revision 9611, and it's basically
noise next to this feature (though it still needs review). I've
prepared two diffs to help review, one containing only the lint and
another with only the feature changes.

== The features ==

Diff: http://pastebin.ubuntu.com/283667/

lib/devscripts/ec2test/builtins.py

  Create EC2SessionName instances based on EC2TestRunner.name instead
  of passing the name in directly.

lib/devscripts/ec2test/instance.py

  Check that the name is an instance of EC2SessionName, and call new
  method account.collect_garbage() in place of just
  account.delete_previous_key_pairs(). Incidentally, the former calls
  the latter.

lib/devscripts/ec2test/account.py

  Don't try to delete old security groups in
  acquire_security_group(). Removing old security groups is now done
  in delete_previous_security_groups().

  The method delete_previous_security_groups() and
  delete_previous_key_pairs() are very similar in the way they handle
  expiry timestamps.

lib/devscripts/ec2test/session.py
lib/devscripts/ec2test/tests/test_session.py

  Implementation and tests for EC2SessionName. This is a subclass of
  str with a few useful properties and a classmethod to aid in
  generating a good and unique name.

lib/devscripts/ec2test/utils.py
lib/devscripts/ec2test/tests/test_utils.py

  A few functions used to help with generating and parsing session
  names, and their tests.

  make_random_string() is a little unfortunate because there's a uuid
  module in Python 2.5 and beyond that does a better job of
  this. However, this script needs to support Python 2.4, so it'll
  stay for now. It does no harm really I guess.

== The lint fixes ==

http://pastebin.ubuntu.com/283676/

There is some outstanding lint in this branch:

  lib/devscripts/ec2test/builtins.py
    223: [W0102, cmd_test.run] Dangerous default value [] as argument
    289: [W0102, cmd_demo.run] Dangerous default value [] as argument
    363: [W0102, cmd_update_image.run] Dangerous default value [] as argument

  lib/devscripts/ec2test/instance.py
    408: [W0703, EC2Instance.set_up_and_run] Catch "Exception"

There aren't any tests for these scripts other than the ones I've
written in the branch, so I'm going to leave this lint in place
because changing them could affect behaviour. I'm build engineer for
another 3 weeks so I'll probably get to fixing these (and maybe even
add some tests) at some point.

== Testing and using it ==

utilities/ec2 test -o '-vvm devscripts.ec2test' &
utilities/ec2 test -o '-vvm devscripts.ec2test' &

« Back to merge proposal