Index: fedd/federation/protogeni_access.py
===================================================================
--- fedd/federation/protogeni_access.py	(revision 42cd8a754077fa679c180043a1da9080d486cc29)
+++ fedd/federation/protogeni_access.py	(revision 37ed9a57381d7cf01251d983372ff2bc901e4264)
@@ -366,8 +366,11 @@
 	    raise service_error(service_error.req, "No such allocation")
 
-    # Turn the manifest into a dict were each virtual nodename (i.e. the topdl
-    # name) has an entry with the allocated machine in hostname and the
-    # interfaces in 'interfaces'.  I love having XML parser code lying around.
     def manifest_to_dict(self, manifest, ignore_debug=False):
+	"""
+	Turn the manifest into a dict were each virtual nodename (i.e. the
+	topdl name) has an entry with the allocated machine in hostname and the
+	interfaces in 'interfaces'.  I love having XML parser code lying
+	around.
+	"""
 	if self.create_debug and not ignore_debug: 
 	    self.log.debug("Returning null manifest dict")
@@ -416,4 +419,8 @@
 
     def fake_manifest(self, topo):
+	"""
+	Fake the output of manifest_to_dict with a bunch of generic node an
+	interface names, for debugging.
+	"""
 	node = { }
 	for i, e in enumerate([ e for e in topo.elements \
@@ -648,9 +655,19 @@
 		    continue
 
-    def configure_nodes(self, segment_commands, topo, nodes, user, pubkey, secretkey, 
-	    stagingdir, tmpdir):
-
-	# These little functions/functors just make things more readable
+    def write_node_config_script(self, elem, node, user, pubkey,
+	    secretkey, stagingdir, tmpdir):
+	"""
+	Write out the configuration script that is to run on the node
+	represented by elem in the topology.  This is called
+	once per node to configure.
+	"""
+	# These little functions/functors just make things more readable.  Each
+	# one encapsulates a small task of copying software files or installing
+	# them.
 	class stage_file_type:
+	    """
+	    Write code copying file sfrom the staging host to the host on which
+	    this will run.
+	    """
 	    def __init__(self, user, host, stagingdir):
 		self.user = user
@@ -668,4 +685,7 @@
 
 	def install_tar(script, loc, base):
+	    """
+	    Print code to script to install a tarfile in loc.
+	    """
 	    tar = "/bin/tar"
 	    mkdir="/bin/mkdir"
@@ -675,7 +695,13 @@
 
 	def install_rpm(script, base):
+	    """
+	    Print code to script to install an rpm
+	    """
 	    rpm = "/bin/rpm"
 	    print >>script, "%s --install %s" % (rpm, base)
 
+	ifconfig = "/sbin/ifconfig"
+	stage_file = stage_file_type(user, self.staging_host, stagingdir)
+	pname = node.get('hostname', None)
 	fed_dir = "/usr/local/federation"
 	fed_etc_dir = "%s/etc" % fed_dir
@@ -683,29 +709,59 @@
 	fed_lib_dir = "%s/lib" % fed_dir
 
-	ifconfig = "/sbin/ifconfig"
-
-	stage_file = stage_file_type(user, self.staging_host, stagingdir)
-
-	for e in [ e for e in topo.elements if isinstance(e, topdl.Computer)]:
-	    vname = e.name
-	    node = nodes.get(vname, { })
-	    pname = node.get('hostname', None)
-	    if pname:
-		script = open("%s/%s.startup" %(tmpdir, pname), "w")
-		# Reset the interfaces to the ones in the topo file
-		for i in [ i for i in e.interface \
-			if not i.get_attribute('portal')]:
-		    pinf = node['interfaces'].get(i.name, None)
-		    addr = i.get_attribute('ip4_address') 
-		    netmask = i.get_attribute('ip4_netmask') or '255.255.255.0'
-		    if pinf and addr:
-			print >>script, \
-				"%s %s %s netmask %s"  % \
-				(ifconfig, pinf, addr, netmask)
-		    else:
-			self.log.error("Missing interface or address for %s" \
-				% i.name)
-		    
-		for l, f in self.federation_software:
+	if pname:
+	    sfile = "%s/%s.startup" % (tmpdir, pname)
+	    script = open(sfile, "w")
+	    # Reset the interfaces to the ones in the topo file
+	    for i in [ i for i in elem.interface \
+		    if not i.get_attribute('portal')]:
+		pinf = node['interfaces'].get(i.name, None)
+		addr = i.get_attribute('ip4_address') 
+		netmask = i.get_attribute('ip4_netmask') or '255.255.255.0'
+		if pinf and addr:
+		    print >>script, \
+			    "%s %s %s netmask %s"  % \
+			    (ifconfig, pinf, addr, netmask)
+		else:
+		    self.log.error("Missing interface or address for %s" \
+			    % i.name)
+		
+	    for l, f in self.federation_software:
+		base = os.path.basename(f)
+		stage_file(script, base)
+		if l: install_tar(script, l, base)
+		else: install_rpm(script, base)
+
+	    for s in elem.software:
+		s_base = s.location.rpartition('/')[2]
+		stage_file(script, s_base)
+		if s.install: install_tar(script, s.install, s_base)
+		else: install_rpm(script, s_base)
+
+	    for f in ('hosts', pubkey, secretkey, 'client.conf', 
+		    'userconf'):
+		stage_file(script, f, fed_etc_dir)
+	    if self.sshd:
+		stage_file(script, self.sshd, fed_bin_dir)
+	    if self.sshd_config:
+		stage_file(script, self.sshd_config, fed_etc_dir)
+
+	    # Look in tmpdir to get the names.  They've all been copied
+	    # into the (remote) staging dir
+	    if os.access("%s/%s.gw.conf" % (tmpdir, elem.name), os.R_OK):
+		stage_file(script, "%s.gw.conf" % elem.name, fed_etc_dir)
+
+	    # Hackery dackery dock: the ProtoGENI python is really ancient.
+	    # A modern version (though packaged for Mandrake (remember
+	    # Mandrake?  good times, good times)) should be in the
+	    # federation_software list, but we need to move rename is for
+	    # SEER.
+	    print >>script, "rm /usr/bin/python"
+	    print >>script, "ln /usr/bin/python2.4 /usr/bin/python"
+	    # Back to less hacky stuff
+
+	    # Start commands
+	    if elem.get_attribute('portal') and self.portal_startcommand:
+		# Install portal software
+		for l, f in self.portal_software:
 		    base = os.path.basename(f)
 		    stage_file(script, base)
@@ -713,55 +769,39 @@
 		    else: install_rpm(script, base)
 
-		for s in e.software:
-		    s_base = s.location.rpartition('/')[2]
-		    stage_file(script, s_base)
-		    if s.install: install_tar(script, s.install, s_base)
-		    else: install_rpm(script, s_base)
-
-		for f in ('hosts', pubkey, secretkey, 'client.conf', 
-			'userconf'):
-		    stage_file(script, f, fed_etc_dir)
-		if self.sshd:
-		    stage_file(script, self.sshd, fed_bin_dir)
-		if self.sshd_config:
-		    stage_file(script, self.sshd_config, fed_etc_dir)
-
-		# Look in tmpdir to get the names.  They've all been copied
-		# into the (remote) staging dir
-		if os.access("%s/%s.gw.conf" % (tmpdir, vname), os.R_OK):
-		    stage_file(script, "%s.gw.conf" % vname, fed_etc_dir)
-
-		# Hackery dackery dock: the ProtoGENI python is really ancient.
-		# A modern version (though packaged for Mandrake (remember
-		# Mandrake?  good times, good times)) should be in the
-		# federation_software list, but we need to move rename is for
-		# SEER.
-		print >>script, "rm /usr/bin/python"
-		print >>script, "ln /usr/bin/python2.4 /usr/bin/python"
-		# Back to less hacky stuff
-
-		# Start commands
-		if e.get_attribute('portal') and self.portal_startcommand:
-		    # Install portal software
-		    for l, f in self.portal_software:
-			base = os.path.basename(f)
-			stage_file(script, base)
-			if l: install_tar(script, l, base)
-			else: install_rpm(script, base)
-
-		    # Portals never have a user-specified start command
-		    print >>script, self.portal_startcommand
-		elif self.node_startcommand:
-		    # XXX: debug
-		    print >>script, "sudo perl -I%s %simport_key.pl /users/%s/.ssh/authorized_keys /root/.ssh/authorized_keys" % (fed_lib_dir, fed_bin_dir, user)
-		    # XXX: debug
-		    if e.get_attribute('startup'):
-			print >>script, "%s \\$USER '%s'" % \
-				(self.node_startcommand, e.get_attribute('startup'))
-		    else:
-			print >>script, self.node_startcommand
-		script.close()
-		if not segment_commands.scp_file("%s/%s.startup" % (tmpdir, pname), 
-			user, pname):
+		# Portals never have a user-specified start command
+		print >>script, self.portal_startcommand
+	    elif self.node_startcommand:
+		# XXX: debug
+		print >>script, "sudo perl -I%s %simport_key.pl /users/%s/.ssh/authorized_keys /root/.ssh/authorized_keys" % (fed_lib_dir, fed_bin_dir, user)
+		# XXX: debug
+		if elem.get_attribute('startup'):
+		    print >>script, "%s \\$USER '%s'" % \
+			    (self.node_startcommand,
+				    elem.get_attribute('startup'))
+		else:
+		    print >>script, self.node_startcommand
+	    script.close()
+	    return sfile, pname
+	else:
+	    return None, None
+
+
+    def configure_nodes(self, segment_commands, topo, nodes, user, 
+	    pubkey, secretkey, stagingdir, tmpdir):
+	"""
+	For each node in the topology, generate a script file that copies
+	software onto it and installs it in the proper places and then runs the
+	startup command (including the federation commands.
+	"""
+
+
+
+	for e in [ e for e in topo.elements if isinstance(e, topdl.Computer)]:
+	    vname = e.name
+	    sfile, pname = self.write_node_config_script(e,
+		    nodes.get(vname, { }),
+		    user, pubkey, secretkey, stagingdir, tmpdir)
+	    if sfile:
+		if not segment_commands.scp_file(sfile, user, pname):
 		    self.log.error("Could not copy script to %s" % pname)
 	    else:
@@ -769,4 +809,8 @@
 
     def start_node(self, user, host, node, segment_commands):
+	"""
+	Copy an identity to a node for the configuration script to be able to
+	import data and then run the startup script remotely.
+	"""
 	# Place an identity on the node so that the copying can succeed
 	segment_commands.ssh_cmd(user, host, "scp .ssh/id_rsa %s:.ssh" % node)
@@ -775,4 +819,8 @@
 
     def start_nodes(self, user, host, nodes, segment_commands):
+	"""
+	Start a thread to initialize each node and wait for them to complete.
+	Each thread runs start_node.
+	"""
 	threads = [ ]
 	for n in nodes:
@@ -788,35 +836,11 @@
 	    done = [not t.isAlive() for t in threads]
 
-
-
-
-    def start_segment(self, segment_commands, aid, user, rspec, pubkey,
-	    secretkey, ename, stagingdir, tmpdir, certfile, certpw,
-	    export_certfile, topo, connInfo, services, timeout=0):
-	"""
-	Start a sub-experiment on a federant.
-
-	Get the current state, modify or create as appropriate, ship data
-	and configs and start the experiment.  There are small ordering
-	differences based on the initial state of the sub-experiment.
-	"""
-
-	def random_slicename(user):
-	    slicename = user
-	    for i in range(0,5):
-		slicename += random.choice(string.ascii_letters)
-	    return slicename
-
-	host = self.staging_host
-	if not os.access(certfile, os.R_OK):
-	    self.log.error("[start_segment]: Cannot read certfile: %s" % \
-		    certfile)
-	    return False
-	ctxt = fedd_ssl_context(my_cert=certfile, password=certpw)
-	# Local software dir
-	lsoftdir = "%s/software" % tmpdir
-
-	# Open up a temporary file to contain a script for setting up the
-	# filespace for the new experiment.
+    def set_up_staging_filespace(self, segment_commands, user, host,
+	    stagingdir):
+	"""
+	Set up teh staging area on the staging machine.  To reduce the number
+	of ssh commands, we compose a script and execute it remotely.
+	"""
+
 	self.log.info("[start_segment]: creating script file")
 	try:
@@ -845,4 +869,16 @@
 	    return False
 
+    def initialize_protogeni_context(self, segment_commands, certfile, certpw):
+	"""
+	Protogeni interactions take a context and a protogeni certificate.
+	This establishes both for later calls and returns them.
+	"""
+	if os.access(certfile, os.R_OK):
+	    ctxt = fedd_ssl_context(my_cert=certfile, password=certpw)
+	else:
+	    self.log.error("[start_segment]: Cannot read certfile: %s" % \
+		    certfile)
+	    return None, None
+
 	try:
 	    gcred = segment_commands.pg_call(self.sa_url, 
@@ -851,6 +887,22 @@
 	    raise service_error(service_error.federant,
 		    "ProtoGENI: %s" % e)
-	# Find a slicename not in use
-	slicename = "fabereGpgL"
+
+	return ctxt, gcred
+
+    def get_free_slicename(self, segment_commands, user, gcred, ctxt):
+	"""
+	Find a usable slice name by trying random ones until there's no
+	collision.
+	"""
+
+	def random_slicename(user):
+	    """
+	    Return a random slicename by appending 5 letters to the username.
+	    """
+	    slicename = user
+	    for i in range(0,5):
+		slicename += random.choice(string.ascii_letters)
+	    return slicename
+
 	while True:
 	    slicename = random_slicename(user)
@@ -866,11 +918,13 @@
 		break
 
-	self.log.info("Creating %s" % slicename)
-	f = open("./rspec", "w")
-	print >>f, "%s" % rspec
-	f.close()
-	# Create the slice and allocate resources.  If any of this stuff fails,
-	# the allocations will time out on PG in short order, so we just raise
-	# the service_error.
+	return slicename
+
+    def allocate_slice(self, segment_commands, slicename, rspec, gcred, ctxt):
+	"""
+	Create the slice and allocate resources.  If any of this stuff fails,
+	the allocations will time out on PG in short order, so we just raise
+	the service_error.  Return the slice and sliver credentials as well as
+	the manifest.
+	"""
 	try:
 	    param = {
@@ -879,5 +933,6 @@
 		    'type': 'Slice'
 		    }
-	    slice_cred = segment_commands.pg_call(self.sa_url, 'Register', param, ctxt)
+	    slice_cred = segment_commands.pg_call(self.sa_url, 'Register',
+		    param, ctxt)
 	    f = open("./slice_cred", "w")
 	    print >>f, slice_cred
@@ -887,5 +942,6 @@
 		    'credential': gcred, 
 		    }
-	    keys =  segment_commands.pg_call(self.sa_url, 'GetKeys', param, ctxt)
+	    keys =  segment_commands.pg_call(self.sa_url, 'GetKeys', param, 
+		    ctxt)
 	    # Grab and redeem a ticket
 	    param = {
@@ -893,5 +949,6 @@
 		    'rspec': rspec,
 		    }
-	    ticket = segment_commands.pg_call(self.cm_url, 'GetTicket', param, ctxt)
+	    ticket = segment_commands.pg_call(self.cm_url, 'GetTicket', param,
+		    ctxt)
 	    f = open("./ticket", "w")
 	    print >>f, ticket
@@ -919,4 +976,68 @@
 		    "ProtoGENI: %s %s" % (e.code, e))
 
+	return (slice_cred, sliver_cred, manifest)
+
+    def wait_for_slice(self, segment_commands, slice_cred, ctxt):
+	"""
+	Wait for the given slice to finish its startup.  Return the final
+	status.
+	"""
+	status = 'notready'
+	try:
+	    while status == 'notready':
+		param = { 
+			'credential': slice_cred
+			}
+		r = segment_commands.pg_call(self.cm_url,
+			'SliceStatus', param, ctxt)
+		status = r.get('status', 'notready')
+		if status == 'notready':
+		    time.sleep(30)
+	except segment_commands.ProtoGENIError, e:
+	    raise service_error(service_error.federant,
+		    "ProtoGENI: %s %s" % (e.code, e))
+
+	return status
+
+    def delete_slice(self, segment_commands, slice_cred, ctxt):
+	"""
+	Delete the slice resources.  An error from the service is ignores,
+	because the soft state will go away anyway.
+	"""
+	try:
+	    param = { 'credential': slice_cred }
+	    segment_commands.pg_call(self.cm_url, 'DeleteSliver',
+		    param, ctxt)
+	except segment_commands.ProtoGENIError, e:
+	    self.log.warn("ProtoGENI: %s" % e)
+
+
+
+    def start_segment(self, segment_commands, aid, user, rspec, pubkey,
+	    secretkey, ename, stagingdir, tmpdir, certfile, certpw,
+	    export_certfile, topo, connInfo, services, timeout=0):
+	"""
+	Start a sub-experiment on a federant.
+
+	Get the current state, modify or create as appropriate, ship data
+	and configs and start the experiment.  There are small ordering
+	differences based on the initial state of the sub-experiment.
+	"""
+
+	# Local software dir
+	lsoftdir = "%s/software" % tmpdir
+	host = self.staging_host
+
+	ctxt, gcred = self.initialize_protogeni_context(segment_commands, 
+		certfile, certpw)
+
+	if not ctxt: return False
+
+	self.set_up_staging_filespace(segment_commands, user, host, stagingdir)
+	slicename = self.get_free_slicename(segment_commands, user, gcred, ctxt)
+	self.log.info("Creating %s" % slicename)
+	slice_cred, sliver_cred, manifest = self.allocate_slice(
+		segment_commands, slicename, rspec, gcred, ctxt)
+
 	# With manifest in hand, we can export the portal node names.
 	if self.create_debug: nodes = self.fake_manifest(topo)
@@ -941,31 +1062,11 @@
 
 	# Now we wait for the nodes to start on PG
-	status = 'notready'
-	try:
-	    while status == 'notready':
-		param = { 
-			'credential': slice_cred
-			}
-		r = segment_commands.pg_call(self.cm_url, 'SliceStatus', param, ctxt)
-		print r
-		status = r.get('status', 'notready')
-		if status == 'notready':
-		    time.sleep(30)
-	except segment_commands.ProtoGENIError, e:
-	    raise service_error(service_error.federant,
-		    "ProtoGENI: %s %s" % (e.code, e))
-
+	status = self.wait_for_slice(segment_commands, slice_cred, ctxt)
 	if status == 'failed':
 	    self.log.error('Sliver failed to start on ProtoGENI')
-	    try:
-		param = { 
-			'credential': slice_cred
-			}
-		segment_commands.pg_call(self.cm_url, 'DeleteSliver', param, ctxt)
-	    except segment_commands.ProtoGENIError, e:
-		raise service_error(service_error.federant,
-		    "ProtoGENI: %s" % e)
+	    self.delete_slice(segment_commands, slice_cred, ctxt)
 	    return False
 	else:
+	    # All good: save ProtoGENI info in shared state
 	    self.state_lock.acquire()
 	    self.allocation[aid]['slice_name'] = slicename
@@ -979,6 +1080,6 @@
 
 	# Now we have configuration to do for ProtoGENI
-	self.configure_nodes(segment_commands, topo, nodes, user, pubkey, secretkey, 
-		stagingdir, tmpdir)
+	self.configure_nodes(segment_commands, topo, nodes, user, pubkey,
+		secretkey, stagingdir, tmpdir)
 
 	self.start_nodes(user, self.staging_host, 
@@ -1229,5 +1330,5 @@
 		    "%s/%s" % (self.staging_dir, ename), tmpdir, cf, cpw,
 		    certfile, topo, connInfo, services)
-	except EnvironmentError:
+	except EnvironmentError, e:
 	    err = service_error(service_error.internal, "%s" % e)
 	except service_error, e:
@@ -1264,13 +1365,5 @@
 		self.log.error('Removing Sliver on ProtoGENI')
 		ctxt = fedd_ssl_context(my_cert=certfile, password=certpw)
-		try:
-		    param = { 
-			    'credential': slice_cred
-			    }
-		    segment_commands.pg_call(self.cm_url, 'DeleteSlice',
-			    param, ctxt)
-		except segment_commands.ProtoGENIError, e:
-		    raise service_error(service_error.federant,
-			"ProtoGENI: %s" % e)
+		self.delete_slice(segment_commands, slice_cred, ctxt)
 	    return True
 	except self.ssh_cmd_timeout:
@@ -1315,9 +1408,13 @@
     def renew_segment(self, segment_commands, name, scred, interval, 
 	    certfile, certpw):
+	"""
+	Linear code through the segment renewal calls.
+	"""
 	ctxt = fedd_ssl_context(my_cert=certfile, password=certpw)
 	try:
 	    expiration = time.strftime("%Y%m%dT%H:%M:%S",
 		    time.gmtime(time.time() + interval))
-	    cred = segment_commands.pg_call(self.sa_url, 'GetCredential', {}, ctxt)
+	    cred = segment_commands.pg_call(self.sa_url, 'GetCredential', 
+		    {}, ctxt)
 
 	    param = {
@@ -1331,5 +1428,6 @@
 		    'type': 'Slice',
 		    }
-	    slice = segment_commands.pg_call(self.sa_url, 'Resolve', param, ctxt)
+	    slice = segment_commands.pg_call(self.sa_url, 'Resolve', 
+		    param, ctxt)
 	    uuid = slice.get('uuid', None)
 	    if uuid == None:
@@ -1342,5 +1440,6 @@
 		    'type': 'Slice',
 		    }
-	    new_scred = segment_commands.pg_call(self.sa_url, 'GetCredential', param, ctxt)
+	    new_scred = segment_commands.pg_call(self.sa_url, 'GetCredential',
+		    param, ctxt)
 	    f = open('./new_slice_cred', 'w')
 	    print >>f, new_scred
