starred/netbox
1
0
Fork 0
mirror of https://github.com/netbox-community/netbox.git synced 2026-01-13 05:50:33 +01:00
Code Issues 273 Packages Projects Releases 10 Wiki Activity

add note about regular session cleanup to installation instructions #3293

New Issue
Closed
opened 2025-12-29 18:27:29 +01:00 by adam · 14 comments
adam commented 2025-12-29 18:27:29 +01:00
Owner
Copy Link

Originally created by @netsandbox on GitHub (Feb 7, 2020).

Originally assigned to: @jeremystretch on GitHub.

Change Type

[ x] Addition
[ ] Correction
[ ] Deprecation
[ ] Cleanup (formatting, typos, etc.)

Area

[ x] Installation instructions
[ ] Configuration parameters
[ ] Functionality/features
[ ] REST API
[ ] Administration/development
[ ] Other

Proposed Changes

Django doesn't clean-up sessions by themself, but provides a command to do so:
https://docs.djangoproject.com/en/2.2/topics/http/sessions/#clearing-the-session-store

Maybe it is worth to add a short note to the installation instructions to setup a daily cronjob for this:

echo -e '#!/bin/sh\npython3 /opt/netbox/netbox/manage.py clearsessions' > /etc/cron.daily/netbox-clearsessions
chmod +x /etc/cron.daily/netbox-clearsessions
Originally created by @netsandbox on GitHub (Feb 7, 2020). Originally assigned to: @jeremystretch on GitHub. ### Change Type [ x] Addition [ ] Correction [ ] Deprecation [ ] Cleanup (formatting, typos, etc.) ### Area [ x] Installation instructions [ ] Configuration parameters [ ] Functionality/features [ ] REST API [ ] Administration/development [ ] Other ### Proposed Changes Django doesn't clean-up sessions by themself, but provides a command to do so: https://docs.djangoproject.com/en/2.2/topics/http/sessions/#clearing-the-session-store Maybe it is worth to add a short note to the installation instructions to setup a daily cronjob for this: ```shell echo -e '#!/bin/sh\npython3 /opt/netbox/netbox/manage.py clearsessions' > /etc/cron.daily/netbox-clearsessions chmod +x /etc/cron.daily/netbox-clearsessions ```
adam added the status: acceptedtype: feature labels 2025-12-29 18:27:29 +01:00
adam closed this issue 2025-12-29 18:27:29 +01:00
adam commented 2025-12-29 18:27:29 +01:00
Author
Owner
Copy Link

@kobayashi commented on GitHub (Feb 10, 2020):

This may be worth to add in Administration Section of netbox docs.

@kobayashi commented on GitHub (Feb 10, 2020): This may be worth to add in Administration Section of netbox docs.
adam commented 2025-12-29 18:27:29 +01:00
Author
Owner
Copy Link

@hSaria commented on GitHub (Feb 10, 2020):

Might be better to implement this in NetBox instead of adding another thing to be maintained by the user. I personally don't see an issue in having it run on every login considering it's a single query and web logins are generally few and far between. Or maybe even doing it in background.

The clearsessions command calls

engine = import_module(settings.SESSION_ENGINE)
try:
    engine.SessionStore.clear_expired()
except NotImplementedError:
    self.stderr.write("Session engine '%s' doesn't support clearing "
                      "expired sessions.\n" % settings.SESSION_ENGINE)
@hSaria commented on GitHub (Feb 10, 2020): Might be better to implement this in NetBox instead of adding another thing to be maintained by the user. I personally don't see an issue in having it run on every login considering it's a single query and web logins are generally few and far between. Or maybe even doing it in background. The `clearsessions` command calls ```python engine = import_module(settings.SESSION_ENGINE) try: engine.SessionStore.clear_expired() except NotImplementedError: self.stderr.write("Session engine '%s' doesn't support clearing " "expired sessions.\n" % settings.SESSION_ENGINE) ```
adam commented 2025-12-29 18:27:29 +01:00
Author
Owner
Copy Link

@netsandbox commented on GitHub (Feb 11, 2020):

I don't think that it makes sense to run on every login because:

  1. clearsessions works only for file and database sessions and the user maybe use another session engine
  2. if LOGIN_TIMEOUTis > one day (default is 14 days) it doesn't make sense to run clearsessions more than once a day, which can happen if a user login and logout multiple times a day
  3. this would slow down the login (clearsessions needs on my host 2 seconds)
@netsandbox commented on GitHub (Feb 11, 2020): I don't think that it makes sense to run on every login because: 1. clearsessions works only for file and database sessions and the user maybe use another session engine 2. if `LOGIN_TIMEOUT`is > one day (default is 14 days) it doesn't make sense to run clearsessions more than once a day, which can happen if a user login and logout multiple times a day 3. this would slow down the login (clearsessions needs on my host 2 seconds)
adam commented 2025-12-29 18:27:29 +01:00
Author
Owner
Copy Link

@hSaria commented on GitHub (Feb 11, 2020):

clearsessions works only for file and database sessions and the user maybe use another session engine

This is why the clearsessions already catches NotImplementedError.

if LOGIN_TIMEOUTis > one day (default is 14 days) it doesn't make sense to run clearsessions more than once a day, which can happen if a user login and logout multiple times a day

True, which is why I also suggested an alternative solution of "doing it in background."

this would slow down the login (clearsessions needs on my host 2 seconds)

It takes 2 seconds because every time you run a command through manage.py, it will load the modules and perform any other initialization tasks (e.g. checks on settings, connecting to db, etc...). In runtime, this near instantaneous (it's a single DB call or os.listdir + session.load per session).

I just dislike the idea of throwing more things at the user; the installation steps for a bare setup are already plenty long.

Edit: Performing this at logout is yet another way of doing it while minimizing the effects of login (you usually login more often than logout). Here's the logout diff, but doing it at login is just as fine.

diff --git a/netbox/users/views.py b/netbox/users/views.py
index 6a241027..bf5449f7 100644
--- a/netbox/users/views.py
+++ b/netbox/users/views.py
@@ -11,6 +11,7 @@ from django.utils.decorators import method_decorator
 from django.utils.http import is_safe_url
 from django.views.decorators.debug import sensitive_post_parameters
 from django.views.generic import View
+from importlib import import_module
 
 from secrets.forms import UserKeyForm
 from secrets.models import SessionKey, UserKey
@@ -74,6 +75,13 @@ class LogoutView(View):
         response = HttpResponseRedirect(reverse('home'))
         response.delete_cookie('session_key')
 
+        # Remove any expired sessions
+        engine = import_module(settings.SESSION_ENGINE)
+        try:
+            engine.SessionStore.clear_expired()
+        except NotImplementedError:
+            pass
+
         return response
@hSaria commented on GitHub (Feb 11, 2020): > clearsessions works only for file and database sessions and the user maybe use another session engine This is why the `clearsessions` already catches `NotImplementedError`. > if LOGIN_TIMEOUTis > one day (default is 14 days) it doesn't make sense to run clearsessions more than once a day, which can happen if a user login and logout multiple times a day True, which is why I also suggested an alternative solution of "doing it in background." > this would slow down the login (clearsessions needs on my host 2 seconds) It takes 2 seconds because every time you run a command through `manage.py`, it will load the modules and perform any other initialization tasks (e.g. checks on settings, connecting to db, etc...). In runtime, this near instantaneous (it's a single DB call or `os.listdir` + `session.load` per session). I just dislike the idea of throwing more things at the user; the installation steps for a *bare* setup are already plenty long. Edit: Performing this at logout is yet another way of doing it while minimizing the effects of login (you usually login more often than logout). Here's the logout diff, but doing it at login is just as fine. ```diff diff --git a/netbox/users/views.py b/netbox/users/views.py index 6a241027..bf5449f7 100644 --- a/netbox/users/views.py +++ b/netbox/users/views.py @@ -11,6 +11,7 @@ from django.utils.decorators import method_decorator from django.utils.http import is_safe_url from django.views.decorators.debug import sensitive_post_parameters from django.views.generic import View +from importlib import import_module from secrets.forms import UserKeyForm from secrets.models import SessionKey, UserKey @@ -74,6 +75,13 @@ class LogoutView(View): response = HttpResponseRedirect(reverse('home')) response.delete_cookie('session_key') + # Remove any expired sessions + engine = import_module(settings.SESSION_ENGINE) + try: + engine.SessionStore.clear_expired() + except NotImplementedError: + pass + return response ```
adam commented 2025-12-29 18:27:29 +01:00
Author
Owner
Copy Link

