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

fix(dao) fix incorrect function field reassignment #8789

Conversation

marc-charpentier
Copy link
Contributor

The Plugins DAO's select_by_cache_key translator function was not disabled even if checked migration had actually been executed before.

Summary

In order to load a new plugin entity, a worker executes the Plugins.select_by_cache_key function (call to kong.db.plugins:select_by_cache_key(...), which delegates the load to DAO:select_by_cache_key, and checks whether the core.009_200_to_210 migration was executed previously.
If yes, before returning the loaded entity, the Plugins.select_by_cache_key function tries to self-replace by the DAO's one, but it doesn't work because the assigned field's object isn't the right one.
As an impact, an important extra latency is added to the plugin's load one, for each request involving such load.

This PR is to fix this behavior, so that extra latency may happen only once per worker.

Full changelog

  • [Fix ever checking core.009_200_to_210 execution on plugin load]

Issue reference

Issue #8788

The Plugins DAO's select_by_cache_key translator function was not
disabled even if checked migration had actually been executed before.
@CLAassistant
Copy link

CLAassistant commented May 11, 2022

CLA assistant check
All committers have signed the CLA.

@bungle
Copy link
Member

bungle commented Jun 2, 2022

@marc-charpentier as we are approaching 3.0, I think we can remove this check altogether, right @hishamhm?

@hishamhm
Copy link
Contributor

hishamhm commented Jun 2, 2022

@bungle in the past we have removed those checks once their corresponding migrations were "squashed". I don't know which migrations will be squashed for 3.0, but if 200_to_210 migration file stops existing in the 3.0 codebase then I believe this translator function can go away as well in 3.0.

bungle added a commit that referenced this pull request Jun 3, 2022
### Summary

When Kong 2.1.0 was release we needed to add special workaround to enable
blue-green deployment with the introduction of workspaces.

We needed to add special `select_by_cache_key` to `Plugins` DAO. The check
that we added is hurting the Kong performance, as seen reported in #8788.
We got further PR from the author with #8789. While the fix is good,
I do think we can remove this by now altogether. The only downside of this
is that is people migrate directly from Kong < 2.1.x to `3.0.0` we don't
support the blue-green deployment.

### Issues Resolved

Fixes #8788
Closes #8789
@marc-charpentier
Copy link
Contributor Author

@bungle Indeed, with my workmates we chose to remove the translator function altogether when deploying Kong 2.8 in our environments because we ensure that all migrations are run before.

@bungle bungle closed this in #8896 Jun 14, 2022
bungle added a commit that referenced this pull request Jun 14, 2022
### Summary

When Kong 2.1.0 was release we needed to add special workaround to enable
blue-green deployment with the introduction of workspaces.

We needed to add special `select_by_cache_key` to `Plugins` DAO. The check
that we added is hurting the Kong performance, as seen reported in #8788.
We got further PR from the author with #8789. While the fix is good,
I do think we can remove this by now altogether. The only downside of this
is that is people migrate directly from Kong < 2.1.x to `3.0.0` we don't
support the blue-green deployment.

### Issues Resolved

Fixes #8788
Closes #8789
@marc-charpentier
Copy link
Contributor Author

@bungle
Hello,
And sorry for this late answer.
Although we chose to remove the function for our own Kong instances, I had wished to contribute with a fix not thwarting blue-green deployment support, and thought it would be nice to have it in a 2.8.2+ release.
Also, I suppose the function's complete removal was possible for us because we chose the 'no downtime way' involving a Cassandra keyspace copy and migration before starting 2.8 nodes.

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

Successfully merging this pull request may close these issues.

4 participants