Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Fix Role Migrations #3167

Merged
merged 7 commits into from
Jan 31, 2023
Merged

Fix Role Migrations #3167

merged 7 commits into from
Jan 31, 2023

Conversation

bryanculver
Copy link
Member

@bryanculver bryanculver commented Jan 26, 2023 •
edited

Closes: #DNE

What's Changed

  • VLAN.legacy_role__name is not an attribute. Can't use the QuerySet filter to access this. Instead, check if we don't get a string back and instead grab the name of the associated object.
  • ObjectChange records associated to ipam.Role make stale content type clean up sad
  • Probably need to do the same cleanup for DeviceRole...
  • There exists another bug using the demo dataset and get collisions if an ipam.Role has the same slug as the previously choice-only role (possibly if name doesn't match but slug does?)

TODO

  • Explanation of Change(s)
  • Added change log fragment(s) (for more information see the documentation)
  • Attached Screenshots, Payload Example
  • [?] Unit, Integration Tests
  • [NA] Documentation Updates (when adding/changing features)
  • [NA] Example Plugin Updates (when adding/changing features)
  • [NA] Outline Remaining Work, Constraints from Design

@bryanculver bryanculver changed the base branch from develop to next January 26, 2023 05:34
@bryanculver bryanculver added this to the v2.0.0-alpha.1 milestone Jan 26, 2023
@bryanculver bryanculver requested review from a team and glennmatthews January 27, 2023 19:36
glennmatthews
glennmatthews reviewed Jan 27, 2023
View reviewed changes
nautobot/extras/migrations/0057_collect_roles_from_related_apps_roles.py Outdated
Comment on lines 80 to 85
name=data.name,
slug=data.slug,
description=data.description,
color=getattr(data, "color", color_map["default"]),
weight=getattr(data, "weight", None),
),
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we instead be using Role.objects.get_or_create() as a way to handle existing roles?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe? But there's issues in the data migrations in the soon-to-be-deleted roles and associated objects.

We need some logic around "if slug matches but name differs" or "if name matches but description differs" etc.

nautobot/extras/migrations/0057_collect_roles_from_related_apps_roles.py Outdated
Comment on lines 94 to 98
if old_role_ct:
# Move over existing object change records to the new role we created
ObjectChange.objects.filter(changed_object_type=old_role_ct, changed_object_id=old_role.pk).update(
changed_object_type=new_role_ct, changed_object_id=new_role.pk
)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Won't this skip over ObjectChanges for roles that happen to already have been created with a different content-type?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unless my logic is extremely faulty the way I've read through the order of these migrations is that won't ever happen. The migration doesn't appear to collapse Roles into a single object, hence the collision problem that still exists.

I would ultimately like to refactor the way these migrations happen before we ship 2.0.

Currently, we have a dependency on the non-extras apps schema migrations on this data migration and we loosely associate the old role name to the new ones by hoping they have a similar name and/or slug.

I've tested this pretty extensively locally, restoring the DBs and re-running for different scenarios.

nautobot/extras/migrations/0057_collect_roles_from_related_apps_roles.py Outdated

# This is for all of the change records for roles which no longer exist
if old_role_ct:
# TODO: Should we null out the changed_object_id?
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think we do for object deletes in general?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We don't which is why I didn't do it here and a UUID is a UUID and even if it were in the same ContentType the likelihood you'll re-use a UUID is staggeringly small.

glennmatthews
glennmatthews reviewed Jan 30, 2023
View reviewed changes
Copy link
Contributor

@glennmatthews glennmatthews left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just tried running the updated migrations from a develop dataset to this branch, and while it no longer errors out, I get the following:

  Applying ipam.0010_migrate_ipam_role_data...
16:48:20.419 ERROR   nautobot.extras.utils utils.py        get_instances_to_pk_and_new_role() :
  Role with name  not found
Role with name  not found
16:48:20.422 ERROR   nautobot.extras.utils utils.py        get_instances_to_pk_and_new_role() :
  Role with name  not found
Role with name  not found
16:48:20.427 ERROR   nautobot.extras.utils utils.py        get_instances_to_pk_and_new_role() :
  Role with name  not found
Role with name  not found
16:48:20.429 ERROR   nautobot.extras.utils utils.py        get_instances_to_pk_and_new_role() :
  Role with name  not found
Role with name  not found
16:48:20.432 ERROR   nautobot.extras.utils utils.py        get_instances_to_pk_and_new_role() :
  Role with name  not found
Role with name  not found
16:48:20.434 ERROR   nautobot.extras.utils utils.py        get_instances_to_pk_and_new_role() :
  Role with name  not found
Role with name  not found
16:48:20.437 ERROR   nautobot.extras.utils utils.py        get_instances_to_pk_and_new_role() :
  Role with name  not found
Role with name  not found
16:48:20.438 ERROR   nautobot.extras.utils utils.py        get_instances_to_pk_and_new_role() :
  Role with name  not found
Role with name  not found
16:48:20.439 ERROR   nautobot.extras.utils utils.py        get_instances_to_pk_and_new_role() :
  Role with name  not found
Role with name  not found
16:48:20.443 ERROR   nautobot.extras.utils utils.py        get_instances_to_pk_and_new_role() :
  Role with name  not found
Role with name  not found
16:48:20.445 ERROR   nautobot.extras.utils utils.py        get_instances_to_pk_and_new_role() :
  Role with name  not found
Role with name  not found
16:48:20.447 ERROR   nautobot.extras.utils utils.py        get_instances_to_pk_and_new_role() :
  Role with name  not found
Role with name  not found
16:48:20.450 ERROR   nautobot.extras.utils utils.py        get_instances_to_pk_and_new_role() :
  Role with name  not found
Role with name  not found
16:48:20.453 ERROR   nautobot.extras.utils utils.py        get_instances_to_pk_and_new_role() :
  Role with name  not found
Role with name  not found
16:48:20.454 ERROR   nautobot.extras.utils utils.py        get_instances_to_pk_and_new_role() :
  Role with name  not found
Role with name  not found
16:48:20.456 ERROR   nautobot.extras.utils utils.py        get_instances_to_pk_and_new_role() :
  Role with name  not found
Role with name  not found
16:48:20.462 ERROR   nautobot.extras.utils utils.py        get_instances_to_pk_and_new_role() :
  Role with name  not found
Role with name  not found
16:48:20.466 ERROR   nautobot.extras.utils utils.py        get_instances_to_pk_and_new_role() :
  Role with name  not found
Role with name  not found
16:48:20.473 ERROR   nautobot.extras.utils utils.py        get_instances_to_pk_and_new_role() :
  Role with name  not found
Role with name  not found
16:48:20.476 ERROR   nautobot.extras.utils utils.py        get_instances_to_pk_and_new_role() :
  Role with name  not found
Role with name  not found
16:48:20.480 ERROR   nautobot.extras.utils utils.py        get_instances_to_pk_and_new_role() :
  Role with name  not found
Role with name  not found

which seems suspect to me?

glennmatthews
glennmatthews approved these changes Jan 30, 2023
View reviewed changes
Copy link
Contributor

@glennmatthews glennmatthews left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good enough for the moment. Let's make sure we get a backlog item opened to revisit this though.

@bryanculver bryanculver merged commit ed292dd into next Jan 31, 2023
@glennmatthews glennmatthews mentioned this pull request Feb 9, 2023
Open
@bryanculver bryanculver deleted the bsc-fix-role-migrations branch February 14, 2023 14:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@glennmatthews glennmatthews glennmatthews approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
v2.0.0-alpha.1
Development

Successfully merging this pull request may close these issues.

None yet
2 participants
@bryanculver @glennmatthews