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

[IMP] base: restrict _gc_sessions to a single database #152880

Closed
wants to merge 1 commit into from

Conversation

ghost
Copy link

@ghost ghost commented Feb 6, 2024

Background

Previously, _gc_sessions ran for all databases regardless of the active one. This could cause problems in SaaS environments with shared session storage across databases.

Change

We propose introducing a limitation within the _gc_sessions method. The method will now check for the presence of the ODOO_SKIP_GC_SESSIONS environment variable. If set, it skips the cleanup process. This behavior is typically desired in environments launched without a specified database. On the other hand, if the environment variable is not set, the method will likely assume a non-SaaS environment (e.g., on-premise or development) and continue with the regular cleanup process.

Impact

Ensures isolated session management for individual databases in SaaS environments.
Maintains automatic cleanup for developers and on-premise users.
task-3716956

@robodoo
Copy link
Contributor

robodoo commented Feb 6, 2024

Pull request status dashboard.

@C3POdoo C3POdoo added the OE the report is linked to a support ticket (opw-...) label Feb 6, 2024
@ghost ghost force-pushed the master-limit_gc_sessions-joda branch 2 times, most recently from e08380f to 588ca2e Compare February 8, 2024 12:20
@ghost ghost requested a review from Julien00859 March 13, 2024 11:19
Copy link
Member

@Julien00859 Julien00859 left a comment

Choose a reason for hiding this comment

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

Don't just copy/paste the task spec inside the commit message. You don't frame a commit message like you frame a specification. Please adapt it so it better fit what we usually read inside commit messages.

odoo/addons/base/models/ir_http.py Outdated Show resolved Hide resolved
@ghost ghost force-pushed the master-limit_gc_sessions-joda branch from 588ca2e to c331760 Compare April 9, 2024 07:35
@ghost ghost marked this pull request as ready for review April 9, 2024 11:27
@C3POdoo C3POdoo requested review from a team, rco-odoo, HydrionBurst and Julien00859 and removed request for a team April 9, 2024 11:36
@ghost ghost force-pushed the master-limit_gc_sessions-joda branch from c331760 to 7e8858d Compare April 9, 2024 11:37
@xmo-odoo
Copy link
Collaborator

xmo-odoo commented Apr 9, 2024

This ensures session management remains isolated for individual databases in SaaS environments. Developers and on-premise users will continue to benefit from automatic cleanup.

That seems debatable, any on-premise user who has multiple databases on the same server will be tripped by this.

@Julien00859
Copy link
Member

I agree with xmo, anyone running a large system (think of our partner who are running their own SaaS plateform) kinda benefits from _gc_session and has been for a couple of years now. That we silently do not gc the session anymore will be a bad surprise for them. I think that adding a logger.info when the gc is skipped is the minimum we shall do but I think that it deserves more. On the other hand we kinda don't want it to be too much advertised for obvious reason...

@xmo-odoo
Copy link
Collaborator

I think it should probably be a configuration flag of some sort, this way it's easy to toggle (including for partners if they get to a scale where it becomes useful), but it doesn't take people by surprise.

@@ -270,6 +271,9 @@ def routing_map(self, key=None):

@api.autovacuum
def _gc_sessions(self):
if not config["db_name"]:
Copy link
Member

@Julien00859 Julien00859 Apr 10, 2024

Choose a reason for hiding this comment

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

So something like so?

Suggested change
if not config["db_name"]:
if os.getenv('ODOO_SKIP_GC_SESSIONS'):
# on some deployment, a POSIX cron is used instead

@ghost ghost force-pushed the master-limit_gc_sessions-joda branch 3 times, most recently from ddc5ff6 to c953f5b Compare April 10, 2024 12:56
@ghost ghost force-pushed the master-limit_gc_sessions-joda branch from c953f5b to 939d3ea Compare April 10, 2024 13:18
@ghost ghost force-pushed the master-limit_gc_sessions-joda branch 2 times, most recently from 1b24cd2 to 80ab128 Compare April 10, 2024 13:20
@ghost
Copy link
Author

ghost commented Apr 12, 2024

@odoo/rd-security can I have your review on this ?

@Julien00859
Copy link
Member

Julien00859 commented Apr 12, 2024

@joda-odoo red ci/style

I see you force-pushed your branch a few times, was it due to false-positive bugs on the runbot? I'm getting quite a lot these days myself. Sometimes it is faster to "just" re-build the specific build that failed right on the runbot dashboard:

image

You need to be connected on runbot (via oauth with odoo.com) to be able to see the button.

Copy link
Collaborator

@xmo-odoo xmo-odoo left a comment

Choose a reason for hiding this comment

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

@robodoo override=ci/security

Background
=========

Previously, _gc_sessions ran for all databases regardless of the active one. This could cause problems in SaaS environments with shared session storage across databases.

Change
======

We propose introducing a limitation within the _gc_sessions method. The method will now check for the presence of the ODOO_SKIP_GC_SESSIONS environment variable. If set, it skips the cleanup process. This behavior is typically desired in environments launched without a specified database. On the other hand, if the environment variable is not set, the method will likely assume a non-SaaS environment (e.g., on-premise or development) and continue with the regular cleanup process.

Impact
=====

Ensures isolated session management for individual databases in SaaS environments.
Maintains automatic cleanup for developers and on-premise users.
task-3716956
@ghost ghost force-pushed the master-limit_gc_sessions-joda branch from 80ab128 to 8bbaae0 Compare April 23, 2024 07:14
@Julien00859
Copy link
Member

@robodoo r+

robodoo pushed a commit that referenced this pull request Apr 23, 2024
Background
=========

Previously, _gc_sessions ran for all databases regardless of the active one. This could cause problems in SaaS environments with shared session storage across databases.

Change
======

We propose introducing a limitation within the _gc_sessions method. The method will now check for the presence of the ODOO_SKIP_GC_SESSIONS environment variable. If set, it skips the cleanup process. This behavior is typically desired in environments launched without a specified database. On the other hand, if the environment variable is not set, the method will likely assume a non-SaaS environment (e.g., on-premise or development) and continue with the regular cleanup process.

Impact
=====

Ensures isolated session management for individual databases in SaaS environments.
Maintains automatic cleanup for developers and on-premise users.
task-3716956

closes #152880

Signed-off-by: Julien Castiaux (juc) <juc@odoo.com>
@robodoo robodoo closed this Apr 23, 2024
@robodoo robodoo added the 17.3 label Apr 23, 2024
@fw-bot fw-bot deleted the master-limit_gc_sessions-joda branch May 7, 2024 16:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
17.3 OE the report is linked to a support ticket (opw-...)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants