add note about regular session cleanup to installation instructions

[netsandbox]

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

[kobayashi]

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

[hSaria]

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)

[netsandbox]

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)

[hSaria]

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

[kobayashi]

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.

[jeremystretch]

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.

[hSaria]

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?

[jeremystretch]

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."

[DanSheps]

Instead of Cron, could we queue these with rq?

[jeremystretch]

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

[DanSheps]

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?

[tyler-8]

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.

[jeremystretch]

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]

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