From 402c32094b8003db6186f17742a97925405657a6 Mon Sep 17 00:00:00 2001 From: Julia Kreger Date: Fri, 23 Jun 2023 12:44:47 -0700 Subject: [PATCH] Handle SAWarning around allocations FK Constratins We have started to notice an SAWarning from sqlalchemy indicating: SAWarning: Cannot correctly sort tables; there are unresolvable cycles between tables "allocations, nodes", which is usually caused by mutually dependent foreign key constraints. Foreign key constraints involving these tables will not be considered; this warning may raise an error in a future release. Hunting this down, it appears to be the two data consistency Foreign Key constraints in the "allocations" table where an allocation would try to have a conductor_affinity value mapped to conductors.id and also have a direct association to a node, which *also* had the same constraint. And then similarlly, mapping in reverse, asserting a fk constraint, when nodes also had it's own constraint back on allocations. Sort of a circular loop. Anyhow, removes it, and adds a db migration to remove the two constraints. Change-Id: I5596008e4971a29c635c45b24cb85db2d0d13ed3 --- ...df1bab88_remove_extra_fk_constraint_in_.py | 50 +++++++++++++++++++ .../dd67b91a1981_add_allocations_table.py | 14 +++++- ironic/db/sqlalchemy/models.py | 5 +- ...ation-fk-constraints-0f59170f4d164a6e.yaml | 6 +++ 4 files changed, 70 insertions(+), 5 deletions(-) create mode 100644 ironic/db/sqlalchemy/alembic/versions/d163df1bab88_remove_extra_fk_constraint_in_.py create mode 100644 releasenotes/notes/remove-excess-allocation-fk-constraints-0f59170f4d164a6e.yaml diff --git a/ironic/db/sqlalchemy/alembic/versions/d163df1bab88_remove_extra_fk_constraint_in_.py b/ironic/db/sqlalchemy/alembic/versions/d163df1bab88_remove_extra_fk_constraint_in_.py new file mode 100644 index 0000000000..12be23cb62 --- /dev/null +++ b/ironic/db/sqlalchemy/alembic/versions/d163df1bab88_remove_extra_fk_constraint_in_.py @@ -0,0 +1,50 @@ +# 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. + + +"""remove_extra_fk_constraint in allocations + +Revision ID: d163df1bab88 +Revises: 163040c5513f +Create Date: 2023-06-23 11:20:10.904262 + +""" + + +from alembic import op +from oslo_db import exception as db_exc +from oslo_db.sqlalchemy import enginefacade + + +# revision identifiers, used by Alembic. +revision = 'd163df1bab88' +down_revision = '163040c5513f' + + +def upgrade(): + engine = enginefacade.reader.get_engine() + + if engine.dialect.name == 'mysql': + # Remove the outer level conductor_affinity constraint which pointed + # to the conductor.id field, when the allocation's table also already + # points to unique node constraints where a node also has a conductor + # affinity field on the conductor.id. Removing because this is + # expected to be come a hard error for SQLAlchemy at some point. + try: + op.drop_constraint('allocations_ibfk_1', 'allocations', + type_="foreignkey") + op.drop_constraint('allocations_ibfk_2', 'allocations', + type_="foreignkey") + except db_exc.DBNonExistentConstraint: + # This is to ignore this issue on newer deployments where + # key mappings may not already exist. + pass diff --git a/ironic/db/sqlalchemy/alembic/versions/dd67b91a1981_add_allocations_table.py b/ironic/db/sqlalchemy/alembic/versions/dd67b91a1981_add_allocations_table.py index 74ab297a5b..426889ed29 100644 --- a/ironic/db/sqlalchemy/alembic/versions/dd67b91a1981_add_allocations_table.py +++ b/ironic/db/sqlalchemy/alembic/versions/dd67b91a1981_add_allocations_table.py @@ -44,8 +44,18 @@ def upgrade(): sa.Column('candidate_nodes', types.JsonEncodedList(), nullable=True), sa.Column('extra', types.JsonEncodedDict(), nullable=True), sa.Column('conductor_affinity', sa.Integer(), nullable=True), - sa.ForeignKeyConstraint(['conductor_affinity'], ['conductors.id'], ), - sa.ForeignKeyConstraint(['node_id'], ['nodes.id'], ), + # NOTE(TheJulia): Commenting these out to remove the constraints + # as below we link nodes to allocations on a unique foreign key + # constraint mapping, and nodes also have the same conductor affinity + # constraint, which raises an SAWarning error as sqlalchemy cannot + # sort the data model relationships, and expects this to become an + # error at some point in the future. As such, since a node is + # generally a primary object, and allocations are more secondary + # relationship/association mapping objects, the two commented out + # lines are redundant. We'll remove the relationships in a migration + # as well and ignore errors if they are encountered for new installs. + # sa.ForeignKeyConstraint(['conductor_affinity'], ['conductors.id'], ), + # sa.ForeignKeyConstraint(['node_id'], ['nodes.id'], ), sa.PrimaryKeyConstraint('id'), sa.UniqueConstraint('name', name='uniq_allocations0name'), sa.UniqueConstraint('uuid', name='uniq_allocations0uuid'), diff --git a/ironic/db/sqlalchemy/models.py b/ironic/db/sqlalchemy/models.py index d15b06d86a..bff247ac1b 100644 --- a/ironic/db/sqlalchemy/models.py +++ b/ironic/db/sqlalchemy/models.py @@ -412,7 +412,7 @@ class Allocation(Base): id = Column(Integer, primary_key=True) uuid = Column(String(36), nullable=False) name = Column(String(255), nullable=True) - node_id = Column(Integer, ForeignKey('nodes.id'), nullable=True) + node_id = Column(Integer, nullable=True) state = Column(String(15), nullable=False) owner = Column(String(255), nullable=True) last_error = Column(Text, nullable=True) @@ -421,8 +421,7 @@ class Allocation(Base): candidate_nodes = Column(db_types.JsonEncodedList) extra = Column(db_types.JsonEncodedDict) # The last conductor to handle this allocation (internal field). - conductor_affinity = Column(Integer, ForeignKey('conductors.id'), - nullable=True) + conductor_affinity = Column(Integer, nullable=True) class DeployTemplate(Base): diff --git a/releasenotes/notes/remove-excess-allocation-fk-constraints-0f59170f4d164a6e.yaml b/releasenotes/notes/remove-excess-allocation-fk-constraints-0f59170f4d164a6e.yaml new file mode 100644 index 0000000000..47f35b0906 --- /dev/null +++ b/releasenotes/notes/remove-excess-allocation-fk-constraints-0f59170f4d164a6e.yaml @@ -0,0 +1,6 @@ +--- +upgrade: + - | + This release removes two internal foreign key constraints which were + redundant and which SQLAlchemy indicated may result in an error at some + point in time. No action is required by an operator for this.