@kobayashi commented on GitHub (Feb 12, 2020):

IMO, it is better to leave session store as user preference. Some users want to keep clean session store, but other may not. If this would be implemented in netbox itself, that should be optional. In any case, documentation is required.

@kobayashi commented on GitHub (Feb 12, 2020): IMO, it is better to leave session store as user preference. Some users want to keep clean session store, but other may not. If this would be implemented in netbox itself, that should be optional. In any case, documentation is required.
adam commented 2025-12-29 18:27:30 +01:00
Author
Owner
Copy Link

@jeremystretch commented on GitHub (Feb 13, 2020):

This brings to mind an unrelated but similar task that we currently handle in middleware: The deletion of expired ObjectChange records. Maybe it makes sense to establish a "housekeeping" management command that can be invoked via a cron task to handle these tasks and any other routine maintenance.

It might also be possible to do something clever using a Redis queue to handle running the task. Not sure what the catalyst would be to enqueue the job though.

@jeremystretch commented on GitHub (Feb 13, 2020): This brings to mind an unrelated but similar task that we currently [handle in middleware](https://github.com/netbox-community/netbox/blob/0c89534bfba2f3a32a97e617de4f9a1cbe95b91a/netbox/extras/middleware.py#L127:L133): The deletion of expired ObjectChange records. Maybe it makes sense to establish a "housekeeping" management command that can be invoked via a cron task to handle these tasks and any other routine maintenance. It might also be possible to do something clever using a Redis queue to handle running the task. Not sure what the catalyst would be to enqueue the job though.
adam commented 2025-12-29 18:27:30 +01:00
Author
Owner
Copy Link

@hSaria commented on GitHub (Feb 13, 2020):

Maybe it makes sense to establish a "housekeeping" management command that can be invoked via a cron task to handle these tasks and any other routine maintenance.

I would personally avoid giving the user access to those things because some may excessively use it or, even worse, use it as a temporary remediation task. If it's something that can be coded internally, then let the software handle itself.

It might also be possible to do something clever using a Redis queue to handle running the task. Not sure what the catalyst would be to enqueue the job though.

Considering a login generates the row, maybe that could be the trigger?

@hSaria commented on GitHub (Feb 13, 2020): > Maybe it makes sense to establish a "housekeeping" management command that can be invoked via a cron task to handle these tasks and any other routine maintenance. I would _personally_ avoid giving the user access to those things because some may excessively use it or, even worse, use it as a temporary remediation task. If it's something that can be coded internally, then let the software handle itself. > It might also be possible to do something clever using a Redis queue to handle running the task. Not sure what the catalyst would be to enqueue the job though. Considering a login generates the row, maybe that could be the trigger?
adam commented 2025-12-29 18:27:30 +01:00
Author
Owner
Copy Link

@jeremystretch commented on GitHub (Feb 13, 2020):

I should clarify: by "any other routine maintenance," I mean any other built-in tasks that we identify in the future, not something that would be configurable by the end user. The management command would be idempotent, simply deleting any applicable records upon each execution, so there's no danger of a user running it "excessively."

@jeremystretch commented on GitHub (Feb 13, 2020): I should clarify: by "any other routine maintenance," I mean any other built-in tasks that we identify in the future, not something that would be configurable by the end user. The management command would be idempotent, simply deleting any applicable records upon each execution, so there's no danger of a user running it "excessively."
adam commented 2025-12-29 18:27:30 +01:00
Author
Owner
Copy Link

@DanSheps commented on GitHub (Feb 13, 2020):

Instead of Cron, could we queue these with rq?

@DanSheps commented on GitHub (Feb 13, 2020): Instead of Cron, could we queue these with rq?
adam commented 2025-12-29 18:27:30 +01:00
Author
Owner
Copy Link

@jeremystretch commented on GitHub (Feb 13, 2020):

@DanSheps Yeah, that's what I was getting at. We'd still need a way to trigger the initial queuing of the job though.

@jeremystretch commented on GitHub (Feb 13, 2020): @DanSheps Yeah, that's what I was getting at. We'd still need a way to trigger the initial queuing of the job though.
adam commented 2025-12-29 18:27:30 +01:00
Author
Owner
Copy Link

@DanSheps commented on GitHub (Feb 13, 2020):

What about a like you said, a management command but also an API endpoint and a configurable option in settings on when to run, give people 3 different options so they can choose what is best?

@DanSheps commented on GitHub (Feb 13, 2020): What about a like you said, a management command but also an API endpoint and a configurable option in settings on when to run, give people 3 different options so they can choose what is best?
adam commented 2025-12-29 18:27:30 +01:00
Author
Owner
Copy Link

@tyler-8 commented on GitHub (Feb 25, 2020):

The best way to approach this is to move session handling to a cache - it's already self-invalidating and doesn't require any regular maintenance to clean up old sessions.

And since redis is now mandatory, we should have all the dependencies we need to customize this

The Django docs even suggest a cache-based session mechanism as the ideal over both database and file.

@tyler-8 commented on GitHub (Feb 25, 2020): The best way to approach this is to move session handling to a cache - it's already self-invalidating and doesn't require any regular maintenance to clean up old sessions. And since `redis` is now mandatory, we should have all the dependencies we need [to customize this](https://apirobot.me/posts/tale-about-redis-and-sessions-in-django) The Django [docs even suggest](https://docs.djangoproject.com/en/3.0/topics/http/sessions/#using-cached-sessions) a cache-based session mechanism as the ideal over both database and file.
adam commented 2025-12-29 18:27:30 +01:00
Author
Owner
Copy Link

@jeremystretch commented on GitHub (Feb 27, 2020):

This isn't a strong case for introducing the cache backend when a simple solution (the built-in clearsessions command) already exists. I'd rather not invite new problems while solving a very minimal one. Carrying even a few thousand stale sessions in the database has such a negligible impact on performance I think we'd be fine just calling clearsessions in the upgrade script.

@jeremystretch commented on GitHub (Feb 27, 2020): This isn't a strong case for introducing the cache backend when a simple solution (the built-in `clearsessions` command) already exists. I'd rather not invite new problems while solving a very minimal one. Carrying even a few thousand stale sessions in the database has such a negligible impact on performance I think we'd be fine just calling `clearsessions` in the upgrade script.
adam commented 2025-12-29 18:27:30 +01:00
Author
Owner
Copy Link

@jeremystretch commented on GitHub (Mar 4, 2020):

I've added the command manage.py clearsessions to the upgrade script and made a note of it in the documentation.

@jeremystretch commented on GitHub (Mar 4, 2020): I've added the command `manage.py clearsessions` to the upgrade script and made a note of it in the documentation.
Sign in to join this conversation.
No Branch/Tag Specified
Branches Tags
Labels
Clear labels
beta
breaking change
complexity: high
complexity: low
complexity: medium
needs milestone
netbox
pending closure
plugin candidate
pull-request
Mirrored from GitHub Pull Request
 severity: high
severity: low
severity: medium
status: accepted
status: backlog
status: blocked
status: duplicate
status: needs owner
status: needs triage
status: revisions needed
status: under review
topic: GraphQL
topic: Internationalization
topic: OpenAPI
topic: UI/UX
topic: cabling
topic: event rules
topic: htmx navigation
topic: industrialization
topic: migrations
topic: plugins
topic: scripts
topic: templating
topic: testing
type: bug
type: deprecation
type: documentation
type: feature
type: housekeeping
type: translation
No Label status: accepted type: feature
Milestone
No items
No Milestone
Projects
Clear projects
No project
Assignees
Clear assignees
adam
No Assignees
1 Participants
Notifications
Due Date
No due date set.
Dependencies

No dependencies set.

Reference: starred/netbox#3293
Delete Branch "%!s()"

Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?