From 68625686a40b3eb75502c8116f23d2297e288ca1 Mon Sep 17 00:00:00 2001 From: zhanghao Date: Sat, 26 Oct 2019 16:57:27 -0400 Subject: [PATCH] Make the MTU attribute not nullable This patch sets the MTU attribute to non-nullable, the code that get the MTU and update the network in some methods can be removed. If the MTU is empty before the pike version, it is set to the default value of 1500. Change-Id: Id4d738dde7fa4b7caccabad0aac542b82b4d7af1 Closes-Bug: #1842261 --- neutron/cmd/upgrade_checks/checks.py | 33 +++++++++++ neutron/db/db_base_plugin_v2.py | 2 +- .../alembic_migrations/versions/EXPAND_HEAD | 2 +- .../86274d77933e_change_mtu_to_not_null.py | 55 +++++++++++++++++++ neutron/db/models_v2.py | 6 +- neutron/objects/network.py | 15 ++++- neutron/plugins/ml2/plugin.py | 17 ------ .../unit/cmd/upgrade_checks/test_checks.py | 22 ++++++++ .../tests/unit/db/test_db_base_plugin_v2.py | 6 +- neutron/tests/unit/objects/test_objects.py | 2 +- ...ake-mtu-not-nullable-2b2765bc85379545.yaml | 5 ++ 11 files changed, 138 insertions(+), 27 deletions(-) create mode 100644 neutron/db/migration/alembic_migrations/versions/ussuri/expand/86274d77933e_change_mtu_to_not_null.py create mode 100644 releasenotes/notes/make-mtu-not-nullable-2b2765bc85379545.yaml diff --git a/neutron/cmd/upgrade_checks/checks.py b/neutron/cmd/upgrade_checks/checks.py index 07439a101d0..637e338b31c 100644 --- a/neutron/cmd/upgrade_checks/checks.py +++ b/neutron/cmd/upgrade_checks/checks.py @@ -22,6 +22,7 @@ from oslo_upgradecheck import upgradecheck from neutron._i18n import _ from neutron.cmd.upgrade_checks import base from neutron.db.models import agent as agent_model +from neutron.db import models_v2 def get_l3_agents(): @@ -33,6 +34,13 @@ def get_l3_agents(): return query.all() +def get_networks(): + ctx = context.get_admin_context() + query = model_query.get_collection_query(ctx, + models_v2.Network) + return query.all() + + class CoreChecks(base.BaseChecks): def get_checks(self): @@ -123,3 +131,28 @@ class CoreChecks(base.BaseChecks): return upgradecheck.Result( upgradecheck.Code.SUCCESS, _("L3 agents can use multiple networks as external gateways.")) + + @staticmethod + def network_mtu_check(checker): + if not cfg.CONF.database.connection: + return upgradecheck.Result( + upgradecheck.Code.WARNING, + _("Database connection string is not set. Check of 'mtu' in " + "networks can't be done")) + + networks_with_empty_mtu_attr = [] + for network in get_networks(): + mtu = network.get('mtu', None) + if not mtu: + networks_with_empty_mtu_attr.append(network.get("id")) + + if networks_with_empty_mtu_attr: + networks_list = ", ".join(networks_with_empty_mtu_attr) + return upgradecheck.Result( + upgradecheck.Code.WARNING, + _("The 'mtu' attribute of networks %s are not set " + "This attribute can't be null now.") % networks_list) + else: + return upgradecheck.Result( + upgradecheck.Code.SUCCESS, + _("The 'mtu' attribute of all networks are set.")) diff --git a/neutron/db/db_base_plugin_v2.py b/neutron/db/db_base_plugin_v2.py index b47ee562e3d..f75104c00eb 100644 --- a/neutron/db/db_base_plugin_v2.py +++ b/neutron/db/db_base_plugin_v2.py @@ -404,7 +404,7 @@ class NeutronDbPluginV2(db_base_plugin_common.DbBasePluginCommon, args = {'tenant_id': n['tenant_id'], 'id': n.get('id') or uuidutils.generate_uuid(), 'name': n['name'], - 'mtu': n.get('mtu'), + 'mtu': n.get('mtu', constants.DEFAULT_NETWORK_MTU), 'admin_state_up': n['admin_state_up'], 'status': n.get('status', constants.NET_STATUS_ACTIVE), 'description': n.get('description')} diff --git a/neutron/db/migration/alembic_migrations/versions/EXPAND_HEAD b/neutron/db/migration/alembic_migrations/versions/EXPAND_HEAD index e7189617eb7..67d8fc4faeb 100644 --- a/neutron/db/migration/alembic_migrations/versions/EXPAND_HEAD +++ b/neutron/db/migration/alembic_migrations/versions/EXPAND_HEAD @@ -1 +1 @@ -c3e9d13c4367 +86274d77933e diff --git a/neutron/db/migration/alembic_migrations/versions/ussuri/expand/86274d77933e_change_mtu_to_not_null.py b/neutron/db/migration/alembic_migrations/versions/ussuri/expand/86274d77933e_change_mtu_to_not_null.py new file mode 100644 index 00000000000..2a048a39faf --- /dev/null +++ b/neutron/db/migration/alembic_migrations/versions/ussuri/expand/86274d77933e_change_mtu_to_not_null.py @@ -0,0 +1,55 @@ +# Copyright 2019 OpenStack Foundation +# +# Licensed under the Apache License, Version 2.0 (the "License"); you may +# not use this file except in compliance with the License. You may obtain +# a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, software +# distributed under the License is distributed on an "AS IS" BASIS, WITHOUT +# WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the +# License for the specific language governing permissions and limitations +# under the License. +# + +from alembic import op +from neutron_lib import constants +import sqlalchemy as sa + + +"""change_mtu_to_not_null + +Revision ID: 86274d77933e +Revises: c3e9d13c4367 +Create Date: 2019-08-30 15:52:30.015146 + +""" + +# revision identifiers, used by Alembic. +revision = '86274d77933e' +down_revision = 'c3e9d13c4367' + + +networks = sa.Table( + 'networks', sa.MetaData(), + sa.Column('id', sa.String(length=36), nullable=False), + sa.Column('mtu', sa.Integer(), nullable=True)) + + +def upgrade_existing_records(): + session = sa.orm.Session(bind=op.get_bind()) + with session.begin(subtransactions=True): + for row in session.query(networks): + if row[1] is None: + session.execute(networks.update().values( + mtu=constants.DEFAULT_NETWORK_MTU).where( + networks.c.id == row[0])) + session.commit() + + +def upgrade(): + upgrade_existing_records() + op.alter_column('networks', 'mtu', nullable=False, + server_default=str(constants.DEFAULT_NETWORK_MTU), + existing_type=sa.INTEGER()) diff --git a/neutron/db/models_v2.py b/neutron/db/models_v2.py index cd82965c087..e56eb4a81a9 100644 --- a/neutron/db/models_v2.py +++ b/neutron/db/models_v2.py @@ -268,9 +268,9 @@ class Network(standard_attr.HasStandardAttributes, model_base.BASEV2, lazy='subquery', cascade='all, delete, delete-orphan') availability_zone_hints = sa.Column(sa.String(255)) - # TODO(ihrachys) provide data migration path to fill in mtus for existing - # networks in Queens when all controllers run Pike+ code - mtu = sa.Column(sa.Integer, nullable=True) + mtu = sa.Column(sa.Integer, nullable=False, + default=constants.DEFAULT_NETWORK_MTU, + server_default=str(constants.DEFAULT_NETWORK_MTU)) dhcp_agents = orm.relationship( 'Agent', lazy='subquery', viewonly=True, secondary=ndab_model.NetworkDhcpAgentBinding.__table__) diff --git a/neutron/objects/network.py b/neutron/objects/network.py index 22f0bade516..560bcb2bd3a 100644 --- a/neutron/objects/network.py +++ b/neutron/objects/network.py @@ -14,7 +14,9 @@ from neutron_lib.api.definitions import availability_zone as az_def from neutron_lib.api.validators import availability_zone as az_validator +from neutron_lib import constants from oslo_utils import versionutils +from oslo_versionedobjects import exception from oslo_versionedobjects import fields as obj_fields import sqlalchemy as sa @@ -214,7 +216,8 @@ class ExternalNetwork(base.NeutronDbObject): @base.NeutronObjectRegistry.register class Network(rbac_db.NeutronRbacObject): # Version 1.0: Initial version - VERSION = '1.0' + # Version 1.1: Changed 'mtu' to be not nullable + VERSION = '1.1' rbac_db_cls = NetworkRBAC db_model = models_v2.Network @@ -231,7 +234,7 @@ class Network(rbac_db.NeutronRbacObject): nullable=True), 'shared': obj_fields.BooleanField(default=False), - 'mtu': obj_fields.IntegerField(nullable=True), + 'mtu': obj_fields.IntegerField(default=constants.DEFAULT_NETWORK_MTU), # TODO(ihrachys): consider exposing availability zones @@ -344,6 +347,14 @@ class Network(rbac_db.NeutronRbacObject): # TODO(ihrachys): provide actual implementation return set() + def obj_make_compatible(self, primitive, target_version): + _target_version = versionutils.convert_version_to_tuple(target_version) + if _target_version >= (1, 1): + if primitive['mtu'] is None: + # mtu will not be nullable after + raise exception.IncompatibleObjectVersion( + objver=target_version, objname=self.__class__.__name__) + @base.NeutronObjectRegistry.register class SegmentHostMapping(base.NeutronDbObject): diff --git a/neutron/plugins/ml2/plugin.py b/neutron/plugins/ml2/plugin.py index 3edc649ac8f..167a82de975 100644 --- a/neutron/plugins/ml2/plugin.py +++ b/neutron/plugins/ml2/plugin.py @@ -1144,12 +1144,6 @@ class Ml2Plugin(db_base_plugin_v2.NeutronDbPluginV2, with db_api.CONTEXT_WRITER.using(context): net_db = self._get_network(context, id) - # NOTE(ihrachys) pre Pike networks may have null mtus; update them - # in database if needed - # TODO(ihrachys) remove in Queens+ when mtu is not nullable - if net_db.mtu is None: - net_db.mtu = self._get_network_mtu(net_db, validate=False) - net_data = self._make_network_dict(net_db, context=context) self.type_manager.extend_network_dict_provider(context, net_data) @@ -1164,13 +1158,8 @@ class Ml2Plugin(db_base_plugin_v2.NeutronDbPluginV2, nets_db = super(Ml2Plugin, self)._get_networks( context, filters, None, sorts, limit, marker, page_reverse) - # NOTE(ihrachys) pre Pike networks may have null mtus; update them - # in database if needed - # TODO(ihrachys) remove in Queens+ when mtu is not nullable net_data = [] for net in nets_db: - if net.mtu is None: - net.mtu = self._get_network_mtu(net, validate=False) net_data.append(self._make_network_dict(net, context=context)) self.type_manager.extend_networks_dict_provider(context, net_data) @@ -1241,12 +1230,6 @@ class Ml2Plugin(db_base_plugin_v2.NeutronDbPluginV2, result, net_db, ipam_sub = self._create_subnet_precommit( context, subnet) - # NOTE(ihrachys) pre Pike networks may have null mtus; update them - # in database if needed - # TODO(ihrachys) remove in Queens+ when mtu is not nullable - if net_db['mtu'] is None: - net_db['mtu'] = self._get_network_mtu(net_db, validate=False) - self.extension_manager.process_create_subnet( context, subnet[subnet_def.RESOURCE_NAME], result) network = self._make_network_dict(net_db, context=context) diff --git a/neutron/tests/unit/cmd/upgrade_checks/test_checks.py b/neutron/tests/unit/cmd/upgrade_checks/test_checks.py index fd018bd4ba9..64a48b47136 100644 --- a/neutron/tests/unit/cmd/upgrade_checks/test_checks.py +++ b/neutron/tests/unit/cmd/upgrade_checks/test_checks.py @@ -106,3 +106,25 @@ class TestChecks(base.BaseTestCase): self.assertIn('Host B', result.details) self.assertNotIn('Host A', result.details) self.assertNotIn('Host C', result.details) + + def test_network_mtu_check_good(self): + networks = [ + {'id': 'net-uuid-a', 'mtu': 1500}, + {'id': 'net-uuid-b', 'mtu': 1450} + ] + with mock.patch.object(checks, "get_networks", return_value=networks): + result = checks.CoreChecks.network_mtu_check( + mock.Mock()) + self.assertEqual(Code.SUCCESS, result.code) + + def test_network_mtu_check_bad(self): + networks = [ + {'id': 'net-uuid-a', 'mtu': None}, + {'id': 'net-uuid-b', 'mtu': 1500}, + ] + with mock.patch.object(checks, "get_networks", return_value=networks): + result = checks.CoreChecks.network_mtu_check( + mock.Mock()) + self.assertEqual(Code.WARNING, result.code) + self.assertIn('net-uuid-a', result.details) + self.assertNotIn('net-uuid-b', result.details) diff --git a/neutron/tests/unit/db/test_db_base_plugin_v2.py b/neutron/tests/unit/db/test_db_base_plugin_v2.py index 47ff036a4e4..ea3cdd288a6 100644 --- a/neutron/tests/unit/db/test_db_base_plugin_v2.py +++ b/neutron/tests/unit/db/test_db_base_plugin_v2.py @@ -6503,7 +6503,8 @@ class DbModelMixin(object): with db_api.CONTEXT_WRITER.using(ctx): network = models_v2.Network(name="net_net", status="OK", admin_state_up=True, - project_id='fake_project') + project_id='fake_project', + mtu=1500) ctx.session.add(network) with db_api.autonested_transaction(ctx.session): sg = sg_models.SecurityGroup(name='sg', description='sg') @@ -6516,7 +6517,8 @@ class DbModelMixin(object): project_id=network.project_id, target_tenant='*').create() net2 = models_v2.Network(name="net_net2", status="OK", - admin_state_up=True) + admin_state_up=True, + mtu=1500) ctx.session.add(net2) pl = db_base_plugin_common.DbBasePluginCommon() self.assertTrue(pl._make_network_dict(network, context=ctx)['shared']) diff --git a/neutron/tests/unit/objects/test_objects.py b/neutron/tests/unit/objects/test_objects.py index a72590353b4..647ea723a77 100644 --- a/neutron/tests/unit/objects/test_objects.py +++ b/neutron/tests/unit/objects/test_objects.py @@ -57,7 +57,7 @@ object_data = { 'MeteringLabel': '1.0-cc4b620a3425222447cbe459f62de533', 'MeteringLabelRule': '1.0-b5c5717e7bab8d1af1623156012a5842', 'Log': '1.0-6391351c0f34ed34375a19202f361d24', - 'Network': '1.0-f2f6308f79731a767b92b26b0f4f3849', + 'Network': '1.1-c3e9ecc0618ee934181d91b143a48901', 'NetworkDhcpAgentBinding': '1.1-d9443c88809ffa4c45a0a5a48134b54a', 'NetworkDNSDomain': '1.0-420db7910294608534c1e2e30d6d8319', 'NetworkPortSecurity': '1.0-b30802391a87945ee9c07582b4ff95e3', diff --git a/releasenotes/notes/make-mtu-not-nullable-2b2765bc85379545.yaml b/releasenotes/notes/make-mtu-not-nullable-2b2765bc85379545.yaml new file mode 100644 index 00000000000..668d2817582 --- /dev/null +++ b/releasenotes/notes/make-mtu-not-nullable-2b2765bc85379545.yaml @@ -0,0 +1,5 @@ +--- +upgrade: + - | + The network ``mtu`` attribute is set to be non-nullable. If the ``mtu`` is + empty(create before Pike version), it is set to the default value of 1500.