From: Tom Clegg <tom@curoverse.com>
Date: Thu, 23 Mar 2017 19:18:51 +0000 (-0400)
Subject: 11065: Update comments.
X-Git-Tag: 1.1.0~360^2~1
X-Git-Url: https://git.arvados.org/arvados.git/commitdiff_plain/6722b420effcab24dffd9b47fa277f8830bd4cca?ds=sidebyside

11065: Update comments.
---

diff --git a/services/api/config/application.default.yml b/services/api/config/application.default.yml
index fbecc964b5..5241cb4378 100644
--- a/services/api/config/application.default.yml
+++ b/services/api/config/application.default.yml
@@ -232,10 +232,12 @@ common:
   # crunchstat logs from the logs table.
   clean_container_log_rows_after: <%= 30.days %>
 
-  # Time to keep audit logs (a row in the log table added each time an
-  # Arvados object is created, modified, or deleted) in the PostgreSQL
-  # database. Currently, websocket event notifications rely on audit
-  # logs, so this should not be set lower than 600 (5 minutes).
+  # Time to keep audit logs, in seconds. (An audit log is a row added
+  # to the "logs" table in the PostgreSQL database each time an
+  # Arvados object is created, modified, or deleted.)
+  #
+  # Currently, websocket event notifications rely on audit logs, so
+  # this should not be set lower than 600 (5 minutes).
   max_audit_log_age: 1209600
 
   # Maximum number of log rows to delete in a single SQL transaction.
@@ -480,4 +482,3 @@ test:
   websocket_address: <% if ENV['ARVADOS_TEST_EXPERIMENTAL_WS'] %>"wss://0.0.0.0:<%= ENV['ARVADOS_TEST_WSS_PORT'] %>/websocket"<% else %>false<% end %>
   trash_sweep_interval: -1
   docker_image_formats: ["v1"]
-  max_audit_log_delete_batch: 0
diff --git a/services/api/test/unit/log_test.rb b/services/api/test/unit/log_test.rb
index 370bec1101..7376876bb4 100644
--- a/services/api/test/unit/log_test.rb
+++ b/services/api/test/unit/log_test.rb
@@ -323,6 +323,8 @@ class LogTest < ActiveSupport::TestCase
     Log.unscoped.where('event_type in (?)', %w(create update destroy delete))
   end
 
+  # Default settings should not delete anything -- some sites rely on
+  # the original "keep everything forever" behavior.
   test 'retain old audit logs with default settings' do
     assert_no_logs_deleted do
       AuditLogs.delete_old(
@@ -331,20 +333,26 @@ class LogTest < ActiveSupport::TestCase
     end
   end
 
-  test 'retain old audit logs with batch=0' do
+  # Batch size 0 should retain all logs -- even if max_age is very
+  # short, and even if the default settings (and associated test) have
+  # changed.
+  test 'retain old audit logs with max_audit_log_delete_batch=0' do
     assert_no_logs_deleted do
       AuditLogs.delete_old(max_age: 1, max_batch: 0)
     end
   end
 
+  # We recommend a more conservative age of 5 minutes for production,
+  # but 3 minutes suits our test data better (and is test-worthy in
+  # that it's expected to work correctly in production).
   test 'delete old audit logs with production settings' do
     initial_log_count = Log.unscoped.all.count
     AuditLogs.delete_old(max_age: 180, max_batch: 100000)
     assert_operator remaining_audit_logs.count, :<, initial_log_count
   end
 
-  test 'delete old audit logs in multiple batches' do
-    AuditLogs.delete_old(max_age: 0, max_batch: 2)
+  test 'delete all audit logs in multiple batches' do
+    AuditLogs.delete_old(max_age: 0.00001, max_batch: 2)
     assert_equal [], remaining_audit_logs.collect(&:uuid)
   end