Move Aphlict logging and PID configuration options to config file

Summary: Ref T10697. Mostly straightforward. Also allow the server to have multiple logs and log options in the future (e.g., different verbosities or separate admin/client logs or whatever). No specific plans for this, but the default log is pretty noisy today.

Test Plan: Set up a couple of logs, started server, saw it log to them.

Reviewers: chad

Reviewed By: chad

Maniphest Tasks: T10697

Differential Revision: https://secure.phabricator.com/D15702

Author: epriestley

conf/aphlict/aphlict.default.json

@@ -14,5 +14,11 @@
       "ssl.key": null,
       "ssl.cert": null
     }
-  ]
+  ],
+  "logs": [
+    {
+      "path": "/var/log/aphlict.log"
+    }
+  ],
+  "pidfile": "/var/tmp/aphlict/pid/aphlict.pid"
 }

src/applications/aphlict/management/PhabricatorAphlictManagementWorkflow.php

@@ -4,6 +4,7 @@ abstract class PhabricatorAphlictManagementWorkflow
   extends PhabricatorManagementWorkflow {
 
   private $debug = false;
+  private $configData;
   private $configPath;
 
   final protected function setDebug($debug) {
@@ -74,6 +75,8 @@ protected function parseLaunchArguments(PhutilArgumentParser $args) {
         $data,
         array(
           'servers' => 'list<wild>',
+          'logs' => 'optional list<wild>',
+          'pidfile' => 'string',
         ));
     } catch (Exception $ex) {
       throw new PhutilArgumentUsageException(
@@ -174,43 +177,54 @@ protected function parseLaunchArguments(PhutilArgumentParser $args) {
           'admin'));
     }
 
-    $this->configPath = $full_path;
-  }
+    $logs = $data['logs'];
+    foreach ($logs as $index => $log) {
+      PhutilTypeSpec::checkMap(
+        $log,
+        array(
+          'path' => 'string',
+        ));
 
-  final public function getPIDPath() {
-    $path = PhabricatorEnv::getEnvConfig('notification.pidfile');
+      $path = $log['path'];
 
-    try {
-      $dir = dirname($path);
-      if (!Filesystem::pathExists($dir)) {
-        Filesystem::createDirectory($dir, 0755, true);
+      try {
+        $dir = dirname($path);
+        if (!Filesystem::pathExists($dir)) {
+          Filesystem::createDirectory($dir, 0755, true);
+        }
+      } catch (FilesystemException $ex) {
+        throw new PhutilArgumentUsageException(
+          pht(
+            'Failed to create directory "%s" for specified log file (with '.
+            'index "%s"). You should manually create this directory or '.
+            'choose a different logfile location. %s',
+            $dir,
+            $ex->getMessage()));
       }
-    } catch (FilesystemException $ex) {
-      throw new Exception(
-        pht(
-          "Failed to create '%s'. You should manually create this directory.",
-          $dir));
     }
 
-    return $path;
-  }
-
-  final public function getLogPath() {
-    $path = PhabricatorEnv::getEnvConfig('notification.log');
+    $this->configData = $data;
+    $this->configPath = $full_path;
 
+    $pid_path = $this->getPIDPath();
     try {
       $dir = dirname($path);
       if (!Filesystem::pathExists($dir)) {
         Filesystem::createDirectory($dir, 0755, true);
       }
     } catch (FilesystemException $ex) {
-      throw new Exception(
+      throw new PhutilArgumentUsageException(
         pht(
-          "Failed to create '%s'. You should manually create this directory.",
-          $dir));
+          'Failed to create directory "%s" for specified PID file. You '.
+          'should manually create this directory or choose a different '.
+          'PID file location. %s',
+          $dir,
+          $ex->getMessage()));
     }
+  }
 
-    return $path;
+  final public function getPIDPath() {
+    return $this->configData['pidfile'];
   }
 
   final public function getPID() {
@@ -293,12 +307,8 @@ final protected function willLaunch() {
   }
 
   private function getServerArgv() {
-    $log = $this->getLogPath();
-
     $server_argv = array();
     $server_argv[] = '--config='.$this->configPath;
-    $server_argv[] = '--log='.$log;
-
     return $server_argv;
   }
 

src/applications/config/check/PhabricatorExtraConfigSetupCheck.php

@@ -305,6 +305,8 @@ public static function getAncientConfig() {
 
       'notification.ssl-cert' => $aphlict_reason,
       'notification.ssl-key' => $aphlict_reason,
+      'notification.pidfile' => $aphlict_reason,
+      'notification.log' => $aphlict_reason,
     );
 
     return $ancient_config;

src/applications/config/option/PhabricatorNotificationConfigOptions.php

@@ -44,13 +44,6 @@ public function getOptions() {
         'string',
         'http://localhost:22281/')
         ->setDescription(pht('Location of the notification receiver server.')),
-      $this->newOption('notification.log', 'string', '/var/log/aphlict.log')
-        ->setDescription(pht('Location of the server log file.')),
-      $this->newOption(
-        'notification.pidfile',
-        'string',
-        '/var/tmp/aphlict/pid/aphlict.pid')
-        ->setDescription(pht('Location of the server PID file.')),
     );
   }
 

