-
Notifications
You must be signed in to change notification settings - Fork 0
Prevent Goggles memory amplification and contain worker pressure #230
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -34,6 +34,14 @@ services: | |
| build: . | ||
| init: true | ||
| restart: unless-stopped | ||
| # Keep a pathological request inside the Goggles cgroup instead of letting | ||
| # it exhaust host RAM and swap. Equal memory/swap limits disable additional | ||
| # swap allocation for this container. | ||
| mem_limit: ${GOGGLES_WEB_MEMORY_LIMIT:-16g} | ||
| memswap_limit: ${GOGGLES_WEB_MEMORY_LIMIT:-16g} | ||
|
Comment on lines
+40
to
+41
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win 🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
# Locate and inspect the compose file around the cited lines.
file="docker-compose.yml"
wc -l "$file"
cat -n "$file" | sed -n '1,140p'
# Show only the service block if present.
rg -n "mem_limit|memswap_limit|env_file|GOGGLES_WEB_MEMORY_LIMIT|GOGGLES_ENV_FILE" "$file"Repository: marmot-protocol/goggles Length of output: 3742 🏁 Script executed: #!/bin/bash
set -euo pipefail
# Inspect repository references for the environment-file mechanism and memory limit variable.
rg -n "GOGGLES_ENV_FILE|GOGGLES_WEB_MEMORY_LIMIT|docker compose|docker-compose" .
# Look for docs explaining how the env file is meant to be supplied to Compose.
fd -a -t f 'README*|*.md' .Repository: marmot-protocol/goggles Length of output: 2126 🏁 Script executed: #!/bin/bash
set -euo pipefail
# Read the documentation around the memory limit configuration and env-file usage.
sed -n '120,150p' README.md
printf '\n---\n'
sed -n '60,90p' docs/deployment.md
printf '\n---\n'
sed -n '1,80p' .env.example 2>/dev/null || trueRepository: marmot-protocol/goggles Length of output: 4029 Use a Compose-time source for the memory limit. 🤖 Prompt for AI Agents |
||
| mem_swappiness: 0 | ||
| cpus: ${GOGGLES_WEB_CPUS:-2.0} | ||
| pids_limit: ${GOGGLES_WEB_PIDS_LIMIT:-256} | ||
| env_file: ${GOGGLES_ENV_FILE:-.env} | ||
|
Comment on lines
+40
to
45
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Issue:
Verified with Moving the same value into a top-level Worth either (a) documenting the constraint explicitly (" |
||
| depends_on: | ||
| db: | ||
|
|
@@ -45,9 +53,18 @@ services: | |
| command: > | ||
| sh -c "python manage.py migrate --noinput && | ||
| python manage.py collectstatic --noinput && | ||
| gunicorn config.wsgi:application --bind 0.0.0.0:8000 | ||
| --workers 3 --threads 4 --timeout 300 | ||
| --max-requests 500 --max-requests-jitter 50" | ||
| exec gunicorn config.wsgi:application | ||
| --bind 0.0.0.0:8000 | ||
| --workers $${GOGGLES_WEB_WORKERS:-3} | ||
| --threads $${GOGGLES_WEB_THREADS:-4} | ||
| --timeout $${GOGGLES_WEB_TIMEOUT_SECONDS:-300} | ||
| --max-requests $${GOGGLES_WEB_MAX_REQUESTS:-500} | ||
| --max-requests-jitter $${GOGGLES_WEB_MAX_REQUESTS_JITTER:-50}" | ||
| logging: | ||
| driver: json-file | ||
| options: | ||
| max-size: ${GOGGLES_WEB_LOG_MAX_SIZE:-20m} | ||
| max-file: ${GOGGLES_WEB_LOG_MAX_FILES:-5} | ||
| healthcheck: | ||
| test: | ||
| [ | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,4 +1,5 @@ | ||
| from django.contrib import admin | ||
| from django.db.models.functions import Length, Substr | ||
| from django.urls import reverse | ||
| from django.utils.html import format_html | ||
| from django.utils.http import urlencode | ||
|
|
@@ -97,6 +98,12 @@ class AuditFileAdmin(admin.ModelAdmin): | |
| "events_link", | ||
| ) | ||
|
|
||
| def get_queryset(self, request): | ||
| # Admin changelists/autocomplete render metadata only. Keep the complete | ||
| # upload body out of those page-sized query results; the change-page | ||
| # preview intentionally resolves the one deferred body it displays. | ||
| return super().get_queryset(request).defer("raw_text", "user_agent") | ||
|
|
||
| @admin.display(description="Raw text (preview)") | ||
| def raw_text_preview(self, obj): | ||
| """Bounded, read-only preview of the uploaded JSONL. | ||
|
|
@@ -167,6 +174,78 @@ class AuditEventAdmin(admin.ModelAdmin): | |
| ) | ||
| autocomplete_fields = ("audit_file", "group") | ||
| show_full_result_count = False | ||
| # Raw evidence is immutable forensic input and can be as large as the | ||
| # complete quarantined upload. Keep it out of Django's generated form and | ||
| # expose one bounded line preview plus an intentional link to the complete | ||
| # evidence response. | ||
| exclude = ( | ||
| "raw_line", | ||
| "raw_event", | ||
| "raw_kind", | ||
| "raw_context", | ||
| "context_human_action", | ||
| "context_transport", | ||
| "context_engine", | ||
| "context_group", | ||
| "context_convergence", | ||
| "context_source", | ||
| ) | ||
| readonly_fields = ("raw_line_preview", "evidence_link") | ||
|
|
||
| def get_queryset(self, request): | ||
| return ( | ||
| super() | ||
| .get_queryset(request) | ||
| .defer( | ||
| "raw_line", | ||
| "raw_event", | ||
| "raw_kind", | ||
| "raw_context", | ||
| "context_human_action", | ||
| "context_transport", | ||
| "context_engine", | ||
| "context_group", | ||
| "context_convergence", | ||
| "context_source", | ||
| ) | ||
| ) | ||
|
Comment on lines
+195
to
+211
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Issue: This This matters more after this PR than before it: Confirmed directly: opening the admin change page for a quarantined event with a 2 MiB Given |
||
|
|
||
| @admin.display(description="Raw line (preview)") | ||
| def raw_line_preview(self, obj): | ||
| if obj is None or obj.pk is None: | ||
| return "\u2014" | ||
| # Slice in SQL: resolving the deferred TextField first would still pull | ||
| # a quarantined 50 MiB upload into the worker merely to render 2,000 | ||
| # characters of it. | ||
| row = ( | ||
| AuditEvent.objects.filter(pk=obj.pk) | ||
| .annotate( | ||
| preview=Substr("raw_line", 1, RAW_TEXT_PREVIEW_CHARS), | ||
| total_chars=Length("raw_line"), | ||
| ) | ||
| .values("preview", "total_chars") | ||
| .get() | ||
| ) | ||
| preview = row["preview"] or "" | ||
| total_chars = row["total_chars"] or 0 | ||
| notice = ( | ||
| f"Showing first {RAW_TEXT_PREVIEW_CHARS} of {total_chars} characters." | ||
| if total_chars > RAW_TEXT_PREVIEW_CHARS | ||
| else f"{total_chars} characters." | ||
| ) | ||
| return format_html( | ||
| '<p><em>{}</em></p><pre style="max-height: 24em; overflow: auto; ' | ||
| 'white-space: pre-wrap;">{}</pre>', | ||
| notice, | ||
| preview, | ||
| ) | ||
|
|
||
| @admin.display(description="Complete evidence") | ||
| def evidence_link(self, obj): | ||
| if obj is None or obj.pk is None: | ||
| return "\u2014" | ||
| url = reverse("api-event-evidence", kwargs={"event_id": obj.pk}) | ||
| return format_html('<a href="{}">Open JSON evidence</a>', url) | ||
|
|
||
| def lookup_allowed(self, lookup, value, request): | ||
| if lookup == "audit_file__id__exact": | ||
|
|
@@ -181,6 +260,9 @@ class AnalysisRunAdmin(admin.ModelAdmin): | |
| autocomplete_fields = ("group", "created_by") | ||
| readonly_fields = ("created_at",) | ||
|
|
||
| def get_queryset(self, request): | ||
| return super().get_queryset(request).defer("report_json") | ||
|
|
||
|
|
||
| @admin.register(DeliveryArtifact) | ||
| class DeliveryArtifactAdmin(admin.ModelAdmin): | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Suggestion: this PR documents the pre-existing
GOGGLES_UPLOADS_ENABLEDkill switch for the first time but leaves its siblingGOGGLES_EXPORTS_ENABLED(streaming group export) undocumented in.env.example. Since this section is already being touched, worth adding it too.