Cable post_save signal cause exception in loaddata management command

[Grokzen]

Environment

• Python version: 3.6.x
• NetBox version: 2.5.8

Background

Some background that will describe what we are doing and how we found this bug. At our production envrionment we do dumps of the data once a day for backup purposes. We use the djangos built-in dumpdata for this. It outputs a json file that we commit to git to see what data changes over time. We also use these snapshots as test data that we can load to test envrionment to build prod replicas and we subset this sometimes at our local dev machines to load and test features and data migrations before going into prod. We use the djangos built-in loaddata to read the json backups back from file into the database.

After we merged into our local fork and deployed a upgrade from 2.5.6 to 2.5.8 we started to see some error with loaddata where it would throw a exception when run. I dug around and thought it was a merge issue but i could not find any problems with the merge. So i dug into the data to sett if i could dig up the problem object and code. The problem object was identified and i tracked the problem to the root cause that was a signal that is emit on post_save on Cable object saves. This is the stack trace it output

https://gist.github.com/Grokzen/1bed27569b80cbe34b6522c7e9746db4

The important part is when the code enters into the signal

# dcim/signals.py L37
def update_connected_endpoints(...)

To reproduce this bug, i extracted and sanitized out all data from the json data dump into this minimal data dump that should trigger the bug. I got this bug to reproduce on a clean/unmodified 2.5.8 installation so i hope it can be replicated elsewhere.

Steps to Reproduce

1. Take the content of this gist https://gist.github.com/Grokzen/8f3cbce805296471a8cb52188db7232d and save it to a db-dump.json file
2. Start with a fresh and clean database. You need to

• drop the database
• create it again with a base migrate to get all tables and content types
• Since the data dump includes all content types, you need to remove all entries in that table

./manage.py shell -i python << END
from django.contrib.contenttypes.models import ContentType
ContentType.objects.all().delete()
END

3. Run python3 manage.py loaddata db-dump.json to start loading the data into the database

Expected Behavior

The data should be loaded by django into the database and usable w/o any exception thrown.

Observed Behavior

The exception as mentioned link above is thrown

When i PDB the exception i get the following from the signal frame

(Pdb) instance.termination_a
<Interface: TenGigabitEthernet1/2>
(Pdb) instance.termination_b
<FrontPort: Port 3>

(Pdb) instance.termination_a.model
*** AttributeError: 'Interface' object has no attribute 'model'
(Pdb) instance.termination_b.model
*** AttributeError: 'FrontPort' object has no attribute 'model'

Possible solution

Since the problem is in the signal that is implemented here, this is not a django specific problem with dumpdata/loaddata per say as they perform and resolve issues as expected.

Django does implement a feature that when you run management commands, django will inject a kwarg raw=True into all signals that is emited during the run of management commands.

The simplest solution would be to change it to the following

def update_connected_endpoints(instance, **kwargs):
    """
    When a Cable is saved, check for and update its two connected endpoints
    """
    if kwargs.get('raw', False):
        return False

But a more useful solution would be to make a decorator for it like

def disable_for_loaddata(signal_handler):
    @wraps(signal_handler)
    def wrapper(*args, **kwargs):
        if kwargs['raw']:
            return
        signal_handler(*args, **kwargs)
    return wrapper

@receiver(post_save, sender=Cable)
@disable_for_loaddata
def update_connected_endpoints(instance, **kwargs):
    """
    When a Cable is saved, check for and update its two connected endpoints
    """
    ....

then it can be reused if other signals show signs of the same problem and it is more explicit about it's purpose and what it does.

There might be a possible fix further down the stack, but i would argue that during loading of a datadump, the problem signal have no real need to run on each cable save as the data from the datadump is already in a good state.

Note

I can't say how we got the database to be in this state or the steps to really reproduce it from the GUI that would produce the same or a similar data dump file. All i could figure our from the person probably responsible was that he was trying to make patch panels work and he got the data to be in this state.

[DanSheps]

Thanks for the report, I will try and reproduce it.

However, it is worth noting we probably can't get rid of update_connected_endpoints as that updates connected interfaces on the interfaces table.

[Grokzen]

No that is very true, but you do not need the signal to be emited during a manage.py loaddata event as that load will import all objects and connections and setup everything exactly as it was in the source netbox installation. Adding that check will only block the signal from executing it's code during loaddata management command and nothing else.

[DanSheps]

I finished reviewing the dump data, and I agree, we don't need to include the signals (at all even).

I am going to do this and stash it as a PR for @jeremystretch and @lampwins to review.

[jeremystretch]

This looks related to #3849, where the order in which the objects are exported prevents them from being cleanly imported. Unfortunately Django's dumpdata management command is not smart enough to infer inter-model dependencies: It merely exports models in the order it finds them. This can likely be fixed by simply rearranging the models as they appear in dcim/device_components.py.

[jeremystretch]

Just noticed this was reported on v2.5.8. This is likely resolved by #3849 in v2.6.11. @Grokzen please try exporting and re-import data using release v2.6.11 or later and see if your issue is resolved.

[DanSheps]

Can confirm that this does appear to be resolved by #3849 going to close this one out.