Dark launch: Fix 1+N queries in third party auth config fetching by timmc-edx · Pull Request #23824 · openedx/openedx-platform

timmc-edx · 2020-04-28T16:19:01Z

Dark launch comparison of a fix for the 1+N issue on the login page. Old implementation is still run and the results used, but if there are discrepancies with the new implementation, logs metrics and debugging info to New Relic.

This should also fix an issue with configs shadowing each other across sites, but we only have configs for one site in Production, so I'm not expecting the comparison to trip over that.

feanil · 2020-04-28T19:09:04Z

-            provider = OAuth2ProviderConfig.current(oauth2_backend_name)
+        oauth2_backends = OAuth2ProviderConfig.objects.prefetch_related('site').all()
+        for provider in unique_everseen(oauth2_backends, key=lambda p: p.backend_name):
            if provider.enabled_for_current_site and provider.backend_name in _PSA_OAUTH2_BACKENDS:


Since we're only ever doing this for the site we're on, can we do the reverse and get all the providers where the site is the current site?

If I'm reading this right, it looks like a provider is mapped to at most one site:
https://github.com/edx/edx-platform/blob/0f5c474f8f49e7dbc1a7d60e15f5409a29dfe69f/common/djangoapps/third_party_auth/models.py#L138

Agreed that that would make more sense. For that matter, why not add site, enabled, and backend_name to the query and skip the entire guard clause that's inside the for loop?

I was being cautious because I'm not sure why it wasn't written that way in the first place... but maybe there's not a good reason. :-)

Yea, if we find one later, we can always undo the change and put in the good reason as a comment. Let's just make this a more sensible query.

Curiously, most of the tests pass if I do the below, including one of the two that was failing before—but the other one (test_provider.py:38) is still failing as if the query is being cached.

site = Site.objects.get_current(get_current_request()) oauth2_backends = OAuth2ProviderConfig.objects.filter(enabled=True, site=site, backend_name__in=_PSA_OAUTH2_BACKENDS) for provider in unique_everseen(oauth2_backends, key=lambda p: p.backend_name): yield provider

Failure:

def test_runtime_configuration(self): self.configure_google_provider(enabled=True) enabled_providers = provider.Registry.enabled() self.assertEqual(len(enabled_providers), 1) self.assertEqual(enabled_providers[0].name, "Google") self.assertEqual(enabled_providers[0].get_setting("SECRET"), "opensesame") self.configure_google_provider(enabled=False) enabled_providers = provider.Registry.enabled() > self.assertEqual(len(enabled_providers), 0) E AssertionError: 1 != 0 common/djangoapps/third_party_auth/tests/test_provider.py:38: AssertionError

OK, I think I see what's happening here. The test configures a provider that is enabled, then configures one that is disabled, both with the same backend name, so that we end up with two objects in the DB. The code on master is supposed to pick up only the most recent object, and then discard it if it is disabled. That's why it's not part of the query.

(But then I don't understand why site wasn't part of the query! Surely we'd want to partition that append-to-override behavior by site.)

(And the reason my code was failing is because it was getting the first row with a given slug or backend or key, not the last.)

So this is because it's backed by config models right? The config model table is essentially keeping a history of all the ways a setting has been set. So to get the correct setting, we want to get the latest setting for any given name and treat it as the source of truth. So if the latest version is disabled, then we treat the value as disabled. It seems like the test is valid.

Yeah, and my iterator was skipping the disabled one. I think the existing code has a bug around handling of sites, though; if two providers are registered for the same backend_name but different sites, the existing code makes the more recent one shadow the other.

nasthagiri · 2020-04-30T10:49:57Z

Curious: Is there a ticket for this work? I'm wondering if we are investing time in preemptive optimization here.

schenedx · 2020-04-30T13:11:10Z

@timmc-edx What is the context here? What problem did you see that prompt this change? The change is not really obvious to me it improves things. I read your PR description and comments and couldn't make good sense out of them.

timmc-edx · 2020-04-30T13:25:18Z

Sorry, yes, this is https://openedx.atlassian.net/browse/ARCHBOM-1139 -- we're seeing an inordinate number of SELECTs on django_site driven just by loading the login page.

schenedx · 2020-04-30T13:42:34Z

@timmc-edx Great. With that ticket, now I know what you are trying to do. I'll need to review again. Thanks

timmc-edx · 2020-04-30T16:26:18Z

At this point, all I can say for sure is that there's something I'm missing around how current, KEY_FIELDS, and site_id are interacting with the various subclasseses of ProviderConfig in provider.py. :-)