src/docs/user/configuration/notifications.diviner

@@ -75,7 +75,9 @@ it exists) or specify a configuration file explicitly with the `--config` flag:
 
 The configuration file has these settings:
 
-  - `servers`: A list of servers to start.
+  - `servers`: //Required list.// A list of servers to start.
+  - `logs`: //Optional list.// A list of logs to write to.
+  - `pidfile`: //Required string.// Path to a PID file.
 
 Each server in the `servers` list should be an object with these keys:
 
@@ -91,6 +93,10 @@ Each server in the `servers` list should be an object with these keys:
   - `ssl.cert`: //Optional string.// If you want to use SSL on this port,
     the path to an SSL certificate.
 
+Each log in the `logs` list should be an object with these keys:
+
+  - `path`: //Required string.// Path to the log file.
+
 The defaults are appropriate for simple cases, but you may need to adjust them
 if you are running a more complex configuration.
 
@@ -104,9 +110,6 @@ You may also want to adjust these settings:
     connect to in order to listen for notifications.
   - `notification.server-uri` Internally-facing host and port that Phabricator
     will connect to in order to publish notifications.
-  - `notification.log` Log file location for the server.
-  - `notification.pidfile` Pidfile location used to stop any running server when
-    aphlict is restarted.
 
 
 Verifying Server Status

support/aphlict/server/aphlict_server.js

@@ -8,7 +8,6 @@ var fs = require('fs');
 
 function parse_command_line_arguments(argv) {
   var args = {
-    log: '/var/log/aphlict.log',
     test: false,
     config: null
   };
@@ -49,9 +48,9 @@ function set_exit_code(code) {
 
 process.on('uncaughtException', function(err) {
   var context = null;
-  if (err.code == 'EACCES' && err.path == args.log) {
+  if (err.code == 'EACCES') {
     context = util.format(
-      'Unable to open logfile ("%s"). Check that permissions are set ' +
+      'Unable to open file ("%s"). Check that permissions are set ' +
       'correctly.',
       err.path);
   }
@@ -68,11 +67,6 @@ process.on('uncaughtException', function(err) {
   set_exit_code(1);
 });
 
-// Add the logfile so we'll fail if we can't write to it.
-if (args.log) {
-  debug.addLog(args.log);
-}
-
 try {
   require('ws');
 } catch (ex) {
@@ -89,6 +83,12 @@ require('./lib/AphlictAdminServer');
 require('./lib/AphlictClientServer');
 
 var ii;
+
+var logs = config.logs || [];
+for (ii = 0; ii < logs.length; ii++) {
+  debug.addLog(logs[ii].path);
+}
+
 var servers = [];
 for (ii = 0; ii < config.servers.length; ii++) {
   var spec = config.servers[ii];
@@ -116,6 +116,10 @@ if (args.test) {
 
 debug.log('Starting servers (service PID %d).', process.pid);
 
+for (ii = 0; ii < logs.length; ii++) {
+  debug.log('Logging to "%s".', logs[ii].path);
+}
+
 var aphlict_servers = [];
 var aphlict_clients = [];
 var aphlict_admins = [];