diff --git a/openstack/cloud/_compute.py b/openstack/cloud/_compute.py index a883ad3c7..942d76b04 100644 --- a/openstack/cloud/_compute.py +++ b/openstack/cloud/_compute.py @@ -39,6 +39,11 @@ class ComputeCloudMixin(_normalize.Normalizer): self._servers_time = 0 self._servers_lock = threading.Lock() + @property + def _compute_region(self): + # This is only used in exception messages. Can we get rid of it? + return self.config.get_region_name('compute') + def get_flavor_name(self, flavor_id): flavor = self.get_flavor(flavor_id, get_extra=False) if flavor: @@ -896,7 +901,7 @@ class ComputeCloudMixin(_normalize.Normalizer): 'Network {network} is not a valid network in' ' {cloud}:{region}'.format( network=network, - cloud=self.name, region=self.config.region_name)) + cloud=self.name, region=self._compute_region)) nics.append({'net-id': network_obj['id']}) kwargs['nics'] = nics @@ -1025,7 +1030,7 @@ class ComputeCloudMixin(_normalize.Normalizer): 'Volume {boot_volume} is not a valid volume' ' in {cloud}:{region}'.format( boot_volume=boot_volume, - cloud=self.name, region=self.config.region_name)) + cloud=self.name, region=self._compute_region)) block_mapping = { 'boot_index': '0', 'delete_on_termination': terminate_volume, @@ -1046,7 +1051,7 @@ class ComputeCloudMixin(_normalize.Normalizer): 'Image {image} is not a valid image in' ' {cloud}:{region}'.format( image=image, - cloud=self.name, region=self.config.region_name)) + cloud=self.name, region=self._compute_region)) block_mapping = { 'boot_index': '0', @@ -1076,7 +1081,7 @@ class ComputeCloudMixin(_normalize.Normalizer): 'Volume {volume} is not a valid volume' ' in {cloud}:{region}'.format( volume=volume, - cloud=self.name, region=self.config.region_name)) + cloud=self.name, region=self._compute_region)) block_mapping = { 'boot_index': '-1', 'delete_on_termination': False, diff --git a/openstack/cloud/_normalize.py b/openstack/cloud/_normalize.py index c5002247f..c5f9d760a 100644 --- a/openstack/cloud/_normalize.py +++ b/openstack/cloud/_normalize.py @@ -563,7 +563,10 @@ class Normalizer(object): ret['config_drive'] = config_drive ret['project_id'] = project_id ret['tenant_id'] = project_id - ret['region'] = self.config.region_name + # TODO(efried): This is hardcoded to 'compute' because this method + # should only ever be used by the compute proxy. (That said, it + # doesn't appear to be used at all, so can we get rid of it?) + ret['region'] = self.config.get_region_name('compute') ret['cloud'] = self.config.name ret['az'] = az for key, val in ret['properties'].items(): diff --git a/openstack/cloud/meta.py b/openstack/cloud/meta.py index 1312d00d9..0ff6fb1dc 100644 --- a/openstack/cloud/meta.py +++ b/openstack/cloud/meta.py @@ -340,7 +340,9 @@ def _get_interface_ip(cloud, server): def get_groups_from_server(cloud, server, server_vars): groups = [] - region = cloud.config.region_name + # NOTE(efried): This is hardcoded to 'compute' because this method is only + # used from ComputeCloudMixin. + region = cloud.config.get_region_name('compute') cloud_name = cloud.name # Create a group for the cloud diff --git a/openstack/cloud/openstackcloud.py b/openstack/cloud/openstackcloud.py index 1468a14fd..5f37ad929 100755 --- a/openstack/cloud/openstackcloud.py +++ b/openstack/cloud/openstackcloud.py @@ -358,7 +358,7 @@ class _OpenStackCloudMixin(object): service_name=self.config.get_service_name(service_type), interface=self.config.get_interface(service_type), endpoint_override=self.config.get_endpoint(service_type), - region_name=self.config.region_name, + region_name=self.config.get_region_name(service_type), statsd_prefix=self.config.get_statsd_prefix(), statsd_client=self.config.get_statsd_client(), prometheus_counter=self.config.get_prometheus_counter(), @@ -374,7 +374,7 @@ class _OpenStackCloudMixin(object): service_name=self.config.get_service_name(service_type), interface=self.config.get_interface(service_type), endpoint_override=self.config.get_endpoint(service_type), - region_name=self.config.region_name, + region_name=self.config.get_region_name(service_type), min_version=min_version, max_version=max_version) @@ -413,7 +413,7 @@ class _OpenStackCloudMixin(object): interface=self.config.get_interface(service_type), endpoint_override=self.config.get_endpoint( service_type) or endpoint_override, - region_name=self.config.region_name) + region_name=self.config.get_region_name(service_type)) def _is_client_version(self, client, version): client_name = '_{client}_client'.format(client=client) @@ -529,7 +529,9 @@ class _OpenStackCloudMixin(object): def _get_current_location(self, project_id=None, zone=None): return munch.Munch( cloud=self.name, - region_name=self.config.region_name, + # TODO(efried): This is wrong, but it only seems to be used in a + # repr; can we get rid of it? + region_name=self.config.get_region_name(), zone=zone, project=self._get_project_info(project_id), ) @@ -649,7 +651,8 @@ class _OpenStackCloudMixin(object): return self.name def get_region(self): - return self.config.region_name + # TODO(efried): This seems to be unused. Can we get rid of it? + return self.config.get_region_name() def get_session_endpoint(self, service_key): try: @@ -666,7 +669,7 @@ class _OpenStackCloudMixin(object): " {error}".format( service=service_key, cloud=self.name, - region=self.config.region_name, + region=self.config.get_region_name(service_key), error=str(e))) return endpoint diff --git a/openstack/config/cloud_region.py b/openstack/config/cloud_region.py index 044669fee..766a3b901 100644 --- a/openstack/config/cloud_region.py +++ b/openstack/config/cloud_region.py @@ -127,8 +127,6 @@ def from_conf(conf, session=None, **kwargs): raise exceptions.ConfigException("A Session must be supplied.") config_dict = kwargs.pop('config', config_defaults.get_defaults()) stm = os_service_types.ServiceTypes() - # TODO(mordred) Think about region_name here - region_name = kwargs.pop('region_name', None) for st in stm.all_types_by_service_type: project_name = stm.get_project_name(st) if project_name not in conf: @@ -143,14 +141,10 @@ def from_conf(conf, session=None, **kwargs): continue # Load them into config_dict under keys prefixed by ${service_type}_ for raw_name, opt_val in opt_dict.items(): - if raw_name == 'region_name': - region_name = opt_val - continue config_name = '_'.join([st, raw_name]) config_dict[config_name] = opt_val return CloudRegion( - session=session, region_name=region_name, config=config_dict, - **kwargs) + session=session, config=config_dict, **kwargs) class CloudRegion(object): @@ -158,6 +152,28 @@ class CloudRegion(object): A CloudRegion encapsulates the config information needed for connections to all of the services in a Region of a Cloud. + + TODO(efried): Doc the rest of the kwargs + + :param str region_name: + The default region name for all services in this CloudRegion. If + both ``region_name`` and ``config['region_name'] are specified, the + kwarg takes precedence. May be overridden for a given ${service} + via a ${service}_region_name key in the ``config`` dict. + :param dict config: + A dict of configuration values for the CloudRegion and its + services. The key for a ${config_option} for a specific ${service} + should be ${service}_${config_option}. For example, to configure + the endpoint_override for the block_storage service, the ``config`` + dict should contain:: + + 'block_storage_endpoint_override': 'http://...' + + To provide a default to be used if no service-specific override is + present, just use the unprefixed ${config_option} as the service + key, e.g.:: + + 'interface': 'public' """ def __init__(self, name=None, region_name=None, config=None, force_ipv4=False, auth_plugin=None, @@ -170,8 +186,12 @@ class CloudRegion(object): statsd_host=None, statsd_port=None, statsd_prefix=None, collector_registry=None): self._name = name - self.region_name = region_name self.config = _util.normalize_keys(config) + # NOTE(efried): For backward compatibility: a) continue to accept the + # region_name kwarg; b) make it take precedence over (non-service_type- + # specific) region_name set in the config dict. + if region_name is not None: + self.config['region_name'] = region_name self._extra_config = extra_config or {} self.log = _log.setup_logging('openstack.config') self._force_ipv4 = force_ipv4 @@ -213,7 +233,8 @@ class CloudRegion(object): def __eq__(self, other): return ( self.name == other.name - and self.region_name == other.region_name + # Ew + and self.get_region_name() == other.get_region_name() and self.config == other.config) def __ne__(self, other): @@ -236,12 +257,13 @@ class CloudRegion(object): Always returns a valid string. It will have name and region_name or just one of the two if only one is set, or else 'unknown'. """ - if self.name and self.region_name: - return ":".join([self.name, self.region_name]) - elif self.name and not self.region_name: + region_name = self.get_region_name() + if self.name and region_name: + return ":".join([self.name, region_name]) + elif self.name and not region_name: return self.name - elif not self.name and self.region_name: - return self.region_name + elif not self.name and region_name: + return region_name else: return 'unknown' @@ -335,6 +357,12 @@ class CloudRegion(object): if st in config_dict: return config_dict[st] + def get_region_name(self, service_type=None): + # If a region_name for the specific service_type is configured, use it; + # else use the one configured for the CloudRegion as a whole. + return self._get_config( + 'region_name', service_type, fallback_to_unprefixed=True) + def get_interface(self, service_type=None): return self._get_config( 'interface', service_type, fallback_to_unprefixed=True) @@ -523,12 +551,13 @@ class CloudRegion(object): # Seriously. Don't think about the existential crisis # that is the next line. You'll wind up in cthulhu's lair. service_type = self.get_service_type(service_type) + region_name = self.get_region_name(service_type) versions = self.get_session().get_all_version_data( service_type=service_type, interface=self.get_interface(service_type), - region_name=self.region_name, + region_name=region_name, ) - region_versions = versions.get(self.region_name, {}) + region_versions = versions.get(region_name, {}) interface_versions = region_versions.get( self.get_interface(service_type), {}) return interface_versions.get(service_type, []) @@ -539,7 +568,7 @@ class CloudRegion(object): service_type=self.get_service_type(service_type), service_name=self.get_service_name(service_type), interface=self.get_interface(service_type), - region_name=self.region_name, + region_name=self.get_region_name(service_type), ) endpoint = adapter.get_endpoint() if not endpoint.rstrip().rsplit('/')[-1] == 'v2.0': @@ -567,6 +596,7 @@ class CloudRegion(object): """ version_request = self._get_version_request(service_type, version) + kwargs.setdefault('region_name', self.get_region_name(service_type)) kwargs.setdefault('connect_retries', self.get_connect_retries(service_type)) kwargs.setdefault('status_code_retries', @@ -599,7 +629,6 @@ class CloudRegion(object): service_type=self.get_service_type(service_type), service_name=self.get_service_name(service_type), interface=self.get_interface(service_type), - region_name=self.region_name, version=version, min_version=min_api_version, max_version=max_api_version, @@ -666,6 +695,7 @@ class CloudRegion(object): if override_endpoint: return override_endpoint + region_name = self.get_region_name(service_type) service_name = self.get_service_name(service_type) interface = self.get_interface(service_type) session = self.get_session() @@ -680,7 +710,7 @@ class CloudRegion(object): # the request endpoint = session.get_endpoint( service_type=service_type, - region_name=self.region_name, + region_name=region_name, interface=interface, service_name=service_name, **version_kwargs @@ -695,7 +725,7 @@ class CloudRegion(object): service_type, service_name, interface, - self.region_name, + region_name, ) return endpoint diff --git a/openstack/service_description.py b/openstack/service_description.py index f154e92f8..d2292b4b4 100644 --- a/openstack/service_description.py +++ b/openstack/service_description.py @@ -178,20 +178,21 @@ class ServiceDescription(object): ) found_version = temp_adapter.get_api_major_version() if found_version is None: + region_name = instance.config.get_region_name(self.service_type) if version_kwargs: raise exceptions.NotSupported( "The {service_type} service for {cloud}:{region_name}" " exists but does not have any supported versions.".format( service_type=self.service_type, cloud=instance.name, - region_name=instance.config.region_name)) + region_name=region_name)) else: raise exceptions.NotSupported( "The {service_type} service for {cloud}:{region_name}" " exists but no version was discoverable.".format( service_type=self.service_type, cloud=instance.name, - region_name=instance.config.region_name)) + region_name=region_name)) proxy_class = self.supported_versions.get(str(found_version[0])) if not proxy_class: # Maybe openstacksdk is being used for the passthrough diff --git a/openstack/tests/unit/cloud/test_meta.py b/openstack/tests/unit/cloud/test_meta.py index c1ee49946..410d7cf71 100644 --- a/openstack/tests/unit/cloud/test_meta.py +++ b/openstack/tests/unit/cloud/test_meta.py @@ -25,7 +25,9 @@ PUBLIC_V6 = '2001:0db8:face:0da0:face::0b00:1c' # rfc3849 class FakeConfig(object): - region_name = 'test-region' + def get_region_name(self, service_type=None): + # TODO(efried): Validate service_type? + return 'test-region' class FakeCloud(object): diff --git a/openstack/tests/unit/cloud/test_operator.py b/openstack/tests/unit/cloud/test_operator.py index 1dee45e0d..0c4c84fe2 100644 --- a/openstack/tests/unit/cloud/test_operator.py +++ b/openstack/tests/unit/cloud/test_operator.py @@ -74,7 +74,7 @@ class TestOperatorCloud(base.TestCase): session_mock.get_endpoint.side_effect = side_effect get_session_mock.return_value = session_mock self.cloud.name = 'testcloud' - self.cloud.config.region_name = 'testregion' + self.cloud.config.config['region_name'] = 'testregion' with testtools.ExpectedException( exc.OpenStackCloudException, "Error getting image endpoint on testcloud:testregion:" diff --git a/openstack/tests/unit/config/test_cloud_config.py b/openstack/tests/unit/config/test_cloud_config.py index 5a8b9df2e..2bfcbcefa 100644 --- a/openstack/tests/unit/config/test_cloud_config.py +++ b/openstack/tests/unit/config/test_cloud_config.py @@ -181,6 +181,55 @@ class TestCloudRegion(base.TestCase): self.assertEqual(1, cc.get_connect_retries('compute')) self.assertEqual(3, cc.get_connect_retries('baremetal')) + def test_get_region_name(self): + # TODO(efried): ddt this + + # No region_name kwarg, no regions specified in services dict + # (including the default). + cc = cloud_region.CloudRegion(config=fake_services_dict) + self.assertIsNone(cc.region_name) + self.assertIsNone(cc.get_region_name()) + self.assertIsNone(cc.get_region_name(service_type=None)) + self.assertIsNone(cc.get_region_name(service_type='compute')) + self.assertIsNone(cc.get_region_name(service_type='placement')) + + # Only region_name kwarg; it's returned for everything + cc = cloud_region.CloudRegion( + region_name='foo', config=fake_services_dict) + self.assertEqual('foo', cc.region_name) + self.assertEqual('foo', cc.get_region_name()) + self.assertEqual('foo', cc.get_region_name(service_type=None)) + self.assertEqual('foo', cc.get_region_name(service_type='compute')) + self.assertEqual('foo', cc.get_region_name(service_type='placement')) + + # No region_name kwarg; values (including default) show through from + # config dict + services_dict = dict( + fake_services_dict, + region_name='the-default', compute_region_name='compute-region') + cc = cloud_region.CloudRegion(config=services_dict) + self.assertEqual('the-default', cc.region_name) + self.assertEqual('the-default', cc.get_region_name()) + self.assertEqual('the-default', cc.get_region_name(service_type=None)) + self.assertEqual( + 'compute-region', cc.get_region_name(service_type='compute')) + self.assertEqual( + 'the-default', cc.get_region_name(service_type='placement')) + + # region_name kwarg overrides config dict default (for backward + # compatibility), but service-specific region_name takes precedence. + services_dict = dict( + fake_services_dict, + region_name='dict', compute_region_name='compute-region') + cc = cloud_region.CloudRegion( + region_name='kwarg', config=services_dict) + self.assertEqual('kwarg', cc.region_name) + self.assertEqual('kwarg', cc.get_region_name()) + self.assertEqual('kwarg', cc.get_region_name(service_type=None)) + self.assertEqual( + 'compute-region', cc.get_region_name(service_type='compute')) + self.assertEqual('kwarg', cc.get_region_name(service_type='placement')) + def test_aliases(self): services_dict = fake_services_dict.copy() services_dict['volume_api_version'] = 12 diff --git a/openstack/tests/unit/config/test_from_conf.py b/openstack/tests/unit/config/test_from_conf.py index 7308b9397..c60a48f84 100644 --- a/openstack/tests/unit/config/test_from_conf.py +++ b/openstack/tests/unit/config/test_from_conf.py @@ -56,13 +56,6 @@ class TestFromConf(base.TestCase): oslocfg, session=self.cloud.session, name='from_conf.example.com') self.assertEqual('from_conf.example.com', config.name) - # TODO(efried): Currently region_name gets set to the last value seen - # in the config, which is nondeterministic and surely incorrect. - # Sometimes that's SpecialRegion, but some tests use the base fixtures - # which have no compute endpoint in SpecialRegion. Force override for - # now to make those tests work. - config.region_name = None - return connection.Connection(config=config) def test_adapter_opts_set(self): @@ -94,8 +87,7 @@ class TestFromConf(base.TestCase): ]) adap = conn.orchestration - # TODO(efried): Fix this when region_name behaves correctly. - # self.assertEqual('SpecialRegion', adap.region_name) + self.assertEqual('SpecialRegion', adap.region_name) self.assertEqual('orchestration', adap.service_type) self.assertEqual('internal', adap.interface) self.assertEqual('https://example.org:8888/heat/v2',