From c05da9848cdf3390694dfc548c7d09b874c93520 Mon Sep 17 00:00:00 2001 From: Todd Willey Date: Fri, 25 Jun 2010 22:03:45 -0400 Subject: Fixes based on code review 27001. --- nova/adminclient.py | 4 ++ nova/auth/users.py | 3 +- nova/compute/model.py | 141 ++++++++++++++++++------------------- nova/compute/node.py | 6 +- nova/endpoint/admin.py | 21 +++--- nova/tests/model_unittest.py | 164 ++++++++++++++++++++----------------------- nova/utils.py | 8 +-- run_tests.py | 2 +- 8 files changed, 170 insertions(+), 179 deletions(-) diff --git a/nova/adminclient.py b/nova/adminclient.py index 19dba28aa..480e907c9 100644 --- a/nova/adminclient.py +++ b/nova/adminclient.py @@ -46,9 +46,11 @@ class UserInfo(object): def __repr__(self): return 'UserInfo:%s' % self.username + # this is needed by the sax parser, so ignore the ugly name def startElement(self, name, attrs, connection): return None + # this is needed by the sax parser, so ignore the ugly name def endElement(self, name, value, connection): if name == 'username': self.username = str(value) @@ -78,9 +80,11 @@ class HostInfo(object): def __repr__(self): return 'Host:%s' % self.hostname + # this is needed by the sax parser, so ignore the ugly name def startElement(self, name, attrs, connection): return None + # this is needed by the sax parser, so ignore the ugly name def endElement(self, name, value, connection): setattr(self, name, value) diff --git a/nova/auth/users.py b/nova/auth/users.py index 1105fea82..b9d77f86f 100644 --- a/nova/auth/users.py +++ b/nova/auth/users.py @@ -328,7 +328,8 @@ class UserManager(object): user = self.get_user_from_access_key(access_key) if user == None: - raise exception.NotFound('No user found for access key %s' % access_key) + raise exception.NotFound('No user found for access key %s' % + access_key) if project_name is '': project_name = user.name diff --git a/nova/compute/model.py b/nova/compute/model.py index ddcb120e4..a4f0eac4d 100644 --- a/nova/compute/model.py +++ b/nova/compute/model.py @@ -56,9 +56,11 @@ from nova import utils FLAGS = flags.FLAGS + class ConnectionError(exception.Error): pass + def absorb_connection_error(fn): def _wrapper(*args, **kwargs): try: @@ -67,12 +69,13 @@ def absorb_connection_error(fn): raise ConnectionError(str(ce)) return _wrapper + # TODO(ja): singleton instance of the directory class InstanceDirectory(object): - """an api for interacting with the global state of instances """ + """an api for interacting with the global state of instances""" def get(self, instance_id): - """ returns an instance object for a given id """ + """returns an instance object for a given id""" return Instance(instance_id) def __getitem__(self, item): @@ -80,26 +83,26 @@ class InstanceDirectory(object): @absorb_connection_error def by_project(self, project): - """ returns a list of instance objects for a project """ + """returns a list of instance objects for a project""" for instance_id in datastore.Redis.instance().smembers('project:%s:instances' % project): yield Instance(instance_id) def by_node(self, node_id): - """ returns a list of instances for a node """ + """returns a list of instances for a node""" for instance in self.all: if instance['node_name'] == node_id: yield instance def by_ip(self, ip_address): - """ returns an instance object that is using the IP """ + """returns an instance object that is using the IP""" for instance in self.all: if instance['private_dns_name'] == ip_address: return instance return None def by_volume(self, volume_id): - """ returns the instance a volume is attached to """ + """returns the instance a volume is attached to""" pass @absorb_connection_error @@ -109,12 +112,12 @@ class InstanceDirectory(object): @property @absorb_connection_error def all(self): - """ returns a list of all instances """ + """returns a list of all instances""" for instance_id in datastore.Redis.instance().smembers('instances'): yield Instance(instance_id) def new(self): - """ returns an empty Instance object, with ID """ + """returns an empty Instance object, with ID""" instance_id = utils.generate_uid('i') return self.get(instance_id) @@ -129,13 +132,13 @@ class BasicModel(object): self.state = self.default_state() def default_state(self): - """ You probably want to define this in your subclass """ + """You probably want to define this in your subclass""" return {} @classmethod def lookup(cls, identifier): rv = cls(identifier) - if rv.new_record(): + if rv.is_new_record(): return None else: return rv @@ -143,7 +146,7 @@ class BasicModel(object): @classmethod @absorb_connection_error def all(cls): - """ yields all objects in the store """ + """yields all objects in the store""" redis_set = cls._redis_set_name(cls.__name__) for identifier in datastore.Redis.instance().smembers(redis_set): yield cls(identifier) @@ -162,15 +165,13 @@ class BasicModel(object): @classmethod def _redis_association_name(cls, foreign_type, foreign_id): - return cls._redis_set_name( - "%s:%s:%s" % - (foreign_type, foreign_id, cls.__name__) - ) + return cls._redis_set_name("%s:%s:%s" % + (foreign_type, foreign_id, cls.__name__)) @property def identifier(self): - """ You DEFINITELY want to define this in your subclass """ - raise Exception("Your sublcass should define identifier") + """You DEFINITELY want to define this in your subclass""" + raise NotImplementedError("Your sublcass should define identifier") @property def __redis_key(self): @@ -205,10 +206,10 @@ class BasicModel(object): return self.state[item] def __delitem__(self, item): - """ We don't support this """ + """We don't support this""" raise Exception("Silly monkey, models NEED all their properties.") - def new_record(self): + def is_new_record(self): return self.initial_state == {} @absorb_connection_error @@ -221,23 +222,24 @@ class BasicModel(object): set_name = self.__class__._redis_set_name(self.__class__.__name__) datastore.Redis.instance().srem(set_name, self.identifier) + @absorb_connection_error + def remove_from_index(self): + set_name = self.__class__._redis_set_name(self.__class__.__name__) + datastore.Redis.instance().srem(set_name, self.identifier) + @absorb_connection_error def associate_with(self, foreign_type, foreign_id): # note the extra 's' on the end is for plurality # to match the old data without requiring a migration of any sort self.add_associated_model_to_its_set(foreign_type, foreign_id) - redis_set = self.__class__._redis_association_name( - foreign_type, - foreign_id - ) + redis_set = self.__class__._redis_association_name(foreign_type, + foreign_id) datastore.Redis.instance().sadd(redis_set, self.identifier) @absorb_connection_error def unassociate_with(self, foreign_type, foreign_id): - redis_set = self.__class__._redis_association_name( - foreign_type, - foreign_id - ) + redis_set = self.__class__._redis_association_name(foreign_type, + foreign_id) datastore.Redis.instance().srem(redis_set, self.identifier) def add_associated_model_to_its_set(self, my_type, my_id): @@ -248,10 +250,9 @@ class BasicModel(object): my_inst = my_class(my_id) my_inst.save() else: - logging.warning( - "no model class for %s when building association from %s" % - (klsname, self) - ) + logging.warning("no model class for %s when building" + " association from %s", + klsname, self) @absorb_connection_error def save(self): @@ -276,32 +277,29 @@ class BasicModel(object): deletes all related records from datastore. does NOT do anything to running libvirt state. """ - logging.info( - "Destroying datamodel for %s %s", - (self.__class__.__name__, self.identifier) - ) + logging.info("Destroying datamodel for %s %s", + self.__class__.__name__, self.identifier) datastore.Redis.instance().delete(self.__redis_key) + self.remove_from_index() return True class Instance(BasicModel): - """ Wrapper around stored properties of an instance """ + """Wrapper around stored properties of an instance""" def __init__(self, instance_id): - """ loads an instance from the datastore if exists """ + """loads an instance from the datastore if exists""" # set instance data before super call since it uses default_state self.instance_id = instance_id super(Instance, self).__init__() def default_state(self): - return { - 'state': 0, - 'state_description': 'pending', - 'instance_id': self.instance_id, - 'node_name': 'unassigned', - 'project_id': 'unassigned', - 'user_id': 'unassigned' - } + return {'state': 0, + 'state_description': 'pending', + 'instance_id': self.instance_id, + 'node_name': 'unassigned', + 'project_id': 'unassigned', + 'user_id': 'unassigned'} @property def identifier(self): @@ -315,58 +313,55 @@ class Instance(BasicModel): @property def volumes(self): - """ returns a list of attached volumes """ + """returns a list of attached volumes""" pass @property def reservation(self): - """ Returns a reservation object """ + """Returns a reservation object""" pass def save(self): - """ Call into superclass to save object, then save associations """ - # XXX: doesn't track migration between projects, just adds the first one - should_update_project = self.new_record() + """Call into superclass to save object, then save associations""" + # NOTE(todd): doesn't track migration between projects, + # it just adds the first one + should_update_project = self.is_new_record() success = super(Instance, self).save() if success and should_update_project: self.associate_with("project", self.project) return True def destroy(self): - """ Destroy associations, then destroy the object """ + """Destroy associations, then destroy the object""" self.unassociate_with("project", self.project) return super(Instance, self).destroy() class Host(BasicModel): """ - A Host is the base machine that runs many virtualized Instance. - Hosts are usually controlled vi nova.compute.node.Node, this model - just stores stats about a host in redis. + A Host is the machine where a Daemon is running. """ def __init__(self, hostname): - """ loads an instance from the datastore if exists """ + """loads an instance from the datastore if exists""" # set instance data before super call since it uses default_state self.hostname = hostname super(Host, self).__init__() def default_state(self): - return { - "hostname": self.hostname - } + return {"hostname": self.hostname} @property def identifier(self): return self.hostname -class Worker(BasicModel): +class Daemon(BasicModel): """ - A Worker is a job (compute, api, network, ...) that runs on a host. + A Daemon is a job (compute, api, network, ...) that runs on a host. """ def __init__(self, host_or_combined, binpath=None): - """ loads an instance from the datastore if exists """ + """loads an instance from the datastore if exists""" # set instance data before super call since it uses default_state # since loading from datastore expects a combined key that # is equivilent to identifier, we need to expect that, while @@ -377,34 +372,34 @@ class Worker(BasicModel): self.binary = binpath else: self.hostname, self.binary = host_or_combined.split(":") - super(Worker, self).__init__() + super(Daemon, self).__init__() def default_state(self): - return { - "hostname": self.hostname, - "binary": self.binary, - "updated_at": utils.timestamp() - } + return {"hostname": self.hostname, + "binary": self.binary, + "updated_at": utils.isotime() + } @property def identifier(self): return "%s:%s" % (self.hostname, self.binary) def save(self): - """ Call into superclass to save object, then save associations """ - # XXX: doesn't clear out from host list after crash, termination, etc - success = super(Worker, self).save() + """Call into superclass to save object, then save associations""" + # NOTE(todd): this makes no attempt to destroy itsself, + # so after termination a record w/ old timestmap remains + success = super(Daemon, self).save() if success: self.associate_with("host", self.hostname) return True def destroy(self): - """ Destroy associations, then destroy the object """ + """Destroy associations, then destroy the object""" self.unassociate_with("host", self.hostname) - return super(Worker, self).destroy() + return super(Daemon, self).destroy() def heartbeat(self): - self['updated_at'] = utils.timestamp() + self['updated_at'] = utils.isotime() self.save() return True diff --git a/nova/compute/node.py b/nova/compute/node.py index b0f6173c9..faf1ddd15 100644 --- a/nova/compute/node.py +++ b/nova/compute/node.py @@ -142,9 +142,11 @@ class Node(object, service.Service): return retval @defer.inlineCallbacks - def report_state(self, hostname, worker): + def report_state(self, nodename, daemon): + # TODO(termie): Termie has an idea for wrapping this connection failure + # pattern to be more elegant. -todd try: - record = model.Worker(hostname, worker) + record = model.Daemon(nodename, daemon) record.heartbeat() if getattr(self, "model_disconnected", False): self.model_disconnected = False diff --git a/nova/endpoint/admin.py b/nova/endpoint/admin.py index 839cd9ad4..4471a4ef4 100644 --- a/nova/endpoint/admin.py +++ b/nova/endpoint/admin.py @@ -25,7 +25,7 @@ Admin API controller, exposed through http via the api worker. import base64 def user_dict(user, base64_file=None): - """ Convert the user object to a result dict """ + """Convert the user object to a result dict""" if user: return { 'username': user.id, @@ -37,16 +37,16 @@ def user_dict(user, base64_file=None): return {} def host_dict(host): - """ Convert a host model object to a result dict """ + """Convert a host model object to a result dict""" if host: return host.state else: return {} def admin_only(target): - """ Decorator for admin-only API calls """ + """Decorator for admin-only API calls""" def wrapper(*args, **kwargs): - """ Internal wrapper method for admin-only API calls """ + """Internal wrapper method for admin-only API calls""" context = args[1] if context.user.is_admin(): return target(*args, **kwargs) @@ -71,19 +71,18 @@ class AdminController(object): @admin_only def describe_user(self, _context, name, **_kwargs): - """ Returns user data, including access and secret keys. """ + """Returns user data, including access and secret keys.""" return user_dict(self.user_manager.get_user(name)) @admin_only def describe_users(self, _context, **_kwargs): - """ Returns all users - should be changed to deal with a list. """ + """Returns all users - should be changed to deal with a list.""" return {'userSet': [user_dict(u) for u in self.user_manager.get_users()] } @admin_only def register_user(self, _context, name, **_kwargs): - """ Creates a new user, and returns generated credentials. - """ + """Creates a new user, and returns generated credentials.""" return user_dict(self.user_manager.create_user(name)) @admin_only @@ -118,11 +117,9 @@ class AdminController(object): * DHCP servers running * Iptables / bridges """ - return {'hostSet': - [host_dict(h) for h in self.host_manager.all()] } + return {'hostSet': [host_dict(h) for h in self.host_manager.all()]} @admin_only def describe_host(self, _context, name, **_kwargs): - """Returns status info for single node. - """ + """Returns status info for single node.""" return host_dict(self.host_manager.lookup(name)) diff --git a/nova/tests/model_unittest.py b/nova/tests/model_unittest.py index 4eecdfeeb..23e2f9e73 100644 --- a/nova/tests/model_unittest.py +++ b/nova/tests/model_unittest.py @@ -26,12 +26,12 @@ from nova import utils from nova.compute import model from nova.compute import node + FLAGS = flags.FLAGS class ModelTestCase(test.TrialTestCase): def setUp(self): - logging.getLogger().setLevel(logging.DEBUG) super(ModelTestCase, self).setUp() self.flags(fake_libvirt=True, fake_storage=True, @@ -40,7 +40,7 @@ class ModelTestCase(test.TrialTestCase): def tearDown(self): model.Instance('i-test').destroy() model.Host('testhost').destroy() - model.Worker('testhost', 'nova-testworker').destroy() + model.Daemon('testhost', 'nova-testdaemon').destroy() def create_instance(self): inst = model.Instance('i-test') @@ -60,45 +60,45 @@ class ModelTestCase(test.TrialTestCase): host.save() return host - def create_worker(self): - worker = model.Worker('testhost', 'nova-testworker') - worker.save() - return worker + def create_daemon(self): + daemon = model.Daemon('testhost', 'nova-testdaemon') + daemon.save() + return daemon @defer.inlineCallbacks def test_create_instance(self): - """ store with create_instace, then test that a load finds it """ + """store with create_instace, then test that a load finds it""" instance = yield self.create_instance() old = yield model.Instance(instance.identifier) - self.assertEqual(False, old.new_record()) + self.assertFalse(old.is_new_record()) @defer.inlineCallbacks def test_delete_instance(self): - """ create, then destroy, then make sure loads a new record """ + """create, then destroy, then make sure loads a new record""" instance = yield self.create_instance() yield instance.destroy() newinst = yield model.Instance('i-test') - self.assertEqual(True, newinst.new_record()) + self.assertTrue(newinst.is_new_record()) @defer.inlineCallbacks def test_instance_added_to_set(self): - """ create, then check that it is listed for the project """ + """create, then check that it is listed for the project""" instance = yield self.create_instance() found = False for x in model.InstanceDirectory().all: if x.identifier == 'i-test': found = True - self.assertEqual(True, found) + self.assert_(found) @defer.inlineCallbacks def test_instance_associates_project(self): - """ create, then check that it is listed for the project """ + """create, then check that it is listed for the project""" instance = yield self.create_instance() found = False for x in model.InstanceDirectory().by_project(instance.project): if x.identifier == 'i-test': found = True - self.assertEqual(True, found) + self.assert_(found) @defer.inlineCallbacks def test_host_class_finds_hosts(self): @@ -112,102 +112,94 @@ class ModelTestCase(test.TrialTestCase): @defer.inlineCallbacks def test_create_host(self): - """ store with create_host, then test that a load finds it """ + """store with create_host, then test that a load finds it""" host = yield self.create_host() old = yield model.Host(host.identifier) - self.assertEqual(False, old.new_record()) + self.assertFalse(old.is_new_record()) @defer.inlineCallbacks def test_delete_host(self): - """ create, then destroy, then make sure loads a new record """ + """create, then destroy, then make sure loads a new record""" instance = yield self.create_host() yield instance.destroy() newinst = yield model.Host('testhost') - self.assertEqual(True, newinst.new_record()) + self.assertTrue(newinst.is_new_record()) @defer.inlineCallbacks def test_host_added_to_set(self): - """ create, then check that it is included in list """ + """create, then check that it is included in list""" instance = yield self.create_host() found = False for x in model.Host.all(): if x.identifier == 'testhost': found = True - self.assertEqual(True, found) - - @defer.inlineCallbacks - def test_create_worker_two_args(self): - """ create a worker with two arguments """ - w = yield self.create_worker() - self.assertEqual( - False, - model.Worker('testhost', 'nova-testworker').new_record() - ) - - @defer.inlineCallbacks - def test_create_worker_single_arg(self): - """ Create a worker using the combined host:bin format """ - w = yield model.Worker("testhost:nova-testworker") - w.save() - self.assertEqual( - False, - model.Worker('testhost:nova-testworker').new_record() - ) - - @defer.inlineCallbacks - def test_equality_of_worker_single_and_double_args(self): - """ Create a worker using the combined host:bin arg, find with 2 """ - w = yield model.Worker("testhost:nova-testworker") - w.save() - self.assertEqual( - False, - model.Worker('testhost', 'nova-testworker').new_record() - ) - - @defer.inlineCallbacks - def test_equality_worker_of_double_and_single_args(self): - """ Create a worker using the combined host:bin arg, find with 2 """ - w = yield self.create_worker() - self.assertEqual( - False, - model.Worker('testhost:nova-testworker').new_record() - ) - - @defer.inlineCallbacks - def test_delete_worker(self): - """ create, then destroy, then make sure loads a new record """ - instance = yield self.create_worker() + self.assert_(found) + + @defer.inlineCallbacks + def test_create_daemon_two_args(self): + """create a daemon with two arguments""" + d = yield self.create_daemon() + d = model.Daemon('testhost', 'nova-testdaemon') + self.assertFalse(d.is_new_record()) + + @defer.inlineCallbacks + def test_create_daemon_single_arg(self): + """Create a daemon using the combined host:bin format""" + d = yield model.Daemon("testhost:nova-testdaemon") + d.save() + d = model.Daemon('testhost:nova-testdaemon') + self.assertFalse(d.is_new_record()) + + @defer.inlineCallbacks + def test_equality_of_daemon_single_and_double_args(self): + """Create a daemon using the combined host:bin arg, find with 2""" + d = yield model.Daemon("testhost:nova-testdaemon") + d.save() + d = model.Daemon('testhost', 'nova-testdaemon') + self.assertFalse(d.is_new_record()) + + @defer.inlineCallbacks + def test_equality_daemon_of_double_and_single_args(self): + """Create a daemon using the combined host:bin arg, find with 2""" + d = yield self.create_daemon() + d = model.Daemon('testhost:nova-testdaemon') + self.assertFalse(d.is_new_record()) + + @defer.inlineCallbacks + def test_delete_daemon(self): + """create, then destroy, then make sure loads a new record""" + instance = yield self.create_daemon() yield instance.destroy() - newinst = yield model.Worker('testhost', 'nova-testworker') - self.assertEqual(True, newinst.new_record()) + newinst = yield model.Daemon('testhost', 'nova-testdaemon') + self.assertTrue(newinst.is_new_record()) @defer.inlineCallbacks - def test_worker_heartbeat(self): - """ Create a worker, sleep, heartbeat, check for update """ - w = yield self.create_worker() - ts = w['updated_at'] - yield time.sleep(2) - w.heartbeat() - w2 = model.Worker('testhost', 'nova-testworker') - ts2 = w2['updated_at'] - self.assertEqual(True, (ts2 > ts)) + def test_daemon_heartbeat(self): + """Create a daemon, sleep, heartbeat, check for update""" + d = yield self.create_daemon() + ts = d['updated_at'] + time.sleep(2) + d.heartbeat() + d2 = model.Daemon('testhost', 'nova-testdaemon') + ts2 = d2['updated_at'] + self.assert_(ts2 > ts) @defer.inlineCallbacks - def test_worker_added_to_set(self): - """ create, then check that it is included in list """ - instance = yield self.create_worker() + def test_daemon_added_to_set(self): + """create, then check that it is included in list""" + instance = yield self.create_daemon() found = False - for x in model.Worker.all(): - if x.identifier == 'testhost:nova-testworker': + for x in model.Daemon.all(): + if x.identifier == 'testhost:nova-testdaemon': found = True - self.assertEqual(True, found) + self.assert_(found) @defer.inlineCallbacks - def test_worker_associates_host(self): - """ create, then check that it is listed for the host """ - instance = yield self.create_worker() + def test_daemon_associates_host(self): + """create, then check that it is listed for the host""" + instance = yield self.create_daemon() found = False - for x in model.Worker.by_host('testhost'): - if x.identifier == 'testhost:nova-testworker': + for x in model.Daemon.by_host('testhost'): + if x.identifier == 'testhost:nova-testdaemon': found = True - self.assertEqual(True, found) + self.assertTrue(found) diff --git a/nova/utils.py b/nova/utils.py index e445a8bc4..094de5d74 100644 --- a/nova/utils.py +++ b/nova/utils.py @@ -29,7 +29,7 @@ import os.path import inspect import subprocess import random -import time +from datetime import datetime from nova import flags @@ -116,7 +116,7 @@ def get_my_ip(): csock.close() return addr -def timestamp(at=None): +def isotime(at=None): if not at: - at = time.gmtime() - return time.strftime("%Y-%m-%dT%H:%M:%SZ", at) + at = datetime.utcnow() + return at.strftime("%Y-%m-%dT%H:%M:%SZ") diff --git a/run_tests.py b/run_tests.py index 7d5e74887..91e886c76 100644 --- a/run_tests.py +++ b/run_tests.py @@ -53,6 +53,7 @@ from nova.tests.access_unittest import * from nova.tests.api_unittest import * from nova.tests.cloud_unittest import * from nova.tests.keeper_unittest import * +from nova.tests.model_unittest import * from nova.tests.network_unittest import * from nova.tests.node_unittest import * from nova.tests.objectstore_unittest import * @@ -61,7 +62,6 @@ from nova.tests.storage_unittest import * from nova.tests.users_unittest import * from nova.tests.datastore_unittest import * from nova.tests.validator_unittest import * -from nova.tests.model_unittest import * FLAGS = flags.FLAGS -- cgit