(The Slack thread you were tagged in contains some of my stream of consciousness, so maybe that's already obvious by now.)

The filtering of SAML provider configs is the most likely culprit for what is causing a large number of requests for django_site when loading the login page in production. (ARCHBOM-1139)

schenedx

How many queries would this change shrink down to?
My comments are minor in importance.

schenedx · 2020-05-01T19:36:27Z

        self.assertEqual(prov.enabled_for_current_site, False)

+    @with_site_configuration(SITE_DOMAIN_A)
+    def test_providers_with_same_key_independent_across_sites(self):


I believe we can have another test case where each site will have both oauth_provider set and SAML_provider set. Then the return should be the latest oauth_provider and latest saml provider in a list.

schenedx · 2020-05-01T19:38:19Z

+                if config.enabled:
+                    yield config
+
+        for provider in enabled_in_site(OAuth2ProviderConfig, {'backend_name__in': _PSA_OAUTH2_BACKENDS}):


Can you please add more comments for this loop? Like First, we should get all the ID providers using oauth2

- Prep for moving `current_partitioned` (now `current_all`) to django-config-models - Unroll `enabled_in_site` - Don't bother with `backend_name` in DB query: It's very unlikely to reduce the response size; it makes the queries harder to read; and most importantly increases the necessary complexity for `current_all` (since if `'site_id'` is added to `KEY_FIELDS` we will then not need any additional filters at all.) - Add comments - Add additional test

timmc-edx · 2020-05-01T21:19:52Z

Addressed some feedback. I'm also going to see about:

Reconciling this with get_enabled_by_backend_name lower in the same file
Adding 'site_id' to KEY_FIELDS -- this would be a larger change, but would make all of this structurally safer, and the changes to django-config-models would be simpler (no need for a filter argument in current_all). Might turn into a separate PR?

timmc-edx · 2020-05-05T15:49:57Z

I've changed it to be a dark launch for now, so that the old implementation is still used, but the new code is run under a try/catch and any failures or result mismatches are sent to New Relic.

My intention is to open a separate PR to put current_all into django-config-models, then update this PR, but I'm keeping it here until just before merge for the sake of a faster review cycle. Note that it still lacks caching; if I were able to add site_id to the KEY_FIELDS of the models, I could get rid of the filters param and add caching, but lacking that I don't think it makes sense.

Not moving `current_all` to django-config-models after all; it's enough of a hack that it doesn't really belong perfectly in either place. Some renaming and reworking of parameters.

edx-status-bot · 2020-05-06T13:41:52Z

Your PR has finished running tests. There were no failures.

edx-pipeline-bot · 2020-05-06T14:55:00Z

EdX Release Notice: This PR has been deployed to the staging environment in preparation for a release to production.

edx-pipeline-bot · 2020-05-06T15:20:59Z

EdX Release Notice: This PR has been deployed to the production environment.

…y auth config fetching (#23824)" This reverts commit 7942064.

…y auth config fetching (#23824)" (#23935) This reverts commit 7942064.

timmc-edx requested a review from a team April 28, 2020 16:19

timmc-edx force-pushed the timmc/ARCHBOM-1139-login-site-queries branch from f8cf82e to fa1bd03 Compare April 28, 2020 17:49

timmc-edx changed the title ~~Fix 1+N DB queries in SAML config fetching on login page~~ Fix 1+N queries in third party auth config fetching Apr 28, 2020

feanil reviewed Apr 28, 2020

View reviewed changes

timmc-edx added 3 commits April 30, 2020 22:26

Add failing test to demonstrate multi-site provider shadowing bug

dd39270

Fix 1+N queries on login page; fix shadowing of providers between sites

7aa25b1

The filtering of SAML provider configs is the most likely culprit for what is causing a large number of requests for django_site when loading the login page in production. (ARCHBOM-1139)

Abstracting config retrieval and using KEY_FIELDS directly

894dc12

timmc-edx force-pushed the timmc/ARCHBOM-1139-login-site-queries branch from fa1bd03 to 894dc12 Compare May 1, 2020 18:30

schenedx suggested changes May 1, 2020

View reviewed changes

timmc-edx added 2 commits May 1, 2020 21:37

Turn get_enabled_by_backend_name into a filter on _enabled_providers

b0cfac6

Dark launch new TPA provider config retrieval

289137b

timmc-edx changed the title ~~Fix 1+N queries in third party auth config fetching~~ Dark launch: Fix 1+N queries in third party auth config fetching May 4, 2020

Remove extra newline (quality failure)

49bd35b

feanil approved these changes May 5, 2020

View reviewed changes

Cleanup, and comment for clarification; add request caching

ce8a0b7

Not moving `current_all` to django-config-models after all; it's enough of a hack that it doesn't really belong perfectly in either place. Some renaming and reworking of parameters.

timmc-edx force-pushed the timmc/ARCHBOM-1139-login-site-queries branch from ac09934 to ce8a0b7 Compare May 6, 2020 13:14

timmc-edx merged commit 7942064 into master May 6, 2020

timmc-edx deleted the timmc/ARCHBOM-1139-login-site-queries branch May 6, 2020 13:54

robrap added a commit that referenced this pull request May 7, 2020

Revert "Dark launch: Fix site shadowing and 1+N queries in third part…

6e972a2

…y auth config fetching (#23824)" This reverts commit 7942064.

robrap mentioned this pull request May 7, 2020

Revert "Dark launch: Fix 1+N queries in third party auth config fetching" #23935

Merged

robrap added a commit that referenced this pull request May 7, 2020

Revert "Dark launch: Fix site shadowing and 1+N queries in third part…

b98d200

…y auth config fetching (#23824)" (#23935) This reverts commit 7942064.

timmc-edx mentioned this pull request May 8, 2020

Address 1+N django_site queries on login page by comparing keys directly #23947

Merged

Uh oh!

Conversation

timmc-edx commented Apr 28, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nasthagiri commented Apr 30, 2020

Uh oh!

schenedx commented Apr 30, 2020

Uh oh!

timmc-edx commented Apr 30, 2020

Uh oh!

schenedx commented Apr 30, 2020

Uh oh!

timmc-edx commented Apr 30, 2020

Uh oh!

schenedx left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

timmc-edx commented May 1, 2020

Uh oh!

timmc-edx commented May 5, 2020

Uh oh!

edx-status-bot commented May 6, 2020

Uh oh!

edx-pipeline-bot commented May 6, 2020

Uh oh!

edx-pipeline-bot commented May 6, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

timmc-edx commented Apr 28, 2020 •

edited

Loading