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.

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

Fixes #262: Missing or duplicated results for .filter() when threading is enabled #263

Merged
merged 1 commit into from
Jul 6, 2020

Conversation

weisdd
Copy link
Contributor

@weisdd weisdd commented Jul 5, 2020

As mentioned in #262, we sometimes see missing or duplicated results when calling nb.dcim.devices.filter() with threading enabled.
Troubleshooting showed that the requests themselves are scheduled with correct offsets, though when the actual calls happen, params seem to be overwritten, further causing incorrect results. Not always, but often enough, and especially visible with low MAX_PAGE_SIZE values.
image

In the implementation of query.py, I've noticed that concurrent sessions seem to use the same copy of the attrubute "self.filters" (that params = self.filters). That's why params.update(add_params) would sometimes cause collisions depending on the time when a particular request is called.

        params = {}
         if not url_override:
             if self.filters:
                 params = self.filters
             if add_params:
                 params.update(add_params)

An example of this dict:

{'role': ['switch', 'router'], 'has_primary_ip': True, 'limit': 20, 'offset': 60}

I think changing it to:

        params = {}
        if not url_override:
            if self.filters:
                params.update(self.filters)
            if add_params:
                params.update(add_params)

will help to mitigate the issue. At least, in my tests it seems to work.

UPD: a bit rewritten my text to make it clearer.

@weisdd
Copy link
Contributor Author

weisdd commented Jul 5, 2020

@zachmoody :)

@zachmoody
Copy link
Contributor

Makes sense, nice catch.

@zachmoody zachmoody merged commit 064b72e into netbox-community:master Jul 6, 2020
@tyler-8
Copy link
Contributor

tyler-8 commented Jul 6, 2020

Sorry for the headache from my introduction of threading, great find!

@weisdd
Copy link
Contributor Author

weisdd commented Jul 7, 2020

@tyler-8 No worries, after working in TAC, I have a soft spot for troubleshooting activities :)
And thanks for the threads, in our use case they make a huge difference!

@MattIPv4
Copy link

Hey, @weisdd - Thanks a ton for this sweet PR! 😄

Would you please shoot me an email when you get a chance?
mcowley at digitalocean dot com 🎉

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants