Remove redundant call to get/create default security group
In the instance_create DB API method, it ensures the (legacy) default security group gets created for the specified project_id if it does not already exist. If the security group does not exist, it is created in a separate transaction. Later in the instance_create method, it reads the default security group back that it wrote earlier (via the same ensure default security group code). But since it was written in a separate transaction, the current transaction will not be able to see it and will get back 0 rows. So, it creates a duplicate default security group record if project_id=NULL (which it will be, if running nova-manage db online_data_migrations, which uses an anonymous RequestContext with project_id=NULL). This succeeds despite the unique constraint on project_id because in MySQL, unique constraints are only enforced on non-NULL values [1]. To avoid creation of a duplicate default security group for project_id=NULL, we can use the default security group object that was returned from the first security_group_ensure_default call earlier in instance_create method and remove the second, redundant call. This also breaks out the security groups setup code from a nested method as it was causing confusion during code review and is not being used for any particular purpose. Inspection of the original commit where it was added in 2012 [2] did not contain any comments about the nested method and it appeared to either be a way to organize the code or a way to reuse the 'models' module name as a local variable name. Closes-Bug: #1824435 [1] https://dev.mysql.com/doc/refman/8.0/en/create-index.html#create-index-unique [2] https://review.opendev.org/#/c/8973/2/nova/db/sqlalchemy/api.py@1339 Change-Id: Idb205ab5b16bbf96965418cd544016fa9cc92de9
This commit is contained in:
parent
7c41365f19
commit
6ea945e3b1
|
@ -266,13 +266,8 @@ $MANAGE db online_data_migrations
|
|||
# creation of a new deleted marker instance.
|
||||
set +e
|
||||
archive_deleted_rows
|
||||
set -e
|
||||
|
||||
# Verify whether online data migrations run after archiving will succeed.
|
||||
# See for more details: https://bugs.launchpad.net/nova/+bug/1824435
|
||||
$MANAGE db online_data_migrations
|
||||
rc=$?
|
||||
set -e
|
||||
if [[ $rc -ne 2 ]]; then
|
||||
echo "Expected return code 2 from online_data_migrations until bug 1824435 is fixed"
|
||||
exit 2
|
||||
fi
|
||||
|
|
|
@ -1753,7 +1753,7 @@ def instance_create(context, values):
|
|||
values - dict containing column values.
|
||||
"""
|
||||
|
||||
security_group_ensure_default(context)
|
||||
default_group = security_group_ensure_default(context)
|
||||
|
||||
values = values.copy()
|
||||
values['metadata'] = _metadata_refs(
|
||||
|
@ -1782,21 +1782,19 @@ def instance_create(context, values):
|
|||
instance_ref['extra'].update(values.pop('extra', {}))
|
||||
instance_ref.update(values)
|
||||
|
||||
def _get_sec_group_models(security_groups):
|
||||
models = []
|
||||
default_group = _security_group_ensure_default(context)
|
||||
if 'default' in security_groups:
|
||||
models.append(default_group)
|
||||
# Generate a new list, so we don't modify the original
|
||||
security_groups = [x for x in security_groups if x != 'default']
|
||||
if security_groups:
|
||||
models.extend(_security_group_get_by_names(
|
||||
context, security_groups))
|
||||
return models
|
||||
# Gather the security groups for the instance
|
||||
sg_models = []
|
||||
if 'default' in security_groups:
|
||||
sg_models.append(default_group)
|
||||
# Generate a new list, so we don't modify the original
|
||||
security_groups = [x for x in security_groups if x != 'default']
|
||||
if security_groups:
|
||||
sg_models.extend(_security_group_get_by_names(
|
||||
context, security_groups))
|
||||
|
||||
if 'hostname' in values:
|
||||
_validate_unique_server_name(context, values['hostname'])
|
||||
instance_ref.security_groups = _get_sec_group_models(security_groups)
|
||||
instance_ref.security_groups = sg_models
|
||||
context.session.add(instance_ref)
|
||||
|
||||
# create the instance uuid to ec2_id mapping entry for instance
|
||||
|
|
Loading…
Reference in New Issue